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: TCPSeverity: 3 - Normal
Item Group: Faulty BehaviourStatus: Fixed
Privacy: PublicAssigned to: None
Open/Closed: ClosedPlanned Release: None
lwIP version: CVS Head

(Jump to the original submission 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 Administrator
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 Administrator
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 Administrator
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 Administrator
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 Administrator
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
  • -unavailable- added by goldsimon (Posted a comment)
  • -unavailable- added by olegreen (Posted a comment)
  • -unavailable- added by kieranm (Posted a comment)
  • -unavailable- added by hastings (Submitted the item)
  •  

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

    Only logged-in users can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 13 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Mon 27 Jul 2009 02:03:51 PM UTCkieranmStatusReady For Test=>Fixed
      Open/ClosedOpen=>Closed
    Thu 25 Jun 2009 09:58:45 AM UTCkieranmStatusNone=>Ready For Test
    Fri 01 May 2009 12:39:59 PM UTCkieranmPlanned Release=>1.3.1
    Thu 30 Apr 2009 04:19:42 PM UTColegreenAttached File-=>Added fin_in_seq3.diff, #18063
    Fri 24 Apr 2009 11:31:17 AM UTColegreenAttached File-=>Added fin_in_seq2.diff, #18007
    Thu 23 Apr 2009 05:39:07 PM UTCgoldsimonStatusInvalid=>None
      Assigned togoldsimon=>None
      Open/ClosedClosed=>Open
    Thu 23 Apr 2009 05:38:00 PM UTCgoldsimonStatusNone=>Invalid
      Assigned toNone=>goldsimon
      Open/ClosedOpen=>Closed
    Thu 23 Apr 2009 01:16:24 PM UTColegreenAttached File-=>Added fin_in_seq.diff, #18000

    Back to the top


    Powered by Savane 3.1-cleanup1