[RFA:] fix linker deallocation bug: link_info.hash deallocated too soon

[RFA:] fix linker deallocation bug: link_info.hash deallocated too soon

[Hans-Peter Nilsson]

This bug is fallout from the free-sprinkling-spree of December 2012.
The bug is premature freeing of the link_info.hash table.  See the
comments in the patch.  It affects only "weird" non-ELF backends.

A BFD target is allowed to hold on to stuff, e.g. symbols until its
_bfd_elf_write_object_contents method, called at time of bfd_close.
The "mmo" target does this, as the position for symbols depend on the
size of all other file contents including escape codes (the "mmo"
format is stream- or record-like, like ihex, not mapped like ELF and
without record length information, hence escape codes).  ELF targets
don't see the bug as they don't output "regular" symbols through
_bfd_elf_write_object_contents.  There is no alternative BFD backend
function where contents could be emitted, and frankly I'd rather fix
BFD-usage bugs in the linker than change the BFD API or silently use
another BFD backend function in a sequence not required by the BFD
documentation.

I noticed this through massive regressions for mmix-knuth-mmixware on
gcc test-results, where the bug caused invalid symbols (empty ones and
duplicate ones) for which the mmo symbol-writer code aborted.  With a
binutils test-run doctored to call valgrind, valgrind somewhat
surprisingly still showed nothing for the ordinary tests!  Apparently
--wrap is required to expose the problem, and --wrap is only tested
for ELF targets in the linker test-suite (and only for native targets
and requires a compiler, so those test-cases aren't easily adopted to
a wider audience).

I added four test-cases alas MMIX-specific, as I had trouble coming up
with a generic clean test-case.  (N.B.: only ld-mmix/wrap1 trigs the
bug; there's also a elf64mmix one and two dummies; variants that just
produce the same binary without --wrap).  Valgrind is still required
to spot the bug.  Only a truly big test-case (at least 6647 of the gcc
test-cases) requires memory allocation in the mmo target such that the
symbol table info is corrupted making the bug visible without
valgrind-like tools - and still not enough to crash the linker.  Maybe
a plugin is required too, but an initial wild-goose chase found only
that it was not the cause of the bug (I should have used valgrind
at once as that pointed out the bug, but plugins was a "usual suspect").

At first I tried plainly moving the bfd_link_hash_table_free call into
ld_cleanup, but valgrind alerted me that bfd_link_hash_table_free
needs to deference the (deallocated) output_bfd.  The hash_table_free
function is "virtual"; it needs to dereference the BFD xvec "vtable".

Alternative solution 1: add two new BFD functions that together
perform bfd_close; i.e. split out the bfd deallocation such that
link_info.output_bfd is still valid as an parameter to BFD functions.
I'm on the fence on this one, but it'd still require moving the
link_info.hash deallocation to later - and the below is the "cleanest"
move as opposed to moving it to ld_cleanup mentioned above, which
would move it further away from the context where it was created.

Alternative solution 2: don't call bfd_link_hash_table_free at all.
But that'd be counter to adding it in the first place and would
require adding a comment in lang_finish so it won't happen again.  Too
much trouble! :-P  Oh, and we wouldn't get a clean no-leak bill from
whatever leak checkers people like to run on binutils.

Tested mmix-knuth-mmixware and cris-elf.
(Also ran the gcc test-suite for mmix-knuth-mmixware which now shows
sane results; as sane as with a binutils before December 2012.)

Ok to commit?

ld:
	* ldlang.c (lang_finish_atexit): New function.
	(output_bfd_hash_table_free_fn): New variable.
	(lang_finish): Arrange to call lang_finish_atexit at exit-time
	to free link_info.hash.

ld/testsuite:
	* ld-mmix/wrap1.d, ld-mmix/wrap1a.s, ld-mmix/wrap1b.s,
	ld-mmix/wrap1c.s, ld-mmix/wrap2.d, ld-mmix/wrap3.d,
	ld-mmix/wrap3a.s, ld-mmix/wrap3b.s, ld-mmix/wrap4.d: New
	tests.

