[PING] LD: Hidden/internal versioned symbol preemption bug

[PING] LD: Hidden/internal versioned symbol preemption bug

[Maciej W. Rozycki]

Hi,

 Is there anything I could do to help with the review of this change?

 It is a pretty fatal failure affecting all targets, so I would like to 
see it fixed in the upcoming release which I have been told is happening 
by the end of this month and I'll be disappearing for a fortnight starting 
from Wednesday.

 Included below verbatim for easy reference and the complete series posted 
here:

http://sourceware.org/ml/binutils/2012-08/msg00159.html
http://sourceware.org/ml/binutils/2012-08/msg00160.html
http://sourceware.org/ml/binutils/2012-08/msg00161.html

 I will appreciate your feedback.

  Maciej

---------- Forwarded message ----------
Message-ID: <alpine.DEB.1.10.1208071701230.20608@tp.orcam.me.uk>
Date: Wed, 8 Aug 2012 13:19:40 +0100
From: Maciej W. Rozycki <macro@codesourcery.com>
To: binutils@sourceware.org
Cc: Alan Modra <amodra@gmail.com>
Subject: [PATCH 1/3] LD: Hidden/internal versioned symbol preemption bug

Hi,

 While working on the MIPS _gp special symbol scope bug I have addressed 
recently I ran the glibc test suite to double-check the bug fix did not 
have any adverse effect on the library and stumbled across this mysterious 
progression for PLT (o32) multilibs only (the test already succeeded for 
n64):

-FAIL: build dlfcn/bug-atexit3-lib.so

for PLT multilibs only (the test already succeeded for n64).  Upon a 
closer inspection I have discovered the following link failure, 
mysteriously, has been removed:

.../mips-linux-gnu/bin/ld: BFD (...) 2.22.52.20120308 assertion fail .../bfd/elfxx-mips.c:3475
.../lib/gcc/mips-linux-gnu/4.6.3/libgcc_eh.a(unwind-dw2.o): In function `_Unwind_FindEnclosingFunction':
.../gcc/unwind-dw2.c:311:(.text+0x35b4): relocation truncated to fit: R_MIPS_CALL16 against`_Unwind_Find_FDE@@GCC_3.0'
collect2: ld returned 1 exit status

and the test fully passed.  There was another assertion failure 
immediately preceding though:

.../mips-linux-gnu/bin/ld: BFD (...) 2.22.52.20120308 assertion fail .../bfd/elfxx-mips.c:3471

that had been retained and the test harness is not smart enough to choke 
on it (there was no link failure, apparently LD included in the somewhat 
older version of binutils did not terminate on assertion failures).  
These assertions are:

BFD_ASSERT (got_index < htab->sgot->size);

and:

BFD_ASSERT (h->dynindx >= global_got_dynindx);

respectively in mips_elf_global_got_index and the latter assertion 
triggered across all multilibs including SVR4 stubs ones (n64).

 This is quite obscure, but I was able to reduce the scenario as follows.  
A shared library is being linked.  There are a couple of shared libraries 
involved in the link and there are also a number of static archives pulled 
from.  There is a symbol reference from one of these static archives that 
is satisfied with a versioned symbol defined in one of the shared 
libraries.  A GOT entry is assigned for the symbol reference.

 Later on, another archive member is pulled that overrides the versioned 
symbol with a hidden definition.  The GOT entry for the old reference is 
discarded and the linker is supposed to have overwritten the definition 
from the shared library with the hidden one that will be ultimately used 
within this shared library being linked.

 However when the dynamic object is finally layed out, the original 
reference to the versioned symbol turns out to have been retained and a 
GOT reference is attempted to be finalised to satisfy the respective 
relocation.  This ends up unsuccessful as the entry assignment has already 
beed discarded (dynindx set to -1).  This causes the assertion failure, 
and also a link failure in our current sources.

 On further debugging this issue I made the following observations.  The 
bug affects all Linux targets (with a representative sample actually 
examined), causing different failure modes:

1. For ARM a broken binary is produced that uses the wrong symbol's final
   address.

2. For MIPS an assertion failure is triggered as noted above and the link 
   fails.

3. For Power a broken binary is produced that has an incorrect dynamic
   relocation:

000101d0  ffffff01 R_PPC_ADDR32      bad symbol index: 00ffffff

4. For x86 the link fails:

