[llvm-readobj/llvm-readelf] - Report a proper warning when dumping a broken dynamic relocation.

[grimar]

Summary

When we have a dynamic relocation with a broken symbol's st_name,
tools report a useless error: "Invalid data was encountered while parsing the file".

After this change we report a warning + "<corrupt>" as a symbol name.

[MaskRay]

test/tools/llvm-readobj/elf-broken-dynamic-reloc-name.test

31 ↗

(On Diff #217119)

Address and Info can be deleted.

50 ↗

(On Diff #217119)

Is the PT_LOAD used?

[grimar]

• Addressed review comments.

test/tools/llvm-readobj/elf-broken-dynamic-reloc-name.test

31 ↗

(On Diff #217119)

Right, removed.

50 ↗

(On Diff #217119)

Yeah. Otherwise here we'd have "Unable to parse DT_RELA: virtual address is not in any segment" error.

[rupprecht]

include/llvm/Object/ELFTypes.h

251–255 ↗

(On Diff #217119)

nit: use createStringError w/ a format string

tools/llvm-readobj/ELFDumper.cpp

3539 ↗

(On Diff #217119)

using Elf_Sym = typename ELFT::Sym at the start of this method should allow you to keep it like before (and is more idiomatic)

[grimar]

include/llvm/Object/ELFTypes.h

251–255 ↗

(On Diff #217119)

I think this what I initially did (used createStringError) , but it did not compile for me under linux (I am using windows for work most of the time), seems it wanted me to include a ELF.h header, but I was not sure it is a good idea to do that.

I'll revisit.

[MaskRay]

include/llvm/Object/ELFTypes.h

251–255 ↗

(On Diff #217119)

return createStringError(
    object_error::parse_failed,
    "st_name (0x" + utohexstr(Offset) +
        ") is past the end of the string table of size 0x" +
        utohexstr(StrTab.size()));

should work. createStringError is defined in include/llvm/Support/Error.h which is included

tools/llvm-readobj/ELFDumper.cpp

3539 ↗

(On Diff #217119)

ELFT::Sym is used once. I think it is fine to keep it as is.

[grimar]

include/llvm/Object/ELFTypes.h

251–255 ↗

(On Diff #217119)

Yeah, I found that tried to use createError from 'ELF.h'. But createStringError works fine here.

[grimar]

• Addressed review comments.

tools/llvm-readobj/ELFDumper.cpp

3539 ↗

(On Diff #217119)

I also came to this. We often use using or declaring vars when have multiple usings, but here we don't, so it would be one more line which we can avoid probably.

[jhenderson]

include/llvm/Object/ELFTypes.h

253 ↗

(On Diff #217157)

I think you want to replace utohextr(Offset) with PRIx32 to take advantage of createStringError's printf formatting rules:

return createStringError(object_error::parse_failed,
    "st_name (0x" PRIx32 ") is past the end of the string table"
    " of size 0x%zx", Offset, StrTab.size());

test/tools/llvm-readobj/elf-broken-dynamic-reloc-name.test

1 ↗

(On Diff #217157)

if a dynamic

14 ↗

(On Diff #217157)

I think the offset here is irrelevant, right? Can it be replaced with {{.+}}?

tools/llvm-readobj/ELFDumper.cpp

3537 ↗

(On Diff #217157)

Maybe R -> Reloc

3547 ↗

(On Diff #217157)

I'd replace "a dynamic symbol" with "the dynamic symbol"

[grimar]

• Addressed review comments.

include/llvm/Object/ELFTypes.h

253 ↗

(On Diff #217157)

Ok. I am not in favour of using format strings usually, because remembering non trivial format specifiers
is hard. And I also faced wierd issues before because of wrong specifer used by mistake (or because of a variable type change).

But since both you and Jordan think it is better.. Done.

test/tools/llvm-readobj/elf-broken-dynamic-reloc-name.test

14 ↗

(On Diff #217157)

Yes, done. BTW, I searched for "at offset" and noticed our tests are using {{.*}} for testing this line which is not entirely correct.

[jhenderson]

LGTM.

test/tools/llvm-readobj/elf-broken-dynamic-reloc-name.test

14 ↗

(On Diff #217157)

I think in most cases it probably doesn't matter either way, as we aren't really testing the printing of this line.

llvm/trunk/include/llvm/Object/ELFTypes.h

struct Elf_Sym_Impl : Elf_Sym_Base<ELFT> {
 
   Expected<StringRef> getName(StringRef StrTab) const;
 };
 
 template <class ELFT>
 Expected<StringRef> Elf_Sym_Impl<ELFT>::getName(StringRef StrTab) const {
   uint32_t Offset = this->st_name;
   if (Offset >= StrTab.size())
-    return errorCodeToError(object_error::parse_failed);
+    return createStringError(object_error::parse_failed,
+                             "st_name (0x%" PRIx32
+                             ") is past the end of the string table"
+                             " of size 0x%zx",
+                             Offset, StrTab.size());
   return StringRef(StrTab.data() + Offset);
 }
 
 /// Elf_Versym: This is the structure of entries in the SHT_GNU_versym section
 /// (.gnu.version). This structure is identical for ELF32 and ELF64.
 template <class ELFT>
 struct Elf_Versym_Impl {
   LLVM_ELF_IMPORT_TYPES_ELFT(ELFT)

llvm/trunk/test/Object/invalid.test

- Name:     .foo
     ShOffset: 0xFFFF0000
 
 ## Check that llvm-readobj reports an error if symbol name
 ## offset goes past the end of the symbol string table.
 
 # RUN: yaml2obj %s --docnum=19 -o %t19
 # RUN: not llvm-readobj --symbols %t19 2>&1 | FileCheck -DFILE=%t19 --check-prefix=INVALID-SYM-NAME %s
 
-# INVALID-SYM-NAME: error: '[[FILE]]': Invalid data was encountered while parsing the file
+# INVALID-SYM-NAME: error: '[[FILE]]': st_name (0x1) is past the end of the string table of size 0x1
 
 --- !ELF
 FileHeader:
   Class:   ELFCLASS64
   Data:    ELFDATA2LSB
   Type:    ET_REL
   Machine: EM_X86_64
 Sections:

llvm/trunk/test/tools/llvm-readobj/elf-broken-dynamic-reloc-name.test

+## Check that llvm-readobj/llvm-readelf reports an error when dumping relocations if a dynamic
+## symbol name offset is broken (goes past the end of the dynamic symbol string table).
+
+# RUN: yaml2obj %s -o %t
+# RUN: llvm-readobj --dyn-relocations %t 2>&1 | FileCheck %s -DFILE=%t --check-prefix=LLVM
+# RUN: llvm-readelf --dyn-relocations %t 2>&1 | FileCheck %s -DFILE=%t --check-prefix=GNU
+
+# LLVM:      Dynamic Relocations {
+# LLVM-EMPTY:
+# LLVM-NEXT: warning: '[[FILE]]': unable to get name of the dynamic symbol with index 1: st_name (0x1234) is past the end of the string table of size 0x1
+# LLVM-NEXT:   0x0 R_X86_64_NONE <corrupt> 0x0
+# LLVM-NEXT: }
+
+# GNU:      'RELA' relocation section at offset {{.+}} contains 24 bytes:
+# GNU-NEXT: Offset           Info             Type          Symbol's Value   Symbol's Name + Addend
+# GNU-EMPTY:
+# GNU-NEXT: warning: '[[FILE]]': unable to get name of the dynamic symbol with index 1: st_name (0x1234) is past the end of the string table of size 0x1
+# GNU-NEXT: 0000000000000000 0000000100000000 R_X86_64_NONE 0000000000000000 <corrupt> + 0
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_DYN
+  Machine: EM_X86_64
+Sections:
+  - Name:    .rela.dyn
+    Type:    SHT_RELA
+    Link:    .dynsym
+    Relocations:
+      - Offset: 0x0
+        Symbol: 1 ## Index of a dynamic symbol with a broken st_name.
+        Type:   R_X86_64_NONE
+  - Name: .dynamic
+    Type: SHT_DYNAMIC
+    Entries:
+      - Tag:   DT_RELA
+        Value: 0x0000000000000000
+      - Tag:   DT_RELASZ
+        Value: 0x0000000000000018
+      - Tag:   DT_RELAENT
+        Value: 0x0000000000000018
+      - Tag:   DT_NULL
+        Value: 0x0000000000000000
+DynamicSymbols:
+  - NameIndex: 0x1234
+ProgramHeaders:
+  - Type:  PT_LOAD
+    Sections:
+      - Section: .rela.dyn
+      - Section: .dynamic

llvm/trunk/tools/llvm-readobj/ELFDumper.cpp

if (BelongsToSegment.find(&Sec) == BelongsToSegment.end())
           unwrapOrError(this->FileName, Obj->getSectionName(&Sec)).str() + ' ';
   }
   if (!Sections.empty()) {
     OS << "   None  " << Sections << '\n';
     OS.flush();
   }
 }
 
+namespace {
+template <class ELFT> struct RelSymbol {
+  const typename ELFT::Sym *Sym;
+  std::string Name;
+};
+
+template <class ELFT>
+RelSymbol<ELFT> getSymbolForReloc(const ELFFile<ELFT> *Obj, StringRef FileName,
+                                  const ELFDumper<ELFT> *Dumper,
+                                  const typename ELFT::Rela &Reloc) {
+  uint32_t SymIndex = Reloc.getSymbol(Obj->isMips64EL());
+  const typename ELFT::Sym *Sym = Dumper->dynamic_symbols().begin() + SymIndex;
+  Expected<StringRef> ErrOrName = Sym->getName(Dumper->getDynamicStringTable());
+
+  std::string Name;
+  if (ErrOrName) {
+    Name = maybeDemangle(*ErrOrName);
+  } else {
+    reportWarning(
+        createError("unable to get name of the dynamic symbol with index " +
+                    Twine(SymIndex) + ": " + toString(ErrOrName.takeError())),
+        FileName);
+    Name = "<corrupt>";
+  }
+
+  return {Sym, std::move(Name)};
+}
+} // namespace
+
 template <class ELFT>
 void GNUStyle<ELFT>::printDynamicRelocation(const ELFO *Obj, Elf_Rela R,
                                             bool IsRela) {
-  uint32_t SymIndex = R.getSymbol(Obj->isMips64EL());
-  const Elf_Sym *Sym = this->dumper()->dynamic_symbols().begin() + SymIndex;
-  std::string SymbolName = maybeDemangle(unwrapOrError(
-      this->FileName, Sym->getName(this->dumper()->getDynamicStringTable())));
-  printRelocation(Obj, Sym, SymbolName, R, IsRela);
+  RelSymbol<ELFT> S = getSymbolForReloc(Obj, this->FileName, this->dumper(), R);
+  printRelocation(Obj, S.Sym, S.Name, R, IsRela);
 }
 
 template <class ELFT> void GNUStyle<ELFT>::printDynamic(const ELFO *Obj) {
   Elf_Dyn_Range Table = this->dumper()->dynamic_table();
   if (Table.empty())
     return;
 
   const DynRegionInfo &DynamicTableRegion =
void LLVMStyle<ELFT>::printDynamicRelocations(const ELFO *Obj) {
   W.unindent();
   W.startLine() << "}\n";
 }
 
 template <class ELFT>
 void LLVMStyle<ELFT>::printDynamicRelocation(const ELFO *Obj, Elf_Rela Rel) {
   SmallString<32> RelocName;
   Obj->getRelocationTypeName(Rel.getType(Obj->isMips64EL()), RelocName);
-  std::string SymbolName;
-  uint32_t SymIndex = Rel.getSymbol(Obj->isMips64EL());
-  const Elf_Sym *Sym = this->dumper()->dynamic_symbols().begin() + SymIndex;
-  SymbolName = maybeDemangle(unwrapOrError(
-      this->FileName, Sym->getName(this->dumper()->getDynamicStringTable())));
+  std::string SymbolName =
+      getSymbolForReloc(Obj, this->FileName, this->dumper(), Rel).Name;
+
   if (opts::ExpandRelocs) {
     DictScope Group(W, "Relocation");
     W.printHex("Offset", Rel.r_offset);
     W.printNumber("Type", RelocName, (int)Rel.getType(Obj->isMips64EL()));
     W.printString("Symbol", !SymbolName.empty() ? SymbolName : "-");
     W.printHex("Addend", Rel.r_addend);
   } else {
     raw_ostream &OS = W.startLine();