diff --git a/ld/ldlang.c b/ld/ldlang.c
index d147ee0..c612294 100644
--- a/ld/ldlang.c
+++ b/ld/ldlang.c
@@ -1234,10 +1234,32 @@ lang_init (void)
   asneeded_list_tail = &asneeded_list_head;
 }

+static void (*output_bfd_hash_table_free_fn) (struct bfd_link_hash_table *);
+
+/* Helper for lang_finish: complete deferred deallocation.  */
+
+static void
+lang_finish_atexit (void)
+{
+  (*output_bfd_hash_table_free_fn) (link_info.hash);
+}
+
 void
 lang_finish (void)
 {
-  bfd_link_hash_table_free (link_info.output_bfd, link_info.hash);
+  /* We want to please memory leak checkers by deleting
+     link_info.hash.  We can't do it now, as a bfd target may hold
+     references to symbols in this table and use them when their
+     _bfd_write_contents function is invoked, as part of bfd_close on
+     the output_bfd.  But, output_bfd is deallocated at bfd_close, so
+     we can't refer to output_bfd after that time, and dereferencing
+     it is needed to call "bfd_link_hash_table_free".  Smash this
+     dependency deadlock and grab the function pointer; arrange to
+     call it on link_info.hash before exiting.  */
+  output_bfd_hash_table_free_fn
+    = link_info.output_bfd->xvec->_bfd_link_hash_table_free;
+  xatexit (lang_finish_atexit);
+
   bfd_hash_table_free (&lang_definedness_table);
   output_section_statement_table_free ();
 }

diff --git a/ld/testsuite/ld-mmix/wrap1.d b/ld/testsuite/ld-mmix/wrap1.d
new file mode 100644
index 0000000..02d7bef
--- /dev/null
+++ b/ld/testsuite/ld-mmix/wrap1.d
@@ -0,0 +1,21 @@
+#source: start.s
+#source: wrap1a.s
+#source: wrap1b.s
+#source: wrap1c.s
+#ld: -m mmo --wrap deal
+#as: -no-expand
+#objdump: -d
+
+.*:     file format mmo
+
+Disassembly of section \.text:
+
+0+ <(_start|Main)>:
+   0:	e3fd0001 	setl \$253,0x1
+   4:	f2000001 	pushj \$0,8 <__wrap_deal>
+
+0+8 <__wrap_deal>:
+   8:	f0000001 	jmp c <deal>
+
+0+c <deal>:
+   c:	fd000000 	swym 0,0,0
diff --git a/ld/testsuite/ld-mmix/wrap1a.s b/ld/testsuite/ld-mmix/wrap1a.s
new file mode 100644
index 0000000..88a5cd2
--- /dev/null
+++ b/ld/testsuite/ld-mmix/wrap1a.s
@@ -0,0 +1,2 @@
+ .text
+ pushj $0,deal
diff --git a/ld/testsuite/ld-mmix/wrap1b.s b/ld/testsuite/ld-mmix/wrap1b.s
new file mode 100644
index 0000000..367aea0
--- /dev/null
+++ b/ld/testsuite/ld-mmix/wrap1b.s
@@ -0,0 +1,4 @@
+ .text
+ .globl __wrap_deal
+__wrap_deal:
+ jmp __real_deal
diff --git a/ld/testsuite/ld-mmix/wrap1c.s b/ld/testsuite/ld-mmix/wrap1c.s
new file mode 100644
index 0000000..a7678d4
--- /dev/null
+++ b/ld/testsuite/ld-mmix/wrap1c.s
@@ -0,0 +1,4 @@
+ .text
+ .globl deal
+deal:
+ swym 0
diff --git a/ld/testsuite/ld-mmix/wrap2.d b/ld/testsuite/ld-mmix/wrap2.d
new file mode 100644
index 0000000..49b4d3b
--- /dev/null
+++ b/ld/testsuite/ld-mmix/wrap2.d
@@ -0,0 +1,21 @@
+#source: start.s
+#source: wrap1a.s
+#source: wrap1b.s
+#source: wrap1c.s
+#ld: -m elf64mmix --wrap deal
+#as: -no-expand
+#objdump: -d
+
+.*:     file format elf64-mmix
+
+Disassembly of section \.text:
+
+0+ <(_start|Main)>:
+   0:	e3fd0001 	setl \$253,0x1
+   4:	f2000001 	pushj \$0,8 <__wrap_deal>
+
+0+8 <__wrap_deal>:
+   8:	f0000001 	jmp c <deal>
+
+0+c <deal>:
+   c:	fd000000 	swym 0,0,0
diff --git a/ld/testsuite/ld-mmix/wrap3.d b/ld/testsuite/ld-mmix/wrap3.d
new file mode 100644
index 0000000..80b20f1
--- /dev/null
+++ b/ld/testsuite/ld-mmix/wrap3.d
@@ -0,0 +1,21 @@
+#source: start.s
+#source: wrap3a.s
+#source: wrap3b.s
+#source: wrap1c.s
+#ld: -m mmo
+#as: -no-expand
+#objdump: -d
+
+.*:     file format mmo
+
+Disassembly of section \.text:
+
+0+ <(_start|Main)>:
+   0:	e3fd0001 	setl \$253,0x1
+   4:	f2000001 	pushj \$0,8 <__wrap_deal>
+
+0+8 <__wrap_deal>:
+   8:	f0000001 	jmp c <deal>
+
+0+c <deal>:
+   c:	fd000000 	swym 0,0,0
diff --git a/ld/testsuite/ld-mmix/wrap3a.s b/ld/testsuite/ld-mmix/wrap3a.s
new file mode 100644
index 0000000..7192a93
--- /dev/null
+++ b/ld/testsuite/ld-mmix/wrap3a.s
@@ -0,0 +1,2 @@
+ .text
+ pushj $0,__wrap_deal
diff --git a/ld/testsuite/ld-mmix/wrap3b.s b/ld/testsuite/ld-mmix/wrap3b.s
new file mode 100644
index 0000000..6a8a606
--- /dev/null
+++ b/ld/testsuite/ld-mmix/wrap3b.s
@@ -0,0 +1,4 @@
+ .text
+ .globl __wrap_deal
+__wrap_deal:
+ jmp deal
diff --git a/ld/testsuite/ld-mmix/wrap4.d b/ld/testsuite/ld-mmix/wrap4.d
new file mode 100644
index 0000000..a64578d
--- /dev/null
+++ b/ld/testsuite/ld-mmix/wrap4.d
@@ -0,0 +1,21 @@
+#source: start.s
+#source: wrap3a.s
+#source: wrap3b.s
+#source: wrap1c.s
+#ld: -m elf64mmix
+#as: -no-expand
+#objdump: -d
+
+.*:     file format elf64-mmix
+
+Disassembly of section \.text:
+
+0+ <(_start|Main)>:
+   0:	e3fd0001 	setl \$253,0x1
+   4:	f2000001 	pushj \$0,8 <__wrap_deal>
+
+0+8 <__wrap_deal>:
+   8:	f0000001 	jmp c <deal>
+
+0+c <deal>:
+   c:	fd000000 	swym 0,0,0

brgds, H-P

Re: [RFA:] fix linker deallocation bug: link_info.hash deallocated too soon

[Alan Modra]

On Thu, May 08, 2014 at 07:27:02AM -0400, Hans-Peter Nilsson wrote:
> At first I tried plainly moving the bfd_link_hash_table_free call into
> ld_cleanup, but valgrind alerted me that bfd_link_hash_table_free
> needs to deference the (deallocated) output_bfd.  The hash_table_free
> function is "virtual"; it needs to dereference the BFD xvec "vtable".

I think it still makes sense to use ld_cleanup, simply to avoid
another atexit function.  Also, set output_bfd_hash_table_free_fn
in open_output, after the hash table is created.  The patch is OK with
those changes.

-- 
Alan Modra
Australia Development Lab, IBM

Re: [RFA:] fix linker deallocation bug: link_info.hash deallocated too soon

[Hans-Peter Nilsson]

