fixed bug #33871: rejecting TCP_EVENT_RECV() for the last packet including FIN can lose data

CHANGELOG

@@ -45,6 +45,10 @@ HISTORY
 
  ++ Bugfixes:
 
+  2011-09-23: Simon Goldschmidt
+  * pbuf.h, tcp.c, tcp_in.c: fixed bug #33871: rejecting TCP_EVENT_RECV() for
+    the last packet including FIN can lose data
+
   2011-09-22: Simon Goldschmidt
   * tcp_impl.h: fixed bug #34355: nagle does not take snd_buf/snd_queuelen into
     account

src/core/tcp.c

@@ -1026,13 +1026,32 @@ tcp_fasttmr(void)
     if (pcb->refused_data != NULL) {
       /* Notify again application with data previously received. */
       err_t err;
+      u8_t refused_flags = pcb->refused_data->flags;
+      /* set pcb->refused_data to NULL in case the callback frees it and then
+         closes the pcb */
+      struct pbuf *refused_data = pcb->refused_data;
+      pcb->refused_data = NULL;
       LWIP_DEBUGF(TCP_INPUT_DEBUG, ("tcp_fasttmr: notify kept packet\n"));
-      TCP_EVENT_RECV(pcb, pcb->refused_data, ERR_OK, err);
+      TCP_EVENT_RECV(pcb, refused_data, ERR_OK, err);
       if (err == ERR_OK) {
-        pcb->refused_data = NULL;
+        /* did refused_data include a FIN? If so, handle it now. */
+        if (refused_flags & PBUF_FLAG_TCP_FIN) {
+          /* correct rcv_wnd as the application won't call tcp_recved()
+             for the FIN's seqno */
+          if (pcb->rcv_wnd != TCP_WND) {
+            pcb->rcv_wnd++;
+          }
+          TCP_EVENT_CLOSED(pcb, err);
+          if (err == ERR_ABRT) {
+            pcb = NULL;
+          }
+        }
       } else if (err == ERR_ABRT) {
         /* if err == ERR_ABRT, 'pcb' is already deallocated */
         pcb = NULL;
+      } else {
+        /* data is still refused */
+        pcb->refused_data = refused_data;
       }
     }
 

src/core/tcp_in.c

@@ -303,11 +303,27 @@ tcp_input(struct pbuf *p, struct netif *inp)
 
     /* If there is data which was previously "refused" by upper layer */
     if (pcb->refused_data != NULL) {
+      u8_t refused_flags = pcb->refused_data->flags;
+      /* set pcb->refused_data to NULL in case the callback frees it and then
+         closes the pcb */
+      struct pbuf *refused_data = pcb->refused_data;
+      pcb->refused_data = NULL;
       /* Notify again application with data previously received. */
       LWIP_DEBUGF(TCP_INPUT_DEBUG, ("tcp_input: notify kept packet\n"));
-      TCP_EVENT_RECV(pcb, pcb->refused_data, ERR_OK, err);
+      TCP_EVENT_RECV(pcb, refused_data, ERR_OK, err);
       if (err == ERR_OK) {
-        pcb->refused_data = NULL;
+        /* did refused_data include a FIN? */
+        if (refused_flags & PBUF_FLAG_TCP_FIN) {
+          /* correct rcv_wnd as the application won't call tcp_recved()
+             for the FIN's seqno */
+          if (pcb->rcv_wnd != TCP_WND) {
+            pcb->rcv_wnd++;
+          }
+          TCP_EVENT_CLOSED(pcb, err);
+          if (err == ERR_ABRT) {
+            goto dropped;
+          }
+        }
       } else if ((err == ERR_ABRT) || (tcplen > 0)) {
         /* if err == ERR_ABRT, 'pcb' is already deallocated */
         /* Drop incoming packets because pcb is "full" (only if the incoming
@@ -317,6 +333,9 @@ tcp_input(struct pbuf *p, struct netif *inp)
         snmp_inc_tcpinerrs();
         pbuf_free(p);
         return;
+      } else {
+        /* data is still refused, pbuf is still valid (go on for ACK-only packets) */
+        pcb->refused_data = refused_data;
       }
     }
     tcp_input_pcb = pcb;
@@ -375,14 +394,19 @@ tcp_input(struct pbuf *p, struct netif *inp)
         /* If a FIN segment was received, we call the callback
            function with a NULL buffer to indicate EOF. */
         if (recv_flags & TF_GOT_FIN) {
-          /* correct rcv_wnd as the application won't call tcp_recved()
-             for the FIN's seqno */
-          if (pcb->rcv_wnd != TCP_WND) {
-            pcb->rcv_wnd++;
-          }
-          TCP_EVENT_CLOSED(pcb, err);
-          if (err == ERR_ABRT) {
-            goto aborted;
+          if (pcb->refused_data != NULL) {
+            /* Delay this if we have refused data. */
+            pcb->refused_data->flags |= PBUF_FLAG_TCP_FIN;
+          } else {
+            /* correct rcv_wnd as the application won't call tcp_recved()
+               for the FIN's seqno */
+            if (pcb->rcv_wnd != TCP_WND) {
+              pcb->rcv_wnd++;
+            }
+            TCP_EVENT_CLOSED(pcb, err);
+            if (err == ERR_ABRT) {
+              goto aborted;
+            }
           }
         }
 
@@ -425,6 +449,10 @@ aborted:
 
   LWIP_ASSERT("tcp_input: tcp_pcbs_sane()", tcp_pcbs_sane());
   PERF_STOP("tcp_input");
+dropped:
+  TCP_STATS_INC(tcp.drop);
+  snmp_inc_tcpinerrs();
+  pbuf_free(p);
 }
 
 /**

src/include/lwip/pbuf.h

@@ -69,6 +69,8 @@ typedef enum {
 #define PBUF_FLAG_IS_CUSTOM 0x02U
 /** indicates this pbuf is UDP multicast to be looped back */
 #define PBUF_FLAG_MCASTLOOP 0x04U
+/** indicates this pbuf includes a TCP FIN flag */
+#define PBUF_FLAG_TCP_FIN   0x20U
 
 struct pbuf {
   /** next pbuf in singly linked pbuf chain */