[ARM] Fix lld crash introduced by r321154

[vit9696]

Summary

Since SyntheticSection::getParent() may return null, dereferencing this pointer in ARMExidxSentinelSection::empty() call from removeUnusedSyntheticSections() results in crashes when linking ARM binaries.

A check against null is added to ARMExidxSentinelSection::empty() similarly to SyntheticSection::getVA(). Another check is added to removeUnusedSyntheticSections, because in other functions (e.g. applySynthetic) getParent is normally checked first. While either change will fix the code, both of them seem reasonable to me.

Please commit this for me after review, since I do not have commit access.

[peter.smith]

Thanks for sending the patch. What I'd like to know first is in what circumstances the synthetic section has no parent and is this expected to be possible? If it isn't expected to be possible then it would be better to fix that and not try to account for the no parent case. The implementation has changed quite a bit since I last looked so I'm not sure of the intention.

Do you have a test case that you could add to your patch? For example find a case that currently faults without the patch and check that it links correctly with it. Perhaps checking that the output has no .ARM.exidx sections present.

[vit9696]

In D42471#986333, @peter.smith wrote:

Thanks for sending the patch. What I'd like to know first is in what circumstances the synthetic section has no parent and is this expected to be possible? If it isn't expected to be possible then it would be better to fix that and not try to account for the no parent case. The implementation has changed quite a bit since I last looked so I'm not sure of the intention.

Do you have a test case that you could add to your patch? For example find a case that currently faults without the patch and check that it links correctly with it. Perhaps checking that the output has no .ARM.exidx sections present.

Thanks for a quick reply. The reason why it has no parent (and crashes) is because we discard this section in our custom linker script, and do not include it anywhere.
It may be discussible whether it is fine to do, but lld should not crash regardless of that.
I could provide an example, but it is kind of obvious.

[ikudrin]

Thank you for the patch. I agree with Peter, the fix should be accomplished with a test.

In D42471#986379, @vit9696 wrote:

I could provide an example, but it is kind of obvious.

The simpler, the better. Such tests help us avoid regressions in the future.

ELF/SyntheticSections.cpp

2595

Use early return here. It's better than increasing the level of nesting.

ELF/Writer.cpp

1312

I think this change is independent. It's more about style, so it should be applied separately.

[vit9696]

Added a test case and restyled the check like Igor suggested.

[vit9696]

ELF/Writer.cpp

1312

Here I have to disagree. This avoids a potentially insecure code, which has already caused a crash.
Since a check is put here, and not before getParent, it is obvious that there exists some connection, so it is safer to firstly check for null.

[peter.smith]

Thanks for adding the test. I've added a small suggestion in an inline comment.

test/ELF/arm-exidx-discard.s

6

May I suggest adding a comment to explain what the test is for? Something like "// Check that the linker generated .ARM.exidx sentinel section is correctly removed. You then could use llvm-readobj to dump the Sections and use CHECK-NOT for .arm.exidx. That would make sure that we don't regress by crashing or by somehow keeping the linker generated sentinel.

[vit9696]

Updated with suggested test extension by Peter Smith.

[peter.smith]

Thanks for the change. I've no further comments.

[vit9696]

This was committed in r323366, thanks to @rafael!

ELF/SyntheticSections.cpp

void ARMExidxSentinelSection::writeTo(uint8_t *Buf) {
   uint64_t P = getVA();
   Target->relocateOne(Buf, R_ARM_PREL31, S - P);
   write32le(Buf + 4, 1);
 }
 
 // The sentinel has to be removed if there are no other .ARM.exidx entries.
 bool ARMExidxSentinelSection::empty() const {
   OutputSection *OS = getParent();
+  if (!OS)
+    return true;
   for (auto *B : OS->SectionCommands)
     if (auto *ISD = dyn_cast<InputSectionDescription>(B))
       for (auto *S : ISD->Sections)
         if (!isa<ARMExidxSentinelSection>(S))
           return false;
   return true;
 }
 
 ThunkSection::ThunkSection(OutputSection *OS, uint64_t Off)
     : SyntheticSection(SHF_ALLOC | SHF_EXECINSTR, SHT_PROGBITS,
                        Config->Wordsize, ".text.thunk") {
   this->Parent = OS;
   this->OutSecOff = Off;

ELF/Writer.cpp

static void removeUnusedSyntheticSections() {
   // All input synthetic sections that can be empty are placed after
   // all regular ones. We iterate over them all and exit at first
   // non-synthetic.
   for (InputSectionBase *S : llvm::reverse(InputSections)) {
     SyntheticSection *SS = dyn_cast<SyntheticSection>(S);
     if (!SS)
       return;
     OutputSection *OS = SS->getParent();
-    if (!SS->empty() || !OS)
+    if (!OS || !SS->empty())
       continue;
 
     std::vector<BaseCommand *>::iterator Empty = OS->SectionCommands.end();
     for (auto I = OS->SectionCommands.begin(), E = OS->SectionCommands.end();
          I != E; ++I) {
       BaseCommand *B = *I;
       if (auto *ISD = dyn_cast<InputSectionDescription>(B)) {
         llvm::erase_if(ISD->Sections,

test/ELF/arm-exidx-discard.s

+// RUN: llvm-mc -filetype=obj -triple arm-gnu-linux-eabi -mcpu cortex-a7 -arm-add-build-attributes %s -o %t.o
+// RUN: echo "ENTRY(__entrypoint) SECTIONS { . = 0x10000; .text : { *(.text .text.*) } /DISCARD/ : { *(.ARM.exidx*) *(.gnu.linkonce.armexidx.*) } }" > %t.script
+// RUN: ld.lld -T %t.script %t.o -o %t.elf 2>&1
+// RUN: llvm-readobj -sections %t.elf | FileCheck %s
+// REQUIRES: arm
+
+.globl  __entrypoint
+__entrypoint:
+    bx  lr
+
+// Check that .ARM.exidx/.gnu.linkonce.armexidx
+// are correctly removed if they were added.
+// CHECK-NOT: .ARM.exidx
+// CHECK-NOT: .gnu.linkonce.armexidx.