[llvm-objcopy] - Do not crash on object that has relocations but no symbol table.

[grimar]

Summary

It was revealed by D69260.

Tool crashed when scanned relocations in a object without a symbol table.
This patch teaches it either to handle such objects (when relocations
does not use symbols we do not need a symbol table to proceed)
or to show an appropriate error otherwise.

[MaskRay]

An alternative approach is to generalize else if (EnsureSymtab) for non --add-symbol= operations as well. I need to investigate more to check whether that is feasible. If it does, it can save us some SymbolTable nullness checks later. ()I should also address another problem raised in D69093 (SectionNames is initialized after if (Obj.SectionNames != &Sec))

test/tools/llvm-objcopy/ELF/relocations-no-symtab.test

2 ↗

(On Diff #226034)

that holds symbol index 0

4 ↗

(On Diff #226034)

-o

[grimar]

In D69304#1718059, @MaskRay wrote:

An alternative approach is to generalize else if (EnsureSymtab) for non --add-symbol= operations as well.

I am not sure I understood the idea honestly. Here we have an object that has a relocation which references a symbol when symbol table is not exist.
In my understanding it is not a situation where we might want to create a symbol table, it is probably about we should report an error?

[MaskRay]

test/tools/llvm-objcopy/ELF/relocations-no-symtab.test

31 ↗

(On Diff #226034)

a relocation refers to a non-zero symbol index but there is no symbol table.

[grimar]

• Addressed review comments.

[jhenderson]

llvm/test/tools/llvm-objcopy/ELF/relocations-no-symtab.test

3

I think "with a symbol index of 0" would be more natural than "that holds symbol index 0"

35

-o for consistency.

44

It probably makes slightly more sense for this to be ET_REL.

llvm/tools/llvm-objcopy/ELF/Object.cpp

908–909

This looks dodgy if no symbol exists.

1427–1428

I'm not sure this error message is clear enough. I think it is not obvious what the number in it represents. Perhaps it could be rephrased "relocation references symbol with index N, but there is no symbol table". What do you think?

[MaskRay]

llvm/test/tools/llvm-objcopy/ELF/relocations-no-symtab.test

36

%t4 -> /dev/null

llvm/tools/llvm-objcopy/ELF/Object.cpp

1427–1428

"relocation references symbol with index N, but there is no symbol table"

+1

[grimar]

• Addressed review comments.

llvm/tools/llvm-objcopy/ELF/Object.cpp

908–909

"relocation references symbol with index..." error is triggered earlier than execution flow might reach here.

It is reported from initRelocations which is called inside Reader.create() here:
https://github.com/llvm-mirror/llvm/blob/master/tools/llvm-objcopy/ELF/ELFObjcopy.cpp#L773

markSymbols() is called from updateAndRemoveSymbols which is itself caled in handleArgs:
https://github.com/llvm-mirror/llvm/blob/master/tools/llvm-objcopy/ELF/ELFObjcopy.cpp#L799
https://github.com/llvm-mirror/llvm/blob/master/tools/llvm-objcopy/ELF/ELFObjcopy.cpp#L613
https://github.com/llvm-mirror/llvm/blob/master/tools/llvm-objcopy/ELF/ELFObjcopy.cpp#L402

handleArgs is called later than Reader.create().

Also, it checks for an empty symbol table at its begining:
https://github.com/llvm-mirror/llvm/blob/master/tools/llvm-objcopy/ELF/ELFObjcopy.cpp#L350

1427–1428

Yep, that is better.

[grimar]

Going to check one more case for RelocationSection::markSymbols() mentioned.

[jhenderson]

Okay, everything else LGTM. If you don't find any more issues, feel free to commit from my point of view.

[grimar]

In D69304#1724971, @jhenderson wrote:

Okay, everything else LGTM. If you don't find any more issues, feel free to commit from my point of view.

I've found you actually was right. I.e. I found a different scenario to trigger a crash in markSymbols. Working on a test case, going to update the patch soon.

[grimar]

• Added a test case and did a code change for a one more case.

[grimar]

llvm/tools/llvm-objcopy/ELF/Object.cpp

908–909

I've found this concern was valid. To trigger an issue we need an object with a symbol table and a relocation with a symbol index of 0. Then it is possible to reach markSymbol for example when --strip-unneeded is given.

I've added a test to no-symbol-relocation.test.

[jhenderson]

LGTM. Thanks!

llvm/tools/llvm-objcopy/ELF/Object.cpp

908–909

It's probably safer this way, even if it was unreachable. But I'm glad my caution was not misplaced this time!

llvm/test/tools/llvm-objcopy/ELF/no-symbol-relocation.test

-# RUN: yaml2obj %s > %t
-# RUN: llvm-objcopy %t %t2
+# RUN: yaml2obj --docnum=1 %s > %t1
+# RUN: llvm-objcopy %t1 %t2
 # RUN: llvm-readobj --relocations %t2 | FileCheck %s
 
-!ELF
+--- !ELF
 FileHeader:
   Class:           ELFCLASS64
   Data:            ELFDATA2LSB
   Type:            ET_EXEC
   Machine:         EM_X86_64
 Sections:
   - Name:            .text
     Type:            SHT_PROGBITS
     Flags:           [ SHF_ALLOC, SHF_EXECINSTR ]
     Address:         0x1000
     AddressAlign:    0x0000000000000010
     Content:         "0000000000000000"
   - Name:            .rel.text
     Type:            SHT_REL
     Info:            .text
     Relocations:
       - Offset: 0x1000
         Type:   R_X86_64_RELATIVE
-## TODO: llvm-objcopy crashes without the following line.
-Symbols: []
 
 # CHECK:     Relocations [
 # CHECK-NEXT:  Section (2) .rel.text {
 # CHECK-NEXT:    0x1000 R_X86_64_RELATIVE - 0x0
 # CHECK-NEXT:  }
 # CHECK-NEXT:]
+
+## Check we produce a valid output when stripping unneeded symbols from an object that
+## has a symbol table and a relocation with a symbol index of 0.
+
+# RUN: yaml2obj --docnum=2 %s > %t3
+# RUN: llvm-objcopy --strip-unneeded %t3 %t4
+# RUN: llvm-readobj --relocations --sections --symbols %t4 | FileCheck %s --check-prefix=STRIP
+
+# STRIP:      Relocations [
+# STRIP-NEXT:   Section {{.*}} .rel.text {
+# STRIP-NEXT:     0x1000 R_X86_64_NONE - 0x0
+# STRIP-NEXT:   }
+# STRIP-NEXT: ]
+# STRIP-NEXT: Symbols [
+# STRIP-NEXT:   Symbol {
+# STRIP-NEXT:     Name:  (0)
+# STRIP-NEXT:     Value: 0x0
+# STRIP-NEXT:     Size: 0
+# STRIP-NEXT:     Binding: Local (0x0)
+# STRIP-NEXT:     Type: None (0x0)
+# STRIP-NEXT:     Other: 0
+# STRIP-NEXT:     Section: Undefined (0x0)
+# STRIP-NEXT:   }
+# STRIP-NEXT: ]
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_REL
+  Machine: EM_X86_64
+Sections:
+  - Name:  .text
+    Type:  SHT_PROGBITS
+  - Name:  .rel.text
+    Type:  SHT_REL
+    Info:  .text
+    Relocations:
+      - Offset: 0x1000
+        Type:   R_X86_64_NONE
+Symbols: []

llvm/test/tools/llvm-objcopy/ELF/relocations-no-symtab.test

+## Check that we can copy an object that has a relocation
+## with a symbol index of 0 even when there is no symbol table.
+
+# RUN: yaml2obj --docnum=1 %s -o %t1
+# RUN: llvm-objcopy %t1 %t2
+# RUN: llvm-readobj --relocations %t2 | FileCheck %s
+
+# CHECK:     Relocations [
+# CHECK-NEXT:  Section {{.*}} .rel.text {
+# CHECK-NEXT:    0x1000 R_X86_64_RELATIVE - 0x0
+# CHECK-NEXT:  }
+# CHECK-NEXT:]
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_REL
+  Machine: EM_X86_64
+Sections:
+  - Name:  .text
+    Type:  SHT_PROGBITS
+    Flags: [ SHF_ALLOC, SHF_EXECINSTR ]
+  - Name:  .rel.text
+    Type:  SHT_REL
+    Info:  .text
+    Relocations:
+      - Offset: 0x1000
+        Type:   R_X86_64_RELATIVE
+
+## Check that we report an error when a relocation refers to a
+## non-zero symbol index but there is no symbol table.
+
+# RUN: yaml2obj --docnum=2 %s -o %t3
+# RUN: not llvm-objcopy %t3 /dev/null 2>&1 | FileCheck %s --check-prefix=ERR
+
+# ERR: error: '.rel.text': relocation references symbol with index 1, but there is no symbol table
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_REL
+  Machine: EM_X86_64
+Sections:
+  - Name:  .text
+    Type:  SHT_PROGBITS
+    Flags: [ SHF_ALLOC, SHF_EXECINSTR ]
+  - Name:  .rel.text
+    Type:  SHT_REL
+    Info:  .text
+    Relocations:
+      - Offset: 0x1000
+        Symbol: 1
+        Type:   R_X86_64_NONE

llvm/tools/llvm-objcopy/ELF/Object.cpp

if (!AllowBrokenLinks)
           llvm::errc::invalid_argument,
           "symbol table '%s' cannot be removed because it is "
           "referenced by the relocation section '%s'",
           Symbols->Name.data(), this->Name.data());
     Symbols = nullptr;
   }
 
   for (const Relocation &R : Relocations) {
-    if (!R.RelocSymbol->DefinedIn || !ToRemove(R.RelocSymbol->DefinedIn))
+    if (!R.RelocSymbol || !R.RelocSymbol->DefinedIn ||
+        !ToRemove(R.RelocSymbol->DefinedIn))
       continue;
     return createStringError(llvm::errc::invalid_argument,
                              "section '%s' cannot be removed: (%s+0x%" PRIx64
                              ") has relocation against symbol '%s'",
                              R.RelocSymbol->DefinedIn->Name.data(),
                              SecToApplyRel->Name.data(), R.Offset,
                              R.RelocSymbol->Name.c_str());
   }
static void setAddend(Elf_Rel_Impl<ELFT, true> &Rela, uint64_t Addend) {
   Rela.r_addend = Addend;
 }
 
 template <class RelRange, class T>
 static void writeRel(const RelRange &Relocations, T *Buf) {
   for (const auto &Reloc : Relocations) {
     Buf->r_offset = Reloc.Offset;
     setAddend(*Buf, Reloc.Addend);
-    Buf->setSymbolAndType(Reloc.RelocSymbol->Index, Reloc.Type, false);
+    Buf->setSymbolAndType(Reloc.RelocSymbol ? Reloc.RelocSymbol->Index : 0,
+                          Reloc.Type, false);
     ++Buf;
   }
 }
 
 template <class ELFT>
 void ELFSectionWriter<ELFT>::visit(const RelocationSection &Sec) {
   uint8_t *Buf = Out.getBufferStart() + Sec.Offset;
   if (Sec.Type == SHT_REL)
     writeRel(Sec.Relocations, reinterpret_cast<Elf_Rel *>(Buf));
   else
     writeRel(Sec.Relocations, reinterpret_cast<Elf_Rela *>(Buf));
 }
 
 void RelocationSection::accept(SectionVisitor &Visitor) const {
   Visitor.visit(*this);
 }
 
 void RelocationSection::accept(MutableSectionVisitor &Visitor) {
   Visitor.visit(*this);
 }
 
 Error RelocationSection::removeSymbols(
     function_ref<bool(const Symbol &)> ToRemove) {
   for (const Relocation &Reloc : Relocations)
-    if (ToRemove(*Reloc.RelocSymbol))
+    if (Reloc.RelocSymbol && ToRemove(*Reloc.RelocSymbol))
       return createStringError(
           llvm::errc::invalid_argument,
           "not stripping symbol '%s' because it is named in a relocation",
           Reloc.RelocSymbol->Name.data());
   return Error::success();
 }
 
 void RelocationSection::markSymbols() {
   for (const Relocation &Reloc : Relocations)
+    if (Reloc.RelocSymbol)
       Reloc.RelocSymbol->Referenced = true;
 }
 
 void RelocationSection::replaceSectionReferences(
     const DenseMap<SectionBase *, SectionBase *> &FromTo) {
   // Update the target section if it was replaced.
   if (SectionBase *To = FromTo.lookup(SecToApplyRel))
     SecToApplyRel = To;
 }

 template <class T>
 static void initRelocations(RelocationSection *Relocs,
                             SymbolTableSection *SymbolTable, T RelRange) {
   for (const auto &Rel : RelRange) {
     Relocation ToAdd;
     ToAdd.Offset = Rel.r_offset;
     getAddend(ToAdd.Addend, Rel);
     ToAdd.Type = Rel.getType(false);
-    ToAdd.RelocSymbol = SymbolTable->getSymbolByIndex(Rel.getSymbol(false));
+
+    if (uint32_t Sym = Rel.getSymbol(false)) {
+      if (!SymbolTable)
+        error("'" + Relocs->Name +
+              "': relocation references symbol with index " + Twine(Sym) +
+              ", but there is no symbol table");
+      ToAdd.RelocSymbol = SymbolTable->getSymbolByIndex(Sym);
+    }
+
     Relocs->addRelocation(ToAdd);
   }
 }
 
 SectionBase *SectionTableRef::getSection(uint32_t Index, Twine ErrMsg) {
   if (Index == SHN_UNDEF || Index > Sections.size())
     error(ErrMsg);
   return Sections[Index - 1].get();