buglwIP - A Lightweight TCP/IP stack - Bugs: bug #26301, Out of order FIN closes...

 
 

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

bug #26301: Out of order FIN closes half-closed connection

Submitted by:  Ben Hastings <hastings>
Submitted on:  Wed 22 Apr 2009 11:02:05 PM UTC  
 
Category:  TCP Severity:  3 - Normal
Item Group:  Faulty Behaviour Status:  Fixed
Privacy:  Public Assigned to:  None
Open/Closed:  Closed Planned Release:  None
lwIP version:  CVS Head

( Jump to the original submission)

Thu 25 Jun 2009 09:58:45 AM UTC, comment #14: 

I've checked in some code which I hope should fix this, but I would welcome a second opinion if you're able to test it.

Kieran Mansley <kieranm>
Project Member
Thu 30 Apr 2009 04:19:42 PM UTC, comment #13: 

In previous patch I moved tcplen calculation. It was wrong.
Now corrected patch.

(file #18063)

Oleg Tyshev <olegreen>
Fri 24 Apr 2009 12:15:44 PM UTC, comment #12: 

I hope to find some time to look at both of these bugs.

Kieran Mansley <kieranm>
Project Member
Fri 24 Apr 2009 11:31:17 AM UTC, comment #11: 

If somebody want patch together with
changes for bug #26267 (snd_nxt replaced with snd_max)
Actually it is one error.

(file #18007)

Oleg Tyshev <olegreen>
Thu 23 Apr 2009 05:39:07 PM UTC, comment #10: 

Sorry, I clicked the wrong bug lol

Simon Goldschmidt <goldsimon>
Project Administrator
Thu 23 Apr 2009 05:38:00 PM UTC, comment #9: 

Since this is definitively not our bug, I'm closing it as invalid.

As to struct copying: I think the current implementation is good enough, but we could discuss about checking where we use it (especially regarding pointer casting).

Simon Goldschmidt <goldsimon>
Project Administrator
Thu 23 Apr 2009 01:16:24 PM UTC, comment #8: 

It works for me.
See applied patch fin_in_seq.diff

(file #18000)

Oleg Tyshev <olegreen>
Thu 23 Apr 2009 12:20:03 PM UTC, comment #7: 

It should work better here and other places.

case FIN_WAIT_1:
tcp_receive(pcb);
-if (flags & TCP_FIN) {
+if (recv_flags == TF_GOT_FIN) {

Flag TF_GOT_FIN means that FIN segment was received in sequence.
I'll try it and say about results.

Oleg Tyshev <olegreen>
Thu 23 Apr 2009 11:51:09 AM UTC, comment #6: 

and may be we can call tcp_update_rcv_ann_wnd() only once,
when inseg segment processed and oos queue processed.

Oleg Tyshev <olegreen>
Thu 23 Apr 2009 11:32:25 AM UTC, comment #5: 

Agree with you,
replace with
pcb->rcv_nxt = seqno + tcplen;

and may be we can move string
where recalculated length of trimmed segment

tcplen = TCP_TCPLEN(&inseg);

to the place where it was trimmed

/* We have to trim the second edge of the incoming
segment. */
inseg.len = (u16_t)(pcb->ooseq->tcphdr->seqno - seqno);
pbuf_realloc(inseg.p, inseg.len);
+tcplen = TCP_TCPLEN(&inseg);

Oleg Tyshev <olegreen>
Thu 23 Apr 2009 09:40:54 AM UTC, comment #4: 

The code came from Kelvin Lawson in December 2005:

http://lists.nongnu.org/archive/html/lwip-users/2005-12/msg00034.html

I don't understand how that code could make any difference either: if the received packet is a duplicate it wouldn't get there as its seqno would not be equal to rcv_nxt.

Kieran Mansley <kieranm>
Project Member
Thu 23 Apr 2009 09:26:50 AM UTC, comment #3: 

Reply to comment #2

It looks good, but I don't understand why this code for checking
if (pcb->state != CLOSE_WAIT) {
was written early.

For this code condition
if (pcb->rcv_nxt == seqno) {
is already true.

It means
pcb->rcv_nxt += tcplen;
equal to
pcb->rcv_nxt = seqno + tcplen;

Oleg Tyshev <olegreen>
Thu 23 Apr 2009 08:18:47 AM UTC, comment #2: 

As we know we're dealing with an in sequence packet at that point, could we not replace with:

    pcb->rcv_nxt = seqno + tcplen;

This would work for all states I think.  Any duplicate FINs would not come here as they'd be out of the window. 

Kieran Mansley <kieranm>
Project Member
Thu 23 Apr 2009 08:10:25 AM UTC, comment #1: 

This was also mentioned in BUG #26267.

I think there might be something to fix in tcp_receive() too:

        /* First received FIN will be ACKed +1, on any successive (duplicate)
         * FINs we are already in CLOSE_WAIT and have already done +1.
         */
        if (pcb->state != CLOSE_WAIT) {
          pcb->rcv_nxt += tcplen;
        }

I think we should do the same for the CLOSING state where we have also already received a FIN.

Kieran Mansley <kieranm>
Project Member
Wed 22 Apr 2009 11:02:05 PM UTC, original submission:  

After lwip initiates closing a connection, an out-of-order FIN will transition the state from FIN_WAIT_1 or FIN_WAIT_2 to TIME_WAIT.  Any retransmitted segments that arrive after the FIN are then ACK'ed but never delivered to the application.

The following patch for tcp_in.c (1.3.0-STABLE) uses the same transition criteria as the ESTABLISHED state, and appears to fix the problem.

@@ -640,8 +640,8 @@
     }
     break;
   case FIN_WAIT_1:
-    tcp_receive(pcb);
-    if (flags & TCP_FIN) {
+    accepted_inseq = tcp_receive(pcb);
+    if ((flags & TCP_FIN) && accepted_inseq) {
       if (flags & TCP_ACK && ackno == pcb->snd_nxt) {
         LWIP_DEBUGF(TCP_DEBUG,
           ("TCP connection closed %"U16_F" -> %"U16_F".\n", inseg.tcphdr->src, inseg.tcphdr->dest));
@@ -659,8 +659,8 @@
     }
     break;
   case FIN_WAIT_2:
-    tcp_receive(pcb);
-    if (flags & TCP_FIN) {
+    accepted_inseq = tcp_receive(pcb);
+    if ((flags & TCP_FIN) && accepted_inseq) {
       LWIP_DEBUGF(TCP_DEBUG, ("TCP connection closed %"U16_F" -> %"U16_F".\n", inseg.tcphdr->src, inseg.tcphdr->dest));
       tcp_ack_now(pcb);
       tcp_pcb_purge(pcb);

Ben Hastings <hastings>

 

Attached Files
file #18063:  fin_in_seq3.diff added by olegreen (4KiB - application/octet-stream)
file #18007:  fin_in_seq2.diff added by olegreen (4KiB - application/octet-stream)
file #18000:  fin_in_seq.diff added by olegreen (2KiB - application/octet-stream)

 

Depends on the following items: None found

Items that depend on this one: None found

 

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

    Do you think this task is very important?
    If so, you can add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

     

     

    Follow 13 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2009-07-27 kieranm StatusReady For Test => Fixed
        Open/ClosedOpen => Closed
    2009-06-25 kieranm StatusNone => Ready For Test
    2009-05-01 kieranm Planned Release => 1.3.1
    2009-04-30 olegreen Attached File- => Added fin_in_seq3.diff, #18063
    2009-04-24 olegreen Attached File- => Added fin_in_seq2.diff, #18007
    2009-04-23 goldsimon StatusInvalid => None
        Assigned togoldsimon => None
        Open/ClosedClosed => Open
    2009-04-23 goldsimon StatusNone => Invalid
        Assigned toNone => goldsimon
        Open/ClosedOpen => Closed
    2009-04-23 olegreen Attached File- => Added fin_in_seq.diff, #18000

    Back to the top


    Powered by Savane 3.5