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

[Kenneth Graunke]

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.

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).

--Ken
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20141106/01d2f8c2/attachment.sig>