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

[George Rimar via Phabricator via llvm-commits]

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
----------------
uweigand wrote:
> jhenderson wrote:
> > 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.
> As I said in my comment yesterday, in this particular executable we do have the correct size of dynsym from the section table -- it's just that this (correct) size is later replaced by (incorrect) information derived from the .hash section.
> I'm a little struggling to follow the full details of your comment though. so I may be misunderstanding your points/questions.

Nope. You got me right. I'll update the patch to report a warning and will stop printing a bogus symbol.


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

https://reviews.llvm.org/D88561