[llvm-readelf] - --elf-hash-histogram: do not crash when the .gnu.hash goes past the EOF.

[grimar]

Summary

llvm-readelf might crash when the .gnu.hash table goes past the EOF.

This patch splits and updates the code of a helper function checkGNUHashTable,
which is similar to checkHashTable and fixes the issue.

[MaskRay]

LGTM

[jhenderson]

llvm/test/tools/llvm-readobj/ELF/hash-histogram.test

275

This and below includes reference to 2 variables (MASKWORDS and NBUCKETS) that don't appear to be used?

Should you also add something that shows that the hash histogram isn't printed in these cases?

297

Perhaps move this comment and the similar one below, to where they are actually used.

llvm/tools/llvm-readobj/ELFDumper.cpp

2683

Again, probably in a follow-up, perhaps this should be changed to reportUniqueWarning (and similar in checkHashTable). That way, if a user requested both the hash histogram and hash tables (which seems like a not unreasonable request), they'd only get the warning once.

2706–2707

I note that printHashTable doesn't print any information at all if the hash table is bad, but this prints the header information. Perhaps (in a follow-up), printHashTable could be changed to print the header information like here?

[grimar]

This patch still depends on D80204, I'll address other comments after.

llvm/tools/llvm-readobj/ELFDumper.cpp

2706–2707

Yes, I've noticed this difference too when wrote this patch. I've addressed this and above comment here: D80373

[grimar]

llvm/tools/llvm-readobj/ELFDumper.cpp

2708–2719

It is incorrect to do this check so late I think. I'll move it earlier (https://reviews.llvm.org/D80373#inline-739398).

[grimar]

• Updated implementation.
• Addressed review comments.

[grimar]

llvm/test/tools/llvm-readobj/ELF/hash-histogram.test

275

Fixed.

297

Moved right parts.

llvm/tools/llvm-readobj/ELFDumper.cpp

2683

Fixed in this patch, because I had to change the implementation of this method to support the case when the header can be read, but the rest of the table is broken.

[jhenderson]

Looks good with one question.

llvm/test/tools/llvm-readobj/ELF/hash-histogram.test

273

Could you clarify why the no-op values have changed?

[grimar]

llvm/test/tools/llvm-readobj/ELF/hash-histogram.test

273

BloomFilter and HashBuckets both have one value (0x0) each:

## The number of words in the Bloom filter.
      MaskWords: [[MASKWORDS]]
## The number of hash buckets.
      NBuckets:  [[NBUCKETS]]
    BloomFilter: [ 0x0 ]
    HashBuckets: [ 0x0 ]

So previous values (2 and 3) were wrong. I guess I've took these arrays from another test,
simplified them, but forgot to change the no-op values previously...

llvm/test/tools/llvm-readobj/ELF/gnuhash.test

 
 # ERR:      GnuHashTable {
 # ERR-NEXT:   Num Buckets: [[NBUCKETS]]
 # ERR-NEXT:   First Hashed Symbol Index: 1
 # ERR-NEXT:   Num Mask Words: [[MASKWORDS]]
 # ERR-NEXT:   Shift Count: 2
 # ERR-NEXT: warning: '[[FILE]]': unable to dump the SHT_GNU_HASH section at 0x78: it goes past the end of the file
 # ERR-NEXT: }
+
+## Check we report a single warning about the broken GNU hash table when both
+## --gnu-hash-table and --elf-hash-histogram options are requested.
+# RUN: llvm-readelf --gnu-hash-table --elf-hash-histogram %t.err.nbuckets 2>&1 | \
+# RUN:   FileCheck %s -DFILE=%t.err.nbuckets -DMASKWORDS=2 -DNBUCKETS=4294967295 --check-prefix=ERR --implicit-check-not=warning

llvm/test/tools/llvm-readobj/ELF/hash-histogram.test

 DynamicSymbols:
   - Name: foo
 ProgramHeaders:
   - Type:  PT_LOAD
     Sections:
       - Section: .hash
       - Section: .gnu.hash
       - Section: .dynamic
+
+## Check we report a proper warning when the GNU hash table goes past the end of the file.
+
+## Case A: the 'nbuckets' field is set so that the GNU hash table goes past the end of the file.
+##         The value of 1 for the NBUCKETS is no-op.
+# RUN: yaml2obj --docnum=6 -D MASKWORDS=4294967295 -D NBUCKETS=1 %s -o %t7
+# RUN: llvm-readelf --elf-hash-histogram %t7 2>&1 | \
+# RUN:   FileCheck %s -DFILE=%t7 --check-prefix=ERR5 --implicit-check-not="Histogram"
+
+# ERR5: warning: '[[FILE]]': unable to dump the SHT_GNU_HASH section at 0x78: it goes past the end of the file
+
+## Case B: the 'maskwords' field is set so that the GNU hash table goes past the end of the file.
+##         The value of 1 for the MASKWORDS is no-op.
+# RUN: yaml2obj --docnum=6 -D MASKWORDS=1 -D NBUCKETS=4294967295 %s -o %t8
+# RUN: llvm-readelf --elf-hash-histogram %t8 2>&1 | \
+# RUN:   FileCheck %s -DFILE=%t8 --check-prefix=ERR5 --implicit-check-not="Histogram"
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_DYN
+  Machine: EM_X86_64
+Sections:
+  - Name:  .gnu.hash
+    Type:  SHT_GNU_HASH
+    Flags: [ SHF_ALLOC ]
+    Header:
+      SymNdx: 0x0
+      Shift2: 0x0
+## The number of words in the Bloom filter.
+      MaskWords: [[MASKWORDS]]
+## The number of hash buckets.
+      NBuckets:  [[NBUCKETS]]
+    BloomFilter: [ 0x0 ]
+    HashBuckets: [ 0x0 ]
+    HashValues:  [ 0x0 ]
+  - Name:  .dynamic
+    Type:  SHT_DYNAMIC
+    Flags: [ SHF_ALLOC ]
+    Link:  .dynstr
+    Entries:
+      - Tag:   DT_GNU_HASH
+        Value: 0x0
+      - Tag:   DT_NULL
+        Value: 0x0
+DynamicSymbols: []
+ProgramHeaders:
+  - Type: PT_LOAD
+    Sections:
+      - Section: .gnu.hash
+      - Section: .dynamic

