This is the mail archive of the
gdb-patches@sourceware.cygnus.com
mailing list for the GDB project.
Re: RFA: PACKET_OVERHEAD constant added to remote.c
On Nov 17, 5:47am, Andrew Cagney wrote:
> Kevin Buettner wrote:
> >
> > On Nov 16, 1:20pm, Andrew Cagney wrote:
> >
> > > > As far as remote_write_bytes() or remote_read_bytes() are concerned,
> > > > they get their packet sizes by calling get_memory_write_packet_size()
> > > > or get_memory_read_packet_size() which in turn determine the size by
> > > > calling get_memory_packet_size(). It is the latter function which
> > > > was returning too large a value. It is also in this function where
> > > > I chose to make an adjustment:
> > >
> > > Yes. I'm just trying to understand if this is a recently introduced bug
> > > (by me) or has always been in there. (In particular prior to the change
> > > to remote.c below).
> >
> > Unclear. I was able to use an x86 linux gdbserver with gdb prior to
> > your change below (without seeing the buffer overflow problem). I
> > don't whether it was your change which caused the breakage or something
> > else.
>
> It is highly likely :-(
>
> > Let me know if you'd like me to pursue this; I could do an update
> > to Nov 3 and see if the problem still exists...
>
> If you can? Otherwize I'll try to find a linux box - I should probably
> look at the details.
Okay... I've pursued.
I think this problem has always existed... or it has at least as long
as we've been cutting the memory read packet size down to the size of
the remote registers.
The problem arises when the remote registers haven't been fetched yet
(via a g packet) and we try to read a large block of memory from the
remote. If this should happen, it is possible to request a packet
(from gdbserver in my case) which is larger than remote.c is prepared
to cope with. (If the remote registers haven't been fetched, we don't
know how large the remote's register buffer is and therefore we can't
restrict the size.)
It turns out that this was very difficult to reproduce. I first
noticed it when Jesper Skov alerted me to the fact that gdbserver was
broken (in devo) for Linux. gdbserver was not completely broken; it
just wouldn't fetch registers correctly. This led to an error
condition. (The _start breakpoint wasn't recognized and gdb saw a
SIGTRAP instead.) I've captured what gdb was trying to do with the
following backtrace...
Packet received: 010000006833014010a400400000000000000040
Sending packet: $m40013368,c8#cd...
Breakpoint 3, putpkt_binary (buf=0xbfffe730 "m40013368,c8", cnt=12)
at ../../../devo/gdb/remote.c:3701
3701 if (SERIAL_WRITE (remote_desc, buf2, p - buf2))
(top-gdb) bt
#0 putpkt_binary (buf=0xbfffe730 "m40013368,c8", cnt=12)
at ../../../devo/gdb/remote.c:3701
#1 0x809cde4 in putpkt (buf=0xbfffe730 "m40013368,c8")
at ../../../devo/gdb/remote.c:3650
#2 0x809cb29 in remote_read_bytes (memaddr=1073820520, myaddr=0x83cd8a4 "",
len=552) at ../../../devo/gdb/remote.c:3445
#3 0x80a00a4 in dcache_xfer_memory (dcache=0x83c7048, memaddr=1073820520,
myaddr=0x83cd8a4 "", len=552, should_write=0)
at ../../../devo/gdb/dcache.c:503
#4 0x809cc5e in remote_xfer_memory (mem_addr=1073820520, buffer=0x83cd8a4 "",
mem_len=552, should_write=0, target=0x833ed20)
at ../../../devo/gdb/remote.c:3503
#5 0x80adabb in target_xfer_memory (memaddr=1073820520, myaddr=0x83cd8a4 "",
len=552, write=0, bfd_section=0x0) at ../../../devo/gdb/target.c:846
#6 0x80ad9ee in target_read_memory (memaddr=1073820520, myaddr=0x83cd8a4 "",
len=552) at ../../../devo/gdb/target.c:784
#7 0x80b86aa in read_memory (memaddr=1073820520, myaddr=0x83cd8a4 "", len=552)
at ../../../devo/gdb/corefile.c:268
#8 0x8093fb3 in find_solib (so_list_ptr=0x0) at ../../../devo/gdb/solib.c:1048
#9 0x8094629 in solib_address (address=0) at ../../../devo/gdb/solib.c:1350
#10 0x804f7c2 in print_frame_info_base (fi=0x8341980, level=-1, source=1,
args=1) at ../../../devo/gdb/stack.c:652
#11 0x804f9a5 in print_frame_info (fi=0x8341980, level=-1, source=1, args=1)
at ../../../devo/gdb/stack.c:748
#12 0x804f0e8 in show_and_print_stack_frame_stub (args=0xbfffed68)
at ../../../devo/gdb/stack.c:158
#13 0x80c578b in catch_errors (
func=0x804f090 <show_and_print_stack_frame_stub>, args=0xbfffed68,
errstring=0x8277300 "", mask=3) at ../../../devo/gdb/top.c:588
#14 0x804f264 in show_and_print_stack_frame (fi=0x8341980, level=-1, source=1)
at ../../../devo/gdb/stack.c:256
#15 0x808029a in normal_stop () at ../../../devo/gdb/infrun.c:3466
...
Note that 0xc8 is 200 decimal which is half of 400. (Which is what
remote_packet_size starts out at if REGISTER_BYTES is too small.) This
is the value of remote_packet_size and half this number is the number
of bytes that gdb is requesting from gdbserver. Clearly this won't do
since the packet overhead has not been accounted for.
I've also been able to reproduce the problem with a working x86 linux
gdbserver as well. (I used a broken one to get the above trace.)
I used the following program:
--- sigtrap.c ---
char trap[] = { 0xcc };
main()
{
(*(void (*)()) trap) ();
}
--- end sigtrap.c ---
Then, in order to reproduce the problem, I start up gdbserver, start
up gdb, and then simply connect to gdbserver and continue. I.e, I'm
using the following commands:
set remotedebug 1
target remote ocotillo:11200
c
Let me know if you'd like to see a complete session.
> FYI, I suspect that rather than chop down the packet size by 32 bytes,
> it will be better to figure out what went wrong and fix that - 32 bytes
> less in a packet is a significant reduction.
I've looked at the code and I've thought about it and I really don't
see a better place to fix it. Note that I'm only chopping 32 bytes
off of remote_packet_size in order to set what_they_get in
get_memory_packet_size(). Buffer allocations still rely on an
unchopped remote_packet_size. Anyone calling get_memory_packet_size()
wants to know the amount that's safe to put into one of these buffers.
(It also tries to make sure that the target's buffers don't overflow
either.)
In addition, there are other pathways through get_memory_packet_size()
which avoid the chopping altogether. So if you explicitly set
memory-read-packet-size or memory-write-packet-size to something
really small, then 32 will not get chopped off. (Which is what I
think you were concerned about.)
To refresh your memory, here's what get_memory_packet_size looks like
after my patch has been applied:
static long
get_memory_packet_size (struct memory_packet_config *config)
{
/* NOTE: The somewhat arbitrary 16k comes from the knowledge (folk
law?) that some hosts don't cope very well with large alloca()
calls. Eventually the alloca() code will be replaced by calls to
xmalloc() and make_cleanups() allowing this restriction to either
be lifted or removed. */
#ifndef MAX_REMOTE_PACKET_SIZE
#define MAX_REMOTE_PACKET_SIZE 16384
#endif
/* NOTE: 16 is just chosen at random. */
#ifndef MIN_REMOTE_PACKET_SIZE
#define MIN_REMOTE_PACKET_SIZE 16
#endif
long what_they_get;
if (config->fixed_p)
{
if (config->size <= 0)
what_they_get = MAX_REMOTE_PACKET_SIZE;
else
what_they_get = config->size;
}
else
{
what_they_get = remote_packet_size - PACKET_OVERHEAD;
/* Limit the packet to the size specified by the user. */
if (config->size > 0
&& what_they_get > config->size)
what_they_get = config->size;
/* Limit it to the size of the targets ``g'' response. */
if (actual_register_packet_size > 0
&& what_they_get > actual_register_packet_size)
what_they_get = actual_register_packet_size;
}
if (what_they_get > MAX_REMOTE_PACKET_SIZE)
what_they_get = MAX_REMOTE_PACKET_SIZE;
if (what_they_get < MIN_REMOTE_PACKET_SIZE)
what_they_get = MIN_REMOTE_PACKET_SIZE;
return what_they_get;
}
Kevin