From 95d18e30041c6838e413c5ef05cd35f99b26a639 Mon Sep 17 00:00:00 2001 From: Roman Janota Date: Fri, 28 Aug 2026 10:38:49 +0200 Subject: [PATCH 1/6] session server BUGFIX interruptible CH handshake 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. --- src/session_p.h | 42 +++++++- src/session_server.c | 55 ++++++++-- src/session_server_ssh.c | 40 ++++++-- src/session_server_tls.c | 6 ++ tests/test_ch.c | 215 +++++++++++++++++++++++++++++++++++++++ 5 files changed, 342 insertions(+), 16 deletions(-) diff --git a/src/session_p.h b/src/session_p.h index 5e1c763f..3b3b3d81 100644 --- a/src/session_p.h +++ b/src/session_p.h @@ -73,6 +73,11 @@ extern struct nc_server_opts server_opts; */ #define NC_TRANSPORT_MSG_TIMEOUT 2000 +/** + * Maximum time in msec a transport handshake may block before it checks whether it was interrupted. + */ +#define NC_HANDSHAKE_INTERRUPT_STEP 100 + /** * Timeout in msec for acquiring a lock of a session (used with a condition, so higher numbers could be required * only in case of extreme concurrency). @@ -1120,6 +1125,16 @@ struct nc_session { const struct nc_server_config *config; #ifdef NC_ENABLED_SSH_TLS + /** + * @brief Running flag of the Call Home thread performing the transport handshake. + * + * A borrowed pointer to ::nc_server_ch_thread_arg.thread_running, NOT owned by the + * session - the handshake is aborted as soon as it becomes 0. NULL for a handshake that + * cannot be interrupted, which is every handshake done by ::nc_accept(). Set and cleared + * by ::nc_connect_ch_endpt() exactly like ::nc_session.opts.server.config. + */ + ATOMIC_T *ch_thread_running; + uint16_t ssh_auth_attempts; /**< number of failed SSH authentication attempts */ void *client_cert; /**< TLS client certificate if used for authentication */ #endif /* NC_ENABLED_SSH_TLS */ @@ -1655,13 +1670,36 @@ int _nc_connect_ch_client_dispatch(const char *client_name, nc_server_ch_session */ struct nc_session *nc_accept_callhome_ssh_sock(int sock, const char *host, uint16_t port, struct ly_ctx *ctx); +/** + * @brief Check whether the transport handshake of a session should be aborted. + * + * Only a handshake performed by a Call Home thread can be interrupted, see + * ::nc_session.opts.server.ch_thread_running. Handshakes of accepted sessions are never interrupted. + * + * @param[in] session Session performing a transport handshake. + * @return 1 if the handshake should be aborted, 0 otherwise. + */ +int nc_session_handshake_interrupted(const struct nc_session *session); + +/** + * @brief Cap a transport handshake poll timeout so that an interrupt is noticed in time. + * + * A handshake that cannot be interrupted gets @p timeout unchanged, there is nothing it could + * notice by waking up before the data it is waiting for arrive. + * + * @param[in] session Session performing a transport handshake. + * @param[in] timeout Timeout in msec the handshake would like to wait for, negative means indefinitely. + * @return Timeout in msec to actually use, never negative for an interruptible handshake. + */ +int32_t nc_session_handshake_poll_timeout(const struct nc_session *session, int32_t timeout); + /** * @brief Establish SSH transport on a socket. * * @param[in] session Session structure of the new connection. * @param[in] opts SSH server options to use. * @param[in] sock Socket of the new connection, closed if not set to the session. - * @return 1 on success, 0 on timeout, -1 on error. + * @return 1 on success, 0 on timeout or interrupt, -1 on error. */ int nc_accept_ssh_session(struct nc_session *session, struct nc_server_ssh_opts *opts, int sock); @@ -1677,7 +1715,7 @@ struct nc_session *nc_accept_callhome_tls_sock(int sock, const char *host, uint1 * @param[in] session Session structure of the new connection. * @param[in] sock Socket of the new connection. * @param[in] timeout Transport operations timeout in msec. - * @return 1 on success, 0 on timeout, -1 on error. + * @return 1 on success, 0 on timeout or interrupt, -1 on error. */ int nc_accept_tls_session(struct nc_session *session, struct nc_server_tls_opts *opts, int sock); diff --git a/src/session_server.c b/src/session_server.c index ba94b0e1..fd641189 100644 --- a/src/session_server.c +++ b/src/session_server.c @@ -3744,6 +3744,35 @@ nc_accept(int timeout, const struct ly_ctx *ctx, struct nc_session **session) #ifdef NC_ENABLED_SSH_TLS +int +nc_session_handshake_interrupted(const struct nc_session *session) +{ + ATOMIC_T *ch_thread_running = session->opts.server.ch_thread_running; + + if (!ch_thread_running) { + /* not a Call Home handshake, there is nobody to interrupt it */ + return 0; + } + + return !ATOMIC_LOAD_RELAXED(*ch_thread_running); +} + +int32_t +nc_session_handshake_poll_timeout(const struct nc_session *session, int32_t timeout) +{ + if (!session->opts.server.ch_thread_running) { + /* nothing can interrupt the handshake, there is no reason to wake up early */ + return timeout; + } + + /* a negative timeout means waiting indefinitely, which must not happen if we have to notice an interrupt */ + if ((timeout < 0) || (timeout > NC_HANDSHAKE_INTERRUPT_STEP)) { + return NC_HANDSHAKE_INTERRUPT_STEP; + } + + return timeout; +} + API int nc_server_ch_is_client(const char *name) { @@ -3808,6 +3837,8 @@ nc_server_ch_client_is_endpt(const char *client_name, const char *endpt_name) * @param[in] config Pinned server configuration @p endpt belongs to, pinned into the created session * for the duration of the transport handshake. * @param[in] endpt Endpoint to use. + * @param[in] ch_thread_running Running flag of the calling Call Home thread, the transport handshake + * is aborted as soon as it becomes 0. * @param[in,out] cur_sock_pending Current pending socket for the connection. * @param[in] acquire_ctx_cb Callback for acquiring the libyang context. * @param[in] release_ctx_cb Callback for releasing the libyang context. @@ -3816,9 +3847,9 @@ nc_server_ch_client_is_endpt(const char *client_name, const char *endpt_name) * @return NC_MSG values. */ static NC_MSG_TYPE -nc_connect_ch_endpt(const struct nc_server_config *config, const struct nc_ch_endpt *endpt, int *cur_sock_pending, - nc_server_ch_session_acquire_ctx_cb acquire_ctx_cb, nc_server_ch_session_release_ctx_cb release_ctx_cb, - void *ctx_cb_data, struct nc_session **session) +nc_connect_ch_endpt(const struct nc_server_config *config, const struct nc_ch_endpt *endpt, + ATOMIC_T *ch_thread_running, int *cur_sock_pending, nc_server_ch_session_acquire_ctx_cb acquire_ctx_cb, + nc_server_ch_session_release_ctx_cb release_ctx_cb, void *ctx_cb_data, struct nc_session **session) { NC_MSG_TYPE msgtype; const struct ly_ctx *ctx = NULL; @@ -3856,6 +3887,9 @@ nc_connect_ch_endpt(const struct nc_server_config *config, const struct nc_ch_en /* pin the configuration for the duration of the transport handshake, it is a borrowed pointer */ (*session)->opts.server.config = config; + /* let the handshake be aborted as soon as this thread is told to stop, also a borrowed pointer */ + (*session)->opts.server.ch_thread_running = ch_thread_running; + /* sock gets assigned to session or closed */ if (endpt->ti == NC_TI_SSH) { ret = nc_accept_ssh_session(*session, endpt->opts.ssh, sock); @@ -3887,8 +3921,10 @@ nc_connect_ch_endpt(const struct nc_server_config *config, const struct nc_ch_en goto fail; } - /* the transport handshake is over, the configuration must not be reached through the session anymore */ + /* the transport handshake is over, neither the configuration nor the running flag must be + * reached through the session anymore */ (*session)->opts.server.config = NULL; + (*session)->opts.server.ch_thread_running = NULL; /* assign new SID atomically */ (*session)->id = ATOMIC_INC_RELAXED(server_opts.new_session_id); @@ -3910,6 +3946,7 @@ nc_connect_ch_endpt(const struct nc_server_config *config, const struct nc_ch_en fail: if (*session) { (*session)->opts.server.config = NULL; + (*session)->opts.server.ch_thread_running = NULL; } nc_session_free(*session, NULL); *session = NULL; @@ -4245,8 +4282,8 @@ nc_ch_client_thread(void *arg) } /* try to connect to the endpoint, the configuration stays pinned for the whole handshake */ - msgtype = nc_connect_ch_endpt(config, cur_endpt, &cur_sock_pending, data->acquire_ctx_cb, - data->release_ctx_cb, data->ctx_cb_data, &session); + msgtype = nc_connect_ch_endpt(config, cur_endpt, &data->thread_running, &cur_sock_pending, + data->acquire_ctx_cb, data->release_ctx_cb, data->ctx_cb_data, &session); if (msgtype == NC_MSG_HELLO) { /* session established, the configuration is not needed anymore */ nc_server_config_release(config); @@ -4331,6 +4368,12 @@ nc_ch_client_thread(void *arg) } cur_attempts = 0; } else { + if (!ATOMIC_LOAD_RELAXED(data->thread_running)) { + /* the handshake was interrupted because this thread should stop, do not count it as + * a failed attempt and do not bother the user with it */ + goto cleanup; + } + /* session was not created, wait a little bit and try again */ ++cur_attempts; diff --git a/src/session_server_ssh.c b/src/session_server_ssh.c index ef127d5a..1ef46b0d 100644 --- a/src/session_server_ssh.c +++ b/src/session_server_ssh.c @@ -1625,6 +1625,11 @@ nc_accept_ssh_session_open_netconf_channel(struct nc_session *session, struct nc return -1; } + if (nc_session_handshake_interrupted(session)) { + VRB(session, "Waiting for the \"netconf\" SSH subsystem interrupted, the Call Home thread is terminating."); + return 0; + } + time_diff = nc_timeouttime_cur_diff(&ts_timeout); if (time_diff < 1) { /* timeout */ @@ -1633,14 +1638,13 @@ nc_accept_ssh_session_open_netconf_channel(struct nc_session *session, struct nc } /* This functions listens to the network and automatically calls callback funcitons. */ - ret = ssh_event_dopoll(session->ti.libssh.event, time_diff); + ret = ssh_event_dopoll(session->ti.libssh.event, nc_session_handshake_poll_timeout(session, time_diff)); if (ret == SSH_ERROR) { ERR(session, "Failed to poll SSH event (%s).", ssh_get_error(session->ti.libssh.session)); return -1; - } else if (ret == SSH_AGAIN) { - /* Timeout reached */ - break; } + /* SSH_AGAIN only means the poll timeout elapsed, which may have been shortened to notice an + * interrupt, so ts_timeout checked at the top of the loop is the only authority */ } if (session->flags & NC_SESSION_SSH_SUBSYS_NETCONF) { @@ -1666,6 +1670,11 @@ nc_accept_ssh_session_open_netconf_channel(struct nc_session *session, struct nc return 1; } + if (nc_session_handshake_interrupted(session)) { + VRB(session, "Waiting for the \"netconf\" SSH subsystem interrupted, the Call Home thread is terminating."); + return 0; + } + usleep(NC_TIMEOUT_STEP); if (nc_timeouttime_cur_diff(&ts_timeout) < 1) { /* timeout */ @@ -1761,6 +1770,11 @@ nc_accept_ssh_session_auth(struct nc_session *session, struct nc_server_ssh_opts return -1; } + if (nc_session_handshake_interrupted(session)) { + VRB(session, "SSH authentication interrupted, the Call Home thread is terminating."); + return 0; + } + if (opts->auth_timeout) { time_diff = nc_timeouttime_cur_diff(&ts_timeout); if (time_diff < 1) { @@ -1773,14 +1787,13 @@ nc_accept_ssh_session_auth(struct nc_session *session, struct nc_server_ssh_opts } /* This functions listens to the network and automatically calls callback funcitons. */ - ret = ssh_event_dopoll(event, time_diff); + ret = ssh_event_dopoll(event, nc_session_handshake_poll_timeout(session, time_diff)); if (ret == SSH_ERROR) { ERR(session, "Failed to poll SSH event (%s).", ssh_get_error(session->ti.libssh.session)); return -1; - } else if (ret == SSH_AGAIN) { - /* Timeout reached */ - break; } + /* SSH_AGAIN only means the poll timeout elapsed, which may have been shortened to notice an + * interrupt, so ts_timeout checked at the top of the loop is the only authority */ } #else while (1) { @@ -1801,6 +1814,11 @@ nc_accept_ssh_session_auth(struct nc_session *session, struct nc_server_ssh_opts break; } + if (nc_session_handshake_interrupted(session)) { + VRB(session, "SSH authentication interrupted, the Call Home thread is terminating."); + return 0; + } + usleep(NC_TIMEOUT_STEP); if (opts->auth_timeout && (nc_timeouttime_cur_diff(&ts_timeout) < 1)) { /* timeout */ @@ -1964,6 +1982,12 @@ nc_accept_ssh_session(struct nc_session *session, struct nc_server_ssh_opts *opt DBG(session, "Performing SSH key exchange..."); nc_timeouttime_get(&ts_timeout, NC_TRANSPORT_HANDSHAKE_TIMEOUT); while ((r = ssh_handle_key_exchange(session->ti.libssh.session)) == SSH_AGAIN) { + if (nc_session_handshake_interrupted(session)) { + VRB(session, "SSH key exchange interrupted, the Call Home thread is terminating."); + rc = 0; + goto cleanup; + } + /* this tends to take longer */ usleep(NC_TIMEOUT_STEP * 20); if (nc_timeouttime_cur_diff(&ts_timeout) < 1) { diff --git a/src/session_server_tls.c b/src/session_server_tls.c index d3a7226f..77cfac26 100644 --- a/src/session_server_tls.c +++ b/src/session_server_tls.c @@ -997,6 +997,12 @@ nc_accept_tls_session(struct nc_session *session, struct nc_server_tls_opts *opt /* do the handshake */ nc_timeouttime_get(&ts_timeout, NC_TRANSPORT_HANDSHAKE_TIMEOUT); while ((rc = nc_server_tls_handshake_step_wrap(session->ti.tls.session)) == 0) { + if (nc_session_handshake_interrupted(session)) { + VRB(session, "TLS handshake interrupted, the Call Home thread is terminating."); + timeouted = 1; + goto fail; + } + usleep(NC_TIMEOUT_STEP); if (nc_timeouttime_cur_diff(&ts_timeout) < 1) { ERR(session, "TLS accept timeout."); diff --git a/tests/test_ch.c b/tests/test_ch.c index 6be19740..95053200 100644 --- a/tests/test_ch.c +++ b/tests/test_ch.c @@ -15,13 +15,18 @@ #define _GNU_SOURCE +#include #include +#include #include #include #include #include #include #include +#include +#include +#include #include @@ -758,6 +763,214 @@ test_nc_ch_two_simultaneous(void **state) } } +/* + * Test: a stalled transport handshake must not block a configuration apply + * + * The Call Home client connects to a plain TCP listener that accepts the connection and then stays + * silent, so the thread ends up stuck in the transport handshake. Deleting the client from the + * configuration has to interrupt the handshake, otherwise the apply waits for the whole + * NC_TRANSPORT_HANDSHAKE_TIMEOUT (10 seconds by default) before the thread can be joined. + */ + +/* maximum time in msec the apply may take, an order of magnitude below the handshake timeout */ +#define TEST_CH_INTERRUPT_LIMIT 3000 + +static int interrupt_listen_sock = -1; +static char interrupt_port_str[16]; + +/** + * @brief Start listening on a loopback port without ever speaking any protocol on it. + * + * @return Listening socket. + */ +static int +test_ch_silent_listen(void) +{ + int sock, opt = 1; + struct sockaddr_in saddr = {0}; + socklen_t saddr_len = sizeof saddr; + + sock = socket(AF_INET, SOCK_STREAM, 0); + assert_int_not_equal(sock, -1); + assert_int_equal(setsockopt(sock, SOL_SOCKET, SO_REUSEADDR, &opt, sizeof opt), 0); + + saddr.sin_family = AF_INET; + saddr.sin_addr.s_addr = htonl(INADDR_LOOPBACK); + /* an ephemeral port, so that this never collides with the ports assigned by CTest */ + saddr.sin_port = 0; + assert_int_equal(bind(sock, (struct sockaddr *)&saddr, sizeof saddr), 0); + assert_int_equal(listen(sock, 1), 0); + + assert_int_equal(getsockname(sock, (struct sockaddr *)&saddr, &saddr_len), 0); + sprintf(interrupt_port_str, "%" PRIu16, ntohs(saddr.sin_port)); + + return sock; +} + +/** + * @brief Let the Call Home client connect, then delete it and measure how long the apply took. + * + * @param[in] state Test state. + */ +static void +test_ch_interrupt_apply(void **state) +{ + int ret, sock; + int64_t elapsed_ms; + struct timespec ts_start, ts_end; + struct nc_pollsession *ps; + struct ln2_test_ctx *test_ctx = *state; + struct test_ch_data *test_data = test_ctx->test_data; + + assert_non_null(state); + + ps = nc_ps_new(); + assert_non_null(ps); + + /* start the Call Home thread, it connects to the silent listener */ + ret = nc_connect_ch_client_dispatch("ch_interrupt", ch_session_acquire_ctx_cb, + ch_session_release_ctx_cb, test_ctx, ch_new_session_cb, ps); + assert_int_equal(ret, 0); + + /* accept the connection but do not say anything, the thread is now in the transport handshake */ + sock = accept(interrupt_listen_sock, NULL, NULL); + assert_int_not_equal(sock, -1); + + /* make sure the thread really got into the handshake loop */ + usleep(100000); + + /* delete the client, this joins its thread */ + clock_gettime(CLOCK_MONOTONIC, &ts_start); + ret = nc_server_config_setup_data(test_data->tree); + assert_int_equal(ret, 0); + clock_gettime(CLOCK_MONOTONIC, &ts_end); + + elapsed_ms = ((int64_t)ts_end.tv_sec - ts_start.tv_sec) * 1000 + + ((int64_t)ts_end.tv_nsec - ts_start.tv_nsec) / 1000000; + printf("apply with a stalled handshake took %" PRId64 " ms\n", elapsed_ms); + assert_true(elapsed_ms < TEST_CH_INTERRUPT_LIMIT); + + close(sock); + close(interrupt_listen_sock); + interrupt_listen_sock = -1; + + nc_ps_clear(ps, 1, NULL); + nc_ps_free(ps); +} + +static int +setup_interrupt_ssh(void **state) +{ + int ret; + struct lyd_node *tree = NULL; + struct ln2_test_ctx *test_ctx; + struct test_ch_data *test_data; + + ret = ln2_glob_test_setup(&test_ctx); + assert_int_equal(ret, 0); + + test_data = calloc(1, sizeof *test_data); + assert_non_null(test_data); + + test_ctx->test_data = test_data; + test_ctx->free_test_data = test_nc_ch_free_test_data; + *state = test_ctx; + + interrupt_listen_sock = test_ch_silent_listen(); + + ret = nc_server_config_add_ch_address_port(test_ctx->ctx, "ch_interrupt", "endpt", NC_TI_SSH, + "127.0.0.1", interrupt_port_str, &tree); + assert_int_equal(ret, 0); + + ret = nc_server_config_add_ch_persistent(test_ctx->ctx, "ch_interrupt", &tree); + assert_int_equal(ret, 0); + + ret = nc_server_config_add_ch_ssh_hostkey(test_ctx->ctx, "ch_interrupt", "endpt", "hostkey", + TESTS_DIR "/data/key_ecdsa", NULL, &tree); + assert_int_equal(ret, 0); + + ret = nc_server_config_add_ch_ssh_user_pubkey(test_ctx->ctx, "ch_interrupt", "endpt", "test_ch_interrupt", + "pubkey", TESTS_DIR "/data/id_ed25519.pub", &tree); + assert_int_equal(ret, 0); + + ret = nc_server_config_setup_data(tree); + assert_int_equal(ret, 0); + + /* prepare the configuration without the client, applied by the test itself */ + ret = nc_server_config_del_ch_client("ch_interrupt", &tree); + assert_int_equal(ret, 0); + + test_data->tree = tree; + return 0; +} + +static int +setup_interrupt_tls(void **state) +{ + int ret; + struct lyd_node *tree = NULL; + struct ln2_test_ctx *test_ctx; + struct test_ch_data *test_data; + + ret = ln2_glob_test_setup(&test_ctx); + assert_int_equal(ret, 0); + + test_data = calloc(1, sizeof *test_data); + assert_non_null(test_data); + + test_ctx->test_data = test_data; + test_ctx->free_test_data = test_nc_ch_free_test_data; + *state = test_ctx; + + interrupt_listen_sock = test_ch_silent_listen(); + + ret = nc_server_config_add_ch_address_port(test_ctx->ctx, "ch_interrupt", "endpt", NC_TI_TLS, + "127.0.0.1", interrupt_port_str, &tree); + assert_int_equal(ret, 0); + + ret = nc_server_config_add_ch_persistent(test_ctx->ctx, "ch_interrupt", &tree); + assert_int_equal(ret, 0); + + ret = nc_server_config_add_ch_tls_server_cert(test_ctx->ctx, "ch_interrupt", "endpt", + TESTS_DIR "/data/server.key", NULL, TESTS_DIR "/data/server.crt", &tree); + assert_int_equal(ret, 0); + + ret = nc_server_config_add_ch_tls_client_cert(test_ctx->ctx, "ch_interrupt", "endpt", "ee-cert", + TESTS_DIR "/data/client.crt", &tree); + assert_int_equal(ret, 0); + + ret = nc_server_config_add_ch_tls_ca_cert(test_ctx->ctx, "ch_interrupt", "endpt", "ca-cert", + TESTS_DIR "/data/serverca.pem", &tree); + assert_int_equal(ret, 0); + + ret = nc_server_config_add_ch_tls_ctn(test_ctx->ctx, "ch_interrupt", "endpt", 1, + "04:85:6B:75:D1:1A:86:E0:D8:FE:5B:BD:72:F5:73:1D:07:EA:32:BF:09:11:21:6A:6E:23:78:8E:B6:D5:73:C3:2D", + NC_TLS_CTN_SPECIFIED, "ch_client_tls", &tree); + assert_int_equal(ret, 0); + + ret = nc_server_config_setup_data(tree); + assert_int_equal(ret, 0); + + /* prepare the configuration without the client, applied by the test itself */ + ret = nc_server_config_del_ch_client("ch_interrupt", &tree); + assert_int_equal(ret, 0); + + test_data->tree = tree; + return 0; +} + +static void +test_nc_ch_interrupt_ssh_handshake(void **state) +{ + test_ch_interrupt_apply(state); +} + +static void +test_nc_ch_interrupt_tls_handshake(void **state) +{ + test_ch_interrupt_apply(state); +} + int main(void) { @@ -766,6 +979,8 @@ main(void) cmocka_unit_test_setup_teardown(test_nc_ch_tls, setup_tls, ln2_glob_test_teardown), cmocka_unit_test_setup_teardown(test_nc_ch_delete_client_while_session, setup_delete_while_session, ln2_glob_test_teardown), cmocka_unit_test_setup_teardown(test_nc_ch_two_simultaneous, setup_two_simultaneous, ln2_glob_test_teardown), + cmocka_unit_test_setup_teardown(test_nc_ch_interrupt_ssh_handshake, setup_interrupt_ssh, ln2_glob_test_teardown), + cmocka_unit_test_setup_teardown(test_nc_ch_interrupt_tls_handshake, setup_interrupt_tls, ln2_glob_test_teardown), }; if (ln2_glob_test_get_ports(4, &TEST_PORT, &TEST_PORT_STR, &TEST_PORT_2, &TEST_PORT_2_STR, From 640acf4e9dc39e325f74bac2371240707a1005cc Mon Sep 17 00:00:00 2001 From: Roman Janota Date: Fri, 28 Aug 2026 11:02:28 +0200 Subject: [PATCH 2/6] session BUGFIX bound the whole CRL download 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. --- src/session.c | 6 ++++++ src/session_p.h | 10 ++++++++++ 2 files changed, 16 insertions(+) diff --git a/src/session.c b/src/session.c index 4d779c81..be9415a0 100644 --- a/src/session.c +++ b/src/session.c @@ -2108,6 +2108,12 @@ nc_session_curl_init(CURL **handle, struct nc_curl_data *data) return 1; } + /* limit the whole transfer, a host that connects and then stalls would block the TLS handshake */ + if (curl_easy_setopt(*handle, CURLOPT_TIMEOUT_MS, NC_CURL_TIMEOUT_MS)) { + ERR(NULL, "Setting curl transfer timeout failed."); + return 1; + } + /* do not use signals for timeouts, required for thread safety */ if (curl_easy_setopt(*handle, CURLOPT_NOSIGNAL, 1L)) { ERR(NULL, "Setting CURLOPT_NOSIGNAL failed."); diff --git a/src/session_p.h b/src/session_p.h index 3b3b3d81..6e0de203 100644 --- a/src/session_p.h +++ b/src/session_p.h @@ -134,6 +134,16 @@ extern struct nc_server_opts server_opts; */ #define NC_CURL_CONNECT_TIMEOUT_MS 2000 +/** + * @brief Timeout in msec for a whole CRL download. + * + * The connection timeout alone does not bound a CRL distribution point that accepts the connection + * and then sends data very slowly or not at all. Since the download happens in the middle of a TLS + * handshake, and a Call Home handshake blocks a configuration apply, the whole transfer has to be + * bounded as well. + */ +#define NC_CURL_TIMEOUT_MS 10000 + /** * @brief Timeout in msec for acquiring the hello_lock * (iterating through all YANG modules + building capability strings) From 37fc8401996115428cab83cdbab249a14297daea Mon Sep 17 00:00:00 2001 From: Roman Janota Date: Fri, 28 Aug 2026 11:15:27 +0200 Subject: [PATCH 3/6] session server BUGFIX free unhanded CH session 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. --- src/session_server.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/src/session_server.c b/src/session_server.c index fd641189..cd555740 100644 --- a/src/session_server.c +++ b/src/session_server.c @@ -3991,6 +3991,10 @@ nc_server_ch_client_get_idle_timeout(const char *client_name, uint32_t *idle_tim /** * @brief Wait for any event after a NC session was established on a CH client. * + * The session is given to the user by ::nc_server_ch_thread_arg.new_session_cb. Until that + * succeeds the session still belongs to the Call Home thread, so it is freed here on any error. + * Afterwards it belongs to the user and is never freed here. + * * @param[in] data CH client thread argument. * @param[in] session New NC session. The session is invalid upon being freed (= function exit). * @return 0 if session was terminated normally, @@ -4006,6 +4010,9 @@ nc_server_ch_client_thread_session_cond_wait(struct nc_server_ch_thread_arg *dat /* CH LOCK */ if (nc_mutex_lock(&session->opts.server.ch_lock, NC_SESSION_CH_LOCK_TIMEOUT, __func__) != 1) { + /* the session has not been given to the user yet, so it is still ours to free */ + nc_session_free(session, NULL); + data->release_ctx_cb(data->ctx_cb_data); return -1; } @@ -4292,7 +4299,11 @@ nc_ch_client_thread(void *arg) cur_endpt = NULL; if (!ATOMIC_LOAD_RELAXED(data->thread_running)) { - /* thread should stop running */ + /* thread should stop running, the session has not been given to the user yet, + * so it is still ours to free */ + nc_session_free(session, NULL); + session = NULL; + data->release_ctx_cb(data->ctx_cb_data); goto cleanup; } From 0afb1d53d825a27537f5435d003cc07325380d73 Mon Sep 17 00:00:00 2001 From: Roman Janota Date: Fri, 28 Aug 2026 11:29:17 +0200 Subject: [PATCH 4/6] session BUGFIX wait on ch_cond only when locked 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. --- src/session.c | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-) diff --git a/src/session.c b/src/session.c index be9415a0..d5954fc9 100644 --- a/src/session.c +++ b/src/session.c @@ -1171,17 +1171,24 @@ nc_session_free(struct nc_session *session, void (*data_free)(void *)) session->status = NC_STATUS_CLOSING; if ((session->side == NC_SERVER) && (session->flags & NC_SESSION_CH_THREAD)) { + /* signaling a condition does not require its mutex to be held */ pthread_cond_signal(&session->opts.server.ch_cond); - nc_timeouttime_get(&ts, NC_SESSION_FREE_LOCK_TIMEOUT); + if (ch_locked) { + nc_timeouttime_get(&ts, NC_SESSION_FREE_LOCK_TIMEOUT); - /* wait for CH thread to actually wake up and terminate */ - r = 0; - while (!r && (session->flags & NC_SESSION_CH_THREAD)) { - r = pthread_cond_clockwait(&session->opts.server.ch_cond, &session->opts.server.ch_lock, COMPAT_CLOCK_ID, &ts); - } - if (r) { - ERR(session, "Waiting for Call Home thread failed (%s).", strerror(r)); + /* wait for CH thread to actually wake up and terminate */ + r = 0; + while (!r && (session->flags & NC_SESSION_CH_THREAD)) { + r = pthread_cond_clockwait(&session->opts.server.ch_cond, &session->opts.server.ch_lock, COMPAT_CLOCK_ID, &ts); + } + if (r) { + ERR(session, "Waiting for Call Home thread failed (%s).", strerror(r)); + } + } else { + /* waiting on a condition requires its mutex to be held by the caller, so there is no + * way to wait for the Call Home thread without ch_lock */ + ERR(session, "Freeing a Call Home session without its lock, not waiting for its thread."); } } From 508b4397164a9cbbea6ffa5c052eb2b088822b70 Mon Sep 17 00:00:00 2001 From: Roman Janota Date: Fri, 28 Aug 2026 14:05:53 +0200 Subject: [PATCH 5/6] tests config UPDATE bound the Call Home waits 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. --- tests/test_config.c | 49 +++++++++++++++++++++++++++++++-------------- 1 file changed, 34 insertions(+), 15 deletions(-) diff --git a/tests/test_config.c b/tests/test_config.c index 78fb0b11..17f0c455 100644 --- a/tests/test_config.c +++ b/tests/test_config.c @@ -1149,6 +1149,9 @@ static unsigned int test_stall_auth_sleep = TEST_STALL_AUTH_SLEEP; /** @brief Maximum number of distinct Call Home threads the test keeps track of. */ #define TEST_CH_TID_MAX 8 +/** @brief Time in seconds to wait for a Call Home client to report what a test is waiting for. */ +#define TEST_CH_WAIT_TIME 10 + struct test_ch_threads { pthread_mutex_t lock; pthread_cond_t cond; @@ -1295,16 +1298,22 @@ test_ch_dispatch_not_duplicated(void **state) ret = nc_server_config_setup_data(tree); assert_int_equal(ret, 0); - /* wait until its thread reports a failed connection attempt */ + /* wait until its thread reports a failed connection attempt, the deadline is absolute so that + * repeated wakeups cannot extend the wait indefinitely */ + ret = 0; pthread_mutex_lock(&threads.lock); - while (!threads.tid_count) { - clock_gettime(CLOCK_REALTIME, &ts); - ts.tv_sec += 10; + clock_gettime(CLOCK_REALTIME, &ts); + ts.tv_sec += TEST_CH_WAIT_TIME; + while (!threads.tid_count && !ret) { ret = pthread_cond_timedwait(&threads.cond, &threads.lock, &ts); - assert_int_equal(ret, 0); } + tid_count = threads.tid_count; pthread_mutex_unlock(&threads.lock); + /* only report the failure once the lock is released */ + assert_int_equal(ret, 0); + assert_int_not_equal(tid_count, 0); + /* apply the very same data again, the client is already running */ ret = nc_server_config_setup_data(tree); assert_int_equal(ret, 0); @@ -1334,6 +1343,7 @@ static void test_ch_endpoint_order(void **state) { int ret; + char endpt[64] = {0}; struct lyd_node *tree = NULL, *diff = NULL; struct ln2_test_ctx *test_ctx = *state; struct test_ch_threads threads = {0}; @@ -1381,17 +1391,22 @@ test_ch_endpoint_order(void **state) test_ch_new_session_cb, NULL); assert_int_equal(ret, 0); - /* wait for the first failed connection attempt */ + /* wait for the first failed connection attempt, the deadline is absolute so that repeated + * wakeups cannot extend the wait indefinitely */ + ret = 0; pthread_mutex_lock(&threads.lock); - while (!threads.endpt[0]) { - clock_gettime(CLOCK_REALTIME, &ts); - ts.tv_sec += 10; + clock_gettime(CLOCK_REALTIME, &ts); + ts.tv_sec += TEST_CH_WAIT_TIME; + while (!threads.endpt[0] && !ret) { ret = pthread_cond_timedwait(&threads.cond, &threads.lock, &ts); - assert_int_equal(ret, 0); } - assert_string_equal(threads.endpt, "second"); + strncpy(endpt, threads.endpt, sizeof endpt - 1); pthread_mutex_unlock(&threads.lock); + /* only report the failure once the lock is released */ + assert_int_equal(ret, 0); + assert_string_equal(endpt, "second"); + lyd_free_all(diff); lyd_free_all(tree); pthread_cond_destroy(&threads.cond); @@ -1687,14 +1702,18 @@ test_ch_wait_for_endpt(struct test_ch_threads *threads, const char *endpt_name) int ret; struct timespec ts; + /* the deadline is absolute so that repeated wakeups cannot extend the wait indefinitely */ + ret = 0; pthread_mutex_lock(&threads->lock); - while (strcmp(threads->last_endpt, endpt_name)) { - clock_gettime(CLOCK_REALTIME, &ts); - ts.tv_sec += 10; + clock_gettime(CLOCK_REALTIME, &ts); + ts.tv_sec += TEST_CH_WAIT_TIME; + while (strcmp(threads->last_endpt, endpt_name) && !ret) { ret = pthread_cond_timedwait(&threads->cond, &threads->lock, &ts); - assert_int_equal(ret, 0); } pthread_mutex_unlock(&threads->lock); + + /* only report the failure once the lock is released */ + assert_int_equal(ret, 0); } /** From 506320bdf9d74cad5b414bdba31790c25eef7790 Mon Sep 17 00:00:00 2001 From: Roman Janota Date: Fri, 28 Aug 2026 14:23:47 +0200 Subject: [PATCH 6/6] session server BUGFIX fail a socket accept error 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. --- src/session_client.c | 2 +- src/session_server.c | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/src/session_client.c b/src/session_client.c index 6af5d98b..a947d569 100644 --- a/src/session_client.c +++ b/src/session_client.c @@ -1847,7 +1847,7 @@ nc_client_ch_del_bind(const char *address, uint16_t port, NC_TRANSPORT_IMPL ti) API int nc_accept_callhome(int timeout, struct ly_ctx *ctx, struct nc_session **session) { - int ret, sock; + int ret, sock = -1; char *host = NULL; uint16_t port, bind_idx = 0; diff --git a/src/session_server.c b/src/session_server.c index cd555740..568691e2 100644 --- a/src/session_server.c +++ b/src/session_server.c @@ -1087,6 +1087,7 @@ nc_sock_accept_pollfds(struct pollfd *pollfds, uint16_t pollfd_count, const char /* make the socket non-blocking */ if (((flags = fcntl(client_sock, F_GETFL)) == -1) || (fcntl(client_sock, F_SETFL, flags | O_NONBLOCK) == -1)) { ERR(NULL, "Fcntl failed (%s).", strerror(errno)); + ret = -1; goto cleanup; }