This is an archive of the discontinued LLVM Phabricator instance.

[ELF] Change sh_link of .rel{,a}.plt to make GNU strip happy
ClosedPublic

Authored by MaskRay on Nov 1 2018, 12:44 PM.

Details

Summary

D52830 sets sh_link to .symtab in static link, which breaks executable stripped by GNU strip.
It may also be odd that .rela.plt (SHF_ALLOC) points to .symtab (non-SHF_ALLOC).

Change the logic as pcc's suggestion.

Before:

% clang -fuse-ld=lld -static -xc =(printf 'int main(){}') # or gcc
% strip a.out; ./a.out
unexpected reloc type in static binary[1] 61634 segmentation fault ./a.out

Event Timeline

MaskRay created this revision.Nov 1 2018, 12:44 PM
MaskRay updated this revision to Diff 172209.Nov 1 2018, 12:55 PM

Add tests and GNU strip date (git HEAD as of 2018-10)

MaskRay updated this revision to Diff 172212.Nov 1 2018, 12:57 PM
MaskRay edited the summary of this revision. (Show Details)
MaskRay removed a subscriber: javed.absar.

A simple reproduce command. I forgot to add --verbatim in my arc diff command :)

ruiu added inline comments.Nov 1 2018, 1:09 PM
ELF/SyntheticSections.cpp
1494

Looks like this new comment is not easy to understand for those who do not already know the problem you are trying to fix in this. What is this link supposed to have by the spec? You shouldn't focus too much on a workaround when describing something, but focus on what it is supposed to be (and briefly mention the deviation from the expected situation.)

MaskRay added inline comments.Nov 1 2018, 1:29 PM
ELF/SyntheticSections.cpp
1494

D52830 says:

ELF spec (https://docs.oracle.com/cd/E19683-01/816-1386/chapter6-94076/index.html)
says that for SHT_REL and SHT_RELA, sh_link references the associated symbol table
and sh_info the "section to which the relocation applies."

I think either .symtab and .dynsym can be used. binutils/readelf.c emits the warning when the pointed symbol table section is of neither SHT_SYMTAB nor SHT_DYNSYM.

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;

Let me report an issue on binutils and simplify the comment with a link to that bug

MaskRay marked an inline comment as done.Nov 1 2018, 1:52 PM
MaskRay added inline comments.
ELF/SyntheticSections.cpp
1494

Simplified it with a link to https://docs.oracle.com/cd/E19683-01/816-1386/6m7qcoblh/index.html

Basically it reverts the sh_link part of D52830

I am happy to change the comments at your request.

MaskRay updated this revision to Diff 172229.Nov 1 2018, 2:02 PM
MaskRay marked an inline comment as done.

Better comment

MaskRay added inline comments.Nov 1 2018, 2:09 PM
ELF/SyntheticSections.cpp
1494

Or how about this:

if (In.DynSymTab)
  getParent()->Link = In.DynSymTab->getParent()->SectionIndex;
else if (In.SymTab && In.RelaIplt != this) // && In.RelaIplt != this  is the new condition
  getParent()->Link = In.SymTab->getParent()->SectionIndex;
else
  getParent()->Link = 0;

.rela.plt (In.RelaIplt) is a special relocation section invented for IFUNC. We can keep the work around to a minimum case.

pcc added a subscriber: pcc.Nov 1 2018, 2:25 PM

D52830 sets sh_link to .symtab in static link, which makes more sense than 0,

I don't know about that. Irrespective of the bug it seems a little odd for a SHF_ALLOC section to be linked to a non-SHF_ALLOC section. Should this be instead:

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

?

MaskRay updated this revision to Diff 172243.Nov 1 2018, 2:48 PM
MaskRay retitled this revision from [ELF] Set sh_link to 0 in static link to make GNU strip happy to [ELF] Change sh_link of .rel{,a}.plt to make GNU strip happy.
MaskRay edited the summary of this revision. (Show Details)

Apply pcc's suggestion

ruiu added inline comments.Nov 1 2018, 3:22 PM
ELF/SyntheticSections.cpp
1496

Please move the assignment out of the if.

ruiu accepted this revision.Nov 1 2018, 3:25 PM

Other than that LGTM

This revision is now accepted and ready to land.Nov 1 2018, 3:25 PM
MaskRay updated this revision to Diff 172251.Nov 1 2018, 3:28 PM
MaskRay edited the summary of this revision. (Show Details)

move the assignment out of the if.

MaskRay marked 3 inline comments as done.Nov 1 2018, 3:29 PM
This revision was automatically updated to reflect the committed changes.

Heads-up https://sourceware.org/bugzilla/show_bug.cgi?id=23850 Alan Modra has fixed the GNU strip issue (when .rela.pla has sh_link pointing to .symtab, the stripped executable will crash) and the readelf -S warning.

The patches should be included in GNU binutils 2.32