Check for truncated registers in process_g_packet

Check for truncated registers in process_g_packet

[Lionel Flandrin]

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.

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 4b642b8..73b9b9e 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,9 @@
+2016-10-18  Lionel Flandrin <lionel@svkt.org>
+
+	* remote.c (process_g_packet): Detect truncated registers in 'g'
+	packets and raise an error. Fixes a potential out-of-bounds buffer
+	access if the remote sent a truncated 'g' packet.
+
 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..5ec5ea6 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"));

Re: Check for truncated registers in process_g_packet

[Simon Marchi]

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.

> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> index 4b642b8..73b9b9e 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,3 +1,9 @@
> +2016-10-18  Lionel Flandrin <lionel@svkt.org>

Missing space between name and email.

> +
> +	* remote.c (process_g_packet): Detect truncated registers in 'g'
> +	packets and raise an error. Fixes a potential out-of-bounds buffer
> +	access if the remote sent a truncated 'g' packet.
> +

The ChangeLog should only contain "what has changed", and not the "why".  In
your case, I think the first sentence would be sufficient:

+	* remote.c (process_g_packet): Detect truncated registers in 'g'
+	packets and raise an error.

If somebody wants to know why that was changed, they would go look at the commit
message, where you explained the issue in details.  It's easy to go from the
ChangeLog entry to the commit using git blame.

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

Missing space after "register_size".

> +      /* 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

rsa->sizeof_g_packet ?

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

Use two spaces after each period (including the final one, before the */.

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

Space after register_size.

Thanks!

Simon

Re: Check for truncated registers in process_g_packet

[Lionel Flandrin]

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


-- 
Lionel Flandrin

Re: Check for truncated registers in process_g_packet

[Lionel Flandrin]

