RFC: Run length encoding bug in remote.c?

RFC: Run length encoding bug in remote.c?

[Orjan Friberg]

I've had problems for a long time printing the task_struct pointer
"current" in our Linux kernel port.  The gdb stub in the kernel does
run-length encoding of the data it sends, which is used heavily when
printing the current struct since it contains lots of zeros.

After receiving the first g packet from the stub, host-gdb asks for
memory chunks of that size.  For my target, REGISTER_SIZE is 106 bytes,
which is 0x6a.  With 2 hex chars per byte that amounts to 212 bytes.

If I turn off run-length encoding in the stub, the first read of the
current struct results in the following communication:

Sending packet: $m600f6060,6a#f8...Ack
Packet received:
000000000f00000000000000000000000000000000000000000000000000000032000000000000000000000000000000706d0e60000000000000000000000000000000000040176000c0166000801660004016600000166000c01f6000001f6000000000000000000000

The packet received is 212 bytes like it should be.

Turning on run-length encoding the following happens:

Sending packet: $m600f6060,6a#f8...Ack
Repeat count 20 too large for buffer:
000000000f000000000000000000000000000000000000000000000000000000370000000000000000000000000000008c6d0e60000000000000000000000000000000000040176000c0166000801660004016600000166000c01f6000001f60

What is missing is the last 20 zeros from the previous example.  So, in
effect, what gdb claims doesn't fit in the buffer fits perfectly in
fact.

To me there seems to be a off-by-one error in the following snippet of
code (read_frame() in remote.c):

/* The character before ``*'' is repeated. */

if (repeat > 0 && repeat <= 255 
    && bc > 0
    && bc + repeat < sizeof_buf - 1)
  {
    memset (&buf[bc], buf[bc - 1], repeat);
    bc += repeat;
    continue;
  }

I suspect that the (bc + repeat) in the condition should in fact be (bc
+ repeat - 1) since the memset begins already at position bc in buf,
which means that the last position in buf to be memsetted is at position
(bc + repeat - 1).  *That* position must be < sizeof_buf - 1 to allow
room for null-termination of buf.

The patch below makes that change.  Though this works (tested with
"print *current" in gdb) and looks sensible to me, I have a feeling that
this error should show up quite often for other people as well, since it
would happen every time the response to an 'm' packet ends with
run-length encoding of the data.

Comments?


2001-08-08  Orjan Friberg  <orjanf@axis.com>

        * remote.c (read_frame): Correct off-by-one error in condition.


Index: remote.c
===================================================================
RCS file: /cvs/src/src/gdb/remote.c,v
retrieving revision 1.58
diff -c -3 -p -r1.58 remote.c
*** remote.c    2001/07/15 20:34:14     1.58
--- remote.c    2001/08/08 13:26:00
*************** read_frame (char *buf,
*** 4189,4195 ****
  
            if (repeat > 0 && repeat <= 255 
                && bc > 0
!                 && bc + repeat < sizeof_buf - 1)
              {
                memset (&buf[bc], buf[bc - 1], repeat);
                bc += repeat;
--- 4189,4195 ----
  
            if (repeat > 0 && repeat <= 255 
                && bc > 0
!                 && bc + repeat - 1 < sizeof_buf - 1)
              {
                memset (&buf[bc], buf[bc - 1], repeat);
                bc += repeat;

-- 
Orjan Friberg              E-mail: orjan.friberg@axis.com
Axis Communications AB     Phone:  +46 46 272 17 68

Re: RFC: Run length encoding bug in remote.c?

[Andrew Cagney]

> The patch below makes that change.  Though this works (tested with
> "print *current" in gdb) and looks sensible to me, I have a feeling that
> this error should show up quite often for other people as well, since it
> would happen every time the response to an 'm' packet ends with
> run-length encoding of the data.


I don't know that many targets use run-length incoding and hence, 
probably few have noticed the bug.

> I suspect that the (bc + repeat) in the condition should in fact be (bc
> + repeat - 1) since the memset begins already at position bc in buf,
> which means that the last position in buf to be memsetted is at position
> (bc + repeat - 1).  *That* position must be < sizeof_buf - 1 to allow
> room for null-termination of buf.


Yes, I'm convinced - my walk through came up with the assertion ``bc + 
repeat <= sizeof_buf - 1'' which is equivalent to your patch.

So approved.

	Andrew

Re: RFC: Run length encoding bug in remote.c?

[Daniel Jacobowitz]

On Fri, Aug 10, 2001 at 03:11:25AM -0400, Andrew Cagney wrote:
> > The patch below makes that change.  Though this works (tested with
> > "print *current" in gdb) and looks sensible to me, I have a feeling that
> > this error should show up quite often for other people as well, since it
> > would happen every time the response to an 'm' packet ends with
> > run-length encoding of the data.
> 
> 
> I don't know that many targets use run-length incoding and hence, 
> probably few have noticed the bug.

For reference, and for amusement at the synchronicity, we ran into this
again today on a Hitachi SH firmware stub; I'll second the patch.

-- 
Daniel Jacobowitz                           Carnegie Mellon University
MontaVista Software                         Debian GNU/Linux Developer

Re: RFC: Run length encoding bug in remote.c?

[Andrew Cagney]

> On Fri, Aug 10, 2001 at 03:11:25AM -0400, Andrew Cagney wrote:
> 
>> > The patch below makes that change.  Though this works (tested with
>> > "print *current" in gdb) and looks sensible to me, I have a feeling that
>> > this error should show up quite often for other people as well, since it
>> > would happen every time the response to an 'm' packet ends with
>> > run-length encoding of the data.
> 
>> 
>> 
>> I don't know that many targets use run-length incoding and hence, 
>> probably few have noticed the bug.
> 
> 
> For reference, and for amusement at the synchronicity, we ran into this
> again today on a Hitachi SH firmware stub; I'll second the patch.


In that case, it should probably also go on the 5.1 branch.

	Andrew

Re: RFC: Run length encoding bug in remote.c?

[Orjan Friberg]

Andrew Cagney wrote:
> 
> > On Fri, Aug 10, 2001 at 03:11:25AM -0400, Andrew Cagney wrote:
> >
> >> > The patch below makes that change.  Though this works (tested with
> >> > "print *current" in gdb) and looks sensible to me, I have a feeling that
> >> > this error should show up quite often for other people as well, since it
> >> > would happen every time the response to an 'm' packet ends with
> >> > run-length encoding of the data.
> >
> >>
> >>
> >> I don't know that many targets use run-length incoding and hence,
> >> probably few have noticed the bug.
> >
> >
> > For reference, and for amusement at the synchronicity, we ran into this
> > again today on a Hitachi SH firmware stub; I'll second the patch.
> 
> In that case, it should probably also go on the 5.1 branch.
> 
>         Andrew

Ok, committed to both trunk and branch.  Thanks.

-- 
Orjan Friberg              E-mail: orjan.friberg@axis.com
Axis Communications AB     Phone:  +46 46 272 17 68