buglwIP - A Lightweight TCP/IP stack - Bugs: bug #28241, oos processing

 
 

You are not allowed to post comments on this tracker with your current authentication level.

bug #28241: oos processing

Submitter:  Oleg Tyshev <olegreen>
Submitted:  Wed 09 Dec 2009 03:36:48 PM UTC
   
 
Category:  TCP Severity:  3 - Normal
Item Group:  Change Request Status:  Fixed
Privacy:  Public Assigned to:  goldsimon
Open/Closed:  Closed Planned Release:  None
lwIP version:  CVS Head

Jump to the original submission

Fri 08 Jan 2010 03:11:21 PM UTC, comment #17: 

Closing this as it doesn't seem too important (for an extra bugfix release).

Simon Goldschmidt <goldsimon>
Group administrator
Sun 27 Dec 2009 11:20:22 AM UTC, comment #16: 

Unfortunately, there's a bug in the patch from comment #14 which has made it into the 1.3.2 release:

In tcp_in.c:tcp_receive(), line 1210 (and 1217 for the assertion),

next->tcphdr->seqno

must be used instead of

pcb->ooseq->tcphdr->seqno

One could argue if this happens often (incoming segment being greater than the first on ooseq and partly overlapping the second on ooseq) as most often, retransmitted segments will be the same as before, so I don't know how serious this bug is.

Still, running the corresponding unit test once showed that there was a bug...

Oh, and about that "a bit hard to follow what it's changing and why": I'll add some comments there, I think that's easier to read than the graphics.

I'll check it in but leave this open to discuss what to do about this bug regarding the release.

Simon Goldschmidt <goldsimon>
Group administrator
Thu 24 Dec 2009 03:30:42 PM UTC, comment #15: 

I'm committing both these patches (with a few modifications, references to next->mpTcpHdr for example), although I must confess the second one is a bit hard to follow what it's changing and why.

Resolving this bug fixed.

Kieran Mansley <kieranm>
Group Member
Tue 15 Dec 2009 01:11:59 PM UTC, comment #14: 

2nd patch for processing oos queue after receiving in-sequence segment

