[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

that holds symbol index 0

4

-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

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

2 ↗

(On Diff #226626)

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

34 ↗

(On Diff #226626)

-o for consistency.

43 ↗

(On Diff #226626)

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

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

908 ↗

(On Diff #226626)

This looks dodgy if no symbol exists.

1426–1427 ↗

(On Diff #226626)

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

35 ↗

(On Diff #226626)

%t4 -> /dev/null

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

1426–1427 ↗

(On Diff #226626)

"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 ↗

(On Diff #226626)

"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

1426–1427 ↗

(On Diff #226626)

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 ↗

(On Diff #226626)

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 ↗

(On Diff #226626)

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

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

+## Check that we can copy an object that has a relocation
+## without symbol associated even when there is no symbol table.
+
+# RUN: yaml2obj --docnum=1 %s > %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_EXEC
+  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 relocation refers to a symbol,
+## but we have no symbol table.
+
+# RUN: yaml2obj --docnum=2 %s > %t3
+# RUN: not llvm-objcopy %t3 %t4 2>&1 | FileCheck %s --check-prefix=ERR
+
+# ERR: error: '.rel.text': relocation references a symbol (1), but there is no symbol table
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_EXEC
+  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

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() {

 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 a symbol (" +
+              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();