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

[Ian Romanick]

On 11/10/2014 08:12 PM, Timothy Arceri wrote:
> On Mon, 2014-11-10 at 06:09 -0800, 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.
> 
> I'm not sure what applications you looked at in cachegrind but OpenArena
> and Nexuiz both spend a lot of time in here. On my Ivybridge:
> 
> OpenArena 5.4% out of a total 27.94% in i965_dri.so
> Nexuiz 3.28% out of a total of 29.4% in i965_dri.so

I was looking at an apitrace of Tesseract.  It's not spending quite as
much time in these functions as OpenArena and Nexuiz, but it's still
quite a bit of time.

> For those not up to speed are you able to give a one line explanation of
> why some hardware can get away without this check?

DX10 requires that the hardware bounds-check accesses to buffer objects.
 In most cases DX10 requires that out-of-bounds reads return zero, and
out-of-bounds writes are always dropped.

>>> 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
>>
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev