This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
Re: [PATCH] Mach-O: fix two memory leaks
- From: Tristan Gingold <gingold at adacore dot com>
- To: shinichiro hamaji <shinichiro dot hamaji at gmail dot com>
- Cc: binutils Development <binutils at sourceware dot org>
- Date: Tue, 13 Dec 2011 16:03:18 +0100
- Subject: Re: [PATCH] Mach-O: fix two memory leaks
- References: <CALOyH=Liaf8dTTP+afOw=QV9ANhO32a--y8C96_QYGVWwbtDMg@mail.gmail.com>
Hi,
On Dec 13, 2011, at 3:45 PM, shinichiro hamaji wrote:
> Hi,
>
> This patch fixes the memory leaks I've noticed while writing my
> previous patch. The patch which can be applied to the CVS head:
> http://shinh.skr.jp/t/mach-o-leaks.patch
>
> I'd like to add a few descriptions about this although they might be
> too obvious for binutils gurus...
>
> For bfd_mach_o_canonicalize_reloc, this leak can be reproduced by something like
>
> % valgrind --leak-check=full objdump -b mach-o-x86-64 -x hello.o
>
> As for bfd_mach_o_canonicalize_dynamic_reloc, I confirmed this patch
> fixes the leaks with a dynamic library created by
>
> % gcc -dynamiclib mach/hello.c -mmacosx-version-min=10.4 -o hello.dylib
>
> This patch compiled with --enable-targets=all and "make check" passed.
>
> bfd/
> 2011-12-13 Shinichiro Hamaji <shinichiro.hamaji@gmail.com>
>
> * mach-o.c (bfd_mach_o_canonicalize_reloc): Use bfd_alloc instead
> of bfd_malloc to get rid of memory leaks.
> (bfd_mach_o_canonicalize_dynamic_reloc): Free the malloced
> buffer.
>
> diff --git a/bfd/mach-o.c b/bfd/mach-o.c
> index 54edd07..32d6d6c 100644
> --- a/bfd/mach-o.c
> +++ b/bfd/mach-o.c
> @@ -810,7 +810,7 @@ bfd_mach_o_canonicalize_reloc (bfd *abfd, asection *asect,
> if (bed->_bfd_mach_o_swap_reloc_in == NULL)
> return 0;
>
> - res = bfd_malloc (asect->reloc_count * sizeof (arelent));
> + res = bfd_alloc (abfd, asect->reloc_count * sizeof (arelent));
> if (res == NULL)
> return -1;
This one is correct but not ideal.
> @@ -881,6 +881,7 @@ bfd_mach_o_canonicalize_dynamic_reloc (bfd *abfd,
> arelent **rels,
> for (i = 0; i < dysymtab->nextrel + dysymtab->nlocrel; i++)
> rels[i] = &res[i];
> rels[i] = NULL;
> + free (res);
> return i;
This one is wrong, as res is still referenced by rels[]. I think you should use bfd_alloc here too.
The issue with bfd_alloc is that you can't free just the relocs, which might be an issue in the context of gdb.
Why not keep using bfd_malloc and freeing relocs in close_and_cleanup and in free_cached_info ?
Tristan.