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

[Tom de Vries <tdevries@suse.de>]

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:
>>> +      gdb_assert (buffer.size () >= value->type ()->length ());
>>> +
>>
>> This causes a regression on s390x-linux for test-case gdb.base/return.exp:
>> ...
>> (gdb) PASS: gdb.base/return.exp: continue to return of -5
>> return 5^M
>> Make func2 return now? (y or n) y^M
>> /home/vries/gdb/src/gdb/frame.c:1207: internal-error: frame_register_unwind: Assertion
>> `buffer.size () >= value->type ()->length ()' failed.^M
>> A problem internal to GDB has been detected,^M
>> further debugging may prove unreliable.^M
>> ----- Backtrace -----^M
>> FAIL: gdb.base/return.exp: return value 5 (GDB internal error)
>> ...
>>
>> Before the commit, the test-case produces a fail, but doesn't assert:
>> ...
>> (gdb) PASS: gdb.base/return.exp: continue to return of -5
>> return 5^M
>> Make func2 return now? (y or n) y^M
>> value has been optimized out^M
>> (gdb) FAIL: gdb.base/return.exp: return value 5
>> ...
>>
>> Concretely, we're trying to read machine register r11, which according to the CFI is saved
>> in dwarf register 16:
>> ...
>>    DW_CFA_register: r11 in r16 (f0)
>> ...
>>
>> Dwarf register 16 corresponds to f0 / v0 according to the ABI, and since v0 is available,
>> v0 is picked by s390_dwarf_reg_to_regnum.
>>
>> The assert then fails because the buffer that should hold the value of 8 byte register
>> r11:
>> ...
>> (gdb) p buffer.size ()
>> $1 = 8
>> ...
>> is smaller than the size of register v0:
>> ...
>> (gdb) p value->type ()->length ()
>> $2 = 16
>> ...
>>
>> Removing the assert reverts back to previous behaviour.
> 
> 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.

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

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);

+  frame_info_ptr this_frame = get_prev_frame (next_frame);
+  struct gdbarch *gdbarch = frame_unwind_arch (this_frame);
+  size_t reg_size = register_size (gdbarch, regnum);
+
+  if (value->type ()->length () > reg_size)
+    {
+      struct value *part_val
+	= value::allocate_register (this_frame, regnum);
+      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.

Thanks,
- Tom

>>         if (!*optimizedp && !*unavailablep)
>> -	memcpy (buffer.data (), value->contents_all ().data (),
>> -		value->type ()->length ());
>> +	{
>> +	  auto value_part
>> +	    = value->contents_all ().slice (value->offset (), buffer.size ());
>> +	  memcpy (buffer.data (), value_part.data (), buffer.size ());
>> +	}
>>         else
>> -	memset (buffer.data (), 0, value->type ()->length ());
>> +	memset (buffer.data (), 0, buffer.size ());
>>       }
>>
>>     /* Dispose of the new value.  This prevents watchpoints from
>> ...
> 
> Thank you for the patch! With the fix in the assert comparison:
> 
> Reviewed-by: Thiago Jung Bauermann <thiago.bauermann@linaro.org>
> 
> --
> Thiago