[-- Attachment #1: Type: text/plain, Size: 3845 bytes --]

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.

-- 
Lionel Flandrin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: Check for truncated registers in process_g_packet

[Pedro Alves]

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!

-- 
Pedro Alves

Re: Check for truncated registers in process_g_packet

[Yao Qi]

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.

-- 
Yao (齐尧)

Re: Check for truncated registers in process_g_packet

[Simon Marchi]

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

Re: Check for truncated registers in process_g_packet

[Simon Marchi]

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.

[PATCH 0/4] Try to fix the gdb.multi/multi-arch-exec.exp failure

[Simon Marchi]

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

-- 
2.7.4

[PATCH 2/4] Read stop_pc after updating the gdbarch when exec'ing

[Simon Marchi]

When an inferior execs and changes architecture (e.g. 64 bits to 32
bits), the gdbarch associated to the inferior is updated by the
follow_exec call in handle_inferior_event_1.  We should avoid doing any
register read before that point, because the registers sent by the
remote side will be those of the new architecture, but we would
interpret them using the old architecture.  We do just that by setting
stop_pc during this window, which obviously requires reading the
registers.  This results in gdb.multi/multi-arch-exec.exp failing, GDB
outputting the following error:

  Truncated register 50 in remote 'g' packet

This patch fixes that by postponing the setting of stop_pc to after
we've updated the inferior gdbarch.

This bug was hiding another problem, and as such introduces some
failures in gdb.base/foll-exec-mode.exp.  The following patch takes care
of that.

gdb/ChangeLog:

	* infrun.c (handle_inferior_event_1): When exec'ing, read
	stop_pc after follow_exec.
---
 gdb/infrun.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/gdb/infrun.c b/gdb/infrun.c
index d0e4105..de0605f 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -5318,8 +5318,6 @@ Cannot fill $_exitsignal with the correct signal number.\n"));
       if (!ptid_equal (ecs->ptid, inferior_ptid))
 	context_switch (ecs->ptid);
 
-      stop_pc = regcache_read_pc (get_thread_regcache (ecs->ptid));
-
       /* Do whatever is necessary to the parent branch of the vfork.  */
       handle_vfork_child_exec_or_exit (1);
 
@@ -5328,6 +5326,8 @@ Cannot fill $_exitsignal with the correct signal number.\n"));
          stop.  */
       follow_exec (inferior_ptid, ecs->ws.value.execd_pathname);
 
+      stop_pc = regcache_read_pc (get_thread_regcache (ecs->ptid));
+
       /* In follow_exec we may have deleted the original thread and
 	 created a new one.  Make sure that the event thread is the
 	 execd thread for that case (this is a nop otherwise).  */
-- 
2.7.4

Re: [PATCH 2/4] Read stop_pc after updating the gdbarch when exec'ing

[Yao Qi]

On 17-08-27 12:15:32, Simon Marchi wrote:
> 
> gdb/ChangeLog:
> 
> 	* infrun.c (handle_inferior_event_1): When exec'ing, read
> 	stop_pc after follow_exec.

Patch is good to me.  Sooner or later, we need to remove the global variable
stop_pc.

-- 
Yao (齐尧)

[PATCH 4/4] Test different follow-exec-mode settings in gdb.multi/multi-arch-exec.exp

[Simon Marchi]

Using follow-exec-mode "new" takes a different code path than "same", so
it's interesting to test this path in combination with a change in
architecture of the inferior.  This test fails if you remove the
previous patch.

gdb/testsuite/ChangeLog:

	* gdb.multi/multi-arch-exec.exp: Test with different
	"follow-exec-mode" settings.
	(do_test): New procedure.
---
 gdb/testsuite/gdb.multi/multi-arch-exec.exp | 25 ++++++++++++++++++-------
 1 file changed, 18 insertions(+), 7 deletions(-)

diff --git a/gdb/testsuite/gdb.multi/multi-arch-exec.exp b/gdb/testsuite/gdb.multi/multi-arch-exec.exp
index ed98532..3053345 100644
--- a/gdb/testsuite/gdb.multi/multi-arch-exec.exp
+++ b/gdb/testsuite/gdb.multi/multi-arch-exec.exp
@@ -76,12 +76,23 @@ if { [prepare_for_testing "failed to prepare" ${exec2} "${srcfile2}" \
     return -1
 }
 
-clean_restart ${exec1}
-if ![runto_main] then {
-    fail "couldn't run to main"
-    return -1
+proc do_test { mode } {
+	global exec1
+
+	clean_restart ${exec1}
+	if ![runto_main] then {
+	    fail "couldn't run to main"
+	    return -1
+	}
+
+	gdb_test_no_output "set follow-exec-mode $mode"
+
+	# Test that GDB updates the target description / arch successfuly
+	# after the exec.
+	gdb_test "continue" "Breakpoint 1, main.*" "continue across exec that changes architecture"
+	gdb_test "info inferior"
 }
 
-# Test that GDB updates the target description / arch successfuly
-# after the exec.
-gdb_test "continue" "Breakpoint 1, main.*" "continue across exec that changes architecture"
+foreach follow_exec_mode {"same" "new"} {
+    do_test $follow_exec_mode
+}
-- 
2.7.4

Re: [PATCH 4/4] Test different follow-exec-mode settings in gdb.multi/multi-arch-exec.exp

[Yao Qi]

On 17-08-27 12:15:34, Simon Marchi wrote:
> +	# Test that GDB updates the target description / arch successfuly
> +	# after the exec.
> +	gdb_test "continue" "Breakpoint 1, main.*" "continue across exec that changes architecture"
> +	gdb_test "info inferior"

Nit, any reason not to match the output here?

>  }
>  
> -# Test that GDB updates the target description / arch successfuly
> -# after the exec.
> -gdb_test "continue" "Breakpoint 1, main.*" "continue across exec that changes architecture"

Patch is good to me.

-- 
Yao (齐尧)

Re: [PATCH 4/4] Test different follow-exec-mode settings in gdb.multi/multi-arch-exec.exp

[Simon Marchi]

On 2017-09-05 12:39 PM, Yao Qi wrote:
> On 17-08-27 12:15:34, Simon Marchi wrote:
>> +	# Test that GDB updates the target description / arch successfuly
>> +	# after the exec.
>> +	gdb_test "continue" "Breakpoint 1, main.*" "continue across exec that changes architecture"
>> +	gdb_test "info inferior"
> 
> Nit, any reason not to match the output here?

Errr, I think I used that to debug and it shouldn't be there.  I'll remove it.

>>  }
>>  
>> -# Test that GDB updates the target description / arch successfuly
>> -# after the exec.
>> -gdb_test "continue" "Breakpoint 1, main.*" "continue across exec that changes architecture"
> 
> Patch is good to me.

Thanks, I am pushing the series to master and gdb-8.0-branch.

Simon

[PATCH 1/4] Improve "'g' reply is is to long" error message

[Simon Marchi]

... by adding the expected size, and the received size.  I found this
useful when debugging gdbarch/remote issues, since it gives a hint of
what gdb expects and what the remote sent.

gdb/ChangeLog:

	* remote.c (process_g_packet): Update error message.
---
 gdb/remote.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/gdb/remote.c b/gdb/remote.c
index 2249533..ea8474d 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -7581,7 +7581,8 @@ process_g_packet (struct regcache *regcache)
 
   /* Further sanity checks, with knowledge of the architecture.  */
   if (buf_len > 2 * rsa->sizeof_g_packet)
-    error (_("Remote 'g' packet reply is too long: %s"), rs->buf);
+    error (_("Remote 'g' packet reply is too long (expected %ld bytes, got %d "
+	     "bytes): %s"), rsa->sizeof_g_packet, buf_len / 2, rs->buf);
 
   /* Save the size of the packet sent to us by the target.  It is used
      as a heuristic when determining the max size of packets that the
-- 
2.7.4

Re: [PATCH 1/4] Improve "'g' reply is is to long" error message

[Yao Qi]

On 17-08-27 12:15:31, Simon Marchi wrote:
> ... by adding the expected size, and the received size.  I found this
> useful when debugging gdbarch/remote issues, since it gives a hint of
> what gdb expects and what the remote sent.
> 
> gdb/ChangeLog:
> 
> 	* remote.c (process_g_packet): Update error message.

Patch is good to me.

-- 
Yao (齐尧)

[PATCH 3/4] Add thread after updating gdbarch when exec'ing

[Simon Marchi]

As mentioned in the previous patch, we should avoid doing register reads
after a process does an exec and before we've updated that inferior's
gdbarch.  Otherwise, we may interpret the registers using the wrong
architecture.  When a process does an exec with "follow-exec-mode new",
a new inferior is added by follow_exec.  The gdbarch of that new
inferior is at first set to some default value, probably specific to the
gdb build (I get "i386" here), which may not be the right one.  It is
updated later by the call to target_find_description.  Before that
point, if we try to read the inferior's registers, we may not interpret
them correctly.  This has been exposed by a failure in
gdb.base/foll-exec-mode.exp after the previous patch, with:

  Remote 'g' packet reply is too long (expected 312 bytes, got 816 bytes)

The call to "add_thread" done just after adding the inferior is
problematic, because it ends up reading the registers (because the ptid
is re-used, we end up doing a switch_to_thread to it, which tries to
update stop_pc).  The registers returned by gdbserver are the x86-64
ones, while we try to interpret them using the "i386" gdbarch.

Postponing the call to add_thread to until the target
description/gdbarch has been updated seems to fix the issue.

As to why this issue was uncovered by the previous patch: what I think
happened before that patch is that since we were updating stop_pc before
switching to the new inferior, we were filling the regcache associated
to the ptid (this worked fine as long as the architectures of the
previous and new process images were the same).  The call to
switch_to_thread then worked, because the register read hit the
regcache.  Now, it triggers a register read, while the gdbarch is not
set correctly, leading to the "reply is too long" error.  If this is
right, it sounds wrong that we delete and re-add a thread with the same
ptid, and are able to access the registers from the deleted thread.
When we delete a thread, should we clear the regcache associated to that
ptid, so that the new thread starts with a fresh/empty regcache?

gdb/ChangeLog:

	* infrun.c (follow_exec): Call add_thread after
	target_find_description.
---
 gdb/infrun.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/gdb/infrun.c b/gdb/infrun.c
index de0605f..25beaf4 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -1211,7 +1211,6 @@ follow_exec (ptid_t ptid, char *exec_file_target)
 
       set_current_inferior (inf);
       set_current_program_space (inf->pspace);
-      add_thread (ptid);
     }
   else
     {
@@ -1243,6 +1242,11 @@ follow_exec (ptid_t ptid, char *exec_file_target)
      registers.  */
   target_find_description ();
 
+  /* The add_thread call ends up reading registers, so do it after updating the
+     target description.  */
+  if (follow_exec_mode_string == follow_exec_mode_new)
+    add_thread (ptid);
+
   solib_create_inferior_hook (0);
 
   jit_inferior_created_hook ();
-- 
2.7.4

Re: [PATCH 3/4] Add thread after updating gdbarch when exec'ing

[Yao Qi]

On 17-08-27 12:15:33, Simon Marchi wrote:
> As mentioned in the previous patch, we should avoid doing register reads
> after a process does an exec and before we've updated that inferior's
> gdbarch.  Otherwise, we may interpret the registers using the wrong
> architecture.  When a process does an exec with "follow-exec-mode new",
> a new inferior is added by follow_exec.  The gdbarch of that new
> inferior is at first set to some default value, probably specific to the
> gdb build (I get "i386" here), which may not be the right one.  It is
> updated later by the call to target_find_description.  Before that
> point, if we try to read the inferior's registers, we may not interpret
> them correctly.  This has been exposed by a failure in
> gdb.base/foll-exec-mode.exp after the previous patch, with:
> 
>   Remote 'g' packet reply is too long (expected 312 bytes, got 816 bytes)
> 
> The call to "add_thread" done just after adding the inferior is
> problematic, because it ends up reading the registers (because the ptid
> is re-used, we end up doing a switch_to_thread to it, which tries to
> update stop_pc).  The registers returned by gdbserver are the x86-64
> ones, while we try to interpret them using the "i386" gdbarch.

The analysis is great!

> 
> Postponing the call to add_thread to until the target
> description/gdbarch has been updated seems to fix the issue.

This imposes an odd restriction on using add_thread, that is, we must
keep in mind that we can't use add_thread until the inferior's gdbarch
or target description is updated.  The question in my mind is that why
do we need to update stop_pc in add_thread? or can we remove stop_pc?

-- 
Yao (齐尧)

Re: [PATCH 3/4] Add thread after updating gdbarch when exec'ing

[Simon Marchi]

On 2017-09-05 12:37 PM, Yao Qi wrote:
> On 17-08-27 12:15:33, Simon Marchi wrote:
>> As mentioned in the previous patch, we should avoid doing register reads
>> after a process does an exec and before we've updated that inferior's
>> gdbarch.  Otherwise, we may interpret the registers using the wrong
>> architecture.  When a process does an exec with "follow-exec-mode new",
>> a new inferior is added by follow_exec.  The gdbarch of that new
>> inferior is at first set to some default value, probably specific to the
>> gdb build (I get "i386" here), which may not be the right one.  It is
>> updated later by the call to target_find_description.  Before that
>> point, if we try to read the inferior's registers, we may not interpret
>> them correctly.  This has been exposed by a failure in
>> gdb.base/foll-exec-mode.exp after the previous patch, with:
>>
>>   Remote 'g' packet reply is too long (expected 312 bytes, got 816 bytes)
>>
>> The call to "add_thread" done just after adding the inferior is
>> problematic, because it ends up reading the registers (because the ptid
>> is re-used, we end up doing a switch_to_thread to it, which tries to
>> update stop_pc).  The registers returned by gdbserver are the x86-64
>> ones, while we try to interpret them using the "i386" gdbarch.
> 
> The analysis is great!
> 
>>
>> Postponing the call to add_thread to until the target
>> description/gdbarch has been updated seems to fix the issue.
> 
> This imposes an odd restriction on using add_thread, that is, we must
> keep in mind that we can't use add_thread until the inferior's gdbarch
> or target description is updated.  The question in my mind is that why
> do we need to update stop_pc in add_thread? or can we remove stop_pc?

Instinctively, I'd say that we should probably get rid of stop_pc, and instead
get the same info through the current thread instead.

I have a question for you, since you know much more about tdesc than me.  Installing
a default tdesc (e.g. i386) that is replaced just after with the right tdesc
(e.g. i386:x86-64) creates the risk, like in this case, of using the wrong tdesc.
Do you think it would be feasible (and a good idea) to instead have no tdesc
installed until we figure out which one to use?  We could then catch other operations
that are done while the wrong tdesc is present.

For now I'll push this patchset, so that it is in 8.1, but I feel it's just
covering the real problem of having the wrong tdesc installed.

Thanks for the review,

Simon

Re: [PATCH 3/4] Add thread after updating gdbarch when exec'ing

[Simon Marchi]

On 2017-09-05 05:30 PM, Simon Marchi wrote:
> For now I'll push this patchset, so that it is in 8.1, but I feel it's just
> covering the real problem of having the wrong tdesc installed.

Err, this should have said 8.0.1 and not 8.1.