[Mesa-dev] [PATCH 2/2] nir: Fix clone/sweep of info allocated by nir_builder_init_simple_shader().

[Timothy Arceri]

On Thu, 2016-10-27 at 13:57 -0700, Jason Ekstrand wrote:
> On Thu, Oct 27, 2016 at 1:30 PM, Eric Anholt <eric at anholt.net> wrote:
> > Fixes use-after-free-caused segfaults in the tgsi_to_nir() path.
> > ---
> > 
> > I don't like the boolean I added here, any better plans, though?  I
> > haven't fully piglited this, but it's enough to get gears to not
> > crash
> > on master (it at least touches the GLSL to NIR and TGSI to NIR
> > paths.)
> 
> I don't either. Let's consider the options...
> 
> Would it be better to simply make nir_shader_create() always allocate
> a shader_info and just memcpy old one if provided or zero it out if
> no shader info is provided?  Having a pointer which may or may not be
> parented to the nir_shader is a disaster waiting to happen.  Since
> the one provided to nir_shader_create() isn't required to be
> ralloc'd, we can't even use ralloc_parent() to check.
> 
> Another option would be to say that the NIR shader doesn't own the
> info ever. Maybe even get rid of the shader_info pointer in
> nir_shader.  It would probably mean manually passing the shader_info
> around a few more places, but it would work.
> 
> Tim, thoughts?

Hi guys,

Options:

1) Make nir_shader_create() always allocate a shader_info and just
memcpy old one if provided or zero it out if no shader info is
provided.

2) Detach shader_info from nir_shader. Downside we need to pass both
around. This looks like it will be most painful in Vulkan were
nir_shader seems to be the highest level stucture we pass around and we
don't have something like gl_program in GLSL where we can get access to
them both.

3) Allocate the info to attach to the shader being produced, and then
assert that nir_shader_create() always gets an info.

4) Switch the order. Have shader_info (we could rename it if that would
help make more sense) contain a pointer to nir_shader. This is how we
do things in GLSL IR, gl_program already contains a pointer to nir.
This way the shader_info is created and managed by the implementation
rather than nir.

Personally I think I like option 4. I suggested this originally but
Jason wasn't sold at the time. However looking at the problem again
this make the most sense to me.

What do you guys think?    

>  
> >  src/compiler/nir/nir.c       |  2 ++
> >  src/compiler/nir/nir.h       |  2 ++
> >  src/compiler/nir/nir_clone.c | 14 +++++++++++++-
> >  src/compiler/nir/nir_sweep.c |  3 +++
> >  4 files changed, 20 insertions(+), 1 deletion(-)
> > 
> > diff --git a/src/compiler/nir/nir.c b/src/compiler/nir/nir.c
> > index 09aad57e87f8..1448837d8fd1 100644
> > --- a/src/compiler/nir/nir.c
> > +++ b/src/compiler/nir/nir.c
> > @@ -45,6 +45,8 @@ nir_shader_create(void *mem_ctx,
> >     shader->options = options;
> > 
> >     shader->info = si ? si : rzalloc(shader, shader_info);
> > +   if (!si)
> > +      shader->shader_owns_info = true;
> > 
> >     exec_list_make_empty(&shader->functions);
> >     exec_list_make_empty(&shader->registers);
> > diff --git a/src/compiler/nir/nir.h b/src/compiler/nir/nir.h
> > index 92647632462b..7dde8bd4ebd6 100644
> > --- a/src/compiler/nir/nir.h
> > +++ b/src/compiler/nir/nir.h
> > @@ -1807,6 +1807,8 @@ typedef struct nir_shader {
> >     /** Various bits of compile-time information about a given
> > shader */
> >     struct shader_info *info;
> > 
> > +   bool shader_owns_info;
> > +
> >     /** list of global variables in the shader (nir_variable) */
> >     struct exec_list globals;
> > 
> > diff --git a/src/compiler/nir/nir_clone.c
> > b/src/compiler/nir/nir_clone.c
> > index 9f3bd7ff6baa..cd6063325274 100644
> > --- a/src/compiler/nir/nir_clone.c
> > +++ b/src/compiler/nir/nir_clone.c
> > @@ -710,7 +710,19 @@ nir_shader_clone(void *mem_ctx, const
> > nir_shader *s)
> >     clone_reg_list(&state, &ns->registers, &s->registers);
> >     ns->reg_alloc = s->reg_alloc;
> > 
> > -   ns->info = s->info;
> > +   if (s->shader_owns_info) {
> > +      ns->info = ralloc(ns, struct shader_info);
> > +      *ns->info = *s->info;
> > +      ns->shader_owns_info = true;
> > +   } else {
> > +      ns->info = s->info;
> > +   }
> > +
> > +   /* XXX: Note that for !shader_owns_info, we're making the info
> > pointed to
> > +    * by s have data owned by ns, so if ns is freed before s then
> > we might
> > +    * use-after-free the name/label.  We should probably have
> > name/label be
> > +    * owned by the info.
> > +    */
> >     ns->info->name = ralloc_strdup(ns, ns->info->name);
> >     if (ns->info->label)
> >        ns->info->label = ralloc_strdup(ns, ns->info->label);
> > diff --git a/src/compiler/nir/nir_sweep.c
> > b/src/compiler/nir/nir_sweep.c
> > index faf696d6decd..43664d4d5b96 100644
> > --- a/src/compiler/nir/nir_sweep.c
> > +++ b/src/compiler/nir/nir_sweep.c
> > @@ -153,6 +153,9 @@ nir_sweep(nir_shader *nir)
> >     /* First, move ownership of all the memory to a temporary
> > context; assume dead. */
> >     ralloc_adopt(rubbish, nir);
> > 
> > +   if (nir->shader_owns_info)
> > +      ralloc_steal(nir, nir->info);
> > +
> >     ralloc_steal(nir, (char *)nir->info->name);
> >     if (nir->info->label)
> >        ralloc_steal(nir, (char *)nir->info->label);
> > --
> > 2.10.1
> > 
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> > 
> 
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev