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

[Francisco Jerez]

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.

Thanks.

> thanks,
> Jan
>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 229 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20131022/98a3d2d2/attachment.pgp>