buglwIP - A Lightweight TCP/IP stack - Bugs: bug #20779, Keep-Alive and SYNs

 
 

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

bug #20779: Keep-Alive and SYNs

Submitter:  Oleg Tyshev <olegreen>
Submitted:  Tue 14 Aug 2007 09:43:55 AM UTC
   
 
Category:  TCP Severity:  3 - Normal
Item Group:  None Status:  Fixed
Privacy:  Public Assigned to:  kieranm
Open/Closed:  Closed Planned Release:  None
lwIP version:  1.3.0

Jump to the original submission

Tue 31 Mar 2009 10:56:14 AM UTC, comment #17: 

OK, fix checked in.  Hopefully this will get some testing now.  Resolving as fixed.

Kieran Mansley <kieranm>
Group Member
Thu 26 Mar 2009 04:01:35 PM UTC, comment #16: 

It looks good now.
Unfortunately I can't test it.

Oleg Tyshev <olegreen>
Thu 26 Mar 2009 02:15:56 PM UTC, comment #15: 

OK, how about this?

diff -r 163ba0f0bd9a core/tcp_in.c
--- a/core/tcp_in.c
+++ b/core/tcp_in.c
@@ -536,6 +536,12 @@ tcp_process(struct tcp_pcb *pcb)
     }
   }
 
+  if ((flags & TCP_SYN) && (pcb->state != SYN_SENT && pcb->state != SYN_RCVD)) {
+    /* Cope with new connection attempt after remote end crashed */
+    tcp_ack_now(pcb);
+    return ERR_OK;
+  }

   /* Update the PCB (in)activity timer. */
   pcb->tmr = tcp_ticks;
   pcb->keep_cnt_sent = 0;
@@ -597,8 +603,7 @@ tcp_process(struct tcp_pcb *pcb)
     }
     break;
   case SYN_RCVD:
-    if (flags & TCP_ACK &&
-       !(flags & TCP_RST)) {
+    if (flags & TCP_ACK) {
       /* expected ACK number? */
       if (TCP_SEQ_BETWEEN(ackno, pcb->lastack+1, pcb->snd_nxt)) {
         u16_t old_cwnd;
@@ -633,6 +638,9 @@ tcp_process(struct tcp_pcb *pcb)
         tcp_rst(ackno, seqno + tcplen, &(iphdr->dest), &(iphdr->src),
                 tcphdr->dest, tcphdr->src);
       }
+    } else if ((flags & TCP_SYN) && (seqno == pcb->rcv_nxt - 1)) {
+      /* Looks like another copy of the SYN - retransmit our SYN-ACK */
+      tcp_rexmit(pcb);
     }
     break;
   case CLOSE_WAIT:

Kieran Mansley <kieranm>
Group Member
Wed 25 Mar 2009 05:21:47 PM UTC, comment #14: 

That's  OK.
But it seems to me, that in SYN_RCVD state we should reply to TCP_SYN flag and the same sequence number with resend SYN_ACK (this packet is in unacked queue).

Oleg Tyshev <olegreen>
Wed 25 Mar 2009 02:03:50 PM UTC, comment #13: 

How's this for a patch to solve this problem:

Index: tcp_in.c
===================================================================
RCS file: /sources/lwip/lwip/src/core/tcp_in.c,v
retrieving revision 1.102
diff -u -r1.102 tcp_in.c
--- tcp_in.c 19 Dec 2008 18:08:30 -0000 1.102
+++ tcp_in.c 25 Mar 2009 13:46:59 -0000
@@ -531,6 +535,12 @@
     }
   }
 
+  if ((flags & TCP_SYN) && (pcb->state != SYN_SENT && pcb->state != SYN_RCVD)) {
+    /* Cope with new connection attempt after remote end crashed */
+    tcp_ack_now(pcb);
+    return ERR_OK;
+  }

   /* Update the PCB (in)activity timer. */
   pcb->tmr = tcp_ticks;
   pcb->keep_cnt_sent = 0;

Kieran Mansley <kieranm>
Group Member
Fri 09 Nov 2007 01:04:02 PM UTC, comment #12: 

Sounds reasonable.  However, I'm going to drop this from gating 1.3.0 unless anyone can provide a complete patch.

Kieran Mansley <kieranm>
Group Member
Mon 10 Sep 2007 01:29:51 PM UTC, comment #11: 

Of couкse, I mean only synchronized states where we don't await SYN.

Oleg Tyshev <olegreen>
Mon 10 Sep 2007 01:24:19 PM UTC, comment #10: 

I think, that in tcp_process() should be added SYN check for any state.
tcp_receive() is corect now only if sequence number of the packet outside of the receive window. Otherwise it could be queued into out of sequence queue or processed as data (by us tcplen = p->tot_len + ((flags & TCP_FIN || flags & TCP_SYN)? 1: 0);)

for example instead

  case CLOSE_WAIT:
    /* FALLTHROUGH */
  case ESTABLISHED:
    accepted_inseq = tcp_receive(pcb);
    if ((flags & TCP_FIN) && accepted_inseq) { /* passive close */
      tcp_ack_now(pcb);
      pcb->state = CLOSE_WAIT;
    }
    break;

would be more correct following code

  case CLOSE_WAIT:
    /* FALLTHROUGH */
  case ESTABLISHED:
    if (flags & TCP_SYN) {
      tcp_ack_now(pcb);
    }
    else {
      accepted_inseq = tcp_receive(pcb);
      if ((flags & TCP_FIN) && accepted_inseq) { /* passive close */
        tcp_ack_now(pcb);
        pcb->state = CLOSE_WAIT;
      }
    }
    break;

Oleg Tyshev <olegreen>
Wed 29 Aug 2007 09:33:01 AM UTC, comment #9: 

Sorry, since I'm not TCP expert, I can't propose better than my comment #6 solution (for the moment ;) )

Frédéric Bernon <fbernon>
Group Member
Tue 14 Aug 2007 02:09:06 PM UTC, comment #8: 

Stats is OK.

We should not drop SYN silently,
lwip should reply and replies with old SEQ_NO and old ACK.
It is correct.
(RFC 793, 3.4, Figure 10 Half-Open Connection Discovery)

The question is now only about (in)activity timer.
Should it be updated on discarded packets or not.

RFC 1122 - 4.2.3.6 TCP Keep-Alives
Keep-alive packets MUST only be sent when no data or acknowledgement packets have been received for the connection within an interval.

Does SYN packet data or not?

And second,
variable tcplen for SYN packet is 1,
if SEQ_NO of SYN is inside receive window of old pcb,
it would be processed in
tcp_receive and added to out of sequence queue.

Small fix to tcp_in ver 1.74.
Lines 1214, 1216 is possible to delete without any loss in logic.

Oleg Tyshev <olegreen>
Tue 14 Aug 2007 01:31:13 PM UTC, comment #7: 

Re comment #3: a new SYN should not kill an existing connection surely! As per the bit of RFC793 that Oleg quoted, it should only be killed by a RST.

You can find what Oleg is quoting on page 33 here: http://www.faqs.org/rfcs/rfc793.html

Jonathan Larmour <jifl>
Group Member
Tue 14 Aug 2007 12:28:27 PM UTC, comment #6: 

No objects to add these errors stats? (and yes, I know, it's not directly a patch for this bug report, sorry)

Index: src/core/tcp_in.c
===================================================================
RCS file: /sources/lwip/lwip/src/core/tcp_in.c,v
retrieving revision 1.74
diff -w -b -u -0 -p -r1.74 tcp_in.c
--- src/core/tcp_in.c 9 Aug 2007 22:21:45 -0000 1.74
+++ src/core/tcp_in.c 14 Aug 2007 12:24:42 -0000
@@ -117,0 +118 @@ tcp_input(struct pbuf p, struct netif
+    snmp_inc_tcpinerrs();
@@ -124,0 +126,2 @@ tcp_input(struct pbuf p, struct netif
+    TCP_STATS_INC(tcp.proterr);
+    TCP_STATS_INC(tcp.drop);
@@ -156,0 +160 @@ tcp_input(struct pbuf p, struct netif
+    snmp_inc_tcpinerrs();

Frédéric Bernon <fbernon>
Group Member
Tue 14 Aug 2007 12:17:39 PM UTC, comment #5: 

if ((flags & TCP_SYN) && (pcb->state != LISTEN) && (pcb->state != SYN_SENT)) {

Finally, if I understand the TCP/IP State Transition Diagram (RFC793)... If any TCP expert can confirm...

Frédéric Bernon <fbernon>
Group Member
Tue 14 Aug 2007 12:13:15 PM UTC, comment #4: 

if ((flags & TCP_SYN) && (pcb->state != LISTEN) && (pcb->state != SYN_RCVD) && (pcb->state != SYN_SENT)) {

should be better...

Frédéric Bernon <fbernon>
Group Member
Tue 14 Aug 2007 12:06:22 PM UTC, comment #3: 

If the problem is due to tcp_process's lines you give, I think the the filter should be done in tcp_input to avoid to "tcp_process" the old pcb. Perhaps do something like:

if ((flags & TCP_SYN) && (pcb->state != LISTEN)) {
  /* drop SYN packets on non-LISTEN pcb */
  TCP_STATS_INC(tcp.chkerr);
  TCP_STATS_INC(tcp.drop);
  snmp_inc_tcpinerrs();
  pbuf_free(p);
  return;
}

I see that in tcp_in.c, in tcp_input(), lines ~260 (of last cvs head file).

But perhaps a better solution will be to abord the "old" connection? I don't know if there is any RFC requirements on that point...

Other point, it seems that we should add some "snmp_inc_tcpinerrs();" after each "TCP_STATS_INC(tcp.drop);", and "TCP_STATS_INC(tcp.drop);" in the "/* Don't even process incoming broadcasts/multicasts. */" part...

Frédéric Bernon <fbernon>
Group Member
Tue 14 Aug 2007 11:55:36 AM UTC, comment #2: 

This case is described in RFC 793:

      TCP A                                           TCP B

  1.  (CRASH)                               (send 300,receive 100)

  2.  CLOSED                                           ESTABLISHED

  3.  SYN-SENT --> <SEQ=400><CTL=SYN>              --> (??)

  4.  (!!)     <-- <SEQ=300><ACK=100><CTL=ACK>     <-- ESTABLISHED

  5.  SYN-SENT --> <SEQ=100><CTL=RST>              --> (Abort!!)

  6.  SYN-SENT                                         CLOSED

  7.  SYN-SENT --> <SEQ=400><CTL=SYN>              -->

Half-Open Connection Discovery
Figure 10.

Lwip replies correct, but without analyse SYN flag.
The question about activity timer remains open.

Oleg Tyshev <olegreen>
Tue 14 Aug 2007 09:49:30 AM UTC, comment #1: 

May be I too narrow described the problem.
(In)activity timer should be uppdated only if packet accepted and processed correctly.

Oleg Tyshev <olegreen>
Tue 14 Aug 2007 09:43:55 AM UTC, original submission:  

Hello All,

I will try to describe scenario

- Server listens on port 100, keep-alive is on.
- Client does connect to server from port 200. (Not ephemeral port, always 200)
- Successful connect
- No data exchange
...
- Client - power off
- Client - power on
- Client tries again to connect from port 200 to port 100. (SYNs)
- Server sees activity on connection and keep-alive expires never.

I have seen to tcp_process and tcp_receive in TCP state ESTABLISHED.
There is no check TCP_SYN flag.
I think it should be silently discarded, but without modification (in)activity timer
(in tcp_process()
  /* Update the PCB (in)activity timer. */
  pcb->tmr = tcp_ticks;
  pcb->keep_cnt_sent = 0;)
It is discarded now too, but only due to sequence number without analyze TCP_SYN flag.

Oleg Tyshev

Oleg Tyshev <olegreen>

 

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

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by kieranm (Updated the item)
  • -email is unavailable- added by jifl (Posted a comment)
  • -email is unavailable- added by fbernon (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 8 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2009-03-31 kieranm StatusNone Fixed
        Open/ClosedOpen Closed
    2008-03-25 kieranm Assigned toNone kieranm
    2008-03-04 kieranm Planned Release 1.3.1
    2007-11-09 kieranm Severity4 - Important 3 - Normal
        Planned Release1.3.0
    2007-08-29 kieranm Severity3 - Normal 4 - Important
        Planned Release 1.3.0

    Back to the top

    Powered by Savane 3.13-d3ae.
    Corresponding source code