[Mesa-dev] [Mesa-stable] [PATCH 2/4] glsl: fix use-after-free when processing array re-declarations

Ian Romanick idr at freedesktop.org
Wed Feb 22 20:44:55 UTC 2017


I thought we already landed a different patch for this.  I'll look through 
the archives when I get back in front of a computer...


On February 22, 2017 11:05:26 AM Nicolai Hähnle <nhaehnle at gmail.com> wrote:

> From: Nicolai Hähnle <nicolai.haehnle at amd.com>
>
> When determining whether the array is implicitly sized, we must avoid
> accessing var->data.mode (around line 5270), because var may have been
> deleted.
>
> Instead of adding yet another ternary condition based on earlier == NULL,
> refactor get_variable_being_redeclared so that we do not keep dangling
> pointers around.
>
> Found by ASAN in GL45-CTS.gtf21.GL.build.array10_frag.
>
> Cc: mesa-stable at lists.freedesktop.org
> ---
>  src/compiler/glsl/ast_to_hir.cpp | 55 ++++++++++++++++++++++------------------
>  1 file changed, 30 insertions(+), 25 deletions(-)
>
> diff --git a/src/compiler/glsl/ast_to_hir.cpp 
> b/src/compiler/glsl/ast_to_hir.cpp
> index b90ad97..57a9db9 100644
> --- a/src/compiler/glsl/ast_to_hir.cpp
> +++ b/src/compiler/glsl/ast_to_hir.cpp
> @@ -3942,44 +3942,50 @@ apply_type_qualifier_to_variable(const struct 
> ast_type_qualifier *qual,
>                         "the shared storage qualifiers can only be used with "
>                         "compute shaders");
>     }
>
>     apply_image_qualifier_to_variable(qual, var, state, loc);
>  }
>
>  /**
>   * Get the variable that is being redeclared by this declaration
>   *
> + * \p pvar is changed to point to an existing variable in the current scope if
> + * the declaration that it initially points to is a redeclaration.
> + *
> + * The declaration originally pointed to by \p pvar may be deleted.
> + *
>   * Semantic checks to verify the validity of the redeclaration are also
>   * performed.  If semantic checks fail, compilation error will be emitted via
> - * \c _mesa_glsl_error, but a non-\c NULL pointer will still be returned.
> + * \c _mesa_glsl_error, but a non-\c NULL pointer will still be provided.
>   *
>   * \returns
> - * A pointer to an existing variable in the current scope if the declaration
> - * is a redeclaration, \c NULL otherwise.
> + * Whether the declaration is a redeclaration.
>   */
> -static ir_variable *
> -get_variable_being_redeclared(ir_variable *var, YYLTYPE loc,
> +static bool
> +get_variable_being_redeclared(ir_variable **pvar, YYLTYPE loc,
>                                struct _mesa_glsl_parse_state *state,
>                                bool allow_all_redeclarations)
>  {
> +   ir_variable *var = *pvar;
> +
>     /* Check if this declaration is actually a re-declaration, either to
>      * resize an array or add qualifiers to an existing variable.
>      *
>      * This is allowed for variables in the current scope, or when at
>      * global scope (for built-ins in the implicit outer scope).
>      */
>     ir_variable *earlier = state->symbols->get_variable(var->name);
>     if (earlier == NULL ||
>         (state->current_function != NULL &&
>         !state->symbols->name_declared_this_scope(var->name))) {
> -      return NULL;
> +      return false;
>     }
>
>
>     /* From page 24 (page 30 of the PDF) of the GLSL 1.50 spec,
>      *
>      * "It is legal to declare an array without a size and then
>      *  later re-declare the same name as an array of the same
>      *  type and specify a size."
>      */
>     if (earlier->type->is_unsized_array() && var->type->is_array()
> @@ -4082,21 +4088,22 @@ get_variable_being_redeclared(ir_variable *var, 
> YYLTYPE loc,
>                            var->name);
>        } else if (earlier->type != var->type) {
>           _mesa_glsl_error(&loc, state,
>                            "redeclaration of `%s' has incorrect type",
>                            var->name);
>        }
>     } else {
>        _mesa_glsl_error(&loc, state, "`%s' redeclared", var->name);
>     }
>
> -   return earlier;
> +   *pvar = earlier;
> +   return true;
>  }
>
>  /**
>   * Generate the IR for an initializer in a variable declaration
>   */
>  ir_rvalue *
>  process_initializer(ir_variable *var, ast_declaration *decl,
>                      ast_fully_specified_type *type,
>                      exec_list *initializer_instructions,
>                      struct _mesa_glsl_parse_state *state)
> @@ -5196,56 +5203,54 @@ ast_declarator_list::hir(exec_list *instructions,
>         * list.  This list will be added to the instruction stream (below) after
>         * the declaration is added.  This is done because in some cases (such as
>         * redeclarations) the declaration may not actually be added to the
>         * instruction stream.
>         */
>        exec_list initializer_instructions;
>
>        /* Examine var name here since var may get deleted in the next call */
>        bool var_is_gl_id = is_gl_identifier(var->name);
>
> -      ir_variable *earlier =
> -         get_variable_being_redeclared(var, decl->get_location(), state,
> +      bool is_redeclaration =
> +         get_variable_being_redeclared(&var, decl->get_location(), state,
>                                         false /* allow_all_redeclarations */);
> -      if (earlier != NULL) {
> +      if (is_redeclaration) {
>           if (var_is_gl_id &&
> -             earlier->data.how_declared == ir_var_declared_in_block) {
> +             var->data.how_declared == ir_var_declared_in_block) {
>              _mesa_glsl_error(&loc, state,
>                               "`%s' has already been redeclared using "
> -                             "gl_PerVertex", earlier->name);
> +                             "gl_PerVertex", var->name);
>           }
> -         earlier->data.how_declared = ir_var_declared_normally;
> +         var->data.how_declared = ir_var_declared_normally;
>        }
>
>        if (decl->initializer != NULL) {
> -         result = process_initializer((earlier == NULL) ? var : earlier,
> -                                      decl, this->type,
> +         result = process_initializer(var, decl, this->type,
>                                        &initializer_instructions, state);
>        } else {
>           validate_array_dimensions(var_type, state, &loc);
>        }
>
>        /* From page 23 (page 29 of the PDF) of the GLSL 1.10 spec:
>         *
>         *     "It is an error to write to a const variable outside of
>         *      its declaration, so they must be initialized when
>         *      declared."
>         */
>        if (this->type->qualifier.flags.q.constant && decl->initializer == NULL) {
>           _mesa_glsl_error(& loc, state,
>                            "const declaration of `%s' must be initialized",
>                            decl->identifier);
>        }
>
>        if (state->es_shader) {
> -         const glsl_type *const t = (earlier == NULL)
> -            ? var->type : earlier->type;
> +         const glsl_type *const t = var->type;
>
>           /* Skip the unsized array check for TCS/TES/GS inputs & TCS outputs.
>            *
>            * The GL_OES_tessellation_shader spec says about inputs:
>            *
>            *    "Declaring an array size is optional. If no size is specified,
>            *     it will be taken from the implementation-dependent maximum
>            *     patch size (gl_MaxPatchVertices)."
>            *
>            * and about TCS outputs:
> @@ -5286,21 +5291,21 @@ ast_declarator_list::hir(exec_list *instructions,
>               */
>              _mesa_glsl_error(& loc, state,
>                               "unsized array declarations are not allowed in "
>                               "GLSL ES");
>        }
>
>        /* If the declaration is not a redeclaration, there are a few additional
>         * semantic checks that must be applied.  In addition, variable that was
>         * created for the declaration should be added to the IR stream.
>         */
> -      if (earlier == NULL) {
> +      if (!is_redeclaration) {
>           validate_identifier(decl->identifier, loc, state);
>
>           /* Add the variable to the symbol table.  Note that the initializer's
>            * IR was already processed earlier (though it hasn't been emitted
>            * yet), without the variable in scope.
>            *
>            * This differs from most C-like languages, but it follows the GLSL
>            * specification.  From page 28 (page 34 of the PDF) of the GLSL 1.50
>            * spec:
>            *
> @@ -7862,34 +7867,34 @@ ast_interface_block::hir(exec_list *instructions,
>              var->data.matrix_layout = fields[i].matrix_layout;
>           }
>
>           if (var->data.mode == ir_var_shader_storage)
>              apply_memory_qualifiers(var, fields[i]);
>
>           /* Examine var name here since var may get deleted in the next call */
>           bool var_is_gl_id = is_gl_identifier(var->name);
>
>           if (redeclaring_per_vertex) {
> -            ir_variable *earlier =
> -               get_variable_being_redeclared(var, loc, state,
> +            bool is_redeclaration =
> +               get_variable_being_redeclared(&var, loc, state,
>                                               true /* allow_all_redeclarations */);
> -            if (!var_is_gl_id || earlier == NULL) {
> +            if (!var_is_gl_id || !is_redeclaration) {
>                 _mesa_glsl_error(&loc, state,
>                                  "redeclaration of gl_PerVertex can only "
>                                  "include built-in variables");
> -            } else if (earlier->data.how_declared == 
> ir_var_declared_normally) {
> +            } else if (var->data.how_declared == ir_var_declared_normally) {
>                 _mesa_glsl_error(&loc, state,
>                                  "`%s' has already been redeclared",
> -                                earlier->name);
> +                                var->name);
>              } else {
> -               earlier->data.how_declared = ir_var_declared_in_block;
> -               earlier->reinit_interface_type(block_type);
> +               var->data.how_declared = ir_var_declared_in_block;
> +               var->reinit_interface_type(block_type);
>              }
>              continue;
>           }
>
>           if (state->symbols->get_variable(var->name) != NULL)
>              _mesa_glsl_error(&loc, state, "`%s' redeclared", var->name);
>
>           /* Propagate the "binding" keyword into this UBO/SSBO's fields.
>            * The UBO declaration itself doesn't get an ir_variable unless it
>            * has an instance name.  This is ugly.
> --
> 2.9.3
>
> _______________________________________________
> mesa-stable mailing list
> mesa-stable at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-stable




More information about the mesa-dev mailing list