[llvm-readelf/llvm-readobj] - Improve dumping of broken versioning sections.

[grimar]

Summary

This updates the elf-invalid-versioning.test test case:
makes a cleanup, adds llvm-readobj calls and fixes 2
crash/assert issues I've found (test cases are provided).

[grimar]

Going to remove a dependency on D68704.

[grimar]

• Removed dependency on D68704.

[grimar]

• Rebased. Ping.

[jhenderson]

llvm/test/tools/llvm-readobj/elf-invalid-versioning.test

154

I found it slightly hard to parse this last sentence. Perhaps change != 0 to non-zero.

llvm/tools/llvm-readobj/ELFDumper.cpp

671

This and the other errors here shouldn't use report_fatal_error. They should use reportError or similar. I'm okay with that being a separate change, but perhaps it should be first to avoid adding another bad usage.

The message could include more context, for example giving the offset or similar, so that a user might be able to identify the bad entry.

3927–3928

You probably need a test case for vn_file > StringTable.size() (but the string table exists) and vn_file == StringTable.size().

3941–3943

You probably need a test case for vna_name == StringTable.size().

5720–5723

Same comment as GNU style.

5735–5738

Same comment as GNU style.

[grimar]

• Addressed review comments.

llvm/tools/llvm-readobj/ELFDumper.cpp

671

I've used reportError here instead of report_fatal_error and updated the test case accordingly.

3927–3928

It had an issue when vn_file == StringTable.size(). I've fixed the condition.

[jhenderson]

Looks good to me, aside from some minor comments.

llvm/test/tools/llvm-readobj/elf-invalid-versioning.test

195

"with the .gnu.version_r section."
"we use the same"

llvm/test/tools/llvm-readobj/elf-invalid-versioning.test

-# RUN: yaml2obj %s -o %t
-# RUN: llvm-readelf -V %t | FileCheck %s --check-prefix=INVALID
+## Here we test how llvm-readelf/llvm-readobj behave when inputs have
+## invalid versioning sections.
 
-# INVALID:      Version symbols section '.gnu.version' contains 2 entries:
-# INVALID-NEXT:  Addr: 0000000000200210  Offset: 0x000040  Link: 5 (.dynsym)
-# INVALID-NEXT:   000:   0 (*local*)       3 (*invalid*)
+## In the first case we have a SHT_GNU_versym section that refers to
+## a version listed in a SHT_GNU_verneed section. That version has an
+## empty name, making it invalid.
+
+# RUN: yaml2obj --docnum=1 %s -o %t1
+# RUN: llvm-readelf -V %t1 | FileCheck %s --check-prefix=GNU-VERNEED-NAME
+# RUN: llvm-readobj -V %t1 | FileCheck %s --check-prefix=LLVM-VERNEED-NAME
+
+# GNU-VERNEED-NAME:      Version symbols section '.gnu.version' contains 2 entries:
+# GNU-VERNEED-NAME-NEXT:  Addr: 0000000000200210  Offset: 0x000040  Link: 5 (.dynsym)
+# GNU-VERNEED-NAME-NEXT:   000:   0 (*local*)       2 (*invalid*)
+
+# GNU-VERNEED-NAME:      Version needs section '.gnu.version_r' contains 1 entries:
+# GNU-VERNEED-NAME-NEXT:  Addr: 0000000000000000  Offset: 0x000044  Link: 6 (.dynstr)
+# GNU-VERNEED-NAME-NEXT:   0x0000: Version: 1  File: somefile  Cnt: 1
+# GNU-VERNEED-NAME-NEXT:   0x0010:   Name:   Flags: none  Version: 2
+
+# LLVM-VERNEED-NAME:      VersionSymbols [
+# LLVM-VERNEED-NAME:        Symbol {
+# LLVM-VERNEED-NAME-NEXT:     Version: 0
+# LLVM-VERNEED-NAME-NEXT:     Name:
+# LLVM-VERNEED-NAME-NEXT:   }
+# LLVM-VERNEED-NAME-NEXT:   Symbol {
+# LLVM-VERNEED-NAME-NEXT:     Version: 2
+# LLVM-VERNEED-NAME-NEXT:     Name: foo
+# LLVM-VERNEED-NAME-NEXT:   }
+# LLVM-VERNEED-NAME-NEXT: ]
+
+# LLVM-VERNEED-NAME:      VersionRequirements [
+# LLVM-VERNEED-NAME-NEXT:   Dependency {
+# LLVM-VERNEED-NAME-NEXT:     Version: 1
+# LLVM-VERNEED-NAME-NEXT:     Count: 1
+# LLVM-VERNEED-NAME-NEXT:     FileName: somefile
+# LLVM-VERNEED-NAME-NEXT:     Entries [
+# LLVM-VERNEED-NAME-NEXT:       Entry {
+# LLVM-VERNEED-NAME-NEXT:         Hash:  0
+# LLVM-VERNEED-NAME-NEXT:         Flags: 0x0
+# LLVM-VERNEED-NAME-NEXT:         Index: 2
+# LLVM-VERNEED-NAME-NEXT:         Name: {{$}}
+# LLVM-VERNEED-NAME-NEXT:       }
+# LLVM-VERNEED-NAME-NEXT:     ]
+# LLVM-VERNEED-NAME-NEXT:   }
+# LLVM-VERNEED-NAME-NEXT: ]
 
 --- !ELF
 FileHeader:
-  Class:             ELFCLASS64
-  Data:              ELFDATA2LSB
-  Type:              ET_EXEC
-  Machine:           EM_X86_64
-  Entry:             0x0000000000201000
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_EXEC
+  Machine: EM_X86_64
 Sections:
-  - Name:            .gnu.version
-    Type:            SHT_GNU_versym
-    Flags:           [ SHF_ALLOC ]
-    Address:         0x0000000000200210
-    Link:            .dynsym
-    AddressAlign:    0x0000000000000002
-    EntSize:         0x0000000000000002
-    Entries:         [ 0, 3 ]
-  - Name:            .gnu.version_r
-    Type:            SHT_GNU_verneed
-    Flags:           [ SHF_ALLOC ]
-    Address:         0x0000000000200250
-    Link:            .dynstr
-    AddressAlign:    0x0000000000000004
-    Info:            0x0000000000000001
+  - Name:    .gnu.version
+    Type:    SHT_GNU_versym
+    Flags:   [ SHF_ALLOC ]
+    Address: 0x200210
+    Link:    .dynsym
+    Entries: [ 0, 2 ]
+  - Name:  .gnu.version_r
+    Type:  SHT_GNU_verneed
+    Flags: [ SHF_ALLOC ]
+    Link:  .dynstr
+    Info:  1
+    AddressAlign: 4
     Dependencies:
-      - Version:     1
-        File:        somefile
+      - Version:   1
+        File:      somefile
         Entries:
-          - Name:    ''         # invalid name
-            Hash:    1937
-            Flags:   233
-            Other:   3
+          - Name:  '' ## invalid name
+            Hash:  0
+            Flags: 0
+            Other: 2
 DynamicSymbols:
-  - Name:          f
-    Binding:       STB_GLOBAL
+  - Name:    foo
+    Binding: STB_GLOBAL
 ...
