Home Original page

[PATCH] Mach-O: fix two memory leaks

shinichiro hamaji shinichiro.hamaji@gmail.com
Wed Dec 14 13:52:00 GMT 2011 
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 *);
More information about the Binutils mailing list