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 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;

> +
> +  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.

Ok with these changes.

Thank you for improving Mach-O,
Tristan.

> }
> 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 *);


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