Re: [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
> ? >data.mode : >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 
> > ---
> > 
> > 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(, decl->get_location(), 
> > state,
> > false /*
> > allow_all_redeclarations */);
> >    if (earlier != NULL) {
> >   if (var_is_gl_id &&
> > @@ -7873,7 +7875,7 @@ 

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

[Ian Romanick]

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
? >data.mode : >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.

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?

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 
> ---
> 
> 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(, 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(, loc, state,
>   true /* 
> allow_all_redeclarations */);
>  if (!var_is_gl_id || earlier == NULL) {
> _mesa_glsl_error(, state,
> 

___
mesa-dev mailing list

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

[Bartosz Tomczyk]

Patch is:
Tested-by: Bartosz Tomczyk 

I can confirm it fix use-after-free issue.

On Tue, Feb 7, 2017 at 1:47 PM, Samuel Iglesias Gonsálvez <
sigles...@igalia.com> 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 
> ---
>
> 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(, 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(, loc, state,
>   true /*
> allow_all_redeclarations */);
>  if (!var_is_gl_id || earlier == NULL) {
> _mesa_glsl_error(, state,
> --
> 2.11.0
>
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Samuel Iglesias Gonsálvez]

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 
---

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(, 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(, loc, state,
  true /* allow_all_redeclarations 
*/);
 if (!var_is_gl_id || earlier == NULL) {
_mesa_glsl_error(, state,
-- 
2.11.0

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev