tcp: read the socket state under the protection mutex in accept()

[tinic]

_nx_tcp_server_socket_accept() decides whether to suspend by reading nx_tcp_socket_state before it takes nx_ip_protection, and never reads it again. A connection that is established inside that window suspends the calling thread on a socket that will never resume it.

/* Check if the socket has already made a connection ... */
if (socket_ptr -> nx_tcp_socket_state == NX_TCP_ESTABLISHED)
    return(NX_SUCCESS);                 /* <- read with no mutex held */
...
tx_mutex_get(&(ip_ptr -> nx_ip_protection), TX_WAIT_FOREVER);
...
_nx_tcp_socket_thread_suspend(&(socket_ptr -> nx_tcp_socket_connect_suspended_thread), ...);

The interleaving is:

1. accept() reads SYN_RECEIVED and carries on;
2. the IP thread processes the peer's final ACK, _nx_tcp_socket_state_syn_received() moves the socket to ESTABLISHED, looks at nx_tcp_socket_connect_suspended_thread and finds NX_NULL, because nobody has suspended yet;
3. accept() takes the mutex. The state is now ESTABLISHED, so the LISTEN block is skipped, and the wait_option branch at the bottom suspends the thread;
4. nothing will ever resume it. The connection is up, the peer is waiting, and the server never returns from accept().

Step 2 needs the IP thread to run between steps 1 and 3, which is exactly what happens when it is already holding nx_ip_protection to process that ACK: the calling thread blocks on tx_mutex_get(), yields, and the transition completes while it waits. So the window is not a few instructions wide -- it is as wide as one pass of the IP thread's receive processing, and it opens under load, which is when a server is most likely to be inside accept().

_nxd_tcp_client_socket_connect() already gets this right and is the model for the fix: it takes nx_ip_protection first and reads nx_tcp_socket_state afterwards, releasing the mutex on each early return. This change gives _nx_tcp_server_socket_accept() the same shape. Both early-out paths grow a tx_mutex_put(); nothing else moves.

There is no new lock ordering and no new deadlock: every path through this function that does not return early already acquired the same mutex a few lines further down, including the one where the caller is the IP thread itself inside a listen or receive callback -- ThreadX mutexes are recursive, so that case behaved this way before the change and behaves this way after it. The only cost is one uncontended acquire/release on the "already established" fast path.

Reproduced with two NX_IP instances over nx_ram_network_driver on the linux/gnu port, ThreadX and NetX Duo built at 473d192. The server arms its listener with accept(NX_NO_WAIT), then calls accept(TX_WAIT_FOREVER) while the client connects. A sleep injected immediately before tx_mutex_get() -- the point the IP thread would otherwise have to win by itself -- makes the interleaving certain:

injected delay   before        after
(ticks)
0                returns       returns
1                HUNG          returns
2                HUNG          returns
5                HUNG          returns
25               HUNG          returns

In every HUNG case the client reports NX_SUCCESS from connect(), the server's socket state is 5 (NX_TCP_ESTABLISHED), and nx_tcp_socket_connect_suspended_thread is non-NULL: a thread parked on a connection that completed before it got there. Twenty-four runs with no injected delay all returned, which is what "intermittent" means here -- the defect is in the ordering, not in the timing of any one port.

The obvious alternative repair does not work and is worth naming, because it is what an application is likely to try. Slicing the wait and calling accept() again is unsafe: on a timeout _nx_tcp_server_socket_accept() runs _nx_tcp_connect_cleanup, which winds the socket back to NX_TCP_LISTEN_STATE, so the next call re-enters the LISTEN block and sends a second SYN+ACK on a half-open connection.

Found while porting NetX Duo to m68k AmigaOS, where a blocking accept() on an established connection failed to return in one run out of four.