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

[Igor Kudrin via Phabricator via llvm-commits]

ikudrin 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
----------------
jhenderson wrote:
> 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?
Ah, yes, I forgot about the special code which fixes `OffsetPtr` at the end of the branch for `Opcode == 0`. Well, I cannot imagine another malicious sequence. It is sad that the whole code is not straightforward and requires a long explanatory comment.

I don't insist on any specific wording, but maybe it is worth to add a note about the code at the end of the `Opcode == 0` branch which I missed.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp:927
+  // correspond to an end sequence instruction).
+  if (*OffsetPtr > EndOffset) {
+    RecoverableErrorCallback(createStringError(
----------------
jhenderson wrote:
> 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.
Right, I believe that we do not need to report about the unterminated sequence if we have reported the incorrect offset. The latter is more fundamental, to my taste.


================
Comment at: llvm/unittests/DebugInfo/DWARF/DwarfGenerator.cpp:178
   }
+  P.TotalLength += Contents.size();
   P.FormParams.Version = Version;
----------------
Do I understand it right that this fix along with the corresponding changes in other places may be extracted into a separate patch?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73618