fix: fail closed on unsupported DWARF expressions

Stop DWARF expression parsing from returning values when a valid prefix is
followed by an invalid or unsupported operation.

Make CFI expression parsing reject unsupported opcodes instead of skipping
them, so traces fail rather than using partial compute steps.

Add unit coverage for both DWARF and CFI fail-open regressions.

Author: swananan

ghostscope-dwarf/src/index/cfi_index.rs

@@ -154,7 +154,7 @@ impl CfiIndex {
                 use gimli::Reader;
                 let temp = expression.0.to_slice().ok();
                 let expr_bytes = temp.as_deref().unwrap_or(&[]);
-                let steps = self.parse_dwarf_expression(expr_bytes)?;
+                let steps = Self::parse_dwarf_expression(expr_bytes)?;
                 CfaResult::Expression { steps }
             }
         };
@@ -355,7 +355,7 @@ impl CfiIndex {
         let expression = expr.get(&self.eh_frame)?;
         let temp = expression.0.to_slice().ok();
         let expr_bytes = temp.as_deref().unwrap_or(&[]);
-        self.parse_dwarf_expression(expr_bytes)
+        Self::parse_dwarf_expression(expr_bytes)
     }
 
     fn default_register_rule(register: u16) -> Option<RegisterRule<usize>> {
@@ -368,7 +368,7 @@ impl CfiIndex {
     }
 
     /// Parse DWARF expression bytes into ComputeStep sequence
-    fn parse_dwarf_expression(&self, expr_bytes: &[u8]) -> Result<Vec<ComputeStep>> {
+    fn parse_dwarf_expression(expr_bytes: &[u8]) -> Result<Vec<ComputeStep>> {
         let mut steps = Vec::new();
         let mut pc = 0;
 
@@ -381,7 +381,7 @@ impl CfiIndex {
                 0x70..=0x8f => {
                     let register = (opcode - 0x70) as u16;
                     // Read SLEB128 offset
-                    let (offset, bytes_read) = self.read_sleb128(&expr_bytes[pc..])?;
+                    let (offset, bytes_read) = Self::read_sleb128(&expr_bytes[pc..])?;
                     pc += bytes_read;
 
                     steps.push(ComputeStep::LoadRegister(register));
@@ -392,7 +392,7 @@ impl CfiIndex {
                 }
                 // DW_OP_plus_uconst
                 0x23 => {
-                    let (value, bytes_read) = self.read_uleb128(&expr_bytes[pc..])?;
+                    let (value, bytes_read) = Self::read_uleb128(&expr_bytes[pc..])?;
                     pc += bytes_read;
                     steps.push(ComputeStep::PushConstant(value as i64));
                     steps.push(ComputeStep::Add);
@@ -414,10 +414,15 @@ impl CfiIndex {
                 0x21 => steps.push(ComputeStep::Or),
                 // DW_OP_xor
                 0x27 => steps.push(ComputeStep::Xor),
+                // DW_OP_nop
+                0x96 => {}
 
                 _ => {
-                    debug!("Unhandled DWARF opcode 0x{:02x} in CFA expression", opcode);
-                    // For now, skip unknown opcodes
+                    return Err(anyhow!(
+                        "unsupported DWARF opcode 0x{:02x} in CFA expression at byte offset {}",
+                        opcode,
+                        pc - 1
+                    ));
                 }
             }
         }
@@ -426,7 +431,7 @@ impl CfiIndex {
     }
 
     /// Read ULEB128 from byte slice
-    fn read_uleb128(&self, data: &[u8]) -> Result<(u64, usize)> {
+    fn read_uleb128(data: &[u8]) -> Result<(u64, usize)> {
         let mut result = 0u64;
         let mut shift = 0;
         let mut bytes_read = 0;
@@ -444,7 +449,7 @@ impl CfiIndex {
     }
 
     /// Read SLEB128 from byte slice
-    fn read_sleb128(&self, data: &[u8]) -> Result<(i64, usize)> {
+    fn read_sleb128(data: &[u8]) -> Result<(i64, usize)> {
         let mut result = 0i64;
         let mut shift = 0;
         let mut bytes_read = 0;
@@ -491,9 +496,22 @@ pub struct CfiStats {
 
 #[cfg(test)]
 mod tests {
+    use super::CfiIndex;
+
     #[test]
     fn test_cfi_index_creation() {
         // This would need a real ELF file for testing
         // For now, just ensure the module compiles
     }
+
+    #[test]
+    fn cfa_expression_rejects_unknown_opcode_after_valid_prefix() {
+        let error = CfiIndex::parse_dwarf_expression(&[0x70, 0x00, 0xff])
+            .expect_err("unknown CFI expression opcode must not be skipped");
+
+        assert!(
+            error.to_string().contains("unsupported"),
+            "unexpected error: {error}"
+        );
+    }
 }

ghostscope-dwarf/src/parser/expression_evaluator.rs

@@ -341,7 +341,15 @@ impl ExpressionEvaluator {
         let mut has_stack_value = false;
 
         // Parse all operations in the expression
-        while let Ok(op) = Operation::parse(&mut expression.0, encoding) {
+        while !expression.0.is_empty() {
+            let offset = expr_bytes.len() - expression.0.len();
+            let op = Operation::parse(&mut expression.0, encoding).map_err(|error| {
+                anyhow::anyhow!(
+                    "failed to parse DWARF expression operation at byte offset {}: {}",
+                    offset,
+                    error
+                )
+            })?;
             if matches!(op, Operation::StackValue) {
                 has_stack_value = true;
                 debug!("Found DW_OP_stack_value - this is a computed value");
@@ -353,7 +361,16 @@ impl ExpressionEvaluator {
                 Operation::EntryValue { expression } => {
                     let mut inner = *expression;
                     let mut inner_ops: Vec<Operation<_>> = Vec::new();
-                    while let Ok(iop) = Operation::parse(&mut inner, encoding) {
+                    let inner_len = inner.len();
+                    while !inner.is_empty() {
+                        let offset = inner_len - inner.len();
+                        let iop = Operation::parse(&mut inner, encoding).map_err(|error| {
+                            anyhow::anyhow!(
+                                "failed to parse DW_OP_entry_value inner expression operation at byte offset {}: {}",
+                                offset,
+                                error
+                            )
+                        })?;
                         inner_ops.push(iop);
                     }
                     if inner_ops.len() == 1 {
@@ -524,18 +541,34 @@ impl ExpressionEvaluator {
                         // This marks the result as a computed value, not a memory location
                         // Already handled by has_stack_value flag
                     }
-                    ParsedOperation::Operation(Operation::Deref { size, .. }) => {
+                    ParsedOperation::Operation(Operation::Deref { size, space, .. }) => {
+                        if *space {
+                            return Err(anyhow::anyhow!(
+                                "unsupported DWARF expression operation: {:?}",
+                                op
+                            ));
+                        }
                         let mem_size = match size {
                             1 => MemoryAccessSize::U8,
                             2 => MemoryAccessSize::U16,
                             4 => MemoryAccessSize::U32,
                             8 => MemoryAccessSize::U64,
-                            _ => MemoryAccessSize::U64, // Default
+                            _ => {
+                                return Err(anyhow::anyhow!(
+                                    "unsupported DWARF dereference size {} in operation: {:?}",
+                                    size,
+                                    op
+                                ))
+                            }
                         };
                         steps.push(ComputeStep::Dereference { size: mem_size });
                     }
+                    ParsedOperation::Operation(Operation::Nop) => {}
                     _ => {
-                        debug!("Unhandled operation in expression: {:?}", op);
+                        return Err(anyhow::anyhow!(
+                            "unsupported DWARF expression operation: {:?}",
+                            op
+                        ));
                     }
                 }
             }
@@ -1600,6 +1633,14 @@ mod tests {
     use gimli::{Format, LittleEndian, Register, RunTimeEndian};
     use std::sync::Arc;
 
+    fn test_encoding() -> gimli::Encoding {
+        gimli::Encoding {
+            format: gimli::Format::Dwarf32,
+            version: 5,
+            address_size: 8,
+        }
+    }
+
     fn build_scanned_incoming_entry_value_fixture(
         register: u16,
         caller_value: u64,
@@ -1946,13 +1987,62 @@ mod tests {
         );
     }
 
+    #[test]
+    fn multi_op_expression_rejects_invalid_opcode_after_valid_prefix() {
+        let expr_bytes = [
+            constants::DW_OP_lit1.0,
+            0xff,
+            constants::DW_OP_stack_value.0,
+        ];
+
+        let error = ExpressionEvaluator::parse_expression_with_context(
+            &expr_bytes,
+            RunTimeEndian::Little,
+            test_encoding(),
+            None,
+            0,
+            None,
+            None,
+            None,
+            0,
+        )
+        .expect_err("invalid opcode after a valid prefix must not return a value");
+
+        assert!(
+            error.to_string().contains("DWARF expression"),
+            "unexpected error: {error}"
+        );
+    }
+
+    #[test]
+    fn multi_op_expression_rejects_unsupported_operation_after_valid_prefix() {
+        let expr_bytes = [
+            constants::DW_OP_lit1.0,
+            constants::DW_OP_drop.0,
+            constants::DW_OP_stack_value.0,
+        ];
+
+        let error = ExpressionEvaluator::parse_expression_with_context(
+            &expr_bytes,
+            RunTimeEndian::Little,
+            test_encoding(),
+            None,
+            0,
+            None,
+            None,
+            None,
+            0,
+        )
+        .expect_err("unsupported operation after a valid prefix must not return a value");
+
+        assert!(
+            error.to_string().contains("unsupported"),
+            "unexpected error: {error}"
+        );
+    }
+
     #[test]
     fn single_fbreg_fast_path_saturates_cfa_offset_addition() {
-        let encoding = gimli::Encoding {
-            format: gimli::Format::Dwarf32,
-            version: 5,
-            address_size: 8,
-        };
         let get_cfa = |_address| {
             Ok(Some(CfaResult::RegisterPlusOffset {
                 register: 7,
@@ -1963,7 +2053,7 @@ mod tests {
         let result = ExpressionEvaluator::parse_expression_with_context(
             &[0x91, 0x0a],
             RunTimeEndian::Little,
-            encoding,
+            test_encoding(),
             None,
             0,
             Some(&get_cfa),
@@ -1985,16 +2075,11 @@ mod tests {
 
     #[test]
     fn big_endian_dw_op_addr_preserves_absolute_address() {
-        let encoding = gimli::Encoding {
-            format: gimli::Format::Dwarf32,
-            version: 5,
-            address_size: 8,
-        };
         let expr_bytes = [0x03, 0, 0, 0, 0, 0, 0, 0x12, 0x34];
         let result = ExpressionEvaluator::parse_expression_with_context(
             &expr_bytes,
             gimli::RunTimeEndian::Big,
-            encoding,
+            test_encoding(),
             None,
             0,
             None,