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

[Jason Ekstrand]

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?


>  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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20161027/1d9df626/attachment-0001.html>