Check for truncated registers in process_g_packet

  On Tue, Oct 18, 2016 at 11:49:01AM -0400, Simon Marchi wrote:
> On 16-10-18 07:10 AM, Lionel Flandrin wrote:
> > Hello,
> > 
> > While investigating an unrelated issue in remote.c I noticed that the
> > bound checking for 'g' packets was bogus:
> > 
> > The previous code would only check that the first byte of the register
> > was within bounds before passing the buffer to regcache_raw_supply. If
> > it turned out that the register in the 'g' packet was incomplete then
> > regcache_raw_supply would proceed to memcpy out-of-bounds.
> > 
> > Since the buffer is allocated with alloca it's relatively unlikely to
> > crash (you just end up dumping gdb's stack into the cache) but it's
> > still a bit messy.
> > 
> > I changed this logic to check for truncated registers and raise an
> > error if one is encountered. Hopefully it should make debugging remote
> > stubs a bit easier.
> 
> Hi Lionel,
> 
> This patch looks good to me, a few minor comments below about formatting.
> Someone else with the approval stamp must look at it, but hopefully it will
> save them a bit of work.

Thank you for the feedback, here's the updated patch:
  

On Tue, Oct 18, 2016 at 06:07:40PM +0200, Lionel Flandrin wrote:
> On Tue, Oct 18, 2016 at 11:49:01AM -0400, Simon Marchi wrote:
> > On 16-10-18 07:10 AM, Lionel Flandrin wrote:
> > > Hello,
> > > 
> > > While investigating an unrelated issue in remote.c I noticed that the
> > > bound checking for 'g' packets was bogus:
> > > 
> > > The previous code would only check that the first byte of the register
> > > was within bounds before passing the buffer to regcache_raw_supply. If
> > > it turned out that the register in the 'g' packet was incomplete then
> > > regcache_raw_supply would proceed to memcpy out-of-bounds.
> > > 
> > > Since the buffer is allocated with alloca it's relatively unlikely to
> > > crash (you just end up dumping gdb's stack into the cache) but it's
> > > still a bit messy.
> > > 
> > > I changed this logic to check for truncated registers and raise an
> > > error if one is encountered. Hopefully it should make debugging remote
> > > stubs a bit easier.
> > 
> > Hi Lionel,
> > 
> > This patch looks good to me, a few minor comments below about formatting.
> > Someone else with the approval stamp must look at it, but hopefully it will
> > save them a bit of work.
> 
> Thank you for the feedback, here's the updated patch:
> 
> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> index 4b642b8..3ace874 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,3 +1,8 @@
> +2016-10-18  Lionel Flandrin  <lionel@svkt.org>
> +
> +	* remote.c (process_g_packet): Detect truncated registers in 'g'
> +	packets and raise an error.
> +
>  2016-10-18  Maciej W. Rozycki  <macro@imgtec.com>
>  
>  	* i386-tdep.c (i386_mpx_info_bounds): Make sure the architecture
> diff --git a/gdb/remote.c b/gdb/remote.c
> index af7508a..e1b5ad7 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -7163,18 +7163,31 @@ process_g_packet (struct regcache *regcache)
>       the 'p' packet must be used.  */
>    if (buf_len < 2 * rsa->sizeof_g_packet)
>      {
> -      rsa->sizeof_g_packet = buf_len / 2;
> +      long sizeof_g_packet = buf_len / 2;
>  
>        for (i = 0; i < gdbarch_num_regs (gdbarch); i++)
>  	{
> +	  long offset = rsa->regs[i].offset;
> +	  long reg_size = register_size (gdbarch, i);
> +
>  	  if (rsa->regs[i].pnum == -1)
>  	    continue;
>  
> -	  if (rsa->regs[i].offset >= rsa->sizeof_g_packet)
> +	  if (offset >= sizeof_g_packet)
>  	    rsa->regs[i].in_g_packet = 0;
> +	  else if (offset + reg_size > sizeof_g_packet)
> +	    error (_("Truncated register %d in remote 'g' packet"), i);
>  	  else
>  	    rsa->regs[i].in_g_packet = 1;
>  	}
> +
> +      /* Looks valid enough, we can assume this is the correct length
> +         for a 'g' packet.  It's important not to adjust
> +         rsa->sizeof_g_packet if we have truncated registers otherwise
> +         this "if" won't be run the next time the method is called
> +         with a packet of the same size and one of the internal errors
> +         below will trigger instead.  */
> +      rsa->sizeof_g_packet = sizeof_g_packet;
>      }
>  
>    regs = (char *) alloca (rsa->sizeof_g_packet);
> @@ -7204,10 +7217,11 @@ process_g_packet (struct regcache *regcache)
>    for (i = 0; i < gdbarch_num_regs (gdbarch); i++)
>      {
>        struct packet_reg *r = &rsa->regs[i];
> +      long reg_size = register_size (gdbarch, i);
>  
>        if (r->in_g_packet)
>  	{
> -	  if (r->offset * 2 >= strlen (rs->buf))
> +	  if ((r->offset + reg_size) * 2 > strlen (rs->buf))
>  	    /* This shouldn't happen - we adjusted in_g_packet above.  */
>  	    internal_error (__FILE__, __LINE__,
>  			    _("unexpected end of 'g' packet reply"));
> 

I'm politely bumping this so that it doesn't get forgotten. Sorry for
the noise.
  

On 10/18/2016 05:07 PM, Lionel Flandrin wrote:

>> This patch looks good to me, a few minor comments below about formatting.
>> Someone else with the approval stamp must look at it, but hopefully it will
>> save them a bit of work.
> 
> Thank you for the feedback, here's the updated patch:

This is OK.  I've pushed it in.

Do you have a copyright assignment in place with the FSF?
I looked at the FSF records and couldn't find one.  This patch is about
the largest we can accept without one.  If you'd like to contribute
further, we'll need to get that sorted out.  Feel free to follow
up off list.

Thanks for your contribution!
  

On Tue, Nov 8, 2016 at 10:37 AM, Pedro Alves <palves@redhat.com> wrote:
> On 10/18/2016 05:07 PM, Lionel Flandrin wrote:
>
>>> This patch looks good to me, a few minor comments below about formatting.
>>> Someone else with the approval stamp must look at it, but hopefully it will
>>> save them a bit of work.
>>
>> Thank you for the feedback, here's the updated patch:
>
> This is OK.  I've pushed it in.
>

This patch 9dc193c causes a regression,

$ make check RUNTESTFLAGS="--target_board=native-extended-gdbserver
multi-arch-exec.exp"
FAIL: gdb.multi/multi-arch-exec.exp: continue across exec that changes
architecture

This test passes on the previous commit.  The test
passes also if I revert this commit on mainline.
  

On 2017-08-25 12:53, Yao Qi wrote:
> This patch 9dc193c causes a regression,
> 
> $ make check RUNTESTFLAGS="--target_board=native-extended-gdbserver
> multi-arch-exec.exp"
> FAIL: gdb.multi/multi-arch-exec.exp: continue across exec that changes
> architecture
> 
> This test passes on the previous commit.  The test
> passes also if I revert this commit on mainline.

 From what I can see, the line that causes the problem is

   stop_pc = regcache_read_pc (get_thread_regcache (ecs->ptid));

at infrun.c:5321.  At this point, the process we are debugging has 
exec'ed.  It used to be a 64-bits process, it is now a 32-bits process.  
However, current_inferior_->gdbarch still points to the 64-bits gdbarch. 
  It's only the follow_exec call a few lines below that will update it to 
the new gdbarch.  By reading the PC, we send a g packet.  The response 
contains the registers of a 32-bits process, but we interpret them as 
those of a 64-bits process (because get_remote_arch_state uses 
current_inferior_->gdbarch).

If I move the line mentioned above just after the follow_exec call, gdb 
interprets the g reply with the right/new gdbarch, so the test case 
works.  I don't know if it breaks anything else, but so far I didn't 
find anything before that point that relied on stop_pc.  I sent that 
change to the buildbot to check.

So from what I understand, it looks like a pre-existing bug that this 
patch uncovered.  I think we were interpreting the g reply containing 
32-bits registers using the 64-bits register map all along, which that 
stop_pc had a bogus value.

To confirm this, I checked out the commit just prior this patch.  I see 
stop_pc having a value of 0 (it could be anything I guess).  If I move 
the assignment of stop_pc just after follow_exec, I see a value of 
0xf7fd9a20.  That value is the mapping address of the dynamic loader in 
the process:

   f7fd9000-f7ffb000 r-xp 00000000 fc:01 395792                           
   /lib/i386-linux-gnu/ld-2.23.so

plus the entry point in it:

   Entry point address:               0xa20

so it makes sense that the process is stopped at this address.

Simon
  

On 2017-08-25 23:04, Simon Marchi wrote:
> If I move the line mentioned above just after the follow_exec call,
> gdb interprets the g reply with the right/new gdbarch, so the test
> case works.  I don't know if it breaks anything else, but so far I
> didn't find anything before that point that relied on stop_pc.  I sent
> that change to the buildbot to check.

Hmm, this causes gdb.base/foll-exec-mode.exp to fail.
  

This is an attempt at fixing the failure in gdb.multi/multi-arch-exec.exp
failure found by Yao.  From what I understand, Lionel's patch is good, it just
exposed some pre-existing bugs.  I sent this on the buildbot, I get some
failures on AArch64 and s390, mostly in threads related tests.  I don't think
it's related to this patch.

Simon Marchi (4):
  Improve "'g' reply is is to long" error message
  Read stop_pc after updating the gdbarch when exec'ing
  Add thread after updating gdbarch when exec'ing
  Test different follow-exec-mode settings in
    gdb.multi/multi-arch-exec.exp

 gdb/infrun.c                                | 10 +++++++---
 gdb/remote.c                                |  3 ++-
 gdb/testsuite/gdb.multi/multi-arch-exec.exp | 25 ++++++++++++++++++-------
 3 files changed, 27 insertions(+), 11 deletions(-)