Re: RFA: PACKET_OVERHEAD constant added to remote.c

[Andrew Cagney <ac131313 at cygnus dot com>]

Kevin Buettner wrote:
> 
> 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.

Ah, I feel much better. Much thanks!

> 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...

FYI, I've a number of short programs that pretend to speak GDB protocol.
Very useful for this sort of testing.

> Let me know if you'd like to see a complete session.

No this is more than sufficient, much thanks.

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

What about the following:

o	Add a buffer size parameter to getpkt() so that it
	can check (after being fixed) for buffer overflows.

o	reduce the 400 to 399 (or 398?).
	Have a look at m68k-stub.c for why. Outch!

o	fix the *_read_bytes() code so that it allocates
	a buffer that includes space for the trailing
	NUL.

I can see my way through them in the next few days (unless you beat me
to it :-).

	Andrew