Bug 19823 - gold produces copy reloc of protected symbols
Summary: gold produces copy reloc of protected symbols
Status: RESOLVED FIXED
Alias: None
Product: binutils
Classification: Unclassified
Component: gold (show other bugs)
Version: 2.27
: P2 normal
Target Milestone: ---
Assignee: Cary Coutant
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-03-15 00:28 UTC by Rafael Ávila de Espíndola
Modified: 2019-09-02 03:03 UTC (History)
3 users (show)

See Also:
Host:
Target:
Build:
Last reconfirmed:


Attachments
Patch to disallow copy relocations to protected symbols (3.44 KB, patch)
2016-03-31 08:20 UTC, Cary Coutant
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 2016-03-15 00:28:11 UTC
Given

local:
        .global internal
        .internal internal
internal:
        .global hidden
        .hidden hidden
hidden:
        .global protected
        .protected protected
protected
        .global default
default:
        .quad local
        .quad internal
        .quad hidden
        .quad protected
        .quad default

and linking with

ld test.o -o test.so -shared

gold and bfd agree on every relocation, except for protected where bfd produces

000000000268  000100000001 R_X86_64_64       0000000000000250 protected + 0

and gold produces

0000000002a0  000000000008 R_X86_64_RELATIVE                    288

The issue is that protected symbols cat still show up in copy relocations and in undefined plt entries.
Comment 1 Cary Coutant 2016-03-15 01:14:26 UTC
> The issue is that protected symbols cat still show up in copy relocations and
> in undefined plt entries.

This is from the System V ABI:

> STV_PROTECTED
>
> A symbol defined in the current component is protected if it is
> visible in other components but not preemptable, meaning that any
> reference to such a symbol from within the defining component
> must be resolved to the definition in that component, even if
> there is a definition in another component that would preempt by
> the default rules. A symbol with STB_LOCAL binding may not have
> STV_PROTECTED visibility. If a symbol definition with
> STV_PROTECTED visibility from a shared object is taken as
> resolving a reference from an executable or another shared
> object, the SHN_UNDEF symbol table entry created has STV_DEFAULT
> visibility.

I think you filed this bug report against the wrong linker.

The whole point of protected visibility is to allow static binding
within the object where the symbol is defined. The compiler may have
taken advantage of the protected visibility and generated a code
sequence that cannot reach a copy.

Or, perhaps (I need to check...), gold might be incorrectly allowing
the COPY relocation to a protected symbol, failing to consider that
the bindings within the shared object will be static. (I thought that
came up a while ago, and got fixed, but maybe it was just something
similar.) If that's the case, you can either edit the title of this PR
or close this and file a separate one.
Comment 2 H.J. Lu 2016-03-15 13:06:50 UTC
See:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65248
Comment 3 Rafael Ávila de Espíndola 2016-03-15 14:27:26 UTC
$ cat test.s                                                                                                                        
        .global _start
_start:
        .long foo
$ cat lib.s                                                                                                                         
        .data
        .global foo
        .protected foo
        .type foo, @object
        .size foo, 4
foo:
        .long 0
$ gcc -c lib.s test.s                                                                                                               
$ ld.gold lib.o -shared -o lib.so                                                                                                   
$ ld.gold test.o lib.so -o t                                                                                                        
$ readelf  -r t

Relocation section '.rela.dyn' at offset 0x230 contains 1 entries:
  Offset          Info           Type           Sym. Value    Sym. Name + Addend
000000401350  000100000005 R_X86_64_COPY     0000000000401350 foo + 0
[espindola@localhost llvm]$ readelf  -sW lib.so | grep foo
     1: 0000000000001290     4 OBJECT  GLOBAL PROTECTED    6 foo
     2: 0000000000001290     4 OBJECT  GLOBAL PROTECTED    6 foo
