[Mesa-dev] [Mesa-stable] [PATCH 2/4] glsl: fix use-after-free when processing array re-declarations
Timothy Arceri
t_arceri at yahoo.com.au
Wed Feb 22 23:08:45 UTC 2017
On 23/02/17 07:44, Ian Romanick wrote:
> 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...
>
It looks like the patches are still waiting on a review:
https://patchwork.freedesktop.org/series/19382/
>
> 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
>
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
More information about the mesa-dev
mailing list