[PATCH] D88561: [llvm-readobj] - Fix possible crashes related to dumping gnu hash symbols.

[George Rimar via Phabricator via llvm-commits]

grimar marked an inline comment as done.
grimar added inline comments.


================
Comment at: llvm/test/tools/llvm-readobj/ELF/hash-symbols.test:679-680
+# BUCKET-READ-VALUE-A:      Num Buc:    Value          Size   Type   Bind Vis      Ndx Name
+## Note: we are trying to dump a dynamic symbol using a wrong index and so reading some arbitrary data as symbol data.
+##       That is why the following warning is reported.
+# BUCKET-READ-VALUE-A-NEXT: warning: '[[FILE]]': st_name (0x36) is past the end of the string table of size 0x5
----------------
jhenderson wrote:
> Surely we should be diagnosing the attempt to read using an invalid dynamic symbol index in the first place, so that we don't see semi-random warnings?
I am not sure honestly. Currently our logic is that we can't get here when there is no SHT_DYNSYM section,
because we take the size of the dynamic symbol table from there and there is no other way to get it.
I.e. there is no dynamic tag for that, so in case when we e.g. don't have a `.hash` table to infer the size,
we will never be able to dump symbols currently and we return earlier:

```
void GNUStyle<ELFT>::printGnuHashTableSymbols(const Elf_GnuHash &GnuHash) {
  StringRef StringTable = this->dumper().getDynamicStringTable();
  if (StringTable.empty())
    return;

  Elf_Sym_Range DynSyms = this->dumper().dynamic_symbols();
  const Elf_Sym *FirstSym = DynSyms.empty() ? nullptr : &DynSyms[0];
  if (!FirstSym) {
    Optional<DynRegionInfo> DynSymRegion = this->dumper().getDynSymRegion();
    this->reportUniqueWarning(createError(
        Twine("unable to print symbols for the .gnu.hash table: the "
              "dynamic symbol table ") +
        (DynSymRegion ? "is empty" : "was not found")));
    return;
  }
....
```


At the same time I think loaders have no any checks (i.e. they don't try to use the size from SHT_DYNSYM) and will try to use a dynamic symbol found via `DT_SYMTAB` only. In this case it might probably be useful to try to dump symbols here,
even if they are invalid to reveal what loaders see on their side? Perhaps worth showing an additional warning, saying that we think/found that an index is invalid though. And we can also probably change the code shown above to allow us to pass when `DynSymRegion.Addr != 0` (i.e. when we found a `DT_SYMTAB`, but no `SHT_DYNSYM` and the address is within the file).

If not, we can probably just skip symbols with invalid indices, just like you've suggested.

What do you think?






CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88561/new/

https://reviews.llvm.org/D88561