[llvm-readobj/elf] - Fix a crash when dumping a dynamic relocation that refer to a symbol past the EOF.

[grimar]

Summary

There is a possible scenario when we crash when dumping dynamic relocations.
For that we should have no section headers (to take the number of synamic symbols from)
and a dynamic relocation that refers to a symbol with an index that is too large to be in a file.

The patch fixes it.

[jhenderson]

llvm/test/tools/llvm-readobj/ELF/broken-dynamic-reloc.test

384–385

These values are presumably tied to the size of data in other implicit sections, right? Could we arrange for those sections to appear before .dynsym, as well as the .rela.dyn section? That way, the .dynsym will actually appear at the end of the file, and it would be easier to see that the symbol indexes are what you say they are.

It's probably also worth expanding on what is meant by "valid" here, since the .dynsym appears to be essentially empty (and therefore any indexes into it could be viewed as invalid).

[grimar]

• Addressed review comments.
• Now depends on D90224.

[jhenderson]

llvm/test/tools/llvm-readobj/ELF/broken-dynamic-reloc.test

380

I'm a little confused - why isn't this symbol something like 0x1 or 0x2? If the .dynsym is the very last thing in the output, then the at-EOF index should be the number of dynamic symbols.

[grimar]

llvm/test/tools/llvm-readobj/ELF/broken-dynamic-reloc.test

380

I've investigated it and found multiple reasons:

1. There is a .dynstr section, which we also write after.
2. Also we write the content of .strtab and .shstrtab implicit sections (both are empty and has size == 1). Here I think that the fact we still write the content of .shstrtab with NoHeaders: true is a bug. We don't add section names to it already, but we should just drop it from the output.
3. We still: a) Align the start of the section header table, what affects the output size. (BUG) b) We still write it. (BUG)

I am going to fix it. Though it will be a separate independent set of changes.

384–385

Done.

[jhenderson]

llvm/test/tools/llvm-readobj/ELF/broken-dynamic-reloc.test

380

Sounds reasonable, thanks!

[grimar]

• Rebased after landing D90295.
• Updated the test case.

llvm/test/tools/llvm-readobj/ELF/broken-dynamic-reloc.test

380

I've updated the test.

[jhenderson]

Great, LGTM!

llvm/test/tools/llvm-readobj/ELF/broken-dynamic-reloc.test

## 0x18 == offset of .rel.dyn == size of .rela.dyn section.
         Value: 0x0
 DynamicSymbols: []
 ProgramHeaders:
   - Type:  PT_LOAD
     Sections:
       - Section: .rela.dyn
       - Section: .rel.dyn
       - Section: .dynamic
+
+## Check that llvm-readobj/llvm-readelf reports a warning when dumping a relocation
+## which refers to a symbol past the end of the file.
+
+# RUN: yaml2obj --docnum=7 %s -o %t7
+# RUN: llvm-readobj --dyn-relocations %t7 2>&1 | \
+# RUN:   FileCheck %s -DFILE=%t7 --check-prefix=PAST-EOF-LLVM
+# RUN: llvm-readelf --dyn-relocations %t7 2>&1 | \
+# RUN:   FileCheck %s -DFILE=%t7 --check-prefix=PAST-EOF-GNU
+
+# PAST-EOF-LLVM:      Dynamic Relocations {
+# PAST-EOF-LLVM-NEXT: warning: '[[FILE]]': unable to get name of the dynamic symbol with index 1: st_name (0x1) is past the end of the string table of size 0x0
+# PAST-EOF-LLVM-NEXT:   0x0 R_X86_64_NONE <corrupt> 0x0
+# PAST-EOF-LLVM-NEXT: warning: '[[FILE]]': unable to get name of the dynamic symbol with index 2: symbol at 0x330 goes past the end of the file (0x330)
+# PAST-EOF-LLVM-NEXT:   0x0 R_X86_64_NONE <corrupt> 0x0
+# PAST-EOF-LLVM-NEXT: warning: '[[FILE]]': unable to get name of the dynamic symbol with index 4294967295: symbol at 0x18000002e8 goes past the end of the file (0x330)
+# PAST-EOF-LLVM-NEXT:   0x0 R_X86_64_NONE <corrupt> 0x0
+# PAST-EOF-LLVM-NEXT: }
+
+# PAST-EOF-GNU:      'RELA' relocation section at offset 0x200 contains 72 bytes:
+# PAST-EOF-GNU-NEXT:     Offset             Info             Type               Symbol's Value  Symbol's Name + Addend
+# PAST-EOF-GNU-NEXT: warning: '[[FILE]]': unable to get name of the dynamic symbol with index 1: st_name (0x1) is past the end of the string table of size 0x0
+# PAST-EOF-GNU-NEXT: 0000000000000000  0000000100000000 R_X86_64_NONE          0000000000000000 <corrupt> + 0
+# PAST-EOF-GNU-NEXT: warning: '[[FILE]]': unable to get name of the dynamic symbol with index 2: symbol at 0x330 goes past the end of the file (0x330)
+# PAST-EOF-GNU-NEXT: 0000000000000000  0000000200000000 R_X86_64_NONE                           <corrupt> + 0
+# PAST-EOF-GNU-NEXT: warning: '[[FILE]]': unable to get name of the dynamic symbol with index 4294967295: symbol at 0x18000002e8 goes past the end of the file (0x330)
+# PAST-EOF-GNU-NEXT: 0000000000000000  ffffffff00000000 R_X86_64_NONE                           <corrupt> + 0
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_DYN
+  Machine: EM_X86_64
+Sections:
+  - Name:    .dynamic
+    Type:    SHT_DYNAMIC
+    Flags:   [ SHF_ALLOC ]
+    Address: 0x100
+    Offset:  0x100
+    Entries:
+      - Tag:   DT_RELA
+        Value: 0x200
+      - Tag:   DT_SYMTAB
+        Value: 0x300
+      - Tag:   DT_RELASZ
+        Value: 0x48
+      - Tag:   DT_RELAENT
+        Value: 0x18
+      - Tag:   DT_NULL
+        Value: 0x0
+  - Name:    .rela.dyn
+    Type:    SHT_RELA
+    Flags:   [ SHF_ALLOC ]
+    Address: 0x200
+    Offset:  0x200
+    Relocations:
+## This symbol is located right before the EOF, so we can dump it.
+      - Symbol: 0x1
+        Type:   R_X86_64_NONE
+## The next symbol, which goes past the EOF.
+      - Symbol: 0x2
+        Type:   R_X86_64_NONE
+## One more symbol that goes past the EOF
+## with the maximal possible index.
+      - Symbol: 0xFFFFFFFF
+        Type:   R_X86_64_NONE
+## Place all implicit sections here to make the .dynsym section to be the
+## last in the file. This makes the task of validating offsets a bit easier.
+  - Name: .dynstr
+    Type: SHT_STRTAB
+  - Name: .strtab
+    Type: SHT_STRTAB
+  - Name: .shstrtab
+    Type: SHT_STRTAB
+  - Name:    .dynsym
+    Type:    SHT_DYNSYM
+    Flags:   [ SHF_ALLOC ]
+    Address: 0x300
+    Offset:  0x300
+DynamicSymbols:
+  - Name: foo
+ProgramHeaders:
+  - Type:   PT_LOAD
+    Offset: 0x0
+    Sections:
+      - Section: .dynamic
+      - Section: .rela.dyn
+      - Section: .dynsym
+  - Type: PT_DYNAMIC
+    Sections:
+      - Section: .dynamic
+SectionHeaderTable:
+  NoHeaders: true

llvm/tools/llvm-readobj/ELFDumper.cpp

 }
 
 namespace {
 
 template <class ELFT>
 RelSymbol<ELFT> getSymbolForReloc(const ELFFile<ELFT> &Obj, StringRef FileName,
                                   const ELFDumper<ELFT> &Dumper,
                                   const Relocation<ELFT> &Reloc) {
-  auto WarnAndReturn = [&](const typename ELFT::Sym *Sym,
+  using Elf_Sym = typename ELFT::Sym;
+  auto WarnAndReturn = [&](const Elf_Sym *Sym,
                            const Twine &Reason) -> RelSymbol<ELFT> {
     reportWarning(
         createError("unable to get name of the dynamic symbol with index " +
                     Twine(Reloc.Symbol) + ": " + Reason),
         FileName);
     return {Sym, "<corrupt>"};
   };
 
-  ArrayRef<typename ELFT::Sym> Symbols = Dumper.dynamic_symbols();
-  const typename ELFT::Sym *FirstSym = Symbols.begin();
+  ArrayRef<Elf_Sym> Symbols = Dumper.dynamic_symbols();
+  const Elf_Sym *FirstSym = Symbols.begin();
   if (!FirstSym)
     return WarnAndReturn(nullptr, "no dynamic symbol table found");
 
   // We might have an object without a section header. In this case the size of
   // Symbols is zero, because there is no way to know the size of the dynamic
   // table. We should allow this case and not print a warning.
   if (!Symbols.empty() && Reloc.Symbol >= Symbols.size())
     return WarnAndReturn(
         nullptr,
         "index is greater than or equal to the number of dynamic symbols (" +
             Twine(Symbols.size()) + ")");
 
-  const typename ELFT::Sym *Sym = FirstSym + Reloc.Symbol;
+  const uint64_t FileSize = Obj.getBufSize();
+  const uint64_t SymOffset = ((const uint8_t *)FirstSym - Obj.base()) +
+                             (uint64_t)Reloc.Symbol * sizeof(Elf_Sym);
+  if (SymOffset + sizeof(Elf_Sym) > FileSize)
+    return WarnAndReturn(nullptr, "symbol at 0x" + Twine::utohexstr(SymOffset) +
+                                      " goes past the end of the file (0x" +
+                                      Twine::utohexstr(FileSize) + ")");
+
+  const Elf_Sym *Sym = FirstSym + Reloc.Symbol;
   Expected<StringRef> ErrOrName = Sym->getName(Dumper.getDynamicStringTable());
   if (!ErrOrName)
     return WarnAndReturn(Sym, toString(ErrOrName.takeError()));
 
   return {Sym == FirstSym ? nullptr : Sym, maybeDemangle(*ErrOrName)};
 }
 } // namespace