Fix data races on session status and the Call Home thread flag - #636
Merged
Conversation
ATOMIC_STORE_RELEASE() and ATOMIC_LOAD_ACQUIRE() are needed to hand over ownership of a shared object between two threads without a mutex. The __sync fallback has no load/store builtins, so a release store is an explicit barrier followed by a plain store and an acquire load is a no-op fetch-and-add, which is already a full barrier.
ThreadSanitizer reports a data race on nc_session.status between the Call Home thread reading it in nc_server_ch_client_thread_session_cond_wait() under ch_lock and a poll thread writing it in nc_server_send_reply_io() under rpc_lock. There is no lock held by every thread that uses a session, and there cannot be one without introducing a lock ordering problem with rpc_lock/io_lock, so status is simply accessed lock-free by design. Make that explicit and free of undefined behaviour by turning status into an ATOMIC_T accessed through NC_SESSION_STATUS_GET/SET(). Silencing the race requires converting every access, not just the two the sanitizer happened to pair up. term_reason is written next to status on almost every path and read by the same poll thread, so it has exactly the same problem and is converted along with it. Refs #635
ThreadSanitizer reports a data race between the Call Home thread clearing NC_SESSION_CH_THREAD in nc_session.flags and a poll thread reading NC_SESSION_CALLHOME from the same byte in nc_ps_poll_session_io(). The flag was documented as protected by ch_lock, but a read-modify-write of a byte shared with flags that nobody locks cannot be made safe by any lock. The flag also could not keep that contract in the first place: two error paths had to write it without the lock, and two more returned without clearing it at all, leaving the session marked as owned by a Call Home thread that had already exited. nc_session_free() then burned its whole timeout waiting for nobody. Replace it with a dedicated atomic, nc_session.opts.server .ch_thread_active, so it can be cleared on every path the thread leaves by, and let nc_session_free() poll it instead of waiting on ch_cond. That drops the ch_lock dependency of the wait, so freeing a Call Home session whose lock could not be acquired now waits for the thread properly instead of logging an error and racing on. ch_cond keeps its other direction, nc_session_free() telling the Call Home thread the session is closing, so the signal stays. Fixes #635
The Call Home thread entered pthread_cond_clockwait() without ever checking the session status first, so the signal nc_session_free() sends to say the session is closing was lost whenever it fired before the thread reached the wait. The thread then slept for the full NC_CH_THREAD_IDLE_TIMEOUT_SLEEP, and since ETIMEDOUT skips the status check it took a config lock and a ch_lock re-acquisition on top of that before releasing the session. nc_session_free() waits only NC_SESSION_FREE_LOCK_TIMEOUT, which is the same 1000 ms, so it could give up first and go on to destroy ch_cond and ch_lock and free the session while the thread was still blocked on them. Turn the loop into a while so that the status is tested under ch_lock before the first wait, which is the lock nc_session_free() sets it under.
nc_ps_poll() tested the termination reason with a bitwise AND against NC_SESSION_TERM_CLOSED | NC_SESSION_TERM_KILLED, but NC_SESSION_TERM_REASON is a sequential enum, not a bitmask. That mask is 3, so DROPPED (3), BADHELLO (5) and OTHER (6) all tested non-zero and the session was reported as a clean close without NC_PSPOLL_SESSION_ERROR. Compare against both reasons explicitly, the way nc_ps_poll_sess() already does.
"Send a reply acquiring IO lock as needed. Session RPC lock must be held!" reads as if the caller had to hold the IO lock. It describes what the function does internally, the caller only has to hold the RPC lock; say so. Same for nc_ps_poll_session_io().
Several conditions read nc_session.status twice, which are two separate atomic loads. Another thread can change the status between them, so the two comparisons can be evaluated against different values and reach a conclusion that was never true at any single point in time. nc_session_ssh_msg() showed this best: it tested the status twice to decide the session is not usable and then read it a third time to name it in the log, so the message could say "invalid" about a session the condition had matched as "closing". Take one snapshot into a local and compare that. Checks that are meant to observe a change, such as the second one in nc_write_msg_io() after the message has been written, keep re-reading.
michalvasko
approved these changes
Sep 3, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
nc_session.status/term_reasonare lock-free by design and there is no lock that could cover them without introducing an ordering hazard withrpc_lock/io_lock. They are nowATOMIC_T, accessed viaNC_SESSION_STATUS_GET/SET()andNC_SESSION_TERM_REASON_GET/SET(). All accesses had to convert.NC_SESSION_CH_THREADwas a bit innc_session.flags, so clearing it was a read-modify-write on a byte shared with flags nobody locks. It is now a dedicated atomic,opts.server.ch_thread_active.nc_session_free()polls the atomic instead of waiting onch_cond, which drops thech_lockdependency of the wait.Also fixed:
pthread_cond_clockwait()without checking the status first, so the "session is closing" signal was lost if it fired before the thread reached the wait.nc_ps_poll()testedterm_reasonwith a bitwise AND against a sequential enum, soDROPPED,BADHELLOandOTHERwere reported as clean closes withoutNC_PSPOLL_SESSION_ERROR.Fixes #635