On Fri, 9 May 2014, Alan Modra wrote:
> On Thu, May 08, 2014 at 07:27:02AM -0400, Hans-Peter Nilsson wrote:
> > At first I tried plainly moving the bfd_link_hash_table_free call into
> > ld_cleanup, but valgrind alerted me that bfd_link_hash_table_free
> > needs to deference the (deallocated) output_bfd.  The hash_table_free
> > function is "virtual"; it needs to dereference the BFD xvec "vtable".
>
> I think it still makes sense to use ld_cleanup, simply to avoid
> another atexit function.  Also, set output_bfd_hash_table_free_fn
> in open_output, after the hash table is created.  The patch is OK with
> those changes.

Thanks for the review!

I'd sooner worry about spreading this out over several files,
but here we go.  I'll commit this (and the testcases, excluded
this time) in the next 48h.  Tested as before.

	* ldlang.c (lang_finish): Don't call bfd_link_hash_table_free here.
	(output_bfd_hash_table_free_fn): New variable.
	(open_output): Save the _bfd_link_hash_table_free function for the
	output_bfd into output_bfd_hash_table_free_fn.
	* ldmain.c (ld_cleanup): If set, call output_bfd_hash_table_free_fn
	on link_info.hash.
	* ldlang.h (output_bfd_hash_table_free_fn): Declare.

diff --git a/ld/ldlang.c b/ld/ldlang.c
index d147ee0..8d1e3f7 100644
--- a/ld/ldlang.c
+++ b/ld/ldlang.c
@@ -1237,7 +1237,6 @@ lang_init (void)
 void
 lang_finish (void)
 {
-  bfd_link_hash_table_free (link_info.output_bfd, link_info.hash);
   bfd_hash_table_free (&lang_definedness_table);
   output_section_statement_table_free ();
 }
@@ -3073,6 +3072,9 @@ lang_get_output_target (void)
   return default_target;
 }

+/* Stashed function to free link_info.hash; see open_output.  */
+void (*output_bfd_hash_table_free_fn) (struct bfd_link_hash_table *);
+
 /* Open the output file.  */

 static void
@@ -3152,6 +3154,18 @@ open_output (const char *name)
   if (link_info.hash == NULL)
     einfo (_("%P%F: can not create hash table: %E\n"));

+  /* We want to please memory leak checkers by deleting link_info.hash.
+     We can't do it in lang_finish, as a bfd target may hold references to
+     symbols in this table and use them when their _bfd_write_contents
+     function is invoked, as part of bfd_close on the output_bfd.  But,
+     output_bfd is deallocated at bfd_close, so we can't refer to
+     output_bfd after that time, and dereferencing it is needed to call
+     "bfd_link_hash_table_free".  Smash this dependency deadlock and grab
+     the function pointer; arrange to call it on link_info.hash in
+     ld_cleanup.  */
+  output_bfd_hash_table_free_fn
+    = link_info.output_bfd->xvec->_bfd_link_hash_table_free;
+
   bfd_set_gp_size (link_info.output_bfd, g_switch_value);
 }

diff --git a/ld/ldlang.h b/ld/ldlang.h
index aacd5dc..47cc4df 100644
--- a/ld/ldlang.h
+++ b/ld/ldlang.h
@@ -518,6 +518,8 @@ extern lang_statement_list_type input_file_chain;
 extern int lang_statement_iteration;
 extern struct asneeded_minfo **asneeded_list_tail;

+extern void (*output_bfd_hash_table_free_fn) (struct bfd_link_hash_table *);
+
 extern void lang_init
   (void);
 extern void lang_finish
diff --git a/ld/ldmain.c b/ld/ldmain.c
index 4c7ea68..2d987b8 100644
--- a/ld/ldmain.c
+++ b/ld/ldmain.c
@@ -171,6 +171,10 @@ ld_cleanup (void)
 #endif
   if (output_filename && delete_output_file_on_failure)
     unlink_if_ordinary (output_filename);
+
+  /* See open_output in ldlang.c.  */
+  if (output_bfd_hash_table_free_fn != NULL)
+    (*output_bfd_hash_table_free_fn) (link_info.hash);
 }

 /* If there's a BFD assertion, we'll notice and exit with an error

brgds, H-P