Home Original page

[RFA] ar.c (replace_members): Plug memory leak.

Richard Sandiford richard.sandiford@linaro.org
Tue Mar 15 11:56:00 GMT 2011 
Michael Snyder <msnyder@vmware.com> writes:
> OK?
>
> 2011-03-08  Michael Snyder  <msnyder@msnyder-server.eng.vmware.com>
>
> 	* ar.c (replace_members): Plug memory leak.
>
> Index: ar.c
> ===================================================================
> RCS file: /cvs/src/src/binutils/ar.c,v
> retrieving revision 1.72
> diff -u -p -r1.72 ar.c
> --- ar.c	8 Dec 2010 05:05:30 -0000	1.72
> +++ ar.c	8 Mar 2011 20:14:08 -0000
> @@ -1233,6 +1233,7 @@ replace_members (bfd *arch, char **files
>    bfd **after_bfd;		/* New entries go after this one.  */
>    bfd *current;
>    bfd **current_ptr;
> +  char *tmp1 = NULL, *tmp2 = NULL;
>  
>    while (files_to_move && *files_to_move)
>      {
> @@ -1245,8 +1246,11 @@ replace_members (bfd *arch, char **files
>  
>  	      /* For compatibility with existing ar programs, we
>  		 permit the same file to be added multiple times.  */
> -	      if (FILENAME_CMP (normalize (*files_to_move, arch),
> -				normalize (current->filename, arch)) == 0
> +	      free (tmp1);
> +	      free (tmp2);
> +	      tmp1 = normalize (*files_to_move, arch);
> +	      tmp2 = normalize (current->filename, arch);
> +	      if (FILENAME_CMP (tmp1, tmp2) == 0
>  		  && current->arelt_data != NULL)
>  		{
>  		  if (newer_only)
> @@ -1291,7 +1295,7 @@ replace_members (bfd *arch, char **files
>  			  verbose, make_thin_archive))
>  	changed = TRUE;
>  
> -    next_file:;
> +    next_file:
>  
>        files_to_move++;
>      }
> @@ -1300,6 +1304,9 @@ replace_members (bfd *arch, char **files
>      write_archive (arch);
>    else
>      output_filename = NULL;
> +
> +  free (tmp1);
> +  free (tmp2);
>  }
>  
>  static int

This, and the other normalize-based changes, don't look right to me.
normalize() only allocates memory conditionally, so you can't free
it unconditionally like this.  The path that does allocate even has
the comment:

      /* Space leak.  */

to indicate that this is a deliberate leak.  (Though TBH, I wonder
why the call in e.g. delete_members isn't hoised out of the containing
"while" loop.)

FWIW, I've cowardly left the strings.c patch to another reviewer.
That's certainly a case where the memory lives for pretty much the
lifetime of the program, and I don't want to get into a debate about
whether it's better to free in that case (wasted cycles on
termination, etc).

Richard
More information about the Binutils mailing list