Merge pull request #12367 from brummer-simon/gnrc_tcp-fix_recv_conn_closed

gnrc_tcp: return immediatly on gnrc_tcp_recv if a connection is closing

sys/include/net/gnrc/tcp.h

@@ -160,6 +160,7 @@ ssize_t gnrc_tcp_send(gnrc_tcp_tcb_t *tcb, const void *data, const size_t len,
  *                                           @p user_timeout_duration_us microseconds passed.
  *
  * @returns   The number of bytes read into @p data.
+ *            0, if the connection is closing and no further data can be read.
  *            -ENOTCONN if connection is not established.
  *            -EAGAIN if  user_timeout_duration_us is zero and no data is available.
  *            -ECONNRESET if connection was resetted by the peer.

sys/net/gnrc/transport_layer/tcp/gnrc_tcp.c

@@ -481,6 +481,14 @@ ssize_t gnrc_tcp_recv(gnrc_tcp_tcb_t *tcb, void *data, const size_t max_len,
         return -ENOTCONN;
     }
 
+    /* If FIN was received (CLOSE_WAIT), no further data can be received. */
+    /* Copy received data into given buffer and return number of bytes. Can be zero. */
+    if (tcb->state == FSM_STATE_CLOSE_WAIT) {
+        ret = _fsm(tcb, FSM_EVENT_CALL_RECV, NULL, data, max_len);
+        mutex_unlock(&(tcb->function_lock));
+        return ret;
+    }
+
     /* If this call is non-blocking (timeout_duration_us == 0): Try to read data and return */
     if (timeout_duration_us == 0) {
         ret = _fsm(tcb, FSM_EVENT_CALL_RECV, NULL, data, max_len);
@@ -516,6 +524,11 @@ ssize_t gnrc_tcp_recv(gnrc_tcp_tcb_t *tcb, void *data, const size_t max_len,
         /* Try to read available data */
         ret = _fsm(tcb, FSM_EVENT_CALL_RECV, NULL, data, max_len);
 
+        /* If FIN was received (CLOSE_WAIT), no further data can be received. Leave event loop */
+        if (tcb->state == FSM_STATE_CLOSE_WAIT) {
+            break;
+        }
+
         /* If there was no data: Wait for next packet or until the timeout fires */
         if (ret <= 0) {
             mbox_get(&(tcb->mbox), &msg);

sys/net/gnrc/transport_layer/tcp/gnrc_tcp_fsm.c

@@ -307,11 +307,9 @@ static int _fsm_call_recv(gnrc_tcp_tcb_t *tcb, void *buf, size_t len)
     /* Read data into 'buf' up to 'len' bytes from receive buffer */
     size_t rcvd = ringbuffer_get(&(tcb->rcv_buf), buf, len);
 
-    /* Reopen window if recv buffer can hold a MSS sized payload and FIN was not received. */
-    uint16_t buf_free = ringbuffer_get_free(&tcb->rcv_buf);
-
-    if (buf_free >= GNRC_TCP_MSS && tcb->state != FSM_STATE_CLOSE_WAIT) {
-        tcb->rcv_wnd = buf_free;
+    /* If receive buffer can store more than GNRC_TCP_MSS: open window to available buffer size */
+    if (ringbuffer_get_free(&tcb->rcv_buf) >= GNRC_TCP_MSS) {
+        tcb->rcv_wnd = ringbuffer_get_free(&(tcb->rcv_buf));
 
         /* Send ACK to anounce window update */
         gnrc_pktsnip_t *out_pkt = NULL;

tests/gnrc_tcp/README.md

@@ -22,6 +22,11 @@ in the tests directory.
     This test mostly is a regression test for issues that were found through fuzzing. It uses
     `scapy` to interact with the node.
 
+6) 06-receive_data_closed_conn.py
+    This test covers accessing received data after receiving a FIN packet. If the connection was closed
+    by the peer, a call to gnrc_tcp_recv must return directly with all currently received data
+    or zero if there is no data. The function must return immediatly dispite any given timeout.
+
 Setup
 ==========
 The test requires a tap-device setup. This can be achieved by running 'dist/tools/tapsetup/tapsetup'

tests/gnrc_tcp/main.c

@@ -228,6 +228,10 @@ int gnrc_tcp_recv_cmd(int argc, char **argv)
         int ret = gnrc_tcp_recv(&tcb, buffer + rcvd, to_receive - rcvd,
                                 timeout);
         switch (ret) {
+            case 0:
+                printf("%s: returns 0\n", argv[0]);
+                return ret;
+
             case -EAGAIN:
                 printf("%s: returns -EAGAIN\n", argv[0]);
                 continue;

tests/gnrc_tcp/tests/02-conn_lifecycle_as_server.py

@@ -54,4 +54,4 @@ def testfunc(child):
 
 if __name__ == '__main__':
     sudo_guard()
-    sys.exit(run(testfunc, timeout=5, echo=False, traceback=True))
+    sys.exit(run(testfunc, timeout=7, echo=False, traceback=True))

tests/gnrc_tcp/tests/06-receive_data_closed_conn.py

@@ -0,0 +1,84 @@
+#!/usr/bin/env python3
+
+# Copyright (C) 2019   Simon Brummer <simon.brummer@posteo.de>
+#
+# This file is subject to the terms and conditions of the GNU Lesser
+# General Public License v2.1. See the file LICENSE in the top level
+# directory for more details.
+
+import os
+import sys
+import threading
+
+from testrunner import run
+from shared_func import TcpServer, generate_port_number, get_host_tap_device, \
+                        get_host_ll_addr, get_riot_if_id, setup_internal_buffer, \
+                        read_data_from_internal_buffer, verify_pktbuf_empty, \
+                        sudo_guard
+
+
+def tcp_server(port, shutdown_event, data):
+    with TcpServer(port, shutdown_event) as tcp_srv:
+        tcp_srv.send(data)
+
+
+def testfunc(child):
+    port = generate_port_number()
+    shutdown_event = threading.Event()
+
+    # Try to receive 10 bytes sent from the Host System.
+    data = 'test_data_'
+    data_len = len(data)
+
+    assert (data_len % 2) == 0
+
+    # Verify that RIOT Applications internal buffer can hold test data.
+    assert setup_internal_buffer(child) >= data_len
+
+    server_handle = threading.Thread(target=tcp_server, args=(port, shutdown_event, data))
+    server_handle.start()
+
+    target_addr = get_host_ll_addr(get_host_tap_device()) + '%' + get_riot_if_id(child)
+
+    # Setup RIOT Node to connect to Hostsystems TCP Server
+    child.sendline('gnrc_tcp_tcb_init')
+    child.sendline('gnrc_tcp_open_active AF_INET6 ' + target_addr + " " + str(port) + ' 0')
+    child.expect_exact('gnrc_tcp_open_active: returns 0')
+
+    # Initiate connection teardown from test host
+    shutdown_event.set()
+
+    half_data_len = int(data_len / 2)
+
+    # Read half the test data with timeout. Verify Buffer contents
+    child.sendline('gnrc_tcp_recv 1000000 ' + str(half_data_len))
+    child.expect_exact('gnrc_tcp_recv: received ' + str(half_data_len))
+    assert read_data_from_internal_buffer(child, half_data_len) == data[:half_data_len]
+
+    # Read half the test data without timeout. Verify Buffer contents
+    child.sendline('gnrc_tcp_recv 0 ' + str(half_data_len))
+    child.expect_exact('gnrc_tcp_recv: received ' + str(half_data_len))
+    assert read_data_from_internal_buffer(child, half_data_len) == data[half_data_len:]
+
+    # Buffer should have been read entirly and the connection was closed, there can be no new data.
+    # Reading with a timeout must return 0 not -ETIMEOUT
+    child.sendline('gnrc_tcp_recv 1000000 ' + str(half_data_len))
+    child.expect_exact('gnrc_tcp_recv: returns 0')
+
+    # Reading without a timeout must return 0 not -EAGAIN.
+    child.sendline('gnrc_tcp_recv 0 ' + str(half_data_len))
+    child.expect_exact('gnrc_tcp_recv: returns 0')
+
+    # Close connection
+    child.sendline('gnrc_tcp_close')
+    server_handle.join()
+
+    verify_pktbuf_empty(child)
+
+    # Verify received Data
+    print(os.path.basename(sys.argv[0]) + ': success')
+
+
+if __name__ == '__main__':
+    sudo_guard()
+    sys.exit(run(testfunc, timeout=5, echo=False, traceback=True))