[PATCH][PR gdb/34239][PR gdb/34299] gdb: Check bounds before reading DWARF expression operands

[PATCH][PR gdb/34239][PR gdb/34299] gdb: Check bounds before reading DWARF expression operands

[Jielun Wu]

Some DWARF expression opcodes read fixed-width operands or block payloads
from the expression buffer before checking that the bytes are available.
With a malformed expression, this can read past the end of the expression
buffer.

Add helpers that validate fixed-width integer reads and block payload
skips, and use them in dwarf_expr_context::execute_stack_op.  Also guard
the nested DW_OP_entry_value parser before reading the following opcode.

Add a DWARF assembler test that exercises truncated operands for these
opcodes.

Tested on x86_64-linux.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=34239
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=34299
---
 gdb/dwarf2/expr.c                             | 166 +++++++++------
 .../gdb.dwarf2/dw2-bad-dwarf-expr-bounds.exp  | 190 ++++++++++++++++++
 2 files changed, 300 insertions(+), 56 deletions(-)
 create mode 100644 gdb/testsuite/gdb.dwarf2/dw2-bad-dwarf-expr-bounds.exp

diff --git a/gdb/dwarf2/expr.c b/gdb/dwarf2/expr.c
index c71d4725b03..ead918388a4 100644
--- a/gdb/dwarf2/expr.c
+++ b/gdb/dwarf2/expr.c
@@ -1370,7 +1370,45 @@ safe_skip_leb128 (const gdb_byte *buf, const gdb_byte *buf_end)
     error (_("DWARF expression error: ran off end of buffer reading leb128 value"));
   return buf;
 }
-\f
+
+/* Helper to skip BYTES bytes or throw an error.  */
+
+static const gdb_byte *
+safe_skip_bytes (const gdb_byte *buf, const gdb_byte *buf_end,
+		 ULONGEST bytes)
+{
+  if (buf > buf_end || bytes > (ULONGEST) (buf_end - buf))
+    error (_("DWARF expression error: ran off end of buffer reading bytes"));
+  return buf + bytes;
+}
+
+/* Helper to read a fixed-width unsigned integer or throw an error.  */
+
+static const gdb_byte *
+safe_read_unsigned_integer (const gdb_byte *buf, const gdb_byte *buf_end,
+			    int len, bfd_endian byte_order, uint64_t *r)
+{
+  gdb_assert (len >= 0);
+
+  const gdb_byte *data = buf;
+  buf = safe_skip_bytes (buf, buf_end, len);
+  *r = extract_unsigned_integer (data, len, byte_order);
+  return buf;
+}
+
+/* Helper to read a fixed-width signed integer or throw an error.  */
+
+static const gdb_byte *
+safe_read_signed_integer (const gdb_byte *buf, const gdb_byte *buf_end,
+			  int len, bfd_endian byte_order, int64_t *r)
+{
+  gdb_assert (len >= 0);
+
+  const gdb_byte *data = buf;
+  buf = safe_skip_bytes (buf, buf_end, len);
+  *r = extract_signed_integer (data, len, byte_order);
+  return buf;
+}
 
 /* Check that the current operator is either at the end of an
    expression, or that it is followed by a composition operator or by
@@ -1478,7 +1516,7 @@ dwarf_block_to_dwarf_reg_deref (gdb::array_view<const gdb_byte> block,
       if (buf == NULL)
 	return -1;
       if ((int) dwarf_reg != dwarf_reg)
-       return -1;
+	return -1;
     }
   else
     return -1;
@@ -1488,6 +1526,8 @@ dwarf_block_to_dwarf_reg_deref (gdb::array_view<const gdb_byte> block,
     return -1;
   if (offset != 0)
     return -1;
+  if (buf >= buf_end)
+    return -1;
 
   if (*buf == DW_OP_deref)
     {
@@ -1498,7 +1538,7 @@ dwarf_block_to_dwarf_reg_deref (gdb::array_view<const gdb_byte> block,
     {
       buf++;
       if (buf >= buf_end)
-       return -1;
+	return -1;
       *deref_size_return = *buf++;
     }
   else
@@ -1688,9 +1728,10 @@ dwarf_expr_context::execute_stack_op (gdb::array_view<const gdb_byte> expr)
 	  break;
 
 	case DW_OP_addr:
-	  result = extract_unsigned_integer (op_ptr,
-					     this->m_addr_size, byte_order);
-	  op_ptr += this->m_addr_size;
+	  op_ptr = safe_read_unsigned_integer (op_ptr, op_end,
+					       this->m_addr_size,
+					       byte_order, &uoffset);
+	  result = uoffset;
 	  /* Some versions of GCC emit DW_OP_addr before
 	     DW_OP_GNU_push_tls_address.  In this case the value is an
 	     index, not an address.  We don't support things like
@@ -1723,44 +1764,52 @@ dwarf_expr_context::execute_stack_op (gdb::array_view<const gdb_byte> expr)
 	  break;
 
 	case DW_OP_const1u:
-	  result = extract_unsigned_integer (op_ptr, 1, byte_order);
+	  op_ptr = safe_read_unsigned_integer (op_ptr, op_end, 1,
+					       byte_order, &uoffset);
+	  result = uoffset;
 	  result_val = value_from_ulongest (address_type, result);
-	  op_ptr += 1;
 	  break;
 	case DW_OP_const1s:
-	  result = extract_signed_integer (op_ptr, 1, byte_order);
+	  op_ptr = safe_read_signed_integer (op_ptr, op_end, 1,
+					     byte_order, &offset);
+	  result = offset;
 	  result_val = value_from_ulongest (address_type, result);
-	  op_ptr += 1;
 	  break;
 	case DW_OP_const2u:
-	  result = extract_unsigned_integer (op_ptr, 2, byte_order);
+	  op_ptr = safe_read_unsigned_integer (op_ptr, op_end, 2,
+					       byte_order, &uoffset);
+	  result = uoffset;
 	  result_val = value_from_ulongest (address_type, result);
-	  op_ptr += 2;
 	  break;
 	case DW_OP_const2s:
-	  result = extract_signed_integer (op_ptr, 2, byte_order);
+	  op_ptr = safe_read_signed_integer (op_ptr, op_end, 2,
+					     byte_order, &offset);
+	  result = offset;
 	  result_val = value_from_ulongest (address_type, result);
-	  op_ptr += 2;
 	  break;
 	case DW_OP_const4u:
-	  result = extract_unsigned_integer (op_ptr, 4, byte_order);
+	  op_ptr = safe_read_unsigned_integer (op_ptr, op_end, 4,
+					       byte_order, &uoffset);
+	  result = uoffset;
 	  result_val = value_from_ulongest (address_type, result);
-	  op_ptr += 4;
 	  break;
 	case DW_OP_const4s:
-	  result = extract_signed_integer (op_ptr, 4, byte_order);
+	  op_ptr = safe_read_signed_integer (op_ptr, op_end, 4,
+					     byte_order, &offset);
+	  result = offset;
 	  result_val = value_from_ulongest (address_type, result);
-	  op_ptr += 4;
 	  break;
 	case DW_OP_const8u:
-	  result = extract_unsigned_integer (op_ptr, 8, byte_order);
+	  op_ptr = safe_read_unsigned_integer (op_ptr, op_end, 8,
+					       byte_order, &uoffset);
+	  result = uoffset;
 	  result_val = value_from_ulongest (address_type, result);
-	  op_ptr += 8;
 	  break;
 	case DW_OP_const8s:
-	  result = extract_signed_integer (op_ptr, 8, byte_order);
+	  op_ptr = safe_read_signed_integer (op_ptr, op_end, 8,
+					     byte_order, &offset);
+	  result = offset;
 	  result_val = value_from_ulongest (address_type, result);
-	  op_ptr += 8;
 	  break;
 	case DW_OP_constu:
 	  op_ptr = safe_read_uleb128 (op_ptr, op_end, &uoffset);
@@ -1836,12 +1885,10 @@ dwarf_expr_context::execute_stack_op (gdb::array_view<const gdb_byte> expr)
 	    uint64_t len;
 
 	    op_ptr = safe_read_uleb128 (op_ptr, op_end, &len);
-	    if (op_ptr + len > op_end)
-	      error (_("DW_OP_implicit_value: too few bytes available."));
 	    this->m_len = len;
 	    this->m_data = op_ptr;
 	    this->m_location = DWARF_VALUE_LITERAL;
-	    op_ptr += len;
+	    op_ptr = safe_skip_bytes (op_ptr, op_end, len);
 	    dwarf_expr_require_composition (op_ptr, op_end,
 					    "DW_OP_implicit_value");
 	  }
@@ -1861,9 +1908,10 @@ dwarf_expr_context::execute_stack_op (gdb::array_view<const gdb_byte> expr)
 	    int ref_addr_size = this->m_per_cu->ref_addr_size ();
 
 	    /* The referred-to DIE of sect_offset kind.  */
-	    this->m_len = extract_unsigned_integer (op_ptr, ref_addr_size,
-						  byte_order);
-	    op_ptr += ref_addr_size;
+	    op_ptr = safe_read_unsigned_integer (op_ptr, op_end,
+						 ref_addr_size, byte_order,
+						 &uoffset);
+	    this->m_len = uoffset;
 
 	    /* The byte offset into the data.  */
 	    op_ptr = safe_read_sleb128 (op_ptr, op_end, &len);
@@ -1972,7 +2020,9 @@ dwarf_expr_context::execute_stack_op (gdb::array_view<const gdb_byte> expr)
 	  goto no_push;
 
 	case DW_OP_pick:
-	  offset = *op_ptr++;
+	  op_ptr = safe_read_unsigned_integer (op_ptr, op_end, 1,
+					       byte_order, &uoffset);
+	  offset = uoffset;
 	  result_val = fetch (offset);
 	  in_stack_memory = fetch_in_stack_memory (offset);
 	  break;
@@ -2016,7 +2066,13 @@ dwarf_expr_context::execute_stack_op (gdb::array_view<const gdb_byte> expr)
 	case DW_OP_deref_type:
 	case DW_OP_GNU_deref_type:
 	  {
-	    int addr_size = (op == DW_OP_deref ? this->m_addr_size : *op_ptr++);
+	    int addr_size = this->m_addr_size;
+	    if (op != DW_OP_deref)
+	      {
+		op_ptr = safe_read_unsigned_integer (op_ptr, op_end, 1,
+						     byte_order, &uoffset);
+		addr_size = uoffset;
+	      }
 	    CORE_ADDR addr = fetch_address (0);
 	    struct type *type;
 
@@ -2249,8 +2305,8 @@ dwarf_expr_context::execute_stack_op (gdb::array_view<const gdb_byte> expr)
 	  break;
 
 	case DW_OP_skip:
-	  offset = extract_signed_integer (op_ptr, 2, byte_order);
-	  op_ptr += 2;
+	  op_ptr = safe_read_signed_integer (op_ptr, op_end, 2, byte_order,
+					     &offset);
 	  op_ptr += offset;
 	  goto no_push;
 
@@ -2258,8 +2314,8 @@ dwarf_expr_context::execute_stack_op (gdb::array_view<const gdb_byte> expr)
 	  {
 	    struct value *val;
 
-	    offset = extract_signed_integer (op_ptr, 2, byte_order);
-	    op_ptr += 2;
+	    op_ptr = safe_read_signed_integer (op_ptr, op_end, 2, byte_order,
+					       &offset);
 	    val = fetch (0);
 	    dwarf_require_integral (val->type ());
 	    if (value_as_long (val) != 0)
@@ -2313,18 +2369,18 @@ dwarf_expr_context::execute_stack_op (gdb::array_view<const gdb_byte> expr)
 
 	case DW_OP_call2:
 	  {
-	    cu_offset cu_off
-	      = (cu_offset) extract_unsigned_integer (op_ptr, 2, byte_order);
-	    op_ptr += 2;
+	    op_ptr = safe_read_unsigned_integer (op_ptr, op_end, 2,
+						 byte_order, &uoffset);
+	    cu_offset cu_off = (cu_offset) uoffset;
 	    this->dwarf_call (cu_off);
 	  }
 	  goto no_push;
 
 	case DW_OP_call4:
 	  {
-	    cu_offset cu_off
-	      = (cu_offset) extract_unsigned_integer (op_ptr, 4, byte_order);
-	    op_ptr += 4;
+	    op_ptr = safe_read_unsigned_integer (op_ptr, op_end, 4,
+						 byte_order, &uoffset);
+	    cu_offset cu_off = (cu_offset) uoffset;
 	    this->dwarf_call (cu_off);
 	  }
 	  goto no_push;
@@ -2334,11 +2390,10 @@ dwarf_expr_context::execute_stack_op (gdb::array_view<const gdb_byte> expr)
 	    ensure_have_per_cu (this->m_per_cu, "DW_OP_GNU_variable_value");
 	    int ref_addr_size = this->m_per_cu->ref_addr_size ();
 
-	    sect_offset sect_off
-	      = (sect_offset) extract_unsigned_integer (op_ptr,
-							ref_addr_size,
-							byte_order);
-	    op_ptr += ref_addr_size;
+	    op_ptr = safe_read_unsigned_integer (op_ptr, op_end,
+						 ref_addr_size, byte_order,
+						 &uoffset);
+	    sect_offset sect_off = (sect_offset) uoffset;
 	    result_val = sect_variable_value (sect_off, this->m_per_cu,
 					      this->m_per_objfile);
 	    result_val = value_cast (address_type, result_val);
@@ -2353,15 +2408,13 @@ dwarf_expr_context::execute_stack_op (gdb::array_view<const gdb_byte> expr)
 	    union call_site_parameter_u kind_u;
 
 	    op_ptr = safe_read_uleb128 (op_ptr, op_end, &len);
-	    if (op_ptr + len > op_end)
-	      error (_("DW_OP_entry_value: too few bytes available."));
+	    const gdb_byte *expr_ptr = op_ptr;
+	    op_ptr = safe_skip_bytes (op_ptr, op_end, len);
 
-	    auto entry_value_expr = gdb::make_array_view (op_ptr, len);
+	    auto entry_value_expr = gdb::make_array_view (expr_ptr, len);
 	    kind_u.dwarf_reg = dwarf_block_to_dwarf_reg (entry_value_expr);
 	    if (kind_u.dwarf_reg != -1)
 	      {
-		op_ptr += len;
-
 		if (trivial_entry_value (this->m_frame))
 		  {
 		    /* We can assume that DW_OP_entry_value (expr) == expr.
@@ -2384,7 +2437,6 @@ dwarf_expr_context::execute_stack_op (gdb::array_view<const gdb_byte> expr)
 	      {
 		if (deref_size == -1)
 		  deref_size = this->m_addr_size;
-		op_ptr += len;
 
 		if (trivial_entry_value (this->m_frame))
 		  {
@@ -2410,9 +2462,9 @@ dwarf_expr_context::execute_stack_op (gdb::array_view<const gdb_byte> expr)
 	  {
 	    union call_site_parameter_u kind_u;
 
-	    kind_u.param_cu_off
-	      = (cu_offset) extract_unsigned_integer (op_ptr, 4, byte_order);
-	    op_ptr += 4;
+	    op_ptr = safe_read_unsigned_integer (op_ptr, op_end, 4,
+						 byte_order, &uoffset);
+	    kind_u.param_cu_off = (cu_offset) uoffset;
 	    this->push_dwarf_reg_entry_value (CALL_SITE_PARAMETER_PARAM_OFFSET,
 					      kind_u,
 					      -1 /* deref_size */);
@@ -2429,9 +2481,11 @@ dwarf_expr_context::execute_stack_op (gdb::array_view<const gdb_byte> expr)
 	    op_ptr = safe_read_uleb128 (op_ptr, op_end, &uoffset);
 	    cu_offset type_die_cu_off = (cu_offset) uoffset;
 
-	    n = *op_ptr++;
+	    op_ptr = safe_read_unsigned_integer (op_ptr, op_end, 1, byte_order,
+						 &uoffset);
+	    n = uoffset;
 	    data = op_ptr;
-	    op_ptr += n;
+	    op_ptr = safe_skip_bytes (op_ptr, op_end, n);
 
 	    type = get_base_type (type_die_cu_off);
 
diff --git a/gdb/testsuite/gdb.dwarf2/dw2-bad-dwarf-expr-bounds.exp b/gdb/testsuite/gdb.dwarf2/dw2-bad-dwarf-expr-bounds.exp
new file mode 100644
index 00000000000..7f5fa22388d
--- /dev/null
+++ b/gdb/testsuite/gdb.dwarf2/dw2-bad-dwarf-expr-bounds.exp
@@ -0,0 +1,190 @@
+# Copyright 2026 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# Test malformed DWARF expressions whose opcodes have truncated operands.
+
+load_lib dwarf.exp
+
+require dwarf2_support
+
+standard_testfile main.c -dw.S
+
+set asm_file [standard_output_file $srcfile2]
+Dwarf::assemble $asm_file {
+    global srcfile
+
+    declare_labels int_label
+
+    cu {label cu_label} {
+	DW_TAG_compile_unit {
+	    DW_AT_name $srcfile
+	    DW_AT_language @DW_LANG_C
+	} {
+	    int_label: DW_TAG_base_type {
+		DW_AT_name "int"
+		DW_AT_encoding @DW_ATE_signed
+		DW_AT_byte_size 4 DW_FORM_sdata
+	    }
+
+	    foreach {var op} {
+		truncated_addr DW_OP_addr
+		truncated_const1u DW_OP_const1u
+		truncated_const1s DW_OP_const1s
+		truncated_const2u DW_OP_const2u
+		truncated_const2s DW_OP_const2s
+		truncated_const4u DW_OP_const4u
+		truncated_const4s DW_OP_const4s
+		truncated_const8u DW_OP_const8u
+		truncated_const8s DW_OP_const8s
+		truncated_implicit_pointer DW_OP_implicit_pointer
+		truncated_gnu_implicit_pointer DW_OP_GNU_implicit_pointer
+		truncated_pick DW_OP_pick
+		truncated_deref_size DW_OP_deref_size
+		truncated_deref_type DW_OP_deref_type
+		truncated_gnu_deref_type DW_OP_GNU_deref_type
+		truncated_skip DW_OP_skip
+		truncated_bra DW_OP_bra
+		truncated_parameter_ref DW_OP_GNU_parameter_ref
+	    } {
+		DW_TAG_variable {
+		    DW_AT_name $var
+		    DW_AT_type :$int_label
+		    DW_AT_location {
+			_op .byte $Dwarf::_constants($op) $op
+		    } SPECIAL_expr
+		}
+	    }
+
+	    DW_TAG_variable {
+		DW_AT_name "truncated_implicit_value"
+		DW_AT_type :$int_label
+		DW_AT_location {
+		    _op .byte $Dwarf::_constants(DW_OP_implicit_value) \
+			DW_OP_implicit_value
+		    _op .uleb128 4 "implicit value length"
+		    _op .2byte 0 "truncated implicit value bytes"
+		} SPECIAL_expr
+	    }
+
+	    DW_TAG_variable {
+		DW_AT_name "truncated_entry_value"
+		DW_AT_type :$int_label
+		DW_AT_location {
+		    _op .byte $Dwarf::_constants(DW_OP_entry_value) \
+			DW_OP_entry_value
+		    _op .uleb128 4 "entry value expression length"
+		    _op .2byte 0 "truncated entry value expression"
+		} SPECIAL_expr
+	    }
+
+	    DW_TAG_variable {
+		DW_AT_name "truncated_gnu_entry_value"
+		DW_AT_type :$int_label
+		DW_AT_location {
+		    _op .byte $Dwarf::_constants(DW_OP_GNU_entry_value) \
+			DW_OP_GNU_entry_value
+		    _op .uleb128 4 "entry value expression length"
+		    _op .2byte 0 "truncated entry value expression"
+		} SPECIAL_expr
+	    }
+
+	    DW_TAG_variable {
+		DW_AT_name "truncated_const_type_size"
+		DW_AT_type :$int_label
+		DW_AT_location {
+		    _op .byte $Dwarf::_constants(DW_OP_const_type) \
+			DW_OP_const_type
+		    _op .uleb128 "$int_label - $cu_label" "type DIE offset"
+		} SPECIAL_expr
+	    }
+
+	    DW_TAG_variable {
+		DW_AT_name "truncated_const_type_payload"
+		DW_AT_type :$int_label
+		DW_AT_location {
+		    _op .byte $Dwarf::_constants(DW_OP_const_type) \
+			DW_OP_const_type
+		    _op .uleb128 "$int_label - $cu_label" "type DIE offset"
+		    _op .byte 4 "constant block length"
+		    _op .2byte 0 "truncated constant block"
+		} SPECIAL_expr
+	    }
+
+	    DW_TAG_variable {
+		DW_AT_name "truncated_gnu_const_type_size"
+		DW_AT_type :$int_label
+		DW_AT_location {
+		    _op .byte $Dwarf::_constants(DW_OP_GNU_const_type) \
+			DW_OP_GNU_const_type
+		    _op .uleb128 "$int_label - $cu_label" "type DIE offset"
+		} SPECIAL_expr
+	    }
+
+	    DW_TAG_variable {
+		DW_AT_name "truncated_gnu_const_type_payload"
+		DW_AT_type :$int_label
+		DW_AT_location {
+		    _op .byte $Dwarf::_constants(DW_OP_GNU_const_type) \
+			DW_OP_GNU_const_type
+		    _op .uleb128 "$int_label - $cu_label" "type DIE offset"
+		    _op .byte 4 "constant block length"
+		    _op .2byte 0 "truncated constant block"
+		} SPECIAL_expr
+	    }
+	}
+    }
+}
+
+if {[prepare_for_testing "failed to prepare" ${testfile} \
+	 [list $srcfile $asm_file] nodebug]} {
+    return
+}
+
+if {![runto_main]} {
+    return
+}
+
+foreach var {
+    truncated_addr
+    truncated_const1u
+    truncated_const1s
+    truncated_const2u
+    truncated_const2s
+    truncated_const4u
+    truncated_const4s
+    truncated_const8u
+    truncated_const8s
+    truncated_implicit_value
+    truncated_implicit_pointer
+    truncated_gnu_implicit_pointer
+    truncated_pick
+    truncated_deref_size
+    truncated_deref_type
+    truncated_gnu_deref_type
+    truncated_skip
+    truncated_bra
+    truncated_entry_value
+    truncated_gnu_entry_value
+    truncated_parameter_ref
+    truncated_const_type_size
+    truncated_const_type_payload
+    truncated_gnu_const_type_size
+    truncated_gnu_const_type_payload
+} {
+    with_test_prefix $var {
+	gdb_test "print $var" \
+	    ".*DWARF expression error: ran off end of buffer reading .*"
+    }
+}
-- 
2.34.1

Re: [PATCH][PR gdb/34239][PR gdb/34299] gdb: Check bounds before reading DWARF expression operands

[Guinevere Larsen]

On 6/18/26 3:01 PM, Jielun Wu wrote:
> Some DWARF expression opcodes read fixed-width operands or block payloads
> from the expression buffer before checking that the bytes are available.
> With a malformed expression, this can read past the end of the expression
> buffer.
>
> Add helpers that validate fixed-width integer reads and block payload
> skips, and use them in dwarf_expr_context::execute_stack_op.  Also guard
> the nested DW_OP_entry_value parser before reading the following opcode.
>
> Add a DWARF assembler test that exercises truncated operands for these
> opcodes.
>
> Tested on x86_64-linux.
>
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=34239
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=34299
> ---

Hi! Thanks for working on this!

I'm not too familiar with the dwarf parser, so I can only do a 
superficial review. Most of my comments are questions and nitpicks, I 
don't think you necessarily need to send a new version just from this 
review.

I have tested this and see no regressions, so feel free to add my test 
tag to the git trailers

Tested-By: Guinevere Larsen <guinevere@redhat.com>

>   gdb/dwarf2/expr.c                             | 166 +++++++++------
>   .../gdb.dwarf2/dw2-bad-dwarf-expr-bounds.exp  | 190 ++++++++++++++++++
>   2 files changed, 300 insertions(+), 56 deletions(-)
>   create mode 100644 gdb/testsuite/gdb.dwarf2/dw2-bad-dwarf-expr-bounds.exp
>
> diff --git a/gdb/dwarf2/expr.c b/gdb/dwarf2/expr.c
> index c71d4725b03..ead918388a4 100644
> --- a/gdb/dwarf2/expr.c
> +++ b/gdb/dwarf2/expr.c
> @@ -1370,7 +1370,45 @@ safe_skip_leb128 (const gdb_byte *buf, const gdb_byte *buf_end)
>       error (_("DWARF expression error: ran off end of buffer reading leb128 value"));
>     return buf;
>   }
> -\f
> +
> +/* Helper to skip BYTES bytes or throw an error.  */
> +
> +static const gdb_byte *
> +safe_skip_bytes (const gdb_byte *buf, const gdb_byte *buf_end,
> +		 ULONGEST bytes)
> +{
> +  if (buf > buf_end || bytes > (ULONGEST) (buf_end - buf))
> +    error (_("DWARF expression error: ran off end of buffer reading bytes"));
> +  return buf + bytes;
> +}
> +
> +/* Helper to read a fixed-width unsigned integer or throw an error.  */
> +
> +static const gdb_byte *
> +safe_read_unsigned_integer (const gdb_byte *buf, const gdb_byte *buf_end,
> +			    int len, bfd_endian byte_order, uint64_t *r)
> +{
> +  gdb_assert (len >= 0);
> +
> +  const gdb_byte *data = buf;
> +  buf = safe_skip_bytes (buf, buf_end, len);
> +  *r = extract_unsigned_integer (data, len, byte_order);
> +  return buf;

Oh, ok, it took me a while of thinking but I think I now understand why 
you wrote it like this.

You advance the buffer first to use that as a check that it is possible 
to read that many bytes from the buffer, instead of needing to double up 
on the if condition, right?

If so, I think it would be nice to have a comment explaining it, because 
I went through 2 incorrect explanations before reaching that conclusion.

> +}
> +
> +/* Helper to read a fixed-width signed integer or throw an error.  */
> +
> +static const gdb_byte *
> +safe_read_signed_integer (const gdb_byte *buf, const gdb_byte *buf_end,
> +			  int len, bfd_endian byte_order, int64_t *r)
> +{
> +  gdb_assert (len >= 0);
> +
> +  const gdb_byte *data = buf;
> +  buf = safe_skip_bytes (buf, buf_end, len);
> +  *r = extract_signed_integer (data, len, byte_order);
> +  return buf;
Similar comment as above
> +}
>   
>   /* Check that the current operator is either at the end of an
>      expression, or that it is followed by a composition operator or by
> @@ -1478,7 +1516,7 @@ dwarf_block_to_dwarf_reg_deref (gdb::array_view<const gdb_byte> block,
>         if (buf == NULL)
>   	return -1;
>         if ((int) dwarf_reg != dwarf_reg)
> -       return -1;
> +	return -1;
>       }
>     else
>       return -1;
> @@ -1488,6 +1526,8 @@ dwarf_block_to_dwarf_reg_deref (gdb::array_view<const gdb_byte> block,
>       return -1;
>     if (offset != 0)
>       return -1;
> +  if (buf >= buf_end)
> +    return -1;
>   
>     if (*buf == DW_OP_deref)
>       {
> @@ -1498,7 +1538,7 @@ dwarf_block_to_dwarf_reg_deref (gdb::array_view<const gdb_byte> block,
>       {
>         buf++;
>         if (buf >= buf_end)
> -       return -1;
> +	return -1;
>         *deref_size_return = *buf++;
>       }
>     else
> @@ -1688,9 +1728,10 @@ dwarf_expr_context::execute_stack_op (gdb::array_view<const gdb_byte> expr)
>   	  break;
>   
>   	case DW_OP_addr:
> -	  result = extract_unsigned_integer (op_ptr,
> -					     this->m_addr_size, byte_order);
> -	  op_ptr += this->m_addr_size;
> +	  op_ptr = safe_read_unsigned_integer (op_ptr, op_end,
> +					       this->m_addr_size,
> +					       byte_order, &uoffset);

Why not just send "&result" as the parameter here, instead of &uoffset

Same question for all the occurrences below

> +	  result = uoffset;
>   	  /* Some versions of GCC emit DW_OP_addr before
>   	     DW_OP_GNU_push_tls_address.  In this case the value is an
>   	     index, not an address.  We don't support things like
> @@ -1723,44 +1764,52 @@ dwarf_expr_context::execute_stack_op (gdb::array_view<const gdb_byte> expr)
>   	  break;
>   
>   	case DW_OP_const1u:
> -	  result = extract_unsigned_integer (op_ptr, 1, byte_order);
> +	  op_ptr = safe_read_unsigned_integer (op_ptr, op_end, 1,
> +					       byte_order, &uoffset);
> +	  result = uoffset;
>   	  result_val = value_from_ulongest (address_type, result);
> -	  op_ptr += 1;
>   	  break;
>   	case DW_OP_const1s:
> -	  result = extract_signed_integer (op_ptr, 1, byte_order);
> +	  op_ptr = safe_read_signed_integer (op_ptr, op_end, 1,
> +					     byte_order, &offset);
> +	  result = offset;
>   	  result_val = value_from_ulongest (address_type, result);
> -	  op_ptr += 1;
>   	  break;
>   	case DW_OP_const2u:
> -	  result = extract_unsigned_integer (op_ptr, 2, byte_order);
> +	  op_ptr = safe_read_unsigned_integer (op_ptr, op_end, 2,
> +					       byte_order, &uoffset);
> +	  result = uoffset;
>   	  result_val = value_from_ulongest (address_type, result);
> -	  op_ptr += 2;
>   	  break;
>   	case DW_OP_const2s:
> -	  result = extract_signed_integer (op_ptr, 2, byte_order);
> +	  op_ptr = safe_read_signed_integer (op_ptr, op_end, 2,
> +					     byte_order, &offset);
> +	  result = offset;
>   	  result_val = value_from_ulongest (address_type, result);
> -	  op_ptr += 2;
>   	  break;
>   	case DW_OP_const4u:
> -	  result = extract_unsigned_integer (op_ptr, 4, byte_order);
> +	  op_ptr = safe_read_unsigned_integer (op_ptr, op_end, 4,
> +					       byte_order, &uoffset);
> +	  result = uoffset;
>   	  result_val = value_from_ulongest (address_type, result);
> -	  op_ptr += 4;
>   	  break;
>   	case DW_OP_const4s:
> -	  result = extract_signed_integer (op_ptr, 4, byte_order);
> +	  op_ptr = safe_read_signed_integer (op_ptr, op_end, 4,
> +					     byte_order, &offset);
> +	  result = offset;
>   	  result_val = value_from_ulongest (address_type, result);
> -	  op_ptr += 4;
>   	  break;
>   	case DW_OP_const8u:
> -	  result = extract_unsigned_integer (op_ptr, 8, byte_order);
> +	  op_ptr = safe_read_unsigned_integer (op_ptr, op_end, 8,
> +					       byte_order, &uoffset);
> +	  result = uoffset;
>   	  result_val = value_from_ulongest (address_type, result);
> -	  op_ptr += 8;
>   	  break;
>   	case DW_OP_const8s:
> -	  result = extract_signed_integer (op_ptr, 8, byte_order);
> +	  op_ptr = safe_read_signed_integer (op_ptr, op_end, 8,
> +					     byte_order, &offset);
> +	  result = offset;
>   	  result_val = value_from_ulongest (address_type, result);
> -	  op_ptr += 8;
>   	  break;
>   	case DW_OP_constu:
>   	  op_ptr = safe_read_uleb128 (op_ptr, op_end, &uoffset);
> @@ -1836,12 +1885,10 @@ dwarf_expr_context::execute_stack_op (gdb::array_view<const gdb_byte> expr)
>   	    uint64_t len;
>   
>   	    op_ptr = safe_read_uleb128 (op_ptr, op_end, &len);
> -	    if (op_ptr + len > op_end)
> -	      error (_("DW_OP_implicit_value: too few bytes available."));
>   	    this->m_len = len;
>   	    this->m_data = op_ptr;
>   	    this->m_location = DWARF_VALUE_LITERAL;
> -	    op_ptr += len;
> +	    op_ptr = safe_skip_bytes (op_ptr, op_end, len);
>   	    dwarf_expr_require_composition (op_ptr, op_end,
>   					    "DW_OP_implicit_value");
>   	  }
> @@ -1861,9 +1908,10 @@ dwarf_expr_context::execute_stack_op (gdb::array_view<const gdb_byte> expr)
>   	    int ref_addr_size = this->m_per_cu->ref_addr_size ();
>   
>   	    /* The referred-to DIE of sect_offset kind.  */
> -	    this->m_len = extract_unsigned_integer (op_ptr, ref_addr_size,
> -						  byte_order);
> -	    op_ptr += ref_addr_size;
> +	    op_ptr = safe_read_unsigned_integer (op_ptr, op_end,
> +						 ref_addr_size, byte_order,
> +						 &uoffset);
> +	    this->m_len = uoffset;
Again, why not send "&this->m_len" ?
>   
>   	    /* The byte offset into the data.  */
>   	    op_ptr = safe_read_sleb128 (op_ptr, op_end, &len);
> @@ -1972,7 +2020,9 @@ dwarf_expr_context::execute_stack_op (gdb::array_view<const gdb_byte> expr)
>   	  goto no_push;
>   
>   	case DW_OP_pick:
> -	  offset = *op_ptr++;
> +	  op_ptr = safe_read_unsigned_integer (op_ptr, op_end, 1,
> +					       byte_order, &uoffset);
> +	  offset = uoffset;
>   	  result_val = fetch (offset);
>   	  in_stack_memory = fetch_in_stack_memory (offset);
>   	  break;
> @@ -2016,7 +2066,13 @@ dwarf_expr_context::execute_stack_op (gdb::array_view<const gdb_byte> expr)
>   	case DW_OP_deref_type:
>   	case DW_OP_GNU_deref_type:
>   	  {
> -	    int addr_size = (op == DW_OP_deref ? this->m_addr_size : *op_ptr++);
> +	    int addr_size = this->m_addr_size;
> +	    if (op != DW_OP_deref)
> +	      {
> +		op_ptr = safe_read_unsigned_integer (op_ptr, op_end, 1,
> +						     byte_order, &uoffset);
> +		addr_size = uoffset;

Maybe I'm missing something, but this seems like an incorrect change?

It seems like you should call safe_skip_bytes instead of 
read_unsigned_integer, and assign addr_size to *op_ptr?

> +	      }
>   	    CORE_ADDR addr = fetch_address (0);
>   	    struct type *type;
>   
> @@ -2249,8 +2305,8 @@ dwarf_expr_context::execute_stack_op (gdb::array_view<const gdb_byte> expr)
>   	  break;
>   
>   	case DW_OP_skip:
> -	  offset = extract_signed_integer (op_ptr, 2, byte_order);
> -	  op_ptr += 2;
> +	  op_ptr = safe_read_signed_integer (op_ptr, op_end, 2, byte_order,
> +					     &offset);
>   	  op_ptr += offset;
This should also use safe_skip_bytes
>   	  goto no_push;
>   
> @@ -2258,8 +2314,8 @@ dwarf_expr_context::execute_stack_op (gdb::array_view<const gdb_byte> expr)
>   	  {
>   	    struct value *val;
>   
> -	    offset = extract_signed_integer (op_ptr, 2, byte_order);
> -	    op_ptr += 2;
> +	    op_ptr = safe_read_signed_integer (op_ptr, op_end, 2, byte_order,
> +					       &offset);
>   	    val = fetch (0);
>   	    dwarf_require_integral (val->type ());
>   	    if (value_as_long (val) != 0)
> @@ -2313,18 +2369,18 @@ dwarf_expr_context::execute_stack_op (gdb::array_view<const gdb_byte> expr)
>   
>   	case DW_OP_call2:
>   	  {
> -	    cu_offset cu_off
> -	      = (cu_offset) extract_unsigned_integer (op_ptr, 2, byte_order);
> -	    op_ptr += 2;
> +	    op_ptr = safe_read_unsigned_integer (op_ptr, op_end, 2,
> +						 byte_order, &uoffset);
> +	    cu_offset cu_off = (cu_offset) uoffset;
>   	    this->dwarf_call (cu_off);
>   	  }
>   	  goto no_push;
>   
>   	case DW_OP_call4:
>   	  {
> -	    cu_offset cu_off
> -	      = (cu_offset) extract_unsigned_integer (op_ptr, 4, byte_order);
> -	    op_ptr += 4;
> +	    op_ptr = safe_read_unsigned_integer (op_ptr, op_end, 4,
> +						 byte_order, &uoffset);
> +	    cu_offset cu_off = (cu_offset) uoffset;
>   	    this->dwarf_call (cu_off);
>   	  }
>   	  goto no_push;
> @@ -2334,11 +2390,10 @@ dwarf_expr_context::execute_stack_op (gdb::array_view<const gdb_byte> expr)
>   	    ensure_have_per_cu (this->m_per_cu, "DW_OP_GNU_variable_value");
>   	    int ref_addr_size = this->m_per_cu->ref_addr_size ();
>   
> -	    sect_offset sect_off
> -	      = (sect_offset) extract_unsigned_integer (op_ptr,
> -							ref_addr_size,
> -							byte_order);
> -	    op_ptr += ref_addr_size;
> +	    op_ptr = safe_read_unsigned_integer (op_ptr, op_end,
> +						 ref_addr_size, byte_order,
> +						 &uoffset);
> +	    sect_offset sect_off = (sect_offset) uoffset;
>   	    result_val = sect_variable_value (sect_off, this->m_per_cu,
>   					      this->m_per_objfile);
>   	    result_val = value_cast (address_type, result_val);
> @@ -2353,15 +2408,13 @@ dwarf_expr_context::execute_stack_op (gdb::array_view<const gdb_byte> expr)
>   	    union call_site_parameter_u kind_u;
>   
>   	    op_ptr = safe_read_uleb128 (op_ptr, op_end, &len);
> -	    if (op_ptr + len > op_end)
> -	      error (_("DW_OP_entry_value: too few bytes available."));
> +	    const gdb_byte *expr_ptr = op_ptr;
> +	    op_ptr = safe_skip_bytes (op_ptr, op_end, len);
>   
> -	    auto entry_value_expr = gdb::make_array_view (op_ptr, len);
> +	    auto entry_value_expr = gdb::make_array_view (expr_ptr, len);
>   	    kind_u.dwarf_reg = dwarf_block_to_dwarf_reg (entry_value_expr);
>   	    if (kind_u.dwarf_reg != -1)
>   	      {
> -		op_ptr += len;
> -
>   		if (trivial_entry_value (this->m_frame))
>   		  {
>   		    /* We can assume that DW_OP_entry_value (expr) == expr.
> @@ -2384,7 +2437,6 @@ dwarf_expr_context::execute_stack_op (gdb::array_view<const gdb_byte> expr)
>   	      {
>   		if (deref_size == -1)
>   		  deref_size = this->m_addr_size;
> -		op_ptr += len;
>   
>   		if (trivial_entry_value (this->m_frame))
>   		  {
> @@ -2410,9 +2462,9 @@ dwarf_expr_context::execute_stack_op (gdb::array_view<const gdb_byte> expr)
>   	  {
>   	    union call_site_parameter_u kind_u;
>   
> -	    kind_u.param_cu_off
> -	      = (cu_offset) extract_unsigned_integer (op_ptr, 4, byte_order);
> -	    op_ptr += 4;
> +	    op_ptr = safe_read_unsigned_integer (op_ptr, op_end, 4,
> +						 byte_order, &uoffset);
> +	    kind_u.param_cu_off = (cu_offset) uoffset;
>   	    this->push_dwarf_reg_entry_value (CALL_SITE_PARAMETER_PARAM_OFFSET,
>   					      kind_u,
>   					      -1 /* deref_size */);
> @@ -2429,9 +2481,11 @@ dwarf_expr_context::execute_stack_op (gdb::array_view<const gdb_byte> expr)
>   	    op_ptr = safe_read_uleb128 (op_ptr, op_end, &uoffset);
>   	    cu_offset type_die_cu_off = (cu_offset) uoffset;
>   
> -	    n = *op_ptr++;
> +	    op_ptr = safe_read_unsigned_integer (op_ptr, op_end, 1, byte_order,
> +						 &uoffset);
> +	    n = uoffset;
>   	    data = op_ptr;
> -	    op_ptr += n;
> +	    op_ptr = safe_skip_bytes (op_ptr, op_end, n);
>   
>   	    type = get_base_type (type_die_cu_off);
>   
> diff --git a/gdb/testsuite/gdb.dwarf2/dw2-bad-dwarf-expr-bounds.exp b/gdb/testsuite/gdb.dwarf2/dw2-bad-dwarf-expr-bounds.exp
> new file mode 100644
> index 00000000000..7f5fa22388d
> --- /dev/null
> +++ b/gdb/testsuite/gdb.dwarf2/dw2-bad-dwarf-expr-bounds.exp
> @@ -0,0 +1,190 @@
> +# Copyright 2026 Free Software Foundation, Inc.
> +
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +# Test malformed DWARF expressions whose opcodes have truncated operands.
> +
> +load_lib dwarf.exp
> +
> +require dwarf2_support
> +
> +standard_testfile main.c -dw.S
> +
> +set asm_file [standard_output_file $srcfile2]
> +Dwarf::assemble $asm_file {
> +    global srcfile
> +
> +    declare_labels int_label
> +
> +    cu {label cu_label} {
> +	DW_TAG_compile_unit {
> +	    DW_AT_name $srcfile
> +	    DW_AT_language @DW_LANG_C
> +	} {
> +	    int_label: DW_TAG_base_type {
> +		DW_AT_name "int"
> +		DW_AT_encoding @DW_ATE_signed
> +		DW_AT_byte_size 4 DW_FORM_sdata
> +	    }
> +
> +	    foreach {var op} {
> +		truncated_addr DW_OP_addr
> +		truncated_const1u DW_OP_const1u
> +		truncated_const1s DW_OP_const1s
> +		truncated_const2u DW_OP_const2u
> +		truncated_const2s DW_OP_const2s
> +		truncated_const4u DW_OP_const4u
> +		truncated_const4s DW_OP_const4s
> +		truncated_const8u DW_OP_const8u
> +		truncated_const8s DW_OP_const8s
> +		truncated_implicit_pointer DW_OP_implicit_pointer
> +		truncated_gnu_implicit_pointer DW_OP_GNU_implicit_pointer
> +		truncated_pick DW_OP_pick
> +		truncated_deref_size DW_OP_deref_size
> +		truncated_deref_type DW_OP_deref_type
> +		truncated_gnu_deref_type DW_OP_GNU_deref_type
> +		truncated_skip DW_OP_skip
> +		truncated_bra DW_OP_bra
> +		truncated_parameter_ref DW_OP_GNU_parameter_ref
> +	    } {
> +		DW_TAG_variable {
> +		    DW_AT_name $var
> +		    DW_AT_type :$int_label
> +		    DW_AT_location {
> +			_op .byte $Dwarf::_constants($op) $op
> +		    } SPECIAL_expr
> +		}
> +	    }
> +
> +	    DW_TAG_variable {
> +		DW_AT_name "truncated_implicit_value"
> +		DW_AT_type :$int_label
> +		DW_AT_location {
> +		    _op .byte $Dwarf::_constants(DW_OP_implicit_value) \
> +			DW_OP_implicit_value
> +		    _op .uleb128 4 "implicit value length"
> +		    _op .2byte 0 "truncated implicit value bytes"
> +		} SPECIAL_expr
> +	    }
> +
> +	    DW_TAG_variable {
> +		DW_AT_name "truncated_entry_value"
> +		DW_AT_type :$int_label
> +		DW_AT_location {
> +		    _op .byte $Dwarf::_constants(DW_OP_entry_value) \
> +			DW_OP_entry_value
> +		    _op .uleb128 4 "entry value expression length"
> +		    _op .2byte 0 "truncated entry value expression"
> +		} SPECIAL_expr
> +	    }
> +
> +	    DW_TAG_variable {
> +		DW_AT_name "truncated_gnu_entry_value"
> +		DW_AT_type :$int_label
> +		DW_AT_location {
> +		    _op .byte $Dwarf::_constants(DW_OP_GNU_entry_value) \
> +			DW_OP_GNU_entry_value
> +		    _op .uleb128 4 "entry value expression length"
> +		    _op .2byte 0 "truncated entry value expression"
> +		} SPECIAL_expr
> +	    }
> +
> +	    DW_TAG_variable {
> +		DW_AT_name "truncated_const_type_size"
> +		DW_AT_type :$int_label
> +		DW_AT_location {
> +		    _op .byte $Dwarf::_constants(DW_OP_const_type) \
> +			DW_OP_const_type
> +		    _op .uleb128 "$int_label - $cu_label" "type DIE offset"
> +		} SPECIAL_expr
> +	    }
> +
> +	    DW_TAG_variable {
> +		DW_AT_name "truncated_const_type_payload"
> +		DW_AT_type :$int_label
> +		DW_AT_location {
> +		    _op .byte $Dwarf::_constants(DW_OP_const_type) \
> +			DW_OP_const_type
> +		    _op .uleb128 "$int_label - $cu_label" "type DIE offset"
> +		    _op .byte 4 "constant block length"
> +		    _op .2byte 0 "truncated constant block"
> +		} SPECIAL_expr
> +	    }
> +
> +	    DW_TAG_variable {
> +		DW_AT_name "truncated_gnu_const_type_size"
> +		DW_AT_type :$int_label
> +		DW_AT_location {
> +		    _op .byte $Dwarf::_constants(DW_OP_GNU_const_type) \
> +			DW_OP_GNU_const_type
> +		    _op .uleb128 "$int_label - $cu_label" "type DIE offset"
> +		} SPECIAL_expr
> +	    }
> +
> +	    DW_TAG_variable {
> +		DW_AT_name "truncated_gnu_const_type_payload"
> +		DW_AT_type :$int_label
> +		DW_AT_location {
> +		    _op .byte $Dwarf::_constants(DW_OP_GNU_const_type) \
> +			DW_OP_GNU_const_type
> +		    _op .uleb128 "$int_label - $cu_label" "type DIE offset"
> +		    _op .byte 4 "constant block length"
> +		    _op .2byte 0 "truncated constant block"
> +		} SPECIAL_expr
> +	    }
> +	}
> +    }
> +}
> +
> +if {[prepare_for_testing "failed to prepare" ${testfile} \
> +	 [list $srcfile $asm_file] nodebug]} {
> +    return
> +}
> +
> +if {![runto_main]} {
> +    return
> +}
> +
> +foreach var {
> +    truncated_addr
> +    truncated_const1u
> +    truncated_const1s
> +    truncated_const2u
> +    truncated_const2s
> +    truncated_const4u
> +    truncated_const4s
> +    truncated_const8u
> +    truncated_const8s
> +    truncated_implicit_value
> +    truncated_implicit_pointer
> +    truncated_gnu_implicit_pointer
> +    truncated_pick
> +    truncated_deref_size
> +    truncated_deref_type
> +    truncated_gnu_deref_type
> +    truncated_skip
> +    truncated_bra
> +    truncated_entry_value
> +    truncated_gnu_entry_value
> +    truncated_parameter_ref
> +    truncated_const_type_size
> +    truncated_const_type_payload
> +    truncated_gnu_const_type_size
> +    truncated_gnu_const_type_payload
> +} {
> +    with_test_prefix $var {
There exists a "foreach_with_prefix", that does basically what you did 
here.
> +	gdb_test "print $var" \
> +	    ".*DWARF expression error: ran off end of buffer reading .*"
> +    }
> +}


-- 
Cheers,
Guinevere Larsen
it/its
she/her (deprecated)

Re: [PATCH][PR gdb/34239][PR gdb/34299] gdb: Check bounds before reading DWARF expression operands

[Firmiana]

[-- Attachment #1: Type: text/plain, Size: 27273 bytes --]

Hi,

Thanks for the review and for testing the patch.

Before preparing v2, I would like to respond to a few points.

About the safe_read_* helper implementation: yes, your understanding is
right.  The helper saves the original pointer, advances through
safe_skip_bytes to validate that the requested byte range is available,
and only then calls extract_*_integer on the saved pointer.  I agree that
this was not obvious enough, so I will make the helper comments and/or
implementation clearer in v2.

On the question of reading into `uoffset` and then assigning to the real
destination: that was mainly done to mirror the nearby
safe_read_uleb128/safe_read_sleb128 helper style, which uses fixed output
types and output pointer parameters.  Based on your comment and the other
review, I plan to change the new fixed-width helpers to use references,
probably templated, so many call sites can read directly into `result`,
`this->m_len`, or the relevant field.

For DW_OP_deref_size / DW_OP_deref_type: the v1 code is intended to be
equivalent to:

  addr_size = *op_ptr;
  op_ptr++;

but with a bounds check first.  safe_read_unsigned_integer keeps the
original pointer for extract_unsigned_integer and uses safe_skip_bytes to
validate and compute the next pointer.  Since this was confusing, I will
rewrite the branch more explicitly in v2.

I will also switch the testcase loop to foreach_with_prefix.

Thanks,
Jielun


On Wed, Jun 24, 2026 at 2:56 AM Guinevere Larsen <guinevere@redhat.com>
wrote:

> On 6/18/26 3:01 PM, Jielun Wu wrote:
> > Some DWARF expression opcodes read fixed-width operands or block payloads
> > from the expression buffer before checking that the bytes are available.
> > With a malformed expression, this can read past the end of the expression
> > buffer.
> >
> > Add helpers that validate fixed-width integer reads and block payload
> > skips, and use them in dwarf_expr_context::execute_stack_op.  Also guard
> > the nested DW_OP_entry_value parser before reading the following opcode.
> >
> > Add a DWARF assembler test that exercises truncated operands for these
> > opcodes.
> >
> > Tested on x86_64-linux.
> >
> > Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=34239
> > Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=34299
> > ---
>
> Hi! Thanks for working on this!
>
> I'm not too familiar with the dwarf parser, so I can only do a
> superficial review. Most of my comments are questions and nitpicks, I
> don't think you necessarily need to send a new version just from this
> review.
>
> I have tested this and see no regressions, so feel free to add my test
> tag to the git trailers
>
> Tested-By: Guinevere Larsen <guinevere@redhat.com>
>
> >   gdb/dwarf2/expr.c                             | 166 +++++++++------
> >   .../gdb.dwarf2/dw2-bad-dwarf-expr-bounds.exp  | 190 ++++++++++++++++++
> >   2 files changed, 300 insertions(+), 56 deletions(-)
> >   create mode 100644
> gdb/testsuite/gdb.dwarf2/dw2-bad-dwarf-expr-bounds.exp
> >
> > diff --git a/gdb/dwarf2/expr.c b/gdb/dwarf2/expr.c
> > index c71d4725b03..ead918388a4 100644
> > --- a/gdb/dwarf2/expr.c
> > +++ b/gdb/dwarf2/expr.c
> > @@ -1370,7 +1370,45 @@ safe_skip_leb128 (const gdb_byte *buf, const
> gdb_byte *buf_end)
> >       error (_("DWARF expression error: ran off end of buffer reading
> leb128 value"));
> >     return buf;
> >   }
> > -
> > +
> > +/* Helper to skip BYTES bytes or throw an error.  */
> > +
> > +static const gdb_byte *
> > +safe_skip_bytes (const gdb_byte *buf, const gdb_byte *buf_end,
> > +              ULONGEST bytes)
> > +{
> > +  if (buf > buf_end || bytes > (ULONGEST) (buf_end - buf))
> > +    error (_("DWARF expression error: ran off end of buffer reading
> bytes"));
> > +  return buf + bytes;
> > +}
> > +
> > +/* Helper to read a fixed-width unsigned integer or throw an error.  */
> > +
> > +static const gdb_byte *
> > +safe_read_unsigned_integer (const gdb_byte *buf, const gdb_byte
> *buf_end,
> > +                         int len, bfd_endian byte_order, uint64_t *r)
> > +{
> > +  gdb_assert (len >= 0);
> > +
> > +  const gdb_byte *data = buf;
> > +  buf = safe_skip_bytes (buf, buf_end, len);
> > +  *r = extract_unsigned_integer (data, len, byte_order);
> > +  return buf;
>
> Oh, ok, it took me a while of thinking but I think I now understand why
> you wrote it like this.
>
> You advance the buffer first to use that as a check that it is possible
> to read that many bytes from the buffer, instead of needing to double up
> on the if condition, right?
>
> If so, I think it would be nice to have a comment explaining it, because
> I went through 2 incorrect explanations before reaching that conclusion.
>
> > +}
> > +
> > +/* Helper to read a fixed-width signed integer or throw an error.  */
> > +
> > +static const gdb_byte *
> > +safe_read_signed_integer (const gdb_byte *buf, const gdb_byte *buf_end,
> > +                       int len, bfd_endian byte_order, int64_t *r)
> > +{
> > +  gdb_assert (len >= 0);
> > +
> > +  const gdb_byte *data = buf;
> > +  buf = safe_skip_bytes (buf, buf_end, len);
> > +  *r = extract_signed_integer (data, len, byte_order);
> > +  return buf;
> Similar comment as above
> > +}
> >
> >   /* Check that the current operator is either at the end of an
> >      expression, or that it is followed by a composition operator or by
> > @@ -1478,7 +1516,7 @@ dwarf_block_to_dwarf_reg_deref
> (gdb::array_view<const gdb_byte> block,
> >         if (buf == NULL)
> >       return -1;
> >         if ((int) dwarf_reg != dwarf_reg)
> > -       return -1;
> > +     return -1;
> >       }
> >     else
> >       return -1;
> > @@ -1488,6 +1526,8 @@ dwarf_block_to_dwarf_reg_deref
> (gdb::array_view<const gdb_byte> block,
> >       return -1;
> >     if (offset != 0)
> >       return -1;
> > +  if (buf >= buf_end)
> > +    return -1;
> >
> >     if (*buf == DW_OP_deref)
> >       {
> > @@ -1498,7 +1538,7 @@ dwarf_block_to_dwarf_reg_deref
> (gdb::array_view<const gdb_byte> block,
> >       {
> >         buf++;
> >         if (buf >= buf_end)
> > -       return -1;
> > +     return -1;
> >         *deref_size_return = *buf++;
> >       }
> >     else
> > @@ -1688,9 +1728,10 @@ dwarf_expr_context::execute_stack_op
> (gdb::array_view<const gdb_byte> expr)
> >         break;
> >
> >       case DW_OP_addr:
> > -       result = extract_unsigned_integer (op_ptr,
> > -                                          this->m_addr_size,
> byte_order);
> > -       op_ptr += this->m_addr_size;
> > +       op_ptr = safe_read_unsigned_integer (op_ptr, op_end,
> > +                                            this->m_addr_size,
> > +                                            byte_order, &uoffset);
>
> Why not just send "&result" as the parameter here, instead of &uoffset
>
> Same question for all the occurrences below
>
> > +       result = uoffset;
> >         /* Some versions of GCC emit DW_OP_addr before
> >            DW_OP_GNU_push_tls_address.  In this case the value is an
> >            index, not an address.  We don't support things like
> > @@ -1723,44 +1764,52 @@ dwarf_expr_context::execute_stack_op
> (gdb::array_view<const gdb_byte> expr)
> >         break;
> >
> >       case DW_OP_const1u:
> > -       result = extract_unsigned_integer (op_ptr, 1, byte_order);
> > +       op_ptr = safe_read_unsigned_integer (op_ptr, op_end, 1,
> > +                                            byte_order, &uoffset);
> > +       result = uoffset;
> >         result_val = value_from_ulongest (address_type, result);
> > -       op_ptr += 1;
> >         break;
> >       case DW_OP_const1s:
> > -       result = extract_signed_integer (op_ptr, 1, byte_order);
> > +       op_ptr = safe_read_signed_integer (op_ptr, op_end, 1,
> > +                                          byte_order, &offset);
> > +       result = offset;
> >         result_val = value_from_ulongest (address_type, result);
> > -       op_ptr += 1;
> >         break;
> >       case DW_OP_const2u:
> > -       result = extract_unsigned_integer (op_ptr, 2, byte_order);
> > +       op_ptr = safe_read_unsigned_integer (op_ptr, op_end, 2,
> > +                                            byte_order, &uoffset);
> > +       result = uoffset;
> >         result_val = value_from_ulongest (address_type, result);
> > -       op_ptr += 2;
> >         break;
> >       case DW_OP_const2s:
> > -       result = extract_signed_integer (op_ptr, 2, byte_order);
> > +       op_ptr = safe_read_signed_integer (op_ptr, op_end, 2,
> > +                                          byte_order, &offset);
> > +       result = offset;
> >         result_val = value_from_ulongest (address_type, result);
> > -       op_ptr += 2;
> >         break;
> >       case DW_OP_const4u:
> > -       result = extract_unsigned_integer (op_ptr, 4, byte_order);
> > +       op_ptr = safe_read_unsigned_integer (op_ptr, op_end, 4,
> > +                                            byte_order, &uoffset);
> > +       result = uoffset;
> >         result_val = value_from_ulongest (address_type, result);
> > -       op_ptr += 4;
> >         break;
> >       case DW_OP_const4s:
> > -       result = extract_signed_integer (op_ptr, 4, byte_order);
> > +       op_ptr = safe_read_signed_integer (op_ptr, op_end, 4,
> > +                                          byte_order, &offset);
> > +       result = offset;
> >         result_val = value_from_ulongest (address_type, result);
> > -       op_ptr += 4;
> >         break;
> >       case DW_OP_const8u:
> > -       result = extract_unsigned_integer (op_ptr, 8, byte_order);
> > +       op_ptr = safe_read_unsigned_integer (op_ptr, op_end, 8,
> > +                                            byte_order, &uoffset);
> > +       result = uoffset;
> >         result_val = value_from_ulongest (address_type, result);
> > -       op_ptr += 8;
> >         break;
> >       case DW_OP_const8s:
> > -       result = extract_signed_integer (op_ptr, 8, byte_order);
> > +       op_ptr = safe_read_signed_integer (op_ptr, op_end, 8,
> > +                                          byte_order, &offset);
> > +       result = offset;
> >         result_val = value_from_ulongest (address_type, result);
> > -       op_ptr += 8;
> >         break;
> >       case DW_OP_constu:
> >         op_ptr = safe_read_uleb128 (op_ptr, op_end, &uoffset);
> > @@ -1836,12 +1885,10 @@ dwarf_expr_context::execute_stack_op
> (gdb::array_view<const gdb_byte> expr)
> >           uint64_t len;
> >
> >           op_ptr = safe_read_uleb128 (op_ptr, op_end, &len);
> > -         if (op_ptr + len > op_end)
> > -           error (_("DW_OP_implicit_value: too few bytes available."));
> >           this->m_len = len;
> >           this->m_data = op_ptr;
> >           this->m_location = DWARF_VALUE_LITERAL;
> > -         op_ptr += len;
> > +         op_ptr = safe_skip_bytes (op_ptr, op_end, len);
> >           dwarf_expr_require_composition (op_ptr, op_end,
> >                                           "DW_OP_implicit_value");
> >         }
> > @@ -1861,9 +1908,10 @@ dwarf_expr_context::execute_stack_op
> (gdb::array_view<const gdb_byte> expr)
> >           int ref_addr_size = this->m_per_cu->ref_addr_size ();
> >
> >           /* The referred-to DIE of sect_offset kind.  */
> > -         this->m_len = extract_unsigned_integer (op_ptr, ref_addr_size,
> > -                                               byte_order);
> > -         op_ptr += ref_addr_size;
> > +         op_ptr = safe_read_unsigned_integer (op_ptr, op_end,
> > +                                              ref_addr_size, byte_order,
> > +                                              &uoffset);
> > +         this->m_len = uoffset;
> Again, why not send "&this->m_len" ?
> >
> >           /* The byte offset into the data.  */
> >           op_ptr = safe_read_sleb128 (op_ptr, op_end, &len);
> > @@ -1972,7 +2020,9 @@ dwarf_expr_context::execute_stack_op
> (gdb::array_view<const gdb_byte> expr)
> >         goto no_push;
> >
> >       case DW_OP_pick:
> > -       offset = *op_ptr++;
> > +       op_ptr = safe_read_unsigned_integer (op_ptr, op_end, 1,
> > +                                            byte_order, &uoffset);
> > +       offset = uoffset;
> >         result_val = fetch (offset);
> >         in_stack_memory = fetch_in_stack_memory (offset);
> >         break;
> > @@ -2016,7 +2066,13 @@ dwarf_expr_context::execute_stack_op
> (gdb::array_view<const gdb_byte> expr)
> >       case DW_OP_deref_type:
> >       case DW_OP_GNU_deref_type:
> >         {
> > -         int addr_size = (op == DW_OP_deref ? this->m_addr_size :
> *op_ptr++);
> > +         int addr_size = this->m_addr_size;
> > +         if (op != DW_OP_deref)
> > +           {
> > +             op_ptr = safe_read_unsigned_integer (op_ptr, op_end, 1,
> > +                                                  byte_order, &uoffset);
> > +             addr_size = uoffset;
>
> Maybe I'm missing something, but this seems like an incorrect change?
>
> It seems like you should call safe_skip_bytes instead of
> read_unsigned_integer, and assign addr_size to *op_ptr?
>
> > +           }
> >           CORE_ADDR addr = fetch_address (0);
> >           struct type *type;
> >
> > @@ -2249,8 +2305,8 @@ dwarf_expr_context::execute_stack_op
> (gdb::array_view<const gdb_byte> expr)
> >         break;
> >
> >       case DW_OP_skip:
> > -       offset = extract_signed_integer (op_ptr, 2, byte_order);
> > -       op_ptr += 2;
> > +       op_ptr = safe_read_signed_integer (op_ptr, op_end, 2, byte_order,
> > +                                          &offset);
> >         op_ptr += offset;
> This should also use safe_skip_bytes
> >         goto no_push;
> >
> > @@ -2258,8 +2314,8 @@ dwarf_expr_context::execute_stack_op
> (gdb::array_view<const gdb_byte> expr)
> >         {
> >           struct value *val;
> >
> > -         offset = extract_signed_integer (op_ptr, 2, byte_order);
> > -         op_ptr += 2;
> > +         op_ptr = safe_read_signed_integer (op_ptr, op_end, 2,
> byte_order,
> > +                                            &offset);
> >           val = fetch (0);
> >           dwarf_require_integral (val->type ());
> >           if (value_as_long (val) != 0)
> > @@ -2313,18 +2369,18 @@ dwarf_expr_context::execute_stack_op
> (gdb::array_view<const gdb_byte> expr)
> >
> >       case DW_OP_call2:
> >         {
> > -         cu_offset cu_off
> > -           = (cu_offset) extract_unsigned_integer (op_ptr, 2,
> byte_order);
> > -         op_ptr += 2;
> > +         op_ptr = safe_read_unsigned_integer (op_ptr, op_end, 2,
> > +                                              byte_order, &uoffset);
> > +         cu_offset cu_off = (cu_offset) uoffset;
> >           this->dwarf_call (cu_off);
> >         }
> >         goto no_push;
> >
> >       case DW_OP_call4:
> >         {
> > -         cu_offset cu_off
> > -           = (cu_offset) extract_unsigned_integer (op_ptr, 4,
> byte_order);
> > -         op_ptr += 4;
> > +         op_ptr = safe_read_unsigned_integer (op_ptr, op_end, 4,
> > +                                              byte_order, &uoffset);
> > +         cu_offset cu_off = (cu_offset) uoffset;
> >           this->dwarf_call (cu_off);
> >         }
> >         goto no_push;
> > @@ -2334,11 +2390,10 @@ dwarf_expr_context::execute_stack_op
> (gdb::array_view<const gdb_byte> expr)
> >           ensure_have_per_cu (this->m_per_cu,
> "DW_OP_GNU_variable_value");
> >           int ref_addr_size = this->m_per_cu->ref_addr_size ();
> >
> > -         sect_offset sect_off
> > -           = (sect_offset) extract_unsigned_integer (op_ptr,
> > -                                                     ref_addr_size,
> > -                                                     byte_order);
> > -         op_ptr += ref_addr_size;
> > +         op_ptr = safe_read_unsigned_integer (op_ptr, op_end,
> > +                                              ref_addr_size, byte_order,
> > +                                              &uoffset);
> > +         sect_offset sect_off = (sect_offset) uoffset;
> >           result_val = sect_variable_value (sect_off, this->m_per_cu,
> >                                             this->m_per_objfile);
> >           result_val = value_cast (address_type, result_val);
> > @@ -2353,15 +2408,13 @@ dwarf_expr_context::execute_stack_op
> (gdb::array_view<const gdb_byte> expr)
> >           union call_site_parameter_u kind_u;
> >
> >           op_ptr = safe_read_uleb128 (op_ptr, op_end, &len);
> > -         if (op_ptr + len > op_end)
> > -           error (_("DW_OP_entry_value: too few bytes available."));
> > +         const gdb_byte *expr_ptr = op_ptr;
> > +         op_ptr = safe_skip_bytes (op_ptr, op_end, len);
> >
> > -         auto entry_value_expr = gdb::make_array_view (op_ptr, len);
> > +         auto entry_value_expr = gdb::make_array_view (expr_ptr, len);
> >           kind_u.dwarf_reg = dwarf_block_to_dwarf_reg (entry_value_expr);
> >           if (kind_u.dwarf_reg != -1)
> >             {
> > -             op_ptr += len;
> > -
> >               if (trivial_entry_value (this->m_frame))
> >                 {
> >                   /* We can assume that DW_OP_entry_value (expr) == expr.
> > @@ -2384,7 +2437,6 @@ dwarf_expr_context::execute_stack_op
> (gdb::array_view<const gdb_byte> expr)
> >             {
> >               if (deref_size == -1)
> >                 deref_size = this->m_addr_size;
> > -             op_ptr += len;
> >
> >               if (trivial_entry_value (this->m_frame))
> >                 {
> > @@ -2410,9 +2462,9 @@ dwarf_expr_context::execute_stack_op
> (gdb::array_view<const gdb_byte> expr)
> >         {
> >           union call_site_parameter_u kind_u;
> >
> > -         kind_u.param_cu_off
> > -           = (cu_offset) extract_unsigned_integer (op_ptr, 4,
> byte_order);
> > -         op_ptr += 4;
> > +         op_ptr = safe_read_unsigned_integer (op_ptr, op_end, 4,
> > +                                              byte_order, &uoffset);
> > +         kind_u.param_cu_off = (cu_offset) uoffset;
> >           this->push_dwarf_reg_entry_value
> (CALL_SITE_PARAMETER_PARAM_OFFSET,
> >                                             kind_u,
> >                                             -1 /* deref_size */);
> > @@ -2429,9 +2481,11 @@ dwarf_expr_context::execute_stack_op
> (gdb::array_view<const gdb_byte> expr)
> >           op_ptr = safe_read_uleb128 (op_ptr, op_end, &uoffset);
> >           cu_offset type_die_cu_off = (cu_offset) uoffset;
> >
> > -         n = *op_ptr++;
> > +         op_ptr = safe_read_unsigned_integer (op_ptr, op_end, 1,
> byte_order,
> > +                                              &uoffset);
> > +         n = uoffset;
> >           data = op_ptr;
> > -         op_ptr += n;
> > +         op_ptr = safe_skip_bytes (op_ptr, op_end, n);
> >
> >           type = get_base_type (type_die_cu_off);
> >
> > diff --git a/gdb/testsuite/gdb.dwarf2/dw2-bad-dwarf-expr-bounds.exp
> b/gdb/testsuite/gdb.dwarf2/dw2-bad-dwarf-expr-bounds.exp
> > new file mode 100644
> > index 00000000000..7f5fa22388d
> > --- /dev/null
> > +++ b/gdb/testsuite/gdb.dwarf2/dw2-bad-dwarf-expr-bounds.exp
> > @@ -0,0 +1,190 @@
> > +# Copyright 2026 Free Software Foundation, Inc.
> > +
> > +# This program is free software; you can redistribute it and/or modify
> > +# it under the terms of the GNU General Public License as published by
> > +# the Free Software Foundation; either version 3 of the License, or
> > +# (at your option) any later version.
> > +#
> > +# This program is distributed in the hope that it will be useful,
> > +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > +# GNU General Public License for more details.
> > +#
> > +# You should have received a copy of the GNU General Public License
> > +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> > +
> > +# Test malformed DWARF expressions whose opcodes have truncated
> operands.
> > +
> > +load_lib dwarf.exp
> > +
> > +require dwarf2_support
> > +
> > +standard_testfile main.c -dw.S
> > +
> > +set asm_file [standard_output_file $srcfile2]
> > +Dwarf::assemble $asm_file {
> > +    global srcfile
> > +
> > +    declare_labels int_label
> > +
> > +    cu {label cu_label} {
> > +     DW_TAG_compile_unit {
> > +         DW_AT_name $srcfile
> > +         DW_AT_language @DW_LANG_C
> > +     } {
> > +         int_label: DW_TAG_base_type {
> > +             DW_AT_name "int"
> > +             DW_AT_encoding @DW_ATE_signed
> > +             DW_AT_byte_size 4 DW_FORM_sdata
> > +         }
> > +
> > +         foreach {var op} {
> > +             truncated_addr DW_OP_addr
> > +             truncated_const1u DW_OP_const1u
> > +             truncated_const1s DW_OP_const1s
> > +             truncated_const2u DW_OP_const2u
> > +             truncated_const2s DW_OP_const2s
> > +             truncated_const4u DW_OP_const4u
> > +             truncated_const4s DW_OP_const4s
> > +             truncated_const8u DW_OP_const8u
> > +             truncated_const8s DW_OP_const8s
> > +             truncated_implicit_pointer DW_OP_implicit_pointer
> > +             truncated_gnu_implicit_pointer DW_OP_GNU_implicit_pointer
> > +             truncated_pick DW_OP_pick
> > +             truncated_deref_size DW_OP_deref_size
> > +             truncated_deref_type DW_OP_deref_type
> > +             truncated_gnu_deref_type DW_OP_GNU_deref_type
> > +             truncated_skip DW_OP_skip
> > +             truncated_bra DW_OP_bra
> > +             truncated_parameter_ref DW_OP_GNU_parameter_ref
> > +         } {
> > +             DW_TAG_variable {
> > +                 DW_AT_name $var
> > +                 DW_AT_type :$int_label
> > +                 DW_AT_location {
> > +                     _op .byte $Dwarf::_constants($op) $op
> > +                 } SPECIAL_expr
> > +             }
> > +         }
> > +
> > +         DW_TAG_variable {
> > +             DW_AT_name "truncated_implicit_value"
> > +             DW_AT_type :$int_label
> > +             DW_AT_location {
> > +                 _op .byte $Dwarf::_constants(DW_OP_implicit_value) \
> > +                     DW_OP_implicit_value
> > +                 _op .uleb128 4 "implicit value length"
> > +                 _op .2byte 0 "truncated implicit value bytes"
> > +             } SPECIAL_expr
> > +         }
> > +
> > +         DW_TAG_variable {
> > +             DW_AT_name "truncated_entry_value"
> > +             DW_AT_type :$int_label
> > +             DW_AT_location {
> > +                 _op .byte $Dwarf::_constants(DW_OP_entry_value) \
> > +                     DW_OP_entry_value
> > +                 _op .uleb128 4 "entry value expression length"
> > +                 _op .2byte 0 "truncated entry value expression"
> > +             } SPECIAL_expr
> > +         }
> > +
> > +         DW_TAG_variable {
> > +             DW_AT_name "truncated_gnu_entry_value"
> > +             DW_AT_type :$int_label
> > +             DW_AT_location {
> > +                 _op .byte $Dwarf::_constants(DW_OP_GNU_entry_value) \
> > +                     DW_OP_GNU_entry_value
> > +                 _op .uleb128 4 "entry value expression length"
> > +                 _op .2byte 0 "truncated entry value expression"
> > +             } SPECIAL_expr
> > +         }
> > +
> > +         DW_TAG_variable {
> > +             DW_AT_name "truncated_const_type_size"
> > +             DW_AT_type :$int_label
> > +             DW_AT_location {
> > +                 _op .byte $Dwarf::_constants(DW_OP_const_type) \
> > +                     DW_OP_const_type
> > +                 _op .uleb128 "$int_label - $cu_label" "type DIE offset"
> > +             } SPECIAL_expr
> > +         }
> > +
> > +         DW_TAG_variable {
> > +             DW_AT_name "truncated_const_type_payload"
> > +             DW_AT_type :$int_label
> > +             DW_AT_location {
> > +                 _op .byte $Dwarf::_constants(DW_OP_const_type) \
> > +                     DW_OP_const_type
> > +                 _op .uleb128 "$int_label - $cu_label" "type DIE offset"
> > +                 _op .byte 4 "constant block length"
> > +                 _op .2byte 0 "truncated constant block"
> > +             } SPECIAL_expr
> > +         }
> > +
> > +         DW_TAG_variable {
> > +             DW_AT_name "truncated_gnu_const_type_size"
> > +             DW_AT_type :$int_label
> > +             DW_AT_location {
> > +                 _op .byte $Dwarf::_constants(DW_OP_GNU_const_type) \
> > +                     DW_OP_GNU_const_type
> > +                 _op .uleb128 "$int_label - $cu_label" "type DIE offset"
> > +             } SPECIAL_expr
> > +         }
> > +
> > +         DW_TAG_variable {
> > +             DW_AT_name "truncated_gnu_const_type_payload"
> > +             DW_AT_type :$int_label
> > +             DW_AT_location {
> > +                 _op .byte $Dwarf::_constants(DW_OP_GNU_const_type) \
> > +                     DW_OP_GNU_const_type
> > +                 _op .uleb128 "$int_label - $cu_label" "type DIE offset"
> > +                 _op .byte 4 "constant block length"
> > +                 _op .2byte 0 "truncated constant block"
> > +             } SPECIAL_expr
> > +         }
> > +     }
> > +    }
> > +}
> > +
> > +if {[prepare_for_testing "failed to prepare" ${testfile} \
> > +      [list $srcfile $asm_file] nodebug]} {
> > +    return
> > +}
> > +
> > +if {![runto_main]} {
> > +    return
> > +}
> > +
> > +foreach var {
> > +    truncated_addr
> > +    truncated_const1u
> > +    truncated_const1s
> > +    truncated_const2u
> > +    truncated_const2s
> > +    truncated_const4u
> > +    truncated_const4s
> > +    truncated_const8u
> > +    truncated_const8s
> > +    truncated_implicit_value
> > +    truncated_implicit_pointer
> > +    truncated_gnu_implicit_pointer
> > +    truncated_pick
> > +    truncated_deref_size
> > +    truncated_deref_type
> > +    truncated_gnu_deref_type
> > +    truncated_skip
> > +    truncated_bra
> > +    truncated_entry_value
> > +    truncated_gnu_entry_value
> > +    truncated_parameter_ref
> > +    truncated_const_type_size
> > +    truncated_const_type_payload
> > +    truncated_gnu_const_type_size
> > +    truncated_gnu_const_type_payload
> > +} {
> > +    with_test_prefix $var {
> There exists a "foreach_with_prefix", that does basically what you did
> here.
> > +     gdb_test "print $var" \
> > +         ".*DWARF expression error: ran off end of buffer reading .*"
> > +    }
> > +}
>
>
> --
> Cheers,
> Guinevere Larsen
> it/its
> she/her (deprecated)
>
>

[-- Attachment #2: Type: text/html, Size: 34380 bytes --]

Re: [PATCH][PR gdb/34239][PR gdb/34299] gdb: Check bounds before reading DWARF expression operands

[Simon Marchi]

On 2026-06-26 04:31, Firmiana wrote:
> 
> Hi,
> 
> Thanks for the review and for testing the patch.
> 
> Before preparing v2, I would like to respond to a few points.
> 
> About the safe_read_* helper implementation: yes, your understanding is
> right.  The helper saves the original pointer, advances through
> safe_skip_bytes to validate that the requested byte range is available,
> and only then calls extract_*_integer on the saved pointer.  I agree that
> this was not obvious enough, so I will make the helper comments and/or
> implementation clearer in v2.
> 
> On the question of reading into `uoffset` and then assigning to the real
> destination: that was mainly done to mirror the nearby
> safe_read_uleb128/safe_read_sleb128 helper style, which uses fixed output
> types and output pointer parameters.  Based on your comment and the other
> review, I plan to change the new fixed-width helpers to use references,
> probably templated, so many call sites can read directly into `result`,
> `this->m_len`, or the relevant field.

The GDB code used to be C, and is now organically evolving into C++.
When you find some opportunities to use some C++ constructs to make the
code better, feel free to do so.

> For DW_OP_deref_size / DW_OP_deref_type: the v1 code is intended to be
> equivalent to:
> 
>   addr_size = *op_ptr;
>   op_ptr++;
> 
> but with a bounds check first.  safe_read_unsigned_integer keeps the
> original pointer for extract_unsigned_integer and uses safe_skip_bytes to
> validate and compute the next pointer.  Since this was confusing, I will
> rewrite the branch more explicitly in v2.
> 
> I will also switch the testcase loop to foreach_with_prefix.

Good idea.

Simon

Re: [PATCH][PR gdb/34239][PR gdb/34299] gdb: Check bounds before reading DWARF expression operands

[Simon Marchi]

On 2026-06-18 14:01, Jielun Wu wrote:
> Some DWARF expression opcodes read fixed-width operands or block payloads
> from the expression buffer before checking that the bytes are available.
> With a malformed expression, this can read past the end of the expression
> buffer.
> 
> Add helpers that validate fixed-width integer reads and block payload
> skips, and use them in dwarf_expr_context::execute_stack_op.  Also guard
> the nested DW_OP_entry_value parser before reading the following opcode.
> 
> Add a DWARF assembler test that exercises truncated operands for these
> opcodes.
> 
> Tested on x86_64-linux.
> 
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=34239
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=34299

I'll look at this patch, but before anything else, do you have a
copyright assignment on file with the FSF?  This is required in order to
submit a non-trivial amount of code to GDB.  See here for details:

https://sourceware.org/gdb/wiki/ContributionChecklist#FSF_copyright_Assignment

Simon

Re: [PATCH][PR gdb/34239][PR gdb/34299] gdb: Check bounds before reading DWARF expression operands

[Simon Marchi]

On 2026-06-18 14:01, Jielun Wu wrote:
> Some DWARF expression opcodes read fixed-width operands or block payloads
> from the expression buffer before checking that the bytes are available.
> With a malformed expression, this can read past the end of the expression
> buffer.
> 
> Add helpers that validate fixed-width integer reads and block payload
> skips, and use them in dwarf_expr_context::execute_stack_op.  Also guard
> the nested DW_OP_entry_value parser before reading the following opcode.
> 
> Add a DWARF assembler test that exercises truncated operands for these
> opcodes.
> 
> Tested on x86_64-linux.

I noted a few comments below.

> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=34239
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=34299
> ---
>  gdb/dwarf2/expr.c                             | 166 +++++++++------
>  .../gdb.dwarf2/dw2-bad-dwarf-expr-bounds.exp  | 190 ++++++++++++++++++
>  2 files changed, 300 insertions(+), 56 deletions(-)
>  create mode 100644 gdb/testsuite/gdb.dwarf2/dw2-bad-dwarf-expr-bounds.exp
> 
> diff --git a/gdb/dwarf2/expr.c b/gdb/dwarf2/expr.c
> index c71d4725b03..ead918388a4 100644
> --- a/gdb/dwarf2/expr.c
> +++ b/gdb/dwarf2/expr.c
> @@ -1370,7 +1370,45 @@ safe_skip_leb128 (const gdb_byte *buf, const gdb_byte *buf_end)
>      error (_("DWARF expression error: ran off end of buffer reading leb128 value"));
>    return buf;
>  }
> -\f
> +
> +/* Helper to skip BYTES bytes or throw an error.  */

Please precise the comment, "throw an error" in what situation?

> +
> +static const gdb_byte *
> +safe_skip_bytes (const gdb_byte *buf, const gdb_byte *buf_end,
> +		 ULONGEST bytes)
> +{
> +  if (buf > buf_end || bytes > (ULONGEST) (buf_end - buf))

I would make the "buf > buf_end" condition an assert:

  gdb_assert (buf <= buf_end);

I think that if "buf > buf_end" happens, something would have went wrong
earlier.

> +    error (_("DWARF expression error: ran off end of buffer reading bytes"));
> +  return buf + bytes;
> +}
> +
> +/* Helper to read a fixed-width unsigned integer or throw an error.  */
> +
> +static const gdb_byte *
> +safe_read_unsigned_integer (const gdb_byte *buf, const gdb_byte *buf_end,
> +			    int len, bfd_endian byte_order, uint64_t *r)

Make `r` a reference.

You appear to use this function most of the time like this:

	  op_ptr = safe_read_unsigned_integer (op_ptr, op_end, 1,
					       byte_order, &uoffset);
	  result = uoffset;


where `result` is a ULONGEST and uoffset a uint64_t.  If you made `r` a
pointer or reference to ULONGEST, wouldn't it make things simpler?  You
could just do:

	  op_ptr = safe_read_unsigned_integer (op_ptr, op_end, 1,
					       byte_order, result);

... and avoid the subsequent assignment.

But it is sometimes used to read into other types, like cu_offset:

	    op_ptr = safe_read_unsigned_integer (op_ptr, op_end, 4,
						 byte_order, &uoffset);
	    kind_u.param_cu_off = (cu_offset) uoffset;

So we could perhaps make the function templated, like this?

  template <typename T>
  static const gdb_byte *
  safe_read_unsigned_integer (const gdb_byte *buf, const gdb_byte *buf_end,
  			    int len, bfd_endian byte_order, T &r)
  {
    gdb_assert (len >= 0);
    gdb_assert (len <= sizeof (T));
  
    const gdb_byte *data = buf;
    buf = safe_skip_bytes (buf, buf_end, len);
    r = static_cast<T> (extract_unsigned_integer (data, len, byte_order));
    return buf;
  }

Then you'll be able to read the integer into whatever destination
integer type directly:

	op_ptr = safe_read_unsigned_integer (op_ptr, op_end, 4, byte_order,
						 &kind_u.param_cu_off);

> +{
> +  gdb_assert (len >= 0);

I would add an assert at len is <= the size of the destination type, as
shown above in my example.

> +  const gdb_byte *data = buf;
> +  buf = safe_skip_bytes (buf, buf_end, len);
> +  *r = extract_unsigned_integer (data, len, byte_order);
> +  return buf;
> +}
> +
> +/* Helper to read a fixed-width signed integer or throw an error.  */
> +
> +static const gdb_byte *
> +safe_read_signed_integer (const gdb_byte *buf, const gdb_byte *buf_end,
> +			  int len, bfd_endian byte_order, int64_t *r)
> +{
> +  gdb_assert (len >= 0);
> +
> +  const gdb_byte *data = buf;
> +  buf = safe_skip_bytes (buf, buf_end, len);
> +  *r = extract_signed_integer (data, len, byte_order);
> +  return buf;

Same comments as above.

>  /* Check that the current operator is either at the end of an
>     expression, or that it is followed by a composition operator or by
> @@ -1478,7 +1516,7 @@ dwarf_block_to_dwarf_reg_deref (gdb::array_view<const gdb_byte> block,
>        if (buf == NULL)
>  	return -1;
>        if ((int) dwarf_reg != dwarf_reg)
> -       return -1;
> +	return -1;

This is an unrelated whitespace change, I will push an obvious patch to
fix it.

>      }
>    else
>      return -1;
> @@ -1488,6 +1526,8 @@ dwarf_block_to_dwarf_reg_deref (gdb::array_view<const gdb_byte> block,
>      return -1;
>    if (offset != 0)
>      return -1;
> +  if (buf >= buf_end)
> +    return -1;

Can this really happen?  It seems to me like gdb_read_sleb128 guarantees
that it will not read past buf_end.  So it's perhaps possible that `buf
== buf_end`, but not `buf > buf_end`.  If so, I would do:

  gdb_assert (buf <= buf_end);

  if (buf == buf_end)
    return -1;

It should be possible to write "selftest" for
dwarf_block_to_dwarf_reg_deref (and dwarf_block_to_dwarf_reg), they look
like pure functions.  Can you add that?  Look at dwarf2/loc.c for
example to see how selftests are registered, we would do the same in
dwarf2/expr.c.

>    if (*buf == DW_OP_deref)
>      {
> @@ -1498,7 +1538,7 @@ dwarf_block_to_dwarf_reg_deref (gdb::array_view<const gdb_byte> block,
>      {
>        buf++;
>        if (buf >= buf_end)
> -       return -1;
> +	return -1;

Same as above, I'll fix it in an obvious patch.

> @@ -2016,7 +2066,13 @@ dwarf_expr_context::execute_stack_op (gdb::array_view<const gdb_byte> expr)
>  	case DW_OP_deref_type:
>  	case DW_OP_GNU_deref_type:
>  	  {
> -	    int addr_size = (op == DW_OP_deref ? this->m_addr_size : *op_ptr++);
> +	    int addr_size = this->m_addr_size;
> +	    if (op != DW_OP_deref)
> +	      {
> +		op_ptr = safe_read_unsigned_integer (op_ptr, op_end, 1,
> +						     byte_order, &uoffset);
> +		addr_size = uoffset;
> +	      }

Please write it with the two branches like this:

	    int addr_size;

	    if (op == DW_OP_deref)
	       addr_size = = this->m_addr_size;
	    else
	      {
		op_ptr = safe_read_unsigned_integer (op_ptr, op_end, 1,
						     byte_order, &uoffset);
		addr_size = uoffset;
	      }

Add an empty line after the curly brace that closes the scope.

Simon

Re: [PATCH][PR gdb/34239][PR gdb/34299] gdb: Check bounds before reading DWARF expression operands

[Firmiana]

[-- Attachment #1: Type: text/plain, Size: 9822 bytes --]

Hi,

Thanks for the review. Given the amount of changes needed, I plan to
prepare a v2 that
supersedes this version. Before preparing v2, I would like to clarify a few
questions.

First, regarding copyright assignment: I do not currently have an FSF
copyright assignment on file.  I noticed the 2022 binutils announcement
that DCO signed contributions are accepted:

  https://sourceware.org/pipermail/binutils/2022-October/123680.html

and binutils/MAINTAINERS documents DCO as an alternative to FSF copyright
assignment.  Does this DCO path also apply to GDB patches touching gdb/?
If so, I will add:

  Signed-off-by: Jielun Wu <firmiana402@gmail.com>

to the v2 commit.  If GDB still requires FSF copyright assignment for
this patch, I am willing to start that process.

On the technical comments, I agree with most of them and plan to address
them in v2:

  - make the helper comments more precise;
  - change the `buf > buf_end` cases in safe_skip_bytes() to assertions;
  - switch the new fixed-width integer helpers to reference/template
    style, unless you prefer keeping them closer to the existing
    safe_read_uleb128/safe_read_sleb128 style;
  - add assertions that LEN fits in the destination type;
  - remove the unrelated whitespace-only changes;
  - change the dwarf_block_to_dwarf_reg_deref guard to the
    `gdb_assert (buf <= buf_end); if (buf == buf_end) return -1;`
    form;
  - add selftests for dwarf_block_to_dwarf_reg and
    dwarf_block_to_dwarf_reg_deref;
  - rewrite the DW_OP_deref* branch using the explicit if/else form you
    suggested.

For the helper style: in v1 I mirrored the nearby safe_read_uleb128 and
safe_read_sleb128 helpers, which use output pointer parameters and short
"or throw an error" comments.  But I agree that these new helpers can be
clearer, so unless you prefer otherwise I will use the reference/template
style you suggested for v2.

Thanks,
Jielun

On Fri, Jun 26, 2026 at 12:36 AM Simon Marchi <simark@simark.ca> wrote:

>
>
> On 2026-06-18 14:01, Jielun Wu wrote:
> > Some DWARF expression opcodes read fixed-width operands or block payloads
> > from the expression buffer before checking that the bytes are available.
> > With a malformed expression, this can read past the end of the expression
> > buffer.
> >
> > Add helpers that validate fixed-width integer reads and block payload
> > skips, and use them in dwarf_expr_context::execute_stack_op.  Also guard
> > the nested DW_OP_entry_value parser before reading the following opcode.
> >
> > Add a DWARF assembler test that exercises truncated operands for these
> > opcodes.
> >
> > Tested on x86_64-linux.
>
> I noted a few comments below.
>
> > Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=34239
> > Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=34299
> > ---
> >  gdb/dwarf2/expr.c                             | 166 +++++++++------
> >  .../gdb.dwarf2/dw2-bad-dwarf-expr-bounds.exp  | 190 ++++++++++++++++++
> >  2 files changed, 300 insertions(+), 56 deletions(-)
> >  create mode 100644
> gdb/testsuite/gdb.dwarf2/dw2-bad-dwarf-expr-bounds.exp
> >
> > diff --git a/gdb/dwarf2/expr.c b/gdb/dwarf2/expr.c
> > index c71d4725b03..ead918388a4 100644
> > --- a/gdb/dwarf2/expr.c
> > +++ b/gdb/dwarf2/expr.c
> > @@ -1370,7 +1370,45 @@ safe_skip_leb128 (const gdb_byte *buf, const
> gdb_byte *buf_end)
> >      error (_("DWARF expression error: ran off end of buffer reading
> leb128 value"));
> >    return buf;
> >  }
> > -
> > +
> > +/* Helper to skip BYTES bytes or throw an error.  */
>
> Please precise the comment, "throw an error" in what situation?
>
> > +
> > +static const gdb_byte *
> > +safe_skip_bytes (const gdb_byte *buf, const gdb_byte *buf_end,
> > +              ULONGEST bytes)
> > +{
> > +  if (buf > buf_end || bytes > (ULONGEST) (buf_end - buf))
>
> I would make the "buf > buf_end" condition an assert:
>
>   gdb_assert (buf <= buf_end);
>
> I think that if "buf > buf_end" happens, something would have went wrong
> earlier.
>
> > +    error (_("DWARF expression error: ran off end of buffer reading
> bytes"));
> > +  return buf + bytes;
> > +}
> > +
> > +/* Helper to read a fixed-width unsigned integer or throw an error.  */
> > +
> > +static const gdb_byte *
> > +safe_read_unsigned_integer (const gdb_byte *buf, const gdb_byte
> *buf_end,
> > +                         int len, bfd_endian byte_order, uint64_t *r)
>
> Make `r` a reference.
>
> You appear to use this function most of the time like this:
>
>           op_ptr = safe_read_unsigned_integer (op_ptr, op_end, 1,
>                                                byte_order, &uoffset);
>           result = uoffset;
>
>
> where `result` is a ULONGEST and uoffset a uint64_t.  If you made `r` a
> pointer or reference to ULONGEST, wouldn't it make things simpler?  You
> could just do:
>
>           op_ptr = safe_read_unsigned_integer (op_ptr, op_end, 1,
>                                                byte_order, result);
>
> ... and avoid the subsequent assignment.
>
> But it is sometimes used to read into other types, like cu_offset:
>
>             op_ptr = safe_read_unsigned_integer (op_ptr, op_end, 4,
>                                                  byte_order, &uoffset);
>             kind_u.param_cu_off = (cu_offset) uoffset;
>
> So we could perhaps make the function templated, like this?
>
>   template <typename T>
>   static const gdb_byte *
>   safe_read_unsigned_integer (const gdb_byte *buf, const gdb_byte *buf_end,
>                             int len, bfd_endian byte_order, T &r)
>   {
>     gdb_assert (len >= 0);
>     gdb_assert (len <= sizeof (T));
>
>     const gdb_byte *data = buf;
>     buf = safe_skip_bytes (buf, buf_end, len);
>     r = static_cast<T> (extract_unsigned_integer (data, len, byte_order));
>     return buf;
>   }
>
> Then you'll be able to read the integer into whatever destination
> integer type directly:
>
>         op_ptr = safe_read_unsigned_integer (op_ptr, op_end, 4, byte_order,
>                                                  &kind_u.param_cu_off);
>
> > +{
> > +  gdb_assert (len >= 0);
>
> I would add an assert at len is <= the size of the destination type, as
> shown above in my example.
>
> > +  const gdb_byte *data = buf;
> > +  buf = safe_skip_bytes (buf, buf_end, len);
> > +  *r = extract_unsigned_integer (data, len, byte_order);
> > +  return buf;
> > +}
> > +
> > +/* Helper to read a fixed-width signed integer or throw an error.  */
> > +
> > +static const gdb_byte *
> > +safe_read_signed_integer (const gdb_byte *buf, const gdb_byte *buf_end,
> > +                       int len, bfd_endian byte_order, int64_t *r)
> > +{
> > +  gdb_assert (len >= 0);
> > +
> > +  const gdb_byte *data = buf;
> > +  buf = safe_skip_bytes (buf, buf_end, len);
> > +  *r = extract_signed_integer (data, len, byte_order);
> > +  return buf;
>
> Same comments as above.
>
> >  /* Check that the current operator is either at the end of an
> >     expression, or that it is followed by a composition operator or by
> > @@ -1478,7 +1516,7 @@ dwarf_block_to_dwarf_reg_deref
> (gdb::array_view<const gdb_byte> block,
> >        if (buf == NULL)
> >       return -1;
> >        if ((int) dwarf_reg != dwarf_reg)
> > -       return -1;
> > +     return -1;
>
> This is an unrelated whitespace change, I will push an obvious patch to
> fix it.
>
> >      }
> >    else
> >      return -1;
> > @@ -1488,6 +1526,8 @@ dwarf_block_to_dwarf_reg_deref
> (gdb::array_view<const gdb_byte> block,
> >      return -1;
> >    if (offset != 0)
> >      return -1;
> > +  if (buf >= buf_end)
> > +    return -1;
>
> Can this really happen?  It seems to me like gdb_read_sleb128 guarantees
> that it will not read past buf_end.  So it's perhaps possible that `buf
> == buf_end`, but not `buf > buf_end`.  If so, I would do:
>
>   gdb_assert (buf <= buf_end);
>
>   if (buf == buf_end)
>     return -1;
>
> It should be possible to write "selftest" for
> dwarf_block_to_dwarf_reg_deref (and dwarf_block_to_dwarf_reg), they look
> like pure functions.  Can you add that?  Look at dwarf2/loc.c for
> example to see how selftests are registered, we would do the same in
> dwarf2/expr.c.
>
> >    if (*buf == DW_OP_deref)
> >      {
> > @@ -1498,7 +1538,7 @@ dwarf_block_to_dwarf_reg_deref
> (gdb::array_view<const gdb_byte> block,
> >      {
> >        buf++;
> >        if (buf >= buf_end)
> > -       return -1;
> > +     return -1;
>
> Same as above, I'll fix it in an obvious patch.
>
> > @@ -2016,7 +2066,13 @@ dwarf_expr_context::execute_stack_op
> (gdb::array_view<const gdb_byte> expr)
> >       case DW_OP_deref_type:
> >       case DW_OP_GNU_deref_type:
> >         {
> > -         int addr_size = (op == DW_OP_deref ? this->m_addr_size :
> *op_ptr++);
> > +         int addr_size = this->m_addr_size;
> > +         if (op != DW_OP_deref)
> > +           {
> > +             op_ptr = safe_read_unsigned_integer (op_ptr, op_end, 1,
> > +                                                  byte_order, &uoffset);
> > +             addr_size = uoffset;
> > +           }
>
> Please write it with the two branches like this:
>
>             int addr_size;
>
>             if (op == DW_OP_deref)
>                addr_size = = this->m_addr_size;
>             else
>               {
>                 op_ptr = safe_read_unsigned_integer (op_ptr, op_end, 1,
>                                                      byte_order, &uoffset);
>                 addr_size = uoffset;
>               }
>
> Add an empty line after the curly brace that closes the scope.
>
> Simon
>

[-- Attachment #2: Type: text/html, Size: 12090 bytes --]

Re: [PATCH][PR gdb/34239][PR gdb/34299] gdb: Check bounds before reading DWARF expression operands

[Simon Marchi]

On 2026-06-26 03:34, Firmiana wrote:
> Hi,
> 
> Thanks for the review. Given the amount of changes needed, I plan to prepare a v2 that
> supersedes this version. Before preparing v2, I would like to clarify a few questions.
> 
> First, regarding copyright assignment: I do not currently have an FSF
> copyright assignment on file.  I noticed the 2022 binutils announcement
> that DCO signed contributions are accepted:
> 
>   https://sourceware.org/pipermail/binutils/2022-October/123680.html <https://sourceware.org/pipermail/binutils/2022-October/123680.html>
> 
> and binutils/MAINTAINERS documents DCO as an alternative to FSF copyright
> assignment.  Does this DCO path also apply to GDB patches touching gdb/?
> If so, I will add:
> 
>   Signed-off-by: Jielun Wu <firmiana402@gmail.com <mailto:firmiana402@gmail.com>>
> 
> to the v2 commit.  If GDB still requires FSF copyright assignment for
> this patch, I am willing to start that process.

Thanks a lot.

No it doesn't apply to GDB.  There are ongoing discussions to maybe use
DCO for GDB too but we haven't reached a consensus yet.

> On the technical comments, I agree with most of them and plan to address
> them in v2:
> 
>   - make the helper comments more precise;
>   - change the `buf > buf_end` cases in safe_skip_bytes() to assertions;
>   - switch the new fixed-width integer helpers to reference/template
>     style, unless you prefer keeping them closer to the existing
>     safe_read_uleb128/safe_read_sleb128 style;
>   - add assertions that LEN fits in the destination type;
>   - remove the unrelated whitespace-only changes;
>   - change the dwarf_block_to_dwarf_reg_deref guard to the
>     `gdb_assert (buf <= buf_end); if (buf == buf_end) return -1;`
>     form;
>   - add selftests for dwarf_block_to_dwarf_reg and
>     dwarf_block_to_dwarf_reg_deref;
>   - rewrite the DW_OP_deref* branch using the explicit if/else form you
>     suggested.
> 
> For the helper style: in v1 I mirrored the nearby safe_read_uleb128 and
> safe_read_sleb128 helpers, which use output pointer parameters and short
> "or throw an error" comments.  But I agree that these new helpers can be
> clearer, so unless you prefer otherwise I will use the reference/template
> style you suggested for v2.

Ack.  It's good to leave the place a little nicer than how you found it
:).

Simon