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

[Simon Marchi <simark@simark.ca>]

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