This is the mail archive of the binutils@sourceware.org mailing list for the binutils project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] Mach-O: fix two memory leaks


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.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]