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