[Mesa-dev] Bogus bounds checking in api_validate.c?

[Ian Romanick]

On 11/06/2014 11:40 PM, Kenneth Graunke wrote:
> On Thursday, November 06, 2014 08:09:18 PM Ian Romanick wrote:
>> While working on some other things, I came across some bounds checking
>> code in _mesa_validate_DrawElements (and related functions) in
>> api_validate.c.
>>
>>       /* use indices in the buffer object */
>>       /* make sure count doesn't go outside buffer bounds */
>>       if (index_bytes(type, count) > ctx->Array.VAO->IndexBufferObj->Size) {
>>          _mesa_warning(ctx, "glDrawElements index out of buffer bounds");
>>          return GL_FALSE;
>>       }
>>
>> index_bytes calculates how many bytes of data "count" indices will
>> occupy based on the type.  The problem is that this doesn't consider
>> the base pointer.  As far as I can tell, if I had a 64 byte buffer
>> object for my index data, and I did
>>
>>     glDrawElements(GL_POINTS, 16, GL_UNSIGNED_INT, 60);
>>
>> _mesa_validate_DrawElements would say, "Ok!"
>>
>> Am I missing something, or is this just broken?
> 
> It sure seems broken to me - but, thankfully, in a conservative fashion.  (It 
> will say some invalid things are OK, but won't say legal things are invalid.)
> 
> Software drivers may be relying on this working to avoid a crash.
> 
> I checked the Ivybridge documentation, and found:
> 
> "Software is responsible for ensuring that accesses outside the IB do not
>  occur. This is possible as software can compute the range of IB values
>  referenced by a 3DPRIMITIVE command (knowing the StartVertexLocation,
>  InstanceCount, and VerticesPerInstance values) and can then compare this
>  range to the IB extent."
> 
> which makes it sound like an accurate computation is necessary.  But, right 
> below that, it says:
> 
> "this field contains the address of the last valid byte in the index buffer.
>  Any index buffer reads past this address returns an index value of 0 (as if
>  the index buffer was zero-extended)."
> 
> So the earlier statement is false; i965 will draw the in-bounds elements 
> correctly, and then repeat element 0 over and over for any out-of-bounds data, 
> resulting in one strange primitive and a lot of degenerate ones.
> 
> It's proabbly worth fixing, but I doubt it's critical either.

Hmm... I came across this while looking at cachegrind traces of GL
applications.  Time spent in _mesa_validate_Draw* was non-trivial.
Since at least some hardware doesn't need this check, I think I want to
move it down into drivers that actually need the check... which is kind
of a bummer since I came up with a clever optimization for index_bytes.

> A more interesting thing to fix, I think, would be enforcing alignment 
> restrictions (i.e. your offset has to be a multiple of the IB element size).

That would probably be useful in debug builds, but I'm pretty sure the
GL spec says the behavior is undefined specifically to avoid the check
in the hot path.

> --Ken


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: OpenPGP digital signature
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20141110/c1d9dcc6/attachment-0001.sig>