[Mesa-dev] [PATCH 08/10] glsl/linker: Modify array_sizing_visitor to handle named interface blocks.

[Paul Berry]

On 8 October 2013 21:17, Jordan Justen <jljusten at gmail.com> wrote:

> On Fri, Sep 27, 2013 at 12:05 PM, Paul Berry <stereotype441 at gmail.com>
> wrote:
> > Unsized arrays appearing inside named interface blocks now get a
> > proper size assigned by the array_sizing_visitor.
> >
> > Fixes piglit tests:
> > - spec/glsl-1.50/execution/unsized-in-named-interface-block
> > - spec/glsl-1.50/execution/unsized-in-named-interface-block-gs
> > - spec/glsl-1.50/linker/unsized-in-named-interface-block
> > - spec/glsl-1.50/linker/unsized-in-named-interface-block-gs
> > - spec/glsl-1.50/linker/unsized-in-unnamed-interface-block-gs (*)
> >
> > (*) is fixed by dumb luck--support for unsized arrays in unnamed
> > interface blocks will come in a later patch.
> > ---
> >  src/glsl/ir.h       | 16 +++++++++++
> >  src/glsl/linker.cpp | 77
> ++++++++++++++++++++++++++++++++++++++++++++++++-----
> >  2 files changed, 87 insertions(+), 6 deletions(-)
> >
> > diff --git a/src/glsl/ir.h b/src/glsl/ir.h
> > index 4f63562..16bb76c 100644
> > --- a/src/glsl/ir.h
> > +++ b/src/glsl/ir.h
> > @@ -420,6 +420,22 @@ public:
> >        }
> >     }
> >
> > +   /**
> > +    * Change this->interface_type on a variable that previously had a
> > +    * different interface_type.  This is used during linking to set the
> size
> > +    * of arrays in interface blocks.
> > +    */
> > +   void change_interface_type(const struct glsl_type *type)
> > +   {
> > +      if (this->max_ifc_array_access != NULL) {
> > +         /* max_ifc_array_access has already been allocated, so make
> sure the
> > +          * new interface has the same number of fields as the old one.
> > +          */
> > +         assert(this->interface_type->length == type->length);
> > +      }
> > +      this->interface_type = type;
> > +   }
> > +
> >     const glsl_type *get_interface_type() const
> >     {
> >        return this->interface_type;
> > diff --git a/src/glsl/linker.cpp b/src/glsl/linker.cpp
> > index c54b704..fe4ccee 100644
> > --- a/src/glsl/linker.cpp
> > +++ b/src/glsl/linker.cpp
> > @@ -1035,15 +1035,80 @@ class array_sizing_visitor : public
> ir_hierarchical_visitor {
> >  public:
> >     virtual ir_visitor_status visit(ir_variable *var)
> >     {
> > -      if (var->type->is_array() && (var->type->length == 0)) {
> > -         const glsl_type *type =
> > -            glsl_type::get_array_instance(var->type->fields.array,
> > -                                          var->max_array_access + 1);
> > -         assert(type != NULL);
> > -         var->type = type;
> > +      fixup_type(&var->type, var->max_array_access);
> > +      if (var->type->is_interface()) {
> > +         if (interface_contains_unsized_arrays(var->type)) {
> > +            const glsl_type *new_type =
> > +               resize_interface_members(var->type,
> var->max_ifc_array_access);
> > +            var->type = new_type;
> > +            var->change_interface_type(new_type);
> > +         }
> > +      } else if (var->type->is_array() &&
> > +                 var->type->fields.array->is_interface()) {
> > +         if
> (interface_contains_unsized_arrays(var->type->fields.array)) {
> > +            const glsl_type *new_type =
> > +               resize_interface_members(var->type->fields.array,
> > +                                        var->max_ifc_array_access);
> > +            var->change_interface_type(new_type);
> > +            var->type =
> > +               glsl_type::get_array_instance(new_type,
> var->type->length);
> > +         }
> >        }
> >        return visit_continue;
> >     }
> > +
> > +private:
> > +   /**
> > +    * If the type pointed to by \c type represents an unsized array,
> replace
> > +    * it with a sized array whose size is determined by
> max_array_access.
> > +    */
> > +   static void fixup_type(const glsl_type **type, unsigned
> max_array_access)
> > +   {
> > +      if ((*type)->is_array() && (*type)->length == 0) {
> > +         *type = glsl_type::get_array_instance((*type)->fields.array,
> > +                                               max_array_access + 1);
> > +         assert(*type != NULL);
> > +      }
> > +   }
> > +
> > +   /**
> > +    * Determine whether the given interface type contains unsized
> arrays (if
> > +    * it doesn't, array_sizing_visitor doesn't need to process it).
> > +    */
> > +   static bool interface_contains_unsized_arrays(const glsl_type *type)
> > +   {
> > +      for (unsigned i = 0; i < type->length; i++) {
> > +         const glsl_type *elem_type = type->fields.structure[i].type;
> > +         if (elem_type->is_array() && elem_type->length == 0)
> > +            return true;
> > +      }
> > +      return false;
> > +   }
> > +
> > +   /**
> > +    * Create a new interface type based on the given type, with unsized
> arrays
> > +    * replaced by sized arrays whose size is determined by
> > +    * max_ifc_array_access.
> > +    */
> > +   static const glsl_type *
> > +   resize_interface_members(const glsl_type *type,
> > +                            const unsigned *max_ifc_array_access)
> > +   {
> > +      unsigned num_fields = type->length;
> > +      glsl_struct_field *fields = new glsl_struct_field[num_fields];
>
> Couldn't you use:
> glsl_struct_field fields[num_fields];
> to allocate on the stack, and drop the delete below?
>

Unfortunately, no.  That requires variable length arrays, which are a C99
feature that's not supported in MSVC.  Also, VLA's carry a security risk
when the size is determined by user input (as it would be in this case),
because they are typically implemented by blindly adjusting the stack
pointer to make room for the array--this means that if the array size is
too big, the program could be made to write to arbitrary memory locations,
which could in principle be used in an exploit.


>
> > +      memcpy(fields, type->fields.structure,
> > +             num_fields * sizeof(*fields));
> > +      for (unsigned i = 0; i < num_fields; i++) {
> > +         fixup_type(&fields[i].type, max_ifc_array_access[i]);
> > +      }
> > +      glsl_interface_packing packing =
> > +         (glsl_interface_packing) type->interface_packing;
> > +      const glsl_type *new_ifc_type =
> > +         glsl_type::get_interface_instance(fields, num_fields,
> > +                                           packing, type->name);
> > +      delete [] fields;
> > +      return new_ifc_type;
> > +   }
> >  };
> >
> >  /**
> > --
> > 1.8.4
> >
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20131009/3d9c6cd6/attachment-0001.html>