Bug 23850 - GNU strip should not discard/move .rela.plt in executable linking glibc statically
Summary: GNU strip should not discard/move .rela.plt in executable linking glibc stati...
Status: RESOLVED FIXED
Alias: None
Product: binutils
Classification: Unclassified
Component: binutils (show other bugs)
Version: 2.32
: P2 normal
Target Milestone: ---
Assignee: Alan Modra
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-11-01 20:43 UTC by Fangrui Song
Modified: 2018-11-02 07:42 UTC (History)
0 users

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


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Fangrui Song 2018-11-01 20:43:27 UTC
This is similar to Bug 10337 but is a different incarnation.

When linking glibc statically, .rel?.plt contains R_*_IRELATIVE (e.g. strcpy) relocations.
If such .rela.plt section has sh_link pointing to .symtab,
GNU strip will produce a corrupted executable that crashes at run time.

% clang -fuse-ld=lld -static -xc =(printf 'int main(){}') -o a
# or gcc on a Debian derivative with the -fuse-ld=lld patch

% readelf -WS a
...
  [Nr] Name              Type            Address          Off    Size   ES Flg Lk Inf Al
...
  # .rela.plt has sh_link pointing to .symtab after https://reviews.llvm.org/D52830
  [ 1] .rela.plt         RELA            0000000000200238 000238 000108 18   A 32  20  8
...
  [32] .symtab           SYMTAB          0000000000000000 0b62a8 00a9e0 18     34 694  8

% strip a -o a.stripped; ./a.stripped
unexpected reloc type in static binary[1]    164865 segmentation fault  ./a.stripped

The crash is because .rela.plt is discarded (in a more complicated case there may be a new .rela.got.plt but I haven't delved into the details)
and its place [__rela_iplt_start, __rela_iplt_end) is filled by zeroes.

    __rela_iplt_start __rela_iplt_end are static relocations resolved at link time.

When running the stripped executable, apply_irel in libc initialization will error `unexpected reloc type in static binary` and a crash ensues.

// https://sourceware.org/git/?p=glibc.git;a=blob;hb=fc783076ec496a55c029be14617ea16a24589f55;f=csu/libc-start.c#l78
glibc/csu/libc-start.c

  static void
  apply_irel (void)
  {
  # ifdef IREL
    /* We use weak references for these so that we'll still work with a linker
       that doesn't define them.  Such a linker doesn't support IFUNC at all
       and so uses won't work, but a statically-linked program that doesn't
       use any IFUNC symbols won't have a problem.  */
    extern const IREL_T IPLT_START[] __attribute__ ((weak));
    extern const IREL_T IPLT_END[] __attribute__ ((weak));
    //////// IPLT_START (__rela_iplt_start) points to a zero-filled region
    for (const IREL_T *ipltent = IPLT_START; ipltent < IPLT_END; ++ipltent)
      IREL (ipltent);
  # endif
  }
  #endif

Note ld.bfd and gold synthesize .rela.plt with sh_link=0 and do not have the issue, but changing sh_link to .symtab will also break. Moreever, GNU readelf has a warning:

% readelf -WS a
...
readelf: Warning: [ 3]: Link field (0) should index a symtab section.

https://sourceware.org/git/?p=binutils-gdb.git;a=blob;hb=cdcda965ea4c70c80b9f8c294597e991417ff9d5;f=binutils/readelf.c#l6307
	case SHT_REL:
	case SHT_RELA:
	  if (section->sh_link < 1
	      || section->sh_link >= filedata->file_header.e_shnum
	      || (filedata->section_headers[section->sh_link].sh_type != SHT_SYMTAB
		  && filedata->section_headers[section->sh_link].sh_type != SHT_DYNSYM))
	    warn (_("[%2u]: Link field (%u) should index a symtab section.\n"),
		  i, section->sh_link);
	  break;


https://docs.oracle.com/cd/E19683-01/816-1386/6m7qcoblj/index.html#chapter6-47976 says
  SHT_REL SH_RELA sections' sh_link point to "The section header index of the associated symbol table."

It seems to me that in absence of .dymsym, sh_link=.symtab makes more sense than 0.
Comment 1 Fangrui Song 2018-11-01 22:23:36 UTC
Is this because .rela.plt (SHF_ALLOC) should not have sh_link pointing to .symtab (non-SHF_ALLOC)? If it is the case, perhaps `readelf -S a` should not warn on such executable.
Comment 2 Alan Modra 2018-11-02 01:13:13 UTC
.rela.plt in a static executable can only contain IRELATIVE relocations that must have a zero symbol index.  Thus it is not necessary for that particular SHT_RELA section to have sh_link index .symtab.  As you recognize, it is also inconsistent to have a SHF_ALLOC SHT_RELA section sh_link point at a non-SHF_ALLOC symbol table.  You should probably report this problem as an lld bug.

That said, objcopy can be fixed without too much trouble, I think.
Comment 3 Fangrui Song 2018-11-02 02:11:05 UTC
(In reply to Alan Modra from comment #2)
> .rela.plt in a static executable can only contain IRELATIVE relocations that
> must have a zero symbol index.  Thus it is not necessary for that particular
> SHT_RELA section to have sh_link index .symtab.  As you recognize, it is
> also inconsistent to have a SHF_ALLOC SHT_RELA section sh_link point at a
> non-SHF_ALLOC symbol table.  You should probably report this problem as an
> lld bug.
> 
> That said, objcopy can be fixed without too much trouble, I think.

I've fixed that lld bug https://reviews.llvm.org/D53993

  InputSection *SymTab = Config->Relocatable ? In.SymTab : In.DynSymTab;
  getParent()->Link = SymTab ? SymTab->getParent()->SectionIndex : 0;

Do you think if readelf -S should be taught not to warn in this case? This was the original motivation that sh_link pointed to .symtab

% readelf -S a
  [ 0]                   NULL            0000000000000000 000000 000000 00      0   0  0
readelf: Warning: [ 1]: Link field (0) should index a symtab section.
  [ 1] .rela.plt         RELA            0000000000200238 000238 000108 18   A  0  20  8
Comment 4 Alan Modra 2018-11-02 03:46:37 UTC
> Do you think if readelf -S should be taught not to warn in this case?

Yes.  I have a patch for that too, which I'll commit after running my usual set of 150 or so binutils test builds.
Comment 5 Sourceware Commits 2018-11-02 04:50:47 UTC
The master branch has been updated by Alan Modra <amodra@sourceware.org>:

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

commit a4bcd733712abd892aa7fe0d79a3f999b461f119
Author: Alan Modra <amodra@gmail.com>
Date:   Fri Nov 2 11:47:07 2018 +1030

    PR23850, strip should not discard/move .rela.plt in executable
    
    strip/objcopy can't deal with alloc reloc sections, not .rela.dyn or
    .rela.plt in a dynamic executable, or .rela.plt/.rela.iplt in a static
    executable.  So, don't have BFD treat them as side-channel data
    associated with the section they are relocating.
    
    	PR 23850
    	* elf.c (bfd_section_from_shdr): Treat SHF_ALLOC SHT_REL* sections
    	in an executable or shared library as normal sections.
Comment 6 Fangrui Song 2018-11-02 07:42:42 UTC
Thanks for the two patches! Closing as all parties are good now (linker,strip,readelf).