session server UPDATE CH thread lifecycle management

Roytak · Roytak · commit 3796262f91ac · 2026-04-14T09:20:16.000+02:00
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
diff --git 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,55 +5354,63 @@ 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;
     }
 
     /*
      * == PHASE 1: START NEW CLIENTS ==
      * 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
@@ -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,15 +1012,17 @@ 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 */
     status = session->status;
 
     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: <close-session> + 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
@@ -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
