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

[Jan Vesely]

On Tue, 2013-10-22 at 13:38 -0700, Francisco Jerez wrote:
> Jan Vesely <jan.vesely at rutgers.edu> writes:
> 
> > 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 completely, the official Khronos OpenCL headers define all object
> IDs as pointers already, and the ICD extension requires them to be
> pointers with a dispatch table pointer located at offset 0.
> 
> That means that all implementations using an ICD loader are also going
> to crash on this test, no matter what Clover does.  Neither ocl-icd nor
> the official Khronos ICD loader bother to keep track of the valid object
> pointers -- and I don't think there's a good reason for them to, it's a
> specification requirement that implies a lot of work for almost no
> benefit.
> 
> > Not doing input checks increases distance from the specs, at least in
> > error paths, do/should we care?
> >
> I'd rather treat this as a spec bug...  It's the kind of thing that
> should really result in unspecified behavior, no real application is
> likely to rely on it, and most implementations are either ignoring it or
> not doing it reliably.
> 
> > 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.
> >
> Yeah, that would be a slightly better solution, but it wouldn't be
> enough.  We're likely to move towards an architecture where we're always
> loaded through the ICD, in that case we would crash at the ICD loader
> before Clover gets control -- If anywhere the fix probably belongs in
> the ICD loader and not in the CL implementation itself.
> 
> Also note that using RTTI as you intend wouldn't work in the general
> case, because Clover API objects don't share a common polymorphic base
> class -- and in fact they can't, because API objects need to be standard
> layout classes in order to have the guarantee that the dispatch table
> pointer will be located at the expected offset so the ICD loader can
> find it.  So, yes, I think you'd absolutely need to use a separate
> std::set for each object type, but again I don't think it's worth the
> effort.
> 
> >> 
> >> 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.
> 
> Hm, I should have said that this test makes nVidia crash 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.
> >
> Hm...  I'd like us to have a consistent behavior across all object
> types, and IMHO it's better to die with a segmentation fault when the
> user passes garbage as one of the pointer arguments than to pretend that
> everything is fine and a context has been successfully allocated in a
> non-existing platform.

OK. Thanks for detailed explanation, I have two more patches that target
error checking in clCreateBuffer, I'll post them soon.


Jan

> 
> Thanks.
> 
> > thanks,
> > Jan
> >
> >

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