[PATCH] D73618: [DebugInfo] Check that we do not run past a line table end when parsing

[James Henderson via Phabricator via llvm-commits]

jhenderson marked 2 inline comments as done.
jhenderson added inline comments.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp:924
+  // where the overflowing opcode appears at the end of the section. However, in
+  // such cases, an unterminated sequence error will be raised instead, as the
+  // data extractor will return zeroes for the trailing bytes (which do not
----------------
ikudrin wrote:
> I guess that the comment is not 100% accurate. Let's imagine the following purely illustrative and meaningless sequence at the end of the section: `0`, `5`, `DW_LNE_set_address`, `0`, `1`, `DW_LNE_end_sequence`. The extractor will not read an argument of `DW_LNE_set_address` and will not increment the offset, but after that, something which looks like a correct termination will be read.
In your example, the set address opcode would consume the three bytes comprising the end sequence opcode, followed by reading a single zero value for the final byte. The offset would be adjusted to the end of the three readable bytes (thus going past the end of the DW_LNE_end_sequence opcode). The parser would then stop as `*OffsetPtr == EndOffset`. This warning wouldn't then be triggered but a no end sequence one would be.

Would changing this statement "As the offset isn't incremented by the data extractor when reading past the end of the available data" to "As the offset isn't incremented by the data extractor past the end of the available data" be clearer?


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp:927
+  // correspond to an end sequence instruction).
+  if (*OffsetPtr > EndOffset) {
+    RecoverableErrorCallback(createStringError(
----------------
ikudrin wrote:
> Maybe it is more consistent to put this code before the previous check. I mean, this check is comparatively low level, and if it triggers, the input is probably corrupted so deeply that it makes no sense to interpret it, no?
> 
> 
> 
> 
Just to be clear, are you saying that we shouldn't report the no end sequence error if we hit this case? That's fine with me. If I did that, I'd probably pull the sort below to before this check, as that would allow me to simply return after the error has been reported, whilst still allowing clients to use the data, if they want to.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73618