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

[Tom de Vries]

On 1/12/25 01:32, Thiago Jung Bauermann wrote:
> 
> 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/
> 

Ack, I've taken this into account in a patch I've just submitted ( 
https://sourceware.org/pipermail/gdb-patches/2025-January/214695.html ).

>> Not the memcpy, but the memset, because the optimized out branch was activated, but buffer
>> overflow indeed.
>>

I seem to have confused myself here.  I thought I saw the test-case 
failing-but-not-asserting at 7fcdec025c0^, but I no longer see that. 
Instead I see it pass at 7fcdec025c0^, so it must have been an artifact 
of somehow not building clean or at the wrong commit.

In other words, indeed the memcpy triggers a buffer overflow.

My apologies for providing misleading information.

>>>> 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. :/
> 

The patch I've submitted no longer has such an assert, instead it 
guarantees no buffer overflow through use of std::min.

>> 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?
> 

I'm not sure.  In this case the type seems less relevant because it's 
not returned from frame_register_unwind.  What is returned is 
value->regnum (), so this size discrepancy could propagate.

Things would be greatly simplified if frame_unwind_register_value 
returned the value of f0 instead of v0 in this case.

But perhaps that is a special case, and we're better off covering the 
generic case here in frame_register_unwind and elsewhere.

FWIW, I'm working on other patches addressing the exact same problem in 
other places.

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

If my proposed patch is acceptable, then there's no need, otherwise we 
might need to revisit that.

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

Yeah, I was and very much still am confused about this.

In my proposed patch, that's no longer relevant, I've pretty much 
reverted to my original approach.

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

I'm not sure I understand your question, but my concern is that I'm not 
picking the right frame.

Picking the wrong frame will only matter in case architecture changes 
between frames, OTOH detecting that we've picked the wrong frame means 
exercising that scenario, and I don't know how to.

Thanks,
- Tom