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

[Kenneth Graunke]

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.

For future reference, you probably want to use quick.tests.  It skips
valgrind and doesn't iterate over every single visual, so it takes a
fraction of the time but still runs basically all the tests.

> 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:
> 
>    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.
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev