On Thu, 2017-02-09 at 13:34 +0100, Ian Romanick wrote: > NAK. > > The way the code is currently architected, if earlier is non-NULL, > var > should not be used. There are quite a few places where the callees > do > things like 'const glsl_type *const t = (earlier == NULL) ? var->type > : > earlier->type;'. This is a bit confusing, but this change muddles > that > and makes it worse. > > I think the actual use-after-free is in setting implicitly_sized at > line > 5265. I think doing something like > > const enum ir_variable_mode *mode = earlier == NULL > ? &var->data.mode : &earlier->data.mode; > const bool implicitly_sized = > (mode == ir_var_shader_in && > state->stage >= MESA_SHADER_TESS_CTRL && > state->stage <= MESA_SHADER_GEOMETRY) || > (mode == ir_var_shader_out && > state->stage == MESA_SHADER_TESS_CTRL); > > should also fix the problem. This should be tagged for stable. >
Right. I'll do it. > After that fix, we may want to look at rearchitecting this code to be > less... confusing. The fundamental problem is > get_variable_being_redeclared is really trying to return two pieces > of > information, but it does this through a single variable. > > 1. Is this actually a redeclaration? > > 2. What is the variable that needs to be modified? > > When get_variable_being_redeclared returns NULL, it's not a > redeclaration, and the variable being modified is the ir_variable > passed > in. When it returns non-NULL, it is a redeclaration, and the > variable > being modified is the one returned. > > Maybe the function should always return non-NULL (returning either > 'earlier' or 'var') and have an extra parameter 'bool > *redeclaration'. > I think that would result in almost no changes to the second caller > and > several simplifications to the first caller. Thoughts? > Yeah, that could improve things. I am going to send two patches, one is the aforementioned fix (with Cc to mesa-stable@) and another with the refactor. Sam > On 02/07/2017 01:47 PM, Samuel Iglesias Gonsálvez wrote: > > The get_variable_being_redeclared() function can free 'var' because > > a re-declaration of an unsized array variable can establish the > > size, so > > we set the array type to the 'earlier' declaration and free 'var' > > as it is > > not needed anymore. > > > > However, the same 'var' is referenced later in > > ast_declarator_list::hir(). > > > > This patch fixes it by assign the pointer 'var' to the pointer > > 'earlier'. > > > > This error was detected by Address Sanitizer. > > > > v2: > > > > * Pointer-to-pointer assignment (Bartosz Tomczyk) > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=99677 > > Signed-off-by: Samuel Iglesias Gonsálvez <sigles...@igalia.com> > > --- > > > > Another possibility is to use reference-to-pointer but it is a C++ > > thing. IIRC, we agreed on avoiding C++-specific features to make it > > easy for C developers, but I have no strong opinion for either > > option. > > > > src/compiler/glsl/ast_to_hir.cpp | 10 ++++++---- > > 1 file changed, 6 insertions(+), 4 deletions(-) > > > > diff --git a/src/compiler/glsl/ast_to_hir.cpp > > b/src/compiler/glsl/ast_to_hir.cpp > > index b31b61d1ed6..93ba1d510fa 100644 > > --- a/src/compiler/glsl/ast_to_hir.cpp > > +++ b/src/compiler/glsl/ast_to_hir.cpp > > @@ -3958,10 +3958,12 @@ apply_type_qualifier_to_variable(const > > struct ast_type_qualifier *qual, > > * is a redeclaration, \c NULL otherwise. > > */ > > static ir_variable * > > -get_variable_being_redeclared(ir_variable *var, YYLTYPE loc, > > +get_variable_being_redeclared(ir_variable **var_pointer, YYLTYPE > > loc, > > struct _mesa_glsl_parse_state > > *state, > > bool allow_all_redeclarations) > > { > > + ir_variable *var = *var_pointer; > > + > > /* Check if this declaration is actually a re-declaration, > > either to > > * resize an array or add qualifiers to an existing variable. > > * > > @@ -3999,7 +4001,7 @@ get_variable_being_redeclared(ir_variable > > *var, YYLTYPE loc, > > > > earlier->type = var->type; > > delete var; > > - var = NULL; > > + *var_pointer = earlier; > > } else if ((state->ARB_fragment_coord_conventions_enable || > > state->is_version(150, 0)) > > && strcmp(var->name, "gl_FragCoord") == 0 > > @@ -5207,7 +5209,7 @@ ast_declarator_list::hir(exec_list > > *instructions, > > bool var_is_gl_id = is_gl_identifier(var->name); > > > > ir_variable *earlier = > > - get_variable_being_redeclared(var, decl->get_location(), > > state, > > + get_variable_being_redeclared(&var, decl->get_location(), > > state, > > false /* > > allow_all_redeclarations */); > > if (earlier != NULL) { > > if (var_is_gl_id && > > @@ -7873,7 +7875,7 @@ ast_interface_block::hir(exec_list > > *instructions, > > > > if (redeclaring_per_vertex) { > > ir_variable *earlier = > > - get_variable_being_redeclared(var, loc, state, > > + get_variable_being_redeclared(&var, loc, state, > > true /* > > allow_all_redeclarations */); > > if (!var_is_gl_id || earlier == NULL) { > > _mesa_glsl_error(&loc, state, > > > >
signature.asc
Description: This is a digitally signed message part
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