+
+## In this case SHT_GNU_verneed is not linked to a dynamic string table. We check we handle
+## this situation properly.
+
+# RUN: yaml2obj --docnum=2 %s -o %t2
+# RUN: llvm-readelf -V %t2 | FileCheck %s --check-prefix=GNU-NOLINK
+# RUN: llvm-readobj -V %t2 | FileCheck %s --check-prefix=LLVM-NOLINK
+
+# GNU-NOLINK:      Version symbols section '.gnu.version' contains 2 entries:
+# GNU-NOLINK-NEXT:  Addr: 0000000000000000  Offset: 0x000040  Link: 5 (.dynsym)
+# GNU-NOLINK-NEXT:   000:   0 (*local*)       2 (bar)
+# GNU-NOLINK:      Version needs section '.gnu.version_r' contains 1 entries:
+# GNU-NOLINK-NEXT:  Addr: 0000000000000000  Offset: 0x000044  Link: 0 ()
+# GNU-NOLINK-NEXT:   0x0000: Version: 1  File: <invalid>  Cnt: 1
+# GNU-NOLINK-NEXT:   0x0010:   Name: <invalid>  Flags: none Version: 2
+
+# LLVM-NOLINK:      VersionSymbols [
+# LLVM-NOLINK:        Symbol {
+# LLVM-NOLINK-NEXT:     Version: 0
+# LLVM-NOLINK-NEXT:     Name:
+# LLVM-NOLINK-NEXT:   }
+# LLVM-NOLINK-NEXT:   Symbol {
+# LLVM-NOLINK-NEXT:     Version: 2
+# LLVM-NOLINK-NEXT:     Name: foo@bar
+# LLVM-NOLINK-NEXT:   }
+# LLVM-NOLINK-NEXT: ]
+
+# LLVM-NOLINK:      VersionRequirements [
+# LLVM-NOLINK-NEXT:   Dependency {
+# LLVM-NOLINK-NEXT:     Version: 1
+# LLVM-NOLINK-NEXT:     Count: 1
+# LLVM-NOLINK-NEXT:     FileName: <invalid>
+# LLVM-NOLINK-NEXT:     Entries [
+# LLVM-NOLINK-NEXT:       Entry {
+# LLVM-NOLINK-NEXT:         Hash: 0
+# LLVM-NOLINK-NEXT:         Flags: 0
+# LLVM-NOLINK-NEXT:         Index: 2
+# LLVM-NOLINK-NEXT:         Name: <invalid>
+# LLVM-NOLINK-NEXT:       }
+# LLVM-NOLINK-NEXT:     ]
+# LLVM-NOLINK-NEXT:   }
+# LLVM-NOLINK-NEXT: ]
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_EXEC
+  Machine: EM_X86_64
+Sections:
+  - Name:    .gnu.version
+    Type:    SHT_GNU_versym
+    Flags:   [ SHF_ALLOC ]
+    Link:    .dynsym
+    Entries: [ 0, 2 ]
+  - Name:  .gnu.version_r
+    Type:  SHT_GNU_verneed
+    Flags: [ SHF_ALLOC ]
+    Link:  0
+    Info:  1
+    AddressAlign: 4
+    Dependencies:
+      - Version: 1
+        File:    somefile
+        Entries:
+          - Name:  'bar'
+            Hash:  0
+            Flags: 0
+            Other: 2
+DynamicSymbols:
+  - Name:    foo
+    Binding: STB_GLOBAL
+
+## We can't parse misaligned auxiliary version records.
+## Here we have a SHT_GNU_verneed section aligned by 1 byte.
+## This makes the first auxiliary record offset % 4 be != 0.
+
+# RUN: yaml2obj --docnum=3 %s -o %t3
+# RUN: not llvm-readelf -V %t3 2>&1 | FileCheck %s --check-prefix=BROKEN-AUX
+# RUN: not llvm-readobj -V %t3 2>&1 | FileCheck %s --check-prefix=BROKEN-AUX
+
+# BROKEN-AUX: LLVM ERROR: an auxiliary record found is misaligned
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_EXEC
+  Machine: EM_X86_64
+Sections:
+  - Name:    .gnu.version
+    Type:    SHT_GNU_versym
+    Flags:   [ SHF_ALLOC ]
+    Link:    .dynsym
+    Entries: [ 2 ]
+  - Name:  .gnu.version_r
+    Type:  SHT_GNU_verneed
+    Flags: [ SHF_ALLOC ]
+    Info:  1
+    AddressAlign: 1
+    Dependencies:
+      - Version: 1
+        File:    somefile
+        Entries:
+          - Name:  'bar'
+            Hash:  0
+            Flags: 0
+            Other: 2
+DynamicSymbols:
+  - Name: foo

llvm/tools/llvm-readobj/ELFDumper.cpp

if (Verneed->vn_version != ELF::VER_NEED_CURRENT)
       report_fatal_error("Unexpected verneed version");
     // Iterate through the Vernaux entries
     const uint8_t *VernauxBuf = VerneedBuf + Verneed->vn_aux;
     for (unsigned VernauxIndex = 0; VernauxIndex < Verneed->vn_cnt;
          ++VernauxIndex) {
       if (VernauxBuf + sizeof(Elf_Vernaux) > VerneedEnd)
         report_fatal_error("Section ended unexpected while scanning auxiliary "
                            "version needed records.");
+      if ((ptrdiff_t)VernauxBuf % sizeof(uint32_t) != 0)
+        report_fatal_error("an auxiliary record found is misaligned");
+
       const Elf_Vernaux *Vernaux =
           reinterpret_cast<const Elf_Vernaux *>(VernauxBuf);
       size_t Index = Vernaux->vna_other & ELF::VERSYM_VERSION;
       if (Index >= VersionMap.size())
         VersionMap.resize(Index + 1);
       VersionMap[Index] = VersionMapEntry(Vernaux);
       VernauxBuf += Vernaux->vna_next;
     }
