Bug 21884

Summary: [2.29/2.30 Regression] ld segfaulting building memtest86
Product: binutils Reporter: Matthias Klose <doko>
Component: ldAssignee: Not yet assigned to anyone <unassigned>
Status: RESOLVED FIXED    
Severity: normal CC: hjl.tools, matz, nickc
Priority: P2    
Version: 2.29   
Target Milestone: 2.30   
Host: Target:
Build: Last reconfirmed:
Attachments: test case
A patch
test case 2
complete 2nd test case

Description Matthias Klose 2017-08-02 06:27:55 UTC
Created attachment 10309 [details]
test case

[forwarded from https://bugs.debian.org/870343]

seen with the 2.29 branch on x86_64-linux-gnu, 2.28 works fine.

ld -T memtest.bin.lds bootsect.o setup.o -b binary memtest_shared.bin -o memtest.bin
Makefile:46: recipe for target 'memtest.bin' failed
make[1]: *** [memtest.bin] Segmentation fault
Comment 1 Matthias Klose 2017-08-02 06:44:03 UTC
Program received signal SIGSEGV, Segmentation fault.
0x000055555558d9c9 in gldelf_x86_64_place_orphan (s=0x5555558c73d0, secname=0x5555558c72f3 ".text", 
    constraint=382) at eelf_x86_64.c:1991
1991                && (elf_section_data (os->bfd_section)->this_hdr.sh_info
(gdb) bt
#0  0x000055555558d9c9 in gldelf_x86_64_place_orphan (s=0x5555558c73d0, secname=0x5555558c72f3 ".text", 
    constraint=382) at eelf_x86_64.c:1991
#1  0x000055555558354d in ldemul_place_orphan (s=0x5555558c73d0, name=0x5555558c72f3 ".text", constraint=0)
    at ../../ld/ldemul.c:124
#2  0x0000555555577562 in ldlang_place_orphan (s=0x5555558c73d0) at ../../ld/ldlang.c:6376
#3  0x0000555555577749 in lang_place_orphans () at ../../ld/ldlang.c:6433
#4  0x0000555555578715 in lang_process () at ../../ld/ldlang.c:7147
#5  0x000055555557cb8f in main (argc=10, argv=0x7fffffffe528) at ../../ld/ldmain.c:437
Comment 2 H.J. Lu 2017-08-02 10:33:24 UTC
[hjl@gnu-tools-1 pr21884]$ cat x.t
OUTPUT_FORMAT("binary")

ENTRY(_main);
SECTIONS {
	. = 0;
	.setup : { *(.setup) }
}
[hjl@gnu-tools-1 pr21884]$ cat x.s
	.text
	.globl	_main
	.type _main,%function
_main:
	.dc.a bar
[hjl@gnu-tools-1 pr21884]$ cat y.s
	.text
	.globl	bar
	.type bar,%function
bar:
	.byte 0
[hjl@gnu-tools-1 pr21884]$ make
as   -o x.o x.s
as   -o y.o y.s
./ld  -T x.t -o x x.o y.o
make: *** [Makefile:13: x] Segmentation fault (core dumped)
make: *** Deleting file 'x'
[hjl@gnu-tools-1 pr21884]$
Comment 3 cvs-commit@gcc.gnu.org 2017-08-02 10:46:28 UTC
The master branch has been updated by Nick Clifton <nickc@sourceware.org>:

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

commit ddff3d84be42fa80c2c9aaa635f2b9269e74e4f9
Author: Nick Clifton <nickc@redhat.com>
Date:   Wed Aug 2 11:45:05 2017 +0100

    Fix seg-fault when trying to place non-ELF orphan sections.
    
    	PR 21884
    	* emultempl/elf32.em (_place_orphan): Skip non-ELF binaries when
    	looking for sections to merge.
Comment 4 Nick Clifton 2017-08-02 10:52:39 UTC
Hi Matthias,

  Thanks for reporting this problem.  I have checked in a patch to fix the
  issue - the orphan section placement code was assuming that it was
  dealing with ELF format sections - and the test now succeeds.

  H.J. - if you have the time, would you mind converting your testcase 
  into a new linker testsuite test ?  Thanks.

Cheers
  Nick
Comment 5 H.J. Lu 2017-08-02 11:48:03 UTC
Created attachment 10310 [details]
A patch
Comment 6 H.J. Lu 2017-08-02 11:49:35 UTC
(In reply to Nick Clifton from comment #4)
> Hi Matthias,
> 
>   Thanks for reporting this problem.  I have checked in a patch to fix the
>   issue - the orphan section placement code was assuming that it was
>   dealing with ELF format sections - and the test now succeeds.
> 
>   H.J. - if you have the time, would you mind converting your testcase 
>   into a new linker testsuite test ?  Thanks.
> 

I prefer a different patch in comment 5, which also includes a testcase.
There is no need to check ELF section header at all if output isn't ELF.
Comment 7 Nick Clifton 2017-08-02 12:07:09 UTC
Hi H.J.,

> I prefer a different patch in comment 5, which also includes a testcase.
> There is no need to check ELF section header at all if output isn't ELF.

That makes sense.  I have approved your patch.

Cheers
  Nick
Comment 8 cvs-commit@gcc.gnu.org 2017-08-02 12:12:39 UTC
The master branch has been updated by H.J. Lu <hjl@sourceware.org>:

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

commit db99ecc08f5b66fbe9cb72e90352c7f77ec71a6e
Author: H.J. Lu <hjl.tools@gmail.com>
Date:   Wed Aug 2 05:10:29 2017 -0700

    Check ELF section header only for ELF output
    
    When placing an orphan input section, check ELF section header only for
    ELF output.
    
    	PR ld/21884
    	* emultempl/elf32.em (gld${EMULATION_NAME}_place_orphan): Check
    	ELF section header only for ELF output.
    	* testsuite/ld-elf/pr21884.d: New test.
    	* testsuite/ld-elf/pr21884.t: Likewise.
    	* testsuite/ld-elf/pr21884a.s: Likewise.
    	* testsuite/ld-elf/pr21884b.s: Likewise.
Comment 9 H.J. Lu 2017-08-02 12:19:02 UTC
Fixed for 2.30 and 2.29 branch.
Comment 10 cvs-commit@gcc.gnu.org 2017-08-02 12:19:10 UTC
The binutils-2_29-branch branch has been updated by H.J. Lu <hjl@sourceware.org>:

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

commit a388b7afeffad6411686d39dc1c62294da48a814
Author: H.J. Lu <hjl.tools@gmail.com>
Date:   Wed Aug 2 05:10:29 2017 -0700

    Check ELF section header only for ELF output
    
    When placing an orphan input section, check ELF section header only for
    ELF output.
    
    	PR ld/21884
    	* emultempl/elf32.em (gld${EMULATION_NAME}_place_orphan): Check
    	ELF section header only for ELF output.
    	* testsuite/ld-elf/pr21884.d: New test.
    	* testsuite/ld-elf/pr21884.t: Likewise.
    	* testsuite/ld-elf/pr21884a.s: Likewise.
    	* testsuite/ld-elf/pr21884b.s: Likewise.
    
    (cherry picked from commit db99ecc08f5b66fbe9cb72e90352c7f77ec71a6e)
Comment 11 cvs-commit@gcc.gnu.org 2017-08-03 05:58:54 UTC
The master branch has been updated by Alan Modra <amodra@sourceware.org>:

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

commit 2e9246e077cdbbff0a56a810c5750cc895382ba3
Author: Alan Modra <amodra@gmail.com>
Date:   Thu Aug 3 14:01:34 2017 +0930

    ELF checks for orphan placement
    
    The loop checking for previous orphan placement should run even when
    the output is non-ELF.
    
    	PR ld/21884
    	* emultempl/elf32.em (gld${EMULATION_NAME}_place_orphan): Revert
    	last change.  Rename iself to elfinput.  Expand comments.  Condition
    	ELF checks on having both input and output ELF files.  Extract..
    	(elf_orphan_compatible): ..this new function.
Comment 12 cvs-commit@gcc.gnu.org 2017-08-03 08:49:08 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=36088682f447540fd8666a2c437fa232064044a7

commit 36088682f447540fd8666a2c437fa232064044a7
Author: Alan Modra <amodra@gmail.com>
Date:   Thu Aug 3 14:01:34 2017 +0930

    ELF checks for orphan placement
    
    The loop checking for previous orphan placement should run even when
    the output is non-ELF.
    
    	PR ld/21884
    	* emultempl/elf32.em (gld${EMULATION_NAME}_place_orphan): Revert
    	last change.  Rename iself to elfinput.  Expand comments.  Condition
    	ELF checks on having both input and output ELF files.  Extract..
    	(elf_orphan_compatible): ..this new function.
Comment 13 cvs-commit@gcc.gnu.org 2017-08-03 11:01:30 UTC
The master branch has been updated by Nick Clifton <nickc@sourceware.org>:

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

commit fd9770d81fba7ad860e8bcfbc13c77f21041f1f3
Author: Nick Clifton <nickc@redhat.com>
Date:   Thu Aug 3 11:59:32 2017 +0100

    Add more targets to the list of architectures not supporting format changing during linking.  Fix seg-faults triggered when this is attempted.
    
    	PR ld/21884
    	* testsuite/ld-elf/pr21884.d: Add AVR, HPPA, IA64, M68HC1x and
    	SCORE to list of targets not supporting file format changes during
    	linking.
    	* testsuite/ld-unique/pr21529.d: Likewise.
    	* emultempl/avrelf.em (_before_allocation): Skip for non-ELF
    	output formats.
    	(avr_elf_create_output_section_statements): Fail if the output
    	format is not ELF.
    	(avr_finish): Do not access the ELF header in non-ELF format
    	output bfds.
    	* emultempl/m68hc1xelf.em (_before_allocation): Skip for non-ELF
    	output formats.
    	(m68hc11elf_create_output_section_statements): Fail if the putput
    	format is not ELF.
    	(m68hc11elf_after_allocation): Skip for non-ELF output formats.
Comment 14 Nick Clifton 2017-08-03 11:04:00 UTC
Hi Guys,

  It turns out that there are several other targets that do not support
  changing the output format whilst linking.  In fact two of them - AVR
  and M68HC1x - trigger seg-faults if this is attempted.  So I have
  updated the linker tests that use this feature and also fixed the 
  seg-faults.

Cheers
  Nick
Comment 15 Matthias Klose 2017-08-04 11:37:54 UTC
there's are report of another segfault: https://bugs.debian.org/870611
Comment 16 Matthias Klose 2017-08-04 11:39:25 UTC
Created attachment 10314 [details]
test case 2
Comment 17 Matthias Klose 2017-08-04 11:55:43 UTC
Program received signal SIGSEGV, Segmentation fault.
0x00007ffff7ad23a7 in _bfd_elf_create_got_section (abfd=0x5555558c7050, 
    info=0x5555558aa280 <link_info>) at ../../bfd/elflink.c:158
158       flags = bed->dynamic_sec_flags;
(gdb) bt
#0  0x00007ffff7ad23a7 in _bfd_elf_create_got_section (abfd=0x5555558c7050, 
    info=0x5555558aa280 <link_info>) at ../../bfd/elflink.c:158
#1  0x00007ffff7b12ede in elf_i386_link_setup_gnu_properties (
    info=0x5555558aa280 <link_info>) at ../../bfd/elf32-i386.c:7025
