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

Submitter:  Ben Hastings <hastings>
Submitted:  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>
Group 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>
Group 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>
Group 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>
Group 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>
Group 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>
Group 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>
Group 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>

 

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

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

    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 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.13-3230.
    Corresponding source code