Comment 4 H.J. Lu 2016-03-15 15:14:06 UTC
(In reply to Cary Coutant from comment #1)
>
> Or, perhaps (I need to check...), gold might be incorrectly allowing
> the COPY relocation to a protected symbol, failing to consider that
> the bindings within the shared object will be static. (I thought that
> came up a while ago, and got fixed, but maybe it was just something
> similar.) If that's the case, you can either edit the title of this PR
> or close this and file a separate one.

It is done on purpose:

https://sourceware.org/ml/binutils/2014-05/msg00059.html
https://gcc.gnu.org/ml/gcc-patches/2015-03/msg00257.html
Comment 5 Rafael Ávila de Espíndola 2016-03-15 17:10:35 UTC
(In reply to H.J. Lu from comment #4)
> (In reply to Cary Coutant from comment #1)
> >
> > Or, perhaps (I need to check...), gold might be incorrectly allowing
> > the COPY relocation to a protected symbol, failing to consider that
> > the bindings within the shared object will be static. (I thought that
> > came up a while ago, and got fixed, but maybe it was just something
> > similar.) If that's the case, you can either edit the title of this PR
> > or close this and file a separate one.
> 
> It is done on purpose:
> 
> https://sourceware.org/ml/binutils/2014-05/msg00059.html
> https://gcc.gnu.org/ml/gcc-patches/2015-03/msg00257.html

I don't think this is related. The testcase is not pie. Gold already produced a copy relocation before a82bef932ec11cc16f205427f8a056c3c0ea517d.
Comment 6 Rafael Ávila de Espíndola 2016-03-21 12:15:02 UTC
> Or, perhaps (I need to check...), gold might be incorrectly allowing
> the COPY relocation to a protected symbol, failing to consider that
> the bindings within the shared object will be static. (I thought that
> came up a while ago, and got fixed, but maybe it was just something
> similar.) If that's the case, you can either edit the title of this PR
> or close this and file a separate one.

Is the testcase in comment 3 what you had in mind?
Comment 7 Cary Coutant 2016-03-31 08:20:26 UTC
Created attachment 9146 [details]
Patch to disallow copy relocations to protected symbols

Here's a gold patch to fix the problem. I'm out of town for the next few days, so I don't want to "commit&run", so I'll commit this when I return next week.
Comment 8 Sourceware Commits 2016-05-19 22:08:31 UTC
The master branch has been updated by Cary Coutant <ccoutant@sourceware.org>:

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

commit 6eeb0170bbb43ffb73e8f01b8b481adde8194c21
Author: Cary Coutant <ccoutant@gmail.com>
Date:   Thu May 19 14:58:18 2016 -0700

    Don't allow COPY relocations for protected symbols.
    
    gold/
    	PR gold/19823
    	* copy-relocs.cc (Copy_relocs::make_copy_reloc): Add object
    	parameter; check for protected symbol.
    	* copy-relocs.h (Copy_relocs::make_copy_reloc): Add object parameter.
    	* mips.cc (Mips_copy_relocs): Adjust call to make_copy_reloc.
    	* symtab.cc (Symbol::init_fields): Initialize is_protected_.
    	(Symbol_table::add_from_dynobj): Mark protected symbols.
    	* symtab.h (Symbol::is_protected): New method.
    	(Symbol::set_is_protected): New method.
    	(Symbol::is_protected_): New data member.
    
    	* testsuite/Makefile.am (copy_test_protected): New test.
    	* testsuite/Makefile.in: Regenerate.
    	* testsuite/copy_test.cc (main): Add legal reference to protected
    	symbol.
    	* testsuite/copy_test_v1.cc (main): Likewise.
    	* testsuite/copy_test_2.cc (ip): Add protected symbol.
    	* testsuite/copy_test_protected.cc: New test source file.
    	* testsuite/copy_test_protected.sh: New test script.
Comment 9 Cary Coutant 2016-05-19 22:14:50 UTC
Fixed on trunk.
Comment 10 Fangrui Song 2019-09-02 03:03:35 UTC
gold stills lacks some checks. A st_size=0 variant of Rafael's reproduce at #c3


% cat a.s
        .long foo
% cat b.s
        .global foo
        .protected foo
        .type foo, @object
        #.size foo, 4            if st_size is 0
foo:
        .long 0

% as a.s -o a.o
% as b.s -o b.o; gold -shared b.o -o b.so

% gold a.o b.so -pie
gold: error: a.o: requires dynamic R_X86_64_32 reloc against 'foo' which may overflow at runtime; recompile with -fPIC
% gold a.o b.so        # silent. it should error