bug #50837: add zero-window probe timeout

This commit adds a timeout to the zero-window probing (persist timer)
mechanism. LwIP has not historically had a timeout for the persist
timer, leading to unbounded blocking if connection drops during the
zero-window condition

This commit also adds two units test, one to check the RTO timeout
and a second to check the zero-window probe timeout

CHANGELOG

@@ -52,6 +52,9 @@ HISTORY
 
   ++ Bugfixes:
 
+  2017-05-09: Joel Cunningham
+  * tcp: add zero-window probe timeout (bug #50837)
+
   2017-04-11: Simon Goldschmidt
   * sockets.c: task #14420 (Remove sys_sem_signal from inside SYS_ARCH_PROTECT
     crit section) done for LWIP_TCPIP_CORE_LOCKING==1

src/core/tcp.c

@@ -1072,17 +1072,21 @@ tcp_slowtmr_start:
       LWIP_DEBUGF(TCP_DEBUG, ("tcp_slowtmr: max DATA retries reached\n"));
     } else {
       if (pcb->persist_backoff > 0) {
-        /* If snd_wnd is zero, use persist timer to send 1 byte probes
-         * instead of using the standard retransmission mechanism. */
-        u8_t backoff_cnt = tcp_persist_backoff[pcb->persist_backoff-1];
-        if (pcb->persist_cnt < backoff_cnt) {
-          pcb->persist_cnt++;
-        }
-        if (pcb->persist_cnt >= backoff_cnt) {
-          if (tcp_zero_window_probe(pcb) == ERR_OK) {
-            pcb->persist_cnt = 0;
-            if (pcb->persist_backoff < sizeof(tcp_persist_backoff)) {
-              pcb->persist_backoff++;
+        if (pcb->persist_probe >= TCP_MAXRTX) {
+          ++pcb_remove; /* max probes reached */
+        } else {
+          /* If snd_wnd is zero, use persist timer to send 1 byte probes
+           * instead of using the standard retransmission mechanism. */
+          u8_t backoff_cnt = tcp_persist_backoff[pcb->persist_backoff-1];
+          if (pcb->persist_cnt < backoff_cnt) {
+            pcb->persist_cnt++;
+          }
+          if (pcb->persist_cnt >= backoff_cnt) {
+            if (tcp_zero_window_probe(pcb) == ERR_OK) {
+              pcb->persist_cnt = 0;
+              if (pcb->persist_backoff < sizeof(tcp_persist_backoff)) {
+                pcb->persist_backoff++;
+              }
             }
           }
         }

src/core/tcp_in.c

@@ -765,6 +765,7 @@ tcp_process(struct tcp_pcb *pcb)
     pcb->tmr = tcp_ticks;
   }
   pcb->keep_cnt_sent = 0;
+  pcb->persist_probe = 0;
 
   tcp_parseopt(pcb);
 
@@ -1093,10 +1094,11 @@ tcp_receive(struct tcp_pcb *pcb)
           /* start persist timer */
           pcb->persist_cnt = 0;
           pcb->persist_backoff = 1;
+          pcb->persist_probe = 0;
         }
       } else if (pcb->persist_backoff > 0) {
         /* stop persist timer */
-          pcb->persist_backoff = 0;
+        pcb->persist_backoff = 0;
       }
       LWIP_DEBUGF(TCP_WND_DEBUG, ("tcp_receive: window update %"TCPWNDSIZE_F"\n", pcb->snd_wnd));
 #if TCP_WND_DEBUG

src/core/tcp_out.c

@@ -1103,6 +1103,7 @@ tcp_output(struct tcp_pcb *pcb)
     if (pcb->persist_backoff == 0) {
       pcb->persist_cnt = 0;
       pcb->persist_backoff = 1;
+      pcb->persist_probe = 0;
     }
     goto output_done;
   }
@@ -1638,6 +1639,14 @@ tcp_zero_window_probe(struct tcp_pcb *pcb)
     return ERR_OK;
   }
 
+  /* increment probe count. NOTE: we record probe even if it fails
+     to actually transmit due to an error. This ensures memory exhaustion/
+     routing problem doesn't leave a zero-window pcb as an indefinite zombie.
+     RTO mechanism has similar behavior, see pcb->nrtx */
+  if (pcb->persist_probe < 0xFF) {
+    ++pcb->persist_probe;
+  }
+
   is_fin = ((TCPH_FLAGS(seg->tcphdr) & TCP_FIN) != 0) && (seg->len == 0);
   /* we want to send one seqno: either FIN or data (no options) */
   len = is_fin ? 0 : 1;

src/include/lwip/tcp.h

