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 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 > > > 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 . > > + > > +# 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) > >