Commit 02699917 authored by Philip Withnall's avatar Philip Withnall Committed by Olivier Crête

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.
parent b58e852d
...@@ -471,6 +471,7 @@ struct _PseudoTcpSocketPrivate { ...@@ -471,6 +471,7 @@ struct _PseudoTcpSocketPrivate {
guint32 rbuf_len, rcv_nxt, rcv_wnd, lastrecv; guint32 rbuf_len, rcv_nxt, rcv_wnd, lastrecv;
guint8 rwnd_scale; // Window scale factor guint8 rwnd_scale; // Window scale factor
PseudoTcpFifo rbuf; PseudoTcpFifo rbuf;
guint32 rcv_fin; /* sequence number of the received FIN octet, or 0 */
// Outgoing data // Outgoing data
GQueue slist; GQueue slist;
...@@ -809,6 +810,8 @@ pseudo_tcp_socket_init (PseudoTcpSocket *obj) ...@@ -809,6 +810,8 @@ pseudo_tcp_socket_init (PseudoTcpSocket *obj)
priv->snd_una = priv->rcv_nxt = 0; priv->snd_una = priv->rcv_nxt = 0;
priv->bReadEnable = TRUE; priv->bReadEnable = TRUE;
priv->bWriteEnable = FALSE; priv->bWriteEnable = FALSE;
priv->rcv_fin = 0;
priv->t_ack = 0; priv->t_ack = 0;
priv->msslevel = 0; priv->msslevel = 0;
...@@ -1529,6 +1532,7 @@ process(PseudoTcpSocket *self, Segment *seg) ...@@ -1529,6 +1532,7 @@ process(PseudoTcpSocket *self, Segment *seg)
gsize available_space; gsize available_space;
guint32 kIdealRefillSize; guint32 kIdealRefillSize;
gboolean is_valuable_ack, is_duplicate_ack, is_fin_ack = FALSE; gboolean is_valuable_ack, is_duplicate_ack, is_fin_ack = FALSE;
gboolean received_fin = FALSE;
/* If this is the wrong conversation, send a reset!?! /* If this is the wrong conversation, send a reset!?!
(with the correct conversation?) */ (with the correct conversation?) */
...@@ -1545,7 +1549,7 @@ process(PseudoTcpSocket *self, Segment *seg) ...@@ -1545,7 +1549,7 @@ process(PseudoTcpSocket *self, Segment *seg)
priv->bOutgoing = FALSE; priv->bOutgoing = FALSE;
if (priv->state == TCP_CLOSED || 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. */ /* Send an RST segment. See: RFC 1122, §4.2.2.13. */
if ((seg->flags & FLAG_RST) == 0) { if ((seg->flags & FLAG_RST) == 0) {
closedown (self, 0, CLOSEDOWN_LOCAL); closedown (self, 0, CLOSEDOWN_LOCAL);
...@@ -1720,19 +1724,34 @@ process(PseudoTcpSocket *self, Segment *seg) ...@@ -1720,19 +1724,34 @@ process(PseudoTcpSocket *self, Segment *seg)
set_state_established (self); 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) { 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. */ /* For the moment, FIN segments must not contain data. */
if (seg->flags & FLAG_FIN && seg->len != 0) { if (seg->flags & FLAG_FIN && seg->len != 0) {
DEBUG (PSEUDO_TCP_DEBUG_NORMAL, "FIN segment contained data; ignored"); DEBUG (PSEUDO_TCP_DEBUG_NORMAL, "FIN segment contained data; ignored");
return FALSE; 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 /* 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. */ * ‘rcv ACK of FIN’ from RFC 793, Figure 6; and RFC 1122, §4.2.2.8. */
switch (priv->state) { switch (priv->state) {
case TCP_ESTABLISHED: case TCP_ESTABLISHED:
if (seg->flags & FLAG_FIN) { if (received_fin) {
/* Received a FIN from the network, RFC 793, §3.5, Case 2. /* Received a FIN from the network, RFC 793, §3.5, Case 2.
* The code below will send an ACK for the FIN. */ * The code below will send an ACK for the FIN. */
set_state (self, TCP_CLOSE_WAIT); set_state (self, TCP_CLOSE_WAIT);
...@@ -1751,20 +1770,20 @@ process(PseudoTcpSocket *self, Segment *seg) ...@@ -1751,20 +1770,20 @@ process(PseudoTcpSocket *self, Segment *seg)
} }
break; break;
case TCP_FIN_WAIT_1: 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, /* Simultaneous close with an ACK for a FIN previously sent,
* RFC 793, §3.5, Case 3. */ * RFC 793, §3.5, Case 3. */
set_state (self, TCP_TIME_WAIT); set_state (self, TCP_TIME_WAIT);
} else if (is_fin_ack) { } else if (is_fin_ack) {
/* Handle the ACK of a locally-sent FIN flag. RFC 793, §3.5, Case 1. */ /* Handle the ACK of a locally-sent FIN flag. RFC 793, §3.5, Case 1. */
set_state (self, TCP_FIN_WAIT_2); 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. */ /* Simultaneous close, RFC 793, §3.5, Case 3. */
set_state (self, TCP_CLOSING); set_state (self, TCP_CLOSING);
} }
break; break;
case TCP_FIN_WAIT_2: case TCP_FIN_WAIT_2:
if (seg->flags & FLAG_FIN) { if (received_fin) {
/* Local user closed the connection, RFC 793, §3.5, Case 1. */ /* Local user closed the connection, RFC 793, §3.5, Case 1. */
set_state (self, TCP_TIME_WAIT); set_state (self, TCP_TIME_WAIT);
} }
...@@ -1776,7 +1795,7 @@ process(PseudoTcpSocket *self, Segment *seg) ...@@ -1776,7 +1795,7 @@ process(PseudoTcpSocket *self, Segment *seg)
case TCP_CLOSED: case TCP_CLOSED:
case TCP_CLOSE_WAIT: case TCP_CLOSE_WAIT:
/* Shouldn’t ever hit these cases. */ /* Shouldn’t ever hit these cases. */
if (seg->flags & FLAG_FIN) { if (received_fin) {
DEBUG (PSEUDO_TCP_DEBUG_NORMAL, DEBUG (PSEUDO_TCP_DEBUG_NORMAL,
"Unexpected state %u when FIN received", priv->state); "Unexpected state %u when FIN received", priv->state);
} else if (is_fin_ack) { } else if (is_fin_ack) {
...@@ -1827,9 +1846,10 @@ process(PseudoTcpSocket *self, Segment *seg) ...@@ -1827,9 +1846,10 @@ process(PseudoTcpSocket *self, Segment *seg)
} else { } else {
sflags = sfDelayedAck; 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; sflags = sfImmediateAck;
priv->rcv_nxt += 1;
} }
if (sflags == sfImmediateAck) { if (sflags == sfImmediateAck) {
...@@ -1869,12 +1889,7 @@ process(PseudoTcpSocket *self, Segment *seg) ...@@ -1869,12 +1889,7 @@ process(PseudoTcpSocket *self, Segment *seg)
bNewData = FALSE; bNewData = FALSE;
if (seg->flags & FLAG_FIN) { if (seg->len > 0) {
/* FIN flags have a sequence number. */
if (seg->seq == priv->rcv_nxt) {
priv->rcv_nxt++;
}
} else if (seg->len > 0) {
if (bIgnoreData) { if (bIgnoreData) {
if (seg->seq == priv->rcv_nxt) { if (seg->seq == priv->rcv_nxt) {
priv->rcv_nxt += seg->len; priv->rcv_nxt += seg->len;
...@@ -1929,6 +1944,12 @@ process(PseudoTcpSocket *self, Segment *seg) ...@@ -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); attempt_send(self, sflags);
// If we have new data, notify the user // If we have new data, notify the user
......
/* /*
* This file is part of the Nice GLib ICE library. * This file is part of the Nice GLib ICE library.
* *
* (C) 2014 Collabora Ltd. * © 2014, 2015 Collabora Ltd.
* Contact: Philip Withnall * Contact: Philip Withnall
* *
* The contents of this file are subject to the Mozilla Public License Version * The contents of this file are subject to the Mozilla Public License Version
...@@ -269,6 +269,13 @@ expect_syn_received (Data *data) ...@@ -269,6 +269,13 @@ expect_syn_received (Data *data)
expect_segment (data->right, data->right_sent, 0, 7, 7, FLAG_SYN); 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. */ /* Return whether the socket accepted the packet. */
static gboolean static gboolean
forward_segment (GQueue/*<owned GBytes>*/ *from, PseudoTcpSocket *to) forward_segment (GQueue/*<owned GBytes>*/ *from, PseudoTcpSocket *to)
...@@ -453,6 +460,8 @@ establish_connection (Data *data) ...@@ -453,6 +460,8 @@ establish_connection (Data *data)
expect_ack (data->left, data->left_sent, 7, 7); expect_ack (data->left, data->left_sent, 7, 7);
forward_segment_ltr (data); forward_segment_ltr (data);
expect_sockets_connected (data); expect_sockets_connected (data);
assert_empty_queues (data);
} }
/* Helper to close the LHS of a socket pair which has not transmitted any /* Helper to close the LHS of a socket pair which has not transmitted any
...@@ -753,6 +762,76 @@ pseudotcp_close_normal_recovery4 (void) ...@@ -753,6 +762,76 @@ pseudotcp_close_normal_recovery4 (void)
data_clear (&data); 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 /* Check that if both FIN segments from a simultaneous FIN handshake are
* dropped, the handshake recovers and completes successfully. * dropped, the handshake recovers and completes successfully.
* See: RFC 793, Figure 14. */ * See: RFC 793, Figure 14. */
...@@ -977,11 +1056,14 @@ pseudotcp_close_rst_afterwards (void) ...@@ -977,11 +1056,14 @@ pseudotcp_close_rst_afterwards (void)
/* Close the LHS. */ /* 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.left), ==, 0);
pseudo_tcp_socket_close (data.left, TRUE);
close_socket (data.left); 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 */ 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. */ /* 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); g_assert_cmpint (pseudo_tcp_socket_send (data.right, "foo", 3), ==, 3);
expect_data (data.right, data.right_sent, 7, 7, 3); expect_data (data.right, data.right_sent, 7, 7, 3);
...@@ -1109,6 +1191,8 @@ main (int argc, char *argv[]) ...@@ -1109,6 +1191,8 @@ main (int argc, char *argv[])
pseudotcp_close_normal_recovery3); pseudotcp_close_normal_recovery3);
g_test_add_func ("/pseudotcp/close/normal/recovery4", g_test_add_func ("/pseudotcp/close/normal/recovery4",
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", g_test_add_func ("/pseudotcp/close/simultaneous/recovery1",
pseudotcp_close_simultaneous_recovery1); pseudotcp_close_simultaneous_recovery1);
g_test_add_func ("/pseudotcp/close/simultaneous/recovery2", g_test_add_func ("/pseudotcp/close/simultaneous/recovery2",
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment