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

[Firmiana <firmiana402@gmail.com>]

[-- 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 --]