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

[Pekka Paalanen]

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:

   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.