[PATCH] D82017: [DebugInfo/DWARF] - Do not hang when CFI are truncated.

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugFrame.cpp:138-139
+        // prohibited in call frame instructions, see sec. 6.4.2 in DWARFv5.
+        Instructions.back().Expression =
+            DWARFExpression(Extractor, Data.getAddressSize());
+        break;
----------------
jhenderson wrote:
> What happens if the expression itself is truncated? I'd expect this to somehow report an error that can be detected here. Same goes below.
A simple truncation is catched by `StringRef Expression = Data.getBytes(C, ExprLength);`.

So you are talking about the case when both expression length and expression data are truncated and
not reported here. It is probably a subject for another review. Perhaps it should be validated
at the place where it is actually dumped, i.e. not here:

I see we have `DWARFExpression::verify(DWARFUnit *U)` method, which seems already used to validate expressions.
I do not know how much complete it is (I guess it should be), but it might cause ` DWARFExpression::Operation::print` to
report "<decoding error>" already. It is possible that everything is already done there.


================
Comment at: llvm/unittests/DebugInfo/DWARF/DWARFDebugFrameTest.cpp:178-182
+  // Unable to read a truncated DW_CFA_advance_loc4 instruction.
+  EXPECT_THAT_ERROR(
+      ParseCFI(TestCIE, {dwarf::DW_CFA_advance_loc4}),
+      FailedWithMessage(
+          "unexpected end of data at offset 0x1 while reading [0x1, 0x5)"));
----------------
jhenderson wrote:
> Accidental duplicate test case?
Yes, thanks!


================
Comment at: llvm/unittests/DebugInfo/DWARF/DWARFDebugFrameTest.cpp:185-188
+  EXPECT_THAT_ERROR(
+      ParseCFI(TestCIE, {dwarf::DW_CFA_restore_extended}),
+      FailedWithMessage("unable to decode LEB128 at offset 0x00000001: "
+                        "malformed uleb128, extends past end"));
----------------
jhenderson wrote:
> Given that this and the next several cases are identical, barring the input value, (as indeed are many) have you considered changing to using a set of parameterised test cases? No idea how much extra complexity (if any) it would add, without actually trying it, so it might not be worth it, but in principle you could have a test body for each of the different possible error messages, and a list of parameters for each test body listing the opcodes that trigger that case.
Having  a test body for each of the different possible error messages is perhaps too much, because there are many unique messages. I've grouped the identical messages together. Does it look OK for you?


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

https://reviews.llvm.org/D82017