pseudotcp: Fix retransmission of segments before handling a FIN

Previously, if peer A transmitted one or more data segments (1),
followed by a FIN segment (2) to peer B, and segments 1 were
dropped, peer B would not request retransmission of them and would
instead continue with the FIN handshake. This effectively meant
segments 1 were lost without peer B realising.

Fix this by only handling the FIN segment once its sequence number is
acknowledged in the receive window.

2 files changed, 122 insertions, 17 deletions

diff --git a/agent/pseudotcp.c b/agent/pseudotcp.c
index b5afea0..4fe8fa1 100644
--- a/agent/pseudotcp.c
+++ b/agent/pseudotcp.c
@@ -471,6 +471,7 @@ struct _PseudoTcpSocketPrivate {
   guint32 rbuf_len, rcv_nxt, rcv_wnd, lastrecv;
   guint8 rwnd_scale; // Window scale factor
   PseudoTcpFifo rbuf;
+  guint32 rcv_fin;  /* sequence number of the received FIN octet, or 0 */
 
   // Outgoing data
   GQueue slist;
@@ -809,6 +810,8 @@ pseudo_tcp_socket_init (PseudoTcpSocket *obj)
   priv->snd_una = priv->rcv_nxt = 0;
   priv->bReadEnable = TRUE;
   priv->bWriteEnable = FALSE;
+  priv->rcv_fin = 0;
+
   priv->t_ack = 0;
 
   priv->msslevel = 0;
@@ -1529,6 +1532,7 @@ process(PseudoTcpSocket *self, Segment *seg)
   gsize available_space;
   guint32 kIdealRefillSize;
   gboolean is_valuable_ack, is_duplicate_ack, is_fin_ack = FALSE;
+  gboolean received_fin = FALSE;
 
   /* If this is the wrong conversation, send a reset!?!
      (with the correct conversation?) */
@@ -1545,7 +1549,7 @@ process(PseudoTcpSocket *self, Segment *seg)
   priv->bOutgoing = FALSE;
 
   if (priv->state == TCP_CLOSED ||
-      (pseudo_tcp_state_has_sent_fin (priv->state) && seg->len > 0)) {
+      (pseudo_tcp_state_has_received_fin (priv->state) && seg->len > 0)) {
     /* Send an RST segment. See: RFC 1122, §4.2.2.13. */
     if ((seg->flags & FLAG_RST) == 0) {
       closedown (self, 0, CLOSEDOWN_LOCAL);
@@ -1720,19 +1724,34 @@ process(PseudoTcpSocket *self, Segment *seg)
     set_state_established (self);
   }
 
-  /* Check for connection closure. */
+  /* Check for connection closure. Only pay attention to FIN segments if they
+   * are in sequence; otherwise we’ve missed a packet earlier in the stream and
+   * need to request retransmission first. */
   if (priv->support_fin_ack) {
+    /* @received_fin is set when, and only when, all segments preceding the FIN
+     * have been acknowledged. This is to handle the case where the FIN arrives
+     * out of order with a preceding data segment. */
+    if (seg->flags & FLAG_FIN && priv->rcv_fin == 0) {
+      priv->rcv_fin = seg->seq;
+      DEBUG (PSEUDO_TCP_DEBUG_NORMAL, "Setting rcv_fin = %u", priv->rcv_fin);
+    } else if (seg->flags & FLAG_FIN && seg->seq != priv->rcv_fin) {
+      DEBUG (PSEUDO_TCP_DEBUG_NORMAL, "Second FIN segment received; ignored");
+      return FALSE;
+    }
+
     /* For the moment, FIN segments must not contain data. */
     if (seg->flags & FLAG_FIN && seg->len != 0) {
       DEBUG (PSEUDO_TCP_DEBUG_NORMAL, "FIN segment contained data; ignored");
       return FALSE;
     }
 
+    received_fin = (priv->rcv_nxt != 0 && priv->rcv_nxt + seg->len == priv->rcv_fin);
+
     /* Update the state machine, implementing all transitions on ‘rcv FIN’ or
      * ‘rcv ACK of FIN’ from RFC 793, Figure 6; and RFC 1122, §4.2.2.8. */
     switch (priv->state) {
     case TCP_ESTABLISHED:
-      if (seg->flags & FLAG_FIN) {
+      if (received_fin) {
         /* Received a FIN from the network, RFC 793, §3.5, Case 2.
          * The code below will send an ACK for the FIN. */
          set_state (self, TCP_CLOSE_WAIT);
@@ -1751,20 +1770,20 @@ process(PseudoTcpSocket *self, Segment *seg)
       }
       break;
     case TCP_FIN_WAIT_1:
-      if (is_fin_ack && seg->flags & FLAG_FIN) {
+      if (is_fin_ack && received_fin) {
         /* Simultaneous close with an ACK for a FIN previously sent,
          * RFC 793, §3.5, Case 3. */
         set_state (self, TCP_TIME_WAIT);
       } else if (is_fin_ack) {
         /* Handle the ACK of a locally-sent FIN flag. RFC 793, §3.5, Case 1. */
         set_state (self, TCP_FIN_WAIT_2);
-      } else if (seg->flags & FLAG_FIN) {
+      } else if (received_fin) {
         /* Simultaneous close, RFC 793, §3.5, Case 3. */
         set_state (self, TCP_CLOSING);
       }
       break;
     case TCP_FIN_WAIT_2:
-      if (seg->flags & FLAG_FIN) {
+      if (received_fin) {
         /* Local user closed the connection, RFC 793, §3.5, Case 1. */
         set_state (self, TCP_TIME_WAIT);
       }
@@ -1776,7 +1795,7 @@ process(PseudoTcpSocket *self, Segment *seg)
     case TCP_CLOSED:
     case TCP_CLOSE_WAIT:
       /* Shouldn’t ever hit these cases. */
-      if (seg->flags & FLAG_FIN) {
+      if (received_fin) {
         DEBUG (PSEUDO_TCP_DEBUG_NORMAL,
            "Unexpected state %u when FIN received", priv->state);
       } else if (is_fin_ack) {
@@ -1827,9 +1846,10 @@ process(PseudoTcpSocket *self, Segment *seg)
     } else {
       sflags = sfDelayedAck;
     }
-  } else if (seg->flags & FLAG_FIN) {
+  } else if (received_fin) {
+    /* FIN flags have a sequence number. Only acknowledge them after all
+     * preceding octets have been acknowledged. */
     sflags = sfImmediateAck;
-    priv->rcv_nxt += 1;
   }
 
   if (sflags == sfImmediateAck) {
@@ -1869,12 +1889,7 @@ process(PseudoTcpSocket *self, Segment *seg)
 
   bNewData = FALSE;
 
-  if (seg->flags & FLAG_FIN) {
-    /* FIN flags have a sequence number. */
-    if (seg->seq == priv->rcv_nxt) {
-      priv->rcv_nxt++;
-    }
-  } else if (seg->len > 0) {
+  if (seg->len > 0) {
     if (bIgnoreData) {
       if (seg->seq == priv->rcv_nxt) {
         priv->rcv_nxt += seg->len;
@@ -1929,6 +1944,12 @@ process(PseudoTcpSocket *self, Segment *seg)
     }
   }
 
+  if (received_fin) {
+    /* FIN flags have a sequence number. */
+    priv->rcv_nxt++;
+  }
+
+
   attempt_send(self, sflags);
 
   // If we have new data, notify the user
diff --git a/tests/test-pseudotcp-fin.c b/tests/test-pseudotcp-fin.c
index b769161..96e6d30 100644
--- a/tests/test-pseudotcp-fin.c
+++ b/tests/test-pseudotcp-fin.c
@@ -1,7 +1,7 @@
 /*
  * This file is part of the Nice GLib ICE library.
  *
- * (C) 2014 Collabora Ltd.
+ * © 2014, 2015 Collabora Ltd.
  *  Contact: Philip Withnall
  *
  * The contents of this file are subject to the Mozilla Public License Version
@@ -269,6 +269,13 @@ expect_syn_received (Data *data)
   expect_segment (data->right, data->right_sent, 0, 7, 7, FLAG_SYN);
 }
 
+static void
+assert_empty_queues (Data *data)
+{
+  g_assert_cmpuint (g_queue_get_length (data->left_sent), ==, 0);
+  g_assert_cmpuint (g_queue_get_length (data->right_sent), ==, 0);
+}
+
 /* Return whether the socket accepted the packet. */
 static gboolean
 forward_segment (GQueue/*<owned GBytes>*/ *from, PseudoTcpSocket *to)
@@ -453,6 +460,8 @@ establish_connection (Data *data)
   expect_ack (data->left,  data->left_sent, 7, 7);
   forward_segment_ltr (data);
   expect_sockets_connected (data);
+
+  assert_empty_queues (data);
 }
 
 /* Helper to close the LHS of a socket pair which has not transmitted any
@@ -753,6 +762,76 @@ pseudotcp_close_normal_recovery4 (void)
   data_clear (&data);
 }
 
+/* Check that closing a connection recovers from a data segment being dropped
+ * immediately before the first FIN is sent. Based on: RFC 793, Figure 13. */
+static void
+pseudotcp_close_normal_recovery_data (void)
+{
+  Data data = { 0, };
+
+  /* Establish a connection. */
+  establish_connection (&data);
+
+  /* Send some data from LHS to RHS, but drop the segment. */
+  g_assert_cmpint (pseudo_tcp_socket_send (data.left, "foo", 3), ==, 3);
+  expect_data (data.left, data.left_sent, 7, 7, 3);
+  drop_segment (data.left, data.left_sent);
+
+  assert_empty_queues(&data);
+
+  /* Close the LHS. */
+  g_assert_cmpint (pseudo_tcp_socket_get_available_bytes (data.left), ==, 0);
+  g_assert_cmpint (pseudo_tcp_socket_get_available_bytes (data.right), ==, 0);
+  close_socket (data.left);
+
+  expect_socket_state (data.left, TCP_FIN_WAIT_1);
+  expect_fin (data.left, data.left_sent, 10, 7);
+  forward_segment_ltr (&data);
+
+  expect_socket_state (data.right, TCP_ESTABLISHED);
+  expect_ack (data.right, data.right_sent, 7, 7);
+  forward_segment_rtl (&data);
+
+  expect_socket_state (data.left, TCP_FIN_WAIT_1);
+
+  assert_empty_queues(&data);
+
+  /* Close the RHS. */
+  close_socket (data.right);
+
+  expect_socket_state (data.right, TCP_FIN_WAIT_1);
+
+  expect_fin (data.right, data.right_sent, 7, 7);
+  forward_segment_rtl (&data);
+
+  expect_socket_state (data.left, TCP_CLOSING);
+
+  expect_ack (data.left, data.left_sent, 11, 8);
+  forward_segment_ltr (&data);
+
+  expect_socket_state (data.right, TCP_FIN_WAIT_2);
+
+  expect_data (data.right, data.right_sent, 8, 7, 0);
+  forward_segment_rtl (&data);
+  expect_socket_state (data.left, TCP_CLOSING);
+
+  expect_data (data.left, data.left_sent, 7, 8, 3);
+  forward_segment_ltr (&data);
+  expect_socket_state (data.right, TCP_TIME_WAIT);
+
+  increment_time_both (&data, 100);  /* Delayed ACK */
+
+  expect_ack (data.right, data.right_sent, 8, 11);
+  forward_segment_rtl (&data);
+  expect_socket_state (data.left, TCP_TIME_WAIT);
+
+  increment_time_both (&data, 10);  /* TIME-WAIT */
+
+  expect_sockets_closed (&data);
+
+  data_clear (&data);
+}
+
 /* Check that if both FIN segments from a simultaneous FIN handshake are
  * dropped, the handshake recovers and completes successfully.
  * See: RFC 793, Figure 14. */
@@ -977,11 +1056,14 @@ pseudotcp_close_rst_afterwards (void)
 
   /* Close the LHS. */
   g_assert_cmpint (pseudo_tcp_socket_get_available_bytes (data.left), ==, 0);
+  pseudo_tcp_socket_close (data.left, TRUE);
   close_socket (data.left);
 
-  expect_fin (data.left, data.left_sent, 7, 7);
+  expect_rst (data.left, data.left_sent, 7, 7);
   drop_segment (data.left, data.left_sent);  /* just to get it out of the way */
 
+  assert_empty_queues(&data);
+
   /* Send some data from RHS to LHS, which should result in an RST. */
   g_assert_cmpint (pseudo_tcp_socket_send (data.right, "foo", 3), ==, 3);
   expect_data (data.right, data.right_sent, 7, 7, 3);
@@ -1109,6 +1191,8 @@ main (int argc, char *argv[])
       pseudotcp_close_normal_recovery3);
   g_test_add_func ("/pseudotcp/close/normal/recovery4",
       pseudotcp_close_normal_recovery4);
+  g_test_add_func ("/pseudotcp/close/normal/recovery-data",
+      pseudotcp_close_normal_recovery_data);
   g_test_add_func ("/pseudotcp/close/simultaneous/recovery1",
       pseudotcp_close_simultaneous_recovery1);
   g_test_add_func ("/pseudotcp/close/simultaneous/recovery2",