Bug 22626 - invalid dynindx used for dynamic relocs against section syms
Summary: invalid dynindx used for dynamic relocs against section syms
Status: RESOLVED FIXED
Alias: None
Product: binutils
Classification: Unclassified
Component: ld (show other bugs)
Version: 2.30
: P2 normal
Target Milestone: ---
Assignee: Alan Modra
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-12-17 20:38 UTC by Sergei Trofimovich
Modified: 2017-12-18 22:45 UTC (History)
1 user (show)

See Also:
Host:
Target: powerpc-unknown-linux-gnu
Build:
Last reconfirmed: 2017-12-17 00:00:00


Attachments
patch under test (1.26 KB, patch)
2017-12-18 12:29 UTC, Alan Modra
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sergei Trofimovich 2017-12-17 20:38:17 UTC
Original bug:
    https://bugs.gentoo.org/641474#c4

The issue manifests as non-working binary when gcc is built with --enable-default-pie.

$ powerpc-unknown-linux-gnu-gcc a.c -o a
/usr/libexec/gcc/powerpc-unknown-linux-gnu/ld: warning: creating a DT_TEXTREL in a shared object.

$ ./a
Segmentation fault

$ readelf -a a
Relocation section '.rela.dyn' at offset 0x2a0 contains 33 entries:
 Offset     Info    Type            Sym.Value  Sym. Name + Addend
...
000004e2  00000906 R_PPC_ADDR16_HA  readelf: Error:  bad symbol index: 00000009 in reloc
000004ea  00000904 R_PPC_ADDR16_LO  readelf: Error:  bad symbol index: 00000009 in reloc
00000512  00000906 R_PPC_ADDR16_HA  readelf: Error:  bad symbol index: 00000009 in reloc
0000051a  00000904 R_PPC_ADDR16_LO  readelf: Error:  bad symbol index: 00000009 in reloc

It looks like .rela.dyn refer to already garbage-collected symbols.

I've bisected it down to the following commit: https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commitdiff;h=6b3b0ab89663306d17927d630304dbbd36e84570

"""Linker assigned symbols should be made dynamic by default only when
creating shared object or relocatable executable."""

Last working: 2.26.1
Tested broken: 2.27, 2.28.1
Comment 1 H.J. Lu 2017-12-17 20:52:12 UTC
Please try binutils master branch.
Comment 2 Sergei Trofimovich 2017-12-17 21:21:37 UTC
Same for master from 

  commit 390c91cfcffe1a0d75b6100d1542cda2544993b3 (HEAD -> master, origin/master, origin/HEAD)
  Author: H.J. Lu <hjl.tools@gmail.com>
  Date:   Sun Dec 17 09:40:54 2017 -0800

    x86: Check pseudo prefix without instruction


built as:
$ ../binutils-gdb/configure --build=x86_64-pc-linux-gnu --host=x86_64-pc-linux-gnu --target=powerpc-unknown-linux-gnu --disable-werror

tested as (exact linker invocation):

./link-dev.sh 
GNU ld (GNU Binutils) 2.29.51.20171217
  Supported emulations:
   elf32ppclinux
   elf32ppc
   elf32ppcsim
   elf32lppclinux
   elf32lppc
   elf32lppcsim
   elf64ppc
   elf64lppc
00000502  00000906 R_PPC_ADDR16_HA  readelf: Error:  bad symbol index: 00000009 in reloc
0000050a  00000904 R_PPC_ADDR16_LO  readelf: Error:  bad symbol index: 00000009 in reloc
00000532  00000906 R_PPC_ADDR16_HA  readelf: Error:  bad symbol index: 00000009 in reloc
0000053a  00000904 R_PPC_ADDR16_LO  readelf: Error:  bad symbol index: 00000009 in reloc

$ cat ./link-dev.sh 
#!/bin/bash

ld/ld-new \
    --sysroot=/usr/powerpc-unknown-linux-gnu \
    --eh-frame-hdr \
    -V \
    --secure-plt \
    -m elf32ppclinux \
    -dynamic-linker /lib/ld.so.1 \
    -pie \
    -o a \
    /usr/powerpc-unknown-linux-gnu/usr/lib/../lib/crt1.o \
    /usr/powerpc-unknown-linux-gnu/usr/lib/../lib/crti.o \
    /usr/lib/gcc/powerpc-unknown-linux-gnu/7.2.0/crtbegin.o \
    -L/usr/lib/gcc/powerpc-unknown-linux-gnu/7.2.0 \
    -L/usr/lib/gcc/powerpc-unknown-linux-gnu/7.2.0/../../../../powerpc-unknown-linux-gnu/lib/../lib \
    -L/usr/powerpc-unknown-linux-gnu/lib/../lib \
    -L/usr/powerpc-unknown-linux-gnu/usr/lib/../lib \
    -L/usr/lib/gcc/powerpc-unknown-linux-gnu/7.2.0/../../../../powerpc-unknown-linux-gnu/lib \
    -L/usr/powerpc-unknown-linux-gnu/lib \
    -L/usr/powerpc-unknown-linux-gnu/usr/lib \
    \
    a.o \
    \
    -lgcc \
    --as-needed -lgcc_s --no-as-needed \
    -lc -lgcc \
    --as-needed -lgcc_s --no-as-needed \
    /usr/lib/gcc/powerpc-unknown-linux-gnu/7.2.0/crtend.o \
    /usr/powerpc-unknown-linux-gnu/usr/lib/../lib/crtn.o

LANG=C readelf -a ./a >log 2>&1
fgrep 'bad symbol index' log
Comment 3 Sergei Trofimovich 2017-12-17 21:25:41 UTC
Original program is:
    int main(){}
Comment 4 H.J. Lu 2017-12-17 22:50:29 UTC
Alan, do you know where the problem is?
Comment 5 Alan Modra 2017-12-18 07:43:38 UTC
If you cannot reproduce the problem with a --disable-default-pie compiler by using the appropriate combination of options (-pie -fPIE), then the problem lies in the compiler.  In comment #2, I see crt1.o and crtbegin.o in the list of object files passed to ld.  That is wrong for a PIE.  It should be Scrt1.o and crtbeginS.o.

The compiler problem should have been fixed with git commit a9b2df6c for gcc master, f5c42d1b for gcc-7-branch, and e764842a for gcc-6-branch.