@@ -310,6 +310,8 @@ struct tcp_pcb {
   u8_t persist_cnt;
   /* Persist timer back-off */
   u8_t persist_backoff;
+  /* Number of persist probes */
+  u8_t persist_probe;
 
   /* KEEPALIVE counter */
   u8_t keep_cnt_sent;

test/unit/tcp/test_tcp.c

@@ -816,6 +816,185 @@ START_TEST(test_tcp_rto_tracking)
 }
 END_TEST
 
+START_TEST(test_tcp_rto_timeout)
+{
+  struct netif netif;
+  struct test_tcp_txcounters txcounters;
+  struct test_tcp_counters counters;
+  struct tcp_pcb *pcb, *cur;
+  err_t err;
+  u16_t i;
+  LWIP_UNUSED_ARG(_i);
+
+  /* Setup data for a single segment */
+  for (i = 0; i < TCP_MSS; i++) {
+    tx_data[i] = (u8_t)i;
+  }
+
+  /* initialize local vars */
+  test_tcp_init_netif(&netif, &txcounters, &test_local_ip, &test_netmask);
+  memset(&counters, 0, sizeof(counters));
+
+  /* create and initialize the pcb */
+  tcp_ticks = SEQNO1 - ISS;
+  pcb = test_tcp_new_counters_pcb(&counters);
+  EXPECT_RET(pcb != NULL);
+  tcp_set_state(pcb, ESTABLISHED, &test_local_ip, &test_remote_ip, TEST_LOCAL_PORT, TEST_REMOTE_PORT);
+  pcb->mss = TCP_MSS;
+  pcb->cwnd = TCP_MSS;
+
+  /* send our segment */
+  err = tcp_write(pcb, &tx_data[0], TCP_MSS, TCP_WRITE_FLAG_COPY);
+  EXPECT_RET(err == ERR_OK);
+  err = tcp_output(pcb);
+  EXPECT(txcounters.num_tx_calls == 1);
+  EXPECT(txcounters.num_tx_bytes == 1 * (TCP_MSS + 40U));
+  memset(&txcounters, 0, sizeof(txcounters));
+
+  /* ensure no errors have been recorded */
+  EXPECT(counters.err_calls == 0);
+  EXPECT(counters.last_err == ERR_OK);
+
+  /* Force us into retransmisson timeout */
+  while (!(pcb->flags & TF_RTO)) {
+    test_tcp_tmr();
+  }
+
+  /* check first rexmit */
+  EXPECT(pcb->nrtx == 1);
+  EXPECT(txcounters.num_tx_calls == 1);
+  EXPECT(txcounters.num_tx_bytes == 1 * (TCP_MSS + 40U));
+
+  /* still no error expected */
+  EXPECT(counters.err_calls == 0);
+  EXPECT(counters.last_err == ERR_OK);
+
+  /* keep running the timer till we hit our maximum RTO */
+  while (counters.last_err == ERR_OK) {
+    test_tcp_tmr();
+  }
+
+  /* check number of retransmissions */
+  EXPECT(txcounters.num_tx_calls == TCP_MAXRTX);
+  EXPECT(txcounters.num_tx_bytes == TCP_MAXRTX * (TCP_MSS + 40U));
+
+  /* check the connection (pcb) has been aborted */
+  EXPECT(counters.err_calls == 1);
+  EXPECT(counters.last_err == ERR_ABRT);
+  /* check our pcb is no longer active */
+  for (cur = tcp_active_pcbs; cur != NULL; cur = cur->next) {
+    EXPECT(cur != pcb);
+  }  
+  EXPECT_RET(MEMP_STATS_GET(used, MEMP_TCP_PCB) == 0);
+}
+END_TEST
+
+START_TEST(test_tcp_zwp_timeout)
+{
+  struct netif netif;
+  struct test_tcp_txcounters txcounters;
+  struct test_tcp_counters counters;
+  struct tcp_pcb *pcb, *cur;
+  struct pbuf* p;
+  err_t err;
+  u16_t i;
+  LWIP_UNUSED_ARG(_i);
+
+  /* Setup data for two segments */
+  for (i = 0; i < 2*TCP_MSS; i++) {
+    tx_data[i] = (u8_t)i;
+  }
+
+  /* initialize local vars */
+  test_tcp_init_netif(&netif, &txcounters, &test_local_ip, &test_netmask);
+  memset(&counters, 0, sizeof(counters));
+
+  /* create and initialize the pcb */
+  tcp_ticks = SEQNO1 - ISS;
+  pcb = test_tcp_new_counters_pcb(&counters);
+  EXPECT_RET(pcb != NULL);
+  tcp_set_state(pcb, ESTABLISHED, &test_local_ip, &test_remote_ip, TEST_LOCAL_PORT, TEST_REMOTE_PORT);
+  pcb->mss = TCP_MSS;
+  pcb->cwnd = TCP_MSS;
+
+  /* send first segment */
+  err = tcp_write(pcb, &tx_data[0], TCP_MSS, TCP_WRITE_FLAG_COPY);
+  EXPECT(err == ERR_OK);
+  err = tcp_output(pcb);
+  EXPECT(err == ERR_OK);
+  
+  /* verify segment is in-flight */
+  EXPECT(pcb->unsent == NULL);
+  check_seqnos(pcb->unacked, 1, seqnos);
+  EXPECT(txcounters.num_tx_calls == 1);
+  EXPECT(txcounters.num_tx_bytes == 1 * (TCP_MSS + 40U));
+  memset(&txcounters, 0, sizeof(txcounters));
+
+  /* ACK the segment and close the TX window */
+  p = tcp_create_rx_segment_wnd(pcb, NULL, 0, 0, TCP_MSS, TCP_ACK, 0);
+  test_tcp_input(p, &netif);
+  EXPECT(pcb->unacked == NULL);
+  EXPECT(pcb->unsent == NULL);
+  EXPECT(pcb->persist_backoff == 1);
+  EXPECT(pcb->snd_wnd == 0);
+
+  /* send second segment, should be buffered */
+  err = tcp_write(pcb, &tx_data[TCP_MSS], TCP_MSS, TCP_WRITE_FLAG_COPY);
+  EXPECT(err == ERR_OK);
+  err = tcp_output(pcb);
+  EXPECT(err == ERR_OK);
+
+  /* ensure it is buffered */
+  EXPECT(pcb->unacked == NULL);
+  check_seqnos(pcb->unsent, 1, &seqnos[1]);
+  EXPECT(txcounters.num_tx_calls == 0);
+  EXPECT(txcounters.num_tx_bytes == 0);
+
+  /* ensure no errors have been recorded */
+  EXPECT(counters.err_calls == 0);
+  EXPECT(counters.last_err == ERR_OK);
+
+  /* run timer till first probe */
+  EXPECT(pcb->persist_probe == 0);
+  while (pcb->persist_probe == 0) {
+    test_tcp_tmr();
+  }
+  EXPECT(txcounters.num_tx_calls == 1);
+  EXPECT(txcounters.num_tx_bytes == (1 + 40U));
+  memset(&txcounters, 0, sizeof(txcounters));
+
+  /* respond to probe with remote's current SEQ, ACK, and zero-window */
+  p = tcp_create_rx_segment_wnd(pcb, NULL, 0, 0, 0, TCP_ACK, 0);
+  test_tcp_input(p, &netif);
+  /* ensure zero-window is still active, but probe count reset */
+  EXPECT(pcb->persist_backoff > 1);
+  EXPECT(pcb->persist_probe == 0);
+  EXPECT(pcb->snd_wnd == 0);
+
+  /* ensure no errors have been recorded */
+  EXPECT(counters.err_calls == 0);
+  EXPECT(counters.last_err == ERR_OK);
+
+  /* now run the timer till we hit our maximum probe count */
+  while (counters.last_err == ERR_OK) {
+    test_tcp_tmr();
+  }
+
+  /* check maximum number of 1 byte probes were sent */
+  EXPECT(txcounters.num_tx_calls == TCP_MAXRTX);
+  EXPECT(txcounters.num_tx_bytes == TCP_MAXRTX * (1 + 40U));
+
+  /* check the connection (pcb) has been aborted */
+  EXPECT(counters.err_calls == 1);
+  EXPECT(counters.last_err == ERR_ABRT);
+  /* check our pcb is no longer active */
+  for (cur = tcp_active_pcbs; cur != NULL; cur = cur->next) {
+    EXPECT(cur != pcb);
+  }  
+  EXPECT_RET(MEMP_STATS_GET(used, MEMP_TCP_PCB) == 0);
+}
+END_TEST
+
 /** Create the suite including all tests for this module */
 Suite *
 tcp_suite(void)
@@ -829,7 +1008,9 @@ tcp_suite(void)
     TESTFUNC(test_tcp_rto_rexmit_wraparound),
     TESTFUNC(test_tcp_tx_full_window_lost_from_unacked),
     TESTFUNC(test_tcp_tx_full_window_lost_from_unsent),
-    TESTFUNC(test_tcp_rto_tracking)
+    TESTFUNC(test_tcp_rto_tracking),
+    TESTFUNC(test_tcp_rto_timeout),
+    TESTFUNC(test_tcp_zwp_timeout)
   };
   return create_suite("TCP", tests, sizeof(tests)/sizeof(testfunc), tcp_setup, tcp_teardown);
 }