Re: [patch] [gdbserver] Fix memory corruption

[Pedro Alves <pedro at codesourcery dot com>]

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