[Mesa-dev] [PATCH v2] glsl: fix heap-use-after-free in ast_declarator_list::hir()

[Samuel Iglesias Gonsálvez]

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 <siglesias at 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,
> > 
> 
> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170209/df3e02ff/attachment-0001.sig>