[patch] [gdbserver] Fix memory corruption

[patch] [gdbserver] Fix memory corruption

[Jan Kratochvil]

Hi,

gdb.server/ext-run.exp always crashes during the nightly regression tests:
	info os processes
	memory clobbered past end of allocated block
	Remote communication error.  Target disconnected.: Connection reset by peer.
	(gdb) FAIL: gdb.server/ext-run.exp: get process list (pattern 1)

Probably OK to check in but I rather ask.

To make it easily reproducible one can disable try_rle() by patching it:
+return 1;
   /* Don't go past '~'.  */

So that putpkt_binary_1's cnt == 16383 will overrun PBUFSIZ 16384 by 4 bytes.


Thanks,
Jan


gdb/gdbserver/
2011-03-01  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* remote-utils.c (putpkt_binary_1): Calculate BUF2 size dynamically.

--- a/gdb/gdbserver/remote-utils.c
+++ b/gdb/gdbserver/remote-utils.c
@@ -725,7 +725,7 @@ putpkt_binary_1 (char *buf, int cnt, int is_notif)
   char *p;
   int cc;
 
-  buf2 = xmalloc (PBUFSIZ);
+  buf2 = xmalloc (1 + cnt + 4);
 
   /* Copy the packet into buffer BUF2, encapsulating it
      and giving it a checksum.  */

Re: [patch] [gdbserver] Fix memory corruption

[Pedro Alves]

On Tuesday 01 March 2011 21:34:28, Jan Kratochvil wrote:
> Hi,
> 
> gdb.server/ext-run.exp always crashes during the nightly regression tests:
> 	info os processes
> 	memory clobbered past end of allocated block
> 	Remote communication error.  Target disconnected.: Connection reset by peer.
> 	(gdb) FAIL: gdb.server/ext-run.exp: get process list (pattern 1)
> 
> Probably OK to check in but I rather ask.
> 
> To make it easily reproducible one can disable try_rle() by patching it:
> +return 1;
>    /* Don't go past '~'.  */

I can't reproduce it.

> 
> So that putpkt_binary_1's cnt == 16383 will overrun PBUFSIZ 16384 by 4 bytes.

How did you get that large of a `cnt' in the first place?  The largest
I get is 16379.

gdb does:

  /* Request only enough to fit in a single packet.  The actual data
     may not, since we don't know how much of it will need to be escaped;
     the target is free to respond with slightly less data.  We subtract
     five to account for the response type and the protocol frame.  */
  n = min (get_remote_packet_size () - 5, len);
  snprintf (rs->buf, get_remote_packet_size () - 4, "qXfer:%s:read:%s:%s,%s",
	    object_name, annex ? annex : "",
	    phex_nz (offset, sizeof offset),
	    phex_nz (n, sizeof n));

that is, you shouldn't get a read request that big.

It looks like server.c:handle_qxfer's len caping is forgetting
to account for the $, # and checksum (should be fixed), but I don't
think that's the real cause in your example, since it only pushes back
to gdb as much data as it requested.

gdb's putpkt_binary reads:

/* Send a packet to the remote machine, with error checking.  The data
   of the packet is in BUF.  The string in BUF can be at most
   get_remote_packet_size () - 5 to account for the $, # and checksum,
   and for a possible /0 if we are debugging (remote_debug) and want
   to print the sent packet as a string.  */

gdbserver's version does not have that comment, although it should.

Both versions lack an assertion catching the case of trying
to send too much for the packet buffer size.

> 
> 
> Thanks,
> Jan
> 
> 
> gdb/gdbserver/
> 2011-03-01  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	* remote-utils.c (putpkt_binary_1): Calculate BUF2 size dynamically.
> 
> --- a/gdb/gdbserver/remote-utils.c
> +++ b/gdb/gdbserver/remote-utils.c
> @@ -725,7 +725,7 @@ putpkt_binary_1 (char *buf, int cnt, int is_notif)
>    char *p;
>    int cc;
>  
> -  buf2 = xmalloc (PBUFSIZ);
> +  buf2 = xmalloc (1 + cnt + 4);
>  
>    /* Copy the packet into buffer BUF2, encapsulating it
>       and giving it a checksum.  */
> 

-- 
Pedro Alves

Re: [patch] [gdbserver] Fix memory corruption

[Jan Kratochvil]

On Wed, 02 Mar 2011 16:35:16 +0100, Pedro Alves wrote:
> On Tuesday 01 March 2011 21:34:28, Jan Kratochvil wrote:
> > To make it easily reproducible one can disable try_rle() by patching it:
> > +return 1;
> >    /* Don't go past '~'.  */
> 
> I can't reproduce it.

You may need also -lmcheck for gdbserver.


> > So that putpkt_binary_1's cnt == 16383 will overrun PBUFSIZ 16384 by 4 bytes.
> 
> How did you get that large of a `cnt' in the first place?  The largest
> I get is 16379.

remote_escape_output is called with OUT_MAXLEN == PBUFSIZ - 2 == 16382 and
returns it +1, therefore 16383.
#8  0x0000000000404ca3 in putpkt_binary_1 (buf=0x258f2c0 "mmn"..., cnt=16381, is_notif=0) at remote-utils.c:801
801	  free (buf2);

Maybe your system does not have
# cat /proc/*/cmdline|tr -cd '$#}*'|wc -c
84


> gdb does:
> 
>   /* Request only enough to fit in a single packet.  The actual data
>      may not, since we don't know how much of it will need to be escaped;
>      the target is free to respond with slightly less data.  We subtract
>      five to account for the response type and the protocol frame.  */
>   n = min (get_remote_packet_size () - 5, len);
>   snprintf (rs->buf, get_remote_packet_size () - 4, "qXfer:%s:read:%s:%s,%s",
> 	    object_name, annex ? annex : "",
> 	    phex_nz (offset, sizeof offset),
> 	    phex_nz (n, sizeof n));
> 
> that is, you shouldn't get a read request that big.

