[llvm-readobj] Don't abort printing of dynamic table if string reference is invalid

[ychen]

Summary

If dynamic table is missing, output "dynamic strtab not found'. If the index is
out of range, output "Invalid Offset<..>".

https://bugs.llvm.org/show_bug.cgi?id=40807

[jhenderson]

I already gave this an LGTM offline, and that still applies. Somebody else should give this a quick look through though too.

[jhenderson]

As you've got D63115 in flight which fixes the FIXME in the test, maybe it would be better holding off committing this until that lands, and then you can avoid having the FIXME at all. What do you think?

[grimar]

Few comments are below. James, what do you think?

llvm/test/tools/llvm-readobj/elf-dynamic-malformed.test

157 ↗

(On Diff #203863)

<not found> is inconsistent with library name not found below.
Should both use <..>?

llvm/tools/llvm-readobj/ELFDumper.cpp

1779 ↗

(On Diff #203863)

It is a bit inconsistent.
I think you need to use upper/lower case for both messages, but do not mix.
I.e dynamic strtab not found -> Dynamic strtab not found.

I would also probably rewrite to "String table is empty or was not found" or something like that.
Because it is possible (in theory) that DT_STRSZ == 0, but DT_STRTAB points to somewhere.

[grimar]

llvm/test/tools/llvm-readobj/elf-dynamic-malformed.test

162 ↗

(On Diff #203863)

Perhaps it should use different forms too?

Shared library: [valid_library_name.so]
Shared library: <dynamic strtab not found>

[jhenderson]

llvm/test/tools/llvm-readobj/elf-dynamic-malformed.test

157 ↗

(On Diff #203863)

As we might change the below output based on GNU, let's make it consistent with that style, if it makes sense. If GNU doesn't print with any markers, then I'm happy with either style, but consistency would be good.

162 ↗

(On Diff #203863)

What does GNU's output look like? If it produces an inline message here, we should probably mirror that, thinking about it.

[ychen]

In D63084#1537606, @jhenderson wrote:

As you've got D63115 in flight which fixes the FIXME in the test, maybe it would be better holding off committing this until that lands, and then you can avoid having the FIXME at all. What do you think?

Yes, that sounds great.

[ychen]

• update

[ychen]

llvm/test/tools/llvm-readobj/elf-dynamic-malformed.test

157 ↗

(On Diff #203863)

done.

162 ↗

(On Diff #203863)

GNU only print the index
0x0000000000000001 (NEEDED) 0x1

162 ↗

(On Diff #203863)

Agree.

[ychen]

• Match GNU output for DT_RPATH and DT_RUNPATH.
• All diagnose messages enclosed with <>.
• Test updated accordingly.

[ychen]

• update

[jhenderson]

My instinct is to say that the RPATH change should be a separate change, since it's not just about the improved error/warning. Please could you split it off (we still want it, just not in a commit to do with error messages). LGTM, aside from that.

[MaskRay]

llvm/tools/llvm-readobj/ELFDumper.cpp

1942 ↗

(On Diff #204158)

DynamicStringTable.empty()

1947 ↗

(On Diff #204158)

O->o i.e. <Invalid offset 0x?

Alternatively, make it all lowercase: <invalid offset 0x. I'm just not sure which style is more common in this file.

[MaskRay]

llvm/tools/llvm-readobj/ELFDumper.cpp

1998 ↗

(On Diff #204158)

I think not found is not entirely accurate. The issue is that the index into the string table is invalid (out of range). Do other reviewers have suggestion for the message used here?

[jhenderson]

llvm/tools/llvm-readobj/ELFDumper.cpp

1998 ↗

(On Diff #204158)

You're probably right. It should probably be something like <library name index out of range> or <library name index invalid> like you suggest.

[ychen]

Address reviewer's comments.

• capitalize first char of diagnose strings.
• split out DT_RPATH & DT_RUNPATH code.

[ychen]

llvm/tools/llvm-readobj/ELFDumper.cpp

1942 ↗

(On Diff #204158)

done

1947 ↗

(On Diff #204158)

done

1998 ↗

(On Diff #204158)

Went with <Library name index out of range>

[jhenderson]

Looks good, with the suggested changes to the if statements.

llvm/tools/llvm-readobj/ELFDumper.cpp

1794 ↗

(On Diff #204335)

LLVM standard has the body of an if on a new line, I believe, i.e.

if (WithBracket)
  OS << "[";

1796 ↗

(On Diff #204335)

Ditto

1942 ↗

(On Diff #204158)

Tip: you can mark individual comments as "Done" by clicking the "Done" button in the comment box header. I find it quite useful when reviewing code if that has been done.

[MaskRay]

D62072 seems to promote lowercasing the first letter in error messages.

[jhenderson]

In D63084#1541260, @MaskRay wrote:

D62072 seems to promote lowercasing the first letter in error messages.

Yes, I agree with that, although it's debatable as to whether these are the same, since they aren't independent error messages printed on stderr, so the guidelines might not be the same. I don't really know though!

[ychen]

• update

[ychen]

Thank you @jhenderson @MaskRay. If this looks good, could you commit it for me?

llvm/tools/llvm-readobj/ELFDumper.cpp

1942 ↗

(On Diff #204158)

Oh, that's much better.

[jhenderson]

@grimar, are you happy for me to land this patch?

[grimar]

In D63084#1543232, @jhenderson wrote:

@grimar, are you happy for me to land this patch?

Sure.

llvm/trunk/test/Object/corrupt.test

 
 
 // Version index in .gnu.version overflows the version map.
 RUN: not llvm-readobj %p/Inputs/corrupt-version.elf-x86_64 -dt \
 RUN:     2>&1 | FileCheck --check-prefix=VER %s
 
 VER: error: Invalid version entry
 
-
-// The file is missing the dynamic string table but has references to it.
-RUN: not llvm-readobj --dynamic-table %p/Inputs/corrupt-invalid-strtab.elf.x86-64 \
-RUN:     2>&1 | FileCheck --check-prefix=STRTAB %s
-
-STRTAB: Invalid dynamic string table reference
-
 RUN: not llvm-readobj -l \
 RUN:   %p/Inputs/corrupt-invalid-phentsize.elf.x86-64 2>&1 | \
 RUN:   FileCheck --check-prefix=PHENTSIZE %s
 
 PHENTSIZE: invalid e_phentsize
 
 RUN: llvm-readobj --dynamic-table \
 RUN:   %p/Inputs/corrupt-invalid-virtual-addr.elf.x86-64 2>&1 | \

llvm/trunk/test/tools/llvm-readobj/elf-dynamic-malformed.test

- Type: PT_LOAD
     VAddr: 0x1000
     Sections:
       - Section: .dynamic
   - Type: PT_DYNAMIC
     VAddr: 0x1000
     Sections:
       - Section: .dynamic
 
-# Test handling of string references pointing past the end of the dynamic string table. In this case,
-# we have a DT_NEEDED tag pointing at offset 1 in a 1-byte string table.
+# Test handling of string references pointing past the end of the dynamic string table.
 # RUN: yaml2obj %s --docnum=3 -o %t.bad-string
-# RUN: not llvm-readobj --dynamic-table %t.bad-string 2>&1 | FileCheck %s --check-prefix BAD-STRING
-# RUN: not llvm-readelf --dynamic-table %t.bad-string 2>&1 | FileCheck %s --check-prefix BAD-STRING
+# RUN: llvm-readobj --dynamic-table %t.bad-string | FileCheck %s --check-prefix BAD-STRING-LLVM
+# RUN: llvm-readelf --dynamic-table %t.bad-string | FileCheck %s --check-prefix BAD-STRING-GNU
 
-# BAD-STRING: Invalid dynamic string table reference
+# BAD-STRING-LLVM: 0x000000000000000A STRSZ         1 (bytes)
+# BAD-STRING-LLVM: 0x0000000000000001 NEEDED        Shared library: <Invalid offset 0x1>
+# BAD-STRING-LLVM: 0x000000007FFFFFFF FILTER        Filter library: <Invalid offset 0x1>
+# BAD-STRING-LLVM: 0x000000007FFFFFFD AUXILIARY     Auxiliary library: <Invalid offset 0x1>
+# BAD-STRING-LLVM: 0x000000007FFFFFFE USED          Not needed object: <Invalid offset 0x1>
+# BAD-STRING-LLVM: 0x000000000000000E SONAME        Library soname: <Invalid offset 0x1>
+# BAD-STRING-LLVM: 0x000000000000000F RPATH         <Invalid offset 0x1>
+# BAD-STRING-LLVM: 0x000000000000001D RUNPATH       <Invalid offset 0x1>
+
+# BAD-STRING-GNU:  0x000000000000000a (STRSZ)       1 (bytes)
+# BAD-STRING-GNU:  0x0000000000000001 (NEEDED)      Shared library: <Invalid offset 0x1>
+# BAD-STRING-GNU:  0x000000007fffffff (FILTER)      Filter library: <Invalid offset 0x1>
+# BAD-STRING-GNU:  0x000000007ffffffd (AUXILIARY)   Auxiliary library: <Invalid offset 0x1>
+# BAD-STRING-GNU:  0x000000007ffffffe (USED)        Not needed object: <Invalid offset 0x1>
+# BAD-STRING-GNU:  0x000000000000000e (SONAME)      Library soname: <Invalid offset 0x1>
+# BAD-STRING-GNU:  0x000000000000000f (RPATH)       <Invalid offset 0x1>
+# BAD-STRING-GNU:  0x000000000000001d (RUNPATH)     <Invalid offset 0x1>
 
 --- !ELF
 FileHeader:
   Class:   ELFCLASS64
   Data:    ELFDATA2LSB
   Type:    ET_EXEC
   Machine: EM_X86_64
 Sections:
   - Name:    .dynstr
     Type:    SHT_STRTAB
     Address: 0x1000
   - Name:    .dynamic
     Type:    SHT_DYNAMIC
     Address: 0x1010
     Entries:
       - Tag:   DT_STRTAB
         Value: 0x1000
       - Tag:   DT_STRSZ
         Value: 1
       - Tag:   DT_NEEDED
         Value: 1
+      - Tag:   DT_FILTER
+        Value: 1
+      - Tag:   DT_AUXILIARY
+        Value: 1
+      - Tag:   DT_USED
+        Value: 1
+      - Tag:   DT_SONAME
+        Value: 1
+      - Tag:   DT_RPATH
+        Value: 1
+      - Tag:   DT_RUNPATH
+        Value: 1
       - Tag:   DT_NULL
         Value: 0
 ProgramHeaders:
   - Type: PT_LOAD
     VAddr: 0x1000
     Sections:
       - Section: .dynstr
       - Section: .dynamic
   - Type: PT_DYNAMIC
     VAddr: 0x1010
     Sections:
       - Section: .dynamic
 
 # Test handling of DT_STRTAB pointing outside the file's address space.
 # RUN: yaml2obj %s --docnum=4 -o %t.bad-strtab
-# RUN: not llvm-readobj --dynamic-table %t.bad-strtab 2>&1 | FileCheck %s --check-prefix BAD-STRTAB
-# RUN: not llvm-readelf --dynamic-table %t.bad-strtab 2>&1 | FileCheck %s --check-prefix BAD-STRTAB
 
-# BAD-STRTAB: warning: Unable to parse DT_STRTAB: Virtual address is not in any segment
-# BAD-STRTAB: error: Invalid dynamic string table reference
+# RUN: llvm-readobj --dynamic-table %t.bad-strtab 2>&1 >/dev/null | FileCheck %s --check-prefix BAD-STRTAB-ERR
+# RUN: llvm-readelf --dynamic-table %t.bad-strtab 2>&1 >/dev/null | FileCheck %s --check-prefix BAD-STRTAB-ERR
+# BAD-STRTAB-ERR: warning: Unable to parse DT_STRTAB: Virtual address is not in any segment
+
+# RUN: llvm-readobj --dynamic-table --needed-libs %t.bad-strtab | FileCheck %s --check-prefixes=BAD-STRTAB,BAD-STRTAB-LLVM
+# RUN: llvm-readelf --dynamic-table --needed-libs %t.bad-strtab | FileCheck %s --check-prefixes=BAD-STRTAB,BAD-STRTAB-GNU
+# BAD-STRTAB-LLVM: LoadName: <Not found>
+# BAD-STRTAB-LLVM: 0x0000000000000001  NEEDED   Shared library: <String table is empty or was not found>
+# BAD-STRTAB-GNU:  0x0000000000000001 (NEEDED)  Shared library: <String table is empty or was not found>
+# BAD-STRTAB:      NeededLibraries [
+# BAD-STRTAB:        <Library name index out of range>
+# BAD-STRTAB:      ]
 
 --- !ELF
 FileHeader:
   Class:   ELFCLASS64
   Data:    ELFDATA2LSB
   Type:    ET_EXEC
   Machine: EM_X86_64
 Sections:

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

private:
   DynRegionInfo createDRIFrom(const Elf_Shdr *S) {
     return checkDRI(
         {ObjF->getELFFile()->base() + S->sh_offset, S->sh_size, S->sh_entsize});
   }
 
   void loadDynamicTable(const ELFFile<ELFT> *Obj);
   void parseDynamicTable();
 
-  StringRef getDynamicString(uint64_t Offset) const;
+  void printDynamicString(uint64_t Offset, raw_ostream &OS,
+                          bool WithBracket = true) const;
   StringRef getSymbolVersion(StringRef StrTab, const Elf_Sym *symb,
                              bool &IsDefault) const;
   void LoadVersionMap() const;
   void LoadVersionNeeds(const Elf_Shdr *ec) const;
   void LoadVersionDefs(const Elf_Shdr *sec) const;
 
   const object::ELFObjectFile<ELFT> *ObjF;
   DynRegionInfo DynRelRegion;
   DynRegionInfo DynRelaRegion;
   DynRegionInfo DynRelrRegion;
   DynRegionInfo DynPLTRelRegion;
   DynRegionInfo DynSymRegion;
   DynRegionInfo DynamicTable;
   StringRef DynamicStringTable;
-  StringRef SOName;
+  StringRef SOName = "<Not found>";
   const Elf_Hash *HashTable = nullptr;
   const Elf_GnuHash *GnuHashTable = nullptr;
   const Elf_Shdr *DotSymtabSec = nullptr;
   const Elf_Shdr *DotCGProfileSec = nullptr;
   const Elf_Shdr *DotAddrsigSec = nullptr;
   StringRef DynSymtabName;
   ArrayRef<Elf_Word> ShndxTable;
 
case ELF::DT_JMPREL:
       break;
     case ELF::DT_PLTRELSZ:
       DynPLTRelRegion.Size = Dyn.getVal();
       break;
     }
   }
   if (StringTableBegin)
     DynamicStringTable = StringRef(StringTableBegin, StringTableSize);
-  if (SONameOffset)
-    SOName = getDynamicString(SONameOffset);
+  if (SONameOffset && SONameOffset < DynamicStringTable.size())
+    SOName = DynamicStringTable.data() + SONameOffset;
 }
 
 template <typename ELFT>
 typename ELFDumper<ELFT>::Elf_Rel_Range ELFDumper<ELFT>::dyn_rels() const {
   return DynRelRegion.getAsArrayRef<Elf_Rel>();
 }
 
 template <typename ELFT>
void printFlags(T Value, ArrayRef<EnumEntry<TFlag>> Flags, raw_ostream &OS) {
   }
 
   for (const auto &Flag : SetFlags) {
     OS << Flag.Name << " ";
   }
 }
 
 template <class ELFT>
-StringRef ELFDumper<ELFT>::getDynamicString(uint64_t Value) const {
-  if (Value >= DynamicStringTable.size())
-    reportError("Invalid dynamic string table reference");
-  return StringRef(DynamicStringTable.data() + Value);
-}
-
-static void printLibrary(raw_ostream &OS, const Twine &Tag, const Twine &Name) {
-  OS << Tag << ": [" << Name << "]";
+void ELFDumper<ELFT>::printDynamicString(uint64_t Value,
+                                         raw_ostream &OS,
+                                         bool WithBracket) const {
+  if (DynamicStringTable.empty())
+    OS << "<String table is empty or was not found> ";
+  else if (Value < DynamicStringTable.size()) {
+    if (WithBracket)
+      OS << "[";
+    OS << StringRef(DynamicStringTable.data() + Value);
+    if (WithBracket)
+      OS << "]";
+  } else
+    OS << "<Invalid offset 0x" << utohexstr(Value) << ">";
 }
 
 template <class ELFT>
 void ELFDumper<ELFT>::printDynamicEntry(raw_ostream &OS, uint64_t Type,
                                         uint64_t Value) const {
   const char *ConvChar =
       (opts::Output == opts::GNU) ? "0x%" PRIx64 : "0x%" PRIX64;
 
void ELFDumper<ELFT>::printDynamicEntry(raw_ostream &OS, uint64_t Type,
   case DT_INIT_ARRAYSZ:
   case DT_FINI_ARRAYSZ:
   case DT_PREINIT_ARRAYSZ:
   case DT_ANDROID_RELSZ:
   case DT_ANDROID_RELASZ:
     OS << Value << " (bytes)";
     break;
   case DT_NEEDED:
-    printLibrary(OS, "Shared library", getDynamicString(Value));
-    break;
   case DT_SONAME:
-    printLibrary(OS, "Library soname", getDynamicString(Value));
-    break;
   case DT_AUXILIARY:
-    printLibrary(OS, "Auxiliary library", getDynamicString(Value));
-    break;
   case DT_USED:
-    printLibrary(OS, "Not needed object", getDynamicString(Value));
-    break;
-  case DT_FILTER:
-    printLibrary(OS, "Filter library", getDynamicString(Value));
+  case DT_FILTER: {
+    const std::map<uint64_t, const char*> TagNames = {
+      {DT_NEEDED,    "Shared library"},
+      {DT_SONAME,    "Library soname"},
+      {DT_AUXILIARY, "Auxiliary library"},
+      {DT_USED,      "Not needed object"},
+      {DT_FILTER,    "Filter library"},
+    };
+    OS << TagNames.at(Type) << ": ";
+    printDynamicString(Value, OS);
     break;
+  }
   case DT_RPATH:
   case DT_RUNPATH:
-    OS << getDynamicString(Value);
+    printDynamicString(Value, OS, false);
     break;
   case DT_FLAGS:
     printFlags(Value, makeArrayRef(ElfDynamicDTFlags), OS);
     break;
   case DT_FLAGS_1:
     printFlags(Value, makeArrayRef(ElfDynamicDTFlags1), OS);
     break;
   default:

 
 template <class ELFT> void ELFDumper<ELFT>::printNeededLibraries() {
   ListScope D(W, "NeededLibraries");
 
   using LibsTy = std::vector<StringRef>;
   LibsTy Libs;
 
   for (const auto &Entry : dynamic_table())
-    if (Entry.d_tag == ELF::DT_NEEDED)
-      Libs.push_back(getDynamicString(Entry.d_un.d_val));
+    if (Entry.d_tag == ELF::DT_NEEDED) {
+      uint64_t Value = Entry.d_un.d_val;
+      if (Value < DynamicStringTable.size())
+        Libs.push_back(StringRef(DynamicStringTable.data() + Value));
+      else
+        Libs.push_back("<Library name index out of range>");
+    }
 
   llvm::stable_sort(Libs);
 
   for (const auto &L : Libs)
     W.startLine() << L << "\n";
 }
 
 template <typename ELFT> void ELFDumper<ELFT>::printHashTable() {