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

[Ian Romanick]

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>

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