This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
Re: [PATCH] Mach-O: fix two memory leaks
On Wed, Dec 14, 2011 at 10:35 PM, Tristan Gingold <gingold@adacore.com> wrote:
> Hi,
>
>
> On Dec 14, 2011, at 2:12 PM, shinichiro hamaji wrote:
>
>> Hi,
>>
>> Thanks for the comments!
>
> […]
>
>> I see. How about this? http://shinh.skr.jp/t/mach-o-leaks-2.patch
>
> Great. ?Just a few minor comments.
>
>> 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.
>
> Indeed, good catch.
>
>> 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
>
> Honestly I don't like this style. ?You don't need to test if x is NULL before calling free, as free(0) is in fact a no-op.
> I prefer you directly write free (…); … = NULL;
OK, I just borrowed this macro from aoutx.h and I'm not a big fun of
this either.
>
>> +
>> + ?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;
>
> Can you just rename reloc_cache to dyn_reloc_cache, just to make its purpose clearer.
Done.
Thanks again for your quick response. Here is the updated patch:
http://shinh.skr.jp/t/mach-o-leaks-3.patch
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..e5da70b 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->dyn_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->dyn_reloc_cache = res;
}
+ res = mdata->dyn_reloc_cache;
for (i = 0; i < dysymtab->nextrel + dysymtab->nlocrel; i++)
rels[i] = &res[i];
rels[i] = NULL;
@@ -3740,9 +3751,26 @@ 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;
+ free (mdata->dyn_reloc_cache);
+ mdata->dyn_reloc_cache = NULL;
+ for (asect = abfd->sections; asect != NULL; asect = asect->next)
+ {
+ free (asect->relocation);
+ asect->relocation = NULL;
+ }
+
+ 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..07c6935 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 *dyn_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 *);