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

[James Henderson via Phabricator via llvm-commits]

jhenderson accepted this revision.
jhenderson added a comment.

LGTM.



================
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:
----------------
grimar wrote:
> 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?
Okay, thanks for the explanation.


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

https://reviews.llvm.org/D82201