[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

277

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?

299

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

llvm/tools/llvm-readobj/ELFDumper.cpp

2677

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.

2703–2706

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

2703–2706

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

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

277

Fixed.

299

Moved right parts.

llvm/tools/llvm-readobj/ELFDumper.cpp

2677

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

275

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

[grimar]

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

275

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/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.
+# RUN: yaml2obj --docnum=6 -D MASKWORDS=4294967295 -D NBUCKETS=3 %s -o %t7
+# RUN: llvm-readelf --elf-hash-histogram %t7 2>&1 | \
+# RUN:   FileCheck %s -DFILE=%t7 -DMASKWORDS=4294967295 -DNBUCKETS=3 --check-prefix=ERR5
+
+# 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.
+# RUN: yaml2obj --docnum=6 -D MASKWORDS=2 -D NBUCKETS=4294967295 %s -o %t8
+# RUN: llvm-readelf --elf-hash-histogram %t8 2>&1 | \
+# RUN:   FileCheck %s -DFILE=%t8 -DMASKWORDS=2 -DNBUCKETS=4294967295 --check-prefix=ERR5
+
+--- !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. The value of 2 is no-op.
+      MaskWords: [[MASKWORDS]]
+## The number of hash buckets. The value of 3 is no-op.
+      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 bool checkGNUHashTable(const ELFFile<ELFT> *Obj,
+                              const typename ELFT::GnuHash *GnuHashTable,
+                              StringRef FileName) {
+  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 (TableOffset +
+          /*Header size:*/ 16 + GnuHashTable->nbuckets * 4 +
+          GnuHashTable->maskwords * sizeof(typename ELFT::Off) >=
+      Obj->getBufSize()) {
+    reportWarning(createError("unable to dump the SHT_GNU_HASH "
+                              "section at 0x" +
+                              Twine::utohexstr(TableOffset) +
+                              ": it goes past the end of the file"),
+                  FileName);
+    return false;
+  }
+  return true;
+}
+
 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;
   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 (!checkGNUHashTable<ELFT>(ObjF->getELFFile(), GnuHashTable,
+                               ObjF->getFileName()))
     return;
-  }
 
   ArrayRef<typename ELFT::Off> BloomFilter = GnuHashTable->filter();
   W.printHexList("Bloom Filter", BloomFilter);
 
   ArrayRef<Elf_Word> Buckets = GnuHashTable->buckets();
   W.printList("Buckets", Buckets);
 
   if (!DynSymRegion) {
void GNUStyle<ELFT>::printHashHistogram(const ELFFile<ELFT> *Obj) {
 
   // Print histogram for the .hash section.
   if (const Elf_Hash *HashTable = this->dumper()->getHashTable())
     if (checkHashTable(Obj, HashTable, this->FileName))
       PrintHashHist(*HashTable);
 
   // Print histogram for the .gnu.hash section.
   if (const Elf_GnuHash *GnuHashTable = this->dumper()->getGnuHashTable())
+    if (checkGNUHashTable<ELFT>(Obj, GnuHashTable, this->FileName))
       PrintGnuHashHist(*GnuHashTable);
 }
 
 template <class ELFT>
 void GNUStyle<ELFT>::printCGProfile(const ELFFile<ELFT> *Obj) {
   OS << "GNUStyle::printCGProfile not implemented\n";
 }
 
 template <class ELFT>