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

[grimar]

Summary

Currently when the .eh_frame section is truncated so that
CFI instructions can't be read, it is possible to enter
an infinite loop.

It happens because CFIProgram::parse does not handle errors properly.
This patch fixes the issue.

[probinson]

Convert to using cursor instead?

[MaskRay]

llvm/lib/DebugInfo/DWARF/DWARFDebugFrame.cpp

79

clang-format may prefer /*SectionIndex=*/nullptr

llvm/test/DebugInfo/X86/eh-frame-truncated-cfi.s

2 ↗

(On Diff #271374)

s/are //

4 ↗

(On Diff #271374)

Drop -unknown-linux. This is a generic ELF behavior not specific to Linux.

[jhenderson]

llvm/test/DebugInfo/X86/eh-frame-truncated-cfi.s

4 ↗

(On Diff #271374)

You've added several new error cases, but only one new test case. You need one for each individual new error check, I think. It may be better to write the testing as unit tests. See similar work I've been doing recently for .debug_line.

[grimar]

• Use DataExtractor::Cursor.
• Use unit tests for testing.

[jhenderson]

llvm/lib/DebugInfo/DWARF/DWARFDebugFrame.cpp

138–139

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.

llvm/unittests/DebugInfo/DWARF/DWARFDebugFrameTest.cpp

178–182

Accidental duplicate test case?

185–188

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.

[grimar]

• Addressed review comments.

llvm/lib/DebugInfo/DWARF/DWARFDebugFrame.cpp

138–139

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.

llvm/unittests/DebugInfo/DWARF/DWARFDebugFrameTest.cpp

178–182

Yes, thanks!

185–188

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?

[jhenderson]

LGTM, thanks.

llvm/unittests/DebugInfo/DWARF/DWARFDebugFrameTest.cpp

255

Here and below, should Op1 be RegNum (that's what it was before)?

[grimar]

llvm/unittests/DebugInfo/DWARF/DWARFDebugFrameTest.cpp

255

I think no, because now CheckOp_ULEB128_Expr is a generic lamda, that knows that
an instruction checked has 3 operands of known types, but probably it shouldn't do any assumptions
about their meanings.

llvm/lib/DebugInfo/DWARF/DWARFDebugFrame.cpp

 
 
 // See DWARF standard v3, section 7.23
 const uint8_t DWARF_CFI_PRIMARY_OPCODE_MASK = 0xc0;
 const uint8_t DWARF_CFI_PRIMARY_OPERAND_MASK = 0x3f;
 
 Error CFIProgram::parse(DWARFDataExtractor Data, uint64_t *Offset,
                         uint64_t EndOffset) {
-  while (*Offset < EndOffset) {
-    uint8_t Opcode = Data.getRelocatedValue(1, Offset);
+  DataExtractor::Cursor C(*Offset);
+  while (C && C.tell() < EndOffset) {
+    uint8_t Opcode = Data.getRelocatedValue(C, 1);
     // Some instructions have a primary opcode encoded in the top bits.
     uint8_t Primary = Opcode & DWARF_CFI_PRIMARY_OPCODE_MASK;
 
     if (Primary) {
       // If it's a primary opcode, the first operand is encoded in the bottom
       // bits of the opcode itself.
       uint64_t Op1 = Opcode & DWARF_CFI_PRIMARY_OPERAND_MASK;
       switch (Primary) {
       default:
         return createStringError(errc::illegal_byte_sequence,
                                  "Invalid primary CFI opcode 0x%" PRIx8,
                                  Primary);
       case DW_CFA_advance_loc:
       case DW_CFA_restore:
         addInstruction(Primary, Op1);
         break;
       case DW_CFA_offset:
-        addInstruction(Primary, Op1, Data.getULEB128(Offset));
+        addInstruction(Primary, Op1, Data.getULEB128(C));
         break;
       }
     } else {
       // Extended opcode - its value is Opcode itself.
       switch (Opcode) {
       default:
         return createStringError(errc::illegal_byte_sequence,
                                  "Invalid extended CFI opcode 0x%" PRIx8,
                                  Opcode);
       case DW_CFA_nop:
       case DW_CFA_remember_state:
       case DW_CFA_restore_state:
       case DW_CFA_GNU_window_save:
         // No operands
         addInstruction(Opcode);
         break;
       case DW_CFA_set_loc:
         // Operands: Address
-        addInstruction(Opcode, Data.getRelocatedAddress(Offset));
+        addInstruction(Opcode, Data.getRelocatedAddress(C));
         break;
       case DW_CFA_advance_loc1:
         // Operands: 1-byte delta
-        addInstruction(Opcode, Data.getRelocatedValue(1, Offset));
+        addInstruction(Opcode, Data.getRelocatedValue(C, 1));
         break;
       case DW_CFA_advance_loc2:
         // Operands: 2-byte delta
-        addInstruction(Opcode, Data.getRelocatedValue(2, Offset));
+        addInstruction(Opcode, Data.getRelocatedValue(C, 2));
         break;
       case DW_CFA_advance_loc4:
         // Operands: 4-byte delta
-        addInstruction(Opcode, Data.getRelocatedValue(4, Offset));
+        addInstruction(Opcode, Data.getRelocatedValue(C, 4));
         break;
       case DW_CFA_restore_extended:
       case DW_CFA_undefined:
       case DW_CFA_same_value:
       case DW_CFA_def_cfa_register:
       case DW_CFA_def_cfa_offset:
       case DW_CFA_GNU_args_size:
         // Operands: ULEB128
-        addInstruction(Opcode, Data.getULEB128(Offset));
+        addInstruction(Opcode, Data.getULEB128(C));
         break;
       case DW_CFA_def_cfa_offset_sf:
         // Operands: SLEB128
-        addInstruction(Opcode, Data.getSLEB128(Offset));
+        addInstruction(Opcode, Data.getSLEB128(C));
         break;
       case DW_CFA_offset_extended:
       case DW_CFA_register:
       case DW_CFA_def_cfa:
       case DW_CFA_val_offset: {
         // Operands: ULEB128, ULEB128
         // Note: We can not embed getULEB128 directly into function
         // argument list. getULEB128 changes Offset and order of evaluation
         // for arguments is unspecified.
-        auto op1 = Data.getULEB128(Offset);
-        auto op2 = Data.getULEB128(Offset);
+        uint64_t op1 = Data.getULEB128(C);
+        uint64_t op2 = Data.getULEB128(C);
         addInstruction(Opcode, op1, op2);
         break;
-        }
-        case DW_CFA_offset_extended_sf:
-        case DW_CFA_def_cfa_sf:
-        case DW_CFA_val_offset_sf: {
-          // Operands: ULEB128, SLEB128
-          // Note: see comment for the previous case
-          auto op1 = Data.getULEB128(Offset);
-          auto op2 = (uint64_t)Data.getSLEB128(Offset);
-          addInstruction(Opcode, op1, op2);
-          break;
-        }
-        case DW_CFA_def_cfa_expression: {
-          uint32_t ExprLength = Data.getULEB128(Offset);
-          addInstruction(Opcode, 0);
-          DataExtractor Extractor(
-              Data.getData().slice(*Offset, *Offset + ExprLength),
-              Data.isLittleEndian(), Data.getAddressSize());
-          // Note. We do not pass the DWARF format to DWARFExpression, because
-          // DW_OP_call_ref, the only operation which depends on the format, is
-          // prohibited in call frame instructions, see sec. 6.4.2 in DWARFv5.
-          Instructions.back().Expression =
-              DWARFExpression(Extractor, Data.getAddressSize());
-          *Offset += ExprLength;
-          break;
-        }
-        case DW_CFA_expression:
-        case DW_CFA_val_expression: {
-          auto RegNum = Data.getULEB128(Offset);
-          auto BlockLength = Data.getULEB128(Offset);
-          addInstruction(Opcode, RegNum, 0);
-          DataExtractor Extractor(
-              Data.getData().slice(*Offset, *Offset + BlockLength),
-              Data.isLittleEndian(), Data.getAddressSize());
-          // Note. We do not pass the DWARF format to DWARFExpression, because
-          // DW_OP_call_ref, the only operation which depends on the format, is
-          // prohibited in call frame instructions, see sec. 6.4.2 in DWARFv5.
-          Instructions.back().Expression =
-              DWARFExpression(Extractor, Data.getAddressSize());
-          *Offset += BlockLength;
-          break;
-        }
+        uint64_t op1 = Data.getULEB128(C);
+        uint64_t op2 = (uint64_t)Data.getSLEB128(C);
+        uint64_t ExprLength = Data.getULEB128(C);
+        StringRef Expression = Data.getBytes(C, ExprLength);
+
+        DataExtractor Extractor(Expression, Data.isLittleEndian(),
+                                Data.getAddressSize());
+        uint64_t RegNum = Data.getULEB128(C);
+
+        uint64_t BlockLength = Data.getULEB128(C);
+        StringRef Expression = Data.getBytes(C, BlockLength);
+        DataExtractor Extractor(Expression, Data.isLittleEndian(),
+                                Data.getAddressSize());
       }
     }
   }
 
-  return Error::success();
+  *Offset = C.tell();
+  return C.takeError();
 }
 
 namespace {
 
 
 } // end anonymous namespace
 
 ArrayRef<CFIProgram::OperandType[2]> CFIProgram::getOperandTypes() {

llvm/unittests/DebugInfo/DWARF/DWARFDebugFrameTest.cpp

 //===- llvm/unittest/DebugInfo/DWARFDebugFrameTest.cpp --------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/BinaryFormat/Dwarf.h"
 #include "llvm/DebugInfo/DWARF/DWARFDebugFrame.h"
+#include "llvm/Testing/Support/Error.h"
 #include "gtest/gtest.h"
 
 using namespace llvm;
 
 namespace {
 
 dwarf::CIE createCIE(bool IsDWARF64, uint64_t Offset, uint64_t Length) {
   return dwarf::CIE(IsDWARF64, Offset, Length,
dwarf::FDE TestFDE(/*IsDWARF64=*/true,
                      /*Cie=*/&TestCIE,
                      /*LSDAAddress=*/None,
                      /*Arch=*/Triple::x86_64);
   expectDumpResult(TestFDE, /*IsEH=*/true,
                    "1111abcdabcd 00002222abcdabcd 0033abcd FDE "
                    "cie=1111ab9a000c pc=4444abcdabcd...5555bcdebcde");
 }
 
+// Here we test how truncated Call Frame Instructions are parsed.
+TEST(DWARFDebugFrame, ParseTruncatedCFITest) {
+  auto ParseCFI = [](dwarf::CIE &C, ArrayRef<uint8_t> Instructions,
+                     Optional<uint64_t> Size = None) {
+    DWARFDataExtractor Data(Instructions, /*IsLittleEndian=*/true,
+                            /*AddressSize=*/8);
+    uint64_t Offset = 0;
+    const uint64_t EndOffset = Size ? *Size : (uint64_t)Instructions.size();
+    return C.cfis().parse(Data, &Offset, EndOffset);
+  };
+
+  dwarf::CIE TestCIE = createCIE(/*IsDWARF64=*/false,
+                                 /*Offset=*/0x0,
+                                 /*Length=*/0xff);
+
+  // Having an empty instructions list is fine.
+  EXPECT_THAT_ERROR(ParseCFI(TestCIE, {}), Succeeded());
+
+  // Unable to read an opcode, because the instructions list is empty, but we
+  // say to the parser that it is not.
+  EXPECT_THAT_ERROR(
+      ParseCFI(TestCIE, {}, /*Size=*/1),
+      FailedWithMessage(
+          "unexpected end of data at offset 0x0 while reading [0x0, 0x1)"));
+
+  // Unable to read a truncated DW_CFA_offset instruction.
+  EXPECT_THAT_ERROR(
+      ParseCFI(TestCIE, {dwarf::DW_CFA_offset}),
+      FailedWithMessage("unable to decode LEB128 at offset 0x00000001: "
+                        "malformed uleb128, extends past end"));
+
+  // Unable to read a truncated DW_CFA_set_loc instruction.
+  EXPECT_THAT_ERROR(
+      ParseCFI(TestCIE, {dwarf::DW_CFA_set_loc}),
+      FailedWithMessage(
+          "unexpected end of data at offset 0x1 while reading [0x1, 0x9)"));
+
+  // Unable to read a truncated DW_CFA_advance_loc1 instruction.
+  EXPECT_THAT_ERROR(
+      ParseCFI(TestCIE, {dwarf::DW_CFA_advance_loc1}),
+      FailedWithMessage(
+          "unexpected end of data at offset 0x1 while reading [0x1, 0x2)"));
+
+  // Unable to read a truncated DW_CFA_advance_loc2 instruction.
+  EXPECT_THAT_ERROR(
+      ParseCFI(TestCIE, {dwarf::DW_CFA_advance_loc2}),
+      FailedWithMessage(
+          "unexpected end of data at offset 0x1 while reading [0x1, 0x3)"));
+
+  // 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)"));
+
+  // A test for an instruction with a single ULEB128 operand.
+  auto CheckOp_ULEB128 = [&](uint8_t Inst) {
+    EXPECT_THAT_ERROR(
+        ParseCFI(TestCIE, Inst),
+        FailedWithMessage("unable to decode LEB128 at offset 0x00000001: "
+                          "malformed uleb128, extends past end"));
+  };
+
+  for (uint8_t Inst :
+       {dwarf::DW_CFA_restore_extended, dwarf::DW_CFA_undefined,
+        dwarf::DW_CFA_same_value, dwarf::DW_CFA_def_cfa_register,
+        dwarf::DW_CFA_def_cfa_offset, dwarf::DW_CFA_GNU_args_size})
+    CheckOp_ULEB128(Inst);
+
+  // Unable to read a truncated DW_CFA_def_cfa_offset_sf instruction.
+  EXPECT_THAT_ERROR(
+      ParseCFI(TestCIE, {dwarf::DW_CFA_def_cfa_offset_sf}),
+      FailedWithMessage("unable to decode LEB128 at offset 0x00000001: "
+                        "malformed sleb128, extends past end"));
+
+  // A test for an instruction with two ULEB128 operands.
+  auto CheckOp_ULEB128_ULEB128 = [&](uint8_t Inst) {
+    EXPECT_THAT_ERROR(
+        ParseCFI(TestCIE, Inst),
+        FailedWithMessage("unable to decode LEB128 at offset 0x00000001: "
+                          "malformed uleb128, extends past end"));
+
+    EXPECT_THAT_ERROR(
+        ParseCFI(TestCIE, {Inst, /*Op1=*/0}),
+        FailedWithMessage("unable to decode LEB128 at offset 0x00000002: "
+                          "malformed uleb128, extends past end"));
+  };
+
+  for (uint8_t Inst : {dwarf::DW_CFA_offset_extended, dwarf::DW_CFA_register,
+                       dwarf::DW_CFA_def_cfa, dwarf::DW_CFA_val_offset})
+    CheckOp_ULEB128_ULEB128(Inst);
+
+  // A test for an instruction with two operands: ULEB128, SLEB128.
+  auto CheckOp_ULEB128_SLEB128 = [&](uint8_t Inst) {
+    EXPECT_THAT_ERROR(
+        ParseCFI(TestCIE, Inst),
+        FailedWithMessage("unable to decode LEB128 at offset 0x00000001: "
+                          "malformed uleb128, extends past end"));
+
+    EXPECT_THAT_ERROR(
+        ParseCFI(TestCIE, {Inst, /*Op1=*/0}),
+        FailedWithMessage("unable to decode LEB128 at offset 0x00000002: "
+                          "malformed sleb128, extends past end"));
+  };
+
+  for (uint8_t Inst : {dwarf::DW_CFA_offset_extended_sf,
+                       dwarf::DW_CFA_def_cfa_sf, dwarf::DW_CFA_val_offset_sf})
+    CheckOp_ULEB128_SLEB128(Inst);
+
+  // Unable to read a truncated DW_CFA_def_cfa_expression instruction.
+  EXPECT_THAT_ERROR(
+      ParseCFI(TestCIE, {dwarf::DW_CFA_def_cfa_expression}),
+      FailedWithMessage("unable to decode LEB128 at offset 0x00000001: "
+                        "malformed uleb128, extends past end"));
+  EXPECT_THAT_ERROR(
+      ParseCFI(TestCIE, {dwarf::DW_CFA_def_cfa_expression,
+                         /*expression length=*/0x1}),
+      FailedWithMessage(
+          "unexpected end of data at offset 0x2 while reading [0x2, 0x3)"));
+  // The DW_CFA_def_cfa_expression can contain a zero length expression.
+  EXPECT_THAT_ERROR(ParseCFI(TestCIE, {dwarf::DW_CFA_def_cfa_expression,
+                                       /*ExprLen=*/0}),
+                    Succeeded());
+
+  // A test for an instruction with three operands: ULEB128, expression length
+  // (ULEB128) and expression bytes.
+  auto CheckOp_ULEB128_Expr = [&](uint8_t Inst) {
+    EXPECT_THAT_ERROR(
+        ParseCFI(TestCIE, {Inst}),
+        FailedWithMessage("unable to decode LEB128 at offset 0x00000001: "
+                          "malformed uleb128, extends past end"));
+    EXPECT_THAT_ERROR(
+        ParseCFI(TestCIE, {Inst, /*Op1=*/0}),
+        FailedWithMessage("unable to decode LEB128 at offset 0x00000002: "
+                          "malformed uleb128, extends past end"));
+    // A zero length expression is fine
+    EXPECT_THAT_ERROR(ParseCFI(TestCIE, {Inst,
+                                         /*Op1=*/0, /*ExprLen=*/0}),
+                      Succeeded());
+    EXPECT_THAT_ERROR(
+        ParseCFI(TestCIE, {Inst,
+                           /*Op1=*/0, /*ExprLen=*/1}),
+        FailedWithMessage(
+            "unexpected end of data at offset 0x3 while reading [0x3, 0x4)"));
+  };
+
+  for (uint8_t Inst : {dwarf::DW_CFA_expression, dwarf::DW_CFA_val_expression})
+    CheckOp_ULEB128_Expr(Inst);
+}
+
 } // end anonymous namespace