(Now in-sequence segment doesn't delete segments from oos queue)


(file #19272)

Oleg Tyshev <olegreen>
Fri 11 Dec 2009 01:15:29 PM UTC, comment #13: 

Oh, that makes sense... in that case, the check should be included or we lose the FIN - it might be retransmitted, but that's inefficient.

In any case: the current code doesn't have this problem. I'd still check this in now, I've also locally adjusted the tests to verify it works.

Simon Goldschmidt <goldsimon>
Group administrator
Fri 11 Dec 2009 11:18:34 AM UTC, comment #12: 

Ahh, now I understand.  The length being used in the comparison (cseg->len) is the payload length, not the sequence length, and so does not include any FIN bits.

Oleg is right, we do therefore need to check whether there is a FIN byte in the old packet before discarding it, and move that byte to the new packet if the new packet does not also have a FIN.

Kieran Mansley <kieranm>
Group Member
Fri 11 Dec 2009 11:03:28 AM UTC, comment #11: 

It is another question, what should we do when we have data behind the FIN,
but I mean here length of data

....45 - length of data 2 + FIN flag
...345 - length of data 3, but without FIN flag

tcp length of both packets is 3,
but seqno of 1st is 4 and 2nd is 3.

In this case we doesn't have contradiction.

Oleg Tyshev <olegreen>
Fri 11 Dec 2009 10:52:12 AM UTC, comment #10: 

The other end is wrong and we can do what we like.  Seqno 5 in the old packet was a FIN, seqno 5 in the new packet is not a FIN.  Which is correct?  We should probably reset the connection in this case.

Kieran Mansley <kieranm>
Group Member
Fri 11 Dec 2009 10:45:34 AM UTC, comment #9: 

May be I have made not correct description

old -
seqno = 4
len   = 2
flag  = FIN

new -
seqno = 3
len   = 3

result -
seqno = 3
len   = 3
flag  = FIN

Oleg Tyshev <olegreen>
Fri 11 Dec 2009 10:36:29 AM UTC, comment #8: 

The FIN uses a byte of sequence space, so if two packets finish on the same sequence byte and one has a FIN and the other does not, that is wrong.  I'm happy with the change as Simon has made it.

Kieran Mansley <kieranm>
Group Member
Fri 11 Dec 2009 10:30:32 AM UTC, comment #7: 

You are right in 4th comment.
It should be
 TCPH_FLAGS_SET(cseg->tcphdr, TCPH_FLAGS(cseg->tcphdr) | TCP_FIN);

All cases are not covered with 1st line
if (TCPH_FLAGS(cseg->tcphdr) & TCP_FIN) {

This is neccesary for case when old segment has FIN flag but
new one - not.

e.g.
old - (4-5)FIN
new - (3-5)
result (3-5)FIN

Oleg Tyshev <olegreen>
Thu 10 Dec 2009 05:37:49 PM UTC, comment #6: 

After all, the extra FIN check is not at all necessary, since that case should already be covered by the first line in tcp_oos_insert_segment():

if (TCPH_FLAGS(cseg->tcphdr) & TCP_FIN) {

I've extended the ooseq unit test and it shows that the suggested patch works: data is delivered in correct order and the number of segments on ooseq is sometimes smaller than without the patch.

Simon Goldschmidt <goldsimon>
Group administrator
Thu 10 Dec 2009 03:51:09 PM UTC, comment #5: 

Anyway, I would have expected it to work without the extra check for the FIN flag as that should already be covered by comparing the sequence numbers.

Simon Goldschmidt <goldsimon>
Group administrator
Thu 10 Dec 2009 02:27:52 PM UTC, comment #4: 

That's true, I would have expected somthing like:
TCPH_FLAGS_SET(cseg->tcphdr, TCPH_FLAGS(cseg->tcphdr) | TCP_FIN);

Simon Goldschmidt <goldsimon>
Group administrator
Thu 10 Dec 2009 10:46:25 AM UTC, comment #3: 

I can see what you mean with the GT -> GEQ change, but I don't understand that FIN change:

+ /* cseg with FIN already processed */
+ if (TCPH_FLAGS(next->mpTcpHdr) & TCP_FIN) {
+ TCPH_FLAGS_SET(cseg->tcphdr, TCPH_FLAGS(cseg->tcphdr) & ~TCP_FIN);
+ }

This reads to me as:
 if (old packet has FIN set) {
   remove FIN from new packet's flags;
 }

Can you explain why you'd want to do that?

Kieran Mansley <kieranm>
Group Member
Thu 10 Dec 2009 10:04:26 AM UTC, comment #2: 

Suppuse we have rcv_nxt==0

1st fragment
seqno=2
len=2
(2-3)
It is queued into oos.

2nd fragment
seqno=1
len=3
(1-3)

Without change we have on oos queue two fragments
(1-1)(2-3)
with change only
(1-3)




Oleg Tyshev <olegreen>
Wed 09 Dec 2009 07:36:11 PM UTC, comment #1: 


> In function tcp_oos_insert_segment()
> some segments covered with new segment are deleted.


I thought that was what we wanted to do with our last changes? Can you go into detail a bit more on what's bad here?

Simon Goldschmidt <goldsimon>
Group administrator
Wed 09 Dec 2009 03:36:48 PM UTC, original submission:  

In function tcp_oos_insert_segment()
some segments covered with new segment are deleted.
It would be better instead of

    while (next &&
           TCP_SEQ_GT((seqno + cseg->len),
                      (next->tcphdr->seqno + next->len))) {
      struct tcp_seg *old_seg = next;
      next = next->next;
      tcp_seg_free(old_seg);
    }

write

     while (next &&
-           TCP_SEQ_GT((seqno + cseg->len),
+           TCP_SEQ_GEQ((seqno + cseg->len),
                       (next->tcphdr->seqno + next->len))) {
       struct tcp_seg *old_seg = next;
       next = next->next;
       tcp_seg_free(old_seg);
     }



or if we don't want to lose FIN flag

     while (next &&
-           TCP_SEQ_GT((seqno + cseg->len),
+           TCP_SEQ_GEQ((seqno + cseg->len),
                       (next->tcphdr->seqno + next->len))) {
+      /* cseg with FIN already processed */
+      if (TCPH_FLAGS(next->mpTcpHdr) & TCP_FIN) {
+        TCPH_FLAGS_SET(cseg->tcphdr, TCPH_FLAGS(cseg->tcphdr) & ~TCP_FIN);
+      }
       struct tcp_seg *old_seg = next;
       next = next->next;
       tcp_seg_free(old_seg);
     }


Oleg Tyshev <olegreen>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attached Files
file #19272:  oos_ins2.patch added by olegreen (3KiB - text/x-patch)
file #19226:  oos_ins.patch added by olegreen (790B - text/x-patch)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by kieranm (Posted a comment)
  • -email is unavailable- added by goldsimon (Posted a comment)
  • -email is unavailable- added by olegreen (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

     

    Follow 9 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2010-01-08 goldsimon StatusIn Progress Fixed
        Assigned toNone goldsimon
        Open/ClosedOpen Closed
    2009-12-27 goldsimon StatusFixed In Progress
        Open/ClosedClosed Open
    2009-12-24 kieranm StatusNone Fixed
        Open/ClosedOpen Closed
    2009-12-15 olegreen Attached File- Added oos_ins2.patch, #19272
    2009-12-09 olegreen Attached File- Added oos_ins.patch, #19226

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code