[Mesa-dev] [Mesa-stable] [PATCH 2/4] glsl: fix use-after-free when processing array re-declarations
Ian Romanick
idr at freedesktop.org
Thu Feb 23 01:40:37 UTC 2017
On 02/22/2017 03:08 PM, Timothy Arceri wrote:
> 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/
D'oh... okay, I've fixed that. Sorry. :(
>> 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