[Mesa-dev] [PATCH] mesa: fixes for DrawRangeElements[BaseVertex] index validation
Roland Scheidegger
sroland at vmware.com
Sun Feb 5 15:45:06 PST 2012
Sorry didn't see this response previously, threading didn't seem to work.
Am 04.02.2012 12:40, schrieb Kenneth Graunke:
> 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.
Ok I'll commit this.
>
>> diff --git a/src/mesa/vbo/vbo_exec_array.c
>> b/src/mesa/vbo/vbo_exec_array.c
>> index 9861b21..3e0fe20 100644
>> --- a/src/mesa/vbo/vbo_exec_array.c
>> +++ b/src/mesa/vbo/vbo_exec_array.c
>> @@ -885,17 +885,18 @@ vbo_exec_DrawRangeElementsBaseVertex(GLenum mode,
>> end = MIN2(end, 0xffff);
>> }
>>
>> - if (end>= ctx->Array.ArrayObj->_MaxElement) {
>> + if ((int)(start + basevertex)< 0 ||
>> + end + basevertex>= ctx->Array.ArrayObj->_MaxElement) {
>> /* the max element is out of bounds of one or more enabled
>> arrays */
>> warnCount++;
>>
>> if (warnCount< 10) {
>> - _mesa_warning(ctx, "glDraw[Range]Elements(start %u, end %u,
>> count %d, "
>> - "type 0x%x, indices=%p)\n"
>> + _mesa_warning(ctx, "glDrawRangeElements[BaseVertex](start
>> %u, end %u, count %d, "
>> + "type 0x%x, indices=%p, base %d)\n"
>> "\tend is out of bounds (max=%u) "
>> "Element Buffer %u (size %d)\n"
>> "\tThis should probably be fixed in the
>> application.",
>> - start, end, count, type, indices,
>> + start, end, count, type, indices, basevertex,
>> ctx->Array.ArrayObj->_MaxElement - 1,
>>
>> ctx->Array.ArrayObj->ElementArrayBufferObj->Name,
>> (int)
>> ctx->Array.ArrayObj->ElementArrayBufferObj->Size);
>> @@ -913,14 +914,14 @@ vbo_exec_DrawRangeElementsBaseVertex(GLenum mode,
>> if (0) {
>> GLuint max = _mesa_max_buffer_index(ctx, count, type, indices,
>>
>> ctx->Array.ArrayObj->ElementArrayBufferObj);
>> - if (max>= ctx->Array.ArrayObj->_MaxElement) {
>> + if (max + basevertex>= ctx->Array.ArrayObj->_MaxElement) {
>> if (warnCount< 10) {
>> - _mesa_warning(ctx, "glDraw[Range]Elements(start %u,
>> end %u, "
>> - "count %d, type 0x%x, indices=%p)\n"
>> + _mesa_warning(ctx,
>> "glDrawRangeElements[BaseVertex](start %u, end %u, "
>> + "count %d, type 0x%x, indices=%p, base
>> %d)\n"
>> "\tindex=%u is out of bounds (max=%u) "
>> "Element Buffer %u (size %d)\n"
>> "\tSkipping the glDrawRangeElements()
>> call",
>> - start, end, count, type, indices, max,
>> + start, end, count, type, indices,
>> basevertex, max,
>> ctx->Array.ArrayObj->_MaxElement - 1,
>>
>> ctx->Array.ArrayObj->ElementArrayBufferObj->Name,
>> (int)
>> ctx->Array.ArrayObj->ElementArrayBufferObj->Size);
>> @@ -928,13 +929,20 @@ vbo_exec_DrawRangeElementsBaseVertex(GLenum mode,
>> }
>> /* XXX we could also find the min index and compare to 'start'
>> * to see if start is correct. But it's more likely to get
>> the
>> - * upper bound wrong.
>> + * upper bound wrong, at least for the non-BaseVertex
>> variant...
>> */
>> }
>
> Ugh, this is hard to think through...
>
> If basevertex < 0, end + basevertex might actually be in the array while
> 'end' would not. We currently clamp in this case, and I think I agree
> with you that we shouldn't.
>
> If basevertex > 0, end might be within the array yet end + basevertex
> might not. So, this would make us clamp in a case where we currently
> don't.
>
> Page 33 of the OpenGL 3.2 Core spec mentions:
> "For DrawRangeElementsBaseVertex, the index values must lie between
> start and end inclusive, prior to adding the basevertex offset."
>
> which seems relevant, but probably isn't. This is about clamping 'end'
> itself to avoid array overflow, not the index values themselves.
> Since we'll be adding basevertex later, end + basevertex would
> indeed be outside of the array, which is bad.
>
> So...I think this is correct...and worth fixing...
This was my understanding of the spec as well. I'd not be totally
surprised if some apps would get the range wrong with BaseVertex but if
we just drop the range in such cases as suggested below nothing bad
should happen in any case.
>
>> /* Set 'end' to the max possible legal value */
>> assert(ctx->Array.ArrayObj->_MaxElement>= 1);
>> - end = ctx->Array.ArrayObj->_MaxElement - 1;
>> + /* XXX Could have positive uint overflow for both start and end
>> too. */
>> + if ((int)(ctx->Array.ArrayObj->_MaxElement - 1 - basevertex)<
>> 0) {
>> + return;
>> + }
>> + end = ctx->Array.ArrayObj->_MaxElement - 1 - basevertex;
>> + if ((int)(start + basevertex)< 0) {
>> + start = -basevertex;
>> + }
>>
>> if (end< start) {
>> return;
>
> 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.
I don't think that's the case. This just sets the end so that after
adding basevertex back it would point to the last array entry -
basically the opposite what was done above to check the range is within
array bounds. If the interpretation of the spec above is right, this
must be true too. But it might indeed not make much sense trying to
adjust one end of the range while keeping the other. (Maybe should check
for start the same way as for end though in light of BaseVertex.)
>
> 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?
That is fine by me (it is really very dodgy indeed). Drivers which will
die if indices are outside some bounds do so anyway as we don't actually
scan the indices.
Roland
More information about the mesa-dev
mailing list