Re: [PATCH v2] gdb: Check bounds before reading DWARF expression operands

[Tom Tromey <tom@tromey.com>]

>>>>> Jielun Wu <firmiana402@gmail.com> writes:

> Some DWARF expression opcodes read fixed-width operands or block payloads
> before checking that the bytes are available in the current expression.
> Malformed expressions can consequently consume opcode or unrelated bytes
> and fail later with misleading results.

Thanks for the patch.

> +/* Return BUF advanced by BYTES bytes.  Throw an error if fewer than BYTES
> +   bytes remain before BUF_END.  */
> +
> +static const gdb_byte *
> +safe_skip_bytes (const gdb_byte *buf, const gdb_byte *buf_end,
> +		 ULONGEST bytes)
> +{
> +  gdb_assert (buf <= buf_end);

It's conventional in C for the end pointer to be one-past-the-end.
And I think that's true in this code too:

      const gdb_byte *op_ptr = expr.data ();
      const gdb_byte *op_end = expr.data () + expr.size ();

    +	    op_ptr = safe_skip_bytes (op_ptr, op_end, len);

So using "<" seems more correct than "<=" here?

OTOH maybe it doesn't matter due to the subsequent check:

> +  if (bytes > (ULONGEST) (buf_end - buf))
> +    error (_("DWARF expression error: ran off end of buffer reading bytes"));

Curious what you think.  Anyway that's my only nit on this patch.

Tom