<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Thu, Oct 27, 2016 at 1:30 PM, Eric Anholt <<a href="mailto:eric@anholt.net" target="_blank">eric@anholt.net</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">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.) </blockquote><div> </div><div>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. </div><div>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. </div><div>Tim, thoughts? </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> 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 <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> <a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> </blockquote></div> </div></div>