unresolvable R_386_32 relocation against symbol `_Unwind_Find_FDE@@GCC_3.0'
final link failed: Nonrepresentable section on output

The bug can be reproduced with a simple test case involving three symbols
and four objects used in the link -- three relocatables and one shared
library.  If a non-versioned symbol is used instead, then the binary
produced is correct.

 I have finally tracked down the cause to a piece of code in 
_bfd_elf_merge_symbol.  Interestingly enough this code has been recently 
updated to handle the case of overriding with a protected symbol 
correctly:

http://sourceware.org/ml/binutils/2012-05/msg00383.html

but the corresponding case of a hidden/internal symbol has not been 
adjusted accordingly.  The rules of preemption are the same for all the 
three non-default export classes (any dynamic definition must be removed 
from the scope, it won't be used in the module produced in this static 
link) and the fix below therefore borrows from code immediately following 
that is used to override the indirect dynamic symbol (or just The Symbol 
when no versioning has been applied).

 Please note that as a side effect this change also removes some code that 
became dead as a result of the change referred to in the previous 
paragraph, specifically these assignments:

  vh->dynamic_def = 1;
  vh->ref_dynamic = 1;

and:

  vh->ref_dynamic = 0;

in the respective conditional blocks, because at the conclusion of the 
containing block this assignment is made:

  h = vh;

and a piece of code below then sets:

  h->ref_dynamic = 0;

or:

  h->ref_dynamic = 1;

as appropriate and:

  h->dynamic_def = 0;

Likewise there's no need to call:

  (*bed->elf_backend_hide_symbol) (info, vh, TRUE);

that's already made for h below under the same condition.

 This fix removes the failures noted above for the four targets and causes 
the correct value of the hidden/internal symbol to be used for relocation.  
It causes no regressions for a superset of Alan's favourite targets.  It 
also causes no regressions in i686-linux, x86_64-linux, mipsel-linux or 
mips64el-linux native testing (regrettably most export class tests are run 
in native testing only; I can't test non-x86 non-MIPS native targets 
reasonably easily or at all).

 OK to apply?

 I'll follow up with two other changes:

1. A piece to add a set of assertions to the Power target to guard against 
   dynindx being minus one similar to what MIPS and other targets use.  
   One of these assertions triggers as appropriate (depending on the 
   relocation type involved) and prevents the value of minus one to 
   propagate to the final binary's dynamic relocation table as observed 
   above.

2. A set of test cases to cover non-default export class preemption of a 
   versioned symbol.  These have been adapted for the LD test suite from 
   code reduced from the case included in glibc.

2012-08-08  Maciej W. Rozycki  <macro@codesourcery.com>

	bfd/
	* elflink.c (_bfd_elf_merge_symbol): Also override the version
	a dynamic symbol defaulted to if preempted with a hidden or
	internal definition.

  Maciej

binutils-bfd-preempt-indirect.diff
Index: binutils-fsf-trunk-quilt/bfd/elflink.c
===================================================================
--- binutils-fsf-trunk-quilt.orig/bfd/elflink.c	2012-07-19 00:13:38.630594071 +0100
+++ binutils-fsf-trunk-quilt/bfd/elflink.c	2012-07-19 00:20:21.130618721 +0100
@@ -1210,23 +1210,25 @@ _bfd_elf_merge_symbol (bfd *abfd,
 	      vh->root.type = h->root.type;
 	      h->root.type = bfd_link_hash_indirect;
 	      (*bed->elf_backend_copy_indirect_symbol) (info, vh, h);
-	      /* Protected symbols will override the dynamic definition
-		 with default version.  */
-	      if (ELF_ST_VISIBILITY (sym->st_other) == STV_PROTECTED)
+
+	      h->root.u.i.link = (struct bfd_link_hash_entry *) vh;
+	      if (ELF_ST_VISIBILITY (sym->st_other) != STV_PROTECTED)
 		{
-		  h->root.u.i.link = (struct bfd_link_hash_entry *) vh;
-		  vh->dynamic_def = 1;
-		  vh->ref_dynamic = 1;
+		  /* If the new symbol is hidden or internal, completely undo
+		     any dynamic link state.  */
+		  (*bed->elf_backend_hide_symbol) (info, h, TRUE);
+		  h->forced_local = 0;
+		  h->ref_dynamic = 0;
 		}
 	      else
-		{
-		  h->root.type = vh->root.type;
-		  vh->ref_dynamic = 0;
-		  /* We have to hide it here since it was made dynamic
-		     global with extra bits when the symbol info was
-		     copied from the old dynamic definition.  */
-		  (*bed->elf_backend_hide_symbol) (info, vh, TRUE);
-		}
+		h->ref_dynamic = 1;
+
+	      h->def_dynamic = 0;
+	      h->dynamic_def = 0;
+	      /* FIXME: Should we check type and size for protected symbol?  */
+	      h->size = 0;
+	      h->type = 0;
+
 	      h = vh;
 	    }
 	  else

Re: [PING] LD: Hidden/internal versioned symbol preemption bug

[H.J. Lu]

On Mon, Aug 13, 2012 at 2:40 PM, Maciej W. Rozycki
<macro@codesourcery.com> wrote:
> Hi,
>
>  Is there anything I could do to help with the review of this change?
>
>  It is a pretty fatal failure affecting all targets, so I would like to
> see it fixed in the upcoming release which I have been told is happening
> by the end of this month and I'll be disappearing for a fortnight starting
> from Wednesday.
>
>  Included below verbatim for easy reference and the complete series posted
> here:
>
> http://sourceware.org/ml/binutils/2012-08/msg00159.html
> http://sourceware.org/ml/binutils/2012-08/msg00160.html
> http://sourceware.org/ml/binutils/2012-08/msg00161.html
>

May I suggest you open a bug report with a testcase and
resubmit your patch with the testcase to fix the bug?

Thanks.

-- 
H.J.

Re: [PING] LD: Hidden/internal versioned symbol preemption bug

[Alan Modra]

On Mon, Aug 13, 2012 at 10:40:35PM +0100, Maciej W. Rozycki wrote:
> 	* elflink.c (_bfd_elf_merge_symbol): Also override the version
> 	a dynamic symbol defaulted to if preempted with a hidden or
> 	internal definition.

This is OK.  What we had before was a bit of a mess.  The followup
testsuite and ppc patches are OK too.

-- 
Alan Modra
Australia Development Lab, IBM

Re: [PING] LD: Hidden/internal versioned symbol preemption bug

[Maciej W. Rozycki]

On Sun, 19 Aug 2012, Alan Modra wrote:

> > 	* elflink.c (_bfd_elf_merge_symbol): Also override the version
> > 	a dynamic symbol defaulted to if preempted with a hidden or
> > 	internal definition.
> 
> This is OK.  What we had before was a bit of a mess.  The followup
> testsuite and ppc patches are OK too.

 Thanks for your review, I have applied all the three patches now.

  Maciej