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