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

[Roland Scheidegger]

Ping? I don't want to commit that without anyone looking at it.

Am 06.02.2012 02:08, schrieb Roland Scheidegger:
> 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(-)
> 
> 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);
>  }