StringRef StringTable = {
       reinterpret_cast<const char *>(Obj->base() + StrTabSec->sh_offset),
       (size_t)StrTabSec->sh_size};
 
   const uint8_t *VerneedBuf = SecData.data();
   for (unsigned I = 0; I < VerneedNum; ++I) {
     const Elf_Verneed *Verneed =
         reinterpret_cast<const Elf_Verneed *>(VerneedBuf);
 
+    StringRef File = StringTable.size() >= Verneed->vn_file
+                         ? StringTable.drop_front(Verneed->vn_file)
+                         : "<invalid>";
+
     OS << format("  0x%04x: Version: %u  File: %s  Cnt: %u\n",
                  reinterpret_cast<const uint8_t *>(Verneed) - SecData.begin(),
-                 (unsigned)Verneed->vn_version,
-                 StringTable.drop_front(Verneed->vn_file).data(),
+                 (unsigned)Verneed->vn_version, File.data(),
                  (unsigned)Verneed->vn_cnt);
 
     const uint8_t *VernauxBuf = VerneedBuf + Verneed->vn_aux;
     for (unsigned J = 0; J < Verneed->vn_cnt; ++J) {
       const Elf_Vernaux *Vernaux =
           reinterpret_cast<const Elf_Vernaux *>(VernauxBuf);
 
+      StringRef Name = StringTable.size() >= Vernaux->vna_name
+                           ? StringTable.drop_front(Vernaux->vna_name)
+                           : "<invalid>";
+
       OS << format("  0x%04x:   Name: %s  Flags: %s  Version: %u\n",
                    reinterpret_cast<const uint8_t *>(Vernaux) - SecData.begin(),
-                   StringTable.drop_front(Vernaux->vna_name).data(),
-                   versionFlagToString(Vernaux->vna_flags).c_str(),
+                   Name.data(), versionFlagToString(Vernaux->vna_flags).c_str(),
                    (unsigned)Vernaux->vna_other);
       VernauxBuf += Vernaux->vna_next;
     }
     VerneedBuf += Verneed->vn_next;
   }
   OS << '\n';
 }
 

 void LLVMStyle<ELFT>::printVersionDependencySection(const ELFFile<ELFT> *Obj,
                                                     const Elf_Shdr *Sec) {
   ListScope SD(W, "VersionRequirements");
   if (!Sec)
     return;
 
   const uint8_t *SecData =
       reinterpret_cast<const uint8_t *>(Obj->base() + Sec->sh_offset);
-  const Elf_Shdr *StrTab =
+  const Elf_Shdr *StrTabSec =
       unwrapOrError(this->FileName, Obj->getSection(Sec->sh_link));
+  StringRef StringTable = {
+      reinterpret_cast<const char *>(Obj->base() + StrTabSec->sh_offset),
+      (size_t)StrTabSec->sh_size};
 
   const uint8_t *VerneedBuf = SecData;
   unsigned VerneedNum = Sec->sh_info;
   for (unsigned I = 0; I < VerneedNum; ++I) {
     const Elf_Verneed *Verneed =
         reinterpret_cast<const Elf_Verneed *>(VerneedBuf);
     DictScope Entry(W, "Dependency");
     W.printNumber("Version", Verneed->vn_version);
     W.printNumber("Count", Verneed->vn_cnt);
-    W.printString("FileName",
-                  StringRef(reinterpret_cast<const char *>(
-                      Obj->base() + StrTab->sh_offset + Verneed->vn_file)));
+
+    StringRef FileName = StringTable.size() >= Verneed->vn_file
+                             ? StringTable.drop_front(Verneed->vn_file)
+                             : "<invalid>";
+    W.printString("FileName", FileName.data());
 
     const uint8_t *VernauxBuf = VerneedBuf + Verneed->vn_aux;
     ListScope L(W, "Entries");
     for (unsigned J = 0; J < Verneed->vn_cnt; ++J) {
       const Elf_Vernaux *Vernaux =
           reinterpret_cast<const Elf_Vernaux *>(VernauxBuf);
       DictScope Entry(W, "Entry");
       W.printNumber("Hash", Vernaux->vna_hash);
       W.printEnum("Flags", Vernaux->vna_flags, makeArrayRef(SymVersionFlags));
       W.printNumber("Index", Vernaux->vna_other);
-      W.printString("Name",
-                    StringRef(reinterpret_cast<const char *>(
-                        Obj->base() + StrTab->sh_offset + Vernaux->vna_name)));
+
+      StringRef Name = StringTable.size() >= Vernaux->vna_name
+                           ? StringTable.drop_front(Vernaux->vna_name)
+                           : "<invalid>";
+      W.printString("Name", Name.data());
       VernauxBuf += Vernaux->vna_next;
     }
     VerneedBuf += Verneed->vn_next;
   }
 }
 
 template <class ELFT>
 void LLVMStyle<ELFT>::printHashHistogram(const ELFFile<ELFT> *Obj) {