Bug 16900

Summary: [PATCH] when linked with gold, NetBSD ld.elf_so crashes due to missing .plt.got entry
Product: binutils Reporter: Ben Gras <ben>
Component: goldAssignee: Cary Coutant <ccoutant>
Status: RESOLVED FIXED    
Severity: normal CC: ccoutant
Priority: P2    
Version: 2.25   
Target Milestone: ---   
Host: Target:
Build: Last reconfirmed:
Attachments: .tar file with script to reproduce the problem; .o files (as the script generates on my end); executables (as the script generates on my end); patch file with suggested solution.

Description Ben Gras 2014-05-04 01:59:06 UTC
Created attachment 7585 [details]
.tar file with script to reproduce the problem; .o files (as the script generates on my end); executables (as the script generates on my end); patch file with suggested solution.

Hi, I'm Ben, from the Minix project (www.minix3.org). Pleased to meet you.

We're using Gold to link (while crossbuilding), among other things, some NetBSD code as part of the Minix build.

This bug is about a problem I can reproduce on unmodified binutils HEAD, 19a170752b with a small test case.

Observations:
  - NetBSD ld.elf_so linked using gold crashes.
  - it turns out ld.elf_so mis-computes the relocbase at startup time.
  - it turns out due to _DYNAMIC ptr not being stored correctly in the GOT.
  - when linking a shared object (the ld.so executable in my case), the first
    entry of .plt.got was 0, where ld.so expects the _DYNAMIC pointer
  - all static executables linked with gold are OK
  - the exact same ld invocation and inputs with bfd-ld results in an OK binary

How to reproduce:
  - see compile.sh script in the attached tar; it is a self-contained, fairly minimal test case to reproduce the problem on my end (native gcc & gold on ubuntu).

 I did the minimum amount of digging to solve the problem for us. Therefore the analysis and patch may be crude. Nevertheless, here they are:

 Analysis:
  - it seems .plt.got is only initialized in gold if there are unresolved external
    references; with -Bsymbolic, which ld.so is linked with, these can all vanish
    and so .plt.got is left uninitialized.

Suggested solution:
  - in patch: as long as there is a global_offset_table_, make sure there is
    a .plt.got initialized to contain it in do_finalize_sections().

The current result of readelf -x .got.plt:
   0x0000128c 00000000 00000000 00000000          ............

with the patch:
   0x000012c4 54120000 00000000 00000000          T...........

With matching nm output for _DYNAMIC:
   00001254 d _DYNAMIC

Attached: .tar file with
  - script to reproduce the problem by compiling and linking 2 C files, with patch and info in comments
  - the generated object files on my side to eliminate compiling dependencies
  - the 2 resulting ('wrong' and 'right') binaries after linking on my side
  - 0001-gold-i386-fix-missing-_DYNAMIC-ptr-in-.plt.got.patch: patchfile containing suggested fix. looks like this:

commit e940b61beb39318c0c457ce85dff19a40a3e3e2f
Author: Ben Gras <ben@minix3.org>
Date:   Sat May 3 19:40:19 2014 +0200

    gold i386: fix missing _DYNAMIC ptr in .plt.got
    
    In some circumstances (e.g. -Bsymbolic and no external references)
    make_plt_section() isn't triggered leaving it empty, while sometimes
    still needed for shared objects.
    
        gold/
            * i386.cc (Target_i386::do_finalize_sections): add call to make_plt_section()
              if there is no plt_ but there is a GOT, so that the first entry is always
              initialized.

