[Mesa-dev] [PATCH] mesa: fix crash in st/mesa after deleting a VAO

[Brian Paul]

On 07/10/2014 05:11 PM, Marek Olšák wrote:
> From: Marek Olšák <marek.olsak at amd.com>
>
> This happens when glGetMultisamplefv (or any other non-draw function) is
> called, which doesn't invoke the VBO module to update _DrawArrays and
> the pointer is invalid at that point.
>
> However st/mesa still dereferences it to setup vertex buffers ==> crash.
> ---
>   src/mesa/main/arrayobj.c   | 15 +++++++++++++++
>   src/mesa/main/mtypes.h     | 14 ++++++++++++++
>   src/mesa/vbo/vbo_context.h | 22 ++++------------------
>   src/mesa/vbo/vbo_exec.c    | 15 ---------------
>   4 files changed, 33 insertions(+), 33 deletions(-)
>
> diff --git a/src/mesa/main/arrayobj.c b/src/mesa/main/arrayobj.c
> index efb9930..0f8f827 100644
> --- a/src/mesa/main/arrayobj.c
> +++ b/src/mesa/main/arrayobj.c
> @@ -427,6 +427,21 @@ bind_vertex_array(struct gl_context *ctx, GLuint id, GLboolean genRequired)
>         }
>      }
>
> +   if (ctx->Array.DrawMethod == DRAW_ARRAYS) {
> +      /* The _DrawArrays pointer is pointing at the VAO being unbound and
> +       * that VAO may be in the process if being deleted. If it's not going

s/if/of/


> +       * to be deleted, this will have no effect, because the pointer needs
> +       * to be updated by the VBO module anyway.
> +       *
> +       * Before the VBO module can update the pointer, we have to set it
> +       * to NULL for drivers not to set up arrays which are not bound,
> +       * or to prevent a crash if the VAO being unbound is going to be
> +       * deleted.
> +       */
> +      ctx->Array._DrawArrays = NULL;
> +      ctx->Array.DrawMethod = DRAW_NONE;
> +   }
> +
>      ctx->NewState |= _NEW_ARRAY;
>      _mesa_reference_vao(ctx, &ctx->Array.VAO, newObj);
>
> diff --git a/src/mesa/main/mtypes.h b/src/mesa/main/mtypes.h
> index a7126fd..b699021 100644
> --- a/src/mesa/main/mtypes.h
> +++ b/src/mesa/main/mtypes.h
> @@ -1640,6 +1640,17 @@ struct gl_vertex_array_object
>   };
>
>
> +/** Used to signal when transitioning from one kind of drawing method
> + * to another.
> + */
> +typedef enum {
> +   DRAW_NONE,          /**< Initial value only */
> +   DRAW_BEGIN_END,
> +   DRAW_DISPLAY_LIST,
> +   DRAW_ARRAYS
> +} gl_draw_method;
> +
> +
>   /**
>    * Vertex array state
>    */
> @@ -1679,6 +1690,9 @@ struct gl_array_attrib
>       * The array pointer is set up only by the VBO module.
>       */
>      const struct gl_client_array **_DrawArrays; /**< 0..VERT_ATTRIB_MAX-1 */
> +
> +   /** One of the DRAW_xxx flags, not consumed by drivers */
> +   gl_draw_method DrawMethod;
>   };
>
>
> diff --git a/src/mesa/vbo/vbo_context.h b/src/mesa/vbo/vbo_context.h
> index 1680e23..e224513 100644
> --- a/src/mesa/vbo/vbo_context.h
> +++ b/src/mesa/vbo/vbo_context.h
> @@ -57,18 +57,6 @@
>   #include "vbo_save.h"
>
>
> -/** Used to signal when transitioning from one kind of drawing method
> - * to another.
> - */
> -enum draw_method
> -{
> -   DRAW_NONE,          /**< Initial value only */
> -   DRAW_BEGIN_END,
> -   DRAW_DISPLAY_LIST,
> -   DRAW_ARRAYS
> -};
> -
> -
>   struct vbo_context {
>      struct gl_client_array currval[VBO_ATTRIB_MAX];
>
> @@ -83,8 +71,6 @@ struct vbo_context {
>       * is responsible for initiating any fallback actions required:
>       */
>      vbo_draw_func draw_prims;
> -
> -   enum draw_method last_draw_method;
>   };
>
>
> @@ -122,11 +108,11 @@ get_program_mode( struct gl_context *ctx )
>    * that arrays may be changing.
>    */
>   static inline void
> -vbo_draw_method(struct vbo_context *vbo, enum draw_method method)
> +vbo_draw_method(struct vbo_context *vbo, gl_draw_method method)
>   {
> -   if (vbo->last_draw_method != method) {
> -      struct gl_context *ctx = vbo->exec.ctx;
> +   struct gl_context *ctx = vbo->exec.ctx;
>
> +   if (ctx->Array.DrawMethod != method) {
>         switch (method) {
>         case DRAW_ARRAYS:
>            ctx->Array._DrawArrays = vbo->exec.array.inputs;
> @@ -142,7 +128,7 @@ vbo_draw_method(struct vbo_context *vbo, enum draw_method method)
>         }
>
>         ctx->NewDriverState |= ctx->DriverFlags.NewArray;
> -      vbo->last_draw_method = method;
> +      ctx->Array.DrawMethod = method;
>      }
>   }
>
> diff --git a/src/mesa/vbo/vbo_exec.c b/src/mesa/vbo/vbo_exec.c
> index bd2b1b1..eb90350 100644
> --- a/src/mesa/vbo/vbo_exec.c
> +++ b/src/mesa/vbo/vbo_exec.c
> @@ -82,21 +82,6 @@ void vbo_exec_invalidate_state( struct gl_context *ctx, GLuint new_state )
>
>      if (!exec->validating && new_state & (_NEW_PROGRAM|_NEW_ARRAY)) {
>         exec->array.recalculate_inputs = GL_TRUE;
> -
> -      /* If we ended up here because a VAO was deleted, the _DrawArrays
> -       * pointer which pointed to the VAO might be invalid now, so set it
> -       * to NULL.  This prevents crashes in driver functions like Clear
> -       * where driver state validation might occur, but the vbo module is
> -       * still in an invalid state.
> -       *
> -       * Drivers should skip vertex array state validation if _DrawArrays
> -       * is NULL.  It also has no effect on performance, because attrib
> -       * bindings will be recalculated anyway.
> -       */
> -      if (vbo->last_draw_method == DRAW_ARRAYS) {
> -         ctx->Array._DrawArrays = NULL;
> -         vbo->last_draw_method = DRAW_NONE;
> -      }
>      }
>
>      if (new_state & _NEW_EVAL)
>

Reviewed-by: Brian Paul <brianp at vmware.com>