[Mesa-dev] [PATCH] mesa: fixes for DrawRangeElements[BaseVertex] index validation

[Roland Scheidegger]

Am 06.02.2012 15:39, schrieb Brian Paul:
> On 02/04/2012 06:14 AM, Jose Fonseca wrote:
>>
>>
>> ----- Original Message -----
>>> On 01/27/2012 06:00 PM, Roland Scheidegger wrote:
>>>> in check_index_bounds the comparison needs to be "greater equal"
>>>> since
>>>> contrary to the name _MaxElement is the count of the array.
>>>> In vbo_exec_DrawRangeElementsBaseVertex, take into account the
>>>> basevertex.
>>>> As far as I can tell it is completely ok (though maybe stupid) to
>>>> have
>>>> start/end of 100/199, with _MaxElement being 100, if the basevertex
>>>> is -100 (since the start/end are prior to adding basevertex). The
>>>> opposite
>>>> is also true (0/99 is not ok if _MaxElement is 100 and and
>>>> basevertex is 100).
>>>> Previously we would have issued a warning in such cases and worse
>>>> probably
>>>> "fixed" end to a bogus value.
>>>> Totally untested, found by code inspection when looking at bug
>>>> 40361 (which
>>>> seems unrelated after all).
>>>> ---
>>>>    src/mesa/main/api_validate.c  |    2 +-
>>>>    src/mesa/vbo/vbo_exec_array.c |   30
>>>>    +++++++++++++++++++-----------
>>>>    2 files changed, 20 insertions(+), 12 deletions(-)
>>>>
>>>> diff --git a/src/mesa/main/api_validate.c
>>>> b/src/mesa/main/api_validate.c
>>>> index b6871d0..1ae491f 100644
>>>> --- a/src/mesa/main/api_validate.c
>>>> +++ b/src/mesa/main/api_validate.c
>>>> @@ -187,7 +187,7 @@ check_index_bounds(struct gl_context *ctx,
>>>> GLsizei count, GLenum type,
>>>>       vbo_get_minmax_indices(ctx,&prim,&ib,&min,&max, 1);
>>>>
>>>>       if ((int)(min + basevertex)<   0 ||
>>>> -       max + basevertex>   ctx->Array.ArrayObj->_MaxElement) {
>>>> +       max + basevertex>= ctx->Array.ArrayObj->_MaxElement) {
>>>>          /* the max element is out of bounds of one or more enabled
>>>>          arrays */
>>>>          _mesa_warning(ctx, "glDrawElements() index=%u is out of
>>>>          bounds (max=%u)",
>>>>                        max, ctx->Array.ArrayObj->_MaxElement);
>>>
>>> I might split this hunk out into a separate patch.  This hunk is:
>>> Reviewed-by: Kenneth Graunke<kenneth at whitecape.org>
>>>
>>> And yeah, _MaxElement does make me think "index of the last element"
>>> (N-1), not "the total number of elements" (N).  I'm not a fan of the
>>> name.
> 
> If someone wants to rename _MaxElement or change its definition, that's
> OK with me.
> 
> 
> [...]
> 
>>> I don't think we want to include basevertex when adjusting 'start'
>>> and
>>> 'end'.  The final call to vbo_validated_drawrangeelements already
>>> passes
>>> basevertex, so I'm pretty sure this will adjust it twice.
>>>
>>> Also, I just sent out a patch (vbo: Ignore invalid element ranges if
>>> fixing 'end' breaks 'start') and realized it'll conflict here.
>>>
>>> I'm leaning toward dropping this code that clamps "end" altogether.
>>> It's
>>> awfully dodgy already, and trying to correctly compensate for
>>> basevertex
>>> is pretty much destroying any faith I had left in it working
>>> properly.
>>>
>>> After all, glDrawRangeElementsBaseVertex is just an optimized version
>>> of
>>> glDrawElementsBaseVertex, so it should be totally safe to just drop
>>> the
>>> range entirely.  Considering that we've already proven the range is
>>> insane, that seems like the safest option.  Plus...optimizing broken
>>> calls?  Really?
>>
>> I'm OK with passing the ranges unmodified to the drivers. And let them
>> handle them. But the drivers (or the hardware) needs to be robust
>> against fetching vertices beyond the vertex buffer's size. I'm not
>> sure if they all do that.
> 
> In addition to debugging, the other reason I added the _MaxElement stuff
> was to try to catch out-of-bounds accesses and prevent segfaults, in
> particular for when Mesa is running in the X server. Otherwise, one
> could bring down the X server with one trivial glDrawElements call.
> 

Within the X server it should still check the actual indices in
check_index_bounds according to Const.CheckArrayBounds - which will scan
the actual index buffer and not care about the range given by
DrawRangeElements one bit.
The range given by DrawRangeElements doesn't really tell you much we
can't validate it (without scanning the index buffer) other than catch
the trivial out-of-bound case so the burden to not blow up if index
elements are outside the given range is already on the driver in any
case (though maybe even blowing up is ok as the spec says it causes
"implementation dependent behavior").
Arguably though in the case when we do actually scan the index buffer we
could trivially check if the indices were inside the given range,
instead of only check if they aren't outside the arrays, it is an error
after all.

Roland