From 6eb2e9cc4e11129bc02d8cb90781bde21ec9361c Mon Sep 17 00:00:00 2001
From: Joel Cunningham <joel.cunningham@me.com>
Date: Wed, 22 Mar 2017 16:19:23 -0500
Subject: [PATCH] bug #50614: move unsent queue check/pcb->snd_buf update

This commit corrects what looks like an ancient incorrect organization
of the logic for processing an ACK which acks new data.  Once moved,
we can also change to using TCP_SEQ_LEQ on ackno instead of TCP_BETWEEN
because ackno has already been checked against snd_nxt

The work of checking the unsent queue and updating pcb->snd_buf (both
steps required for new data ACK) should be located under the conditional
that checks TCP_SEQ_BETWEEN(ackno, pcb->lastack+1, pcb->snd_nxt)

The comment following the unsent queue check/pcb->snd_buf update even
indicates "End of ACK for new data processing" when the logic is clearly
outside of this check

From what I can tell, this mis-organization isn't causing any incorrect
behavior since the unsent queue checked that ackno was between start of
segment and snd_nxt and recv_acked would be 0 during pcb->snd_buf update.
Instead this is waisted work for duplicate ACKS (can be common) and other
old ACKs
---
 src/core/tcp_in.c | 68 +++++++++++++++++++++++++++----------------------------
 1 file changed, 34 insertions(+), 34 deletions(-)

diff --git a/src/core/tcp_in.c b/src/core/tcp_in.c
index 80c080a..9be8759 100644
--- a/src/core/tcp_in.c
+++ b/src/core/tcp_in.c
@@ -1188,45 +1188,45 @@ tcp_receive(struct tcp_pcb *pcb)
         nd6_reachability_hint(ip6_current_src_addr());
       }
 #endif /* LWIP_IPV6 && LWIP_ND6_TCP_REACHABILITY_HINTS*/
-    } else {
-      /* Out of sequence ACK, didn't really ack anything */
-      tcp_send_empty_ack(pcb);
-    }
 
-    /* We go through the ->unsent list to see if any of the segments
-       on the list are acknowledged by the ACK. This may seem
-       strange since an "unsent" segment shouldn't be acked. The
-       rationale is that lwIP puts all outstanding segments on the
-       ->unsent list after a retransmission, so these segments may
-       in fact have been sent once. */
-    while (pcb->unsent != NULL &&
-           TCP_SEQ_BETWEEN(ackno, lwip_ntohl(pcb->unsent->tcphdr->seqno) +
-                           TCP_TCPLEN(pcb->unsent), pcb->snd_nxt)) {
-      LWIP_DEBUGF(TCP_INPUT_DEBUG, ("tcp_receive: removing %"U32_F":%"U32_F" from pcb->unsent\n",
-                                    lwip_ntohl(pcb->unsent->tcphdr->seqno), lwip_ntohl(pcb->unsent->tcphdr->seqno) +
-                                    TCP_TCPLEN(pcb->unsent)));
-
-      next = pcb->unsent;
-      pcb->unsent = pcb->unsent->next;
+      /* We go through the ->unsent list to see if any of the segments
+         on the list are acknowledged by the ACK. This may seem
+         strange since an "unsent" segment shouldn't be acked. The
+         rationale is that lwIP puts all outstanding segments on the
+         ->unsent list after a retransmission, so these segments may
+         in fact have been sent once. */
+      while (pcb->unsent != NULL &&
+             TCP_SEQ_LEQ(lwip_ntohl(pcb->unsent->tcphdr->seqno) +
+                         TCP_TCPLEN(pcb->unsent), ackno)) {
+        LWIP_DEBUGF(TCP_INPUT_DEBUG, ("tcp_receive: removing %"U32_F":%"U32_F" from pcb->unsent\n",
+                                      lwip_ntohl(pcb->unsent->tcphdr->seqno), lwip_ntohl(pcb->unsent->tcphdr->seqno) +
+                                      TCP_TCPLEN(pcb->unsent)));
+
+        next = pcb->unsent;
+        pcb->unsent = pcb->unsent->next;
 #if TCP_OVERSIZE
-      if (pcb->unsent == NULL) {
-        pcb->unsent_oversize = 0;
-      }
+        if (pcb->unsent == NULL) {
+          pcb->unsent_oversize = 0;
+        }
 #endif /* TCP_OVERSIZE */
-      LWIP_DEBUGF(TCP_QLEN_DEBUG, ("tcp_receive: queuelen %"TCPWNDSIZE_F" ... ", (tcpwnd_size_t)pcb->snd_queuelen));
-      LWIP_ASSERT("pcb->snd_queuelen >= pbuf_clen(next->p)", (pcb->snd_queuelen >= pbuf_clen(next->p)));
-      /* Prevent ACK for FIN to generate a sent event */
-      pcb->snd_queuelen -= pbuf_clen(next->p);
-      recv_acked += next->len;
-      tcp_seg_free(next);
-      LWIP_DEBUGF(TCP_QLEN_DEBUG, ("%"TCPWNDSIZE_F" (after freeing unsent)\n", (tcpwnd_size_t)pcb->snd_queuelen));
-      if (pcb->snd_queuelen != 0) {
-        LWIP_ASSERT("tcp_receive: valid queue length",
-          pcb->unacked != NULL || pcb->unsent != NULL);
+        LWIP_DEBUGF(TCP_QLEN_DEBUG, ("tcp_receive: queuelen %"TCPWNDSIZE_F" ... ", (tcpwnd_size_t)pcb->snd_queuelen));
+        LWIP_ASSERT("pcb->snd_queuelen >= pbuf_clen(next->p)", (pcb->snd_queuelen >= pbuf_clen(next->p)));
+        /* Prevent ACK for FIN to generate a sent event */
+        pcb->snd_queuelen -= pbuf_clen(next->p);
+        recv_acked += next->len;
+        tcp_seg_free(next);
+        LWIP_DEBUGF(TCP_QLEN_DEBUG, ("%"TCPWNDSIZE_F" (after freeing unsent)\n", (tcpwnd_size_t)pcb->snd_queuelen));
+        if (pcb->snd_queuelen != 0) {
+          LWIP_ASSERT("tcp_receive: valid queue length",
+            pcb->unacked != NULL || pcb->unsent != NULL);
+        }
       }
+      pcb->snd_buf += recv_acked;
+      /* End of ACK for new data processing. */
+    } else {
+      /* Out of sequence ACK, didn't really ack anything */
+      tcp_send_empty_ack(pcb);
     }
-    pcb->snd_buf += recv_acked;
-    /* End of ACK for new data processing. */
 
     LWIP_DEBUGF(TCP_RTO_DEBUG, ("tcp_receive: pcb->rttest %"U32_F" rtseq %"U32_F" ackno %"U32_F"\n",
                                 pcb->rttest, pcb->rtseq, ackno));
-- 
1.9.5.msysgit.0

