From 3796262f91aceb31f2043573edd40b1fd2903ca6 Mon Sep 17 00:00:00 2001 From: Roman Janota Date: Mon, 13 Apr 2026 15:33:17 +0200 Subject: [PATCH 1/9] session server UPDATE CH thread lifecycle management Store thread IDs in ch_thread_arg to allow proper pthread_join when stopping threads. Add nc_session_server_ch_client_dispatch_stop_if_dispatched() to centralize thread stopping logic, fixing several locking issues: - Only unlock ch_lock if it was successfully acquired in nc_session_free() - Use config_update_lock to prevent data race in nc_server_destroy() - Properly handle config lock unlocking during thread join to avoid deadlock --- src/server_config.c | 172 +++++++++++++++++++------------------------ src/session.c | 28 +++++-- src/session_p.h | 12 +++ src/session_server.c | 139 ++++++++++++++++++++++++---------- 4 files changed, 209 insertions(+), 142 deletions(-) diff --git a/src/server_config.c b/src/server_config.c index ccf309d9..e79883e0 100644 --- a/src/server_config.c +++ b/src/server_config.c @@ -3941,7 +3941,7 @@ config_netconf_client(const struct lyd_node *node, enum nc_operation parent_op, struct lyd_node *n; enum nc_operation op; const char *name; - LY_ARRAY_COUNT_TYPE i = 0, j = 0; + LY_ARRAY_COUNT_TYPE i = 0; struct nc_ch_client *ch_client = NULL; NC_NODE_GET_OP(node, parent_op, &op); @@ -3988,30 +3988,6 @@ config_netconf_client(const struct lyd_node *node, enum nc_operation parent_op, /* all children processed, we can now delete the client */ if (op == NC_OP_DELETE) { - /* CH THREADS DATA RD LOCK */ - if (nc_rwlock_lock(&server_opts.ch_threads_lock, NC_RWLOCK_READ, NC_CH_THREADS_LOCK_TIMEOUT, __func__) != 1) { - return 1; - } - - /* find the thread data for this CH client, not found <==> CH thread not running */ - LY_ARRAY_FOR(server_opts.ch_threads, j) { - if (!strcmp(server_opts.ch_threads[j]->client_name, name)) { - break; - } - } - - if (j < LY_ARRAY_COUNT(server_opts.ch_threads)) { - /* the CH thread is running, notify it to stop */ - if (nc_mutex_lock(&server_opts.ch_threads[j]->cond_lock, NC_CH_COND_LOCK_TIMEOUT, __func__) != 1) { - server_opts.ch_threads[j]->thread_running = 0; - pthread_cond_signal(&server_opts.ch_threads[j]->cond); - nc_mutex_unlock(&server_opts.ch_threads[j]->cond_lock, __func__); - } - } - - /* CH THREADS DATA UNLOCK */ - nc_rwlock_unlock(&server_opts.ch_threads_lock, __func__); - /* we can use 'i' from above */ if (i < LY_ARRAY_COUNT(config->ch_clients) - 1) { config->ch_clients[i] = config->ch_clients[LY_ARRAY_COUNT(config->ch_clients) - 1]; @@ -5330,6 +5306,37 @@ nc_server_config_reconcile_sockets_listen(struct nc_server_config *old_cfg, #ifdef NC_ENABLED_SSH_TLS +/** + * @brief Check if there are any new Call Home clients created in the new configuration. + * + * @param[in] old_cfg Old, currently active server configuration. + * @param[in] new_cfg New server configuration currently being applied. + * @return 1 if there are new CH clients, 0 otherwise. + */ +static int +nc_server_config_new_ch_clients_created(struct nc_server_config *old_cfg, struct nc_server_config *new_cfg) +{ + struct nc_ch_client *old_ch_client, *new_ch_client; + int found; + + /* check if there are any new clients */ + LY_ARRAY_FOR(new_cfg->ch_clients, struct nc_ch_client, new_ch_client) { + found = 0; + LY_ARRAY_FOR(old_cfg->ch_clients, struct nc_ch_client, old_ch_client) { + if (!strcmp(new_ch_client->name, old_ch_client->name)) { + found = 1; + break; + } + } + if (!found) { + return 1; + } + } + + /* no differences found */ + return 0; +} + /** * @brief Atomically dispatch new Call Home clients and reuse existing ones. * @@ -5347,11 +5354,16 @@ nc_server_config_reconcile_chclients_dispatch(struct nc_server_config *old_cfg, int found; LY_ARRAY_COUNT_TYPE i = 0; char **started_clients = NULL, **client_name = NULL; + int dispatch_new_clients = 1; if (!server_opts.ch_dispatch_data.acquire_ctx_cb || !server_opts.ch_dispatch_data.release_ctx_cb || !server_opts.ch_dispatch_data.new_session_cb) { - /* Call Home dispatch callbacks not set, nothing to do */ - return 0; + /* Call Home dispatch callbacks not set, we can't dispatch new clients, but we can still stop deleted ones */ + if (nc_server_config_new_ch_clients_created(old_cfg, new_cfg)) { + WRN(NULL, "New Call Home clients were created but Call Home dispatch callbacks are not set - " + "new clients will not be dispatched automatically."); + } + dispatch_new_clients = 0; } /* @@ -5359,43 +5371,46 @@ nc_server_config_reconcile_chclients_dispatch(struct nc_server_config *old_cfg, * Start clients present in new_cfg that are not already running. * Track successfully started clients for potential rollback. */ - LY_ARRAY_FOR(new_cfg->ch_clients, struct nc_ch_client, new_ch_client) { - found = 0; + if (dispatch_new_clients) { + /* only dispatch if all required CBs are set */ + LY_ARRAY_FOR(new_cfg->ch_clients, struct nc_ch_client, new_ch_client) { + found = 0; - /* CH THREADS LOCK (reading server_opts.ch_threads) */ - if (nc_rwlock_lock(&server_opts.ch_threads_lock, NC_RWLOCK_READ, NC_CH_THREADS_LOCK_TIMEOUT, __func__) != 1) { - rc = 1; - goto rollback; - } + /* CH THREADS LOCK (reading server_opts.ch_threads) */ + if (nc_rwlock_lock(&server_opts.ch_threads_lock, NC_RWLOCK_READ, NC_CH_THREADS_LOCK_TIMEOUT, __func__) != 1) { + rc = 1; + goto rollback; + } - LY_ARRAY_FOR(server_opts.ch_threads, struct nc_server_ch_thread_arg *, ch_thread_arg) { - if (!strcmp(new_ch_client->name, (*ch_thread_arg)->client_name)) { - /* already running, do not start again */ - found = 1; - break; + LY_ARRAY_FOR(server_opts.ch_threads, struct nc_server_ch_thread_arg *, ch_thread_arg) { + if (!strcmp(new_ch_client->name, (*ch_thread_arg)->client_name)) { + /* already running, do not start again */ + found = 1; + break; + } } - } - /* CH THREADS UNLOCK */ - nc_rwlock_unlock(&server_opts.ch_threads_lock, __func__); + /* CH THREADS UNLOCK */ + nc_rwlock_unlock(&server_opts.ch_threads_lock, __func__); - if (!found) { - /* this is a new Call Home client, dispatch it */ - rc = _nc_connect_ch_client_dispatch(new_ch_client, server_opts.ch_dispatch_data.acquire_ctx_cb, - server_opts.ch_dispatch_data.release_ctx_cb, server_opts.ch_dispatch_data.ctx_cb_data, - server_opts.ch_dispatch_data.new_session_cb, server_opts.ch_dispatch_data.new_session_cb_data); - if (rc) { - /* FAILURE! trigger rollback */ - goto rollback; - } + if (!found) { + /* this is a new Call Home client, dispatch it */ + rc = _nc_connect_ch_client_dispatch(new_ch_client, server_opts.ch_dispatch_data.acquire_ctx_cb, + server_opts.ch_dispatch_data.release_ctx_cb, server_opts.ch_dispatch_data.ctx_cb_data, + server_opts.ch_dispatch_data.new_session_cb, server_opts.ch_dispatch_data.new_session_cb_data); + if (rc) { + /* FAILURE! trigger rollback */ + goto rollback; + } - /* successfully started, track it for potential rollback */ - LY_ARRAY_NEW_GOTO(NULL, started_clients, client_name, rc, rollback); - *client_name = strdup(new_ch_client->name); - NC_CHECK_ERRMEM_GOTO(!*client_name, rc = 1, rollback); + /* successfully started, track it for potential rollback */ + LY_ARRAY_NEW_GOTO(NULL, started_clients, client_name, rc, rollback); + *client_name = strdup(new_ch_client->name); + NC_CHECK_ERRMEM_GOTO(!*client_name, rc = 1, rollback); - /* ownership transferred to array */ - client_name = NULL; + /* ownership transferred to array */ + client_name = NULL; + } } } @@ -5415,27 +5430,10 @@ nc_server_config_reconcile_chclients_dispatch(struct nc_server_config *old_cfg, if (!found) { /* this Call Home client was deleted, notify it to stop */ - ch_thread_arg = NULL; - - /* CH THREADS LOCK (reading server_opts.ch_threads) */ - if (nc_rwlock_lock(&server_opts.ch_threads_lock, NC_RWLOCK_READ, NC_CH_THREADS_LOCK_TIMEOUT, __func__) != 1) { - /* Continue even if lock fails - best effort cleanup */ - continue; - } - LY_ARRAY_FOR(server_opts.ch_threads, struct nc_server_ch_thread_arg *, ch_thread_arg) { - if (!strcmp(old_ch_client->name, (*ch_thread_arg)->client_name)) { - /* notify the thread to stop */ - if (nc_mutex_lock(&(*ch_thread_arg)->cond_lock, NC_CH_COND_LOCK_TIMEOUT, __func__) != 1) { - (*ch_thread_arg)->thread_running = 0; - pthread_cond_signal(&(*ch_thread_arg)->cond); - nc_mutex_unlock(&(*ch_thread_arg)->cond_lock, __func__); - } - break; - } + if ((rc = nc_session_server_ch_client_dispatch_stop_if_dispatched(old_ch_client->name, NC_RWLOCK_WRITE))) { + ERR(NULL, "Failed to dispatch stop for Call Home client \"%s\".", old_ch_client->name); + goto rollback; } - /* CH THREADS UNLOCK */ - nc_rwlock_unlock(&server_opts.ch_threads_lock, __func__); - /* Note: if ch_thread_arg is NULL here, the thread wasn't running. That's fine. */ } } @@ -5450,25 +5448,7 @@ nc_server_config_reconcile_chclients_dispatch(struct nc_server_config *old_cfg, * to return to the pre-call state. */ LY_ARRAY_FOR(started_clients, i) { - ch_thread_arg = NULL; - - /* CH THREADS LOCK (reading server_opts.ch_threads) */ - if (nc_rwlock_lock(&server_opts.ch_threads_lock, NC_RWLOCK_READ, NC_CH_THREADS_LOCK_TIMEOUT, __func__) != 1) { - /* Continue even if lock fails - best effort rollback */ - continue; - } - LY_ARRAY_FOR(server_opts.ch_threads, struct nc_server_ch_thread_arg *, ch_thread_arg) { - if (!strcmp(started_clients[i], (*ch_thread_arg)->client_name)) { - /* notify the newly started thread to stop */ - nc_mutex_lock(&(*ch_thread_arg)->cond_lock, NC_CH_COND_LOCK_TIMEOUT, __func__); - (*ch_thread_arg)->thread_running = 0; - pthread_cond_signal(&(*ch_thread_arg)->cond); - nc_mutex_unlock(&(*ch_thread_arg)->cond_lock, __func__); - break; - } - } - /* CH THREADS UNLOCK */ - nc_rwlock_unlock(&server_opts.ch_threads_lock, __func__); + nc_session_server_ch_client_dispatch_stop_if_dispatched(started_clients[i], NC_RWLOCK_WRITE); } /* rc is already set to non-zero from the failure point */ diff --git a/src/session.c b/src/session.c index b3b2c694..ddbf2d06 100644 --- a/src/session.c +++ b/src/session.c @@ -910,7 +910,7 @@ nc_session_free_transport(struct nc_session *session, int *multisession) * NC_CLIENT: we are waiting for the server to acknowledge our SSH channel EOF * by sending us its own SSH channel EOF. */ if (ssh_channel_poll_timeout(session->ti.libssh.channel, NC_SESSION_FREE_SSH_POLL_EOF_TIMEOUT, 0) != SSH_EOF) { - WRN(session, "Timeout for receiving SSH channel EOF from the peer elapsed."); + WRN(session, "Timeout for receiving SSH channel EOF from the %s elapsed.", session->side == NC_CLIENT ? "server" : "client"); } } ssh_channel_free(session->ti.libssh.channel); @@ -1001,7 +1001,7 @@ nc_session_free_transport(struct nc_session *session, int *multisession) API void nc_session_free(struct nc_session *session, void (*data_free)(void *)) { - int r, i, rpc_locked = 0; + int r, i, rpc_locked = 0, ch_locked = 0; int multisession = 0; /* flag for more NETCONF sessions on a single SSH session */ struct timespec ts; NC_STATUS status; @@ -1012,7 +1012,9 @@ nc_session_free(struct nc_session *session, void (*data_free)(void *)) if ((session->side == NC_SERVER) && (session->flags & NC_SESSION_CALLHOME)) { /* CH LOCK, continue on error */ - r = nc_mutex_lock(&session->opts.server.ch_lock, NC_SESSION_CH_LOCK_TIMEOUT, __func__); + if (nc_mutex_lock(&session->opts.server.ch_lock, NC_SESSION_CH_LOCK_TIMEOUT, __func__) == 1) { + ch_locked = 1; + } } /* store status, so we can check if this session is already closing */ @@ -1020,7 +1022,7 @@ nc_session_free(struct nc_session *session, void (*data_free)(void *)) if ((session->side == NC_SERVER) && (session->flags & NC_SESSION_CALLHOME)) { /* CH UNLOCK */ - if (r == 1) { + if (ch_locked) { /* only if we locked it */ nc_mutex_unlock(&session->opts.server.ch_lock, __func__); } @@ -1060,8 +1062,12 @@ nc_session_free(struct nc_session *session, void (*data_free)(void *)) nc_client_msgs_free(session); } - if (session->status == NC_STATUS_RUNNING) { - /* notify the peer that we're closing the session */ + /* notify the peer that we're closing the session, either if: + * - session running - normal disconnect from client + * - session invalid - client disconnected from a Call Home session */ + if ((session->status == NC_STATUS_RUNNING) || + ((session->side == NC_SERVER) && (session->flags & NC_SESSION_CALLHOME) && + (session->status == NC_STATUS_INVALID) && (session->term_reason == NC_SESSION_TERM_CLOSED))) { if (session->side == NC_CLIENT) { /* graceful close: + transport shutdown indication */ nc_session_free_client_close_graceful(session); @@ -1073,7 +1079,10 @@ nc_session_free(struct nc_session *session, void (*data_free)(void *)) if ((session->side == NC_SERVER) && (session->flags & NC_SESSION_CALLHOME)) { /* CH LOCK */ - nc_mutex_lock(&session->opts.server.ch_lock, NC_SESSION_CH_LOCK_TIMEOUT, __func__); + ch_locked = 0; + if (nc_mutex_lock(&session->opts.server.ch_lock, NC_SESSION_CH_LOCK_TIMEOUT, __func__) == 1) { + ch_locked = 1; + } } /* mark session for closing */ @@ -1096,7 +1105,10 @@ nc_session_free(struct nc_session *session, void (*data_free)(void *)) if ((session->side == NC_SERVER) && (session->flags & NC_SESSION_CALLHOME)) { /* CH UNLOCK */ - nc_mutex_unlock(&session->opts.server.ch_lock, __func__); + if (ch_locked) { + /* only if we locked it */ + nc_mutex_unlock(&session->opts.server.ch_lock, __func__); + } } /* transport implementation cleanup */ diff --git a/src/session_p.h b/src/session_p.h index 1cebecf1..60c34ef6 100644 --- a/src/session_p.h +++ b/src/session_p.h @@ -692,6 +692,7 @@ struct nc_server_ch_thread_arg { uint8_t cur_attempt, void *user_data); /**< failed to create a new session cb */ void *new_session_fail_cb_data; /**< new session fail cb data */ + pthread_t tid; /**< Thread ID of the Call Home client thread. */ int thread_running; /**< A boolean value that is truthy while the underlying Call Home thread is running */ pthread_mutex_t cond_lock; /**< Condition's lock used for signalling the thread to terminate */ pthread_cond_t cond; /**< Condition used for signalling the thread to terminate */ @@ -1387,6 +1388,17 @@ NC_MSG_TYPE nc_connect_callhome(const char *host, uint16_t port, NC_TRANSPORT_IM #ifdef NC_ENABLED_SSH_TLS +/** + * @brief Stop a dispatched Call Home client thread, if such thread was dispatched for the given client name. + * + * @param[in] client_name Name of the Call Home client to stop the thread for. + * @param[in] config_lock_mode Lock mode of the configuration lock, if it is held by the caller. + * If NC_RWLOCK_WRITE is passed, config lock will be briefly unlocked and then locked again. + * The caller MUST ensure that it holds the CONFIG APPLY mutex, so that nobody steals the wrlock from him. + * @return 0 if the thread was successfully stopped or not found, 1 on error. + */ +int nc_session_server_ch_client_dispatch_stop_if_dispatched(const char *client_name, enum nc_rwlock_mode config_lock_mode); + /** * @brief Dispatch a thread connecting to a listening NETCONF client and creating Call Home sessions. * diff --git a/src/session_server.c b/src/session_server.c index 3a7ef140..c50b88f4 100644 --- a/src/session_server.c +++ b/src/session_server.c @@ -1299,6 +1299,8 @@ nc_server_init(void) API void nc_server_destroy(void) { + int config_update_locked = 0; + enum nc_rwlock_mode config_lock_mode = NC_RWLOCK_NONE; uint32_t i; for (i = 0; i < server_opts.capabilities_count; i++) { @@ -1316,27 +1318,28 @@ nc_server_destroy(void) nc_server_notif_cert_expiration_thread_stop(1); #endif /* NC_ENABLED_SSH_TLS */ - /* CONFIG WR LOCK */ - nc_rwlock_lock(&server_opts.config_lock, NC_RWLOCK_WRITE, NC_CONFIG_LOCK_TIMEOUT, __func__); - - /* destroy the server configuration */ - nc_server_config_free(&server_opts.config); + /* CONFIG UPDATE LOCK, continue on error */ + if (nc_mutex_lock(&server_opts.config_update_lock, NC_CONFIG_LOCK_TIMEOUT, __func__) == 1) { + config_update_locked = 1; + } - /* CH THREADS LOCK */ - nc_rwlock_lock(&server_opts.ch_threads_lock, NC_RWLOCK_READ, NC_CH_THREADS_LOCK_TIMEOUT, __func__); + /* CONFIG WR LOCK, continue on error */ + if (nc_rwlock_lock(&server_opts.config_lock, NC_RWLOCK_WRITE, NC_CONFIG_LOCK_TIMEOUT, __func__) == 1) { + config_lock_mode = NC_RWLOCK_WRITE; + } - /* notify the CH threads to exit */ - LY_ARRAY_FOR(server_opts.ch_threads, i) { - if (nc_mutex_lock(&server_opts.ch_threads[i]->cond_lock, NC_CH_COND_LOCK_TIMEOUT, __func__) != 1) { - continue; +#ifdef NC_ENABLED_SSH_TLS + if (!config_update_locked || (config_lock_mode != NC_RWLOCK_WRITE)) { + WRN(NULL, "Config locks not acquired, skipping Call Home threads cleanup."); + } else { + /* stop all dispatched CH threads */ + LY_ARRAY_FOR(server_opts.config.ch_clients, i) { + nc_session_server_ch_client_dispatch_stop_if_dispatched(server_opts.config.ch_clients[i].name, config_lock_mode); } - server_opts.ch_threads[i]->thread_running = 0; - pthread_cond_signal(&server_opts.ch_threads[i]->cond); - nc_mutex_unlock(&server_opts.ch_threads[i]->cond_lock, __func__); } - /* CH THREADS UNLOCK */ - nc_rwlock_unlock(&server_opts.ch_threads_lock, __func__); + /* destroy the server configuration */ + nc_server_config_free(&server_opts.config); #ifdef NC_ENABLED_SSH_TLS free(server_opts.authkey_path_fmt); @@ -1359,7 +1362,14 @@ nc_server_destroy(void) server_opts.unix_paths = NULL; /* CONFIG WR UNLOCK */ - nc_rwlock_unlock(&server_opts.config_lock, __func__); + if (config_lock_mode != NC_RWLOCK_NONE) { + nc_rwlock_unlock(&server_opts.config_lock, __func__); + } + + /* CONFIG UPDATE UNLOCK */ + if (config_update_locked) { + nc_mutex_unlock(&server_opts.config_update_lock, __func__); + } #ifdef NC_ENABLED_SSH_TLS curl_global_cleanup(); @@ -3753,29 +3763,85 @@ nc_ch_client_thread(void *arg) } int -_nc_connect_ch_client_dispatch(const struct nc_ch_client *ch_client, nc_server_ch_session_acquire_ctx_cb acquire_ctx_cb, - nc_server_ch_session_release_ctx_cb release_ctx_cb, void *ctx_cb_data, nc_server_ch_new_session_cb new_session_cb, - void *new_session_cb_data) +nc_session_server_ch_client_dispatch_stop_if_dispatched(const char *client_name, enum nc_rwlock_mode config_lock_mode) { int rc = 0, r; - enum nc_rwlock_mode ch_threads_lock = NC_RWLOCK_NONE; + struct nc_server_ch_thread_arg **ch_thread_arg; pthread_t tid; - struct nc_server_ch_thread_arg *arg = NULL, **new_item; - pthread_attr_t attr; + enum nc_rwlock_mode ch_threads_lock = NC_RWLOCK_NONE; - /* init pthread attribute */ - if ((r = pthread_attr_init(&attr))) { - ERR(NULL, "Initializing pthread attributes failed (%s).", strerror(r)); - return -1; + /* CH THREADS LOCK */ + if (nc_rwlock_lock(&server_opts.ch_threads_lock, NC_RWLOCK_READ, NC_CH_THREADS_LOCK_TIMEOUT, __func__) != 1) { + return 1; } + ch_threads_lock = NC_RWLOCK_READ; - /* set the thread to be detached */ - if ((r = pthread_attr_setdetachstate(&attr, PTHREAD_CREATE_DETACHED))) { - ERR(NULL, "Setting pthread attributes to detached failed (%s).", strerror(r)); - rc = -1; - goto cleanup; + if (config_lock_mode == NC_RWLOCK_WRITE) { + /* CONFIG UNLOCK - if the caller holds the config lock in write mode, we need to unlock it + * to prevent deadlock with the CH thread, it tries to acquire the config lock in read mode when it + * checks if the client still exists. + * It is the caller's responsibility to hold config apply mutex, so noone steals the write lock from him */ + nc_rwlock_unlock(&server_opts.config_lock, __func__); + } + + LY_ARRAY_FOR(server_opts.ch_threads, struct nc_server_ch_thread_arg *, ch_thread_arg) { + if (strcmp(client_name, (*ch_thread_arg)->client_name)) { + continue; + } + + /* CH COND LOCK */ + if (nc_mutex_lock(&(*ch_thread_arg)->cond_lock, NC_CH_COND_LOCK_TIMEOUT, __func__) != 1) { + rc = 1; + goto cleanup; + } + + /* notify the thread to stop */ + (*ch_thread_arg)->thread_running = 0; + tid = (*ch_thread_arg)->tid; + pthread_cond_signal(&(*ch_thread_arg)->cond); + + /* CH COND UNLOCK */ + nc_mutex_unlock(&(*ch_thread_arg)->cond_lock, __func__); + + /* CH THREADS UNLOCK - let the thread free itself from the global array, it needs WR lock for that */ + nc_rwlock_unlock(&server_opts.ch_threads_lock, __func__); + ch_threads_lock = NC_RWLOCK_NONE; + + /* wait for the thread to end */ + r = pthread_join(tid, NULL); + if (r) { + ERR(NULL, "Joining Call Home client \"%s\" thread failed (%s).", client_name, strerror(r)); + rc = 1; + goto cleanup; + } + break; } +cleanup: + if (config_lock_mode == NC_RWLOCK_WRITE) { + /* CONFIG LOCK - lock it back if we unlocked it. It MUST succeed, if the caller holds the config apply mutex */ + if (nc_rwlock_lock(&server_opts.config_lock, NC_RWLOCK_WRITE, -1, __func__) != 1) { + ERRINT; + } + } + + if (ch_threads_lock) { + /* CH THREADS UNLOCK */ + nc_rwlock_unlock(&server_opts.ch_threads_lock, __func__); + } + + return rc; +} + +int +_nc_connect_ch_client_dispatch(const struct nc_ch_client *ch_client, nc_server_ch_session_acquire_ctx_cb acquire_ctx_cb, + nc_server_ch_session_release_ctx_cb release_ctx_cb, void *ctx_cb_data, nc_server_ch_new_session_cb new_session_cb, + void *new_session_cb_data) +{ + int rc = 0, r; + enum nc_rwlock_mode ch_threads_lock = NC_RWLOCK_NONE; + struct nc_server_ch_thread_arg *arg = NULL, **new_item; + /* create the thread argument */ arg = calloc(1, sizeof *arg); NC_CHECK_ERRMEM_GOTO(!arg, rc = -1, cleanup); @@ -3804,13 +3870,11 @@ _nc_connect_ch_client_dispatch(const struct nc_ch_client *ch_client, nc_server_c LY_ARRAY_NEW_GOTO(NULL, server_opts.ch_threads, new_item, rc, cleanup); *new_item = arg; - /* CH THREADS UNLOCK */ - nc_rwlock_unlock(&server_opts.ch_threads_lock, __func__); - ch_threads_lock = NC_RWLOCK_NONE; - /* create the CH thread */ - if ((r = pthread_create(&tid, &attr, nc_ch_client_thread, arg))) { + if ((r = pthread_create(&arg->tid, NULL, nc_ch_client_thread, arg))) { ERR(NULL, "Creating a new thread failed (%s).", strerror(r)); + /* remove from the array */ + LY_ARRAY_DECREMENT_FREE(server_opts.ch_threads); rc = -1; goto cleanup; } @@ -3823,7 +3887,6 @@ _nc_connect_ch_client_dispatch(const struct nc_ch_client *ch_client, nc_server_c /* CH THREADS UNLOCK */ nc_rwlock_unlock(&server_opts.ch_threads_lock, __func__); } - pthread_attr_destroy(&attr); if (arg) { free(arg->client_name); free(arg); From fa4515bb2095be68246b5b7bf56ab01fe690263c Mon Sep 17 00:00:00 2001 From: Roman Janota Date: Mon, 13 Apr 2026 16:12:34 +0200 Subject: [PATCH 2/9] session server BUGFIX compile without ssh_tls --- src/session_server.c | 1 + src/session_server.h | 4 ++++ 2 files changed, 5 insertions(+) diff --git a/src/session_server.c b/src/session_server.c index c50b88f4..bc464a2c 100644 --- a/src/session_server.c +++ b/src/session_server.c @@ -1337,6 +1337,7 @@ nc_server_destroy(void) nc_session_server_ch_client_dispatch_stop_if_dispatched(server_opts.config.ch_clients[i].name, config_lock_mode); } } +#endif /* NC_ENABLED_SSH_TLS */ /* destroy the server configuration */ nc_server_config_free(&server_opts.config); diff --git a/src/session_server.h b/src/session_server.h index 59b2483d..be477046 100644 --- a/src/session_server.h +++ b/src/session_server.h @@ -448,6 +448,8 @@ NC_MSG_TYPE nc_session_accept_ssh_channel(struct nc_session *orig_session, struc */ NC_MSG_TYPE nc_ps_accept_ssh_channel(struct nc_pollsession *ps, struct nc_session **session); +#endif /* NC_ENABLED_SSH_TLS */ + /** * @brief Set the UNIX socket path for a given endpoint name. * @@ -491,6 +493,8 @@ int nc_server_get_unix_socket_dir(char **dir); /** @} Server Session */ +#ifdef NC_ENABLED_SSH_TLS + /** * @defgroup server_ssh Server SSH * @ingroup server From 434c4289bbadfbaffffbb0f0ce195cf4d493d3f2 Mon Sep 17 00:00:00 2001 From: Roman Janota Date: Tue, 14 Apr 2026 09:46:48 +0200 Subject: [PATCH 3/9] ci UPDATE use ubuntu 24.04 Ubuntu 22.04 uses libssh-dev v0.9.6, which contains a bug where multiple threads call strtok, which isn't present in v0.10.6 that ubuntu 24.04 uses. --- .github/workflows/ci.yml | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 06675f4c..8fcf6320 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -38,7 +38,7 @@ jobs: config: - { name: "Release, gcc, OpenSSL", - os: "ubuntu-22.04", + os: "ubuntu-24.04", build-type: "Release", dep-build-type: "Release", cc: "gcc", @@ -51,7 +51,7 @@ jobs: } - { name: "Release, gcc, MbedTLS", - os: "ubuntu-22.04", + os: "ubuntu-24.04", build-type: "Release", dep-build-type: "Release", cc: "gcc", @@ -64,7 +64,7 @@ jobs: } - { name: "Release, clang", - os: "ubuntu-22.04", + os: "ubuntu-24.04", build-type: "Release", dep-build-type: "Release", cc: "clang", @@ -77,7 +77,7 @@ jobs: } - { name: "Debug, gcc, OpenSSL", - os: "ubuntu-22.04", + os: "ubuntu-24.04", build-type: "Debug", dep-build-type: "Release", cc: "gcc", @@ -90,7 +90,7 @@ jobs: } - { name: "Debug, gcc, MbedTLS", - os: "ubuntu-22.04", + os: "ubuntu-24.04", build-type: "Debug", dep-build-type: "Release", cc: "gcc", @@ -103,7 +103,7 @@ jobs: } - { name: "Debug, clang", - os: "ubuntu-22.04", + os: "ubuntu-24.04", build-type: "Debug", dep-build-type: "Release", cc: "clang", @@ -117,7 +117,7 @@ jobs: } - { name: "No SSH nor TLS", - os: "ubuntu-22.04", + os: "ubuntu-24.04", build-type: "Debug", dep-build-type: "Release", cc: "gcc", @@ -130,7 +130,7 @@ jobs: } - { name: "ASAN and UBSAN, OpenSSL", - os: "ubuntu-22.04", + os: "ubuntu-24.04", build-type: "Debug", dep-build-type: "Release", cc: "clang", @@ -143,7 +143,7 @@ jobs: } - { name: "ASAN and UBSAN, MbedTLS", - os: "ubuntu-22.04", + os: "ubuntu-24.04", build-type: "Debug", dep-build-type: "Release", cc: "clang", @@ -156,7 +156,7 @@ jobs: } - { name: "DEB Package", - os: "ubuntu-22.04", + os: "ubuntu-24.04", build-type: "Release", dep-build-type: "Release", cc: "gcc", From c5ac09e295b3806c4e020a3f32db1e188202d30a Mon Sep 17 00:00:00 2001 From: Roman Janota Date: Tue, 14 Apr 2026 11:03:56 +0000 Subject: [PATCH 4/9] session server UPDATE nc_server_destroy return value Change return type from void to int to allow callers to handle cleanup failures. Also update nc_server_notif_cert_expiration_thread_stop to return error code so that nc_server_destroy can properly propagate errors. --- src/session_server.c | 17 +++++++++++++---- src/session_server.h | 7 +++++-- 2 files changed, 18 insertions(+), 6 deletions(-) diff --git a/src/session_server.c b/src/session_server.c index bc464a2c..50b5770c 100644 --- a/src/session_server.c +++ b/src/session_server.c @@ -1296,9 +1296,10 @@ nc_server_init(void) return -1; } -API void +API int nc_server_destroy(void) { + int rc = 0; int config_update_locked = 0; enum nc_rwlock_mode config_lock_mode = NC_RWLOCK_NONE; uint32_t i; @@ -1315,7 +1316,10 @@ nc_server_destroy(void) #ifdef NC_ENABLED_SSH_TLS /* destroy the certificate expiration notification thread */ - nc_server_notif_cert_expiration_thread_stop(1); + if ((rc = nc_server_notif_cert_expiration_thread_stop(1))) { + ERR(NULL, "%s: failed to stop certificate expiration notification thread.", __func__); + goto cleanup; + } #endif /* NC_ENABLED_SSH_TLS */ /* CONFIG UPDATE LOCK, continue on error */ @@ -1382,6 +1386,9 @@ nc_server_destroy(void) fclose(server_opts.tls_keylog_file); } #endif /* NC_ENABLED_SSH_TLS */ + +cleanup: + return rc; } API int @@ -4753,7 +4760,7 @@ nc_server_notif_cert_expiration_thread_start(nc_cert_exp_notif_clb cert_exp_noti return ret; } -API void +API int nc_server_notif_cert_expiration_thread_stop(int wait) { int r; @@ -4761,7 +4768,7 @@ nc_server_notif_cert_expiration_thread_stop(int wait) /* LOCK */ if (nc_mutex_lock(&server_opts.cert_exp_notif.lock, NC_CERT_EXP_LOCK_TIMEOUT, __func__) != 1) { - return; + return 1; } tid = server_opts.cert_exp_notif.tid; @@ -4780,12 +4787,14 @@ nc_server_notif_cert_expiration_thread_stop(int wait) } if (r) { ERR(NULL, "Stopping the certificate expiration notification thread failed (%s).", strerror(r)); + return 1; } } else { /* thread is not running */ /* UNLOCK */ nc_mutex_unlock(&server_opts.cert_exp_notif.lock, __func__); } + return 0; } #endif /* NC_ENABLED_SSH_TLS */ diff --git a/src/session_server.h b/src/session_server.h index be477046..8d6645c7 100644 --- a/src/session_server.h +++ b/src/session_server.h @@ -144,8 +144,10 @@ int nc_server_init(void); /** * @brief Destroy any dynamically allocated libssh and/or libssl/libcrypto and * server resources. + * + * @return 0 on success, 1 on error - all resources could not be freed safely. */ -void nc_server_destroy(void); +int nc_server_destroy(void); /** * @brief Initialize a context which can serve as a default server context. @@ -669,8 +671,9 @@ int nc_server_notif_cert_expiration_thread_start(nc_cert_exp_notif_clb cert_exp_ * @brief Stop the certificate expiration notification thread. * * @param[in] wait Boolean representing whether to block and wait for the thread to finish. + * @return 0 on success, 1 on error. */ -void nc_server_notif_cert_expiration_thread_stop(int wait); +int nc_server_notif_cert_expiration_thread_stop(int wait); #endif /* NC_ENABLED_SSH_TLS */ From 731bd6575b864dd6073dd412bb6832189c1e923b Mon Sep 17 00:00:00 2001 From: Roman Janota Date: Tue, 14 Apr 2026 11:04:20 +0000 Subject: [PATCH 5/9] session server REFACTOR move ch thread data to ch_client Move Call Home thread data from global server_opts.ch_threads array to individual ch_client->thread pointer. This simplifies thread management by eliminating the separate ch_threads_lock and array manipulation. Each ch_client now directly owns its thread data, making the lifecycle management more straightforward. --- src/server_config.c | 66 ++++++---------- src/session_p.h | 24 +++--- src/session_server.c | 177 +++++++++++++++---------------------------- 3 files changed, 95 insertions(+), 172 deletions(-) diff --git a/src/server_config.c b/src/server_config.c index e79883e0..0e1e5536 100644 --- a/src/server_config.c +++ b/src/server_config.c @@ -5350,10 +5350,9 @@ nc_server_config_reconcile_chclients_dispatch(struct nc_server_config *old_cfg, { int rc = 0; struct nc_ch_client *old_ch_client, *new_ch_client; - struct nc_server_ch_thread_arg **ch_thread_arg; int found; - LY_ARRAY_COUNT_TYPE i = 0; - char **started_clients = NULL, **client_name = NULL; + LY_ARRAY_COUNT_TYPE i; + struct nc_ch_client **started_clients = NULL, **started_client_ptr; int dispatch_new_clients = 1; if (!server_opts.ch_dispatch_data.acquire_ctx_cb || !server_opts.ch_dispatch_data.release_ctx_cb || @@ -5369,48 +5368,28 @@ nc_server_config_reconcile_chclients_dispatch(struct nc_server_config *old_cfg, /* * == PHASE 1: START NEW CLIENTS == * Start clients present in new_cfg that are not already running. - * Track successfully started clients for potential rollback. + * Track successfully started threads for potential rollback. */ if (dispatch_new_clients) { /* only dispatch if all required CBs are set */ LY_ARRAY_FOR(new_cfg->ch_clients, struct nc_ch_client, new_ch_client) { - found = 0; - - /* CH THREADS LOCK (reading server_opts.ch_threads) */ - if (nc_rwlock_lock(&server_opts.ch_threads_lock, NC_RWLOCK_READ, NC_CH_THREADS_LOCK_TIMEOUT, __func__) != 1) { - rc = 1; - goto rollback; + if (new_ch_client->thread) { + /* already running */ + continue; } - LY_ARRAY_FOR(server_opts.ch_threads, struct nc_server_ch_thread_arg *, ch_thread_arg) { - if (!strcmp(new_ch_client->name, (*ch_thread_arg)->client_name)) { - /* already running, do not start again */ - found = 1; - break; - } + /* this is a new Call Home client, dispatch it */ + rc = _nc_connect_ch_client_dispatch(new_ch_client, server_opts.ch_dispatch_data.acquire_ctx_cb, + server_opts.ch_dispatch_data.release_ctx_cb, server_opts.ch_dispatch_data.ctx_cb_data, + server_opts.ch_dispatch_data.new_session_cb, server_opts.ch_dispatch_data.new_session_cb_data); + if (rc) { + /* FAILURE! trigger rollback */ + goto rollback; } - /* CH THREADS UNLOCK */ - nc_rwlock_unlock(&server_opts.ch_threads_lock, __func__); - - if (!found) { - /* this is a new Call Home client, dispatch it */ - rc = _nc_connect_ch_client_dispatch(new_ch_client, server_opts.ch_dispatch_data.acquire_ctx_cb, - server_opts.ch_dispatch_data.release_ctx_cb, server_opts.ch_dispatch_data.ctx_cb_data, - server_opts.ch_dispatch_data.new_session_cb, server_opts.ch_dispatch_data.new_session_cb_data); - if (rc) { - /* FAILURE! trigger rollback */ - goto rollback; - } - - /* successfully started, track it for potential rollback */ - LY_ARRAY_NEW_GOTO(NULL, started_clients, client_name, rc, rollback); - *client_name = strdup(new_ch_client->name); - NC_CHECK_ERRMEM_GOTO(!*client_name, rc = 1, rollback); - - /* ownership transferred to array */ - client_name = NULL; - } + /* successfully started, track client for potential rollback */ + LY_ARRAY_NEW_GOTO(NULL, started_clients, started_client_ptr, rc, rollback); + *started_client_ptr = new_ch_client; } } @@ -5428,9 +5407,9 @@ nc_server_config_reconcile_chclients_dispatch(struct nc_server_config *old_cfg, } } - if (!found) { + if (!found && old_ch_client->thread) { /* this Call Home client was deleted, notify it to stop */ - if ((rc = nc_session_server_ch_client_dispatch_stop_if_dispatched(old_ch_client->name, NC_RWLOCK_WRITE))) { + if ((rc = nc_session_server_ch_client_dispatch_stop(old_ch_client))) { ERR(NULL, "Failed to dispatch stop for Call Home client \"%s\".", old_ch_client->name); goto rollback; } @@ -5448,15 +5427,12 @@ nc_server_config_reconcile_chclients_dispatch(struct nc_server_config *old_cfg, * to return to the pre-call state. */ LY_ARRAY_FOR(started_clients, i) { - nc_session_server_ch_client_dispatch_stop_if_dispatched(started_clients[i], NC_RWLOCK_WRITE); + nc_session_server_ch_client_dispatch_stop(started_clients[i]); } /* rc is already set to non-zero from the failure point */ cleanup: - /* free the tracking list and its contents */ - LY_ARRAY_FOR(started_clients, i) { - free(started_clients[i]); - } + /* free the tracking list */ LY_ARRAY_FREE(started_clients); return rc; } @@ -6073,6 +6049,8 @@ nc_server_config_dup(const struct nc_server_config *src, struct nc_server_config dst_ch_client->max_attempts = src_ch_client->max_attempts; dst_ch_client->max_wait = src_ch_client->max_wait; + dst_ch_client->thread = src_ch_client->thread; + LY_ARRAY_INCREMENT(dst->ch_clients); } diff --git a/src/session_p.h b/src/session_p.h index 60c34ef6..72511d39 100644 --- a/src/session_p.h +++ b/src/session_p.h @@ -753,6 +753,8 @@ struct nc_server_config { NC_CH_START_WITH start_with; /**< How to select the Call Home endpoint to connect to. */ uint8_t max_attempts; /**< Maximum number of attempts to connect to the given Call Home endpoint. */ uint16_t max_wait; /**< Maximum time to wait for a Call Home connection in seconds. */ + + struct nc_server_ch_thread_arg *thread; /**< Call Home client thread data, if dispatched. */ } *ch_clients; /**< Call Home clients (sized-array, see libyang docs). */ #ifdef NC_ENABLED_SSH_TLS @@ -784,11 +786,6 @@ struct nc_server_opts { void *content_id_data; /**< Data passed to the content_id_clb callback. */ void (*content_id_data_free)(void *data); /**< Callback to free the content_id_data. */ - /* ACCESS locked - call home thread creation/deletion - WRITE lock - * - call home threads data access (e.g. to signal thread to end) - READ lock */ - pthread_rwlock_t ch_threads_lock; /**< Lock for data of Call Home threads. */ - struct nc_server_ch_thread_arg **ch_threads; /**< Call Home threads' data, one for each CH client (sized-array, see libyang docs). */ - /* ACCESS locked - options modified by YANG data/API - WRITE lock * - options read when accepting sessions - READ lock */ pthread_rwlock_t config_lock; /**< Lock for the server configuration. */ @@ -1389,20 +1386,19 @@ NC_MSG_TYPE nc_connect_callhome(const char *host, uint16_t port, NC_TRANSPORT_IM #ifdef NC_ENABLED_SSH_TLS /** - * @brief Stop a dispatched Call Home client thread, if such thread was dispatched for the given client name. + * @brief Stop a dispatched Call Home client thread, if such thread was dispatched for the given client. + * + * @warning The caller MUST hold both WRITE config lock and CONFIG APPLY mutex when calling this function. * - * @param[in] client_name Name of the Call Home client to stop the thread for. - * @param[in] config_lock_mode Lock mode of the configuration lock, if it is held by the caller. - * If NC_RWLOCK_WRITE is passed, config lock will be briefly unlocked and then locked again. - * The caller MUST ensure that it holds the CONFIG APPLY mutex, so that nobody steals the wrlock from him. - * @return 0 if the thread was successfully stopped or not found, 1 on error. + * @param[in] ch_client Call Home client to stop the thread for, can be NULL. + * @return 0 if the thread was successfully stopped, 1 on error. */ -int nc_session_server_ch_client_dispatch_stop_if_dispatched(const char *client_name, enum nc_rwlock_mode config_lock_mode); +int nc_session_server_ch_client_dispatch_stop(struct nc_ch_client *ch_client); /** * @brief Dispatch a thread connecting to a listening NETCONF client and creating Call Home sessions. * - * @note The config lock MUST be held. + * @note The config WRITE lock MUST be held. * * @param[in] ch_client Call Home client to dispatch the thread for. * @param[in] acquire_ctx_cb Callback for acquiring new session context. @@ -1412,7 +1408,7 @@ int nc_session_server_ch_client_dispatch_stop_if_dispatched(const char *client_n * @param[in] new_session_cb_data Arbitrary user data passed to @p new_session_cb. * @return 0 if the thread was successfully created, -1 on error. */ -int _nc_connect_ch_client_dispatch(const struct nc_ch_client *ch_client, nc_server_ch_session_acquire_ctx_cb acquire_ctx_cb, +int _nc_connect_ch_client_dispatch(struct nc_ch_client *ch_client, nc_server_ch_session_acquire_ctx_cb acquire_ctx_cb, nc_server_ch_session_release_ctx_cb release_ctx_cb, void *ctx_cb_data, nc_server_ch_new_session_cb new_session_cb, void *new_session_cb_data); diff --git a/src/session_server.c b/src/session_server.c index 50b5770c..7153c3a4 100644 --- a/src/session_server.c +++ b/src/session_server.c @@ -1272,11 +1272,6 @@ nc_server_init(void) goto error; } - /* CH threads data rwlock */ - if (nc_server_init_rwlock(&server_opts.ch_threads_lock)) { - goto error; - } - if ((r = pthread_mutex_init(&server_opts.cert_exp_notif.lock, NULL))) { ERR(NULL, "%s: failed to init certificate expiration notification thread lock(%s).", __func__, strerror(r)); goto error; @@ -1322,23 +1317,25 @@ nc_server_destroy(void) } #endif /* NC_ENABLED_SSH_TLS */ - /* CONFIG UPDATE LOCK, continue on error */ - if (nc_mutex_lock(&server_opts.config_update_lock, NC_CONFIG_LOCK_TIMEOUT, __func__) == 1) { - config_update_locked = 1; + /* CONFIG UPDATE LOCK */ + if (nc_mutex_lock(&server_opts.config_update_lock, NC_CONFIG_LOCK_TIMEOUT, __func__) != 1) { + rc = 1; + goto cleanup; } + config_update_locked = 1; - /* CONFIG WR LOCK, continue on error */ - if (nc_rwlock_lock(&server_opts.config_lock, NC_RWLOCK_WRITE, NC_CONFIG_LOCK_TIMEOUT, __func__) == 1) { - config_lock_mode = NC_RWLOCK_WRITE; + /* CONFIG WR LOCK */ + if (nc_rwlock_lock(&server_opts.config_lock, NC_RWLOCK_WRITE, NC_CONFIG_LOCK_TIMEOUT, __func__) != 1) { + rc = 1; + goto cleanup; } + config_lock_mode = NC_RWLOCK_WRITE; #ifdef NC_ENABLED_SSH_TLS - if (!config_update_locked || (config_lock_mode != NC_RWLOCK_WRITE)) { - WRN(NULL, "Config locks not acquired, skipping Call Home threads cleanup."); - } else { - /* stop all dispatched CH threads */ - LY_ARRAY_FOR(server_opts.config.ch_clients, i) { - nc_session_server_ch_client_dispatch_stop_if_dispatched(server_opts.config.ch_clients[i].name, config_lock_mode); + /* stop all dispatched CH threads */ + LY_ARRAY_FOR(server_opts.config.ch_clients, i) { + if ((rc = nc_session_server_ch_client_dispatch_stop(&server_opts.config.ch_clients[i]))) { + goto cleanup; } } #endif /* NC_ENABLED_SSH_TLS */ @@ -1366,16 +1363,6 @@ nc_server_destroy(void) LY_ARRAY_FREE(server_opts.unix_paths); server_opts.unix_paths = NULL; - /* CONFIG WR UNLOCK */ - if (config_lock_mode != NC_RWLOCK_NONE) { - nc_rwlock_unlock(&server_opts.config_lock, __func__); - } - - /* CONFIG UPDATE UNLOCK */ - if (config_update_locked) { - nc_mutex_unlock(&server_opts.config_update_lock, __func__); - } - #ifdef NC_ENABLED_SSH_TLS curl_global_cleanup(); nc_tls_backend_destroy_wrap(); @@ -1388,6 +1375,12 @@ nc_server_destroy(void) #endif /* NC_ENABLED_SSH_TLS */ cleanup: + if (config_lock_mode != NC_RWLOCK_NONE) { + nc_rwlock_unlock(&server_opts.config_lock, __func__); + } + if (config_update_locked) { + nc_mutex_unlock(&server_opts.config_update_lock, __func__); + } return rc; } @@ -3546,7 +3539,6 @@ nc_ch_client_thread(void *arg) struct nc_session *session = NULL; struct nc_ch_client *client; uint32_t reconnect_in; - LY_ARRAY_COUNT_TYPE i; /* mark the thread as running */ if (nc_mutex_lock(&data->cond_lock, NC_CH_COND_LOCK_TIMEOUT, __func__) != 1) { @@ -3743,26 +3735,15 @@ nc_ch_client_thread(void *arg) VRB(session, "Call Home client \"%s\" thread exit.", data->client_name); free(cur_endpt_name); - /* CH THREADS WR LOCK */ - if (nc_rwlock_lock(&server_opts.ch_threads_lock, NC_RWLOCK_WRITE, NC_CH_THREADS_LOCK_TIMEOUT, __func__) != 1) { - goto unlock_skip; - } - - /* remove the thread data from the global array */ - LY_ARRAY_FOR(server_opts.ch_threads, i) { - if (server_opts.ch_threads[i] == data) { - break; + /* find the client and clear thread pointer */ + if (nc_rwlock_lock(&server_opts.config_lock, NC_RWLOCK_WRITE, NC_CONFIG_LOCK_TIMEOUT, __func__) == 1) { + client = nc_server_ch_client_get(data->client_name); + if (client && (client->thread == data)) { + client->thread = NULL; } + nc_rwlock_unlock(&server_opts.config_lock, __func__); } - if (i < LY_ARRAY_COUNT(server_opts.ch_threads) - 1) { - server_opts.ch_threads[i] = server_opts.ch_threads[LY_ARRAY_COUNT(server_opts.ch_threads) - 1]; - } - LY_ARRAY_DECREMENT_FREE(server_opts.ch_threads); - /* CH THREADS UNLOCK */ - nc_rwlock_unlock(&server_opts.ch_threads_lock, __func__); - -unlock_skip: free(data->client_name); pthread_mutex_destroy(&data->cond_lock); pthread_cond_destroy(&data->cond); @@ -3771,84 +3752,66 @@ nc_ch_client_thread(void *arg) } int -nc_session_server_ch_client_dispatch_stop_if_dispatched(const char *client_name, enum nc_rwlock_mode config_lock_mode) +nc_session_server_ch_client_dispatch_stop(struct nc_ch_client *ch_client) { int rc = 0, r; - struct nc_server_ch_thread_arg **ch_thread_arg; + struct nc_server_ch_thread_arg *thread_arg; pthread_t tid; - enum nc_rwlock_mode ch_threads_lock = NC_RWLOCK_NONE; - - /* CH THREADS LOCK */ - if (nc_rwlock_lock(&server_opts.ch_threads_lock, NC_RWLOCK_READ, NC_CH_THREADS_LOCK_TIMEOUT, __func__) != 1) { - return 1; - } - ch_threads_lock = NC_RWLOCK_READ; + enum nc_rwlock_mode config_lock_mode = NC_RWLOCK_WRITE; - if (config_lock_mode == NC_RWLOCK_WRITE) { - /* CONFIG UNLOCK - if the caller holds the config lock in write mode, we need to unlock it - * to prevent deadlock with the CH thread, it tries to acquire the config lock in read mode when it - * checks if the client still exists. - * It is the caller's responsibility to hold config apply mutex, so noone steals the write lock from him */ - nc_rwlock_unlock(&server_opts.config_lock, __func__); + if (!ch_client || !ch_client->thread) { + return 0; } - LY_ARRAY_FOR(server_opts.ch_threads, struct nc_server_ch_thread_arg *, ch_thread_arg) { - if (strcmp(client_name, (*ch_thread_arg)->client_name)) { - continue; - } + thread_arg = ch_client->thread; - /* CH COND LOCK */ - if (nc_mutex_lock(&(*ch_thread_arg)->cond_lock, NC_CH_COND_LOCK_TIMEOUT, __func__) != 1) { - rc = 1; - goto cleanup; - } + /* CH COND LOCK */ + if (nc_mutex_lock(&thread_arg->cond_lock, NC_CH_COND_LOCK_TIMEOUT, __func__) != 1) { + rc = 1; + goto cleanup; + } - /* notify the thread to stop */ - (*ch_thread_arg)->thread_running = 0; - tid = (*ch_thread_arg)->tid; - pthread_cond_signal(&(*ch_thread_arg)->cond); + /* notify the thread to stop */ + thread_arg->thread_running = 0; + tid = thread_arg->tid; + pthread_cond_signal(&thread_arg->cond); - /* CH COND UNLOCK */ - nc_mutex_unlock(&(*ch_thread_arg)->cond_lock, __func__); + /* CH COND UNLOCK */ + nc_mutex_unlock(&thread_arg->cond_lock, __func__); - /* CH THREADS UNLOCK - let the thread free itself from the global array, it needs WR lock for that */ - nc_rwlock_unlock(&server_opts.ch_threads_lock, __func__); - ch_threads_lock = NC_RWLOCK_NONE; + /* CONFIG UNLOCK - the caller must hold WRITE config lock, we need to unlock it + * to prevent deadlock with the CH thread, it tries to acquire the config lock in read mode when it + * checks if the client still exists. + * It is the caller's responsibility to hold config apply mutex as well, so noone steals the write lock from him */ + nc_rwlock_unlock(&server_opts.config_lock, __func__); + config_lock_mode = NC_RWLOCK_NONE; - /* wait for the thread to end */ - r = pthread_join(tid, NULL); - if (r) { - ERR(NULL, "Joining Call Home client \"%s\" thread failed (%s).", client_name, strerror(r)); - rc = 1; - goto cleanup; - } - break; + /* wait for the thread to end */ + r = pthread_join(tid, NULL); + if (r) { + ERR(NULL, "Joining Call Home client \"%s\" thread failed (%s).", ch_client->name, strerror(r)); + rc = 1; + goto cleanup; } cleanup: - if (config_lock_mode == NC_RWLOCK_WRITE) { + if (config_lock_mode == NC_RWLOCK_NONE) { /* CONFIG LOCK - lock it back if we unlocked it. It MUST succeed, if the caller holds the config apply mutex */ if (nc_rwlock_lock(&server_opts.config_lock, NC_RWLOCK_WRITE, -1, __func__) != 1) { ERRINT; } } - if (ch_threads_lock) { - /* CH THREADS UNLOCK */ - nc_rwlock_unlock(&server_opts.ch_threads_lock, __func__); - } - return rc; } int -_nc_connect_ch_client_dispatch(const struct nc_ch_client *ch_client, nc_server_ch_session_acquire_ctx_cb acquire_ctx_cb, +_nc_connect_ch_client_dispatch(struct nc_ch_client *ch_client, nc_server_ch_session_acquire_ctx_cb acquire_ctx_cb, nc_server_ch_session_release_ctx_cb release_ctx_cb, void *ctx_cb_data, nc_server_ch_new_session_cb new_session_cb, void *new_session_cb_data) { int rc = 0, r; - enum nc_rwlock_mode ch_threads_lock = NC_RWLOCK_NONE; - struct nc_server_ch_thread_arg *arg = NULL, **new_item; + struct nc_server_ch_thread_arg *arg = NULL; /* create the thread argument */ arg = calloc(1, sizeof *arg); @@ -3865,24 +3828,13 @@ _nc_connect_ch_client_dispatch(const struct nc_ch_client *ch_client, nc_server_c pthread_cond_init(&arg->cond, NULL); pthread_mutex_init(&arg->cond_lock, NULL); - /* CH THREADS WR LOCK */ - if (nc_rwlock_lock(&server_opts.ch_threads_lock, NC_RWLOCK_WRITE, NC_CH_THREADS_LOCK_TIMEOUT, __func__) != 1) { - rc = -1; - goto cleanup; - } - ch_threads_lock = NC_RWLOCK_WRITE; - - /* Append the thread data pointer to the global array. - * Pointer instead of struct so that server_opts.ch_thread_lock does not have to be - * locked everytime the arg is accessed */ - LY_ARRAY_NEW_GOTO(NULL, server_opts.ch_threads, new_item, rc, cleanup); - *new_item = arg; + /* store thread data in the client */ + ch_client->thread = arg; /* create the CH thread */ if ((r = pthread_create(&arg->tid, NULL, nc_ch_client_thread, arg))) { ERR(NULL, "Creating a new thread failed (%s).", strerror(r)); - /* remove from the array */ - LY_ARRAY_DECREMENT_FREE(server_opts.ch_threads); + ch_client->thread = NULL; rc = -1; goto cleanup; } @@ -3891,10 +3843,6 @@ _nc_connect_ch_client_dispatch(const struct nc_ch_client *ch_client, nc_server_c arg = NULL; cleanup: - if (ch_threads_lock) { - /* CH THREADS UNLOCK */ - nc_rwlock_unlock(&server_opts.ch_threads_lock, __func__); - } if (arg) { free(arg->client_name); free(arg); @@ -3915,7 +3863,7 @@ nc_connect_ch_client_dispatch(const char *client_name, nc_server_ch_session_acqu NC_CHECK_SRV_INIT_RET(-1); /* CONFIG READ LOCK */ - if (nc_rwlock_lock(&server_opts.config_lock, NC_RWLOCK_READ, NC_CONFIG_LOCK_TIMEOUT, __func__) != 1) { + if (nc_rwlock_lock(&server_opts.config_lock, NC_RWLOCK_WRITE, NC_CONFIG_LOCK_TIMEOUT, __func__) != 1) { return -1; } @@ -3923,6 +3871,7 @@ nc_connect_ch_client_dispatch(const char *client_name, nc_server_ch_session_acqu ch_client = nc_server_ch_client_get(client_name); NC_CHECK_ERR_GOTO(!ch_client, rc = -1; ERR(NULL, "Call Home client \"%s\" not found.", client_name), cleanup); + /* requires config wr lock */ rc = _nc_connect_ch_client_dispatch(ch_client, acquire_ctx_cb, release_ctx_cb, ctx_cb_data, new_session_cb, new_session_cb_data); From 7a3b975c8509d1fc851419c6f837ba2fc77b94bc Mon Sep 17 00:00:00 2001 From: Roman Janota Date: Tue, 14 Apr 2026 13:27:55 +0200 Subject: [PATCH 6/9] session server ch BUGFIX check ch thread running When ch sess is established, only the status of the sess was checked in a loop, which isn't enough when we just delete a CH client from the config without the client gracefully closing first. --- src/session_server.c | 82 +++++++++++++++++++++++++------------------- 1 file changed, 46 insertions(+), 36 deletions(-) diff --git a/src/session_server.c b/src/session_server.c index 7153c3a4..769d9065 100644 --- a/src/session_server.c +++ b/src/session_server.c @@ -3267,7 +3267,7 @@ nc_connect_ch_endpt(struct nc_ch_endpt *endpt, nc_server_ch_session_acquire_ctx_ * * @param[in] client_name Name of the Call Home client. * @param[out] idle_timeout Idle timeout in seconds. - * @return 0 on success, 1 if the client was not found. + * @return 0 on success, 1 if the client was not found, -1 on error. */ static int nc_server_ch_client_get_idle_timeout(const char *client_name, uint32_t *idle_timeout) @@ -3277,7 +3277,7 @@ nc_server_ch_client_get_idle_timeout(const char *client_name, uint32_t *idle_tim /* CONFIG READ LOCK */ if (nc_rwlock_lock(&server_opts.config_lock, NC_RWLOCK_READ, NC_CONFIG_LOCK_TIMEOUT, __func__) != 1) { - return 1; + return -1; } client = nc_server_ch_client_get(client_name); @@ -3298,6 +3298,35 @@ nc_server_ch_client_get_idle_timeout(const char *client_name, uint32_t *idle_tim return ret; } +/** + * @brief Checks if a Call Home thread should terminate. + * + * Checks the shared boolean variable thread_running. This should be done everytime + * before entering a critical section. + * + * @param[in] data Call Home thread's data. + * + * @return 0 if the thread should stop running, -1 if it can continue. + */ +static int +nc_server_ch_client_thread_is_running(struct nc_server_ch_thread_arg *data) +{ + int ret = -1; + + /* COND LOCK */ + if (nc_mutex_lock(&data->cond_lock, NC_CH_COND_LOCK_TIMEOUT, __func__) != 1) { + return ret; + } + if (!data->thread_running) { + /* thread should stop running */ + ret = 0; + } + /* COND UNLOCK */ + nc_mutex_unlock(&data->cond_lock, __func__); + + return ret; +} + /** * @brief Wait for any event after a NC session was established on a CH client. * @@ -3369,10 +3398,12 @@ nc_server_ch_client_thread_session_cond_wait(struct nc_server_ch_thread_arg *dat /* check if the client still exists and get its idle timeout */ r = nc_server_ch_client_get_idle_timeout(data->client_name, &idle_timeout); if (r) { - /* client was removed, finish thread */ - VRB(session, "Call Home client \"%s\" removed, but an established session will not be terminated.", - data->client_name); - rc = 1; + if (r == 1) { + /* the client must always be found, because if we delete it, then the configuring thread calls + * pthread_join() on this thread with the old config where the client still exists */ + ERRINT; + } + rc = -1; /* CH LOCK - to remain consistent */ if (nc_mutex_lock(&session->opts.server.ch_lock, NC_SESSION_CH_LOCK_TIMEOUT, __func__) != 1) { @@ -3392,9 +3423,15 @@ nc_server_ch_client_thread_session_cond_wait(struct nc_server_ch_thread_arg *dat session->status = NC_STATUS_INVALID; session->term_reason = NC_SESSION_TERM_TIMEOUT; } - } while (session->status == NC_STATUS_RUNNING); + } while ((session->status == NC_STATUS_RUNNING) && nc_server_ch_client_thread_is_running(data)); /* broke out of the loop, but still holding the ch_lock */ + if (session->status == NC_STATUS_RUNNING) { + /* thread is terminating but the session is still running, so just log it */ + VRB(session, "Call Home client \"%s\" removed, but an established session will not be terminated.", + data->client_name); + } + /* signal to nc_session_free() that CH thread is terminating */ session->flags &= ~NC_SESSION_CH_THREAD; pthread_cond_signal(&session->opts.server.ch_cond); @@ -3450,35 +3487,6 @@ nc_server_ch_client_thread_is_running_wait(struct nc_session *session, struct nc return ret; } -/** - * @brief Checks if a Call Home thread should terminate. - * - * Checks the shared boolean variable thread_running. This should be done everytime - * before entering a critical section. - * - * @param[in] data Call Home thread's data. - * - * @return 0 if the thread should stop running, -1 if it can continue. - */ -static int -nc_server_ch_client_thread_is_running(struct nc_server_ch_thread_arg *data) -{ - int ret = -1; - - /* COND LOCK */ - if (nc_mutex_lock(&data->cond_lock, NC_CH_COND_LOCK_TIMEOUT, __func__) != 1) { - return ret; - } - if (!data->thread_running) { - /* thread should stop running */ - ret = 0; - } - /* COND UNLOCK */ - nc_mutex_unlock(&data->cond_lock, __func__); - - return ret; -} - /** * @brief Wait for a Call Home client to have at least one endpoint defined. * @@ -3845,6 +3853,8 @@ _nc_connect_ch_client_dispatch(struct nc_ch_client *ch_client, nc_server_ch_sess cleanup: if (arg) { free(arg->client_name); + pthread_mutex_destroy(&arg->cond_lock); + pthread_cond_destroy(&arg->cond); free(arg); } return rc; From 890d8df5efbfb84f029334a53264851a4d24a7b9 Mon Sep 17 00:00:00 2001 From: Roman Janota Date: Tue, 14 Apr 2026 15:26:26 +0200 Subject: [PATCH 7/9] test_ch UPDATE add ch thread del while sess --- tests/test_ch.c | 185 ++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 181 insertions(+), 4 deletions(-) diff --git a/tests/test_ch.c b/tests/test_ch.c index 451051f1..02df70a9 100644 --- a/tests/test_ch.c +++ b/tests/test_ch.c @@ -29,13 +29,14 @@ struct test_ch_data { struct lyd_node *tree; + pthread_barrier_t *barrier2; }; char buffer[512]; char expected[512]; -int TEST_PORT = 10050, TEST_PORT_2 = 10051; -const char *TEST_PORT_STR = "10050", *TEST_PORT_2_STR = "10051"; +int TEST_PORT = 10050, TEST_PORT_2 = 10051, TEST_PORT_3 = 10052, TEST_PORT_4 = 10053; +const char *TEST_PORT_STR = "10050", *TEST_PORT_2_STR = "10051", *TEST_PORT_3_STR = "10052", *TEST_PORT_4_STR = "10053"; static void test_msg_callback(const struct nc_session *session, NC_VERB_LEVEL level, const char *msg) @@ -169,6 +170,10 @@ test_nc_ch_free_test_data(void *test_data) test_ch_data = test_data; lyd_free_tree(test_ch_data->tree); + if (test_ch_data->barrier2) { + pthread_barrier_destroy(test_ch_data->barrier2); + free(test_ch_data->barrier2); + } free(test_ch_data); } @@ -382,16 +387,188 @@ setup_tls(void **state) return 0; } +/* + * Test: Delete CH client while session is established + * Verifies that deleting a CH client from config properly signals the thread to stop, + * even when the session is in RUNNING state. + */ +static int session_established_flag = 0; + +static int +ch_new_session_set_flag_cb(const char *client_name, struct nc_session *new_session, void *user_data) +{ + int ret = 0; + struct nc_pollsession *ps = (struct nc_pollsession *)user_data; + + (void) client_name; + + ret = nc_ps_add_session(ps, new_session); + assert_int_equal(ret, 0); + + session_established_flag = 1; + return 0; +} + +static void * +server_thread_delete_while_session(void *arg) +{ + int ret; + struct nc_pollsession *ps; + struct ln2_test_ctx *test_ctx = arg; + struct test_ch_data *test_data = test_ctx->test_data; + + nc_set_print_clb_session(test_msg_callback); + buffer[0] = '\0'; + strcpy(expected, "thread exit"); + + ps = nc_ps_new(); + assert_non_null(ps); + + pthread_barrier_wait(&test_ctx->barrier); + + ret = nc_connect_ch_client_dispatch("ch_delete_test", ch_session_acquire_ctx_cb, + ch_session_release_ctx_cb, test_ctx, ch_new_session_set_flag_cb, ps); + assert_int_equal(ret, 0); + + while (!session_established_flag) { + usleep(10000); + } + + do { + ret = nc_ps_poll(ps, NC_PS_POLL_TIMEOUT, NULL); + if (ret & (NC_PSPOLL_TIMEOUT | NC_PSPOLL_NOSESSIONS)) { + usleep(500); + } + } while (ret & NC_PSPOLL_RPC); + + ret = nc_server_config_setup_data(test_data->tree); + assert_int_equal(ret, 0); + + do { + usleep(5000); + } while (!strlen(buffer)); + + assert_true(strlen(buffer) > 0); + + pthread_barrier_wait(test_data->barrier2); + + nc_ps_clear(ps, 1, NULL); + nc_ps_free(ps); + + return NULL; +} + +static void * +client_thread_delete_while_session(void *arg) +{ + int ret; + struct nc_session *session = NULL; + struct ln2_test_ctx *test_ctx = arg; + struct test_ch_data *test_data = test_ctx->test_data; + + nc_client_ssh_ch_set_knownhosts_mode(NC_SSH_KNOWNHOSTS_SKIP); + ret = nc_client_set_schema_searchpath(MODULES_DIR); + assert_int_equal(ret, 0); + ret = nc_client_ssh_ch_set_username("test_ch_delete"); + assert_int_equal(ret, 0); + ret = nc_client_ssh_ch_add_keypair(TESTS_DIR "/data/id_ed25519.pub", TESTS_DIR "/data/id_ed25519"); + assert_int_equal(ret, 0); + ret = nc_client_ssh_ch_add_bind_listen("127.0.0.1", TEST_PORT_3); + assert_int_equal(ret, 0); + + pthread_barrier_wait(&test_ctx->barrier); + + ret = nc_accept_callhome(NC_ACCEPT_TIMEOUT, NULL, &session); + assert_int_equal(ret, 1); + + /* wait until the server deletes the client */ + pthread_barrier_wait(test_data->barrier2); + + ret = nc_client_ssh_ch_del_bind("127.0.0.1", TEST_PORT_3); + assert_int_equal(ret, 0); + + nc_session_free(session, NULL); + return NULL; +} + +static int +setup_delete_while_session(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_data->barrier2 = calloc(1, sizeof(pthread_barrier_t)); + assert_non_null(test_data->barrier2); + pthread_barrier_init(test_data->barrier2, NULL, 2); + + test_ctx->test_data = test_data; + test_ctx->free_test_data = test_nc_ch_free_test_data; + *state = test_ctx; + + session_established_flag = 0; + + ret = nc_server_config_add_ch_address_port(test_ctx->ctx, "ch_delete_test", "endpt", NC_TI_SSH, + "127.0.0.1", TEST_PORT_3_STR, &tree); + assert_int_equal(ret, 0); + + ret = nc_server_config_add_ch_persistent(test_ctx->ctx, "ch_delete_test", &tree); + assert_int_equal(ret, 0); + + ret = nc_server_config_add_ch_ssh_hostkey(test_ctx->ctx, "ch_delete_test", "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_delete_test", "endpt", "test_ch_delete", + "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); + + ret = nc_server_config_del_ch_client("ch_delete_test", &tree); + assert_int_equal(ret, 0); + + test_data->tree = tree; + return 0; +} + +static void +test_nc_ch_delete_client_while_session(void **state) +{ + int ret, i; + pthread_t tids[2]; + + assert_non_null(state); + + ret = pthread_create(&tids[0], NULL, client_thread_delete_while_session, *state); + assert_int_equal(ret, 0); + + ret = pthread_create(&tids[1], NULL, server_thread_delete_while_session, *state); + assert_int_equal(ret, 0); + + for (i = 0; i < 2; i++) { + pthread_join(tids[i], NULL); + } +} + int main(void) { const struct CMUnitTest tests[] = { cmocka_unit_test_setup_teardown(test_nc_ch_ssh, setup_ssh, ln2_glob_test_teardown), 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), }; - /* try to get ports from the environment, otherwise use the default */ - if (ln2_glob_test_get_ports(2, &TEST_PORT, &TEST_PORT_STR, &TEST_PORT_2, &TEST_PORT_2_STR)) { + if (ln2_glob_test_get_ports(4, &TEST_PORT, &TEST_PORT_STR, &TEST_PORT_2, &TEST_PORT_2_STR, + &TEST_PORT_3, &TEST_PORT_3_STR, &TEST_PORT_4, &TEST_PORT_4_STR)) { return 1; } From 23946b737c776a27a64edb02a54fa351368d5326 Mon Sep 17 00:00:00 2001 From: Roman Janota Date: Mon, 13 Apr 2026 15:44:52 +0200 Subject: [PATCH 8/9] SOVERSION bump to version 5.3.7 --- CMakeLists.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index d73a9c46..8fe3d41b 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -66,7 +66,7 @@ set(LIBNETCONF2_VERSION ${LIBNETCONF2_MAJOR_VERSION}.${LIBNETCONF2_MINOR_VERSION # with backward compatible change and micro version is connected with any internal change of the library. set(LIBNETCONF2_MAJOR_SOVERSION 5) set(LIBNETCONF2_MINOR_SOVERSION 3) -set(LIBNETCONF2_MICRO_SOVERSION 6) +set(LIBNETCONF2_MICRO_SOVERSION 7) set(LIBNETCONF2_SOVERSION_FULL ${LIBNETCONF2_MAJOR_SOVERSION}.${LIBNETCONF2_MINOR_SOVERSION}.${LIBNETCONF2_MICRO_SOVERSION}) set(LIBNETCONF2_SOVERSION ${LIBNETCONF2_MAJOR_SOVERSION}) From ab85528b3f4afa3b5ee30baaf8c12fdcd856a31e Mon Sep 17 00:00:00 2001 From: Roman Janota Date: Mon, 13 Apr 2026 15:44:58 +0200 Subject: [PATCH 9/9] VERSION bump to version 4.3.1 --- CMakeLists.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 8fe3d41b..14781983 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -58,7 +58,7 @@ set(CMAKE_MACOSX_RPATH TRUE) # micro version is changed with a set of small changes or bugfixes anywhere in the project. set(LIBNETCONF2_MAJOR_VERSION 4) set(LIBNETCONF2_MINOR_VERSION 3) -set(LIBNETCONF2_MICRO_VERSION 0) +set(LIBNETCONF2_MICRO_VERSION 1) set(LIBNETCONF2_VERSION ${LIBNETCONF2_MAJOR_VERSION}.${LIBNETCONF2_MINOR_VERSION}.${LIBNETCONF2_MICRO_VERSION}) # Version of the library