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

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a subscriber: labath.
dblaikie added a comment.

@labath - maybe some other parts of the DWARF parsing that could benefit from a constrained DWARFDataExtractor



================
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[{{.*}}]'
----------------
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?




================
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:
----------------
(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)


================
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));
----------------
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.


================
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);
----------------
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?))


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73618