[Mesa-dev] [PATCH 1/4] mesa/main: fix MultiDrawElements[BaseVertex] validation of primcount

[Ian Romanick]

On 02/22/2017 11:04 AM, Nicolai Hähnle wrote:
> From: Nicolai Hähnle <nicolai.haehnle at amd.com>
> 
> primcount must be a GLsizei as in the signature for MultiDrawElements
> or bad things can happen.
> 
> Furthermore, an error should be flagged when primcount is negative
> (plus there is an argument about whether it should be flagged when
> primcount is 0, but let's worry about that separately).
> 
> Curiously, this code used to work somewhat correctly even when primcount
> was negative, because the loop that checks count[i] would iterate out of
> bounds and almost certainly hit a negative value at some point.
> 
> Found by an ASAN error in
> GL45-CTS.gtf32.GL3Tests.draw_elements_base_vertex.draw_elements_base_vertex_primcount
> 
> Note that the OpenGL spec seems to have s/primcount/drawcount/ at some
> point, and the code still reflects the old language.
> 
> Cc: mesa-stable at lists.freedesktop.org
> ---
>  src/mesa/main/api_validate.c | 41 +++++++++++++++++++++++++++++++++++++++--
>  src/mesa/main/api_validate.h |  2 +-
>  2 files changed, 40 insertions(+), 3 deletions(-)
> 
> diff --git a/src/mesa/main/api_validate.c b/src/mesa/main/api_validate.c
> index 1e8a714..5b05301 100644
> --- a/src/mesa/main/api_validate.c
> +++ b/src/mesa/main/api_validate.c
> @@ -716,26 +716,63 @@ _mesa_validate_DrawElements(struct gl_context *ctx,
>  
>  /**
>   * Error checking for glMultiDrawElements().  Includes parameter checking
>   * and VBO bounds checking.
>   * \return GL_TRUE if OK to render, GL_FALSE if error found
>   */
>  GLboolean
>  _mesa_validate_MultiDrawElements(struct gl_context *ctx,
>                                   GLenum mode, const GLsizei *count,
>                                   GLenum type, const GLvoid * const *indices,
> -                                 GLuint primcount)
> +                                 GLsizei primcount)
>  {
> -   unsigned i;
> +   GLsizei i;
>  
>     FLUSH_CURRENT(ctx, 0);
>  
> +   /*
> +    * Section 10.4 (Drawing Commands Using Vertex Arrays) of the OpenGL 4.5
> +    * (Compatibility Profile) spec defines MultiDrawElements as equivalent to:
> +    *
> +    *    if (mode, drawcount, or type is invalid)
> +    *       generate appropriate error
> +    *    else {
> +    *       for (int i = 0; i < drawcount; i++)
> +    *          DrawElementsOneInstance(mode, count[i], type,
> +    *                                  indices[i], 0, 0, 0);
> +    *    }
> +    *
> +    * However, it does not say when drawcount is invalid or what the
> +    * appropriate error would be.
> +    *
> +    * The same section does define the error condition for the similar
> +    * MultiDrawElementsIndirect:
> +    *
> +    *    "An INVALID_VALUE error is generated if drawcount is not positive."

There is blanket coverage for this.

Section 2.3.1 (Errors) of the OpenGL 4.5 Core Profile spec says:

   If a negative number is provided where an argument of type sizei or
   sizeiptr is specified, an INVALID_VALUE error is generated.

> +    *
> +    * It would make sense to apply the same condition here, but
> +    *
> +    * (1) this language was only introduced together with the *Indirect() draw
> +    *     calls and is quite far removed textually, and
> +    *
> +    * (2) GL45-CTS.gtf32.GL3Tests.draw_elements_base_vertex.draw_elements_base_vertex_primcount
> +    *     expects to get an error iff the value is negative.
> +    *
> +    * This looks like a gap in the spec, but until that is resolved, let's just
> +    * follow what the CTS expects.

The CTS is also expecting this (from the same section of the spec):

   In other cases, there are no side effects unless otherwise noted;
   the command which generates the error is ignored so that it has no
   effect on GL state or framebuffer contents.

If some count[i] would generate an error, the whole command must have no
side effects.

With the comment changed to reference the spec quotes, this patch is

Reviewed-by: Ian Romanick <ian.d.romanick at intel.com>

> +    */
> +   if (primcount < 0) {
> +      _mesa_error(ctx, GL_INVALID_VALUE,
> +                  "glMultiDrawElements(primcount=%d)", primcount);
> +      return GL_FALSE;
> +   }
> +
>     for (i = 0; i < primcount; i++) {
>        if (count[i] < 0) {
>           _mesa_error(ctx, GL_INVALID_VALUE,
>                       "glMultiDrawElements(count)" );
>           return GL_FALSE;
>        }
>     }
>  
>     if (!_mesa_valid_prim_mode(ctx, mode, "glMultiDrawElements")) {
>        return GL_FALSE;
> diff --git a/src/mesa/main/api_validate.h b/src/mesa/main/api_validate.h
> index e94f02e..de520c9 100644
> --- a/src/mesa/main/api_validate.h
> +++ b/src/mesa/main/api_validate.h
> @@ -50,21 +50,21 @@ _mesa_validate_DrawArrays(struct gl_context *ctx, GLenum mode, GLsizei count);
>  
>  extern GLboolean
>  _mesa_validate_DrawElements(struct gl_context *ctx,
>  			    GLenum mode, GLsizei count, GLenum type,
>  			    const GLvoid *indices);
>  
>  extern GLboolean
>  _mesa_validate_MultiDrawElements(struct gl_context *ctx,
>                                   GLenum mode, const GLsizei *count,
>                                   GLenum type, const GLvoid * const *indices,
> -                                 GLuint primcount);
> +                                 GLsizei primcount);
>  
>  extern GLboolean
>  _mesa_validate_DrawRangeElements(struct gl_context *ctx, GLenum mode,
>  				 GLuint start, GLuint end,
>  				 GLsizei count, GLenum type,
>  				 const GLvoid *indices);
>  
>  
>  extern GLboolean
>  _mesa_validate_DrawArraysInstanced(struct gl_context *ctx, GLenum mode, GLint first,
>