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

Nicolai Hähnle nhaehnle at gmail.com
Wed Feb 22 19:04:38 UTC 2017


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



More information about the mesa-dev mailing list