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

[Ian Romanick]

On 11/10/2014 06:09 AM, Ian Romanick wrote:
> 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.

You may be right, actually.  Section 6.2 (Creating and Modifying Buffer
Object Data Stores) of the 4.5 spec says:

    "Clients must align data elements consistent with the requirements
    of the client platform, with an additional base-level requirement
    that an offset within a buffer to a datum comprising N basic
    machine units be a multiple of N."

Of course, it doesn't say what to do.  I don't see anything in this
section or in section 2.3.1 (Errors) that suggests an error to generate.
 Just ignore the command?  Halt and catch fire?

We have some lovely re-base code in brw_upload_indices that maybe we can
delete.

>> --Ken
> 
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev