Re: [PATCH 1/2] gdb: return when exceeding buffer size in regcache::transfer_regset

[John Baldwin <jhb@FreeBSD.org>]

On 11/30/23 1:20 PM, Simon Marchi wrote:
> regcache::transfer_regset iterates over an array of regcache_map_entry,
> transferring the registers (between regcache and buffer) described by
> those entries.  It stops either when it reaches the end of the
> regcache_map_entry array (marked by a null entry) or (it seems like the
> intent is) when it reaches the end of the buffer (in which case not all
> described registers are transferred).
> 
> I said "seems like the intent is", because there appears to be a small
> bug.  transfer_regset is made of two loops:
> 
>      foreach regcache_map_entry:
>        foreach register described by the regcache_map_entry:
>          if the register doesn't fit in the remainder of the buffer:
> 	  break
> 
>          transfer register
> 
> When stopping because we have reached the end of the buffer, the break
> only breaks out of the inner loop.
> 
> This problem causes some failures when I run tests such as
> gdb.arch/aarch64-sme-core-3.exp (on AArch64 Linux, in qemu).  This is
> partly due to aarch64_linux_iterate_over_regset_sections failing to add
> a null terminator in its regcache_map_entry array, but I think there is
> still a problem in transfer_regset.
> 
> The sequence to the crash is:
> 
>   - The `regcache_map_entry za_regmap` object built in
>     aarch64_linux_iterate_over_regset_sections does not have a null
>     terminator.
>   - When the target does not have a ZA register,
>     aarch64_linux_collect_za_regset calls `regcache->collect_regset` with
>     a size of 0 (it's actually pointless, but still it should work).
>   - transfer_regset gets called with a buffer size of 0.
>   - transfer_regset detects that the register to transfer wouldn't fit in
>     0 bytes, so it breaks out of the inner loop.
>   - The outer loop tries to go read the next regcache_map_entry, but
>     there isn't one, and we start reading garbage.
> 
> Obviously, this would get fixed by making
> aarch64_linux_iterate_over_regset_sections use a null terminator (which
> is what the following patch does).  But I think that when detecting that
> there is not enough buffer left for the current register,
> transfer_regset should return, not only break out of the inner loop.
> 
> This is a kind of contrived scenario, but imagine we have these two
> regcache_map_entry objects:
> 
>    - 2 registers of 8 bytes
>    - 2 registers of 4 bytes
> 
> For some reason, the caller passes a buffer of 12 bytes.
> transfer_regset will detect that the second 8 byte register does not
> fit, and break out of the inner loop.  However, it will then go try the
> next regcache_map_entry.  It will see that it can fit one 4 byte
> register in the remaining buffer space, and transfer it from/to there.
> This is very likely not an expected behavior, we wouldn't expect to
> read/write this sequence of registers from/to the buffer.
> 
> In this example, whether passing a 12 bytes buffer makes sense or
> whether it is a size computation bug in the caller, we don't know, but I
> think that exiting as soon as a register doesn't fit is the sane thing
> to do.
> 
> Change-Id: Ia349627d2e5d281822ade92a8e7a4dea4f839e07
> ---
>   gdb/regcache.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/gdb/regcache.c b/gdb/regcache.c
> index 9dc354ec2b3a..e46a0b58f505 100644
> --- a/gdb/regcache.c
> +++ b/gdb/regcache.c
> @@ -1208,7 +1208,7 @@ regcache::transfer_regset (const struct regset *regset, int regbase,
>   	for (; count--; regno++, offs += slot_size)
>   	  {
>   	    if (offs + slot_size > size)
> -	      break;
> +	      return;
>   
>   	    transfer_regset_register (out_regcache, regno, in_buf, out_buf,
>   				      slot_size, offs);

This makes sense to me.  In particular, note that a few lines below we do a
return for the same condition when an individual register doesn't fit.

Reviewed-By: John Baldwin <jhb@FreeBSD.org>

-- 
John Baldwin