[PATCH] D82201: [llvm-readobj] - Validate the DT_STRSZ value to avoid crash.

Mon Jun 22 04:16:12 PDT 2020

grimar marked 3 inline comments as done.
grimar added inline comments.

================
Comment at: llvm/test/tools/llvm-readobj/ELF/dyn-symbols.test:485-486
+  - StName:  0xffffff00
+## An arbitrary valid symbol to document we report an error before dumping it.
+  - StName:  0x1
+ProgramHeaders:
----------------
jhenderson wrote:
> grimar wrote:
> > jhenderson wrote:
> > > This comment implies you should have a check showing we don't dump the next symbol, but I don't see such a check.
> > > 
> > > I'd expect to see some sort of `-NOT` line after the error to show that we don't dump the next symbol.
> > I check the "error:..." line. Which currently means the application terminates.
> > If we had a warning, I'd use something like `-NOT`.
> > 
> > Does it makes sense?
> It makes sense, but I therefore don't think we need this symbol currently.
We could have a logic that, for example, collects a vector with an information about symbols and then prints this information.
(We have something like this in a few places already. For example, in `getVersionDependencies`: https://github.com/llvm/llvm-project/blob/master/llvm/tools/llvm-readobj/ELFDumper.cpp#L634). 

So technically, a error can be reported either before the symbols/after them (if we had such vector) or in the middle (with the current logic).
Here I document we report it "in the middle". Isn't it useful?

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

https://reviews.llvm.org/D82201