This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
Re: [PATCH] Mach-O: fix two memory leaks
Hi,
Thanks for the comments!
On Wed, Dec 14, 2011 at 12:03 AM, Tristan Gingold <gingold@adacore.com> wrote:
> 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.
Oops, I might somehow miss & in "rels[i] = &res[i];"...
>
> 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 ?
I see. How about this? http://shinh.skr.jp/t/mach-o-leaks-2.patch
Now, it doesn't allocate relocs again if there is the cache, like
elf_slurp_reloc_table in elfcode.h. I modified
bfd_mach_o_get_dynamic_reloc_upper_bound as well because it seems to
need one more space for a NULL pointer.
bfd/
2011-12-14 Shinichiro Hamaji <shinichiro.hamaji@gmail.com>
* mach-o.c (bfd_mach_o_canonicalize_reloc): Update relocation
table only when there isn't the cahce.
(bfd_mach_o_get_dynamic_reloc_upper_bound): Need one more space
for a pointer for the watchdog.
(bfd_mach_o_canonicalize_dynamic_reloc): Utilize cache like
bfd_mach_o_canonicalize_reloc.
(bfd_mach_o_close_and_cleanup): Call bfd_mach_o_free_cached_info.
(bfd_mach_o_free_cached_info): Free up cache data.
* mach-o.h (reloc_cache): A place to store cache of dynamic relocs.
(bfd_mach_o_free_cached_info): Add declaration.
diff --git a/bfd/mach-o.c b/bfd/mach-o.c
index c768689..1c3505c 100644
--- a/bfd/mach-o.c
+++ b/bfd/mach-o.c
@@ -1024,21 +1024,25 @@ 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));
- if (res == NULL)
- return -1;
-
- if (bfd_mach_o_canonicalize_relocs (abfd, asect->rel_filepos,
- asect->reloc_count, res, syms) < 0)
+ if (asect->relocation == NULL)
{
- free (res);
- return -1;
+ res = bfd_malloc (asect->reloc_count * sizeof (arelent));
+ if (res == NULL)
+ return -1;
+
+ if (bfd_mach_o_canonicalize_relocs (abfd, asect->rel_filepos,
+ asect->reloc_count, res, syms) < 0)
+ {
+ free (res);
+ return -1;
+ }
+ asect->relocation = res;
}
+ res = asect->relocation;
for (i = 0; i < asect->reloc_count; i++)
rels[i] = &res[i];
rels[i] = NULL;
- asect->relocation = res;
return i;
}
@@ -1050,7 +1054,7 @@ bfd_mach_o_get_dynamic_reloc_upper_bound (bfd *abfd)
if (mdata->dysymtab == NULL)
return 1;
- return (mdata->dysymtab->nextrel + mdata->dysymtab->nlocrel)
+ return (mdata->dysymtab->nextrel + mdata->dysymtab->nlocrel + 1)
* sizeof (arelent *);
}
@@ -1073,25 +1077,32 @@ bfd_mach_o_canonicalize_dynamic_reloc (bfd
*abfd, arelent **rels,
if (bed->_bfd_mach_o_swap_reloc_in == NULL)
return 0;
- res = bfd_malloc ((dysymtab->nextrel + dysymtab->nlocrel) * sizeof
(arelent));
- if (res == NULL)
- return -1;
-
- if (bfd_mach_o_canonicalize_relocs (abfd, dysymtab->extreloff,
- dysymtab->nextrel, res, syms) < 0)
+ if (mdata->reloc_cache == NULL)
{
- free (res);
- return -1;
- }
+ res = bfd_malloc ((dysymtab->nextrel + dysymtab->nlocrel)
+ * sizeof (arelent));
+ if (res == NULL)
+ return -1;
- if (bfd_mach_o_canonicalize_relocs (abfd, dysymtab->locreloff,
- dysymtab->nlocrel,
- res + dysymtab->nextrel, syms) < 0)
- {
- free (res);
- return -1;
+ if (bfd_mach_o_canonicalize_relocs (abfd, dysymtab->extreloff,
+ dysymtab->nextrel, res, syms) < 0)
+ {
+ free (res);
+ return -1;
+ }
+
+ if (bfd_mach_o_canonicalize_relocs (abfd, dysymtab->locreloff,
+ dysymtab->nlocrel,
+ res + dysymtab->nextrel, syms) < 0)
+ {
+ free (res);
+ return -1;
+ }
+
+ mdata->reloc_cache = res;
}
+ res = mdata->reloc_cache;
for (i = 0; i < dysymtab->nextrel + dysymtab->nlocrel; i++)
rels[i] = &res[i];
rels[i] = NULL;
@@ -3740,9 +3751,24 @@ bfd_mach_o_close_and_cleanup (bfd *abfd)
if (bfd_get_format (abfd) == bfd_object && mdata != NULL)
_bfd_dwarf2_cleanup_debug_info (abfd, &mdata->dwarf2_find_line_info);
+ bfd_mach_o_free_cached_info (abfd);
+
return _bfd_generic_close_and_cleanup (abfd);
}
+bfd_boolean bfd_mach_o_free_cached_info (bfd *abfd)
+{
+ bfd_mach_o_data_struct *mdata = bfd_mach_o_get_data (abfd);
+ asection *asect;
+#define BFCI_FREE(x) if (x != NULL) { free (x); x = NULL; }
+ BFCI_FREE (mdata->reloc_cache);
+ for (asect = abfd->sections; asect != NULL; asect = asect->next)
+ BFCI_FREE (asect->relocation);
+#undef BFCI_FREE
+
+ return TRUE;
+}
+
#define bfd_mach_o_bfd_reloc_type_lookup _bfd_norelocs_bfd_reloc_type_lookup
#define bfd_mach_o_bfd_reloc_name_lookup _bfd_norelocs_bfd_reloc_name_lookup
diff --git a/bfd/mach-o.h b/bfd/mach-o.h
index 0c6f4fd..dadf442 100644
--- a/bfd/mach-o.h
+++ b/bfd/mach-o.h
@@ -519,6 +519,9 @@ typedef struct mach_o_data_struct
/* A place to stash dwarf2 info for this bfd. */
void *dwarf2_find_line_info;
+
+ /* Cache of dynamic relocs. */
+ arelent *reloc_cache;
}
bfd_mach_o_data_struct;
@@ -589,6 +592,7 @@ bfd_boolean bfd_mach_o_find_nearest_line (bfd *,
asection *, asymbol **,
bfd_vma, const char **,
const char **, unsigned int *);
bfd_boolean bfd_mach_o_close_and_cleanup (bfd *);
+bfd_boolean bfd_mach_o_free_cached_info (bfd *);
unsigned int bfd_mach_o_section_get_nbr_indirect (bfd *, bfd_mach_o_section *);
unsigned int bfd_mach_o_section_get_entry_size (bfd *, bfd_mach_o_section *);