[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

80

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

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

2

s/are //

4

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

[jhenderson]

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

4

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

173–174

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 ↗

(On Diff #271989)

Accidental duplicate test case?

185–188 ↗

(On Diff #271989)

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

173–174

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 ↗

(On Diff #271989)

Yes, thanks!

185–188 ↗

(On Diff #271989)

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 ↗

(On Diff #272421)

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

[grimar]

llvm/unittests/DebugInfo/DWARF/DWARFDebugFrameTest.cpp

255 ↗

(On Diff #272421)

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/include/llvm/DebugInfo/DWARF/DWARFDataExtractor.h

uint64_t getRelocatedValue(uint32_t Size, uint64_t *Off,
                              Error *Err = nullptr) const;
   uint64_t getRelocatedValue(Cursor &C, uint32_t Size,
                              uint64_t *SectionIndex = nullptr) const {
     return getRelocatedValue(Size, &getOffset(C), SectionIndex, &getError(C));
   }
 
   /// Extracts an address-sized value and applies a relocation to the result if
   /// one exists for the given offset.
-  uint64_t getRelocatedAddress(uint64_t *Off, uint64_t *SecIx = nullptr) const {
-    return getRelocatedValue(getAddressSize(), Off, SecIx);
+  uint64_t getRelocatedAddress(uint64_t *Off, uint64_t *SecIx = nullptr,
+                               Error *Err = nullptr) const {
+    return getRelocatedValue(getAddressSize(), Off, SecIx, Err);
   }
   uint64_t getRelocatedAddress(Cursor &C, uint64_t *SecIx = nullptr) const {
     return getRelocatedValue(getAddressSize(), &getOffset(C), SecIx,
                              &getError(C));
   }
 
   /// Extracts a DWARF-encoded pointer in \p Offset using \p Encoding.
   /// There is a DWARF encoding that uses a PC-relative adjustment.

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);
+  Error Err = Error::success();
+  while (!Err && *Offset < EndOffset) {
+    uint8_t Opcode =
+        Data.getRelocatedValue(1, Offset, /*SectionIndex =*/nullptr, &Err);
     // 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(Offset, &Err));
         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(
+                                   Offset, /*SectionIndex =*/nullptr, &Err));
         break;
       case DW_CFA_advance_loc1:
         // Operands: 1-byte delta
-        addInstruction(Opcode, Data.getRelocatedValue(1, Offset));
+        addInstruction(Opcode, Data.getRelocatedValue(
+                                   1, Offset, /*SectionIndex =*/nullptr, &Err));
         break;
       case DW_CFA_advance_loc2:
         // Operands: 2-byte delta
-        addInstruction(Opcode, Data.getRelocatedValue(2, Offset));
+        addInstruction(Opcode, Data.getRelocatedValue(
+                                   2, Offset, /*SectionIndex =*/nullptr, &Err));
         break;
       case DW_CFA_advance_loc4:
         // Operands: 4-byte delta
-        addInstruction(Opcode, Data.getRelocatedValue(4, Offset));
+        addInstruction(Opcode, Data.getRelocatedValue(
+                                   4, Offset, /*SectionIndex =*/nullptr, &Err));
         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(Offset, &Err));
         break;
       case DW_CFA_def_cfa_offset_sf:
         // Operands: SLEB128
-        addInstruction(Opcode, Data.getSLEB128(Offset));
+        addInstruction(Opcode, Data.getSLEB128(Offset, &Err));
         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);
+        auto op1 = Data.getULEB128(Offset, &Err);
+        auto op2 = Data.getULEB128(Offset, &Err);
         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);
+          auto op1 = Data.getULEB128(Offset, &Err);
+          auto op2 = (uint64_t)Data.getSLEB128(Offset, &Err);
           addInstruction(Opcode, op1, op2);
           break;
         }
         case DW_CFA_def_cfa_expression: {
-          uint32_t ExprLength = Data.getULEB128(Offset);
+          uint32_t ExprLength = Data.getULEB128(Offset, &Err);
           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);
+          auto RegNum = Data.getULEB128(Offset, &Err);
+          auto BlockLength = Data.getULEB128(Offset, &Err);
           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;
         }
       }
     }
   }
 
-  return Error::success();
+  return Err;
 }
 
 namespace {
 
 
 } // end anonymous namespace
 
 ArrayRef<CFIProgram::OperandType[2]> CFIProgram::getOperandTypes() {
   static OperandType OpTypes[DW_CFA_restore+1][2];
   static bool Initialized = false;
   if (Initialized) {
     return ArrayRef<OperandType[2]>(&OpTypes[0], DW_CFA_restore+1);
   }
   Initialized = true;
 

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

+## Check we report a proper error when the content of the .eh_frame section
+## is truncated so that the initial instructions are can't be read.
+
+# RUN: llvm-mc -triple x86_64-unknown-linux %s -filetype=obj -o %t
+# RUN: not llvm-dwarfdump -debug-frame %t 2>&1 | FileCheck %s
+
+# CHECK: .eh_frame contents:
+# CHECK: error: unexpected end of data at offset 0xd while reading [0xd, 0xe)
+
+.section .eh_frame,"a",@unwind
+ .long 0xff       ## Length
+ .long 0x00000000 ## CIE ID
+ .byte 1          ## Version
+ .byte 0          ## Code Alignment Factor
+ .byte 0          ## Data Alignment Factor
+ .byte 0          ## Augmentation Length
+ .byte 0          ## Return Address Register
+## The set of Call Frame Instructions is supposed to be here,
+## but it is missing.