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.

[fdesbiens]

Thank you — this is an exemplary bug report, and the fix is right. I verified the whole chain rather than the premise alone, because the thing that makes this a hang rather than a hiccup is the absence of a second resume point, and that deserved checking.

The window is where you say it is. nx_tcp_server_socket_accept.c reads nx_tcp_socket_state at :84 and again at :91 with no mutex held, and only acquires nx_ip_protection at :100. If the state has become NX_TCP_ESTABLISHED by then, the if (state == NX_TCP_LISTEN_STATE) block at :102 is skipped and control falls to :175, where the suspend is unconditional — it depends on wait_option and on the caller not being the IP thread, and on nothing else.

And nothing recovers it. This is the part I most wanted to confirm. _nx_tcp_socket_state_syn_received() resumes at nx_tcp_socket_state_syn_received.c:165-170, and that check is a one-shot: it fires only if a thread is already linked at the moment the ACK is processed. Once the socket is ESTABLISHED that handler is not entered again for the connection. I enumerated every other writer of nx_tcp_socket_connect_suspended_thread and none of them helps a healthy connection:

• nx_tcp_socket_state_syn_sent.c:219 is the client path, not accept.
• nx_tcp_socket_driver_establish.c:224 is the driver-deferred establish path.
• nx_http_proxy_client.c:710 is proxy-specific.
• nx_tcp_socket_connection_reset.c:115 and nx_tcp_socket_disconnect.c:336 both route to _nx_tcp_connect_cleanup — they abort the wait, they do not complete it.

So with TX_WAIT_FOREVER on a connection that is up and healthy, the thread parks permanently. That matches your observation of a non-NULL connect_suspended_thread on a socket in state 5.

The fix closes it completely, and I want to record why, because it depends on a fact outside this file. The IP thread holds nx_ip_protection across its deferred-packet processing — nx_ip_thread_entry.c:239 acquires it at the top of each event-loop pass, and the TCP receive dispatch runs inside that. So _nx_tcp_socket_state_syn_received() always makes the SYN_RECEIVED to ESTABLISHED transition with that mutex held. With your change, accept() holds the same mutex from the state sample through to linking itself into the suspension list — _nx_tcp_socket_thread_suspend() links the thread under TX_DISABLE and only then calls tx_mutex_put() (nx_tcp_socket_thread_suspend.c, list insertion then put then _tx_thread_system_suspend). The two operations can no longer interleave in either direction: either accept() links first and the IP thread finds it, or the IP thread transitions first and accept() sees ESTABLISHED and returns.

Your three supporting claims all check out. _nxd_tcp_client_socket_connect() does take the mutex at :305 before reading state at :320, so it is a fair model. ThreadX mutexes are recursive — tx_mutex_get.c:175 increments tx_mutex_ownership_count for the owning thread — so the IP-thread-as-caller case is unaffected, and I confirmed the acquire/release counts stay balanced on all four exits. And _nx_tcp_connect_cleanup does wind the socket back to NX_TCP_LISTEN_STATE at nx_tcp_connect_cleanup.c:166, so your warning about the retry-after-timeout workaround sending a second SYN+ACK is accurate and worth having in the record.

Scope is right. I checked the other _nx_tcp_socket_thread_suspend() callers for the same shape. nx_tcp_socket_receive.c takes the mutex at :97 before its state reads at :119 and :213; nx_tcp_socket_disconnect.c takes it at :99 before :107. nx_tcp_socket_send_internal.c:312 does read state before the mutex, but it only uses it to decide whether to call _nx_tcp_socket_state_wait() and then re-checks under the mutex at :324 — a stale read there costs an optimisation, not a stranded thread. So accept() was the only place where a pre-mutex sample fed an unconditional suspend.

I have not reproduced the hang myself. Your method is sound and your numbers are convincing, and the code path is conclusive enough that a reproduction would only restate it — but say the word and I will build the two-instance harness and confirm before merge.

[fdesbiens]

Minor and cosmetic, but it sits oddly against the PR's own thesis. You place the acquire immediately after the trace insert, so NX_TRACE_IN_LINE_INSERT at :81 still reads socket_ptr -> nx_tcp_socket_state unsynchronised and can record a value that was already stale when it was captured.

That only affects trace data, and a trace of a racing read is arguably an honest record of what the caller saw — so I am not troubled by it. But moving the acquire two lines up, above the trace insert, would make the whole function consistent with the comment you wrote, costs nothing measurable, and removes the one remaining unsynchronised read of the field in this file. Entirely your call.

[fdesbiens]

Not a change request — the trade is obviously right — but the cost description in the PR deserves one qualification, because it will be read by people sizing this for small targets.

You describe the cost as "one uncontended acquire/release on the already established fast path". It is uncontended when the IP thread is idle. Under load it is contended, and that matters for one specific usage pattern: an application polling accept(NX_NO_WAIT) in a loop after the connection is up previously did zero mutex traffic on that path and now blocks on tx_mutex_get() whenever the IP thread happens to hold the mutex. So a busy poller can now be descheduled where before it span freely.

That is still the correct behaviour and enormously preferable to a permanent hang, and the same cost is already paid by _nxd_tcp_client_socket_connect() on its equivalent path. I mention it only so the changelog does not promise a free change to someone who is polling accept() in a tight loop on a heavily loaded interface.

[fdesbiens]

Worth adding to the PR description, because it raises the priority beyond "found while porting".

The FTP client add-on hits this path with a blocking wait in ordinary active-mode operation: addons/ftp/nxd_ftp_client.c:2006 and :3426 both call nx_tcp_server_socket_accept(&(ftp_client_ptr -> nx_ftp_client_data_socket), wait_option) after listening, while the server connects back for the data transfer. That is structurally identical to your reproduction — a listen armed, then a blocking accept racing an inbound connection — so any active-mode FTP transfer is a candidate, not just applications that call accept() directly.

addons/ftp/nxd_ftp_server.c:4271 also uses a real timeout (NX_FTP_SERVER_TIMEOUT) rather than NX_NO_WAIT, so it would stall for that timeout rather than forever, which is milder but still wrong.

None of this changes the fix. It does mean the bug is reachable through shipped add-ons on any port, which I would want a reader of the commit to know.

[fdesbiens]

I am not going to ask you to add a regression test, but I want to set out the reasoning rather than leave it unexplained, since it cuts differently from most PRs.

The race needs the IP thread to win a specific interleaving, and your own numbers show why an unaided test is useless: twenty-four runs with no injected delay all passed. A test that does not force the interleaving would pass on the broken code, which makes it worse than no test — it would look like coverage.

Forcing it needs the delay you injected before tx_mutex_get(), which means a test-only hook in library code. If you think that is worth carrying, a conditional under a test-only macro would do it and I would review that as a separate change; otherwise I am content that the reproduction procedure is documented in the commit message as precisely as you have written it, which is the more useful artifact.

Regression risk from the locking change itself is well covered regardless — 288 tests under test/regression/netxduo_test reference nx_tcp_server_socket_accept, and the FTP suites exercise both the blocking-application-thread path and the recursive case where the caller already owns the mutex. An unbalanced acquire or release would surface immediately.