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

[Kenneth Graunke]

On 02/05/2012 05:08 PM, Roland Scheidegger wrote:
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.
Untested, found by code inspection when looking at bug 40361 (which
seems unrelated after all).
v2: As per mailing list discussion and the suggestion by Kenneth Graunke just 
drop
the questionable effort to fix up the range and revert to have no range instead.
Rearrange some (disabled) debug bits a bit for slightly less complex code.
---
  src/mesa/vbo/vbo_exec_array.c |   63 +++++++++++++++++++---------------------
  1 files changed, 30 insertions(+), 33 deletions(-)

I have to admit, I'm not a huge fan of this patch...it really does a lot 
of things at once:

1. Fix bounds checks to take BaseVertex into account
2. Work around broken apps that give us start >= _MaxElement
3. Remove 'end' clamping in favor of ignoring the range
4. Make the warnings print BaseVertex as well
5. Debugging code motion

I'd much rather see small patches that do one thing at a time, so we can 
bisect across them if we break things.

Also: I agree with Jose, the warning for end >= _MaxElement is just not 
useful.  I'd rather scrap it instead of trying to fix it up.  But we 
-do- want a warning for start >= _MaxElement, since that's clearly an 
application bug (as Jose mentioned).

I'll send out an alternate proposal (4-patch series) here in a moment. 
Let me know what you think.

diff --git a/src/mesa/vbo/vbo_exec_array.c b/src/mesa/vbo/vbo_exec_array.c
index d6b4d61..1f62e3a 100644
--- a/src/mesa/vbo/vbo_exec_array.c
+++ b/src/mesa/vbo/vbo_exec_array.c
@@ -885,17 +885,33 @@ vbo_exec_DrawRangeElementsBaseVertex(GLenum mode,
        end = MIN2(end, 0xffff);
     }

-   if (end>= ctx->Array.ArrayObj->_MaxElement) {
-      /* the max element is out of bounds of one or more enabled arrays */
+   if (0) {
+      printf("glDrawRangeElements{,BaseVertex}"
+            "(start %u, end %u, type 0x%x, count %d) ElemBuf %u, "
+            "base %d\n",
+            start, end, type, count,
+            ctx->Array.ArrayObj->ElementArrayBufferObj->Name,
+            basevertex);
+   }
+
+#if 0
+   check_draw_elements_data(ctx, count, type, indices);
+#else
+   (void) check_draw_elements_data;
+#endif
+
+   if ((int)(start + basevertex)<  0 ||
+       end + basevertex>= ctx->Array.ArrayObj->_MaxElement) {
+      /* the max (or min) 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 +929,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,34 +944,15 @@ 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...
            */
        }
-
-      /* Set 'end' to the max possible legal value */
-      assert(ctx->Array.ArrayObj->_MaxElement>= 1);
-      end = ctx->Array.ArrayObj->_MaxElement - 1;
-
-      if (end<  start) {
-         return;
-      }
-   }
-
-   if (0) {
-      printf("glDraw[Range]Elements{,BaseVertex}"
-            "(start %u, end %u, type 0x%x, count %d) ElemBuf %u, "
-            "base %d\n",
-            start, end, type, count,
-            ctx->Array.ArrayObj->ElementArrayBufferObj->Name,
-            basevertex);
+      /* range is bogus, instead of trying to fix it up just revert to no 
range */
+      vbo_validated_drawrangeelements(ctx, mode, GL_FALSE, ~0, ~0,
+                                      count, type, indices, basevertex, 1);
+      return;
     }

-#if 0
-   check_draw_elements_data(ctx, count, type, indices);
-#else
-   (void) check_draw_elements_data;
-#endif
-
     vbo_validated_drawrangeelements(ctx, mode, GL_TRUE, start, end,
                                   count, type, indices, basevertex, 1);
  }

_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev