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

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

[Michael Snyder]

[-- Attachment #1: Type: text/plain, Size: 5 bytes --]

OK?


[-- Attachment #2: ar.txt --]
[-- Type: text/plain, Size: 1506 bytes --]

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

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

[Richard Sandiford]

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