Re: [PATCH 2/2] GDB: Use gdb::array_view for buffers used in register reading and unwinding

[Thiago Jung Bauermann <thiago.bauermann@linaro.org>]

Tom de Vries <tdevries@suse.de> writes:

> On 1/11/25 16:46, Thiago Jung Bauermann wrote:
>> Hello Tom,
>> Tom de Vries <tdevries@suse.de> writes:
>>
>>> On 1/10/25 23:28, Thiago Jung Bauermann wrote:
>> Thank you for debugging the issue! So memcpy was overflowing the
>> buffer. Nice to see the assert in action. :)
>
> Hi Thiago,
>
> thanks for the quick review.

Too quick, as it turns out. Looking at it again, that fix relies on
buffer.size () reflecting the correct size of the register being
unwound. Simon found a counterexample in i386_unwind_pc, which is used
for both i386 and x86_64 and thus passes an 8-byte buffer:

https://inbox.sourceware.org/gdb-patches/321e71e0-43de-4604-bb7e-34f6f64b83bf@simark.ca/

> Not the memcpy, but the memset, because the optimized out branch was activated, but buffer
> overflow indeed.
>
>>> Properly fixing this requires us to only look at the part that is relevant, copying the
>>> value from there, and checking for optimized out and unavailable only there.
>>>
>>> This worked for s390x-linux:
>>> ...
>>> diff --git a/gdb/frame.c b/gdb/frame.c
>>> index 10a32dcd896..02583857019 100644
>>> --- a/gdb/frame.c
>>> +++ b/gdb/frame.c
>>> @@ -1193,8 +1193,14 @@ frame_register_unwind (const frame_info_ptr &next_frame, int
>>> regnum,
>>>
>>>     gdb_assert (value != NULL);
>>>
>>> -  *optimizedp = value->optimized_out ();
>>> -  *unavailablep = !value->entirely_available ();
>>> +  if (value->lazy ())
>>> +    value->fetch_lazy ();
>>> +
>>> +  *optimizedp
>>> +    = value->bits_any_optimized_out (value->offset () * 8,
>>> +				     buffer.size () * 8);
>>> +  *unavailablep
>>> +    = !value->bytes_available (value->offset (), buffer.size ());
>>>     *lvalp = value->lval ();
>>>     *addrp = value->address ();
>>>     if (*lvalp == lval_register)
>>> @@ -1204,13 +1210,17 @@ frame_register_unwind (const frame_info_ptr &next_frame, int
>>> regnum,
>>>
>>>     if (!buffer.empty ())
>>>       {
>>> -      gdb_assert (buffer.size () >= value->type ()->length ());
>>> +      gdb_assert (buffer.size ()
>>> +		  <= value->type ()->length () - value->offset ());
>> It should be '>=' above.
>>
>
> That doesn't work unfortunately:
> - buffer.size () is 8
> - value->type ()->length () is 16
> - value->offset () == 0
>
> So we'd have gdb_assert (8 >= 16).

Ah, I didn't know offset was 0. We do need an assert that the buffer is
big enough though. :/

> FWIW, I've tried out a less intrusive fix which also works:
> ...
> diff --git a/gdb/frame.c b/gdb/frame.c
> index 10a32dcd896..96e0752888f 100644
> --- a/gdb/frame.c
> +++ b/gdb/frame.c
> @@ -1191,6 +1191,19 @@ frame_register_unwind (const frame_info_ptr &next_frame, int
> regnum,
>
>    value = frame_unwind_register_value (next_frame, regnum);

Your fix below is interesting, but I'm starting to think that the real
bug is that frame_unwind_register_value returns a value with a type that
isn't regnum's type. Even if the value was found in another register,
that shouldn't concern the caller. WDYT?

Also, should I revert the patch while we don't find a solution?

> +  frame_info_ptr this_frame = get_prev_frame (next_frame);
> +  struct gdbarch *gdbarch = frame_unwind_arch (this_frame);

Shouldn't this be either:

  struct gdbarch *gdbarch = frame_unwind_arch (next_frame);

or:

  struct gdbarch *gdbarch = get_frame_arch (this_frame);

? I still get confused about this/next in frame unwinding...

> +  size_t reg_size = register_size (gdbarch, regnum);
> +
> +  if (value->type ()->length () > reg_size)
> +    {
> +      struct value *part_val
> +	= value::allocate_register (this_frame, regnum);

Shouldn't next_frame be used here? (Same confused face as above.)

> +      value->contents_copy (part_val, 0, value->offset (), reg_size);
> +      release_value (value);
> +      value = part_val;
> +    }
> +
>    gdb_assert (value != NULL);
>
>    *optimizedp = value->optimized_out ();
> ...
> but I have doubts whether reg_size is correct in case gdbarch changes between frames and
> register sizes are different.

AFAIU GDB should know the gdbarch of each frame in the stack, otherwise
things can get weird. Also, is there another option? As I mentioned
before, the buffer size can't be used unless we audit callers to make
sure they pass a buffer with the same size as the register being unwound
(as Simon mentioned in the email I linked above).

--
Thiago