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

Mon Feb 3 17:17:58 PST 2020

dblaikie added inline comments.

================
Comment at: llvm/test/tools/llvm-dwarfdump/X86/debug_line_invalid.test:39
 ## Don't stop looking for the later unit if non-fatal issues are found.
-# RUN: llvm-dwarfdump -debug-line=0x2ec %t-malformed.o 2> %t-malformed-off-last.err \
+# RUN: llvm-dwarfdump -debug-line=0x339 %t-malformed.o 2> %t-malformed-off-last.err \
 # RUN:   | FileCheck %s --check-prefix=LAST --implicit-check-not='debug_line[{{.*}}]'
----------------
jhenderson wrote:
> dblaikie wrote:
> > Should this dump in verbose mode to show more clearly which operations were parsed and which ones were not? So they match up with the LNE_* descriptions in the comments in the test?
> > 
> > 
> This last check is just for the last (good) section, so doesn't really need to show things in that much detail. However, I could certainly see a benefit for checking things more verbosely. We already do verbose dumps on line 29 of the test, but the checks don't check the operands. I'll look at updating that, but it should be a separate change.
Oh, I figured this change related to the other change in this review (Case 13) which wasn't super clear to me & figured verbosity might help.

================
Comment at: llvm/test/tools/llvm-dwarfdump/X86/debug_line_invalid.test:185
+# OTHER-NEXT: warning: last sequence in debug line table at offset 0x000002ec is not terminated
+# OTHER-NEXT: warning: last opcode in line table at offset 0x000002ec ended at offset 0x0000033d which is past the expected table end at offset 0x00000339
 # ALL-NOT:  warning:
----------------
jhenderson wrote:
> dblaikie wrote:
> > (similar to other comment) - this warning sounds problematic to me & any chunk of debug info that has a specified length shouldn't be read beyond that length (as if the section itself ended at the end of the length - we should get the same error messages in both those cases)
> That's fair. Saying it's been truncated at this point is somewhat lying, but would be correct once we switch to the limited data extractor.
Right - I meant that the warning implies the implementation isn't what I'd hope/expect (& that both message and implementation should be fixed in the direction of not reading more than the prefixed length indicates for any given entity).

================
Comment at: llvm/unittests/DebugInfo/DWARF/DWARFDebugLineTest.cpp:568-569
+  checkError(
+      "last opcode in line table at offset 0x00000001 ended at offset "
+      "0x00000034 which is past the expected table end at offset 0x00000033",
+      std::move(Recoverable));
----------------
jhenderson wrote:
> dblaikie wrote:
> > This phrasing doesn't match up with not reading past the end of a specified length - I'd expect something more like "last opcode in line table at offset 0x..1 is incomplete/truncated at offset 0x...34" or the like. Maybe with "expected to extend to 0x35" if we know that some localized implied/expressed length extends beyond some broader length that was specified.
> I think your proposal makes sense with the proposed data extractor changes, but possibly not before. See my out-of-line comment.
Right right - implementation and messaging should be fixed in that direction.

================
Comment at: llvm/unittests/DebugInfo/DWARF/DWARFDebugLineTest.cpp:794-796
+  // Arbitrary padding byte to ensure the end of the opcode as claimed by the
+  // opcode length field does not go past the table end.
+  LT.addByte(0xaa);
----------------
jhenderson wrote:
> dblaikie wrote:
> > Could you explain this further? What's incorrect about the existing usage (where is the opcode length field? Is that the 0x2 in the line above? Why would it be too short? (should the DWARFGenerator API be changed? is it computing a length that's too short for the table?))
> `addExtendedOpcode` takes a length for an extended opcode, followed by the opcode itself, and then data for the operands. I realise this byte should be moved into the operands argument, so I'll fix that. I don't think the API here needs changing, as it would prevent us creating broken situations like this (i.e. an extended opcode without sufficient data for its claimed length).
What's the mention of the "table end" here, then? I guess "table end" is indicated by the table length field elsewhere? Is it autogenerated (with a possible override for invalid situations)? Should it be autogenerated differently? 

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73618