#2  0x000055555558c2ec in gldelf_x86_64_after_open () at eelf_x86_64.c:1169
#3  0x0000555555583403 in ldemul_after_open () at ../../ld/ldemul.c:64
#4  0x000055555557866c in lang_process () at ../../ld/ldlang.c:7093
#5  0x000055555557cb8f in main (argc=9, argv=0x7fffffffe548)
    at ../../ld/ldmain.c:437
Comment 18 Matthias Klose 2017-08-04 11:57:04 UTC
Created attachment 10315 [details]
complete 2nd test case
Comment 19 H.J. Lu 2017-08-04 12:30:51 UTC
(In reply to Matthias Klose from comment #18)
> Created attachment 10315 [details]
> complete 2nd test case

It works for me:

[hjl@gnu-tools-1 tst2]$ ./ld  -T memtest.bin.lds bootsect.o setup.o -b binary memtest_shared.bin -o memtest.bin
[hjl@gnu-tools-1 tst2]$ ./ld -V
GNU ld (GNU Binutils) 2.29.51.20170804
  Supported emulations:
   elf_x86_64
   elf32_x86_64
   elf_i386
   elf_iamcu
   i386linux
   elf_l1om
   elf_k1om
[hjl@gnu-tools-1 tst2]$
Comment 20 Matthias Klose 2017-08-04 13:52:39 UTC
hmm, I still can reproduce this with a plain build from the 2.29 branch.
Comment 21 H.J. Lu 2017-08-04 14:42:57 UTC
(In reply to Matthias Klose from comment #20)
> hmm, I still can reproduce this with a plain build from the 2.29 branch.

Please try master branch and download the testcase from Bugzilla to
reproduce it.  BTW, I configured binutils on x86-64 with

	--enable-plugins --with-sysroot=/ --with-system-zlib --disable-gdb --dis
able-libdecnumber --disable-readline --disable-sim \
	--prefix=/usr/local \
	--with-local-prefix=/usr/local
Comment 22 Matthias Klose 2017-08-04 17:51:49 UTC
same with today's trunk 20170804. binutils is configured with
--with-sysroot=/ --enable-shared --enable-plugins --enable-threads --with-system-zlib --prefix=/usr --enable-deterministic-archives --disable-compressed-debug-sections --enable-new-dtags --build=x86_64-linux-gnu --host=x86_64-linux-gnu --with-pkgversion="GNU Binutils for Debian" --disable-werror --enable-targets=x86_64-linux-gnux32,x86_64-pep --enable-ld=default --enable-gold

the gcc used to build is configured to default to pie.
Comment 23 H.J. Lu 2017-08-04 18:13:01 UTC
(In reply to Matthias Klose from comment #22)
> same with today's trunk 20170804. binutils is configured with
> --with-sysroot=/ --enable-shared --enable-plugins --enable-threads
> --with-system-zlib --prefix=/usr --enable-deterministic-archives
> --disable-compressed-debug-sections --enable-new-dtags
> --build=x86_64-linux-gnu --host=x86_64-linux-gnu --with-pkgversion="GNU
> Binutils for Debian" --disable-werror
> --enable-targets=x86_64-linux-gnux32,x86_64-pep --enable-ld=default
> --enable-gold
> 
> the gcc used to build is configured to default to pie.

I still can't reproduce it with the same configure and GCC 7.1 default to PIE.
I am going to close it.  Please open a new bug with a fresh testcase and
instructions how to reproduce it.
Comment 24 cvs-commit@gcc.gnu.org 2017-08-07 09:11:06 UTC
The master branch has been updated by Nick Clifton <nickc@sourceware.org>:

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

commit b7a18930e3925c4092bd975e95bc3603aa1418d9
Author: Nick Clifton <nickc@redhat.com>
Date:   Mon Aug 7 10:09:51 2017 +0100

    Do not choose a non-ELF format input file to hold the linker created GOT sections.
    
    	PR 21884
    	* elf32-i386.c (elf_i386_link_setup_gnu_properties): If the dynobj
    	has not been set then use the bfd returned by
    	_bfd_elf_link_setup_gnu_properties.  If that is null then search
    	through all the input bfds selecting the first normal, ELF format
    	one.
    	* elf64-x86-64.c (elf_x86_64_link_setup_gnu_properties): Likewise.
Comment 25 Nick Clifton 2017-08-07 09:15:26 UTC
Hi Guys,

I was able to reproduce the problem.  The issue is that elf_i386_link_setup_gnu_properties was selecting a non-ELF format bfd to hold the linker created GOT sections.  The problem did not happen with the x86_64 target because that used the bfd returned by _bfd_elf_link_setup_gnu_properties.

I have applied a patch that updates the i386 version so that it too will use the bfd returned by _bfd_elf_link_setup_gnu_properties, if there is one.  If not, it will scan the input bfds like before, but it will skip any non-ELF format ones.
(I am not sure if this scan will ever be needed now, but better to be safe than sorry).

Likewise, the patch updates the x86_64 version so that if it does have to perform its own scan of the input bfds, non-ELF format ones are ignored.

Cheers
  Nick
Comment 26 H.J. Lu 2017-08-07 12:48:36 UTC
(In reply to Nick Clifton from comment #25)
> Hi Guys,
> 
> I was able to reproduce the problem.  The issue is that
> elf_i386_link_setup_gnu_properties was selecting a non-ELF format bfd to
> hold the linker created GOT sections.  The problem did not happen with the
> x86_64 target because that used the bfd returned by
> _bfd_elf_link_setup_gnu_properties.
> 

Can you show me how to reproduce the issue? I'd like to add a testcase.
Comment 27 Nick Clifton 2017-08-08 10:09:57 UTC
(In reply to H.J. Lu from comment #26)
 
> Can you show me how to reproduce the issue?

No. :-(  I tried reverting my patch and the reproducing the failure, but I
could not.  It was very strange.  So I checked further and it seems that the
code that I patched - the ..._setup_gnu_properties() functions - should
never be called on non-ELF input files.  So now I am confused as to how I 
ever reproduced the problem in the first place.

Somehow the elf_i386_link_setup_gnu_property function was being called, but
I do not know how, and I can no longer make it happen.  Sorry.

Cheers
  Nick
Comment 28 Michael Matz 2017-08-11 13:42:08 UTC
This still reproduces for me, on i586 and on x86_64.  I don't need to build
binutils in any special way, on my host x86_64, with gcc 4.7:

% git status
# On branch binutils-2_29-branch
...
nothing added to commit but untracked files present (use "git add" to track)
% mkdir dev && cd dev
% ../configure --disable-gdb
% make CFLAGS=-g -j8
...
% echo "" > foo
% cat memtest.lds
OUTPUT_FORMAT("elf32-i386");
OUTPUT_ARCH(i386);

ENTRY(_start); 
SECTIONS {
        . = 0x10000;
        _start = . ;
        .data : {
                *(.data)
        }
}
% ./ld/ld-new -T memtest.lds -b binary foo -o memtest

valgrind shows me this backtrace:

==3890== Invalid read of size 4
==3890==    at 0x49386B: _bfd_elf_create_got_section (elflink.c:158)
==3890==    by 0x4D3B62: elf_i386_link_setup_gnu_properties (elf32-i386.c:7025)
==3890==    by 0x42C393: gldelf_x86_64_after_open (eelf_x86_64.c:1169)
==3890==    by 0x42373C: ldemul_after_open (ldemul.c:64)
==3890==    by 0x418E3D: lang_process (ldlang.c:7093)
==3890==    by 0x41D1E5: main (ldmain.c:437)
==3890==  Address 0x28 is not stack'd, malloc'd or (recently) free'd

So yes, the elf_i386_link_setup_gnu_properties function is most definitely
called.
Comment 29 Michael Matz 2017-08-11 13:53:44 UTC
And commit b7a18930 from Nick fixed it on master for a x86_64 hosted binutils (I'm qualifying this because I haven't checked a i586 hosted binutils yet).

So at the very minimum this patch needs to be on the 2.29 branch as well.

As testcase my comment #28 should suffice.
Comment 30 cvs-commit@gcc.gnu.org 2017-08-11 15:07:38 UTC
The master branch has been updated by H.J. Lu <hjl@sourceware.org>:

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

commit 9593aade74f0da0c08a4ab55e4c59173b07b1f63
Author: H.J. Lu <hjl.tools@gmail.com>
Date:   Fri Aug 11 08:05:00 2017 -0700

    Add 2 more tests for PR ld/21884
    
    	PR ld/21884
    	* testsuite/ld-i386/i386.exp: Run pr21884.
    	* testsuite/ld-x86-64/x86-64.exp: Likewise.
    	* testsuite/ld-i386/pr21884.d: New file.
    	* testsuite/ld-i386/pr21884.t: Likewise.
    	* testsuite/ld-x86-64/pr21884.d: Likewise.
    	* testsuite/ld-x86-64/pr21884.t: Likewise.
Comment 31 cvs-commit@gcc.gnu.org 2017-08-11 15:19:01 UTC
The binutils-2_29-branch branch has been updated by H.J. Lu <hjl@sourceware.org>:

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

commit 4a1c35c0165f9cbcc63d3af93574b2f6c4544eb7
Author: Nick Clifton <nickc@redhat.com>
Date:   Mon Aug 7 10:09:51 2017 +0100

    Do not choose a non-ELF format input file to hold the linker created GOT sections.
    
    bfd/
    
    	PR 21884
    	* elf32-i386.c (elf_i386_link_setup_gnu_properties): If the dynobj
    	has not been set then use the bfd returned by
    	_bfd_elf_link_setup_gnu_properties.  If that is null then search
    	through all the input bfds selecting the first normal, ELF format
    	one.
    	* elf64-x86-64.c (elf_x86_64_link_setup_gnu_properties): Likewise.
    
    ld/
    
    	PR ld/21884
    	* testsuite/ld-i386/i386.exp: Run pr21884.
    	* testsuite/ld-x86-64/x86-64.exp: Likewise.
    	* testsuite/ld-i386/pr21884.d: New file.
    	* testsuite/ld-i386/pr21884.t: Likewise.
    	* testsuite/ld-x86-64/pr21884.d: Likewise.
    	* testsuite/ld-x86-64/pr21884.t: Likewise.
    
    (cherry picked from commit b7a18930e3925c4092bd975e95bc3603aa1418d9 and
     9593aade74f0da0c08a4ab55e4c59173b07b1f63)
Comment 32 H.J. Lu 2017-08-11 15:21:26 UTC
Fixed for master and 2.29 branch.
Comment 33 cvs-commit@gcc.gnu.org 2017-08-23 03:26: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=7476906bc53665adfef4c4598774584eb34cc5af

commit 7476906bc53665adfef4c4598774584eb34cc5af
Author: Nick Clifton <nickc@redhat.com>
Date:   Thu Aug 3 11:59:32 2017 +0100

    Add more targets to the list of architectures not supporting format changing during linking.  Fix seg-faults triggered when this is attempted.
    
    	PR ld/21884
    	* testsuite/ld-elf/pr21884.d: Add AVR, HPPA, IA64, M68HC1x and
    	SCORE to list of targets not supporting file format changes during
    	linking.
    	* testsuite/ld-unique/pr21529.d: Likewise.
    	* emultempl/avrelf.em (_before_allocation): Skip for non-ELF
    	output formats.
    	(avr_elf_create_output_section_statements): Fail if the output
    	format is not ELF.
    	(avr_finish): Do not access the ELF header in non-ELF format
    	output bfds.
    	* emultempl/m68hc1xelf.em (_before_allocation): Skip for non-ELF
    	output formats.
    	(m68hc11elf_create_output_section_statements): Fail if the putput
    	format is not ELF.
    	(m68hc11elf_after_allocation): Skip for non-ELF output formats.