This is an archive of the discontinued LLVM Phabricator instance.

Differential D44601

LLD: Avoid segfault with --emit-relocs
AbandonedPublic

Authored by dstolfa on Mar 17 2018, 12:44 PM.

Details

Reviewers
• rafael
ruiu
• espindola
Summary

When passing an --emit-relocs flag to lld, there seems to be a possibility for the output section to be NULL. This causes lld to crash which results in the executable not being generated. This patch works around this issue by simply checking if the output section exists and returns "" as the name if it doesn't. The check of the return value then prevents propagation of "" in other bits of the code.

I've inspected a number of resulting ELF files generated with this and it looks correct and it breaks no additional tests.

Diff Detail

Event Timeline

dstolfa created this revision.Mar 17 2018, 12:44 PM
Herald added subscribers: llvm-commits, krytarowski, arichardson, emaste. · View Herald TranscriptMar 17 2018, 12:44 PM
dstolfa updated this revision to Diff 138827.EditedMar 17 2018, 1:53 PM

Generate a minimal test case for the breakage. It seems like the root cause of this are the cfi directives.

arichardson added inline comments.Mar 17 2018, 3:40 PM
ELF/Writer.cpp
101

It seems like the problem here is that when this is called with .rela.eh_frame getRelocatedSection() returns an EhInputSection where ->getParent() returns null.

If I link the test file with ld.bfd it contains relocations so we should probably be emitting the same output and not just omit the relocations:

Relocations [
  Section (3) .rela.eh_frame {
    0x4000A0 R_X86_64_PC32 .text 0x0
  }
]
test/ELF/emit-relocs-freebsd.s
2 ↗(On Diff #138827)

Possibly add a llvm-readobj invocation here to show that the input file contains relocations:

Relocations [
  Section (4) .rela.eh_frame {
    0x20 R_X86_64_PC32 .text 0x0
  }
]
6 ↗(On Diff #138827)

I don't there is much point in testing the aliases for --emit-relocs here. It is already done in emit-relocs.s

8 ↗(On Diff #138827)

This line is not actually running llvm-readobj

24 ↗(On Diff #138827)

I don't think we need to check the symbols in this test. This is about not crashing on relocations in .eh_frame and --emit-relocs

dstolfa retitled this revision from LLD: Avoid segfault on FreeBSD with --emit-relocs to LLD: Avoid segfault with --emit-relocs.EditedMar 18 2018, 6:00 PM
dstolfa edited the summary of this revision. (Show Details)

I've verified the breakage on Linux as well. It seems to be caused by InX::EhFrame->getParent() being nullptr when EmitRelocs is set. This is because near the start of the InputSection vector, there are additional input sections that end up pointing to InX::EhFrame as the parent and getOutputSectionName returns nullptr.

dstolfa updated this revision to Diff 138876.Mar 18 2018, 6:23 PM

Bump the diff to reflect things that @arichardson has helped me find.

The relocations are still not checked in the test itself because it's not clear what the section number should be yet (I'm getting different outputs from different linkers at the moment).

This diff removes the unnecessary duplicate runs that are checked by emit-relocs.s already and renames emit-relocs-freebsd.s to emit-relocs-eh.s as the breakage is not unique to FreeBSD. It does not provide a correct fix yet and still just works around the issue by simply ignoring the problematic entries.

grimar added a subscriber: grimar.Mar 19 2018, 1:06 AM

It is https://bugs.llvm.org//show_bug.cgi?id=36367

dstolfa abandoned this revision.Mar 19 2018, 6:52 AM

Closing this due to a better patch in https://reviews.llvm.org/D44622.

dstolfa updated this revision to Diff 139034.EditedMar 19 2018, 5:35 PM

Fix the issue and add a sensible test.

EDIT: This breaks a couple of tests (my environment didn't update the first time around).

dstolfa updated this revision to Diff 139070.Mar 19 2018, 8:33 PM

I believe that this fully fixes the issue. It adds a check for Config->Relocatable and checks for OutsecName correctly.

All tests pass.

dstolfa marked an inline comment as done.Mar 19 2018, 8:38 PM
grimar mentioned this in D44679: [ELF] - Another fix for "LLD crashes with --emit-relocs when trying to proccess .eh_frame".Mar 20 2018, 4:48 AM
dstolfa added a reviewer: • espindola.Mar 20 2018, 12:56 PM
dstolfa updated this revision to Diff 139227.Mar 20 2018, 5:06 PM

Import the test case from @grimar's D44679 to verify that this patch works with the linkerscript test as well.

Thanks for thinking of this case!

• espindola added a comment.Mar 23 2018, 7:00 PM

This was fixed by r328299, right?

dstolfa abandoned this revision.Mar 23 2018, 7:06 PM
In D44601#1047427, @espindola wrote:

This was fixed by r328299, right?

Correct. I seem to have missed the commit when it happened so it remained open. Closing this diff because of the commit.

Revision Contents

PathSize
ELF/
3 lines
2 lines
test/
ELF/
17 lines
linkerscript/
13 lines

Diff 139227

ELF/LinkerScript.cpp

Show First 20 Lines • Show All 549 Lines • ▼ Show 20 Linesstatic OutputSection *addInputSec(StringMap<OutputSection *> &Map,
// Imagine .zed : { *(.foo) *(.bar) } script. Both foo and bar may have // Imagine .zed : { *(.foo) *(.bar) } script. Both foo and bar may have
// relocation sections .rela.foo and .rela.bar for example. Most tools do // relocation sections .rela.foo and .rela.bar for example. Most tools do
// not allow multiple REL[A] sections for output section. Hence we // not allow multiple REL[A] sections for output section. Hence we
// should combine these relocation sections into single output. // should combine these relocation sections into single output.
// We skip synthetic sections because it can be .rela.dyn/.rela.plt or any // We skip synthetic sections because it can be .rela.dyn/.rela.plt or any
// other REL[A] sections created by linker itself. // other REL[A] sections created by linker itself.
if (!isa<SyntheticSection>(IS) && if (!isa<SyntheticSection>(IS) &&
(IS->Type == SHT_REL || IS->Type == SHT_RELA)) { (IS->Type == SHT_REL || IS->Type == SHT_RELA)) {
if (!Config->Relocatable &&
OutsecName.find(".eh_frame") != std::string::npos)
return createSection(IS, OutsecName);
auto *Sec = cast<InputSection>(IS); auto *Sec = cast<InputSection>(IS);
OutputSection *Out = Sec->getRelocatedSection()->getOutputSection(); OutputSection *Out = Sec->getRelocatedSection()->getOutputSection();
if (Out->RelocationSection) { if (Out->RelocationSection) {
Out->RelocationSection->addSection(Sec); Out->RelocationSection->addSection(Sec);
return nullptr; return nullptr;
} }
▲ Show 20 Lines • Show All 569 Lines • Show Last 20 Lines

ELF/Writer.cpp

Show First 20 Lines • Show All 92 Lines • ▼ Show 20 LinesStringRef elf::getOutputSectionName(InputSectionBase *S) {
// This is for --emit-relocs. If .text.foo is emitted as .text.bar, we want // This is for --emit-relocs. If .text.foo is emitted as .text.bar, we want
// to emit .rela.text.foo as .rela.text.bar for consistency (this is not // to emit .rela.text.foo as .rela.text.bar for consistency (this is not
// technically required, but not doing it is odd). This code guarantees that. // technically required, but not doing it is odd). This code guarantees that.
if ((S->Type == SHT_REL || S->Type == SHT_RELA) && if ((S->Type == SHT_REL || S->Type == SHT_RELA) &&
!isa<SyntheticSection>(S)) { !isa<SyntheticSection>(S)) {
OutputSection *Out = OutputSection *Out =
cast<InputSection>(S)->getRelocatedSection()->getOutputSection(); cast<InputSection>(S)->getRelocatedSection()->getOutputSection();
if (!Out)
arichardsonUnsubmitted
Done
ReplyInline Actions

It seems like the problem here is that when this is called with .rela.eh_frame getRelocatedSection() returns an EhInputSection where ->getParent() returns null.

If I link the test file with ld.bfd it contains relocations so we should probably be emitting the same output and not just omit the relocations:

Relocations [
  Section (3) .rela.eh_frame {
    0x4000A0 R_X86_64_PC32 .text 0x0
  }
]
arichardson: It seems like the problem here is that when this is called with `.rela.eh_frame`…
return S->Name;
if (S->Type == SHT_RELA) if (S->Type == SHT_RELA)
return Saver.save(".rela" + Out->Name); return Saver.save(".rela" + Out->Name);
return Saver.save(".rel" + Out->Name); return Saver.save(".rel" + Out->Name);
} }
for (StringRef V : for (StringRef V :
{".text.", ".rodata.", ".data.rel.ro.", ".data.", ".bss.rel.ro.", {".text.", ".rodata.", ".data.rel.ro.", ".data.", ".bss.rel.ro.",
".bss.", ".init_array.", ".fini_array.", ".ctors.", ".dtors.", ".tbss.", ".bss.", ".init_array.", ".fini_array.", ".ctors.", ".dtors.", ".tbss.",
▲ Show 20 Lines • Show All 2,153 Lines • Show Last 20 Lines

test/ELF/emit-relocs-eh-frame.s

  • This file was added.
# REQUIRES: x86
# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t1.o
# RUN: ld.lld --emit-relocs %t1.o -o %t
# RUN: llvm-readobj -r %t | FileCheck %s
# CHECK: Relocations [
# CHECK-NEXT: Section {{.*}} .rela.eh_frame {
# CHECK-NEXT: 0x{{.*}} R_X86_64_PC32 .text 0x0
# CHECK-NEXT: }
# CHECK-NEXT: ]
.globl _start
.text
_start:
.cfi_startproc
nop
.cfi_endproc

test/ELF/linkerscript/eh-frame-emit-relocs.s

  • This file was added.
# REQUIRES: x86
# RUN: echo "SECTIONS { .foo : { *(.eh_frame) } }" > %t.script
# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t.o
# RUN: ld.lld --emit-relocs %t.o -T %t.script -o %t
# RUN: llvm-objdump -section-headers %t | FileCheck %s
# CHECK-NOT: eh_frame
# CHECK: .rela.foo
# CHECK-NOT: eh_frame
.text
.cfi_startproc
.cfi_endproc