[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 5 inline comments as done.
jhenderson added a comment.

Given the proposed changes to the DWARFDataExtractor to limit it to a certain region, I think this and similar changes I have in the pipeline should probably be shelved, as the behaviour will change and the error messages will need updating. I'll focus on other areas instead in the meantime.



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


================
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:
----------------
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.


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


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


================
Comment at: llvm/unittests/DebugInfo/DWARF/DwarfGenerator.cpp:178
   }
+  P.TotalLength += Contents.size();
   P.FormParams.Version = Version;
----------------
ikudrin wrote:
> Do I understand it right that this fix along with the corresponding changes in other places may be extracted into a separate patch?
It probably can be. It's a requirement for this patch, but dosen't need to be a part of the same commit. I'lll look at splitting it out.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73618