[Mesa-dev] [PATCH 1/2] linker: fix strdup memory leak

[Pekka Paalanen]

On Mon, 19 Dec 2011 13:24:20 -0800
Ian Romanick <idr at freedesktop.org> wrote:

> On 12/19/2011 05:23 AM, Pekka Paalanen wrote:
> > On Fri, 16 Dec 2011 10:46:11 -0800
> > Ian Romanick<idr at freedesktop.org>  wrote:
> >
> >> On 12/14/2011 11:26 PM, Pekka Paalanen wrote:
> >>> string_to_uint_map::put() already does a strdup() for the key
> >>> argument, so we leak the memory allocated by strdup() in
> >>> link_uniforms.cpp.
> >>>
> >>> Remove the extra strdup(), fixes a few Valgrind detected leaks.
> >>
> >> Have you run piglit on this?  I seem to recall adding the extra
> >> strdup for a reason.  The hash table keeps a copy of the key
> >> pointer passed to it, and the underlying object may be deleted
> >> before the hash table is deleted.  This can happen if the back-end
> >> optimizes some uniforms away after the linker has generated the
> >> list of "active" uniforms.  I'm pretty sure there were one or two
> >> test cases that hit this on i965.
> >
> > Sorry, I didn't. Finally got piglit running, though readPixSanity
> > failed on stencil values. Anyway it's running all.tests now for the
> > upstream Mesa master, and then I'll run it with my patches.
> >
> > Oh, this is Sandybridge, btw.
> >
> >>> --- a/src/glsl/link_uniforms.cpp
> >>> +++ b/src/glsl/link_uniforms.cpp
> >>> @@ -174,8 +174,7 @@ private:
> >>>          if (this->map->get(id, name))
> >>>    	 return;
> >>>
> >>> -      char *key = strdup(name);
> >>> -      this->map->put(this->num_active_uniforms, key);
> >>> +      this->map->put(this->num_active_uniforms, name);
> >>>
> >>>          /* Each leaf uniform occupies one entry in the list of
> >>> active
> >>>           * uniforms.
> >
> > The whole visit_field() function is this:
> >
> >     virtual void visit_field(const glsl_type *type, const char
> > *name) {
> >        assert(!type->is_record());
> >        assert(!(type->is_array()&&
> > type->fields.array->is_record()));
> >
> >        /* Count the number of samplers regardless of whether the
> > uniform is
> >         * already in the hash table.  The hash table prevents
> > adding the same
> >         * uniform for multiple shader targets, but in this case we
> > want to
> >         * count it for each shader target.
> >         */
> >        const unsigned values = values_for_type(type);
> >        if (type->contains_sampler()) {
> > 	 this->num_shader_samplers +=
> > 	    type->is_array() ? type->array_size() : 1;
> >        } else {
> > 	 /* Accumulate the total number of uniform slots used by
> > this shader.
> > 	  * Note that samplers do not count against this limit
> > because they
> > 	  * don't use any storage on current hardware.
> > 	  */
> > 	 this->num_shader_uniform_components += values;
> >        }
> >
> >        /* If the uniform is already in the map, there's nothing
> > more to do. */
> >        unsigned id;
> >        if (this->map->get(id, name))
> > 	 return;
> >
> >        char *key = strdup(name);
> >        this->map->put(this->num_active_uniforms, key);
> >
> >        /* Each leaf uniform occupies one entry in the list of active
> >         * uniforms.
> >         */
> >        this->num_active_uniforms++;
> >        this->num_values += values;
> >     }
> >
> > The variable 'key' is not used anywhere else but passed to
> > string_to_uint_map::put(). That function in its whole is:
> >
> >     void put(unsigned value, const char *key)
> >     {
> >        /* The low-level hash table structure returns NULL if key is
> > not in the
> >         * hash table.  However, users of this map might want to
> > store zero as a
> >         * valid value in the table.  Bias the value by +1 so that a
> >         * user-specified zero is stored as 1.  This enables ::get
> > to tell the
> >         * difference between a user-specified zero (returned as 1 by
> >         * hash_table_find) and the key not in the table (returned
> > as 0 by
> >         * hash_table_find).
> >         *
> >         * The net effect is that we can't store UINT_MAX in the
> > table.  This is
> >         * because UINT_MAX+1 = 0.
> >         */
> >        assert(value != UINT_MAX);
> >        hash_table_replace(this->ht,
> > 			 (void *) (intptr_t) (value + 1),
> > 			 strdup(key));
> >     }
> >
> > Therefore, as far as I see, 'key' is not used for anything else than
> > passed again through strdup(), which would effectively be:
> 
> Ah yes, I forgot that I added that in string_to_uint_map::put.  You
> are quite correct.
> 
> Okay, the original patch is:
> 
> Reviewed-by: Ian Romanick <ian.d.romanick at intel.com>

You were right, it does regress in piglit.

This particular patch causes varray-disabled to fail in quick.tests.
From 4 runs of rebuilt Mesa, 3 fail in varray-disabled, and the
remaining one fails in EXT_TIMER_QUERY:time-elapsed instead.

Some of the rebuilds were from clean checkout (git clean -dxf), some
simply by adding the patch on top of a previous build that produced the
reference piglit results.

Unfortunately I have to move on for now, so I'm not pushing this patch.


Thanks.

> >     hash_table_replace(..., ..., strdup(strdup(name)));
> >
> > I don't see any case where this could not be a leak, or prevent a
> > bug.
> >
> > What do I miss? Is there somewhere another put() that overwrites or
> > overloads this version of put()?
> >
> >
> > Thanks,
> > pq
> >
> > ps. As for the other patch, thanks for the advice on how to fix it
> > properly. I don't think I can do that this year, though.