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

[James Henderson via Phabricator via llvm-commits]

jhenderson 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
----------------
grimar wrote:
> 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?
> 
> 
> 
> 
Ah, I forgot that the dynsym has no size in the dynamic table (that's still dumb, but oh well). I think if we have information implying the index looks broken, due to other information (e.g. the section header), we should at least warn that this is probably not correct. I think the warning would be more than enough to say something is a bit bogus. I also think that means we don't need to try to dump invalid index symbols if we know they are bogus (why would a user care what the loader sees if they know that whatever it is is bad - in such situations they'd need to fix the source of the bad index).

I'm a little struggling to follow the full details of your comment though. so I may be misunderstanding your points/questions.


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

https://reviews.llvm.org/D88561