Stop a stalled Call Home peer from blocking configuration applies - #634
Merged
Conversation
config_update_lock is held for a whole configuration apply, which includes joining the threads of the removed Call Home clients. A thread inside a transport handshake notices neither the cleared thread_running flag nor the byte written to its notify pipe, so a single unreachable Call Home peer stalled every apply in the process, and destroying the server with it. Worst case was unbounded: SSH authentication with auth-timeout set to 0 waited in ssh_event_dopoll() forever. Give the handshake a flag it can poll. The Call Home thread lends its thread_running to the session for the duration of the handshake, exactly like the pinned configuration generation, and the SSH and TLS loops abort as soon as it is cleared. Handshakes done by nc_accept() pass no flag and are unchanged. Most loops already sleep in NC_TIMEOUT_STEP slices and only needed the check. The two libssh event loops did not, so they now cap the poll at NC_HANDSHAKE_INTERRUPT_STEP, but only when the handshake really can be interrupted, so an accepted session never wakes up more often than before. Capping the poll means SSH_AGAIN no longer implies the deadline elapsed, so both loops treat it as a spurious wakeup and let the deadline checked at the top of the loop decide. An interrupted handshake reports a timeout, which the Call Home thread now recognizes and leaves without counting a failed attempt. Joining a Call Home thread stuck in a handshake takes about 1 ms instead of the full 10 s handshake timeout.
The CURL handle used to fetch CRLs only limited the connection phase, so a distribution point that accepted the connection and then sent data slowly or not at all was never given up on. That matters beyond the one handshake it delays. The download runs in nc_session_tls_crl_verify_post_handshake(), which for a Call Home session executes on the Call Home thread while config_update_lock is held by whoever is applying a configuration, so an unresponsive CRL server stalls every configuration apply in the process. Set CURLOPT_TIMEOUT_MS so the whole transfer is bounded too.
A Call Home session is handed over to the user by new_session_cb(), called from nc_server_ch_client_thread_session_cond_wait(). Until that call succeeds the session belongs to the Call Home thread, but two exits taken before it simply abandoned the session: - the thread noticing it should stop right after the NETCONF handshake, - failing to lock ch_lock at the very start of the wait. Both leaked the whole session and the libyang context reference acquired for it, roughly 42 kB per occurrence. Free the session and release its context on both. This is safe because neither exit is reachable once new_session_cb() has been called, so the user cannot hold a pointer, and because NC_SESSION_CH_THREAD is not set yet at either point, so nc_session_free() does not wait on ch_cond for the Call Home thread to finish, which is the very thread calling it. The other error exits of the wait are all past the hand over, so they keep leaving the session alone.
nc_session_free() takes ch_lock but tolerates failing to do so, and then waits for the Call Home thread with pthread_cond_clockwait() regardless. Waiting on a condition whose mutex the caller does not own is undefined, and ch_lock is a default mutex, so this is not even guaranteed to fail cleanly with EPERM - it can corrupt the mutex or return with it locked, in which case the guarded unlock below leaves it locked forever. Only wait when the lock was really acquired and report the situation otherwise. Signaling stays unconditional, that one is allowed without the mutex. Reaching this needs a NC_SESSION_CH_LOCK_TIMEOUT timeout on a critical section of a few field assignments, so it should never happen, but the consequence of getting there was much worse than the lock failure itself.
Coverity reports CID 563283 and CID 563284, both claiming an infinite loop because the predicate of a condition wait cannot change inside the loop. That part is a false positive, the Call Home thread assigns those fields under the very same lock and broadcasts, and the loops could not spin forever anyway because a timed out wait trips an assertion. The loops did have a real weakness though. Each iteration computed a fresh deadline, so any wakeup that did not satisfy the predicate started the ten seconds over, which the callback triggers on every failed connection attempt. Compute the deadline once before the loop instead, so the total wait is what the constant says. Leaving the loop on a timed out wait rather than asserting inside it also gives the loops an exit Coverity can see, and moves the assertions out of the critical section - failing them used to abort the test with the mutex still held. The same three line loop is in test_ch_wait_for_endpt(), which Coverity did not flag, so fix all three the same way.
nc_sock_accept_pollfds() initializes its return value to 1 and only overwrites it on the paths that fail before the accept. The branch handling a failed fcntl() of the accepted socket forgot to, so it logged the error, closed the socket and then reported success without ever assigning the output socket. Both callers believe that. nc_accept() ends up passing its own -1 initializer to nc_sock_configure_ka(), which fails on a bad descriptor and hides the real error behind a confusing SO_KEEPALIVE message. nc_accept_callhome() is worse, it left its socket uninitialized, so it could have configured keepalives on, closed, or run a whole transport handshake over an unrelated descriptor. Return -1 there, and initialize the socket in nc_accept_callhome() too so that a caller that forgets to set it can never leak a stray descriptor into the rest of the function. Reported by Coverity as CID 563289.
michalvasko
approved these changes
Aug 28, 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.
A configuration apply joins the threads of the removed Call Home clients, but a thread inside a transport handshake noticed neither the cleared running flag nor its notify pipe, so one unreachable peer stalled every apply in the process, and with
auth-timeout 0the wait was unbounded. The SSH and TLS handshakes now poll a flag lent to them by the Call Home thread, so the join takes about 1 ms instead of the full 10 s handshake timeout.Also fixes, found along the way: an unbounded CRL download, a Call Home session leaked when its thread stops before the user receives it,
pthread_cond_wait()called without its mutex innc_session_free(), and an accept path reporting success without producing a socket.Also partly fixes #627