[Mesa-dev] [PATCH 2/2] mesa: free gl_uniform_storage::name

[Ian Romanick]

On 12/14/2011 11:26 PM, Pekka Paalanen wrote:
> parcel_out_uniform_storage::visit_field() assigns a strdup()'d string
> into gl_uniform_storage::name, but it is never freed.
>
> Free gl_uniform_storage::name, fixes some Valgrind reported memory
> leaks.
>
> Signed-off-by: Pekka Paalanen<ppaalanen at gmail.com>
> ---
>   src/mesa/main/shaderobj.c |    4 ++++
>   1 files changed, 4 insertions(+), 0 deletions(-)
>
> diff --git a/src/mesa/main/shaderobj.c b/src/mesa/main/shaderobj.c
> index 454007f..2275430 100644
> --- a/src/mesa/main/shaderobj.c
> +++ b/src/mesa/main/shaderobj.c
> @@ -39,6 +39,7 @@
>   #include "program/prog_parameter.h"
>   #include "program/hash_table.h"
>   #include "ralloc.h"
> +#include "../glsl/ir_uniform.h"
>
>   /**********************************************************************/
>   /*** Shader object functions                                        ***/
> @@ -276,6 +277,9 @@ _mesa_clear_shader_program_data(struct gl_context *ctx,
>                                   struct gl_shader_program *shProg)
>   {
>      if (shProg->UniformStorage) {
> +      unsigned i;
> +      for (i = 0; i<  shProg->NumUserUniformStorage; ++i)
> +         free(shProg->UniformStorage[i].name);
>         ralloc_free(shProg->UniformStorage);

This only plugs one of the leaks.  If you write a test case that relinks 
a program, the ralloc_free(UniformStorage) at the top of 
link_assign_uniform_locations will also leak.

Since the rest of the uniform data is already tracked using ralloc, a 
better solution is to replace the strdup with ralloc_strdup.  Looking in 
link_uniforms, it seems the names are allocated (via 
count_uniform_size::process) before the UniformStorage is allocated (via 
rzalloc_array on line 336).  I believe the right answer is:

  - Add a 'void *mem_ctx' to the count_uniform_size constructor, and 
store it in the object.

  - Use the mem_ctx in the ralloc_strdup call that will replace the 
existing strdup call.

  - Pass prog to the count_uniform_size constructor as the mem_ctx.

  - At the very end of link_assign_uniform_locations, loop over each 
entry in UniformStorage and ralloc_reparent the name field to be owned 
by UniformStorage.  This will prevent transient leaks if a program is 
relinked (due to the ralloc_free at the top of 
link_assign_uniform_locations).

>         shProg->NumUserUniformStorage = 0;
>         shProg->UniformStorage = NULL;