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

[Ulrich Weigand via Phabricator via llvm-commits]

uweigand 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:
> 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.


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

https://reviews.llvm.org/D88561