Bug 22842 - Handling of R_X86_64_PC32 in a PIE against a function in a shared library could be better
Summary: Handling of R_X86_64_PC32 in a PIE against a function in a shared library cou...
Status: RESOLVED FIXED
Alias: None
Product: binutils
Classification: Unclassified
Component: ld (show other bugs)
Version: 2.31
: P2 normal
Target Milestone: 2.31
Assignee: Not yet assigned to anyone
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-02-13 19:28 UTC by Rafael Ávila de Espíndola
Modified: 2018-12-19 19:52 UTC (History)
3 users (show)

See Also:
Host:
Target:
Build:
Last reconfirmed: 2018-02-13 00:00:00


Attachments
testcase (801.05 KB, application/x-xz)
2018-02-13 19:28 UTC, Rafael Ávila de Espíndola
Details
A patch (1.88 KB, patch)
2018-02-13 21:46 UTC, H.J. Lu
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Rafael Ávila de Espíndola 2018-02-13 19:28:22 UTC
Created attachment 10815 [details]
testcase

The attached testcase has a shared library that defines a function foo with default visibility. The function just prints its own address and argument:

void foo(void *bar) {
  printf("%p %p\n", bar, foo);
}

There is also a position independent executable that uses a R_X86_64_PC32 to find the address of foo and call it with that value.

The expected result is to see the same value printed twice.

If we try to use gold it errors out when linking the executable:

error: test.o: requires dynamic R_X86_64_PC32 reloc against 'foo' which may overflow at runtime; recompile with -fPIC

With ld.bfd it links but fails at runtime:

