[Mesa-dev] [PATCH] clover: Refuse to create context with invalid properties

[Jan Vesely]

On Mon, 2013-10-21 at 22:20 -0700, Francisco Jerez wrote:
> Jan Vesely <jan.vesely at rutgers.edu> writes:
> 
> > the specs say that clCreateContext reutrns error
> > "if platform value specified in properties is not a valid platform"
> >
> > The orignal approach fials if invalid valu other than NULL pointer is provided.
> >
> > Fixes piglit cl-api-create-context.
> >
> Honestly, I don't think this test makes much sense.  It's unreasonable
> to expect that the CL will be able to catch any bad pointer you give it
> as argument and fail gracefully.  The only reliable solution that comes
> to my mind would be to build a global hash table for each CL object type
> that keeps track of the valid objects that have been allocated.  That
> seems like a lot of effort with the only purpose of finding out if the
> user is doing something *very* stupid and very unlikely.

Hi,

My assumption was that if piglit is testing it, it should be handled,
moreover this specific case (platform id), is really simple.

I only partly agree with your statement. The specs speak about IDs, it's
implementation decision to use pointers as IDs (unless I misread
something). Not doing input checks increases distance from the specs, at
least in error paths, do/should we care?

Wouldn't std::set of all API relevant pointers/ids be enough to
implement this? then the check could look like: (set.contains() &&
RTTI), and we don't even need the NULL check.

> 
> That said, we're already doing three forms of object validation: first,
> the pointers provided by the user are compared against NULL; second, we
> make sure that the dispatch table pointer is at the correct location in
> memory; third, if the object is part of a non-trivial class hierarchy,
> as is the case for events and memory objects, we use RTTI to make sure
> that the object is of the expected type.  I don't think we want or need
> more validation, it would probably be more useful to drop that test from
> piglit.
> 
> Apparently nVidia's libOpenCL fails the test as well.

In that case, it might be better to remove the check entirely and never
touch the provided pointer.
It was the case before e5fc61fa, and unexpected success is imo better
than a segfault vector. afaik intel opencl has this behavior.

thanks,
Jan



> 
> Thanks.
> 
> > Signed-off-by: Jan Vesely <jan.vesely at rutgers.edu>
> > ---
> >  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 7b020a6..67adf8f 100644
> > --- a/src/gallium/state_trackers/clover/api/context.cpp
> > +++ b/src/gallium/state_trackers/clover/api/context.cpp
> > @@ -34,14 +34,19 @@ clCreateContext(const cl_context_properties *d_props, cl_uint num_devs,
> >                  void *user_data, cl_int *r_errcode) try {
> >     auto props = obj<property_list_tag>(d_props);
> >     auto devs = objs(d_devs, num_devs);
> > +   cl_platform_id platform;
> > +   cl_uint num_platforms;
> >  
> >     if (!pfn_notify && user_data)
> >        throw error(CL_INVALID_VALUE);
> > +   
> > +   int ret = clGetPlatformIDs(1, &platform, &num_platforms);
> > +   if (ret || !num_platforms)
> > +      throw error(CL_INVALID_PLATFORM);
> >  
> >     for (auto &prop : props) {
> > -      if (prop.first == CL_CONTEXT_PLATFORM)
> > -         obj(prop.second.as<cl_platform_id>());
> > -      else
> > +      if (prop.first != CL_CONTEXT_PLATFORM ||
> > +	  prop.second.as<cl_platform_id>() != platform)
> >           throw error(CL_INVALID_PROPERTY);
> >     }
> >  
> > -- 
> > 1.8.3.1
> >
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/mesa-dev

-- 
Jan Vesely <jan.vesely at rutgers.edu>