The PR driver/81523 patches are important too.
Comment 6 Sergei Trofimovich 2017-12-18 08:23:30 UTC
(In reply to Alan Modra from comment #5)
> If you cannot reproduce the problem with a --disable-default-pie compiler by
> using the appropriate combination of options (-pie -fPIE), then the problem
> lies in the compiler.  In comment #2, I see crt1.o and crtbegin.o in the
> list of object files passed to ld.  That is wrong for a PIE.  It should be
> Scrt1.o and crtbeginS.o.
> 
> The compiler problem should have been fixed with git commit a9b2df6c for gcc
> master, f5c42d1b for gcc-7-branch, and e764842a for gcc-6-branch.
> 
> The PR driver/81523 patches are important too.

Aha, thank you!
Comment 7 Sergei Trofimovich 2017-12-18 08:45:30 UTC
(In reply to Alan Modra from comment #5)
> If you cannot reproduce the problem with a --disable-default-pie compiler by
> using the appropriate combination of options (-pie -fPIE), then the problem
> lies in the compiler.  In comment #2, I see crt1.o and crtbegin.o in the
> list of object files passed to ld.  That is wrong for a PIE.  It should be
> Scrt1.o and crtbeginS.o.
> 
> The compiler problem should have been fixed with git commit a9b2df6c for gcc
> master, f5c42d1b for gcc-7-branch, and e764842a for gcc-6-branch.
> 
> The PR driver/81523 patches are important too.

One more question: is it expected that ld generates final binary relocations to nonexisting symbols? I would expect ld to fail to link instead of producing bad binary.
Comment 8 Alan Modra 2017-12-18 09:37:21 UTC
Hmm, no, ld shouldn't be creating those "bad symbol index" relocs.  I'll see if I can break my gcc to recreate the problem..
Comment 9 Alan Modra 2017-12-18 10:57:31 UTC
It's actually a nasty interaction between _bfd_elf_link_renumber_dynsyms, elf_backend_omit_section_dynsym, and strip_excluded_output_sections.
Comment 10 Alan Modra 2017-12-18 12:29:58 UTC
Created attachment 10693 [details]
patch under test

I'll commit this tomorrow if overnight testing goes well
Comment 11 Alan Modra 2017-12-18 12:32:59 UTC
BTW, the change found by the bisection isn't relevant.  It just reduced dynamic symbols so that an invalid index went out of range rather than pointing at a wrong symbol.
Comment 12 Sourceware Commits 2017-12-18 22:15:48 UTC
The master branch has been updated by Alan Modra <amodra@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=63f452a8bfd9c89b56dcc087cea84151e7a9ec24

commit 63f452a8bfd9c89b56dcc087cea84151e7a9ec24
Author: Alan Modra <amodra@gmail.com>
Date:   Mon Dec 18 22:27:08 2017 +1030

    PR22626, invalid dynindx used for dynamic relocs against section syms
    
    _bfd_elf_link_renumber_dynsyms is called twice by the linker.  The
    first call in bfd_elf_size_dynamic_sections is just to answer the
    question as to whether there are there any dynamic symbols.  The
    second call in bfd_elf_size_dynsym_hash_dynstr sets the st_shndx value
    that dynamic symbols will have.  strip_excluded_output_sections is
    called between these two calls.  So sections seen on the first
    _bfd_elf_link_renumber_dynsyms pass might differ from those seen on
    the second pass.  Unfortunately, that can result in a stripped
    section's dynamic symbol being assigned a dynindx on the first pass
    but not corrected to the final value (of zero, ie. not dynamic) on the
    second pass.  PowerPC, x86, mips, and most other targets that emit
    dynamic section symbols, just test that section symbol dynindx is
    non-zero before using a given section symbol in dynamic relocations.
    
    This patch prevents _bfd_elf_link_renumber_dynsyms from setting any
    section symbol dynindx on the first pass.
    
    	PR 22626
    	* elflink.c (_bfd_elf_link_renumber_dynsyms): Don't set section
    	dynindx when section_sym_count is NULL.
    	(bfd_elf_size_dynamic_sections): Pass NULL section_sym_count to
    	preliminary _bfd_elf_link_renumber_dynsyms call.
Comment 13 Sourceware Commits 2017-12-18 22:31:09 UTC
The binutils-2_29-branch branch has been updated by Alan Modra <amodra@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=4207f3a916969991e9c19a6e27ba504d9e42da6e

commit 4207f3a916969991e9c19a6e27ba504d9e42da6e
Author: Alan Modra <amodra@gmail.com>
Date:   Mon Dec 18 22:27:08 2017 +1030

    PR22626, invalid dynindx used for dynamic relocs against section syms
    
    _bfd_elf_link_renumber_dynsyms is called twice by the linker.  The
    first call in bfd_elf_size_dynamic_sections is just to answer the
    question as to whether there are there any dynamic symbols.  The
    second call in bfd_elf_size_dynsym_hash_dynstr sets the st_shndx value
    that dynamic symbols will have.  strip_excluded_output_sections is
    called between these two calls.  So sections seen on the first
    _bfd_elf_link_renumber_dynsyms pass might differ from those seen on
    the second pass.  Unfortunately, that can result in a stripped
    section's dynamic symbol being assigned a dynindx on the first pass
    but not corrected to the final value (of zero, ie. not dynamic) on the
    second pass.  PowerPC, x86, mips, and most other targets that emit
    dynamic section symbols, just test that section symbol dynindx is
    non-zero before using a given section symbol in dynamic relocations.
    
    This patch prevents _bfd_elf_link_renumber_dynsyms from setting any
    section symbol dynindx on the first pass.
    
    	PR 22626
    	* elflink.c (_bfd_elf_link_renumber_dynsyms): Don't set section
    	dynindx when section_sym_count is NULL.
    	(bfd_elf_size_dynamic_sections): Pass NULL section_sym_count to
    	preliminary _bfd_elf_link_renumber_dynsyms call.
    
    (cherry picked from commit 63f452a8bfd9c89b56dcc087cea84151e7a9ec24)
Comment 14 Alan Modra 2017-12-18 22:45:04 UTC
Fixed