On Thu, 2016-10-27 at 13:57 -0700, Jason Ekstrand wrote:
> On Thu, Oct 27, 2016 at 1:30 PM, Eric Anholt <e...@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@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 mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
