[06/24] gdb: fix bugs in {get,put}_frame_register_bytes

  From: Simon Marchi <simon.marchi@efficios.com>

I found this only by inspection: the myaddr pointer in
{get,put}_frame_register_bytes is reset to `buffer.data ()` at each
iteration.  This means that we will always use the bytes at the
beginning of `buffer` to read or write to the registers, instead of
progressing in `buffer`.

Fix this by re-writing the functions to chip away the beginning of the
buffer array_view as we progress in reading or writing the data.

These bugs was introduced almost 3 years ago [1], and yet nobody
complained.  I'm wondering which architecture relies on that register
"overflow" behavior (reading or writing multiple consecutive registers
with one {get,put}_frame_register_bytes calls), and in which situation.
I find these functions a bit dangerous, if a caller mis-calculates
things, it could end up silently reading or writing to the next
register, even if it's not the intent.

If I could change it, I would prefer to have functions specifically made
for that ({get,put}_frame_register_bytes_consecutive or something like
that) and make {get,put}_frame_register_bytes only able to write within
a single register (which I presume represents most of the use cases of
the current {get,put}_frame_register_bytes).  If a caller mis-calculates
things and an overflow occurs while calling
{get,put}_frame_register_bytes, it would hit an assert.  The problem is
knowing which callers rely on the overflow behavior and which don't.

[1] https://gitlab.com/gnutools/binutils-gdb/-/commit/bdec2917b1e94c7198ba39919f45060067952f43

Change-Id: I43bd4a9f7fa8419d388a2b20bdc57d652688ddf8
---
 gdb/frame.c | 63 +++++++++++++++++++----------------------------------
 1 file changed, 23 insertions(+), 40 deletions(-)
  

Simon Marchi <simon.marchi@polymtl.ca> writes:

> From: Simon Marchi <simon.marchi@efficios.com>
>
> I found this only by inspection: the myaddr pointer in
> {get,put}_frame_register_bytes is reset to `buffer.data ()` at each
> iteration.  This means that we will always use the bytes at the
> beginning of `buffer` to read or write to the registers, instead of
> progressing in `buffer`.
>
> Fix this by re-writing the functions to chip away the beginning of the
> buffer array_view as we progress in reading or writing the data.
>
> These bugs was introduced almost 3 years ago [1], and yet nobody
> complained.  I'm wondering which architecture relies on that register
> "overflow" behavior (reading or writing multiple consecutive registers
> with one {get,put}_frame_register_bytes calls), and in which situation.
> I find these functions a bit dangerous, if a caller mis-calculates
> things, it could end up silently reading or writing to the next
> register, even if it's not the intent.
>
> If I could change it, I would prefer to have functions specifically made
> for that ({get,put}_frame_register_bytes_consecutive or something like
> that) and make {get,put}_frame_register_bytes only able to write within
> a single register (which I presume represents most of the use cases of
> the current {get,put}_frame_register_bytes).  If a caller mis-calculates
> things and an overflow occurs while calling
> {get,put}_frame_register_bytes, it would hit an assert.  The problem is
> knowing which callers rely on the overflow behavior and which don't.

I agree that this overflow behaviour sucks.

I have a memory of being told (years ago now) that this was a result of
some older compilers not emitting correct DWARF for compound value
locations, instead the compiler would just emit a single register
location, and assume that the debugger would read from consecutive DWARF
registers.  Note, this code assumes that the DWARF register numbering is
the same as GDB's register numbering, which is not always the case.

Personally, I'd love for GDB to be more aggressive about removing some
of this legacy behaviour.  What I'd like to do is move things like this
behind a switch, say 'set maintenance deprecated-features on|off', which
would be off by default, but when it is turned on we'd print a message
saying:

  The feature you turned this on for is deprecated, and will be removed
  from future versions of GDB.  To avoid this feature removed, please
  file a bug report here <url> describing the deprecated feature that
  you are using.

Then if nobody complains after a couple of years, we can start deleting
things.

Anyway... just putting my thoughts down.  I think this patch is fine.

Approved-By: Andrew Burgess <aburgess@redhat.com>

Thanks,
Andrew


>
> [1] https://gitlab.com/gnutools/binutils-gdb/-/commit/bdec2917b1e94c7198ba39919f45060067952f43
>
> Change-Id: I43bd4a9f7fa8419d388a2b20bdc57d652688ddf8
> ---
>  gdb/frame.c | 63 +++++++++++++++++++----------------------------------
>  1 file changed, 23 insertions(+), 40 deletions(-)
>
> diff --git a/gdb/frame.c b/gdb/frame.c
> index afadb8ac4e73..b3d99163b4dc 100644
> --- a/gdb/frame.c
> +++ b/gdb/frame.c
> @@ -1482,9 +1482,6 @@ get_frame_register_bytes (frame_info_ptr frame, int regnum,
>  			  int *optimizedp, int *unavailablep)
>  {
>    struct gdbarch *gdbarch = get_frame_arch (frame);
> -  int i;
> -  int maxsize;
> -  int numregs;
>  
>    /* Skip registers wholly inside of OFFSET.  */
>    while (offset >= register_size (gdbarch, regnum))
> @@ -1495,9 +1492,9 @@ get_frame_register_bytes (frame_info_ptr frame, int regnum,
>  
>    /* Ensure that we will not read beyond the end of the register file.
>       This can only ever happen if the debug information is bad.  */
> -  maxsize = -offset;
> -  numregs = gdbarch_num_cooked_regs (gdbarch);
> -  for (i = regnum; i < numregs; i++)
> +  int maxsize = -offset;
> +  int numregs = gdbarch_num_cooked_regs (gdbarch);
> +  for (int i = regnum; i < numregs; i++)
>      {
>        int thissize = register_size (gdbarch, i);
>  
> @@ -1506,20 +1503,15 @@ get_frame_register_bytes (frame_info_ptr frame, int regnum,
>        maxsize += thissize;
>      }
>  
> -  int len = buffer.size ();
> -  if (len > maxsize)
> +  if (buffer.size () > maxsize)
>      error (_("Bad debug information detected: "
> -	     "Attempt to read %d bytes from registers."), len);
> +	     "Attempt to read %zu bytes from registers."), buffer.size ());
>  
>    /* Copy the data.  */
> -  while (len > 0)
> +  while (!buffer.empty ())
>      {
> -      int curr_len = register_size (gdbarch, regnum) - offset;
> -
> -      if (curr_len > len)
> -	curr_len = len;
> -
> -      gdb_byte *myaddr = buffer.data ();
> +      int curr_len = std::min<int> (register_size (gdbarch, regnum) - offset,
> +				    buffer.size ());
>  
>        if (curr_len == register_size (gdbarch, regnum))
>  	{
> @@ -1527,8 +1519,8 @@ get_frame_register_bytes (frame_info_ptr frame, int regnum,
>  	  CORE_ADDR addr;
>  	  int realnum;
>  
> -	  frame_register (frame, regnum, optimizedp, unavailablep,
> -			  &lval, &addr, &realnum, myaddr);
> +	  frame_register (frame, regnum, optimizedp, unavailablep, &lval,
> +			  &addr, &realnum, buffer.data ());
>  	  if (*optimizedp || *unavailablep)
>  	    return false;
>  	}
> @@ -1547,13 +1539,12 @@ get_frame_register_bytes (frame_info_ptr frame, int regnum,
>  	      return false;
>  	    }
>  
> -	  memcpy (myaddr, value->contents_all ().data () + offset,
> -		  curr_len);
> +	  copy (value->contents_all ().slice (offset, curr_len),
> +		buffer.slice (0, curr_len));
>  	  release_value (value);
>  	}
>  
> -      myaddr += curr_len;
> -      len -= curr_len;
> +      buffer = buffer.slice (curr_len);
>        offset = 0;
>        regnum++;
>      }
> @@ -1578,36 +1569,28 @@ put_frame_register_bytes (frame_info_ptr frame, int regnum,
>        regnum++;
>      }
>  
> -  int len = buffer.size ();
>    /* Copy the data.  */
> -  while (len > 0)
> +  while (!buffer.empty ())
>      {
> -      int curr_len = register_size (gdbarch, regnum) - offset;
> +      int curr_len = std::min<int> (register_size (gdbarch, regnum) - offset,
> +				    buffer.size ());
>  
> -      if (curr_len > len)
> -	curr_len = len;
> -
> -      const gdb_byte *myaddr = buffer.data ();
>        if (curr_len == register_size (gdbarch, regnum))
> -	{
> -	  put_frame_register (frame, regnum, myaddr);
> -	}
> +	put_frame_register (frame, regnum, buffer.data ());
>        else
>  	{
> -	  struct value *value
> +	  value *value
>  	    = frame_unwind_register_value (frame_info_ptr (frame->next),
>  					   regnum);
> -	  gdb_assert (value != NULL);
> +	  gdb_assert (value != nullptr);
>  
> -	  memcpy ((char *) value->contents_writeable ().data () + offset,
> -		  myaddr, curr_len);
> -	  put_frame_register (frame, regnum,
> -			      value->contents_raw ().data ());
> +	  copy (buffer.slice (0, curr_len),
> +		value->contents_writeable ().slice (offset, curr_len));
> +	  put_frame_register (frame, regnum, value->contents_raw ().data ());
>  	  release_value (value);
>  	}
>  
> -      myaddr += curr_len;
> -      len -= curr_len;
> +      buffer = buffer.slice (curr_len);
>        offset = 0;
>        regnum++;
>      }
> -- 
> 2.42.1
  

On 11/13/23 10:00, Andrew Burgess wrote:
> Simon Marchi <simon.marchi@polymtl.ca> writes:
> 
>> From: Simon Marchi <simon.marchi@efficios.com>
>>
>> I found this only by inspection: the myaddr pointer in
>> {get,put}_frame_register_bytes is reset to `buffer.data ()` at each
>> iteration.  This means that we will always use the bytes at the
>> beginning of `buffer` to read or write to the registers, instead of
>> progressing in `buffer`.
>>
>> Fix this by re-writing the functions to chip away the beginning of the
>> buffer array_view as we progress in reading or writing the data.
>>
>> These bugs was introduced almost 3 years ago [1], and yet nobody
>> complained.  I'm wondering which architecture relies on that register
>> "overflow" behavior (reading or writing multiple consecutive registers
>> with one {get,put}_frame_register_bytes calls), and in which situation.
>> I find these functions a bit dangerous, if a caller mis-calculates
>> things, it could end up silently reading or writing to the next
>> register, even if it's not the intent.
>>
>> If I could change it, I would prefer to have functions specifically made
>> for that ({get,put}_frame_register_bytes_consecutive or something like
>> that) and make {get,put}_frame_register_bytes only able to write within
>> a single register (which I presume represents most of the use cases of
>> the current {get,put}_frame_register_bytes).  If a caller mis-calculates
>> things and an overflow occurs while calling
>> {get,put}_frame_register_bytes, it would hit an assert.  The problem is
>> knowing which callers rely on the overflow behavior and which don't.
> 
> I agree that this overflow behaviour sucks.
> 
> I have a memory of being told (years ago now) that this was a result of
> some older compilers not emitting correct DWARF for compound value
> locations, instead the compiler would just emit a single register
> location, and assume that the debugger would read from consecutive DWARF
> registers.  Note, this code assumes that the DWARF register numbering is
> the same as GDB's register numbering, which is not always the case.
> 
> Personally, I'd love for GDB to be more aggressive about removing some
> of this legacy behaviour.  What I'd like to do is move things like this
> behind a switch, say 'set maintenance deprecated-features on|off', which
> would be off by default, but when it is turned on we'd print a message
> saying:
> 
>   The feature you turned this on for is deprecated, and will be removed
>   from future versions of GDB.  To avoid this feature removed, please
>   file a bug report here <url> describing the deprecated feature that
>   you are using.
> 
> Then if nobody complains after a couple of years, we can start deleting
> things.
> 
> Anyway... just putting my thoughts down.  I think this patch is fine.

Agreed, it would be nice.

> Approved-By: Andrew Burgess <aburgess@redhat.com>

Thanks,

Simon