Yes, the request is just for 16378 but gdbserver returns more.


> It looks like server.c:handle_qxfer's len caping is forgetting
> to account for the $, # and checksum (should be fixed), but I don't
> think that's the real cause in your example, since it only pushes back
> to gdb as much data as it requested.

Before starting to chase off-by-one here and off-by-one there what is the
practical purpose of such strict packet limits?

When TCP is in use shouldn't the code all around just support arbitrary packet
sizes?  To get rid any constant buffer sizes etc.  Then there still can remain
some vague fragmentation on the GDB client side but both sides should be able
to accept arbitrary packet sizes, shouldn't they?



Thanks,
Jan

Re: [patch] [gdbserver] Fix memory corruption

[Pedro Alves]

On Wednesday 02 March 2011 16:51:35, Jan Kratochvil wrote:
> On Wed, 02 Mar 2011 16:35:16 +0100, Pedro Alves wrote:
> > I can't reproduce it.
> 
> You may need also -lmcheck for gdbserver.

> Maybe your system does not have
> # cat /proc/*/cmdline|tr -cd '$#}*'|wc -c
> 84

Ah!  Yes, that's the reason.  Thanks.  I had tried valgrind before
and nothing complained.  Running a bunch of
"tail -f }}}}}}}}}}}}}}}}}}/a" processes helped.  :-)

> 
> > > So that putpkt_binary_1's cnt == 16383 will overrun PBUFSIZ 16384 by 4 bytes.
> > 
> > How did you get that large of a `cnt' in the first place?  The largest
> > I get is 16379.
> 
> remote_escape_output is called with OUT_MAXLEN == PBUFSIZ - 2 == 16382 and

> returns it +1, therefore 16383.
> #8  0x0000000000404ca3 in putpkt_binary_1 (buf=0x258f2c0 "mmn"..., cnt=16381, is_notif=0) at remote-utils.c:801
> 801	  free (buf2);
> 

> Yes, the request is just for 16378 but gdbserver returns more.

I see now.

> Before starting to chase off-by-one here and off-by-one there what is the
> practical purpose of such strict packet limits?

The remote protocol is designed to be implementable in tiny chips as
well, where you typically have a static buffer for the incoming packet
buffer.  malloc is a luxury you don't have in many of those scenarios.
So for outgoing packets, gdb needs to be careful about that.  For
incoming packets, gdb dynamically grows the buffer as it finds its
receiving larger packets.  So I think your patch is indeed okay.
I wouldn't mind a comment explaining the magic numbers, or replacing
them with 'strlen ("$#NN")' like in remote.c:

  /* Compute the size of the actual payload by subtracting out the
     packet header and footer overhead: "$M<memaddr>,<len>:...#nn".  */

  payload_size -= strlen ("$,:#NN");

> When TCP is in use shouldn't the code all around just support arbitrary packet
> sizes?  To get rid any constant buffer sizes etc.  Then there still can remain
> some vague fragmentation on the GDB client side but both sides should be able
> to accept arbitrary packet sizes, shouldn't they?

FYI, gdbserver works with rs232 as well.

-- 
Pedro Alves

Re: [patch] [gdbserver] Fix memory corruption

[Jan Kratochvil]

On Wed, 02 Mar 2011 19:00:45 +0100, Pedro Alves wrote:
> On Wednesday 02 March 2011 16:51:35, Jan Kratochvil wrote:
> > Before starting to chase off-by-one here and off-by-one there what is the
> > practical purpose of such strict packet limits?
> 
> The remote protocol is designed to be implementable in tiny chips as
> well, where you typically have a static buffer for the incoming packet
> buffer.  malloc is a luxury you don't have in many of those scenarios.
> So for outgoing packets, gdb needs to be careful about that.  For
> incoming packets, gdb dynamically grows the buffer as it finds its
> receiving larger packets.

But FSF gdbserver can receive arbitrarily large packets and allocate
everything dynamically.  It can also send arbitrarily large responses.
Thanks for the info, although for next updates, not this one.


> So I think your patch is indeed okay.
> I wouldn't mind a comment explaining the magic numbers, or replacing
> them with 'strlen ("$#NN")' like in remote.c:

Done.

Checked in.


Thanks,
Jan


http://sourceware.org/ml/gdb-cvs/2011-03/msg00106.html

--- src/gdb/gdbserver/ChangeLog	2011/03/06 07:40:52	1.465
+++ src/gdb/gdbserver/ChangeLog	2011/03/07 20:15:12	1.466
@@ -1,3 +1,7 @@
+2011-03-07  Jan Kratochvil  <jan.kratochvil@redhat.com>
+
+	* remote-utils.c (putpkt_binary_1): Calculate BUF2 size dynamically.
+
 2011-03-06  Yao Qi  <yao@codesourcery.com>
 
 	* Makefile.in (REQUIRED_SUBDIRS): Remove $(LIBCOMMON_DIR).
--- src/gdb/gdbserver/remote-utils.c	2011/01/25 10:09:19	1.84
+++ src/gdb/gdbserver/remote-utils.c	2011/03/07 20:15:12	1.85
@@ -725,7 +725,7 @@
   char *p;
   int cc;
 
-  buf2 = xmalloc (PBUFSIZ);
+  buf2 = xmalloc (strlen ("$") + cnt + strlen ("#nn") + 1);
 
   /* Copy the packet into buffer BUF2, encapsulating it
      and giving it a checksum.  */