diff --git a/gold/i386.cc b/gold/i386.cc
index a2f7522..f06d60d 100644
--- a/gold/i386.cc
+++ b/gold/i386.cc
@@ -2543,6 +2543,10 @@ Target_i386::do_finalize_sections(
   Symbol* sym = this->global_offset_table_;
   if (sym != NULL)
     {
+      // create a plt
+      if (this->plt_ == NULL)
+        this->make_plt_section(symtab, layout);
+
       uint32_t data_size = this->got_plt_->current_data_size();
       symtab->get_sized_symbol<32>(sym)->set_symsize(data_size);
     }



Again, the patch and/or the description in the commit may be too crude, it's just as far as I've gotten understanding gold (and ELF for that matter).
Comment 1 Cary Coutant 2014-05-06 21:26:59 UTC
Thanks for the diagnosis. You're right that the problem is that the GOT is initialized only when there is a PLT, but it shouldn't be necessary to forcibly create a PLT that would otherwise be empty. I've fixed this by changing the .got.plt section to have its own class derived from Output_section_data_build, so that its own do_write() method can write those reserved entries, but I leave the remainder of the section to be written when the PLT is written.
Comment 2 Sourceware Commits 2014-05-06 21:30:55 UTC
This is an automated email from the git hooks/post-receive script. It was
generated because a ref change was pushed to the repository containing
the project "gdb and binutils".

The branch, master has been updated
       via  57b2284c6318841612acef23c60fe7298a1ac91a (commit)
      from  757a636fb585824699b1cc4f8f23dbc3a6a6d914 (commit)

Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.

- Log -----------------------------------------------------------------
https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=57b2284c6318841612acef23c60fe7298a1ac91a

commit 57b2284c6318841612acef23c60fe7298a1ac91a
Author: Cary Coutant <ccoutant@google.com>
Date:   Tue May 6 11:40:04 2014 -0700

    Fix issue where first reserved word of GOT is not initialized if there
    is no PLT.
    
    gold/
    	PR gold/16900
    	* i386.cc (Output_data_got_plt_i386): New class.
    	(Output_data_plt_i386::Output_data_plt_i386): Change type of got_plt
    	parameter. Change all callers.
    	(Output_data_plt_i386::layout_): Remove.
    	(Output_data_plt_i386::got_plt_): Change type.
    	(Target_i386::got_plt_): Change type. Change all references.
    	(Target_i386::got_section): Create instance of new class.
    	(Output_data_got_plt_i386::do_write): New function.
    	* x86_64.cc (Output_data_got_plt_x86_64): New class.
    	(Output_data_plt_x86_64::Output_data_plt_x86_64): Change type of got_plt
    	parameter. Change all callers.
    	(Output_data_plt_x86_64::layout_): Remove.
    	(Output_data_plt_x86_64::got_plt_): Change type.
    	(Target_x86_64::got_plt_): Change type. Change all references.
    	(Target_x86_64::got_section): Create instance of new class.
    	(Output_data_got_plt_x86_64::do_write): New function.
    	(Output_data_plt_x86_64::do_write): Don't write reserved words in GOT.
    	(Target_x86_64<size>::init_got_plt_for_update): Create instance of new
    	class.

-----------------------------------------------------------------------

Summary of changes:
 gold/ChangeLog |   23 ++++++++++++
 gold/i386.cc   |  102 +++++++++++++++++++++++++++++++++++-----------------
 gold/x86_64.cc |  110 ++++++++++++++++++++++++++++++++++++++------------------
 3 files changed, 167 insertions(+), 68 deletions(-)
Comment 3 Sourceware Commits 2014-05-06 21:49:19 UTC
This is an automated email from the git hooks/post-receive script. It was
generated because a ref change was pushed to the repository containing
the project "gdb and binutils".

The branch, binutils-2_24-branch has been updated
       via  6ce193a2807a10a53c1883e573f0ea5e03fbff76 (commit)
       via  367f1035d2f7da6fbad69f659208bdb7da3d55dc (commit)
      from  a9534d052d7714778fa615974c4a8115fc973420 (commit)

Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.

- Log -----------------------------------------------------------------
https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=6ce193a2807a10a53c1883e573f0ea5e03fbff76

commit 6ce193a2807a10a53c1883e573f0ea5e03fbff76
Author: Cary Coutant <ccoutant@google.com>
Date:   Tue May 6 11:40:04 2014 -0700

    Fix issue where first reserved word of GOT is not initialized if there
    is no PLT.
    
    gold/
    	PR gold/16900
    	* i386.cc (Output_data_got_plt_i386): New class.
    	(Output_data_plt_i386::Output_data_plt_i386): Change type of got_plt
    	parameter. Change all callers.
    	(Output_data_plt_i386::layout_): Remove.
    	(Output_data_plt_i386::got_plt_): Change type.
    	(Target_i386::got_plt_): Change type. Change all references.
    	(Target_i386::got_section): Create instance of new class.
    	(Output_data_got_plt_i386::do_write): New function.
    	* x86_64.cc (Output_data_got_plt_x86_64): New class.
    	(Output_data_plt_x86_64::Output_data_plt_x86_64): Change type of got_plt
    	parameter. Change all callers.
    	(Output_data_plt_x86_64::layout_): Remove.
    	(Output_data_plt_x86_64::got_plt_): Change type.
    	(Target_x86_64::got_plt_): Change type. Change all references.
    	(Target_x86_64::got_section): Create instance of new class.
    	(Output_data_got_plt_x86_64::do_write): New function.
    	(Output_data_plt_x86_64::do_write): Don't write reserved words in GOT.
    	(Target_x86_64<size>::init_got_plt_for_update): Create instance of new
    	class.

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

commit 367f1035d2f7da6fbad69f659208bdb7da3d55dc
Author: Cary Coutant <ccoutant@google.com>
Date:   Wed Apr 2 14:21:14 2014 -0700

    Fix handling of __ehdr_start when it cannot be defined.
    
    gold/
    	* defstd.cc (in_segment): Define __ehdr_start here...
    	* layout.cc (Layout::finalize): ...Instead of here.  Set the
    	output segment when known.
    	* resolve.cc (Symbol::override_base_with_special): Remember
    	the original binding.
    	* symtab.cc (Symbol::set_output_segment): New function.
    	(Symbol::set_undefined): New function.
    	* symtab.h (Symbol::is_weak_undefined): Check original undef
    	binding.
    	(Symbol::is_strong_undefined): New function.
    	(Symbol::set_output_segment): New function.
    	(Symbol::set_undefined): New function.
    	* target-reloc.h (is_strong_undefined): Remove.
    	(issue_undefined_symbol_error): Call Symbol::is_weak_undefined.
    	Check for hidden undefs.
    	(relocate_section): Call Symbol::is_strong_undefined.
    
    	* testsuite/Makefile.am (ehdr_start_test_1)
    	(ehdr_start_test_2, ehdr_start_test_3)
    	(ehdr_start_test_4, ehdr_start_test_5): New test cases.
    	* testsuite/Makefile.in: Regenerate.
    	* testsuite/ehdr_start_def.cc: New source file.
    	* testsuite/ehdr_start_test.cc: New source file.
    	* testsuite/ehdr_start_test.t: New linker script.
    	* testsuite/ehdr_start_test_4.sh: New shell script.

-----------------------------------------------------------------------

Summary of changes:
 gold/ChangeLog                      |   51 ++++++++
 gold/defstd.cc                      |   14 ++
 gold/i386.cc                        |  102 +++++++++++-----
 gold/layout.cc                      |   14 ++-
 gold/resolve.cc                     |    4 +
 gold/symtab.cc                      |   25 ++++
 gold/symtab.h                       |   34 +++++-
 gold/target-reloc.h                 |    8 +-
 gold/testsuite/Makefile.am          |   44 +++++++
 gold/testsuite/Makefile.in          |  223 +++++++++++++++++++++++++++++++----
 gold/testsuite/ehdr_start_def.cc    |   26 ++++
 gold/testsuite/ehdr_start_test.cc   |   67 +++++++++++
 gold/testsuite/ehdr_start_test.t    |   42 +++++++
 gold/testsuite/ehdr_start_test_4.sh |   40 ++++++
 gold/x86_64.cc                      |  110 ++++++++++++------
 15 files changed, 699 insertions(+), 105 deletions(-)
 create mode 100644 gold/testsuite/ehdr_start_def.cc
 create mode 100644 gold/testsuite/ehdr_start_test.cc
 create mode 100644 gold/testsuite/ehdr_start_test.t
 create mode 100755 gold/testsuite/ehdr_start_test_4.sh
Comment 4 Cary Coutant 2014-05-06 21:49:51 UTC
Fixed on trunk and backported to binutils-2.24 branch.