This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
Re: [RFA] ar.c (replace_members): Plug memory leak.
- From: Richard Sandiford <richard dot sandiford at linaro dot org>
- To: Michael Snyder <msnyder at vmware dot com>
- Cc: "binutils\ at sourceware dot org" <binutils at sourceware dot org>
- Date: Tue, 15 Mar 2011 11:56:42 +0000
- Subject: Re: [RFA] ar.c (replace_members): Plug memory leak.
- References: <4D768E74.9070600@vmware.com>
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