llvm/tools/llvm-readobj/ELFDumper.cpp

static bool checkHashTable(const ELFFile<ELFT> *Obj,
 
   if (Obj->getBufSize() - SecOffset - HeaderSize <
       ((uint64_t)H->nbucket + H->nchain) * sizeof(typename ELFT::Word))
     return WarnAndReturn(SecOffset, ", nbucket = " + Twine(H->nbucket) +
                                         ", nchain = " + Twine(H->nchain));
   return true;
 }
 
+template <class ELFT>
+static Error checkGNUHashTable(const ELFFile<ELFT> *Obj,
+                               const typename ELFT::GnuHash *GnuHashTable,
+                               bool *IsHeaderValid = nullptr) {
+  const uint8_t *TableData = reinterpret_cast<const uint8_t *>(GnuHashTable);
+  assert(TableData >= Obj->base() &&
+         TableData < Obj->base() + Obj->getBufSize() &&
+         "GnuHashTable must always point to a location inside the file");
+
+  uint64_t TableOffset = TableData - Obj->base();
+  if (IsHeaderValid)
+    *IsHeaderValid = TableOffset + /*Header size:*/ 16 < Obj->getBufSize();
+  if (TableOffset + 16 + GnuHashTable->nbuckets * 4 +
+          GnuHashTable->maskwords * sizeof(typename ELFT::Off) >=
+      Obj->getBufSize())
+    return createError("unable to dump the SHT_GNU_HASH "
+                       "section at 0x" +
+                       Twine::utohexstr(TableOffset) +
+                       ": it goes past the end of the file");
+  return Error::success();
+}
+
 template <typename ELFT> void ELFDumper<ELFT>::printHashTable() {
   DictScope D(W, "HashTable");
   if (!HashTable ||
       !checkHashTable(ObjF->getELFFile(), HashTable, ObjF->getFileName()))
     return;
   W.printNumber("Num Buckets", HashTable->nbucket);
   W.printNumber("Num Chains", HashTable->nchain);
   W.printList("Buckets", HashTable->buckets());
   W.printList("Chains", HashTable->chains());
 }
 
 template <typename ELFT>
 void ELFDumper<ELFT>::printGnuHashTable(const object::ObjectFile *Obj) {
   DictScope D(W, "GnuHashTable");
   if (!GnuHashTable)
     return;
+
+  bool IsHeaderValid;
+  Error Err =
+      checkGNUHashTable<ELFT>(ObjF->getELFFile(), GnuHashTable, &IsHeaderValid);
+  if (IsHeaderValid) {
     W.printNumber("Num Buckets", GnuHashTable->nbuckets);
     W.printNumber("First Hashed Symbol Index", GnuHashTable->symndx);
     W.printNumber("Num Mask Words", GnuHashTable->maskwords);
     W.printNumber("Shift Count", GnuHashTable->shift2);
+  }
 
-  MemoryBufferRef File = Obj->getMemoryBufferRef();
-  const char *TableData = reinterpret_cast<const char *>(GnuHashTable);
-  assert(TableData >= File.getBufferStart() &&
-         TableData < File.getBufferEnd() &&
-         "GnuHashTable must always point to a location inside the file");
-
-  uint64_t TableOffset = TableData - File.getBufferStart();
-  if (TableOffset +
-          /*Header size:*/ 16 + GnuHashTable->nbuckets * 4 +
-          GnuHashTable->maskwords * sizeof(typename ELFT::Off) >=
-      File.getBufferSize()) {
-    reportWarning(createError("unable to dump the SHT_GNU_HASH "
-                              "section at 0x" +
-                              Twine::utohexstr(TableOffset) +
-                              ": it goes past the end of the file"),
-                  ObjF->getFileName());
+  if (Err) {
+    reportUniqueWarning(std::move(Err));
     return;
   }
 
   ArrayRef<typename ELFT::Off> BloomFilter = GnuHashTable->filter();
   W.printHexList("Bloom Filter", BloomFilter);
 
   ArrayRef<Elf_Word> Buckets = GnuHashTable->buckets();
   W.printList("Buckets", Buckets);

 void GNUStyle<ELFT>::printHashHistograms(const ELFFile<ELFT> *Obj) {
   // Print histogram for the .hash section.
   if (const Elf_Hash *HashTable = this->dumper()->getHashTable())
     if (checkHashTable(Obj, HashTable, this->FileName))
       printHashHistogram(*HashTable);
 
   // Print histogram for the .gnu.hash section.
   if (const Elf_GnuHash *GnuHashTable = this->dumper()->getGnuHashTable())
+    if (Error E = checkGNUHashTable<ELFT>(Obj, GnuHashTable))
+      this->reportUniqueWarning(std::move(E));
+    else
       printGnuHashHistogram(*GnuHashTable);
 }
 
 template <class ELFT>
 void GNUStyle<ELFT>::printCGProfile(const ELFFile<ELFT> *Obj) {
   OS << "GNUStyle::printCGProfile not implemented\n";
 }
 
 template <class ELFT>