./test: Symbol `foo' causes overflow in R_X86_64_PC32 relocation         
0x55ae794c62d0 0x7f0b794c62d0

With lld it works since lld create a canonical plt entry for foo in the main executable:

 6: 00000000000011f0     0 FUNC    GLOBAL DEFAULT  UND foo
Comment 1 H.J. Lu 2018-02-13 21:46:16 UTC
Created attachment 10816 [details]
A patch

Please try this.
Comment 2 Rafael Ávila de Espíndola 2018-02-13 22:19:27 UTC
(In reply to H.J. Lu from comment #1)
> Created attachment 10816 [details]
> A patch
> 
> Please try this.

The runtime warning is gone, but I still get two different values for the address:

$ ./run.sh
0x55d0082e2580 0x7f00497992d0

The symbol foo is also just a plain undefined:

0000000000000000     0 FUNC    GLOBAL DEFAULT  UND foo
Comment 3 H.J. Lu 2018-02-13 22:33:29 UTC
(In reply to Rafael Ávila de Espíndola from comment #2)
> (In reply to H.J. Lu from comment #1)
> > Created attachment 10816 [details]
> > A patch
> > 
> > Please try this.
> 
> The runtime warning is gone, but I still get two different values for the
> address:
> 
> $ ./run.sh
> 0x55d0082e2580 0x7f00497992d0
> 
> The symbol foo is also just a plain undefined:
> 
> 0000000000000000     0 FUNC    GLOBAL DEFAULT  UND foo

Fixed on master branch by

commit 1031c264fd23641111df1e12a35d0a8f7e82fb80
Author: H.J. Lu <hjl.tools@gmail.com>
Date:   Tue Feb 13 14:31:53 2018 -0800

    x86: Properly check building shared library
    
    If a symbol is not defined in a regular file, and we are not generating
    a shared library, then set the symbol to its location in the .plt.  This
    is required to make function pointers compare as equal between the normal
    executable and the shared library.
    
            * elfxx-x86.c (elf_x86_allocate_dynrelocs): Check bfd_link_dll,
            instead of bfd_link_pic, for building shared library.
Comment 4 Rafael Ávila de Espíndola 2018-02-13 22:51:35 UTC
(In reply to H.J. Lu from comment #3)
> (In reply to Rafael Ávila de Espíndola from comment #2)
> > (In reply to H.J. Lu from comment #1)
> > > Created attachment 10816 [details]
> > > A patch
> > > 
> > > Please try this.
> > 
> > The runtime warning is gone, but I still get two different values for the
> > address:
> > 
> > $ ./run.sh
> > 0x55d0082e2580 0x7f00497992d0
> > 
> > The symbol foo is also just a plain undefined:
> > 
> > 0000000000000000     0 FUNC    GLOBAL DEFAULT  UND foo
> 
> Fixed on master branch by

Awesome. I can confirm that it works now.

Should I close the bug and open a new one for gold or just reassign this one?
Comment 5 H.J. Lu 2018-02-13 22:55:49 UTC
(In reply to Rafael Ávila de Espíndola from comment #4)
> (In reply to H.J. Lu from comment #3)

> 
> Should I close the bug and open a new one for gold or just reassign this one?

Please leave it open for now until I check in my fix plus tests and
please open a new bug for gold.
Comment 6 Rafael Ávila de Espíndola 2018-02-13 23:01:26 UTC
(In reply to H.J. Lu from comment #5)
> (In reply to Rafael Ávila de Espíndola from comment #4)
> > (In reply to H.J. Lu from comment #3)
> 
> > 
> > Should I close the bug and open a new one for gold or just reassign this one?
> 
> Please leave it open for now until I check in my fix plus tests and
> please open a new bug for gold.

Note that at least for the testcase I created 1031c264fd23641111df1e12a35d0a8f7e82fb80 is sufficient. That is, the test works *without* the patch on this bug.
Comment 7 H.J. Lu 2018-02-13 23:07:31 UTC
(In reply to Rafael Ávila de Espíndola from comment #6)
> (In reply to H.J. Lu from comment #5)
> > (In reply to Rafael Ávila de Espíndola from comment #4)
> > > (In reply to H.J. Lu from comment #3)
> > 
> > > 
> > > Should I close the bug and open a new one for gold or just reassign this one?
> > 
> > Please leave it open for now until I check in my fix plus tests and
> > please open a new bug for gold.
> 
> Note that at least for the testcase I created
> 1031c264fd23641111df1e12a35d0a8f7e82fb80 is sufficient. That is, the test
> works *without* the patch on this bug.

True.  But my patch removes dynamic R_X86_64_PC32 since it can be resolved
to PLT in PIE.
Comment 8 Cary Coutant 2018-02-13 23:32:32 UTC
I still believe that the linker is working as intended. If you want the address of the PLT entry, use the PLT32 reloc.
Comment 9 Rafael Ávila de Espíndola 2018-02-13 23:35:55 UTC
(In reply to Cary Coutant from comment #8)
> I still believe that the linker is working as intended. If you want the
> address of the PLT entry, use the PLT32 reloc.

Why should -pie make a difference?

All 3 linker produce a plt entry without -pie.

This seems similar to the change to support copy relocations in pie executables.
Comment 10 H.J. Lu 2018-02-13 23:35:55 UTC
(In reply to Cary Coutant from comment #8)
> I still believe that the linker is working as intended. If you want the
> address of the PLT entry, use the PLT32 reloc.

You do get the same value at run-time and linker can resolve it without dynamic
relocation.
Comment 11 H.J. Lu 2018-02-13 23:38:39 UTC
(In reply to Rafael Ávila de Espíndola from comment #9)
> (In reply to Cary Coutant from comment #8)
> > I still believe that the linker is working as intended. If you want the
> > address of the PLT entry, use the PLT32 reloc.
> 
> Why should -pie make a difference?

No, it shouldn't.  That is my point.  With and without -pie, there should
no dynamic R_X86_64_PC32.
Comment 12 cvs-commit@gcc.gnu.org 2018-02-14 11:51:57 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=451875b4f976a527395e9303224c7881b65e12ed

commit 451875b4f976a527395e9303224c7881b65e12ed
Author: H.J. Lu <hjl.tools@gmail.com>
Date:   Wed Feb 14 03:50:40 2018 -0800

    x86-64: Use PLT address for PC-relative reloc
    
    Since PLT in PDE and PC-relative PLT in PIE can be used as function
    address, there is no need for dynamic PC-relative relocation against
    a dynamic function definition in PIE.  Linker should resolve PC-relative
    reference to its PLT address.
    
    NB: i386 has non-PIC PLT and PIC PLT.  Only non-PIC PLT in PDE can
    be used as function address.  PIC PLT in PIE can't be used as
    function address.
    
    bfd/
    
    	PR ld/22842
    	* elf32-i386.c (elf_i386_check_relocs): Pass FALSE for non
    	PC-relative PLT to NEED_DYNAMIC_RELOCATION_P.
    	* elf64-x86-64.c (elf_x86_64_check_relocs): Create PLT for
    	R_X86_64_PC32 reloc against dynamic function in data section.
    	Pass TRUE for PC-relative PLT to NEED_DYNAMIC_RELOCATION_P.
    	(elf_x86_64_relocate_section): Use PLT for R_X86_64_PC32 reloc
    	against dynamic function in data section.
    	* elfxx-x86.c (elf_x86_allocate_dynrelocs): Use PLT in PIE as
    	function address only if pcrel_plt is true.
    	(_bfd_x86_elf_link_hash_table_create): Set pcrel_plt.
    	* elfxx-x86.h (NEED_DYNAMIC_RELOCATION_P): Add PCREL_PLT for
    	PC-relative PLT.  If PLT is PC-relative, don't generate dynamic
    	PC-relative relocation against a function definition in data
    	secton in PIE.  Remove the obsolete comments.
    	(elf_x86_link_hash_table): Add pcrel_plt.
    
    ld/
    
    	PR ld/22842
    	* testsuite/ld-i386/i386.exp: Run PR ld/22842 tests.
    	* testsuite/ld-x86-64/x86-64.exp: Likewise.
    	* testsuite/ld-i386/pr22842a.c: New file.
    	* testsuite/ld-i386/pr22842b.S: Likewise.
    	* testsuite/ld-x86-64/pr22842a.c: Likewise.
    	* testsuite/ld-x86-64/pr22842a.rd: Likewise.
    	* testsuite/ld-x86-64/pr22842b.S: Likewise.
    	* testsuite/ld-x86-64/pr22842b.rd: Likewise.
Comment 13 H.J. Lu 2018-02-14 11:56:44 UTC
Fixed.
Comment 14 cvs-commit@gcc.gnu.org 2018-12-19 19:52:16 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=fc999e8020ffe8e1136da70f275bceafaa62a588

commit fc999e8020ffe8e1136da70f275bceafaa62a588
Author: H.J. Lu <hjl.tools@gmail.com>
Date:   Wed Dec 19 11:51:08 2018 -0800

    Rename PR ld/22842 run-time test to "Run pr22842"
    
    	* testsuite/ld-x86-64/x86-64.exp: Rename PR ld/22842 run-time
    	test to "Run pr22842".