[Mesa-dev] [PATCH] clover: fix clCreateContext Piglit test crash

[Francisco Jerez]

EdB <edb+mesa at sigluy.net> writes:

> clCreateContext no longer crash when CL_CONTEXT_PLATFORM is invalid

NAK.  That piglit test is rather dubious, can we please get rid of it?

Passing pointers to inaccessible memory is likely to cause a segfault on
other implementations too, like nVidia's, Intel's or any implementation
using the official Khronos ICD loader -- And I don't see why that's such
a big deal, SEGFAULT is just the way your OS has to tell you that you
don't have the right to dereference that address, which is precisely
what's going on here.

Doing these invalid pointer checks consistently for other object types
would imply lots of ugly and fragile book-keeping, which when using the
ICD would become completely useless since the crash would then happen in
the loader before we have the chance to do any argument validation.

I've seen some discussion on the Khronos bugzilla regarding handle
validation on the ICD loader.  The conclusion seemed to be that full
validation would be too costly and provide little benefit, a minimal
NULL check should be sufficient -- which is less than Clover is already
doing.

> ---
>  src/gallium/state_trackers/clover/api/context.cpp | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/src/gallium/state_trackers/clover/api/context.cpp b/src/gallium/state_trackers/clover/api/context.cpp
> index 021eea3..749d2d7 100644
> --- a/src/gallium/state_trackers/clover/api/context.cpp
> +++ b/src/gallium/state_trackers/clover/api/context.cpp
> @@ -39,10 +39,15 @@ clCreateContext(const cl_context_properties *d_props, cl_uint num_devs,
>        throw error(CL_INVALID_VALUE);
>  
>     for (auto &prop : props) {
> -      if (prop.first == CL_CONTEXT_PLATFORM)
> -         obj(prop.second.as<cl_platform_id>());
> -      else
> +      if (prop.first == CL_CONTEXT_PLATFORM) {
> +         //clover only have one platform
> +         cl_platform_id d_platform;
> +         cl_int ret = clGetPlatformIDs(1, &d_platform, NULL);
> +         if (ret || (prop.second.as<cl_platform_id>() != d_platform))
> +            throw error(CL_INVALID_PLATFORM);
> +      } else {
>           throw error(CL_INVALID_PROPERTY);
> +      }
>     }
>  
>     ret_error(r_errcode, CL_SUCCESS);
> -- 
> 1.9.3
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 212 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20141112/94b945cc/attachment-0001.sig>