Bug 868 - bfd leaks memory in several places
Summary: bfd leaks memory in several places
Status: RESOLVED FIXED
Alias: None
Product: binutils
Classification: Unclassified
Component: binutils (show other bugs)
Version: 2.17
: P2 normal
Target Milestone: ---
Assignee: unassigned
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2005-04-23 19:35 UTC by John Levon
Modified: 2008-05-21 12:04 UTC (History)
3 users (show)

See Also:
Host:
Target:
Build:
Last reconfirmed:


Attachments
Improve memory release function in dwarf2.c (1.64 KB, patch)
2008-02-20 15:53 UTC, Nick Clifton
Details | Diff
Implement and use bfd_realloc_or_free (3.35 KB, patch)
2008-02-20 17:42 UTC, Nick Clifton
Details | Diff
Leak fixing for bfd_find_nearest_line in DWARF2 (441 bytes, patch)
2008-04-15 20:12 UTC, André Johansen
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description John Levon 2005-04-23 19:35:54 UTC
bfd has several memory leaks. I'm fixing the ones in dwarf2.c, but a simple grep
for 'bfd_realloc' shows several obvious leaks on failure:

./bfd/bfdio.c:        bim->buffer = bfd_realloc (bim->buffer, newsize);
./bfd/bfdio.c:            bim->buffer = bfd_realloc (bim->buffer, newsize);
./bfd/bfdwin.c:  i->data = bfd_realloc (i->data, size_to_alloc);
./bfd/elf-eh-frame.c:     sec_info = bfd_realloc (sec_info,
./bfd/elf-strtab.c:       tab->array = bfd_realloc (tab->array, tab->alloced * amt);
./bfd/elf32-arm.c:  map = bfd_realloc (map, mapcount * sizeof
(elf32_arm_section_map));
./bfd/elf32-ppc.c:      contents = bfd_realloc (contents, trampoff);
./bfd/elf32-xtensa.c:      message = (char *) bfd_realloc (message, len);
./bfd/elflink.c:          finfo->symshndxbuf = destshndx = bfd_realloc
(destshndx, amt * 2);
./bfd/elfxx-ia64.c:           contents = (bfd_byte *) bfd_realloc (contents, amt);
./bfd/ihex.c:         buf = bfd_realloc (buf, (bfd_size_type) chars);
./bfd/ihex.c:     buf = bfd_realloc (buf, (bfd_size_type) len * 2);
./bfd/mach-o.c:       buf = bfd_realloc (buf, size);
./bfd/stabs.c:                    symb = bfd_realloc (symb, buf_len);
./bfd/vms-misc.c:         PRIV (vms_buf) = bfd_realloc (vms_buf,
./bfd/vms-tir.c:        = bfd_realloc (PRIV (image_section)->contents, offset +
size);
./bfd/vms-tir.c:  PRIV (sections) = bfd_realloc (PRIV (sections), amt);
./bfd/vms.c:      PRIV (sections) = bfd_realloc (PRIV (sections), amt);

Hopefully at some point somebody can find time to carefully audit the whole
source base, these are just a few obvious leak points.
Comment 1 Nick Clifton 2005-05-05 14:35:54 UTC
Subject: Re:  New: bfd leaks memory in several places

Hi John,

> bfd has several memory leaks. I'm fixing the ones in dwarf2.c, but a simple grep
> for 'bfd_realloc' shows several obvious leaks on failure:
> 
> ./bfd/bfdio.c:        bim->buffer = bfd_realloc (bim->buffer, newsize);
[etc]

The least intrusive way to resolve most of these would be to provide a 
new function called, say, bfd_realloc_or_free() which could used to 
replace most of the calls to bfd_realloc.  It could be implemented like 
this:


Index: bfd/libbfd.c
===================================================================
RCS file: /cvs/src/src/bfd/libbfd.c,v
retrieving revision 1.38
diff -c -3 -p -r1.38 libbfd.c
*** bfd/libbfd.c	4 May 2005 15:53:33 -0000	1.38
--- bfd/libbfd.c	5 May 2005 14:32:21 -0000
*************** bfd_realloc (void *ptr, bfd_size_type si
*** 180,185 ****
--- 180,214 ----
     return ret;
   }

+
+ /* Reallocate memory using realloc.
+    If this fails the pointer is freed before returning. */
+
+ void *
+ bfd_realloc_or_free (void *ptr, bfd_size_type size)
+ {
+   size_t amount = (size_t) size;
+   void *ret;
+
+   if (size != amount)
+     ret = NULL;
+   else if (ptr == NULL)
+     ret = malloc (amount);
+   else
+     ret = realloc (ptr, amount);
+
+   if (ret == NULL)
+     {
+       if (amount > 0)
+ 	bfd_set_error (bfd_error_no_memory);
+
+       if (ptr != NULL)
+ 	free (ptr);
+     }
+
+   return ret;
+ }
+
   /* Allocate memory using malloc and clear it.  */

   void *


What do you think ?

Cheers
   Nick
Comment 2 John Levon 2005-05-05 14:44:49 UTC
Subject: Re:  bfd leaks memory in several places

On Thu, May 05, 2005 at 02:36:01PM -0000, nickc at redhat dot com wrote:

> The least intrusive way to resolve most of these would be to provide a 
> new function called, say, bfd_realloc_or_free() which could used to 
> replace most of the calls to bfd_realloc.  It could be implemented like 
> this:

This sounds sensible to me.

thanks,
john
Comment 3 John Levon 2006-03-28 13:36:24 UTC
A memory leak is not an "enhancement", it's a bug.
Comment 4 Diogo de Carvalho Kraemer 2008-02-14 18:13:17 UTC
 I'm using the utiliy from http://www.plunk.org/~hatch/goodies/backtracefilt.C
to translate de stack back trace into a readable trace.
 It says in its comment on line 1044 that bfd_find_nearest_line (in
binutils-2.15.92.0.2-13) has a memory leak bug. I donwloaded the last
binutils-2.18 and noted that the bug is still there.
 By debugging the bfd library I found the bug in dwar2.c module: the function
scan_unit_for_symbols allocates memory for the function and variable file names,
and doesn't deallocates it.
 As a suggestion for a fix I'd add the following lines into the for loop of the
_bfd_dwarf2_cleanup_debug_info function:

for (each = stash->all_comp_units; each; each = each->next_unit)
   {
       struct funcinfo *function_table = each->function_table;
       struct varinfo *variable_table = each->variable_table;

       /* ...
       * other deallocation stuff already done
       * ...
       */

       while (function_table)
       {
           if (function_table->file)
           {
               free(function_table->file);
               function_table->file = NULL;
           }
           function_table = function_table->prev_func;
       }
       while (variable_table)
       {
           if (variable_table->file)
           {
               free(variable_table->file);
               variable_table->file = NULL;
           }
           variable_table = variable_table->prev_var;
       }
   }

 I hope this is the correct fix and wait forward for its availability in a
future version soon.
Comment 5 Nick Clifton 2008-02-20 15:53:27 UTC
Created attachment 2276 [details]
Improve memory release function in dwarf2.c
Comment 6 Nick Clifton 2008-02-20 15:56:09 UTC
Hi Diogo,

  You are correct, the dwarf2.c code does not clean up its memory usage very
well.  The patch you suggested is a start, but it does not go far enough.  For
example it does not free the varinfo and funcinfo structures that are also
allocated in scan_unit_for_symbols.  So I am going to apply the uploaded
variation on your patch, along with this changelog entry.

  I notice that I did not follow up on the bfd_realloc_or_free function idea
either, so I will do that as well.

Cheers
  Nick

bfd/ChangeLog
2008-02-20  Diogo de Carvalho Kraemer  <diogo@kraemer.eng.br>
	    Nick Clifton  <nickc@redhat.com>

	PR 868
	* dwarf2.c (read_abbrevs): Free the abbreviation table if we run
	out of memory.
	(decode_line_info): Free the line_info_table before returning a
	failure result.
	(_bfd_dwarf2_cleanup_debug_info): Free the abbreviation table.
	Free the line table.  Free the function table.  Free the variable
	table.
Comment 7 Nick Clifton 2008-02-20 17:42:54 UTC
Created attachment 2278 [details]
Implement and use bfd_realloc_or_free
Comment 8 Nick Clifton 2008-02-20 17:44:30 UTC
Hi John,

  I have checked in the patch to implement and use bfd_realloc_or_free.

  I am going to close this issue for, since the specific problem of using
bfd_realloc has now been addressed, but it can be reopened if more memory leaks
are discovered.

Cheers
  Nick

bfd/ChangeLog
2008-02-20  Nick Clifton  <nickc@redhat.com>

	PR 868
	* libbfd.c (bfd_realloc_or_free): New function.  Performs like
	bfd_realloc, but if the (re)allocation fails, the pointer is
	freed.
	* libbfd-in.h: Prototype.
	* libbfd.h: Regenerate.
	* bfdio.c (bfd_bwrite): Use the new function.
	(bfd_seek): Likewise.
	* bfdwin.c:(bfd_get_file_window): Likewise.
	* elf-strtab.c (_bfd_elf_strtab_add): Likewise.
	* elf32-ppc.c (ppc_elf_relax_section): Likewise.
	* elf32-xtensa.c (vsprintf_msg): Likewise.
	* mach-o.c (bfd_mach_o_core_fetch_environment): Likewise.
	* stabs.c (_bfd_link_seciton_stabs): Likewise.
	* vms-misc.c (_bfd_vms_get_record): Likewise.
	* vms-tir.c (check_section): Likewise.
	* vms.c (vms_new_section_hook): Likewise.
	* elf32-arm.c (elf32_arm_section_map_add): Check that the
	allocation of sec_data->map succeeded before using it.
	* elflink.c (elf_link_output_sym): Do not overwrite finfo->
	symshndxbuf until it is known that the reallocation succeeded.
Comment 9 Nick Clifton 2008-02-22 15:26:50 UTC
Hi Diogo,

  I have just realised that my dwarf2.c patch was completely bogus.  There is no
memory leak because the routines are using bfd_alloc and bfd_zalloc, which uses
an objstack.  The memory is freed later on in the linking process when
bfd_release() is called.  So I am going to revert my patch.  Sorry about not
realising this sooner.

Cheers
  Nick

Comment 10 Alan Modra 2008-02-23 01:47:19 UTC
What is the point of the patch in comment #1?  If we fail a realloc we are
shortly going to exit with an error.  Who cares if we haven't freed all memory?
Comment 11 Nick Clifton 2008-02-25 10:23:48 UTC
Hi Alan,

> What is the point of the patch in comment #1?  If we fail a realloc we are
> shortly going to exit with an error.

Not necessarily.  It is possible whatever was being attempted at the time will
be aborted, but that the program will carry on rather than just exiting.

Besides it is better to be paranoid and free an allocated block of memory if all
the remaining pointers to it are about to be discarded, rather than just leaving
it orphaned.

Cheers
  Nick
Comment 12 André Johansen 2008-04-10 09:08:59 UTC
Nick, I'm not sure reverting the cleanup is correct.  At least in binutils 
2.18, the function concat_filename uses bfd_malloc, which again uses malloc.  
As far as I can see, it does the same in current CVS.


I get leak reports like this when running under Valgrind:

==28969== 120,701 bytes in 1,422 blocks are definitely lost in loss record 2 
of 2
==28969==    at 0x4904A06: malloc (vg_replace_malloc.c:149)
==28969==    by 0x407305: bfd_malloc (libbfd.c:173)
==28969==    by 0x46BC09: concat_filename (dwarf2.c:1064)
==28969==    by 0x46CBFC: scan_unit_for_symbols (dwarf2.c:1936)
==28969==    by 0x46CFFE: comp_unit_find_nearest_line (dwarf2.c:2262)
==28969==    by 0x46DD9D: find_line (dwarf2.c:3093)
==28969==    by 0x46E4A6: _bfd_dwarf2_find_nearest_line (dwarf2.c:3127)
==28969==    by 0x41F3DA: _bfd_elf_find_nearest_line (elf.c:6889)


Running with a change similar to the one in comment #14 plus freeing 
caller_file removes the leaks I see when doing call-stack mapping.
(CentOS 4.5/x86_64. using GCC 4 gcc4-4.1.1-53.EL4 and BFD from binutils 2.18.)
Comment 13 André Johansen 2008-04-10 10:15:28 UTC
That should of course be comment #4.
Comment 14 Nick Clifton 2008-04-15 14:05:15 UTC
Subject: Re:  bfd leaks memory in several places

Hi Andre,

> Nick, I'm not sure reverting the cleanup is correct.  At least in binutils 
> 2.18, the function concat_filename uses bfd_malloc, which again uses malloc.  

Hmm, that is a fair point.

> Running with a change similar to the one in comment #14 plus freeing 
> caller_file removes the leaks I see when doing call-stack mapping.

OK, can you submit this as a new patch then please ?

Cheers
   Nick


Comment 15 André Johansen 2008-04-15 20:12:36 UTC
Created attachment 2697 [details]
Leak fixing for bfd_find_nearest_line in DWARF2
Comment 16 Nick Clifton 2008-05-21 12:04:47 UTC
Hi Andre,

  Thanks for the additional patch.  I have now applied it to the sources.

Cheers
  Nick