[Mesa-dev] [PATCH] vbo: fix possible use-after-free segfault after a VAO is deleted

[Brian Paul]

On 04/23/2013 06:21 PM, Marek Olšák wrote:
> This like the fifth attempt to fix the issue.
>
> Also with the new "validating" flag, we can set recalculate_inputs to FALSE
> earlier in vbo_bind_arrays, because _mesa_update_state won't change it.
>
> NOTE: This is a candidate for the stable branches.
> ---
>   src/mesa/vbo/vbo_exec.c       |   20 ++++++++++++++++++--
>   src/mesa/vbo/vbo_exec.h       |    1 +
>   src/mesa/vbo/vbo_exec_array.c |    8 ++++++--
>   3 files changed, 25 insertions(+), 4 deletions(-)
>
> diff --git a/src/mesa/vbo/vbo_exec.c b/src/mesa/vbo/vbo_exec.c
> index 5827f90..fd3a052 100644
> --- a/src/mesa/vbo/vbo_exec.c
> +++ b/src/mesa/vbo/vbo_exec.c
> @@ -79,10 +79,26 @@ void vbo_exec_destroy( struct gl_context *ctx )
>    */
>   void vbo_exec_invalidate_state( struct gl_context *ctx, GLuint new_state )
>   {
> -   struct vbo_exec_context *exec =&vbo_context(ctx)->exec;
> +   struct vbo_context *vbo = vbo_context(ctx);
> +   struct vbo_exec_context *exec =&vbo->exec;
>
> -   if (new_state&  (_NEW_PROGRAM|_NEW_ARRAY)) {
> +   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 affect 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)
> diff --git a/src/mesa/vbo/vbo_exec.h b/src/mesa/vbo/vbo_exec.h
> index 9fc8791..bd3ab3b 100644
> --- a/src/mesa/vbo/vbo_exec.h
> +++ b/src/mesa/vbo/vbo_exec.h
> @@ -81,6 +81,7 @@ struct vbo_exec_context
>      struct gl_context *ctx;
>      GLvertexformat vtxfmt;
>      GLvertexformat vtxfmt_noop;
> +   GLboolean validating; /**<  if we're in the middle of state validation */
>
>      struct {
>         struct gl_buffer_object *bufferobj;
> diff --git a/src/mesa/vbo/vbo_exec_array.c b/src/mesa/vbo/vbo_exec_array.c
> index 7e61f7b..1bf3af4 100644
> --- a/src/mesa/vbo/vbo_exec_array.c
> +++ b/src/mesa/vbo/vbo_exec_array.c
> @@ -501,6 +501,7 @@ vbo_bind_arrays(struct gl_context *ctx)
>
>      if (exec->array.recalculate_inputs) {
>         recalculate_input_bindings(ctx);
> +      exec->array.recalculate_inputs = GL_FALSE;
>
>         /* Again... because we may have changed the bitmask of per-vertex varying
>          * attributes.  If we regenerate the fixed-function vertex program now
> @@ -508,10 +509,13 @@ vbo_bind_arrays(struct gl_context *ctx)
>          * need in the shader.
>          */
>         if (ctx->NewState) {
> +         /* Setting "validating" to TRUE prevents _mesa_update_state from
> +          * invalidating what we just did.
> +          */
> +         exec->validating = GL_TRUE;
>            _mesa_update_state(ctx);
> +         exec->validating = GL_FALSE;
>         }
> -
> -      exec->array.recalculate_inputs = GL_FALSE;
>      }
>   }
>

The VBO code is pretty hairy stuff (I've been digging in it too) but 
this looks OK, AFAICT.

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