From 0bb1fb6abbdcaea6f3dab41f5e90e34c4ad25f4b Mon Sep 17 00:00:00 2001 From: Roman Janota Date: Mon, 24 Aug 2026 20:56:09 +0200 Subject: [PATCH 01/10] session server REFACTOR move listening sockets to a registry A listening socket file descriptor is runtime state, not configuration, but it used to live in struct nc_bind inside struct nc_server_config. The socket reconcile therefore had to write into the currently published configuration to stop nc_server_config_free() from closing a descriptor that the new generation reuses. Move the descriptors into a registry in server_opts, keyed by the endpoint name plus the resolved address and port, guarded by a new binds_lock. struct nc_bind keeps address and port, which are actual configuration. The reconcile now only opens sockets for binds with no registry entry and closes entries the new configuration no longer contains, so nothing is written to either configuration generation and the PHASE 1 / PHASE 2 / rollback socket bookkeeping is gone. A removed endpoint stops listening immediately. nc_server_accept_binds() copies the registry into local poll arrays, releases binds_lock and only then polls, so the lock is never held across the poll. The accepted connection is mapped back to an endpoint by name. nc_server_config_free() no longer closes sockets or unlinks UNIX socket files; both moved to the registry removal path, which also lets nc_server_unix_get_socket_path() become static. The client side Call Home binds keep their own descriptors, which moved to the parallel ch_binds_aux array. --- src/server_config.c | 210 +---------------- src/session_client.c | 12 +- src/session_p.h | 54 +++-- src/session_server.c | 549 ++++++++++++++++++++++++++++++++++--------- 4 files changed, 494 insertions(+), 331 deletions(-) diff --git a/src/server_config.c b/src/server_config.c index 33b0f488..89cc3b3d 100644 --- a/src/server_config.c +++ b/src/server_config.c @@ -513,7 +513,6 @@ nc_server_config_free(struct nc_server_config *config) struct nc_ch_client *ch_client; struct nc_ch_endpt *ch_endpt; LY_ARRAY_COUNT_TYPE i = 0, j = 0; - char *socket_path = NULL; if (!config) { return; @@ -529,31 +528,14 @@ nc_server_config_free(struct nc_server_config *config) LY_ARRAY_FOR(config->endpts, i) { endpt = &config->endpts[i]; - if (endpt->ti == NC_TI_UNIX) { - /* get the socket path before freeing the name */ - socket_path = nc_server_unix_get_socket_path(endpt); - } - free(endpt->name); - /* free binds */ + /* free binds, the listening sockets are owned by the bind registry */ LY_ARRAY_FOR(endpt->binds, j) { - if (endpt->binds[j].sock != -1) { - close(endpt->binds[j].sock); - if (socket_path) { - /* remove the UNIX socket file */ - unlink(socket_path); - } - } free(endpt->binds[j].address); } LY_ARRAY_FREE(endpt->binds); - if (endpt->ti == NC_TI_UNIX) { - free(socket_path); - socket_path = NULL; - } - /* free transport specific options */ switch (endpt->ti) { #ifdef NC_ENABLED_SSH_TLS @@ -782,8 +764,6 @@ config_local_bind(const struct lyd_node *node, enum nc_operation parent_op, stru } else if (op == NC_OP_CREATE) { /* create a new bind */ LY_ARRAY_NEW_RET(LYD_CTX(node), endpt->binds, bind, 1); - /* init the new bind */ - bind->sock = -1; } else { ERR(NULL, "Unsupported operation of node \"%s\".", LYD_NAME(node)); return 1; @@ -3141,7 +3121,7 @@ config_unix_socket_path(const struct lyd_node *node, enum nc_operation parent_op if (op == NC_OP_DELETE) { /* the endpoint must have a single binding, so we can just free it, - * the socket will be closed in ::nc_server_config_free() */ + * the listening socket is closed by the bind registry */ if (!endpt->binds) { ERR(NULL, "No UNIX socket path binding to delete."); return 1; @@ -3160,7 +3140,6 @@ config_unix_socket_path(const struct lyd_node *node, enum nc_operation parent_op LY_ARRAY_NEW_RET(LYD_CTX(node), endpt->binds, bind, 1); bind->address = strdup(lyd_get_value(node)); NC_CHECK_ERRMEM_RET(!bind->address, 1); - bind->sock = -1; /* also set the cleartext path flag */ opts->path_type = NC_UNIX_SOCKET_PATH_FILE; @@ -3182,7 +3161,7 @@ config_unix_hidden_path(const struct lyd_node *node, enum nc_operation parent_op if (op == NC_OP_DELETE) { /* the endpoint must have a single binding, so we can just free it, - * the socket will be closed in ::nc_server_config_free() */ + * the listening socket is closed by the bind registry */ if (!endpt->binds) { ERR(NULL, "No UNIX socket hidden path binding to delete."); return 1; @@ -3199,7 +3178,6 @@ config_unix_hidden_path(const struct lyd_node *node, enum nc_operation parent_op return 1; } LY_ARRAY_NEW_RET(LYD_CTX(node), endpt->binds, bind, 1); - bind->sock = -1; /* also set the hidden path flag */ opts->path_type = NC_UNIX_SOCKET_PATH_HIDDEN; @@ -5396,181 +5374,6 @@ nc_server_config_libnetconf2_netconf_server(const struct lyd_node *tree, int is_ return rc; } -/** - * @brief Check if two server endpoint bindings match. - * - * They match if they use the same transport protocol, address and port. - * - * @param[in] e1 First server endpoint. - * @param[in] b1 First server endpoint binding. - * @param[in] e2 Second server endpoint. - * @param[in] b2 Second server endpoint binding. - * @return 1 if they match, 0 otherwise. - */ -static int -nc_server_config_bindings_match(const struct nc_endpt *e1, const struct nc_bind *b1, - const struct nc_endpt *e2, const struct nc_bind *b2) -{ - int rc = 1; - char *addr1 = NULL, *addr2 = NULL; - - if (e1->ti != e2->ti) { - /* different transport protocols */ - return 0; - } - - if (e1->ti == NC_TI_UNIX) { - /* UNIX sockets may have hidden or cleartext addresses */ - addr1 = nc_server_unix_get_socket_path(e1); - addr2 = nc_server_unix_get_socket_path(e2); - } else { - addr1 = b1->address; - addr2 = b2->address; - } - if (!addr1 || !addr2) { - /* unable to get the address */ - rc = 0; - goto cleanup; - } - - if (strcmp(addr1, addr2) || (b1->port != b2->port)) { - /* different addresses or ports */ - rc = 0; - goto cleanup; - } - -cleanup: - if (e1->ti == NC_TI_UNIX) { - free(addr1); - free(addr2); - } - return rc; -} - -/** - * @brief Atomically starts listening on new sockets and reuses existing ones. - * - * @param[in,out] old_cfg Old, currently active server configuration. - * @param[in,out] new_cfg New server configuration currently being applied. - * @return 0 on success, 1 on error. - */ -static int -nc_server_config_reconcile_sockets_listen(struct nc_server_config *old_cfg, - struct nc_server_config *new_cfg) -{ - int rc = 0, found; - struct nc_endpt *old_endpt, *new_endpt; - struct nc_bind *new_bind, *old_bind; - - /* - * == PHASE 1: RECONCILE OLD AND NEW SOCKETS == - * Match existing sockets from old_cfg to new_cfg to reuse them, - * then create new sockets for new binds. - */ - - /* reuse existing sockets from old_cfg */ - LY_ARRAY_FOR(new_cfg->endpts, struct nc_endpt, new_endpt) { - LY_ARRAY_FOR(new_endpt->binds, struct nc_bind, new_bind) { - found = 0; - LY_ARRAY_FOR(old_cfg->endpts, struct nc_endpt, old_endpt) { - LY_ARRAY_FOR(old_endpt->binds, struct nc_bind, old_bind) { - if (nc_server_config_bindings_match(new_endpt, new_bind, old_endpt, old_bind)) { - /* match found, reuse the socket */ - new_bind->sock = old_bind->sock; - found = 1; - break; - } - } - if (found) { - /* break the outer loop as well, we already found a match for this bind */ - break; - } - } - } - } - - /* create new sockets for new binds */ - LY_ARRAY_FOR(new_cfg->endpts, struct nc_endpt, new_endpt) { - LY_ARRAY_FOR(new_endpt->binds, struct nc_bind, new_bind) { - if (new_bind->sock == -1) { - /* this bind is new, create a listening socket */ - if (nc_server_bind_and_listen(new_endpt, new_bind)) { - /* FAILURE! trigger rollback */ - rc = 1; - goto rollback; - } - } - } - } - - /* - * == PHASE 2: COMMIT CHANGES (WRITE TO old_cfg) == - * new_cfg is now fully valid. We can safely modify old_cfg to prevent - * reused sockets from being closed by the caller. - */ - LY_ARRAY_FOR(old_cfg->endpts, struct nc_endpt, old_endpt) { - LY_ARRAY_FOR(old_endpt->binds, struct nc_bind, old_bind) { - found = 0; - if (old_bind->sock == -1) { - /* already handled or was never active */ - continue; - } - - /* check if this old_bind's socket was reused in the new_cfg */ - LY_ARRAY_FOR(new_cfg->endpts, struct nc_endpt, new_endpt) { - LY_ARRAY_FOR(new_endpt->binds, struct nc_bind, new_bind) { - if (old_bind->sock == new_bind->sock) { - /* match found, invalidate the socket in the old config (dont want to close it) */ - old_bind->sock = -1; - found = 1; - break; - } - } - if (found) { - /* break the outer loop as well, we already found a match for this bind */ - break; - } - } - } - } - - return 0; - -rollback: - /* - * == ROLLBACK LOGIC == - * An error occurred. We do not want to close the reused sockets, so we can roll back to old_cfg. - * So we invalidate all reused sockets in new_cfg, the rest will be closed by the caller later. - */ - LY_ARRAY_FOR(new_cfg->endpts, struct nc_endpt, new_endpt) { - LY_ARRAY_FOR(new_endpt->binds, struct nc_bind, new_bind) { - found = 0; - if (new_bind->sock == -1) { - /* this bind was never assigned a socket */ - continue; - } - - /* was this socket reused from the old config? */ - LY_ARRAY_FOR(old_cfg->endpts, struct nc_endpt, old_endpt) { - LY_ARRAY_FOR(old_endpt->binds, struct nc_bind, old_bind) { - if (new_bind->sock == old_bind->sock) { - /* match found, invalidate the socket in the new config */ - new_bind->sock = -1; - found = 1; - break; - } - } - if (found) { - /* break the outer loop as well, we already found a match for this bind */ - break; - } - } - } - } - - return rc; -} - #ifdef NC_ENABLED_SSH_TLS /** @@ -6232,9 +6035,6 @@ nc_server_config_dup(const struct nc_server_config *src, struct nc_server_config NC_CHECK_ERRMEM_GOTO(!dst_endpt->binds[j].address, rc = 1, cleanup); } dst_endpt->binds[j].port = src_endpt->binds[j].port; - - /* mark the socket as uninitialized, it will be reassigned in ::nc_server_config_reconcile_sockets_listen() */ - dst_endpt->binds[j].sock = -1; LY_ARRAY_INCREMENT(dst_endpt->binds); } @@ -6434,7 +6234,7 @@ nc_server_config_setup_diff(const struct lyd_node *data) } /* start listening on new endpoints */ - NC_CHECK_ERR_GOTO(ret = nc_server_config_reconcile_sockets_listen(&server_opts.config, &config_copy), + NC_CHECK_ERR_GOTO(ret = nc_server_binds_reconcile(&config_copy), ERR(NULL, "Starting to listen on new endpoints failed."), cleanup_unlock); #ifdef NC_ENABLED_SSH_TLS @@ -6530,7 +6330,7 @@ nc_server_config_setup_data(const struct lyd_node *data) } /* start listening on new endpoints */ - NC_CHECK_ERR_GOTO(ret = nc_server_config_reconcile_sockets_listen(&server_opts.config, &config), + NC_CHECK_ERR_GOTO(ret = nc_server_binds_reconcile(&config), ERR(NULL, "Starting to listen on new endpoints failed."), cleanup_unlock); #ifdef NC_ENABLED_SSH_TLS diff --git a/src/session_client.c b/src/session_client.c index 45619917..1b0586c8 100644 --- a/src/session_client.c +++ b/src/session_client.c @@ -80,7 +80,7 @@ nc_client_context_free(void *ptr) #ifdef NC_ENABLED_SSH_TLS for (i = 0; i < c->opts.ch_bind_count; ++i) { - close(c->opts.ch_binds[i].sock); + close(c->opts.ch_binds_aux[i].sock); free((char *)c->opts.ch_binds[i].address); } free(c->opts.ch_binds); @@ -1783,10 +1783,10 @@ nc_client_ch_add_bind_listen(const char *address, uint16_t port, const char *hos } client_opts.ch_binds_aux[client_opts.ch_bind_count - 1].ti = ti; client_opts.ch_binds_aux[client_opts.ch_bind_count - 1].hostname = hostname ? strdup(hostname) : NULL; + client_opts.ch_binds_aux[client_opts.ch_bind_count - 1].sock = sock; client_opts.ch_binds[client_opts.ch_bind_count - 1].address = strdup(address); client_opts.ch_binds[client_opts.ch_bind_count - 1].port = port; - client_opts.ch_binds[client_opts.ch_bind_count - 1].sock = sock; return 0; } @@ -1799,7 +1799,7 @@ nc_client_ch_del_bind(const char *address, uint16_t port, NC_TRANSPORT_IMPL ti) if (!address && !port && !ti) { for (i = 0; i < client_opts.ch_bind_count; ++i) { - close(client_opts.ch_binds[i].sock); + close(client_opts.ch_binds_aux[i].sock); free(client_opts.ch_binds[i].address); free(client_opts.ch_binds_aux[i].hostname); @@ -1818,7 +1818,7 @@ nc_client_ch_del_bind(const char *address, uint16_t port, NC_TRANSPORT_IMPL ti) if ((!address || !strcmp(client_opts.ch_binds[i].address, address)) && (!port || (client_opts.ch_binds[i].port == port)) && (!ti || (client_opts.ch_binds_aux[i].ti == ti))) { - close(client_opts.ch_binds[i].sock); + close(client_opts.ch_binds_aux[i].sock); free(client_opts.ch_binds[i].address); --client_opts.ch_bind_count; @@ -1858,8 +1858,8 @@ nc_accept_callhome(int timeout, struct ly_ctx *ctx, struct nc_session **session) return -1; } - ret = nc_server_ch_accept_binds(client_opts.ch_binds, client_opts.ch_bind_count, timeout, - &host, &port, &bind_idx, &sock); + ret = nc_server_ch_accept_binds(client_opts.ch_binds, client_opts.ch_binds_aux, client_opts.ch_bind_count, + timeout, &host, &port, &bind_idx, &sock); if (ret < 1) { free(host); return ret; diff --git a/src/session_p.h b/src/session_p.h index 446e2be0..0d33f172 100644 --- a/src/session_p.h +++ b/src/session_p.h @@ -140,6 +140,12 @@ extern struct nc_server_opts server_opts; */ #define NC_CERT_EXP_LOCK_TIMEOUT 1000 +/** + * @brief Timeout in msec for acquiring the binds_lock + * (only listening socket registry array manipulation) + */ +#define NC_BINDS_LOCK_TIMEOUT 1000 + /** * @brief Timeout in msec for acquiring the config_lock * (socket binding and Call Home client dispatching can involve network operations) @@ -508,7 +514,6 @@ struct nc_server_unix_opts { struct nc_bind { char *address; /**< Either IPv4/IPv6 address or path to UNIX socket. */ uint16_t port; /**< Either port number or 0 for UNIX socket. */ - int sock; /**< Socket file descriptor, -1 if not created yet. */ }; struct nc_client_unix_opts { @@ -593,9 +598,10 @@ struct nc_client_opts { struct nc_bind *ch_binds; - struct { + struct nc_client_ch_bind_aux { NC_TRANSPORT_IMPL ti; char *hostname; + int sock; /**< Listening socket file descriptor of the corresponding bind. */ } *ch_binds_aux; uint16_t ch_bind_count; @@ -794,6 +800,23 @@ struct nc_server_opts { pthread_rwlock_t config_lock; /**< Lock for the server configuration. */ struct nc_server_config config; /**< YANG Server configuration. */ + /* ACCESS locked - binds lock - leaf lock, never acquire another lock while holding it */ + pthread_mutex_t binds_lock; /**< Lock for the listening socket registry. */ + + /** + * @brief Entry of the listening socket registry. + * + * A listening socket is runtime state, not configuration, so it is kept out of + * ::nc_server_config, which must not be written to while it is being read by an accept path. + */ + struct nc_bind_entry { + char *endpt_name; /**< Name of the endpoint the listening socket belongs to. */ + char *address; /**< IPv4/IPv6 address or the full path of a UNIX socket. */ + uint16_t port; /**< Port number, 0 for a UNIX socket. */ + NC_TRANSPORT_IMPL ti; /**< Transport implementation of the endpoint. */ + int sock; /**< Listening socket file descriptor. */ + } *binds; /**< Listening socket registry (sized-array, see libyang docs). */ + #ifdef NC_ENABLED_SSH_TLS char *authkey_path_fmt; /**< Path to users' public keys that may contain tokens with special meaning. */ char *pam_config_name; /**< PAM configuration file name. */ @@ -1087,21 +1110,23 @@ struct nc_client_context *nc_client_context_location(void); void *nc_realloc(void *ptr, size_t size); /** - * @brief Get the UNIX socket path for the given endpoint. + * @brief Reconcile the listening socket registry with the given server configuration. + * + * Starts listening for every bind of @p config that has no registry entry yet and stops listening + * for every registry entry that @p config no longer contains. Nothing is written to @p config. * - * @param[in] endpt Endpoint to get the socket path for. - * @return Socket path, NULL on error. + * @note Only one thread may reconcile the registry at a time, the callers must be serialized by + * ::nc_server_opts.config_update_lock. + * + * @param[in] config Server configuration to reconcile the registry with. + * @return 0 on success, 1 on error (the registry is left as it was). */ -char *nc_server_unix_get_socket_path(const struct nc_endpt *endpt); +int nc_server_binds_reconcile(const struct nc_server_config *config); /** - * @brief Bind and listen on a socket for the given endpoint and its bind. - * - * @param[in] endpt Endpoint the bind belongs to. - * @param[in] bind Bind to bind and listen for. - * @return 0 on success, 1 on error. + * @brief Stop listening on all the registered sockets and free the listening socket registry. */ -int nc_server_bind_and_listen(struct nc_endpt *endpt, struct nc_bind *bind); +void nc_server_binds_destroy(void); /** * @brief Free server configuration data (only YANG config data). @@ -1327,6 +1352,7 @@ int nc_sock_listen_inet(const char *address, uint16_t port); * @brief Accept a new connection on any of the given Call Home binds. * * @param[in] binds Call Home binds to accept on. + * @param[in] binds_aux Auxiliary data of @p binds holding the listening sockets. * @param[in] bind_count Number of @p binds. * @param[in] timeout Timeout for accepting. * @param[out] host Host of the remote peer. Can be NULL. @@ -1335,8 +1361,8 @@ int nc_sock_listen_inet(const char *address, uint16_t port); * @param[out] sock Accepted socket, if any. * @return -1 on error, 0 on timeout, 1 if a socket was accepted. */ -int nc_server_ch_accept_binds(struct nc_bind *binds, uint16_t bind_count, int timeout, char **host, - uint16_t *port, uint16_t *bind_idx, int *sock); +int nc_server_ch_accept_binds(const struct nc_bind *binds, const struct nc_client_ch_bind_aux *binds_aux, + uint16_t bind_count, int timeout, char **host, uint16_t *port, uint16_t *bind_idx, int *sock); /** * @brief Establish a UNIX transport session. diff --git a/src/session_server.c b/src/session_server.c index 374f7c44..28844279 100644 --- a/src/session_server.c +++ b/src/session_server.c @@ -57,6 +57,7 @@ struct nc_server_opts server_opts = { .hello_lock = PTHREAD_RWLOCK_INITIALIZER, .config_lock = PTHREAD_RWLOCK_INITIALIZER, .config_update_lock = PTHREAD_MUTEX_INITIALIZER, + .binds_lock = PTHREAD_MUTEX_INITIALIZER, }; static nc_rpc_clb global_rpc_clb = NULL; @@ -510,7 +511,13 @@ nc_session_unix_construct_socket_path(const char *filename, char **path) return rc; } -char * +/** + * @brief Get the full path of the UNIX socket of an endpoint. + * + * @param[in] endpt Endpoint to get the socket path for. + * @return Socket path, NULL on error. + */ +static char * nc_server_unix_get_socket_path(const struct nc_endpt *endpt) { LY_ARRAY_COUNT_TYPE i; @@ -748,32 +755,26 @@ nc_sock_host_get(const struct sockaddr_storage *saddr, int client_sock, char **c * @brief Log the accepted connection. * * @param[in] saddr sockaddr_storage. - * @param[in] endpt Endpoint on which the connection was accepted (optional, used for logging). - * @param[in] bind Bind on which the connection was accepted. + * @param[in] address Address of the bind the connection was accepted on, the socket path for AF_UNIX. + * @param[in] port Port of the bind the connection was accepted on. * @param[in] client_address Hostname or IP address of the connecting client. * @param[in] client_port Port number of the connecting client, if any. * @return 0 on success, -1 on error. */ static int -nc_sock_log_accepted(const struct sockaddr_storage *saddr, const struct nc_endpt *endpt, const struct nc_bind *bind, +nc_sock_log_accepted(const struct sockaddr_storage *saddr, const char *address, uint16_t port, const char *client_address, uint16_t client_port) { - char *unix_sockpath = NULL; - if (saddr->ss_family == AF_UNIX) { - /* UNIX socket, get the socket path for logging, - * UNIX socket connection can NOT be over call home (caller = client connect), so endpt is always available */ - assert(endpt); - unix_sockpath = nc_server_unix_get_socket_path(endpt); - VRB(NULL, "Accepted a new connection on %s.", unix_sockpath ? unix_sockpath : "UNIX socket"); - free(unix_sockpath); + /* UNIX socket, the address is the full socket path */ + VRB(NULL, "Accepted a new connection on %s.", address); } else if (saddr->ss_family == AF_INET) { /* IPv4 socket */ - VRB(NULL, "Accepted a new connection on %s:%" PRIu16 " from %s:%" PRIu16 ".", bind->address, bind->port, + VRB(NULL, "Accepted a new connection on %s:%" PRIu16 " from %s:%" PRIu16 ".", address, port, client_address, client_port); } else if (saddr->ss_family == AF_INET6) { /* IPv6 socket */ - VRB(NULL, "Accepted a new connection on [%s]:%" PRIu16 " from [%s]:%" PRIu16 ".", bind->address, bind->port, + VRB(NULL, "Accepted a new connection on [%s]:%" PRIu16 " from [%s]:%" PRIu16 ".", address, port, client_address, client_port); } else { ERR(NULL, "Source host of an unknown protocol family."); @@ -836,8 +837,8 @@ nc_sock_accept_first(struct pollfd *pfd, uint16_t pfd_count, int *client_sock, * * @param[in] pollfds FDs to poll for new connections. * @param[in] pollfd_count Number of FDs in the pollfds array. - * @param[in] endpt_map Map of pollfd indices to endpoints (optional, used for logging). - * @param[in] bind_map Map of pollfd indices to binds (optional, used for logging). + * @param[in] addr_map Map of pollfd indices to bind addresses (used for logging). + * @param[in] port_map Map of pollfd indices to bind ports (used for logging). * @param[in] timeout Timeout for accepting a connection. * @param[out] host Hostname or IP address of the connecting client. * @param[out] port Port number of the connecting client, if any. @@ -846,16 +847,14 @@ nc_sock_accept_first(struct pollfd *pfd, uint16_t pfd_count, int *client_sock, * @return 1 on success, 0 on timeout, -1 on error. */ static int -nc_sock_accept_pollfds(struct pollfd *pollfds, uint16_t pollfd_count, struct nc_endpt **endpt_map, - struct nc_bind **bind_map, int timeout, char **host, uint16_t *port, +nc_sock_accept_pollfds(struct pollfd *pollfds, uint16_t pollfd_count, const char **addr_map, + const uint16_t *port_map, int timeout, char **host, uint16_t *port, uint16_t *fd_idx, int *sock) { uint16_t client_port = 0, matched_pollfd_idx = 0; char *client_address = NULL; struct sockaddr_storage client_saddr; socklen_t saddr_len = sizeof(client_saddr); - struct nc_endpt *endpt; - struct nc_bind *bind; int client_sock = -1, ret = 1, r, flags; if (!pollfd_count) { @@ -883,9 +882,6 @@ nc_sock_accept_pollfds(struct pollfd *pollfds, uint16_t pollfd_count, struct nc_ goto cleanup; } - bind = bind_map[matched_pollfd_idx]; - endpt = endpt_map ? endpt_map[matched_pollfd_idx] : NULL; - /* 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)); @@ -899,7 +895,8 @@ nc_sock_accept_pollfds(struct pollfd *pollfds, uint16_t pollfd_count, struct nc_ } /* log the new accepted connection */ - if ((r = nc_sock_log_accepted(&client_saddr, endpt, bind, client_address, client_port))) { + if ((r = nc_sock_log_accepted(&client_saddr, addr_map[matched_pollfd_idx], port_map[matched_pollfd_idx], + client_address, client_port))) { ret = r; goto cleanup; } @@ -926,9 +923,15 @@ nc_sock_accept_pollfds(struct pollfd *pollfds, uint16_t pollfd_count, struct nc_ } /** - * @brief Accept a new connection on any of the server's listening binds. + * @brief Accept a new connection on any of the registered listening sockets. * - * @param[in] config Server configuration. + * The listening socket registry is only read to build the local poll arrays, the ::poll() itself + * and the ::accept() run with no lock held. + * + * @note A connection accepted on a bind that @p config does not contain is dropped. That can only + * happen for a bind registered after @p config was read, in which case the next call accepts it. + * + * @param[in] config Pinned server configuration used to look the accepting endpoint up. * @param[in] timeout Timeout for accepting a connection. * @param[out] host Hostname or IP address of the connecting client. * @param[out] port Port number of the connecting client, if any. @@ -937,112 +940,156 @@ nc_sock_accept_pollfds(struct pollfd *pollfds, uint16_t pollfd_count, struct nc_ * @return 1 on success, 0 on timeout, -1 on error. */ static int -nc_server_accept_binds(struct nc_server_config *config, int timeout, char **host, +nc_server_accept_binds(const struct nc_server_config *config, int timeout, char **host, uint16_t *port, LY_ARRAY_COUNT_TYPE *idx, int *sock) { struct pollfd *pollfds = NULL; - uint16_t pollfd_count = 0, fd_idx = 0, bind_count = 0; - LY_ARRAY_COUNT_TYPE i; - struct nc_endpt *endpt; - struct nc_bind *bind; - int ret = 1; - struct nc_endpt **endpt_map = NULL; - struct nc_bind **bind_map = NULL; - - /* count the number of valid binds and prepare the pollfd and map parallel arrays */ - LY_ARRAY_FOR(config->endpts, i) { - bind_count += LY_ARRAY_COUNT(config->endpts[i].binds); + uint16_t pollfd_count = 0, fd_idx = 0, i, bind_count = 0; + LY_ARRAY_COUNT_TYPE u; + int ret = 1, binds_locked = 0; + char **addr_map = NULL, **name_map = NULL; + uint16_t *port_map = NULL; + + /* BINDS LOCK */ + if (nc_mutex_lock(&server_opts.binds_lock, NC_BINDS_LOCK_TIMEOUT, __func__) != 1) { + return -1; } + binds_locked = 1; + + bind_count = LY_ARRAY_COUNT(server_opts.binds); if (!bind_count) { /* no binds to accept on, treat as a timeout */ ret = 0; goto cleanup; } + /* copy the registry into local arrays, so that the lock can be released before polling */ pollfds = malloc(bind_count * sizeof *pollfds); - NC_CHECK_ERRMEM_RET(!pollfds, -1); - endpt_map = malloc(bind_count * sizeof *endpt_map); - NC_CHECK_ERRMEM_GOTO(!endpt_map, ret = -1, cleanup); - bind_map = malloc(bind_count * sizeof *bind_map); - NC_CHECK_ERRMEM_GOTO(!bind_map, ret = -1, cleanup); + NC_CHECK_ERRMEM_GOTO(!pollfds, ret = -1, cleanup); + addr_map = calloc(bind_count, sizeof *addr_map); + NC_CHECK_ERRMEM_GOTO(!addr_map, ret = -1, cleanup); + port_map = malloc(bind_count * sizeof *port_map); + NC_CHECK_ERRMEM_GOTO(!port_map, ret = -1, cleanup); + name_map = calloc(bind_count, sizeof *name_map); + NC_CHECK_ERRMEM_GOTO(!name_map, ret = -1, cleanup); - /* fill the arrays */ - LY_ARRAY_FOR(config->endpts, struct nc_endpt, endpt) { - LY_ARRAY_FOR(endpt->binds, struct nc_bind, bind) { - if (bind->sock < 0) { - /* invalid socket */ - continue; - } - - pollfds[pollfd_count].fd = bind->sock; - pollfds[pollfd_count].events = POLLIN; - pollfds[pollfd_count].revents = 0; + for (i = 0; i < bind_count; ++i) { + pollfds[pollfd_count].fd = server_opts.binds[i].sock; + pollfds[pollfd_count].events = POLLIN; + pollfds[pollfd_count].revents = 0; - endpt_map[pollfd_count] = endpt; - bind_map[pollfd_count] = bind; + /* the registry entries may be freed once the lock is released, so copy the strings */ + addr_map[pollfd_count] = strdup(server_opts.binds[i].address); + NC_CHECK_ERRMEM_GOTO(!addr_map[pollfd_count], ret = -1, cleanup); + name_map[pollfd_count] = strdup(server_opts.binds[i].endpt_name); + NC_CHECK_ERRMEM_GOTO(!name_map[pollfd_count], ret = -1, cleanup); + port_map[pollfd_count] = server_opts.binds[i].port; - ++pollfd_count; - } + ++pollfd_count; } + /* BINDS UNLOCK */ + nc_mutex_unlock(&server_opts.binds_lock, __func__); + binds_locked = 0; + /* accept a new connection on any of the sockets */ - ret = nc_sock_accept_pollfds(pollfds, pollfd_count, endpt_map, bind_map, timeout, host, port, &fd_idx, sock); - if (idx && (ret > 0)) { - *idx = endpt_map[fd_idx] - config->endpts; + ret = nc_sock_accept_pollfds(pollfds, pollfd_count, (const char **)addr_map, port_map, timeout, host, port, + &fd_idx, sock); + if (ret > 0) { + /* map the endpoint name back to an endpoint of the pinned configuration */ + LY_ARRAY_FOR(config->endpts, u) { + if (!strcmp(config->endpts[u].name, name_map[fd_idx])) { + break; + } + } + if (u == LY_ARRAY_COUNT(config->endpts)) { + /* the endpoint is not in the configuration we are working with, drop the connection */ + VRB(NULL, "Endpoint \"%s\" not found, dropping the accepted connection.", name_map[fd_idx]); + close(*sock); + *sock = -1; + if (host) { + free(*host); + *host = NULL; + } + ret = 0; + } else if (idx) { + *idx = u; + } } cleanup: + if (binds_locked) { + /* BINDS UNLOCK */ + nc_mutex_unlock(&server_opts.binds_lock, __func__); + } + for (i = 0; i < bind_count; ++i) { + if (addr_map) { + free(addr_map[i]); + } + if (name_map) { + free(name_map[i]); + } + } free(pollfds); - free(endpt_map); - free(bind_map); + free(addr_map); + free(port_map); + free(name_map); return ret; } int -nc_server_ch_accept_binds(struct nc_bind *binds, uint16_t bind_count, int timeout, char **host, - uint16_t *port, uint16_t *bind_idx, int *sock) +nc_server_ch_accept_binds(const struct nc_bind *binds, const struct nc_client_ch_bind_aux *binds_aux, + uint16_t bind_count, int timeout, char **host, uint16_t *port, uint16_t *bind_idx, int *sock) { struct pollfd *pollfds = NULL; uint16_t pollfd_count = 0, fd_idx = 0, i; int ret = 1; - struct nc_bind **bind_map = NULL; + const char **addr_map = NULL; + uint16_t *port_map = NULL, *idx_map = NULL; if (!bind_count) { /* no binds to accept on, treat as a timeout */ - ret = 0; - goto cleanup; + return 0; } /* prepare the pollfd and map parallel arrays */ pollfds = malloc(bind_count * sizeof *pollfds); NC_CHECK_ERRMEM_RET(!pollfds, -1); - bind_map = malloc(bind_count * sizeof *bind_map); - NC_CHECK_ERRMEM_GOTO(!bind_map, ret = -1, cleanup); + addr_map = malloc(bind_count * sizeof *addr_map); + NC_CHECK_ERRMEM_GOTO(!addr_map, ret = -1, cleanup); + port_map = malloc(bind_count * sizeof *port_map); + NC_CHECK_ERRMEM_GOTO(!port_map, ret = -1, cleanup); + idx_map = malloc(bind_count * sizeof *idx_map); + NC_CHECK_ERRMEM_GOTO(!idx_map, ret = -1, cleanup); /* fill the arrays */ for (i = 0; i < bind_count; ++i) { - if (binds[i].sock < 0) { + if (binds_aux[i].sock < 0) { /* invalid socket */ continue; } - pollfds[pollfd_count].fd = binds[i].sock; + pollfds[pollfd_count].fd = binds_aux[i].sock; pollfds[pollfd_count].events = POLLIN; pollfds[pollfd_count].revents = 0; - bind_map[pollfd_count] = &binds[i]; + addr_map[pollfd_count] = binds[i].address; + port_map[pollfd_count] = binds[i].port; + idx_map[pollfd_count] = i; ++pollfd_count; } - ret = nc_sock_accept_pollfds(pollfds, pollfd_count, NULL, bind_map, timeout, host, port, &fd_idx, sock); + ret = nc_sock_accept_pollfds(pollfds, pollfd_count, addr_map, port_map, timeout, host, port, &fd_idx, sock); if (bind_idx && (ret > 0)) { - *bind_idx = bind_map[fd_idx] - binds; + *bind_idx = idx_map[fd_idx]; } cleanup: free(pollfds); - free(bind_map); + free(addr_map); + free(port_map); + free(idx_map); return ret; } @@ -1365,6 +1412,9 @@ nc_server_destroy(void) } #endif /* NC_ENABLED_SSH_TLS */ + /* stop listening on all the registered sockets */ + nc_server_binds_destroy(); + /* destroy the server configuration */ nc_server_config_free(&server_opts.config); @@ -2766,54 +2816,341 @@ nc_ps_clear(struct nc_pollsession *ps, int all, void (*data_free)(void *)) nc_ps_unlock(ps, q_id, __func__); } -int -nc_server_bind_and_listen(struct nc_endpt *endpt, struct nc_bind *bind) +/** + * @brief Description of a listening socket required by a server configuration. + */ +struct nc_bind_desc { + const struct nc_endpt *endpt; /**< Endpoint the listening socket belongs to. */ + char *address; /**< Resolved address, the full socket path for a UNIX endpoint. */ + uint16_t port; /**< Port number, 0 for a UNIX socket. */ + int reused; /**< Whether an already registered socket is being reused. */ + int sock; /**< Newly opened listening socket, -1 if none was opened. */ +}; + +/** + * @brief Start listening on a socket of an endpoint bind. + * + * @param[in] endpt Endpoint the bind belongs to. + * @param[in] address Address to listen on, the full socket path for a UNIX endpoint. + * @param[in] port Port to listen on, 0 for a UNIX endpoint. + * @param[out] sock Created listening socket. + * @return 0 on success, 1 on error. + */ +static int +nc_server_bind_and_listen(const struct nc_endpt *endpt, const char *address, uint16_t port, int *sock) { - char *unix_path = NULL; - int sock = -1, rc = 0; - - /* start listening on the endpoint */ - if (endpt->ti == NC_TI_UNIX) { - /* get the socket path for this endpoint */ - unix_path = nc_server_unix_get_socket_path(endpt); - NC_CHECK_ERR_GOTO(!unix_path, rc = 1, cleanup); - sock = nc_sock_listen_unix(unix_path, endpt->opts.unix); - } else { - assert(bind->address && bind->port); - sock = nc_sock_listen_inet(bind->address, bind->port); - } - if (sock == -1) { - rc = 1; - goto cleanup; - } +#ifndef NC_ENABLED_SSH_TLS + /* only UNIX endpoints exist, which have no port */ + (void)port; +#endif - /* close the old socket if any and store the new one */ - if (bind->sock > -1) { - close(bind->sock); - } - bind->sock = sock; + *sock = -1; switch (endpt->ti) { case NC_TI_UNIX: - VRB(NULL, "Listening on %s for UNIX connections.", unix_path); + *sock = nc_sock_listen_unix(address, endpt->opts.unix); + NC_CHECK_RET(*sock == -1, 1); + VRB(NULL, "Listening on %s for UNIX connections.", address); break; #ifdef NC_ENABLED_SSH_TLS case NC_TI_SSH: - VRB(NULL, "Listening on %s:%u for SSH connections.", bind->address, bind->port); + *sock = nc_sock_listen_inet(address, port); + NC_CHECK_RET(*sock == -1, 1); + VRB(NULL, "Listening on %s:%" PRIu16 " for SSH connections.", address, port); break; case NC_TI_TLS: - VRB(NULL, "Listening on %s:%u for TLS connections.", bind->address, bind->port); + *sock = nc_sock_listen_inet(address, port); + NC_CHECK_RET(*sock == -1, 1); + VRB(NULL, "Listening on %s:%" PRIu16 " for TLS connections.", address, port); break; #endif /* NC_ENABLED_SSH_TLS */ default: ERRINT; + return 1; + } + + return 0; +} + +/** + * @brief Stop listening on a socket that was opened for a bind description. + * + * @param[in,out] desc Bind description to close the socket of, no-op if it has none. + */ +static void +nc_server_bind_desc_close(struct nc_bind_desc *desc) +{ + if (desc->sock == -1) { + return; + } + + close(desc->sock); + desc->sock = -1; + if (desc->endpt->ti == NC_TI_UNIX) { + /* remove the socket file we have just created */ + unlink(desc->address); + } +} + +/** + * @brief Stop listening on a registered socket and free the registry entry members. + * + * @note The bind registry lock must be held. + * + * @param[in] entry Bind registry entry to close. + */ +static void +nc_server_bind_entry_close(struct nc_bind_entry *entry) +{ + close(entry->sock); + if (entry->ti == NC_TI_UNIX) { + /* remove the socket file */ + unlink(entry->address); + VRB(NULL, "Stopped listening on %s.", entry->address); + } else { + VRB(NULL, "Stopped listening on %s:%" PRIu16 ".", entry->address, entry->port); + } + + free(entry->endpt_name); + free(entry->address); +} + +/** + * @brief Check whether a bind registry entry refers to the same listening socket as a bind description. + * + * @param[in] entry Bind registry entry. + * @param[in] desc Bind description. + * @return 1 if they match, 0 otherwise. + */ +static int +nc_server_bind_entry_matches(const struct nc_bind_entry *entry, const struct nc_bind_desc *desc) +{ + return (entry->ti == desc->endpt->ti) && (entry->port == desc->port) && !strcmp(entry->address, desc->address); +} + +/** + * @brief Collect the listening sockets required by a server configuration. + * + * @param[in] config Server configuration. + * @param[out] descs Bind descriptions (sized-array, see libyang docs). + * @return 0 on success, 1 on error. + */ +static int +nc_server_bind_descs_get(const struct nc_server_config *config, struct nc_bind_desc **descs) +{ + int rc = 0; + const struct nc_endpt *endpt; + const struct nc_bind *bind; + struct nc_bind_desc *desc; + LY_ARRAY_COUNT_TYPE u, v; + uint32_t count = 0; + + *descs = NULL; + + LY_ARRAY_FOR(config->endpts, u) { + count += LY_ARRAY_COUNT(config->endpts[u].binds); + } + if (!count) { + return 0; + } + LY_ARRAY_CREATE_GOTO(NULL, *descs, count, rc, cleanup); + + LY_ARRAY_FOR(config->endpts, u) { + endpt = &config->endpts[u]; + + LY_ARRAY_FOR(endpt->binds, v) { + bind = &endpt->binds[v]; + + desc = &(*descs)[LY_ARRAY_COUNT(*descs)]; + desc->endpt = endpt; + desc->port = bind->port; + desc->sock = -1; + + if (endpt->ti == NC_TI_UNIX) { + /* the socket path is not stored in the bind, resolve it */ + desc->address = nc_server_unix_get_socket_path(endpt); + NC_CHECK_ERR_GOTO(!desc->address, rc = 1, cleanup); + } else { + assert(bind->address && bind->port); + desc->address = strdup(bind->address); + NC_CHECK_ERRMEM_GOTO(!desc->address, rc = 1, cleanup); + } + + LY_ARRAY_INCREMENT(*descs); + } + } + +cleanup: + return rc ? 1 : 0; +} + +/** + * @brief Free bind descriptions and close all the sockets they still own. + * + * @param[in] descs Bind descriptions to free. + */ +static void +nc_server_bind_descs_free(struct nc_bind_desc *descs) +{ + LY_ARRAY_COUNT_TYPE u; + + LY_ARRAY_FOR(descs, u) { + nc_server_bind_desc_close(&descs[u]); + free(descs[u].address); + } + LY_ARRAY_FREE(descs); +} + +int +nc_server_binds_reconcile(const struct nc_server_config *config) +{ + int rc = 0, binds_locked = 0, found; + struct nc_bind_desc *descs = NULL; + struct nc_bind_entry *entry; + char *endpt_name, *address; + LY_ARRAY_COUNT_TYPE u, v, added = 0; + uint32_t new_count = 0; + + /* collect all the listening sockets the configuration requires, no lock is needed for that */ + NC_CHECK_GOTO(rc = nc_server_bind_descs_get(config, &descs), cleanup); + + /* BINDS LOCK */ + if (nc_mutex_lock(&server_opts.binds_lock, NC_BINDS_LOCK_TIMEOUT, __func__) != 1) { rc = 1; - break; + goto cleanup; + } + binds_locked = 1; + + /* keep listening on the sockets that are already registered */ + LY_ARRAY_FOR(descs, u) { + LY_ARRAY_FOR(server_opts.binds, v) { + if (!nc_server_bind_entry_matches(&server_opts.binds[v], &descs[u])) { + continue; + } + + /* the socket stays open, but the endpoint owning it may have been renamed */ + if (strcmp(server_opts.binds[v].endpt_name, descs[u].endpt->name)) { + endpt_name = strdup(descs[u].endpt->name); + NC_CHECK_ERRMEM_GOTO(!endpt_name, rc = 1, cleanup); + free(server_opts.binds[v].endpt_name); + server_opts.binds[v].endpt_name = endpt_name; + } + + descs[u].reused = 1; + break; + } + + if (!descs[u].reused) { + ++new_count; + } + } + + /* BINDS UNLOCK - creating the sockets may take a while */ + nc_mutex_unlock(&server_opts.binds_lock, __func__); + binds_locked = 0; + + /* start listening on the sockets that are not registered yet */ + LY_ARRAY_FOR(descs, u) { + if (descs[u].reused) { + continue; + } + + NC_CHECK_GOTO(rc = nc_server_bind_and_listen(descs[u].endpt, descs[u].address, descs[u].port, + &descs[u].sock), cleanup); + } + + /* BINDS LOCK */ + if (nc_mutex_lock(&server_opts.binds_lock, NC_BINDS_LOCK_TIMEOUT, __func__) != 1) { + rc = 1; + goto cleanup; + } + binds_locked = 1; + + /* register the new sockets, reserve the space in advance */ + if (new_count) { + LY_ARRAY_CREATE_GOTO(NULL, server_opts.binds, new_count, rc, cleanup); + } + LY_ARRAY_FOR(descs, u) { + if (descs[u].reused) { + continue; + } + + endpt_name = strdup(descs[u].endpt->name); + NC_CHECK_ERRMEM_GOTO(!endpt_name, rc = 1, cleanup); + address = strdup(descs[u].address); + NC_CHECK_ERRMEM_GOTO(!address, free(endpt_name); rc = 1, cleanup); + + entry = &server_opts.binds[LY_ARRAY_COUNT(server_opts.binds)]; + entry->endpt_name = endpt_name; + entry->address = address; + entry->port = descs[u].port; + entry->ti = descs[u].endpt->ti; + entry->sock = descs[u].sock; + + /* the socket now belongs to the registry */ + descs[u].sock = -1; + LY_ARRAY_INCREMENT(server_opts.binds); + ++added; + } + + /* stop listening on the sockets the configuration no longer contains */ + v = 0; + while (v < LY_ARRAY_COUNT(server_opts.binds)) { + found = 0; + LY_ARRAY_FOR(descs, u) { + if (nc_server_bind_entry_matches(&server_opts.binds[v], &descs[u])) { + found = 1; + break; + } + } + if (found) { + ++v; + continue; + } + + nc_server_bind_entry_close(&server_opts.binds[v]); + + /* swap the last entry into the hole, the order of the registry is irrelevant */ + server_opts.binds[v] = server_opts.binds[LY_ARRAY_COUNT(server_opts.binds) - 1]; + LY_ARRAY_DECREMENT_FREE(server_opts.binds); } cleanup: - free(unix_path); - return rc; + if (rc) { + /* unregister the sockets we have just registered, they are always the last ones */ + while (added) { + entry = &server_opts.binds[LY_ARRAY_COUNT(server_opts.binds) - 1]; + nc_server_bind_entry_close(entry); + LY_ARRAY_DECREMENT_FREE(server_opts.binds); + --added; + } + } + if (binds_locked) { + /* BINDS UNLOCK */ + nc_mutex_unlock(&server_opts.binds_lock, __func__); + } + nc_server_bind_descs_free(descs); + return rc ? 1 : 0; +} + +void +nc_server_binds_destroy(void) +{ + LY_ARRAY_COUNT_TYPE u; + + /* BINDS LOCK */ + if (nc_mutex_lock(&server_opts.binds_lock, NC_BINDS_LOCK_TIMEOUT, __func__) != 1) { + return; + } + + LY_ARRAY_FOR(server_opts.binds, u) { + nc_server_bind_entry_close(&server_opts.binds[u]); + } + LY_ARRAY_FREE(server_opts.binds); + server_opts.binds = NULL; + + /* BINDS UNLOCK */ + nc_mutex_unlock(&server_opts.binds_lock, __func__); } /** From 68f36342c492dda1c51f5c9bb8eb5a69031442fe Mon Sep 17 00:00:00 2001 From: Roman Janota Date: Mon, 24 Aug 2026 21:04:19 +0200 Subject: [PATCH 02/10] session server REFACTOR separate lock for API-settable options config_lock doubled as the lock for the server_opts fields that are set through the API and are not part of struct nc_server_config. Their setters took it in WRITE mode, while their readers were only protected because the transport handshake happened to hold it in READ mode. Give them a lock of their own, opts_lock, so that the handshake can stop holding config_lock without leaving those readers unsynchronised. The fields are ch_dispatch_data, interactive_auth_clb and its data, pam_config_name, authkey_path_fmt, ssh_protocol_string, user_verify_clb, unix_socket_dir and unix_paths. opts_lock is a leaf lock, only config_lock may be held while acquiring it. It is also held on the authentication path, so it is never held across anything slow: the SSH protocol string and the PAM service name are copied out and pam_start() runs unlocked, and the interactive authentication and TLS verify callbacks are read together with their data pointer, then called after unlocking. Also free unix_socket_dir in nc_server_destroy(), it was leaked. --- src/server_config.c | 18 +++- src/session_p.h | 25 ++++- src/session_server.c | 113 +++++++++++++------ src/session_server_ssh.c | 144 +++++++++++++++++++------ src/session_server_ssh_auth_callback.c | 27 ++++- src/session_server_ssh_auth_message.c | 18 +++- src/session_server_ssh_wrapper.h | 23 ++++ src/session_server_tls.c | 22 +++- 8 files changed, 309 insertions(+), 81 deletions(-) diff --git a/src/server_config.c b/src/server_config.c index 89cc3b3d..a1dae573 100644 --- a/src/server_config.c +++ b/src/server_config.c @@ -5423,10 +5423,18 @@ nc_server_config_reconcile_chclients_dispatch(struct nc_server_config *old_cfg, int found; LY_ARRAY_COUNT_TYPE i; struct nc_ch_client **started_clients = NULL, **started_client_ptr; + struct nc_server_ch_dispatch_data dispatch_data; 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) { + /* OPTS READ LOCK */ + if (nc_rwlock_lock(&server_opts.opts_lock, NC_RWLOCK_READ, NC_OPTS_LOCK_TIMEOUT, __func__) != 1) { + return 1; + } + dispatch_data = server_opts.ch_dispatch_data; + /* OPTS READ UNLOCK */ + nc_rwlock_unlock(&server_opts.opts_lock, __func__); + + if (!dispatch_data.acquire_ctx_cb || !dispatch_data.release_ctx_cb || !dispatch_data.new_session_cb) { /* 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 - " @@ -5460,9 +5468,9 @@ nc_server_config_reconcile_chclients_dispatch(struct nc_server_config *old_cfg, } /* 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); + rc = _nc_connect_ch_client_dispatch(new_ch_client, dispatch_data.acquire_ctx_cb, + dispatch_data.release_ctx_cb, dispatch_data.ctx_cb_data, + dispatch_data.new_session_cb, dispatch_data.new_session_cb_data); if (rc) { /* FAILURE! trigger rollback */ goto rollback; diff --git a/src/session_p.h b/src/session_p.h index 0d33f172..a470ee2e 100644 --- a/src/session_p.h +++ b/src/session_p.h @@ -146,6 +146,12 @@ extern struct nc_server_opts server_opts; */ #define NC_BINDS_LOCK_TIMEOUT 1000 +/** + * @brief Timeout in msec for acquiring the opts_lock + * (only a few field reads or a single string duplication) + */ +#define NC_OPTS_LOCK_TIMEOUT 1000 + /** * @brief Timeout in msec for acquiring the config_lock * (socket binding and Call Home client dispatching can involve network operations) @@ -817,6 +823,23 @@ struct nc_server_opts { int sock; /**< Listening socket file descriptor. */ } *binds; /**< Listening socket registry (sized-array, see libyang docs). */ + /** + * @brief Lock for the server options settable only through the API, not through YANG data. + * + * Protects ::nc_server_opts.ch_dispatch_data, ::nc_server_opts.interactive_auth_clb, + * ::nc_server_opts.interactive_auth_data, ::nc_server_opts.interactive_auth_data_free, + * ::nc_server_opts.pam_config_name, ::nc_server_opts.authkey_path_fmt, + * ::nc_server_opts.ssh_protocol_string, ::nc_server_opts.user_verify_clb, + * ::nc_server_opts.unix_socket_dir and ::nc_server_opts.unix_paths. + * + * It is a leaf lock, never acquire another lock while holding it. Only ::nc_server_opts.config_lock + * may be held while acquiring it, never the other way around. Since it is also held on the + * authentication path, it must never be held across anything slow and, most importantly, never + * across a call to a user callback - read the callback and its data pointer as a pair, unlock, + * and only then call it. + */ + pthread_rwlock_t opts_lock; + #ifdef NC_ENABLED_SSH_TLS char *authkey_path_fmt; /**< Path to users' public keys that may contain tokens with special meaning. */ char *pam_config_name; /**< PAM configuration file name. */ @@ -830,7 +853,7 @@ struct nc_server_opts { /** * @brief Data for automatically dispatching Call Home clients. */ - struct { + struct nc_server_ch_dispatch_data { nc_server_ch_session_acquire_ctx_cb acquire_ctx_cb; /**< Acquiring libyang context callback. */ nc_server_ch_session_release_ctx_cb release_ctx_cb; /**< Releasing libyang context callback. */ void *ctx_cb_data; /**< Data passed to the callbacks above. */ diff --git a/src/session_server.c b/src/session_server.c index 28844279..9590a940 100644 --- a/src/session_server.c +++ b/src/session_server.c @@ -58,6 +58,7 @@ struct nc_server_opts server_opts = { .config_lock = PTHREAD_RWLOCK_INITIALIZER, .config_update_lock = PTHREAD_MUTEX_INITIALIZER, .binds_lock = PTHREAD_MUTEX_INITIALIZER, + .opts_lock = PTHREAD_RWLOCK_INITIALIZER, }; static nc_rpc_clb global_rpc_clb = NULL; @@ -250,8 +251,8 @@ nc_server_ch_set_dispatch_data(nc_server_ch_session_acquire_ctx_cb acquire_ctx_c { NC_CHECK_ARG_RET(NULL, acquire_ctx_cb, release_ctx_cb, new_session_cb, ); - /* CONFIG WRITE LOCK */ - if (nc_rwlock_lock(&server_opts.config_lock, NC_RWLOCK_WRITE, NC_CONFIG_LOCK_TIMEOUT, __func__) != 1) { + /* OPTS WRITE LOCK */ + if (nc_rwlock_lock(&server_opts.opts_lock, NC_RWLOCK_WRITE, NC_OPTS_LOCK_TIMEOUT, __func__) != 1) { return; } @@ -261,24 +262,24 @@ nc_server_ch_set_dispatch_data(nc_server_ch_session_acquire_ctx_cb acquire_ctx_c server_opts.ch_dispatch_data.new_session_cb = new_session_cb; server_opts.ch_dispatch_data.new_session_cb_data = new_session_cb_data; - /* CONFIG WRITE UNLOCK */ - nc_rwlock_unlock(&server_opts.config_lock, __func__); + /* OPTS WRITE UNLOCK */ + nc_rwlock_unlock(&server_opts.opts_lock, __func__); } API void nc_server_ch_set_new_session_fail_cb(nc_server_ch_new_session_fail_cb new_session_fail_cb, void *new_session_fail_cb_data) { - /* CONFIG WRITE LOCK */ - if (nc_rwlock_lock(&server_opts.config_lock, NC_RWLOCK_WRITE, NC_CONFIG_LOCK_TIMEOUT, __func__) != 1) { + /* OPTS WRITE LOCK */ + if (nc_rwlock_lock(&server_opts.opts_lock, NC_RWLOCK_WRITE, NC_OPTS_LOCK_TIMEOUT, __func__) != 1) { return; } server_opts.ch_dispatch_data.new_session_fail_cb = new_session_fail_cb; server_opts.ch_dispatch_data.new_session_fail_cb_data = new_session_fail_cb_data; - /* CONFIG WRITE UNLOCK */ - nc_rwlock_unlock(&server_opts.config_lock, __func__); + /* OPTS WRITE UNLOCK */ + nc_rwlock_unlock(&server_opts.opts_lock, __func__); } #endif @@ -414,6 +415,8 @@ nc_sock_listen_inet(const char *address, uint16_t port) /** * @brief Construct the full path to the UNIX socket. * + * @note The options read lock must be held. + * * @param[in] filename Name of the socket file. * @param[out] path Constructed full path to the UNIX socket (must be freed by the caller). * @return 0 on success, 1 on error. @@ -524,6 +527,11 @@ nc_server_unix_get_socket_path(const struct nc_endpt *endpt) const char *p = NULL; char *path = NULL; + /* OPTS READ LOCK */ + if (nc_rwlock_lock(&server_opts.opts_lock, NC_RWLOCK_READ, NC_OPTS_LOCK_TIMEOUT, __func__) != 1) { + return NULL; + } + /* check the endpoints options for type of socket path */ if (endpt->opts.unix->path_type == NC_UNIX_SOCKET_PATH_FILE) { /* UNIX socket endpoints always have only one bind, get its address */ @@ -531,7 +539,8 @@ nc_server_unix_get_socket_path(const struct nc_endpt *endpt) /* it is relative, we need to construct the full path */ if (nc_session_unix_construct_socket_path(p, &path)) { - return NULL; + path = NULL; + goto cleanup; } } else if (endpt->opts.unix->path_type == NC_UNIX_SOCKET_PATH_HIDDEN) { /* search the mappings, no need to construct the path */ @@ -543,15 +552,18 @@ nc_server_unix_get_socket_path(const struct nc_endpt *endpt) } if (!p) { ERR(NULL, "UNIX socket path mapping for endpoint \"%s\" not found.", endpt->name); - return NULL; + goto cleanup; } path = strdup(p); - NC_CHECK_ERRMEM_RET(!path, NULL); + NC_CHECK_ERRMEM_GOTO(!path, path = NULL, cleanup); } else { ERRINT; } +cleanup: + /* OPTS READ UNLOCK */ + nc_rwlock_unlock(&server_opts.opts_lock, __func__); return path; } @@ -1324,6 +1336,10 @@ nc_server_init(void) goto error; } + if (nc_server_init_rwlock(&server_opts.opts_lock)) { + goto error; + } + #ifdef NC_ENABLED_SSH_TLS if (curl_global_init(CURL_GLOBAL_SSL | CURL_GLOBAL_ACK_EINTR)) { ERR(NULL, "%s: failed to init CURL.", __func__); @@ -1364,10 +1380,16 @@ API int nc_server_destroy(void) { int rc = 0; - int config_update_locked = 0; + int config_update_locked = 0, opts_locked = 0; enum nc_rwlock_mode config_lock_mode = NC_RWLOCK_NONE; uint32_t i; +#ifdef NC_ENABLED_SSH_TLS + void *interactive_auth_data; + + void (*interactive_auth_data_free)(void *data); +#endif /* NC_ENABLED_SSH_TLS */ + for (i = 0; i < server_opts.capabilities_count; i++) { free(server_opts.capabilities[i]); } @@ -1418,6 +1440,13 @@ nc_server_destroy(void) /* destroy the server configuration */ nc_server_config_free(&server_opts.config); + /* OPTS WRITE LOCK */ + if (nc_rwlock_lock(&server_opts.opts_lock, NC_RWLOCK_WRITE, NC_OPTS_LOCK_TIMEOUT, __func__) != 1) { + rc = 1; + goto cleanup; + } + opts_locked = 1; + #ifdef NC_ENABLED_SSH_TLS free(server_opts.authkey_path_fmt); server_opts.authkey_path_fmt = NULL; @@ -1425,11 +1454,12 @@ nc_server_destroy(void) server_opts.pam_config_name = NULL; free(server_opts.ssh_protocol_string); server_opts.ssh_protocol_string = NULL; - if (server_opts.interactive_auth_data && server_opts.interactive_auth_data_free) { - server_opts.interactive_auth_data_free(server_opts.interactive_auth_data); - } + server_opts.interactive_auth_clb = NULL; + interactive_auth_data = server_opts.interactive_auth_data; + interactive_auth_data_free = server_opts.interactive_auth_data_free; server_opts.interactive_auth_data = NULL; server_opts.interactive_auth_data_free = NULL; + server_opts.user_verify_clb = NULL; /* Call Home dispatch data, its callback data does not have to be valid once the server is destroyed */ memset(&server_opts.ch_dispatch_data, 0, sizeof server_opts.ch_dispatch_data); @@ -1442,8 +1472,19 @@ nc_server_destroy(void) } LY_ARRAY_FREE(server_opts.unix_paths); server_opts.unix_paths = NULL; + free(server_opts.unix_socket_dir); + server_opts.unix_socket_dir = NULL; + + /* OPTS WRITE UNLOCK */ + nc_rwlock_unlock(&server_opts.opts_lock, __func__); + opts_locked = 0; #ifdef NC_ENABLED_SSH_TLS + /* free the user data only once the lock is released, the callback may call back into the library */ + if (interactive_auth_data && interactive_auth_data_free) { + interactive_auth_data_free(interactive_auth_data); + } + curl_global_cleanup(); nc_tls_backend_destroy_wrap(); ssh_finalize(); @@ -1456,6 +1497,9 @@ nc_server_destroy(void) #endif /* NC_ENABLED_SSH_TLS */ cleanup: + if (opts_locked) { + nc_rwlock_unlock(&server_opts.opts_lock, __func__); + } if (config_lock_mode != NC_RWLOCK_NONE) { nc_rwlock_unlock(&server_opts.config_lock, __func__); } @@ -4278,8 +4322,15 @@ _nc_connect_ch_client_dispatch(struct nc_ch_client *ch_client, nc_server_ch_sess arg->ctx_cb_data = ctx_cb_data; arg->new_session_cb = new_session_cb; arg->new_session_cb_data = new_session_cb_data; + /* OPTS READ LOCK */ + if (nc_rwlock_lock(&server_opts.opts_lock, NC_RWLOCK_READ, NC_OPTS_LOCK_TIMEOUT, __func__) != 1) { + rc = -1; + goto cleanup; + } arg->new_session_fail_cb = server_opts.ch_dispatch_data.new_session_fail_cb; arg->new_session_fail_cb_data = server_opts.ch_dispatch_data.new_session_fail_cb_data; + /* OPTS READ UNLOCK */ + nc_rwlock_unlock(&server_opts.opts_lock, __func__); /* create the self-pipe for signaling the thread to terminate */ if (pipe(arg->notify_pipe) == -1) { @@ -5264,8 +5315,8 @@ nc_server_set_unix_socket_path(const char *endpoint_name, const char *socket_pat NC_CHECK_ARG_RET(NULL, endpoint_name, socket_path, 1); - /* CONFIG WRITE LOCK */ - if (nc_rwlock_lock(&server_opts.config_lock, NC_RWLOCK_WRITE, NC_CONFIG_LOCK_TIMEOUT, __func__) != 1) { + /* OPTS WRITE LOCK */ + if (nc_rwlock_lock(&server_opts.opts_lock, NC_RWLOCK_WRITE, NC_OPTS_LOCK_TIMEOUT, __func__) != 1) { return 1; } @@ -5290,8 +5341,8 @@ nc_server_set_unix_socket_path(const char *endpoint_name, const char *socket_pat NC_CHECK_ERRMEM_GOTO(!pentry->path, rc = 1, cleanup); cleanup: - /* CONFIG WRITE UNLOCK */ - nc_rwlock_unlock(&server_opts.config_lock, __func__); + /* OPTS WRITE UNLOCK */ + nc_rwlock_unlock(&server_opts.opts_lock, __func__); return rc; } @@ -5306,8 +5357,8 @@ nc_server_get_unix_socket_path(const char *endpoint_name, char **socket_path) *socket_path = NULL; - /* CONFIG READ LOCK */ - if (nc_rwlock_lock(&server_opts.config_lock, NC_RWLOCK_READ, NC_CONFIG_LOCK_TIMEOUT, __func__) != 1) { + /* OPTS READ LOCK */ + if (nc_rwlock_lock(&server_opts.opts_lock, NC_RWLOCK_READ, NC_OPTS_LOCK_TIMEOUT, __func__) != 1) { return 1; } @@ -5327,8 +5378,8 @@ nc_server_get_unix_socket_path(const char *endpoint_name, char **socket_path) NC_CHECK_ERRMEM_GOTO(!*socket_path, rc = 1, cleanup); cleanup: - /* CONFIG READ UNLOCK */ - nc_rwlock_unlock(&server_opts.config_lock, __func__); + /* OPTS READ UNLOCK */ + nc_rwlock_unlock(&server_opts.opts_lock, __func__); return rc; } @@ -5337,8 +5388,8 @@ nc_server_set_unix_socket_dir(const char *dir) { int rc = 0; - /* CONFIG WRITE LOCK */ - if (nc_rwlock_lock(&server_opts.config_lock, NC_RWLOCK_WRITE, NC_CONFIG_LOCK_TIMEOUT, __func__) != 1) { + /* OPTS WRITE LOCK */ + if (nc_rwlock_lock(&server_opts.opts_lock, NC_RWLOCK_WRITE, NC_OPTS_LOCK_TIMEOUT, __func__) != 1) { return 1; } @@ -5347,8 +5398,8 @@ nc_server_set_unix_socket_dir(const char *dir) NC_CHECK_ERRMEM_GOTO(!server_opts.unix_socket_dir, rc = 1, cleanup); cleanup: - /* CONFIG WRITE UNLOCK */ - nc_rwlock_unlock(&server_opts.config_lock, __func__); + /* OPTS WRITE UNLOCK */ + nc_rwlock_unlock(&server_opts.opts_lock, __func__); return rc; } @@ -5359,8 +5410,8 @@ nc_server_get_unix_socket_dir(char **dir) *dir = NULL; - /* CONFIG READ LOCK */ - if (nc_rwlock_lock(&server_opts.config_lock, NC_RWLOCK_READ, NC_CONFIG_LOCK_TIMEOUT, __func__) != 1) { + /* OPTS READ LOCK */ + if (nc_rwlock_lock(&server_opts.opts_lock, NC_RWLOCK_READ, NC_OPTS_LOCK_TIMEOUT, __func__) != 1) { return 1; } @@ -5370,7 +5421,7 @@ nc_server_get_unix_socket_dir(char **dir) } cleanup: - /* CONFIG READ UNLOCK */ - nc_rwlock_unlock(&server_opts.config_lock, __func__); + /* OPTS READ UNLOCK */ + nc_rwlock_unlock(&server_opts.opts_lock, __func__); return rc; } diff --git a/src/session_server_ssh.c b/src/session_server_ssh.c index ff9ec325..d40f35a8 100644 --- a/src/session_server_ssh.c +++ b/src/session_server_ssh.c @@ -206,6 +206,8 @@ int nc_server_ssh_kbdint_select_method(struct nc_session *session, int local_users_supported, struct nc_auth_client *auth_client, enum nc_kbdint_backend *backend) { + int custom_clb_set; + assert(!local_users_supported || auth_client); if (!local_users_supported) { @@ -221,7 +223,15 @@ nc_server_ssh_kbdint_select_method(struct nc_session *session, int local_users_s return 1; } - if (server_opts.interactive_auth_clb) { + /* OPTS READ LOCK */ + if (nc_rwlock_lock(&server_opts.opts_lock, NC_RWLOCK_READ, NC_OPTS_LOCK_TIMEOUT, __func__) != 1) { + return 1; + } + custom_clb_set = server_opts.interactive_auth_clb ? 1 : 0; + /* OPTS READ UNLOCK */ + nc_rwlock_unlock(&server_opts.opts_lock, __func__); + + if (custom_clb_set) { /* custom callback has higher priority */ *backend = NC_KBDINT_BACKEND_CUSTOM_CLB; return 0; @@ -408,21 +418,48 @@ nc_server_ssh_pam_conv_fill(struct nc_session *session, struct pam_response *res return PAM_SUCCESS; } +int +nc_server_ssh_get_pam_conf_filename(char **filename) +{ + int rc = 0; + + *filename = NULL; + + /* OPTS READ LOCK */ + if (nc_rwlock_lock(&server_opts.opts_lock, NC_RWLOCK_READ, NC_OPTS_LOCK_TIMEOUT, __func__) != 1) { + return 1; + } + + if (server_opts.pam_config_name) { + *filename = strdup(server_opts.pam_config_name); + NC_CHECK_ERRMEM_GOTO(!*filename, rc = 1, cleanup); + } + +cleanup: + /* OPTS READ UNLOCK */ + nc_rwlock_unlock(&server_opts.opts_lock, __func__); + return rc; +} + int nc_server_ssh_pam_authenticate(struct nc_session *session, const char *username, const struct pam_conv *conv) { pam_handle_t *pam_h = NULL; + char *pam_config_name = NULL; int ret; - /* check the PAM configuration */ - if (!server_opts.pam_config_name) { + /* get the PAM configuration, PAM must not be called with the lock held */ + if (nc_server_ssh_get_pam_conf_filename(&pam_config_name)) { + return 1; + } + if (!pam_config_name) { ERR(session, "PAM configuration filename not set."); return 1; } /* initialize PAM and see if the given configuration file exists */ - ret = pam_start(server_opts.pam_config_name, username, conv, &pam_h); + ret = pam_start(pam_config_name, username, conv, &pam_h); if (ret != PAM_SUCCESS) { ERR(session, "PAM error occurred (%s).", pam_strerror(pam_h, ret)); goto cleanup; @@ -462,6 +499,7 @@ nc_server_ssh_pam_authenticate(struct nc_session *session, const char *username, if (pam_h && (pam_end(pam_h, ret) != PAM_SUCCESS)) { ERR(NULL, "PAM error occurred (%s).", pam_strerror(pam_h, ret)); } + free(pam_config_name); return ret; } @@ -769,11 +807,21 @@ static int nc_server_ssh_get_system_keys_path(const char *username, char **out_path) { int ret = 0, i, have_percent = 0, size = 0, idx = 0; - const char *path_fmt = server_opts.authkey_path_fmt; + char *path_fmt = NULL; char *path = NULL, *buf = NULL, *uid = NULL; struct passwd *pw, pw_buf; size_t buf_len = 0; + /* OPTS READ LOCK */ + if (nc_rwlock_lock(&server_opts.opts_lock, NC_RWLOCK_READ, NC_OPTS_LOCK_TIMEOUT, __func__) != 1) { + return 1; + } + if (server_opts.authkey_path_fmt) { + path_fmt = strdup(server_opts.authkey_path_fmt); + } + /* OPTS READ UNLOCK */ + nc_rwlock_unlock(&server_opts.opts_lock, __func__); + if (!path_fmt) { ERR(NULL, "System public keys path format not set."); return 1; @@ -798,7 +846,7 @@ nc_server_ssh_get_system_keys_path(const char *username, char **out_path) } else { /* no tokens, just copy the path and return */ *out_path = strdup(path_fmt); - NC_CHECK_ERRMEM_RET(!*out_path, 1); + NC_CHECK_ERRMEM_GOTO(!*out_path, ret = 1, cleanup); goto cleanup; } @@ -818,7 +866,7 @@ nc_server_ssh_get_system_keys_path(const char *username, char **out_path) /* UID */ ret = nc_server_ssh_str_append(0, uid, &size, &idx, &path); } else { - ERR(NULL, "Failed to parse system public keys path format \"%s\".", server_opts.authkey_path_fmt); + ERR(NULL, "Failed to parse system public keys path format \"%s\".", path_fmt); ret = 1; } @@ -841,6 +889,7 @@ nc_server_ssh_get_system_keys_path(const char *username, char **out_path) path = NULL; cleanup: + free(path_fmt); free(uid); free(buf); free(path); @@ -1239,8 +1288,8 @@ API void nc_server_ssh_set_interactive_auth_clb(int (*interactive_auth_clb)(const struct nc_session *session, ssh_session ssh_sess, ssh_message msg, void *user_data), void *user_data, void (*free_user_data)(void *user_data)) { - /* CONFIG LOCK */ - if (nc_rwlock_lock(&server_opts.config_lock, NC_RWLOCK_WRITE, NC_CONFIG_LOCK_TIMEOUT, __func__) != 1) { + /* OPTS WRITE LOCK */ + if (nc_rwlock_lock(&server_opts.opts_lock, NC_RWLOCK_WRITE, NC_OPTS_LOCK_TIMEOUT, __func__) != 1) { return; } @@ -1248,8 +1297,29 @@ nc_server_ssh_set_interactive_auth_clb(int (*interactive_auth_clb)(const struct server_opts.interactive_auth_data = user_data; server_opts.interactive_auth_data_free = free_user_data; - /* CONFIG UNLOCK */ - nc_rwlock_unlock(&server_opts.config_lock, __func__); + /* OPTS WRITE UNLOCK */ + nc_rwlock_unlock(&server_opts.opts_lock, __func__); +} + +int +nc_server_ssh_get_interactive_auth_clb(int (**clb)(const struct nc_session *session, ssh_session ssh_sess, + ssh_message msg, void *user_data), void **user_data) +{ + *clb = NULL; + *user_data = NULL; + + /* OPTS READ LOCK */ + if (nc_rwlock_lock(&server_opts.opts_lock, NC_RWLOCK_READ, NC_OPTS_LOCK_TIMEOUT, __func__) != 1) { + return 1; + } + + /* the callback and its data must be read as a pair */ + *clb = server_opts.interactive_auth_clb; + *user_data = server_opts.interactive_auth_data; + + /* OPTS READ UNLOCK */ + nc_rwlock_unlock(&server_opts.opts_lock, __func__); + return 0; } #ifdef HAVE_LIBPAM @@ -1261,8 +1331,8 @@ nc_server_ssh_set_pam_conf_filename(const char *filename) NC_CHECK_ARG_RET(NULL, filename, 1); - /* CONFIG LOCK */ - if (nc_rwlock_lock(&server_opts.config_lock, NC_RWLOCK_WRITE, NC_CONFIG_LOCK_TIMEOUT, __func__) != 1) { + /* OPTS WRITE LOCK */ + if (nc_rwlock_lock(&server_opts.opts_lock, NC_RWLOCK_WRITE, NC_OPTS_LOCK_TIMEOUT, __func__) != 1) { return 1; } @@ -1273,8 +1343,8 @@ nc_server_ssh_set_pam_conf_filename(const char *filename) ret = 1; } - /* CONFIG UNLOCK */ - nc_rwlock_unlock(&server_opts.config_lock, __func__); + /* OPTS WRITE UNLOCK */ + nc_rwlock_unlock(&server_opts.opts_lock, __func__); return ret; } @@ -1297,8 +1367,8 @@ nc_server_ssh_set_authkey_path_format(const char *path) NC_CHECK_ARG_RET(NULL, path, 1); - /* CONFIG LOCK */ - if (nc_rwlock_lock(&server_opts.config_lock, NC_RWLOCK_WRITE, NC_CONFIG_LOCK_TIMEOUT, __func__) != 1) { + /* OPTS WRITE LOCK */ + if (nc_rwlock_lock(&server_opts.opts_lock, NC_RWLOCK_WRITE, NC_OPTS_LOCK_TIMEOUT, __func__) != 1) { return 1; } @@ -1309,8 +1379,8 @@ nc_server_ssh_set_authkey_path_format(const char *path) ret = 1; } - /* CONFIG UNLOCK */ - nc_rwlock_unlock(&server_opts.config_lock, __func__); + /* OPTS WRITE UNLOCK */ + nc_rwlock_unlock(&server_opts.opts_lock, __func__); return ret; } @@ -1357,8 +1427,8 @@ nc_server_ssh_set_protocol_string(const char *prefix) protocol_str = nc_server_ssh_forge_protocol_string(prefix); NC_CHECK_ERRMEM_GOTO(!protocol_str, rc = 1, cleanup); - /* CONFIG LOCK */ - if (nc_rwlock_lock(&server_opts.config_lock, NC_RWLOCK_WRITE, NC_CONFIG_LOCK_TIMEOUT, __func__) != 1) { + /* OPTS WRITE LOCK */ + if (nc_rwlock_lock(&server_opts.opts_lock, NC_RWLOCK_WRITE, NC_OPTS_LOCK_TIMEOUT, __func__) != 1) { rc = 1; goto cleanup; } @@ -1368,8 +1438,8 @@ nc_server_ssh_set_protocol_string(const char *prefix) server_opts.ssh_protocol_string = protocol_str; protocol_str = NULL; - /* CONFIG UNLOCK */ - nc_rwlock_unlock(&server_opts.config_lock, __func__); + /* OPTS WRITE UNLOCK */ + nc_rwlock_unlock(&server_opts.opts_lock, __func__); cleanup: free(protocol_str); @@ -1738,10 +1808,10 @@ int nc_accept_ssh_session(struct nc_session *session, struct nc_server_ssh_opts *opts, int sock) { ssh_bind sbind = NULL; - int rc = 1, r; + int rc = 1, r, proto_str_set = 0; struct timespec ts_timeout; const char *err_msg; - char *proto_str = NULL, *proto_str_dyn = NULL; + char *proto_str = NULL; #if LIBSSH_0_12 struct nc_server_ssh_cb_data *cb_data = NULL; @@ -1822,14 +1892,24 @@ nc_accept_ssh_session(struct nc_session *session, struct nc_server_ssh_opts *opt } } - /* configure the ssh protocol identification string */ + /* configure the ssh protocol identification string, copy it so that the lock is not held any longer */ + /* OPTS READ LOCK */ + if (nc_rwlock_lock(&server_opts.opts_lock, NC_RWLOCK_READ, NC_OPTS_LOCK_TIMEOUT, __func__) != 1) { + rc = -1; + goto cleanup; + } if (server_opts.ssh_protocol_string) { - proto_str = server_opts.ssh_protocol_string; - } else { - proto_str_dyn = nc_server_ssh_forge_protocol_string(NULL); - NC_CHECK_ERRMEM_GOTO(!proto_str_dyn, rc = -1, cleanup); - proto_str = proto_str_dyn; + proto_str_set = 1; + proto_str = strdup(server_opts.ssh_protocol_string); } + /* OPTS READ UNLOCK */ + nc_rwlock_unlock(&server_opts.opts_lock, __func__); + + if (!proto_str_set) { + proto_str = nc_server_ssh_forge_protocol_string(NULL); + } + NC_CHECK_ERRMEM_GOTO(!proto_str, rc = -1, cleanup); + if (ssh_bind_options_set(sbind, SSH_BIND_OPTIONS_BANNER, proto_str)) { rc = -1; goto cleanup; @@ -1912,7 +1992,7 @@ nc_accept_ssh_session(struct nc_session *session, struct nc_server_ssh_opts *opt if (sock > -1) { close(sock); } - free(proto_str_dyn); + free(proto_str); ssh_bind_free(sbind); return rc; } diff --git a/src/session_server_ssh_auth_callback.c b/src/session_server_ssh_auth_callback.c index 646fb588..d9e2676f 100644 --- a/src/session_server_ssh_auth_callback.c +++ b/src/session_server_ssh_auth_callback.c @@ -285,13 +285,18 @@ static int nc_server_ssh_cb_kbdint_pam_request(struct nc_server_ssh_cb_data *cb_data, ssh_message message) { struct nc_server_ssh_cb_pam_data *pam_data; + char *pam_config_name = NULL; int rc; /* check the PAM configuration */ - if (!server_opts.pam_config_name) { + if (nc_server_ssh_get_pam_conf_filename(&pam_config_name)) { + return SSH_AUTH_DENIED; + } + if (!pam_config_name) { ERR(cb_data->session, "PAM configuration filename not set."); return SSH_AUTH_DENIED; } + free(pam_config_name); /* cancel any in-progress PAM exchange, e.g. the client abandoned the previous one */ nc_server_ssh_cb_kbdint_pam_cancel_stored(cb_data); @@ -655,6 +660,10 @@ nc_server_ssh_cb_auth_kbdint(ssh_message message, ssh_session UNUSED(libssh_sess int ret = SSH_AUTH_DENIED; const char *user; + int (*interactive_auth_clb)(const struct nc_session *session, ssh_session ssh_sess, ssh_message msg, + void *user_data); + void *interactive_auth_data; + /* Extract the username from the message. */ if (ssh_message_auth_kbdint_is_response(message) && session->username) { user = session->username; @@ -682,9 +691,19 @@ nc_server_ssh_cb_auth_kbdint(ssh_message message, ssh_session UNUSED(libssh_sess } if (backend == NC_KBDINT_BACKEND_CUSTOM_CLB) { - /* custom interactive auth callback */ - ret = server_opts.interactive_auth_clb(session, - session->ti.libssh.session, message, server_opts.interactive_auth_data); + /* custom interactive auth callback, it must not be called with the options lock held */ + if (nc_server_ssh_get_interactive_auth_clb(&interactive_auth_clb, &interactive_auth_data)) { + nc_server_ssh_auth_attempt_failed(session); + return SSH_AUTH_DENIED; + } + if (!interactive_auth_clb) { + /* the callback was unset in the meantime */ + ERR(session, "Custom keyboard-interactive authentication callback not set."); + nc_server_ssh_auth_attempt_failed(session); + return SSH_AUTH_DENIED; + } + + ret = interactive_auth_clb(session, session->ti.libssh.session, message, interactive_auth_data); } else { ret = nc_server_ssh_cb_kbdint_system(cb_data, message, user); } diff --git a/src/session_server_ssh_auth_message.c b/src/session_server_ssh_auth_message.c index f977463b..e7ba6974 100644 --- a/src/session_server_ssh_auth_message.c +++ b/src/session_server_ssh_auth_message.c @@ -282,15 +282,27 @@ nc_server_ssh_msg_auth_kbdint(struct nc_session *session, int local_users_suppor int r; enum nc_kbdint_backend backend; + int (*interactive_auth_clb)(const struct nc_session *session, ssh_session ssh_sess, ssh_message msg, + void *user_data); + void *interactive_auth_data; + /* select the kbdint backend based on the configuration */ if (nc_server_ssh_kbdint_select_method(session, local_users_supported, auth_client, &backend)) { return 1; } if (backend == NC_KBDINT_BACKEND_CUSTOM_CLB) { - /* custom callback has higher priority */ - r = server_opts.interactive_auth_clb(session, - session->ti.libssh.session, msg, server_opts.interactive_auth_data); + /* custom callback has higher priority, it must not be called with the options lock held */ + if (nc_server_ssh_get_interactive_auth_clb(&interactive_auth_clb, &interactive_auth_data)) { + return 1; + } + if (!interactive_auth_clb) { + /* the callback was unset in the meantime */ + ERR(session, "Custom keyboard-interactive authentication callback not set."); + return 1; + } + + r = interactive_auth_clb(session, session->ti.libssh.session, msg, interactive_auth_data); } else { r = nc_server_ssh_msg_auth_kbdint_system(session, msg); } diff --git a/src/session_server_ssh_wrapper.h b/src/session_server_ssh_wrapper.h index 09b85806..8a68eb5b 100644 --- a/src/session_server_ssh_wrapper.h +++ b/src/session_server_ssh_wrapper.h @@ -373,6 +373,19 @@ enum nc_kbdint_backend { int nc_server_ssh_kbdint_select_method(struct nc_session *session, int local_users_supported, struct nc_auth_client *auth_client, enum nc_kbdint_backend *backend); +/** + * @brief Get the configured custom keyboard-interactive authentication callback and its data. + * + * The callback is an application callback that may call back into the library, so it is read + * together with its data and only called once the options lock is released. + * + * @param[out] clb Custom keyboard-interactive authentication callback, NULL if not set. + * @param[out] user_data Data to pass to @p clb . + * @return 0 on success, 1 on error. + */ +int nc_server_ssh_get_interactive_auth_clb(int (**clb)(const struct nc_session *session, ssh_session ssh_sess, + ssh_message msg, void *user_data), void **user_data); + /** * @brief Check a channel subsystem request against the session state. * @@ -426,6 +439,16 @@ int nc_server_ssh_pam_conv_parse(struct nc_session *session, int n_messages, int nc_server_ssh_pam_conv_fill(struct nc_session *session, struct pam_response *resp, int n_prompts, int n_answers, const char **answers); +/** + * @brief Get a copy of the configured PAM service name. + * + * PAM must never be called while holding the options lock, so the name is always copied. + * + * @param[out] filename PAM service name copy, NULL if none is configured. + * @return 0 on success, 1 on error. + */ +int nc_server_ssh_get_pam_conf_filename(char **filename); + /** * @brief Run the PAM authentication sequence with a prepared conversation. * diff --git a/src/session_server_tls.c b/src/session_server_tls.c index f0118968..08cf0aa6 100644 --- a/src/session_server_tls.c +++ b/src/session_server_tls.c @@ -615,6 +615,8 @@ nc_server_tls_verify_cert(void *cert, int depth, int trusted, struct nc_tls_veri struct nc_session *session = cb_data->session; void *cert_chain = cb_data->chain; + int (*user_verify_clb)(const struct nc_session *session); + if (session->username) { /* already verified */ return 0; @@ -661,7 +663,17 @@ nc_server_tls_verify_cert(void *cert, int depth, int trusted, struct nc_tls_veri goto cleanup; } - if (server_opts.user_verify_clb && !server_opts.user_verify_clb(session)) { + /* OPTS READ LOCK */ + if (nc_rwlock_lock(&server_opts.opts_lock, NC_RWLOCK_READ, NC_OPTS_LOCK_TIMEOUT, __func__) != 1) { + rc = -1; + goto cleanup; + } + user_verify_clb = server_opts.user_verify_clb; + /* OPTS READ UNLOCK */ + nc_rwlock_unlock(&server_opts.opts_lock, __func__); + + /* the callback must not be called with the options lock held */ + if (user_verify_clb && !user_verify_clb(session)) { VRB(session, "Cert verify: user verify callback revoked authorization."); rc = 1; goto cleanup; @@ -688,15 +700,15 @@ nc_session_get_client_cert(const struct nc_session *session) API void nc_server_tls_set_verify_clb(int (*verify_clb)(const struct nc_session *session)) { - /* CONFIG LOCK */ - if (nc_rwlock_lock(&server_opts.config_lock, NC_RWLOCK_WRITE, NC_CONFIG_LOCK_TIMEOUT, __func__) != 1) { + /* OPTS WRITE LOCK */ + if (nc_rwlock_lock(&server_opts.opts_lock, NC_RWLOCK_WRITE, NC_OPTS_LOCK_TIMEOUT, __func__) != 1) { return; } server_opts.user_verify_clb = verify_clb; - /* CONFIG UNLOCK */ - nc_rwlock_unlock(&server_opts.config_lock, __func__); + /* OPTS WRITE UNLOCK */ + nc_rwlock_unlock(&server_opts.opts_lock, __func__); } int From 745384762151556b1ac1cf4cb4df499c21a8eecc Mon Sep 17 00:00:00 2001 From: Roman Janota Date: Tue, 25 Aug 2026 01:12:46 +0200 Subject: [PATCH 03/10] session server REFACTOR refcounted config snapshot server_opts.config was held in READ mode across complete transport handshakes - key exchange plus authentication, bounded only by auth-timeout and unbounded when it is configured 0 - so a stalled handshake blocked every configuration update. It becomes a pointer to a refcounted, immutable struct nc_server_config. A handshake path takes a reference under the READ lock, releases the lock and works from the pinned pointer; an applier builds the next generation off-line, swaps the pointer under the WRITE lock and drops the old generation's reference, which is freed by its last reader. The lock is now held for a pointer read plus a refcount change, never across network I/O. The refcount is decremented with acq_rel so the reads of a generation are ordered before the free() of whoever drops the last reference; the increment stays relaxed, the READ lock orders it. The transport options are resolved lazily during the handshake, so the resolvers and the helpers between them and the handshake entry points take an explicit configuration parameter. The pinned generation lives in exactly one place, session->opts.server.config, a borrowed pointer set only by nc_accept() and nc_connect_ch_endpt() and cleared by them once the handshake is over. struct nc_ch_client.thread is mutable runtime state that used to be copied across generations, so the Call Home thread handles move to a registry in server_opts, superseding the redispatch fix. Stopping a client looks the thread up there, unlinks it, and only then joins it, so the whole unlock/join/relock deadlock avoidance and its leak path are gone. A thread whose client is not in the published generation now waits for it instead of exiting, since the socket and thread reconciles run before the swap. server_opts gains an atomic mirror of idle_timeout, stored at swap time. That fixes the two unlocked reads in nc_send_hello_io() and nc_server_recv_hello_io() and removes the last configuration access from nc_ps_poll_session_io(), so nc_ps_poll_sess() no longer takes the configuration lock on every poll iteration of every session. Also fixes nc_server_notif_cert_exp_dates_get(), which aliased the keystore and the truststore in its declaration list, before taking the lock - harmless with an embedded struct, a use-after-free with a pointer. test_config_update_during_auth now asserts the elapsed time, which is what the change is actually about, and four new tests cover the immediate close of a removed endpoint, the API setters during a handshake, a Call Home thread surviving a swap, and concurrent applies racing an accept. --- compat/compat.h.in | 3 + src/server_config.c | 411 ++++++++++++------ src/session.c | 32 +- src/session_openssl.c | 2 +- src/session_p.h | 128 ++++-- src/session_server.c | 724 ++++++++++++++++++------------- src/session_server_ssh.c | 52 ++- src/session_server_ssh_wrapper.h | 15 +- src/session_server_tls.c | 83 ++-- src/session_wrapper.h | 4 +- tests/test_config.c | 339 ++++++++++++++- 11 files changed, 1254 insertions(+), 539 deletions(-) diff --git a/compat/compat.h.in b/compat/compat.h.in index 597c4b63..cbea52af 100644 --- a/compat/compat.h.in +++ b/compat/compat.h.in @@ -122,6 +122,7 @@ # define ATOMIC_INC_RELAXED(var) atomic_fetch_add_explicit(&(var), 1, memory_order_relaxed) # define ATOMIC_ADD_RELAXED(var, x) atomic_fetch_add_explicit(&(var), x, memory_order_relaxed) # define ATOMIC_DEC_RELAXED(var) atomic_fetch_sub_explicit(&(var), 1, memory_order_relaxed) +# define ATOMIC_DEC_ACQ_REL(var) atomic_fetch_sub_explicit(&(var), 1, memory_order_acq_rel) # define ATOMIC_SUB_RELAXED(var, x) atomic_fetch_sub_explicit(&(var), x, memory_order_relaxed) # define ATOMIC_PTR_COMPARE_EXCHANGE_RELAXED(var, exp, des, result) \ @@ -144,6 +145,8 @@ # define ATOMIC_INC_RELAXED(var) __sync_fetch_and_add(&(var), 1) # define ATOMIC_ADD_RELAXED(var, x) __sync_fetch_and_add(&(var), x) # define ATOMIC_DEC_RELAXED(var) __sync_fetch_and_sub(&(var), 1) +/* __sync_fetch_and_sub() is already a full barrier */ +# define ATOMIC_DEC_ACQ_REL(var) __sync_fetch_and_sub(&(var), 1) # define ATOMIC_SUB_RELAXED(var, x) __sync_fetch_and_sub(&(var), x) # define ATOMIC_PTR_COMPARE_EXCHANGE_RELAXED(var, exp, des, result) \ diff --git a/src/server_config.c b/src/server_config.c index a1dae573..3a4a9a00 100644 --- a/src/server_config.c +++ b/src/server_config.c @@ -502,11 +502,11 @@ nc_server_config_truststore_free(struct nc_truststore *ts) #endif /* NC_ENABLED_SSH_TLS */ /** - * @brief Free server configuration data. + * @brief Free the data of a server configuration generation. * * @param[in] config Server configuration to free. */ -void +static void nc_server_config_free(struct nc_server_config *config) { struct nc_endpt *endpt; @@ -601,6 +601,46 @@ nc_server_config_free(struct nc_server_config *config) memset(config, 0, sizeof(*config)); } +const struct nc_server_config * +nc_server_config_acquire(void) +{ + struct nc_server_config *config; + + /* CONFIG READ LOCK - only the pointer read and the refcount increment */ + if (nc_rwlock_lock(&server_opts.config_lock, NC_RWLOCK_READ, NC_CONFIG_LOCK_TIMEOUT, __func__) != 1) { + return NULL; + } + + config = server_opts.config; + if (config) { + /* the read lock provides the ordering, no new reference to a swapped out generation + * can ever be taken because only server_opts.config is ever read here */ + ATOMIC_INC_RELAXED(config->refcount); + } + + /* CONFIG READ UNLOCK */ + nc_rwlock_unlock(&server_opts.config_lock, __func__); + return config; +} + +void +nc_server_config_release(const struct nc_server_config *config) +{ + struct nc_server_config *cfg = (struct nc_server_config *)config; + + if (!cfg) { + return; + } + + /* acq_rel so that all the reads of this generation are ordered before the free() done + * by whoever drops the last reference */ + if (ATOMIC_DEC_ACQ_REL(cfg->refcount) == 1) { + /* we held the last reference */ + nc_server_config_free(cfg); + free(cfg); + } +} + API int nc_server_config_load_modules(struct ly_ctx **ctx) { @@ -5377,28 +5417,61 @@ nc_server_config_libnetconf2_netconf_server(const struct lyd_node *tree, int is_ #ifdef NC_ENABLED_SSH_TLS /** - * @brief Check if there are any new Call Home clients created in the new configuration. + * @brief Check whether a Call Home client name is present in an array of names. + * + * @param[in] names Array of names (sized-array, see libyang docs). + * @param[in] name Name to look for. + * @return 1 if @p name is present, 0 otherwise. + */ +static int +nc_server_config_ch_name_found(char **names, const char *name) +{ + LY_ARRAY_COUNT_TYPE u; + + LY_ARRAY_FOR(names, u) { + if (!strcmp(names[u], name)) { + return 1; + } + } + + return 0; +} + +/** + * @brief Check whether a server configuration contains a Call Home client of the given name. + * + * @param[in] config Server configuration. + * @param[in] name Name of the Call Home client to look for. + * @return 1 if the client is configured, 0 otherwise. + */ +static int +nc_server_config_ch_client_configured(const struct nc_server_config *config, const char *name) +{ + LY_ARRAY_COUNT_TYPE u; + + LY_ARRAY_FOR(config->ch_clients, u) { + if (!strcmp(config->ch_clients[u].name, name)) { + return 1; + } + } + + return 0; +} + +/** + * @brief Check if the new configuration contains a Call Home client that has no thread running. * - * @param[in] old_cfg Old, currently active server configuration. * @param[in] new_cfg New server configuration currently being applied. + * @param[in] running Names of the Call Home clients with a running thread (sized-array, see libyang docs). * @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) +nc_server_config_new_ch_clients_created(const struct nc_server_config *new_cfg, char **running) { - struct nc_ch_client *old_ch_client, *new_ch_client; - int found; + LY_ARRAY_COUNT_TYPE u; - /* 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) { + LY_ARRAY_FOR(new_cfg->ch_clients, u) { + if (!nc_server_config_ch_name_found(running, new_cfg->ch_clients[u].name)) { return 1; } } @@ -5408,21 +5481,19 @@ nc_server_config_new_ch_clients_created(struct nc_server_config *old_cfg, struct } /** - * @brief Atomically dispatch new Call Home clients and reuse existing ones. + * @brief Atomically dispatch new Call Home clients and keep the already running ones. + * + * The running clients are learned from the Call Home thread registry, not from any configuration. * - * @param[in,out] old_cfg Old, currently active server configuration. - * @param[in,out] new_cfg New server configuration currently being applied. + * @param[in] new_cfg New server configuration currently being applied. * @return 0 on success, 1 on error. */ static int -nc_server_config_reconcile_chclients_dispatch(struct nc_server_config *old_cfg, - struct nc_server_config *new_cfg) +nc_server_config_reconcile_chclients_dispatch(const struct nc_server_config *new_cfg) { int rc = 0; - struct nc_ch_client *old_ch_client, *new_ch_client; - int found; - LY_ARRAY_COUNT_TYPE i; - struct nc_ch_client **started_clients = NULL, **started_client_ptr; + LY_ARRAY_COUNT_TYPE u; + char **running = NULL, **started = NULL, **started_name; struct nc_server_ch_dispatch_data dispatch_data; int dispatch_new_clients = 1; @@ -5434,9 +5505,12 @@ nc_server_config_reconcile_chclients_dispatch(struct nc_server_config *old_cfg, /* OPTS READ UNLOCK */ nc_rwlock_unlock(&server_opts.opts_lock, __func__); + /* learn which clients are running right now */ + NC_CHECK_GOTO(rc = nc_server_ch_thread_names_get(&running), cleanup); + if (!dispatch_data.acquire_ctx_cb || !dispatch_data.release_ctx_cb || !dispatch_data.new_session_cb) { /* 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)) { + if (nc_server_config_new_ch_clients_created(new_cfg, running)) { WRN(NULL, "New Call Home clients were created but Call Home dispatch callbacks are not set - " "new clients will not be dispatched automatically."); } @@ -5450,25 +5524,14 @@ nc_server_config_reconcile_chclients_dispatch(struct nc_server_config *old_cfg, */ 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) { - if (!new_ch_client->thread) { - /* the new config may have been built from scratch (::nc_server_config_setup_data()), in which - * case the thread data of an already running client is only present in the old config */ - LY_ARRAY_FOR(old_cfg->ch_clients, struct nc_ch_client, old_ch_client) { - if (!strcmp(old_ch_client->name, new_ch_client->name)) { - new_ch_client->thread = old_ch_client->thread; - break; - } - } - } - - if (new_ch_client->thread) { + LY_ARRAY_FOR(new_cfg->ch_clients, u) { + if (nc_server_config_ch_name_found(running, new_cfg->ch_clients[u].name)) { /* already running */ continue; } /* this is a new Call Home client, dispatch it */ - rc = _nc_connect_ch_client_dispatch(new_ch_client, dispatch_data.acquire_ctx_cb, + rc = _nc_connect_ch_client_dispatch(new_cfg->ch_clients[u].name, dispatch_data.acquire_ctx_cb, dispatch_data.release_ctx_cb, dispatch_data.ctx_cb_data, dispatch_data.new_session_cb, dispatch_data.new_session_cb_data); if (rc) { @@ -5476,32 +5539,27 @@ nc_server_config_reconcile_chclients_dispatch(struct nc_server_config *old_cfg, goto rollback; } - /* 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; + /* successfully started, track the client for a potential rollback */ + LY_ARRAY_NEW_GOTO(NULL, started, started_name, rc, rollback); + *started_name = strdup(new_cfg->ch_clients[u].name); + NC_CHECK_ERRMEM_GOTO(!*started_name, rc = 1, rollback); } } /* * == PHASE 2: STOP DELETED CLIENTS (COMMIT) == - * All new clients started successfully. Now stop old clients + * All new clients started successfully. Now stop the running clients * that are not present in the new configuration. */ - LY_ARRAY_FOR(old_cfg->ch_clients, struct nc_ch_client, old_ch_client) { - found = 0; - LY_ARRAY_FOR(new_cfg->ch_clients, struct nc_ch_client, new_ch_client) { - if (!strcmp(old_ch_client->name, new_ch_client->name)) { - found = 1; - break; - } + LY_ARRAY_FOR(running, u) { + if (nc_server_config_ch_client_configured(new_cfg, running[u])) { + continue; } - 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(old_ch_client))) { - ERR(NULL, "Failed to dispatch stop for Call Home client \"%s\".", old_ch_client->name); - goto rollback; - } + /* this Call Home client was deleted, notify it to stop */ + if ((rc = nc_session_server_ch_client_dispatch_stop(running[u]))) { + ERR(NULL, "Failed to dispatch stop for Call Home client \"%s\".", running[u]); + goto rollback; } } @@ -5515,15 +5573,15 @@ nc_server_config_reconcile_chclients_dispatch(struct nc_server_config *old_cfg, * An error occurred during PHASE 1. Stop any new threads we *just* started * to return to the pre-call state. */ - LY_ARRAY_FOR(started_clients, i) { - nc_session_server_ch_client_dispatch_stop(started_clients[i]); + LY_ARRAY_FOR(started, u) { + nc_session_server_ch_client_dispatch_stop(started[u]); } /* rc is already set to non-zero from the failure point */ cleanup: - /* free the tracking list */ - LY_ARRAY_FREE(started_clients); - return rc; + nc_server_ch_thread_names_free(running); + nc_server_ch_thread_names_free(started); + return rc ? 1 : 0; } /** @@ -6002,6 +6060,8 @@ nc_server_config_truststore_dup(const struct nc_truststore *src, struct nc_trust /** * @brief Create a deep copy of the server configuration. * + * @note On error, @p dst is left partially filled, freeing it is up to its owner. + * * @param[in] src Source server configuration to copy from. * @param[out] dst Server configuration copy. * @return 0 on success, 1 on error. @@ -6134,8 +6194,6 @@ 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); } @@ -6158,10 +6216,6 @@ nc_server_config_dup(const struct nc_server_config *src, struct nc_server_config #endif /* NC_ENABLED_SSH_TLS */ cleanup: - if (rc) { - nc_server_config_free(dst); - } - return rc; } @@ -6184,11 +6238,51 @@ nc_server_config_cert_exp_notif_thread_wakeup(void) #endif /* NC_ENABLED_SSH_TLS */ +/** + * @brief Allocate a new server configuration generation. + * + * @param[out] config New generation with a single reference held by the caller. + * @return 0 on success, 1 on error. + */ +static int +nc_server_config_new(struct nc_server_config **config) +{ + *config = calloc(1, sizeof **config); + NC_CHECK_ERRMEM_RET(!*config, 1); + + /* the applier's reference, transferred to server_opts.config once the generation is published */ + ATOMIC_STORE_RELAXED((*config)->refcount, 1); + return 0; +} + +/** + * @brief Publish a new server configuration generation and drop the reference of the old one. + * + * @note The configuration WRITE lock must be held. + * + * @param[in] config New generation to publish, its reference is transferred to ::nc_server_opts.config. + * @return Old generation, the caller must release it once the lock is released. + */ +static struct nc_server_config * +nc_server_config_publish(struct nc_server_config *config) +{ + struct nc_server_config *old_config; + + old_config = server_opts.config; + server_opts.config = config; + + /* mirror the idle timeout so that the hello and poll paths do not need the config at all */ + ATOMIC_STORE_RELAXED(server_opts.idle_timeout, config->idle_timeout); + + return old_config; +} + API int nc_server_config_setup_diff(const struct lyd_node *data) { int ret = 0; - struct nc_server_config config_copy = {0}; + const struct nc_server_config *cur_config = NULL; + struct nc_server_config *config_copy = NULL, *old_config = NULL; NC_CHECK_ARG_RET(NULL, data, 1); @@ -6202,75 +6296,87 @@ nc_server_config_setup_diff(const struct lyd_node *data) return 1; } - /* CONFIG RD LOCK */ - if (nc_rwlock_lock(&server_opts.config_lock, NC_RWLOCK_READ, NC_CONFIG_APPLY_LOCK_TIMEOUT, __func__) != 1) { - ERR(NULL, "Timed out waiting for the configuration lock, the new configuration was not applied."); - ret = 1; - goto cleanup; - } + NC_CHECK_GOTO(ret = nc_server_config_new(&config_copy), cleanup); /* create a copy of the current config to work with, so that we can revert to it in case of error */ - NC_CHECK_ERR_GOTO(ret = nc_server_config_dup(&server_opts.config, &config_copy), - ERR(NULL, "Duplicating current server configuration failed."), cleanup_unlock); + cur_config = nc_server_config_acquire(); + NC_CHECK_ERR_GOTO(!cur_config, ERR(NULL, "Acquiring the current server configuration failed."); ret = 1, cleanup); - /* UNLOCK */ - nc_rwlock_unlock(&server_opts.config_lock, __func__); + NC_CHECK_ERR_GOTO(ret = nc_server_config_dup(cur_config, config_copy), + ERR(NULL, "Duplicating current server configuration failed."), cleanup); + + nc_server_config_release(cur_config); + cur_config = NULL; #ifdef NC_ENABLED_SSH_TLS /* configure keystore */ - NC_CHECK_ERR_GOTO(ret = nc_server_config_keystore(data, 1, &config_copy), + NC_CHECK_ERR_GOTO(ret = nc_server_config_keystore(data, 1, config_copy), ERR(NULL, "Applying ietf-keystore configuration failed."), cleanup); /* configure truststore */ - NC_CHECK_ERR_GOTO(ret = nc_server_config_truststore(data, 1, &config_copy), + NC_CHECK_ERR_GOTO(ret = nc_server_config_truststore(data, 1, config_copy), ERR(NULL, "Applying ietf-truststore configuration failed."), cleanup); #endif /* NC_ENABLED_SSH_TLS */ /* configure netconf-server */ - NC_CHECK_ERR_GOTO(ret = nc_server_config_netconf_server(data, 1, &config_copy), + NC_CHECK_ERR_GOTO(ret = nc_server_config_netconf_server(data, 1, config_copy), ERR(NULL, "Applying ietf-netconf-server configuration failed."), cleanup); /* configure libnetconf2-netconf-server */ - NC_CHECK_ERR_GOTO(ret = nc_server_config_libnetconf2_netconf_server(data, NC_OP_UNKNOWN, &config_copy), + NC_CHECK_ERR_GOTO(ret = nc_server_config_libnetconf2_netconf_server(data, NC_OP_UNKNOWN, config_copy), ERR(NULL, "Applying libnetconf2-netconf-server configuration failed."), cleanup); - /* CONFIG WR LOCK */ + /* start listening on new endpoints */ + NC_CHECK_ERR_GOTO(ret = nc_server_binds_reconcile(config_copy), + ERR(NULL, "Starting to listen on new endpoints failed."), cleanup); + +#ifdef NC_ENABLED_SSH_TLS + /* dispatch new call-home threads */ + NC_CHECK_ERR_GOTO(ret = nc_server_config_reconcile_chclients_dispatch(config_copy), + ERR(NULL, "Dispatching new call-home threads failed."), cleanup); +#endif /* NC_ENABLED_SSH_TLS */ + + /* CONFIG WR LOCK - only the pointer swap */ if (nc_rwlock_lock(&server_opts.config_lock, NC_RWLOCK_WRITE, NC_CONFIG_APPLY_LOCK_TIMEOUT, __func__) != 1) { ERR(NULL, "Timed out waiting for the configuration lock, the new configuration was not applied."); ret = 1; - goto cleanup; + goto rollback; } - /* start listening on new endpoints */ - NC_CHECK_ERR_GOTO(ret = nc_server_binds_reconcile(&config_copy), - ERR(NULL, "Starting to listen on new endpoints failed."), cleanup_unlock); + /* publish the new generation, the reference is transferred to server_opts.config */ + old_config = nc_server_config_publish(config_copy); + config_copy = NULL; -#ifdef NC_ENABLED_SSH_TLS - /* dispatch new call-home threads */ - NC_CHECK_ERR_GOTO(ret = nc_server_config_reconcile_chclients_dispatch(&server_opts.config, &config_copy), - ERR(NULL, "Dispatching new call-home threads failed."), cleanup_unlock); -#endif /* NC_ENABLED_SSH_TLS */ + /* CONFIG UNLOCK */ + nc_rwlock_unlock(&server_opts.config_lock, __func__); - /* swap: free old, keep new, zero out the copy just in case to avoid double free */ - nc_server_config_free(&server_opts.config); - server_opts.config = config_copy; - memset(&config_copy, 0, sizeof config_copy); + /* the old generation is freed once its last reader releases it */ + nc_server_config_release(old_config); #ifdef NC_ENABLED_SSH_TLS /* wake up the cert expiration notif thread */ nc_server_config_cert_exp_notif_thread_wakeup(); #endif /* NC_ENABLED_SSH_TLS */ -cleanup_unlock: - /* CONFIG UNLOCK */ - nc_rwlock_unlock(&server_opts.config_lock, __func__); + goto cleanup; -cleanup: - if (ret) { - /* free the new config in case of error */ - nc_server_config_free(&config_copy); +rollback: + /* the sockets and the Call Home threads were already reconciled with the new generation, + * reconcile them back with the one that stays published */ + cur_config = nc_server_config_acquire(); + if (cur_config) { + nc_server_binds_reconcile(cur_config); +#ifdef NC_ENABLED_SSH_TLS + nc_server_config_reconcile_chclients_dispatch(cur_config); +#endif /* NC_ENABLED_SSH_TLS */ } +cleanup: + nc_server_config_release(cur_config); + + /* release the new generation, it was either not published or it is NULL */ + nc_server_config_release(config_copy); + /* CONFIG UPDATE UNLOCK */ nc_mutex_unlock(&server_opts.config_update_lock, __func__); return ret; @@ -6281,7 +6387,8 @@ nc_server_config_setup_data(const struct lyd_node *data) { int ret = 0; const struct lyd_node *tree, *iter; - struct nc_server_config config = {0}; + const struct nc_server_config *cur_config = NULL; + struct nc_server_config *config = NULL, *old_config = NULL; NC_CHECK_ARG_RET(NULL, data, 1); @@ -6311,62 +6418,76 @@ nc_server_config_setup_data(const struct lyd_node *data) * - if something fails, the old config is still intact * - not having to hold the config_lock for a long time while applying the new config */ + NC_CHECK_GOTO(ret = nc_server_config_new(&config), cleanup); #ifdef NC_ENABLED_SSH_TLS /* configure keystore */ - NC_CHECK_ERR_GOTO(ret = nc_server_config_keystore(data, 0, &config), + NC_CHECK_ERR_GOTO(ret = nc_server_config_keystore(data, 0, config), ERR(NULL, "Applying ietf-keystore configuration failed."), cleanup); /* configure truststore */ - NC_CHECK_ERR_GOTO(ret = nc_server_config_truststore(data, 0, &config), + NC_CHECK_ERR_GOTO(ret = nc_server_config_truststore(data, 0, config), ERR(NULL, "Applying ietf-truststore configuration failed."), cleanup); #endif /* NC_ENABLED_SSH_TLS */ /* configure netconf-server */ - NC_CHECK_ERR_GOTO(ret = nc_server_config_netconf_server(data, 0, &config), + NC_CHECK_ERR_GOTO(ret = nc_server_config_netconf_server(data, 0, config), ERR(NULL, "Applying ietf-netconf-server configuration failed."), cleanup); /* configure libnetconf2-netconf-server */ - NC_CHECK_ERR_GOTO(ret = nc_server_config_libnetconf2_netconf_server(data, NC_OP_UNKNOWN, &config), + NC_CHECK_ERR_GOTO(ret = nc_server_config_libnetconf2_netconf_server(data, NC_OP_UNKNOWN, config), ERR(NULL, "Applying libnetconf2-netconf-server configuration failed."), cleanup); - /* CONFIG LOCK */ + /* start listening on new endpoints */ + NC_CHECK_ERR_GOTO(ret = nc_server_binds_reconcile(config), + ERR(NULL, "Starting to listen on new endpoints failed."), cleanup); + +#ifdef NC_ENABLED_SSH_TLS + /* dispatch new call-home connections */ + NC_CHECK_ERR_GOTO(ret = nc_server_config_reconcile_chclients_dispatch(config), + ERR(NULL, "Dispatching new call-home connections failed."), cleanup); +#endif /* NC_ENABLED_SSH_TLS */ + + /* CONFIG WR LOCK - only the pointer swap */ if (nc_rwlock_lock(&server_opts.config_lock, NC_RWLOCK_WRITE, NC_CONFIG_APPLY_LOCK_TIMEOUT, __func__) != 1) { ERR(NULL, "Timed out waiting for the configuration lock, the new configuration was not applied."); ret = 1; - goto cleanup; + goto rollback; } - /* start listening on new endpoints */ - NC_CHECK_ERR_GOTO(ret = nc_server_binds_reconcile(&config), - ERR(NULL, "Starting to listen on new endpoints failed."), cleanup_unlock); + /* publish the new generation, the reference is transferred to server_opts.config */ + old_config = nc_server_config_publish(config); + config = NULL; -#ifdef NC_ENABLED_SSH_TLS - /* dispatch new call-home connections */ - NC_CHECK_ERR_GOTO(ret = nc_server_config_reconcile_chclients_dispatch(&server_opts.config, &config), - ERR(NULL, "Dispatching new call-home connections failed."), cleanup_unlock); -#endif /* NC_ENABLED_SSH_TLS */ + /* CONFIG UNLOCK */ + nc_rwlock_unlock(&server_opts.config_lock, __func__); - /* swap: free old, keep new, zero out the copy just in case to avoid double free */ - nc_server_config_free(&server_opts.config); - server_opts.config = config; - memset(&config, 0, sizeof config); + /* the old generation is freed once its last reader releases it */ + nc_server_config_release(old_config); #ifdef NC_ENABLED_SSH_TLS /* wake up the cert expiration notif thread */ nc_server_config_cert_exp_notif_thread_wakeup(); #endif /* NC_ENABLED_SSH_TLS */ -cleanup_unlock: - /* CONFIG UNLOCK */ - nc_rwlock_unlock(&server_opts.config_lock, __func__); + goto cleanup; -cleanup: - if (ret) { - /* free the new config in case of error */ - nc_server_config_free(&config); +rollback: + /* the sockets and the Call Home threads were already reconciled with the new generation, + * reconcile them back with the one that stays published */ + cur_config = nc_server_config_acquire(); + if (cur_config) { + nc_server_binds_reconcile(cur_config); +#ifdef NC_ENABLED_SSH_TLS + nc_server_config_reconcile_chclients_dispatch(cur_config); +#endif /* NC_ENABLED_SSH_TLS */ + nc_server_config_release(cur_config); } +cleanup: + /* release the new generation, it was either not published or it is NULL */ + nc_server_config_release(config); + /* CONFIG UPDATE UNLOCK */ nc_mutex_unlock(&server_opts.config_update_lock, __func__); return ret; @@ -6611,27 +6732,28 @@ nc_server_config_oper_get_user_password_last_modified(const char *ch_client, con const char *username, time_t *last_modified) { int rc = 0; - LY_ARRAY_COUNT_TYPE i = 0; + LY_ARRAY_COUNT_TYPE i = 0, u; + const struct nc_server_config *config; struct nc_server_ssh_opts *ssh_opts = NULL; - struct nc_endpt *endpt = NULL; - struct nc_ch_client *client = NULL; - struct nc_ch_endpt *ch_endpt = NULL; + const struct nc_endpt *endpt = NULL; + const struct nc_ch_client *client = NULL; + const struct nc_ch_endpt *ch_endpt = NULL; time_t found_time = 0; NC_CHECK_ARG_RET(NULL, endpoint, username, last_modified, 1); *last_modified = 0; - /* LOCK */ - if (nc_rwlock_lock(&server_opts.config_lock, NC_RWLOCK_READ, NC_CONFIG_LOCK_TIMEOUT, __func__) != 1) { + config = nc_server_config_acquire(); + if (!config) { return 1; } if (ch_client) { /* find the call-home client */ - LY_ARRAY_FOR(server_opts.config.ch_clients, i) { - if (!strcmp(server_opts.config.ch_clients[i].name, ch_client)) { - client = &server_opts.config.ch_clients[i]; + LY_ARRAY_FOR(config->ch_clients, u) { + if (!strcmp(config->ch_clients[u].name, ch_client)) { + client = &config->ch_clients[u]; break; } } @@ -6642,7 +6764,8 @@ nc_server_config_oper_get_user_password_last_modified(const char *ch_client, con } /* find the endpoint */ - LY_ARRAY_FOR(client->ch_endpts, struct nc_ch_endpt, ch_endpt) { + LY_ARRAY_FOR(client->ch_endpts, u) { + ch_endpt = &client->ch_endpts[u]; if (!strcmp(ch_endpt->name, endpoint) && (ch_endpt->ti == NC_TI_SSH)) { ssh_opts = ch_endpt->opts.ssh; break; @@ -6656,7 +6779,8 @@ nc_server_config_oper_get_user_password_last_modified(const char *ch_client, con } } else { /* no call-home client specified, search in listening endpoints */ - LY_ARRAY_FOR(server_opts.config.endpts, struct nc_endpt, endpt) { + LY_ARRAY_FOR(config->endpts, u) { + endpt = &config->endpts[u]; if (!strcmp(endpt->name, endpoint) && (endpt->ti == NC_TI_SSH)) { ssh_opts = endpt->opts.ssh; break; @@ -6686,8 +6810,7 @@ nc_server_config_oper_get_user_password_last_modified(const char *ch_client, con *last_modified = found_time; cleanup: - /* UNLOCK */ - nc_rwlock_unlock(&server_opts.config_lock, __func__); + nc_server_config_release(config); return rc; } diff --git a/src/session.c b/src/session.c index 4e5e13ef..e8a92181 100644 --- a/src/session.c +++ b/src/session.c @@ -1338,11 +1338,10 @@ nc_str_append(char **str, uint32_t *used, uint32_t *size, const char *app_format * * @param[in] ctx libyang context. * @param[in] version YANG version of the schemas to be included in result. - * @param[in] config_locked Whether the configuration lock is already held or should be acquired. * @return Array of capabilities terminated with NULL, NULL on error. */ static char ** -_nc_server_get_cpblts_version(const struct ly_ctx *ctx, LYS_VERSION version, int config_locked) +_nc_server_get_cpblts_version(const struct ly_ctx *ctx, LYS_VERSION version) { char **cpblts; const struct lys_module *mod; @@ -1351,9 +1350,13 @@ _nc_server_get_cpblts_version(const struct ly_ctx *ctx, LYS_VERSION version, int char *yl_content_id = NULL; uint32_t wd_also_supported, wd_basic_mode; char *str = NULL; + const struct nc_server_config *config; NC_CHECK_ARG_RET(NULL, ctx, NULL); + /* pin the configuration, only the ignored module names are needed from it */ + config = nc_server_config_acquire(); + cpblts = malloc(3 * sizeof *cpblts); NC_CHECK_ERRMEM_GOTO(!cpblts, , error); cpblts[0] = strdup("urn:ietf:params:netconf:base:1.0"); @@ -1456,7 +1459,7 @@ _nc_server_get_cpblts_version(const struct ly_ctx *ctx, LYS_VERSION version, int /* models */ i = 0; while ((mod = ly_ctx_get_module_iter(ctx, &i))) { - if (nc_server_is_mod_ignored(mod->name, config_locked)) { + if (nc_server_is_mod_ignored(config, mod->name)) { /* ignored, not part of the cababilities */ continue; } @@ -1530,6 +1533,7 @@ _nc_server_get_cpblts_version(const struct ly_ctx *ctx, LYS_VERSION version, int /* HELLO UNLOCK */ nc_rwlock_unlock(&server_opts.hello_lock, __func__); + nc_server_config_release(config); free(str); return cpblts; @@ -1538,6 +1542,7 @@ _nc_server_get_cpblts_version(const struct ly_ctx *ctx, LYS_VERSION version, int nc_rwlock_unlock(&server_opts.hello_lock, __func__); error: + nc_server_config_release(config); if (cpblts) { for (i = 0; cpblts[i]; ++i) { free(cpblts[i]); @@ -1552,13 +1557,13 @@ _nc_server_get_cpblts_version(const struct ly_ctx *ctx, LYS_VERSION version, int API char ** nc_server_get_cpblts_version(const struct ly_ctx *ctx, LYS_VERSION version) { - return _nc_server_get_cpblts_version(ctx, version, 0); + return _nc_server_get_cpblts_version(ctx, version); } API char ** nc_server_get_cpblts(const struct ly_ctx *ctx) { - return _nc_server_get_cpblts_version(ctx, LYS_VERSION_UNDEF, 0); + return _nc_server_get_cpblts_version(ctx, LYS_VERSION_UNDEF); } /** @@ -1664,16 +1669,16 @@ nc_client_get_cpblts(void) * @brief Send NETCONF hello message on a session. * * @param[in] session Session to send the message on. - * @param[in] config_locked Whether the configuration READ lock is already held (only relevant for server side). * @return Sent message type. */ static NC_MSG_TYPE -nc_send_hello_io(struct nc_session *session, int config_locked) +nc_send_hello_io(struct nc_session *session) { NC_MSG_TYPE ret; int i, timeout_io; char **cpblts; uint32_t *sid; + uint16_t idle_timeout; if (session->side == NC_CLIENT) { /* client side hello - send only NETCONF base capabilities */ @@ -1685,7 +1690,7 @@ nc_send_hello_io(struct nc_session *session, int config_locked) timeout_io = NC_CLIENT_HELLO_TIMEOUT * 1000; sid = NULL; } else { - cpblts = _nc_server_get_cpblts_version(session->ctx, LYS_VERSION_1_0, config_locked); + cpblts = _nc_server_get_cpblts_version(session->ctx, LYS_VERSION_1_0); if (!cpblts) { return NC_MSG_ERROR; } @@ -1693,7 +1698,8 @@ nc_send_hello_io(struct nc_session *session, int config_locked) if (session->flags & NC_SESSION_CALLHOME) { timeout_io = NC_SERVER_CH_HELLO_TIMEOUT * 1000; } else { - timeout_io = server_opts.config.idle_timeout ? server_opts.config.idle_timeout * 1000 : -1; + idle_timeout = (uint16_t)ATOMIC_LOAD_RELAXED(server_opts.idle_timeout); + timeout_io = idle_timeout ? idle_timeout * 1000 : -1; } sid = &session->id; } @@ -1811,11 +1817,13 @@ nc_server_recv_hello_io(struct nc_session *session) struct lyd_node_opaq *node; NC_MSG_TYPE rc = NC_MSG_HELLO; int r, ver = -1, flag = 0, timeout_io; + uint16_t idle_timeout; if (session->flags & NC_SESSION_CALLHOME) { timeout_io = NC_SERVER_CH_HELLO_TIMEOUT * 1000; } else { - timeout_io = server_opts.config.idle_timeout ? server_opts.config.idle_timeout * 1000 : -1; + idle_timeout = (uint16_t)ATOMIC_LOAD_RELAXED(server_opts.idle_timeout); + timeout_io = idle_timeout ? idle_timeout * 1000 : -1; } r = nc_read_msg_poll_io(session, timeout_io, &msg); @@ -1875,7 +1883,7 @@ nc_handshake_io(struct nc_session *session) { NC_MSG_TYPE type; - type = nc_send_hello_io(session, 0); + type = nc_send_hello_io(session); if (type != NC_MSG_HELLO) { return type; } @@ -1899,7 +1907,7 @@ nc_ch_handshake_io(struct nc_session *session) return NC_MSG_ERROR; } - type = nc_send_hello_io(session, 1); + type = nc_send_hello_io(session); if (type != NC_MSG_HELLO) { return type; } diff --git a/src/session_openssl.c b/src/session_openssl.c index 8a7c8650..e1808914 100644 --- a/src/session_openssl.c +++ b/src/session_openssl.c @@ -498,7 +498,7 @@ nc_server_tls_verify_cb(int preverify_ok, X509_STORE_CTX *x509_ctx) * if yes, this callback will be called again with the same cert, but with preverify_ok = 1 */ cert = X509_STORE_CTX_get0_cert(x509_ctx); - ret = nc_server_tls_verify_peer_cert(cert, data->opts); + ret = nc_server_tls_verify_peer_cert(cert, data); if (ret) { VRB(NULL, "Cert verify: fail (%s).", X509_verify_cert_error_string(X509_STORE_CTX_get_error(x509_ctx))); ret = -1; diff --git a/src/session_p.h b/src/session_p.h index a470ee2e..b09d0eaf 100644 --- a/src/session_p.h +++ b/src/session_p.h @@ -154,17 +154,16 @@ extern struct nc_server_opts server_opts; /** * @brief Timeout in msec for acquiring the config_lock - * (socket binding and Call Home client dispatching can involve network operations) + * (only a pointer read plus a refcount increment) */ #define NC_CONFIG_LOCK_TIMEOUT 10000 /** * @brief Timeout in msec for the locks acquired while applying a new configuration. * - * A reader may hold the config_lock for the whole duration of a transport handshake (TCP connect, - * SSH/TLS key exchange and authentication), which is far longer than ::NC_CONFIG_LOCK_TIMEOUT. - * Giving up here means losing the configuration change, which the caller generally cannot recover - * from, so wait much longer than any handshake can take. + * The config_lock is only ever held for the pointer swap now, so this can never fire. It is kept as + * a safety net because giving up here means losing the configuration change, which the caller + * generally cannot recover from. */ #define NC_CONFIG_APPLY_LOCK_TIMEOUT 300000 @@ -714,7 +713,26 @@ struct nc_server_ch_thread_arg { int notify_pipe[2]; /**< Self-pipe for signaling the thread to terminate. Index 0 = read end, 1 = write end. */ }; +/** + * @brief Refcounted immutable snapshot of the server configuration. + * + * Once published in ::nc_server_opts.config, a generation is never written to again. Runtime state + * that used to live here (listening sockets, Call Home thread handles) is kept in registries in + * ::nc_server_opts instead. + * + * Reference ownership rules: + * - a generation is allocated by an applier with @p refcount 1, that reference is transferred to + * ::nc_server_opts.config when the generation is published, + * - an applier that fails before publishing releases its reference itself, + * - an applier that published a new generation releases the reference of the old one, + * - ::nc_server_config_acquire() takes a reference, ::nc_server_config_release() drops one and frees + * the generation when the last one is dropped, + * - so ::nc_server_opts.config always holds exactly one reference and @p refcount is at least 1 for + * as long as a generation is published. + */ struct nc_server_config { + ATOMIC_T refcount; /**< Number of references held to this configuration generation. */ + uint16_t idle_timeout; /**< Idle timeout of the server sessions. */ char **ignored_modules; /**< Names of YANG modules that are not reported in the server message (sized-array, see libyang docs). */ @@ -768,8 +786,6 @@ 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 @@ -801,10 +817,25 @@ 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 - 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. */ - struct nc_server_config config; /**< YANG Server configuration. */ + /* ACCESS locked - the published configuration pointer is swapped under the WRITE lock, + * - a reference to it is acquired under the READ lock */ + pthread_rwlock_t config_lock; /**< Lock for the ::nc_server_opts.config pointer. */ + struct nc_server_config *config; /**< Currently published YANG server configuration generation, + NULL until ::nc_server_init(). */ + + /* ACCESS unlocked - mirror of the published config->idle_timeout, stored under the config WRITE lock */ + ATOMIC_T idle_timeout; /**< Idle timeout of the server sessions in seconds, 0 for none. */ + + /* ACCESS locked - CH threads lock - leaf lock, never acquire another lock while holding it */ + pthread_mutex_t ch_threads_lock; /**< Lock for the Call Home thread registry. */ + + /** + * @brief Call Home thread registry, keyed by the client name of the thread argument. + * + * A thread handle is runtime state, not configuration, so it is kept out of + * ::nc_server_config (sized-array, see libyang docs). + */ + struct nc_server_ch_thread_arg **ch_threads; /* ACCESS locked - binds lock - leaf lock, never acquire another lock while holding it */ pthread_mutex_t binds_lock; /**< Lock for the listening socket registry. */ @@ -1010,6 +1041,16 @@ struct nc_session { pthread_mutex_t ch_lock; /**< Call Home thread lock */ pthread_cond_t ch_cond; /**< Call Home thread condition */ + /** + * @brief Configuration generation pinned for the duration of the transport handshake. + * + * A borrowed pointer, NOT a counted reference - the reference belongs to the function + * that acquired it, which also clears this field before the session leaves the handshake. + * ::nc_session_free() must not release it. It is only ever set by ::nc_accept() and + * ::nc_connect_ch_endpt(); do not add a third setter with different ownership. + */ + const struct nc_server_config *config; + #ifdef NC_ENABLED_SSH_TLS uint16_t ssh_auth_attempts; /**< number of failed SSH authentication attempts */ void *client_cert; /**< TLS client certificate if used for authentication */ @@ -1152,11 +1193,24 @@ int nc_server_binds_reconcile(const struct nc_server_config *config); void nc_server_binds_destroy(void); /** - * @brief Free server configuration data (only YANG config data). + * @brief Acquire a reference to the currently published server configuration generation. + * + * The returned generation is guaranteed to stay valid and unchanged until the reference is dropped + * by ::nc_server_config_release(), no lock needs to be held meanwhile. + * + * @return Pinned server configuration. + * @return NULL if the server is not initialized or the configuration lock could not be acquired. + */ +const struct nc_server_config *nc_server_config_acquire(void); + +/** + * @brief Release a reference to a server configuration generation. + * + * Frees the generation if this was the last reference held to it. * - * @param[in] config Server configuration to free. + * @param[in] config Server configuration to release, may be NULL. */ -void nc_server_config_free(struct nc_server_config *config); +void nc_server_config_release(const struct nc_server_config *config); /** * @brief Get passwd entry for UID or a user. @@ -1398,13 +1452,14 @@ int nc_server_ch_accept_binds(const struct nc_bind *binds, const struct nc_clien int nc_connect_unix_session(struct nc_session *session, int sock, const char *username); /** - * @brief Gets a listening endpoint based on its name. + * @brief Gets a listening endpoint of a pinned configuration based on its name. * + * @param[in] config Pinned server configuration to search. * @param[in] name The name of the endpoint. * @param[out] endpt Pointer to the endpoint structure. * @return 0 on success, 1 on failure. */ -int nc_server_endpt_get(const char *name, struct nc_endpt **endpt); +int nc_server_endpt_get(const struct nc_server_config *config, const char *name, struct nc_endpt **endpt); /** * @brief Add a client Call Home bind, listen on it. @@ -1441,22 +1496,43 @@ NC_MSG_TYPE nc_connect_callhome(const char *host, uint16_t port, NC_TRANSPORT_IM #ifdef NC_ENABLED_SSH_TLS +/** + * @brief Get the names of all the Call Home clients that have a thread running. + * + * @param[out] names Copies of the client names (sized-array, see libyang docs), free with + * ::nc_server_ch_thread_names_free(). + * @return 0 on success, 1 on error. + */ +int nc_server_ch_thread_names_get(char ***names); + +/** + * @brief Free the Call Home client names returned by ::nc_server_ch_thread_names_get(). + * + * @param[in] names Client names to free, may be NULL. + */ +void nc_server_ch_thread_names_free(char **names); + +/** + * @brief Stop all the Call Home client threads and free the thread registry. + * + * @return 0 on success, 1 on error. + */ +int nc_server_ch_threads_destroy(void); + /** * @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. + * Takes no configuration lock at all, the thread is looked up in the Call Home thread registry. * - * @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. + * @param[in] client_name Name of the Call Home client to stop the thread for. + * @return 0 if the thread was successfully stopped or none was running, 1 on error. */ -int nc_session_server_ch_client_dispatch_stop(struct nc_ch_client *ch_client); +int nc_session_server_ch_client_dispatch_stop(const char *client_name); /** * @brief Dispatch a thread connecting to a listening NETCONF client and creating Call Home sessions. * - * @note The config WRITE lock MUST be held. - * - * @param[in] ch_client Call Home client to dispatch the thread for. + * @param[in] client_name Name of the Call Home client to dispatch the thread for. * @param[in] acquire_ctx_cb Callback for acquiring new session context. * @param[in] release_ctx_cb Callback for releasing session context. * @param[in] ctx_cb_data Arbitrary user data passed to @p acquire_ctx_cb and @p release_ctx_cb. @@ -1464,7 +1540,7 @@ int nc_session_server_ch_client_dispatch_stop(struct nc_ch_client *ch_client); * @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(struct nc_ch_client *ch_client, nc_server_ch_session_acquire_ctx_cb acquire_ctx_cb, +int _nc_connect_ch_client_dispatch(const char *client_name, 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); @@ -1526,11 +1602,11 @@ int nc_session_tls_crl_verify_post_handshake(void *tls_session, void *cert_store /** * @brief Check whether a module is not ignored by the server. * + * @param[in] config Pinned server configuration, may be NULL. * @param[in] mod_name Module name to check. - * @param[in] config_locked Whether the configuration lock is already held or should be acquired in this function. * @return Whether the module is ignored. */ -int nc_server_is_mod_ignored(const char *mod_name, int config_locked); +int nc_server_is_mod_ignored(const struct nc_server_config *config, const char *mod_name); /** * Functions diff --git a/src/session_server.c b/src/session_server.c index 9590a940..260adbd2 100644 --- a/src/session_server.c +++ b/src/session_server.c @@ -59,6 +59,7 @@ struct nc_server_opts server_opts = { .config_update_lock = PTHREAD_MUTEX_INITIALIZER, .binds_lock = PTHREAD_MUTEX_INITIALIZER, .opts_lock = PTHREAD_RWLOCK_INITIALIZER, + .ch_threads_lock = PTHREAD_MUTEX_INITIALIZER, }; static nc_rpc_clb global_rpc_clb = NULL; @@ -66,23 +67,130 @@ static nc_rpc_clb global_rpc_clb = NULL; #ifdef NC_ENABLED_SSH_TLS /** - * @brief Get a CH client with the given @p name . + * @brief Add a Call Home thread argument to the thread registry. * - * @note The configuration read lock must be held. + * @param[in] thread_arg Thread argument to register. + * @return 0 on success, 1 on error. + */ +static int +nc_server_ch_thread_reg_add(struct nc_server_ch_thread_arg *thread_arg) +{ + int rc = 0; + struct nc_server_ch_thread_arg **item; + + /* CH THREADS LOCK */ + if (nc_mutex_lock(&server_opts.ch_threads_lock, NC_CH_THREADS_LOCK_TIMEOUT, __func__) != 1) { + return 1; + } + + LY_ARRAY_NEW_GOTO(NULL, server_opts.ch_threads, item, rc, cleanup); + *item = thread_arg; + +cleanup: + /* CH THREADS UNLOCK */ + nc_mutex_unlock(&server_opts.ch_threads_lock, __func__); + return rc ? 1 : 0; +} + +/** + * @brief Remove a Call Home thread argument from the thread registry. * + * @param[in] client_name Name of the Call Home client to unregister the thread of. + * @param[out] thread_arg Unregistered thread argument, NULL if the client had no thread registered. + * @return 0 on success, 1 on error. + */ +static int +nc_server_ch_thread_reg_del(const char *client_name, struct nc_server_ch_thread_arg **thread_arg) +{ + LY_ARRAY_COUNT_TYPE u; + + *thread_arg = NULL; + + /* CH THREADS LOCK */ + if (nc_mutex_lock(&server_opts.ch_threads_lock, NC_CH_THREADS_LOCK_TIMEOUT, __func__) != 1) { + return 1; + } + + LY_ARRAY_FOR(server_opts.ch_threads, u) { + if (strcmp(server_opts.ch_threads[u]->client_name, client_name)) { + continue; + } + + *thread_arg = server_opts.ch_threads[u]; + + /* swap the last entry into the hole, the order of the registry is irrelevant */ + server_opts.ch_threads[u] = server_opts.ch_threads[LY_ARRAY_COUNT(server_opts.ch_threads) - 1]; + LY_ARRAY_DECREMENT_FREE(server_opts.ch_threads); + break; + } + + /* CH THREADS UNLOCK */ + nc_mutex_unlock(&server_opts.ch_threads_lock, __func__); + return 0; +} + +void +nc_server_ch_thread_names_free(char **names) +{ + LY_ARRAY_COUNT_TYPE u; + + LY_ARRAY_FOR(names, u) { + free(names[u]); + } + LY_ARRAY_FREE(names); +} + +int +nc_server_ch_thread_names_get(char ***names) +{ + int rc = 0; + LY_ARRAY_COUNT_TYPE u; + char *name; + + *names = NULL; + + /* CH THREADS LOCK */ + if (nc_mutex_lock(&server_opts.ch_threads_lock, NC_CH_THREADS_LOCK_TIMEOUT, __func__) != 1) { + return 1; + } + + if (LY_ARRAY_COUNT(server_opts.ch_threads)) { + LY_ARRAY_CREATE_GOTO(NULL, *names, LY_ARRAY_COUNT(server_opts.ch_threads), rc, cleanup); + LY_ARRAY_FOR(server_opts.ch_threads, u) { + name = strdup(server_opts.ch_threads[u]->client_name); + NC_CHECK_ERRMEM_GOTO(!name, rc = 1, cleanup); + (*names)[u] = name; + LY_ARRAY_INCREMENT(*names); + } + } + +cleanup: + /* CH THREADS UNLOCK */ + nc_mutex_unlock(&server_opts.ch_threads_lock, __func__); + if (rc) { + nc_server_ch_thread_names_free(*names); + *names = NULL; + } + return rc ? 1 : 0; +} + +/** + * @brief Get a CH client with the given @p name from a pinned configuration. + * + * @param[in] config Pinned server configuration to search. * @param[in] name Name of the CH client to find. * @return CH client, NULL if not found. */ -static struct nc_ch_client * -nc_server_ch_client_get(const char *name) +static const struct nc_ch_client * +nc_server_ch_client_get_pinned(const struct nc_server_config *config, const char *name) { - struct nc_ch_client *client = NULL; + LY_ARRAY_COUNT_TYPE u; assert(name); - LY_ARRAY_FOR(server_opts.config.ch_clients, struct nc_ch_client, client) { - if (client->name && !strcmp(client->name, name)) { - return client; + LY_ARRAY_FOR(config->ch_clients, u) { + if (!strcmp(config->ch_clients[u].name, name)) { + return &config->ch_clients[u]; } } @@ -92,15 +200,19 @@ nc_server_ch_client_get(const char *name) #endif /* NC_ENABLED_SSH_TLS */ int -nc_server_endpt_get(const char *name, struct nc_endpt **endpt) +nc_server_endpt_get(const struct nc_server_config *config, const char *name, struct nc_endpt **endpt) { - struct nc_endpt *ep; + LY_ARRAY_COUNT_TYPE u; *endpt = NULL; - LY_ARRAY_FOR(server_opts.config.endpts, struct nc_endpt, ep) { - if (ep->name && !strcmp(ep->name, name)) { - *endpt = ep; + if (!config) { + return 1; + } + + LY_ARRAY_FOR(config->endpts, u) { + if (config->endpts[u].name && !strcmp(config->endpts[u].name, name)) { + *endpt = (struct nc_endpt *)&config->endpts[u]; return 0; } } @@ -1340,6 +1452,15 @@ nc_server_init(void) goto error; } + /* allocate the initial empty configuration generation, its reference belongs to server_opts.config */ + server_opts.config = calloc(1, sizeof *server_opts.config); + if (!server_opts.config) { + ERRMEM; + goto error; + } + ATOMIC_STORE_RELAXED(server_opts.config->refcount, 1); + ATOMIC_STORE_RELAXED(server_opts.idle_timeout, 0); + #ifdef NC_ENABLED_SSH_TLS if (curl_global_init(CURL_GLOBAL_SSL | CURL_GLOBAL_ACK_EINTR)) { ERR(NULL, "%s: failed to init CURL.", __func__); @@ -1381,7 +1502,7 @@ nc_server_destroy(void) { int rc = 0; int config_update_locked = 0, opts_locked = 0; - enum nc_rwlock_mode config_lock_mode = NC_RWLOCK_NONE; + struct nc_server_config *config; uint32_t i; #ifdef NC_ENABLED_SSH_TLS @@ -1418,28 +1539,16 @@ nc_server_destroy(void) } config_update_locked = 1; - /* 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 - /* 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; - } + /* stop all dispatched CH threads, no configuration lock may be held while joining them */ + if ((rc = nc_server_ch_threads_destroy())) { + goto cleanup; } #endif /* NC_ENABLED_SSH_TLS */ /* stop listening on all the registered sockets */ nc_server_binds_destroy(); - /* destroy the server configuration */ - nc_server_config_free(&server_opts.config); - /* OPTS WRITE LOCK */ if (nc_rwlock_lock(&server_opts.opts_lock, NC_RWLOCK_WRITE, NC_OPTS_LOCK_TIMEOUT, __func__) != 1) { rc = 1; @@ -1484,7 +1593,24 @@ nc_server_destroy(void) if (interactive_auth_data && interactive_auth_data_free) { interactive_auth_data_free(interactive_auth_data); } +#endif /* NC_ENABLED_SSH_TLS */ + /* CONFIG WR LOCK - unpublish the configuration, a concurrent acquire must not see a stale pointer */ + if (nc_rwlock_lock(&server_opts.config_lock, NC_RWLOCK_WRITE, NC_CONFIG_LOCK_TIMEOUT, __func__) != 1) { + rc = 1; + goto cleanup; + } + config = server_opts.config; + server_opts.config = NULL; + ATOMIC_STORE_RELAXED(server_opts.idle_timeout, 0); + + /* CONFIG UNLOCK */ + nc_rwlock_unlock(&server_opts.config_lock, __func__); + + /* the configuration is destroyed once its last reader releases it */ + nc_server_config_release(config); + +#ifdef NC_ENABLED_SSH_TLS curl_global_cleanup(); nc_tls_backend_destroy_wrap(); ssh_finalize(); @@ -1500,9 +1626,6 @@ nc_server_destroy(void) if (opts_locked) { nc_rwlock_unlock(&server_opts.opts_lock, __func__); } - 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__); } @@ -2448,8 +2571,8 @@ nc_ps_poll_session_io(struct nc_session *session, int io_timeout, time_t now_mon #endif #endif /* NC_ENABLED_SSH_TLS */ - /* check timeout first */ - idle_timeout = server_opts.config.idle_timeout; + /* check timeout first, read the mirror so that the poll path needs no configuration at all */ + idle_timeout = (uint16_t)ATOMIC_LOAD_RELAXED(server_opts.idle_timeout); if (!(session->flags & NC_SESSION_CALLHOME) && !nc_session_get_notif_status(session) && idle_timeout && (now_mono >= session->opts.server.last_rpc + idle_timeout)) { sprintf(msg, "Session idle timeout elapsed"); @@ -2615,19 +2738,9 @@ nc_ps_poll_sess(struct nc_ps_session *ps_session, time_t now_mono) switch (ps_session->state) { case NC_PS_STATE_NONE: if (ps_session->session->status == NC_STATUS_RUNNING) { - /* session is fine, work with it */ + /* session is fine, work with it, no configuration is accessed */ ps_session->state = NC_PS_STATE_BUSY; - - /* CONFIG READ LOCK */ - if (nc_rwlock_lock(&server_opts.config_lock, NC_RWLOCK_READ, NC_CONFIG_LOCK_TIMEOUT, __func__) != 1) { - ps_session->state = NC_PS_STATE_NONE; - ret = NC_PSPOLL_ERROR; - break; - } else { - ret = nc_ps_poll_session_io(ps_session->session, NC_SESSION_LOCK_TIMEOUT, now_mono, msg); - /* CONFIG UNLOCK */ - nc_rwlock_unlock(&server_opts.config_lock, __func__); - } + ret = nc_ps_poll_session_io(ps_session->session, NC_SESSION_LOCK_TIMEOUT, now_mono, msg); switch (ret) { case NC_PSPOLL_SESSION_TERM | NC_PSPOLL_SESSION_ERROR: @@ -3382,18 +3495,17 @@ nc_accept_unix_session(struct nc_session *session, int sock) API uint32_t nc_server_endpt_count(void) { + const struct nc_server_config *config; uint32_t cnt; - /* CONFIG READ LOCK */ - if (nc_rwlock_lock(&server_opts.config_lock, NC_RWLOCK_READ, NC_CONFIG_LOCK_TIMEOUT, __func__) != 1) { + config = nc_server_config_acquire(); + if (!config) { return 0; } - cnt = LY_ARRAY_COUNT(server_opts.config.endpts); - - /* CONFIG UNLOCK */ - nc_rwlock_unlock(&server_opts.config_lock, __func__); + cnt = LY_ARRAY_COUNT(config->endpts); + nc_server_config_release(config); return cnt; } @@ -3406,7 +3518,7 @@ nc_accept(int timeout, const struct ly_ctx *ctx, struct nc_session **session) uint16_t port = 0; struct timespec ts_cur; LY_ARRAY_COUNT_TYPE endpt_idx; - struct nc_server_config *config; + const struct nc_server_config *config; NC_CHECK_ARG_RET(NULL, ctx, session, NC_MSG_ERROR); @@ -3417,13 +3529,12 @@ nc_accept(int timeout, const struct ly_ctx *ctx, struct nc_session **session) /* init ctx as needed */ nc_server_init_cb_ctx(ctx); - /* CONFIG LOCK */ - if (nc_rwlock_lock(&server_opts.config_lock, NC_RWLOCK_READ, NC_CONFIG_LOCK_TIMEOUT, __func__) != 1) { + /* pin the configuration for the whole accept, no lock is held for any of it */ + config = nc_server_config_acquire(); + if (!config) { return NC_MSG_ERROR; } - config = &server_opts.config; - if (!config->endpts) { ERR(NULL, "No endpoints to accept sessions on."); msgtype = NC_MSG_ERROR; @@ -3442,7 +3553,7 @@ nc_accept(int timeout, const struct ly_ctx *ctx, struct nc_session **session) } /* configure keepalives */ - if (nc_sock_configure_ka(sock, &config->endpts[endpt_idx].ka)) { + if (nc_sock_configure_ka(sock, (struct nc_keepalives *)&config->endpts[endpt_idx].ka)) { msgtype = NC_MSG_ERROR; goto cleanup; } @@ -3456,6 +3567,9 @@ nc_accept(int timeout, const struct ly_ctx *ctx, struct nc_session **session) host = NULL; (*session)->port = port; + /* pin the configuration for the duration of the transport handshake, it is a borrowed pointer */ + (*session)->opts.server.config = config; + /* sock gets assigned to session or closed */ #ifdef NC_ENABLED_SSH_TLS if (config->endpts[endpt_idx].ti == NC_TI_SSH) { @@ -3497,8 +3611,12 @@ nc_accept(int timeout, const struct ly_ctx *ctx, struct nc_session **session) (*session)->data = NULL; - /* CONFIG UNLOCK */ - nc_rwlock_unlock(&server_opts.config_lock, __func__); + /* the transport handshake is over, the configuration must not be reached through the session anymore */ + (*session)->opts.server.config = NULL; + + /* the NETCONF hello needs no configuration */ + nc_server_config_release(config); + config = NULL; /* assign new SID atomically */ (*session)->id = ATOMIC_INC_RELAXED(server_opts.new_session_id); @@ -3520,15 +3638,16 @@ nc_accept(int timeout, const struct ly_ctx *ctx, struct nc_session **session) return msgtype; cleanup: - /* CONFIG UNLOCK */ - nc_rwlock_unlock(&server_opts.config_lock, __func__); - free(host); if (sock > -1) { close(sock); } + if (*session) { + (*session)->opts.server.config = NULL; + } nc_session_free(*session, NULL); *session = NULL; + nc_server_config_release(config); return msgtype; } @@ -3537,71 +3656,66 @@ nc_accept(int timeout, const struct ly_ctx *ctx, struct nc_session **session) API int nc_server_ch_is_client(const char *name) { - struct nc_ch_client *client; + const struct nc_server_config *config; int found = 0; if (!name) { return found; } - /* CONFIG READ LOCK */ - if (nc_rwlock_lock(&server_opts.config_lock, NC_RWLOCK_READ, NC_CONFIG_LOCK_TIMEOUT, __func__) != 1) { + config = nc_server_config_acquire(); + if (!config) { return found; } /* check name against all configured clients */ - LY_ARRAY_FOR(server_opts.config.ch_clients, struct nc_ch_client, client) { - if (!strcmp(client->name, name)) { - found = 1; - break; - } + if (nc_server_ch_client_get_pinned(config, name)) { + found = 1; } - /* CONFIG READ UNLOCK */ - nc_rwlock_unlock(&server_opts.config_lock, __func__); - + nc_server_config_release(config); return found; } API int nc_server_ch_client_is_endpt(const char *client_name, const char *endpt_name) { - struct nc_ch_client *client = NULL; - struct nc_ch_endpt *endpt = NULL; + const struct nc_server_config *config; + const struct nc_ch_client *client; + LY_ARRAY_COUNT_TYPE u; int found = 0; if (!client_name || !endpt_name) { return found; } - /* CONFIG READ LOCK */ - if (nc_rwlock_lock(&server_opts.config_lock, NC_RWLOCK_READ, NC_CONFIG_LOCK_TIMEOUT, __func__) != 1) { + config = nc_server_config_acquire(); + if (!config) { return found; } - client = nc_server_ch_client_get(client_name); + client = nc_server_ch_client_get_pinned(config, client_name); if (!client) { goto cleanup; } - LY_ARRAY_FOR(client->ch_endpts, struct nc_ch_endpt, endpt) { - if (!strcmp(endpt->name, endpt_name)) { + LY_ARRAY_FOR(client->ch_endpts, u) { + if (!strcmp(client->ch_endpts[u].name, endpt_name)) { found = 1; goto cleanup; } } cleanup: - /* CONFIG READ UNLOCK */ - nc_rwlock_unlock(&server_opts.config_lock, __func__); + nc_server_config_release(config); return found; } /** * @brief Create a connection for an endpoint. * - * Config read lock must be held - the configuration is being read. - * + * @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,out] cur_sock_pending Current pending socket for the connection. * @param[in] acquire_ctx_cb Callback for acquiring the libyang context. @@ -3611,7 +3725,7 @@ 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(struct nc_ch_endpt *endpt, int *cur_sock_pending, +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) { @@ -3622,7 +3736,7 @@ nc_connect_ch_endpt(struct nc_ch_endpt *endpt, int *cur_sock_pending, char *ip_host = NULL; sock = nc_sock_connect(endpt->src_addr, endpt->src_port, endpt->dst_addr, endpt->dst_port, - NC_CH_CONNECT_TIMEOUT, &endpt->ka, cur_sock_pending, &ip_host); + NC_CH_CONNECT_TIMEOUT, (struct nc_keepalives *)&endpt->ka, cur_sock_pending, &ip_host); if (sock < 0) { return NC_MSG_ERROR; } @@ -3648,6 +3762,9 @@ nc_connect_ch_endpt(struct nc_ch_endpt *endpt, int *cur_sock_pending, (*session)->host = ip_host; (*session)->port = endpt->dst_port; + /* pin the configuration for the duration of the transport handshake, it is a borrowed pointer */ + (*session)->opts.server.config = config; + /* sock gets assigned to session or closed */ if (endpt->ti == NC_TI_SSH) { ret = nc_accept_ssh_session(*session, endpt->opts.ssh, sock); @@ -3679,6 +3796,9 @@ nc_connect_ch_endpt(struct nc_ch_endpt *endpt, int *cur_sock_pending, goto fail; } + /* the transport handshake is over, the configuration must not be reached through the session anymore */ + (*session)->opts.server.config = NULL; + /* assign new SID atomically */ (*session)->id = ATOMIC_INC_RELAXED(server_opts.new_session_id); @@ -3697,6 +3817,9 @@ nc_connect_ch_endpt(struct nc_ch_endpt *endpt, int *cur_sock_pending, return msgtype; fail: + if (*session) { + (*session)->opts.server.config = NULL; + } nc_session_free(*session, NULL); *session = NULL; if (ctx) { @@ -3708,37 +3831,33 @@ nc_connect_ch_endpt(struct nc_ch_endpt *endpt, int *cur_sock_pending, /** * @brief Get idle timeout for a Call Home client. * + * A client that is not (yet) part of the published configuration simply has no idle timeout, the + * lifetime of its thread is decided by ::nc_server_ch_thread_arg.thread_running only. + * * @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, -1 on error. + * @param[out] idle_timeout Idle timeout in seconds, 0 for none. + * @return 0 on success, -1 on error. */ static int nc_server_ch_client_get_idle_timeout(const char *client_name, uint32_t *idle_timeout) { - int ret = 0; - struct nc_ch_client *client; + const struct nc_server_config *config; + const struct nc_ch_client *client; - /* CONFIG READ LOCK */ - if (nc_rwlock_lock(&server_opts.config_lock, NC_RWLOCK_READ, NC_CONFIG_LOCK_TIMEOUT, __func__) != 1) { - return -1; - } + *idle_timeout = 0; - client = nc_server_ch_client_get(client_name); - if (!client) { - ret = 1; - goto cleanup; + config = nc_server_config_acquire(); + if (!config) { + return -1; } - if (client->conn_type == NC_CH_PERIOD) { + client = nc_server_ch_client_get_pinned(config, client_name); + if (client && (client->conn_type == NC_CH_PERIOD)) { *idle_timeout = client->idle_timeout; - } else { - *idle_timeout = 0; } -cleanup: - /* CONFIG READ UNLOCK */ - nc_rwlock_unlock(&server_opts.config_lock, __func__); - return ret; + nc_server_config_release(config); + return 0; } /** @@ -3811,14 +3930,8 @@ nc_server_ch_client_thread_session_cond_wait(struct nc_server_ch_thread_arg *dat terminate = 0; - /* 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) { - 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; - } + /* get the client's idle timeout */ + if (nc_server_ch_client_get_idle_timeout(data->client_name, &idle_timeout)) { rc = -1; terminate = 1; } @@ -3948,41 +4061,40 @@ nc_server_ch_client_thread_wait(struct nc_session *session, struct nc_server_ch_ } /** - * @brief Wait for a Call Home client to have at least one endpoint defined. + * @brief Acquire a configuration in which the Call Home client has at least one endpoint defined. * - * @note The configuration read lock is expected to be held. + * A client that is missing from the published configuration is waited for the same way as a client + * with no endpoints - it may simply not have been published yet. The thread only ever stops because + * ::nc_server_ch_thread_arg.thread_running was cleared. * * @param[in] data Call Home client thread argument. - * @param[in] name Name of the CH client. - * @return Pointer to the CH client, NULL if the client was removed. + * @param[out] client Found Call Home client of the returned configuration. + * @return Pinned server configuration, the caller must release it. + * @return NULL if the thread should stop running or the configuration could not be acquired. */ -static struct nc_ch_client * -nc_server_ch_client_with_endpt_get(struct nc_server_ch_thread_arg *data, const char *name) +static const struct nc_server_config * +nc_server_ch_client_acquire_with_endpt(struct nc_server_ch_thread_arg *data, const struct nc_ch_client **client) { - struct nc_ch_client *client; + const struct nc_server_config *config; + + *client = NULL; while (ATOMIC_LOAD_RELAXED(data->thread_running)) { - /* get the client */ - client = nc_server_ch_client_get(name); - if (!client) { + config = nc_server_config_acquire(); + if (!config) { return NULL; } - /* check if it has at least one endpoint defined */ - if (client->ch_endpts) { - return client; + *client = nc_server_ch_client_get_pinned(config, data->client_name); + if (*client && (*client)->ch_endpts) { + /* the client is configured and has at least one endpoint */ + return config; } - /* CONFIG READ UNLOCK - allow another thread to modify the configuration */ - nc_rwlock_unlock(&server_opts.config_lock, __func__); - - /* no endpoints defined yet, wait a little bit */ + /* not configured (yet) or no endpoints defined yet, wait a little bit */ + nc_server_config_release(config); + *client = NULL; usleep(NC_CH_NO_ENDPT_WAIT * 1000); - - /* CONFIG READ LOCK */ - if (nc_rwlock_lock(&server_opts.config_lock, NC_RWLOCK_READ, NC_CONFIG_LOCK_TIMEOUT, __func__) != 1) { - return NULL; - } } /* thread is not running */ @@ -4001,44 +4113,43 @@ nc_ch_client_thread(void *arg) struct nc_server_ch_thread_arg *data = arg; NC_MSG_TYPE msgtype; int cur_sock_pending = -1, r; - uint8_t cur_attempts = 0, max_attempts; - uint16_t next_endpt_index, max_wait; + uint8_t cur_attempts = 0, max_attempts = 0; + uint16_t next_endpt_index, max_wait = 0, period = 0; char *cur_endpt_name = NULL; - struct nc_ch_endpt *cur_endpt; + const struct nc_server_config *config = NULL; + const struct nc_ch_client *client; + const struct nc_ch_endpt *cur_endpt; struct nc_session *session = NULL; - struct nc_ch_client *client; uint32_t reconnect_in; + NC_CH_CONN_TYPE conn_type; + NC_CH_START_WITH start_with; + time_t anchor_time; - /* mark the thread as running */ - ATOMIC_STORE_RELAXED(data->thread_running, 1); - - /* CONFIG READ LOCK */ - if (nc_rwlock_lock(&server_opts.config_lock, NC_RWLOCK_READ, NC_CONFIG_LOCK_TIMEOUT, __func__) != 1) { + /* get the client once it is configured with at least one endpoint */ + config = nc_server_ch_client_acquire_with_endpt(data, &client); + if (!config) { goto cleanup; } - /* get the client once it has at least one endpoint */ - client = nc_server_ch_client_with_endpt_get(data, data->client_name); - if (!client) { - VRB(NULL, "Call Home client \"%s\" removed.", data->client_name); - goto cleanup_unlock; - } - - /* config is still locked and ch client has at least 1 endpoint, so select the first one */ + /* the client has at least 1 endpoint, so select the first one */ cur_endpt = &client->ch_endpts[0]; cur_endpt_name = strdup(cur_endpt->name); + NC_CHECK_ERRMEM_GOTO(!cur_endpt_name, , cleanup); while (ATOMIC_LOAD_RELAXED(data->thread_running)) { if (!cur_attempts) { VRB(NULL, "Call Home client \"%s\" endpoint \"%s\" connecting...", data->client_name, cur_endpt_name); } - /* try to connect to the endpoint */ - msgtype = nc_connect_ch_endpt(cur_endpt, &cur_sock_pending, data->acquire_ctx_cb, data->release_ctx_cb, - data->ctx_cb_data, &session); + /* 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); if (msgtype == NC_MSG_HELLO) { - /* CONFIG READ UNLOCK - session established */ - nc_rwlock_unlock(&server_opts.config_lock, __func__); + /* session established, the configuration is not needed anymore */ + nc_server_config_release(config); + config = NULL; + client = NULL; + cur_endpt = NULL; if (!ATOMIC_LOAD_RELAXED(data->thread_running)) { /* thread should stop running */ @@ -4058,56 +4169,50 @@ nc_ch_client_thread(void *arg) goto cleanup; } - /* CONFIG READ LOCK */ - if (nc_rwlock_lock(&server_opts.config_lock, NC_RWLOCK_READ, NC_CONFIG_LOCK_TIMEOUT, __func__) != 1) { + /* get the client again, it may have been changed */ + config = nc_server_ch_client_acquire_with_endpt(data, &client); + if (!config) { goto cleanup; } - /* get the client again, it may have been removed */ - client = nc_server_ch_client_with_endpt_get(data, data->client_name); - if (!client) { - VRB(NULL, "Call Home client \"%s\" removed.", data->client_name); - goto cleanup_unlock; - } - /* session changed status -> it was disconnected for whatever reason, * persistent connection immediately tries to reconnect, periodic connects at specific times */ - if (client->conn_type == NC_CH_PERIOD) { - if (client->anchor_time) { + conn_type = client->conn_type; + period = client->period; + anchor_time = client->anchor_time; + if (conn_type == NC_CH_PERIOD) { + if (anchor_time) { /* anchored */ - reconnect_in = (time(NULL) - client->anchor_time) % (client->period * 60); + reconnect_in = (time(NULL) - anchor_time) % (period * 60); } else { /* fixed timeout */ - reconnect_in = client->period * 60; + reconnect_in = period * 60; } - /* CONFIG READ UNLOCK */ - nc_rwlock_unlock(&server_opts.config_lock, __func__); + /* the configuration is not needed while waiting */ + nc_server_config_release(config); + config = NULL; + client = NULL; /* wait for the timeout to elapse, so we can try to reconnect */ - VRB(session, "Call Home client \"%s\" reconnecting in %" PRIu32 " seconds.", data->client_name, reconnect_in); - r = nc_server_ch_client_thread_wait(session, data, reconnect_in, NULL); + VRB(NULL, "Call Home client \"%s\" reconnecting in %" PRIu32 " seconds.", data->client_name, reconnect_in); + r = nc_server_ch_client_thread_wait(NULL, data, reconnect_in, NULL); if (r == -1) { goto cleanup; } - /* CONFIG READ LOCK */ - if (nc_rwlock_lock(&server_opts.config_lock, NC_RWLOCK_READ, NC_CONFIG_LOCK_TIMEOUT, __func__) != 1) { + config = nc_server_ch_client_acquire_with_endpt(data, &client); + if (!config) { goto cleanup; } - - client = nc_server_ch_client_with_endpt_get(data, data->client_name); - if (!client) { - VRB(NULL, "Call Home client \"%s\" removed.", data->client_name); - goto cleanup_unlock; - } } /* set next endpoint to try */ - if (client->start_with == NC_CH_FIRST_LISTED) { + start_with = client->start_with; + if (start_with == NC_CH_FIRST_LISTED) { next_endpt_index = 0; - } else if (client->start_with == NC_CH_LAST_CONNECTED) { - /* we keep the current one but due to unlock/lock we have to find it again */ + } else if (start_with == NC_CH_LAST_CONNECTED) { + /* we keep the current one but due to the release/acquire we have to find it again */ LY_ARRAY_FOR(client->ch_endpts, next_endpt_index) { if (!strcmp(client->ch_endpts[next_endpt_index].name, cur_endpt_name)) { break; @@ -4128,8 +4233,11 @@ nc_ch_client_thread(void *arg) max_wait = client->max_wait; max_attempts = client->max_attempts; - /* CONFIG READ UNLOCK */ - nc_rwlock_unlock(&server_opts.config_lock, __func__); + /* the configuration is not needed while waiting */ + nc_server_config_release(config); + config = NULL; + client = NULL; + cur_endpt = NULL; /* failed connection attempt */ if (data->new_session_fail_cb) { @@ -4138,7 +4246,7 @@ nc_ch_client_thread(void *arg) } /* wait for max_wait seconds */ - r = nc_server_ch_client_thread_wait(session, data, max_wait, &cur_sock_pending); + r = nc_server_ch_client_thread_wait(NULL, data, max_wait, &cur_sock_pending); if (r == -1) { /* thread should stop running */ goto cleanup; @@ -4151,16 +4259,10 @@ nc_ch_client_thread(void *arg) } /* if r == 1, socket is connected, keep cur_sock_pending for nc_connect_ch_endpt */ - /* CONFIG READ LOCK */ - if (nc_rwlock_lock(&server_opts.config_lock, NC_RWLOCK_READ, NC_CONFIG_LOCK_TIMEOUT, __func__) != 1) { - goto cleanup; - } - /* get the client */ - client = nc_server_ch_client_with_endpt_get(data, data->client_name); - if (!client) { - VRB(NULL, "Call Home client \"%s\" removed.", data->client_name); - goto cleanup_unlock; + config = nc_server_ch_client_acquire_with_endpt(data, &client); + if (!config) { + goto cleanup; } /* try to find our endpoint again */ @@ -4172,7 +4274,7 @@ nc_ch_client_thread(void *arg) if (next_endpt_index >= LY_ARRAY_COUNT(client->ch_endpts)) { /* endpoint was removed, start with the first one */ - VRB(session, "Call Home client \"%s\" endpoint \"%s\" removed.", data->client_name, cur_endpt_name); + VRB(NULL, "Call Home client \"%s\" endpoint \"%s\" removed.", data->client_name, cur_endpt_name); /* close pending socket to the removed endpoint, if any */ if (cur_sock_pending != -1) { @@ -4184,7 +4286,7 @@ nc_ch_client_thread(void *arg) cur_attempts = 0; } else if (cur_attempts == client->max_attempts) { /* we have tried to connect to this endpoint enough times */ - VRB(session, "Call Home client \"%s\" endpoint \"%s\" failed connection attempt limit %" PRIu8 " reached.", + VRB(NULL, "Call Home client \"%s\" endpoint \"%s\" failed connection attempt limit %" PRIu8 " reached.", data->client_name, cur_endpt_name, client->max_attempts); /* close pending socket, switching to a different endpoint */ @@ -4207,14 +4309,12 @@ nc_ch_client_thread(void *arg) cur_endpt = &client->ch_endpts[next_endpt_index]; free(cur_endpt_name); cur_endpt_name = strdup(cur_endpt->name); + NC_CHECK_ERRMEM_GOTO(!cur_endpt_name, , cleanup); } -cleanup_unlock: - /* CONFIG READ UNLOCK */ - nc_rwlock_unlock(&server_opts.config_lock, __func__); - cleanup: VRB(session, "Call Home client \"%s\" thread exit.", data->client_name); + nc_server_config_release(config); free(cur_endpt_name); if (cur_sock_pending != -1) { close(cur_sock_pending); @@ -4224,25 +4324,22 @@ nc_ch_client_thread(void *arg) } int -nc_session_server_ch_client_dispatch_stop(struct nc_ch_client *ch_client) +nc_session_server_ch_client_dispatch_stop(const char *client_name) { - int rc = 0, r; + int r; struct nc_server_ch_thread_arg *thread_arg; - pthread_t tid; - enum nc_rwlock_mode config_lock_mode = NC_RWLOCK_WRITE; - char *ch_client_name = NULL; - if (!ch_client || !ch_client->thread) { + /* unregister the thread first, so that no other caller can find and join the same one */ + if (nc_server_ch_thread_reg_del(client_name, &thread_arg)) { + return 1; + } + if (!thread_arg) { + /* no thread is running for this client */ return 0; } - thread_arg = ch_client->thread; - ch_client_name = strdup(thread_arg->client_name); - NC_CHECK_ERRMEM_GOTO(!ch_client_name, rc = 1, cleanup); - /* notify the thread to stop */ ATOMIC_STORE_RELAXED(thread_arg->thread_running, 0); - tid = thread_arg->tid; /* wake up the thread if it's in thread_wait */ if (write(thread_arg->notify_pipe[1], "x", 1) == -1) { @@ -4252,37 +4349,13 @@ nc_session_server_ch_client_dispatch_stop(struct nc_ch_client *ch_client) /* EAGAIN is fine: pipe buffer is full, meaning it's already been signaled */ } - /* 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); + /* wait for the thread to end, no lock is held so a stalled handshake blocks nothing else */ + r = pthread_join(thread_arg->tid, NULL); if (r) { - ERR(NULL, "Joining Call Home client \"%s\" thread failed (%s).", ch_client_name, strerror(r)); - rc = 1; - goto cleanup; - } - - /* CONFIG WRITE LOCK - re-acquire to clear the thread pointer and free the thread data, - * a reader may be holding the lock for the whole duration of a transport handshake */ - if (nc_rwlock_lock(&server_opts.config_lock, NC_RWLOCK_WRITE, NC_CONFIG_APPLY_LOCK_TIMEOUT, __func__) != 1) { - /* if we fail, attempt to lock again in cleanup. - * ch thread data will cause a memory leak, but we should avoid a possible crash this way */ - ERR(NULL, "Timed out waiting for the configuration lock, Call Home client \"%s\" thread data leaked.", - ch_client_name); - rc = 1; - goto cleanup; + ERR(NULL, "Joining Call Home client \"%s\" thread failed (%s), its data will be leaked.", + client_name, strerror(r)); + return 1; } - config_lock_mode = NC_RWLOCK_WRITE; - - /* clear the thread pointer, - * ch_client MUST remain valid even though we unlocked config lock, - * because the caller MUST hold config apply mutex, so no one can change the config and free the client */ - ch_client->thread = NULL; /* free the thread data */ free(thread_arg->client_name); @@ -4290,19 +4363,45 @@ nc_session_server_ch_client_dispatch_stop(struct nc_ch_client *ch_client) close(thread_arg->notify_pipe[1]); free(thread_arg); -cleanup: - 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, NC_CONFIG_APPLY_LOCK_TIMEOUT, __func__) != 1) { - ERRINT; + return 0; +} + +int +nc_server_ch_threads_destroy(void) +{ + int rc = 0; + char **names = NULL; + LY_ARRAY_COUNT_TYPE u; + + if (nc_server_ch_thread_names_get(&names)) { + return 1; + } + + LY_ARRAY_FOR(names, u) { + if (nc_session_server_ch_client_dispatch_stop(names[u])) { + rc = 1; } } - free(ch_client_name); + nc_server_ch_thread_names_free(names); + + /* CH THREADS LOCK */ + if (nc_mutex_lock(&server_opts.ch_threads_lock, NC_CH_THREADS_LOCK_TIMEOUT, __func__) != 1) { + return 1; + } + if (LY_ARRAY_COUNT(server_opts.ch_threads)) { + ERRINT; + rc = 1; + } + LY_ARRAY_FREE(server_opts.ch_threads); + server_opts.ch_threads = NULL; + /* CH THREADS UNLOCK */ + nc_mutex_unlock(&server_opts.ch_threads_lock, __func__); + return rc; } int -_nc_connect_ch_client_dispatch(struct nc_ch_client *ch_client, nc_server_ch_session_acquire_ctx_cb acquire_ctx_cb, +_nc_connect_ch_client_dispatch(const char *client_name, 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) { @@ -4315,13 +4414,14 @@ _nc_connect_ch_client_dispatch(struct nc_ch_client *ch_client, nc_server_ch_sess NC_CHECK_ERRMEM_GOTO(!arg, rc = -1, cleanup); arg->notify_pipe[0] = -1; arg->notify_pipe[1] = -1; - arg->client_name = strdup(ch_client->name); + arg->client_name = strdup(client_name); NC_CHECK_ERRMEM_GOTO(!arg->client_name, rc = -1, cleanup); arg->acquire_ctx_cb = acquire_ctx_cb; arg->release_ctx_cb = release_ctx_cb; arg->ctx_cb_data = ctx_cb_data; arg->new_session_cb = new_session_cb; arg->new_session_cb_data = new_session_cb_data; + /* OPTS READ LOCK */ if (nc_rwlock_lock(&server_opts.opts_lock, NC_RWLOCK_READ, NC_OPTS_LOCK_TIMEOUT, __func__) != 1) { rc = -1; @@ -4349,18 +4449,34 @@ _nc_connect_ch_client_dispatch(struct nc_ch_client *ch_client, nc_server_ch_sess goto cleanup; } - /* store thread data in the client */ - ch_client->thread = arg; + /* mark the thread as running before it is created, so that it can be stopped right away */ + ATOMIC_STORE_RELAXED(arg->thread_running, 1); /* 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)); - ch_client->thread = NULL; rc = -1; goto cleanup; } - /* arg is now owned by the thread */ + /* register the thread, only now can anyone else find and stop it */ + if (nc_server_ch_thread_reg_add(arg)) { + /* stop the thread we have just created */ + ATOMIC_STORE_RELAXED(arg->thread_running, 0); + if (write(arg->notify_pipe[1], "x", 1) == -1) { + if (errno != EAGAIN) { + ERR(NULL, "Writing to the notify pipe failed (%s).", strerror(errno)); + } + } + if (pthread_join(arg->tid, NULL)) { + /* cannot free the thread data safely */ + arg = NULL; + } + rc = -1; + goto cleanup; + } + + /* arg is now owned by the thread and the registry */ arg = NULL; cleanup: @@ -4383,28 +4499,29 @@ nc_connect_ch_client_dispatch(const char *client_name, nc_server_ch_session_acqu void *new_session_cb_data) { int rc = 0; - struct nc_ch_client *ch_client; + const struct nc_server_config *config; NC_CHECK_ARG_RET(NULL, client_name, acquire_ctx_cb, release_ctx_cb, new_session_cb, -1); NC_CHECK_SRV_INIT_RET(-1); - /* CONFIG WRITE LOCK */ - if (nc_rwlock_lock(&server_opts.config_lock, NC_RWLOCK_WRITE, NC_CONFIG_LOCK_TIMEOUT, __func__) != 1) { + config = nc_server_config_acquire(); + if (!config) { return -1; } /* check ch client existence */ - 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); + if (!nc_server_ch_client_get_pinned(config, client_name)) { + ERR(NULL, "Call Home client \"%s\" not found.", client_name); + rc = -1; + goto cleanup; + } - /* requires config wr lock */ - rc = _nc_connect_ch_client_dispatch(ch_client, acquire_ctx_cb, release_ctx_cb, ctx_cb_data, + rc = _nc_connect_ch_client_dispatch(client_name, acquire_ctx_cb, release_ctx_cb, ctx_cb_data, new_session_cb, new_session_cb_data); cleanup: - /* CONFIG WRITE UNLOCK */ - nc_rwlock_unlock(&server_opts.config_lock, __func__); + nc_server_config_release(config); return rc; } @@ -4877,27 +4994,33 @@ nc_server_notif_cert_exp_dates_get(struct nc_cert_exp_time_interval *intervals, struct nc_cert_expiration **exp_dates, uint32_t *exp_date_count) { int ret = 0; - struct nc_endpt *endpt; - struct nc_ch_client *ch_client; - struct nc_ch_endpt *ch_endpt; + const struct nc_server_config *config; + const struct nc_endpt *endpt; + const struct nc_ch_client *ch_client; + const struct nc_ch_endpt *ch_endpt; struct nc_certificate *cert; - struct nc_keystore *ks = &server_opts.config.keystore; - struct nc_truststore *ts = &server_opts.config.truststore; + const struct nc_keystore *ks; + const struct nc_truststore *ts; struct nc_cert_path_aux cp = {0}; - LY_ARRAY_COUNT_TYPE i; + LY_ARRAY_COUNT_TYPE i, u, v; NC_CHECK_ARG_RET(NULL, intervals, interval_count, exp_dates, exp_date_count, 1); *exp_dates = NULL; *exp_date_count = 0; - /* CONFIG READ LOCK */ - if (nc_rwlock_lock(&server_opts.config_lock, NC_RWLOCK_READ, NC_CONFIG_LOCK_TIMEOUT, __func__) != 1) { + config = nc_server_config_acquire(); + if (!config) { return 1; } + /* the aliases must only be taken from the pinned configuration */ + ks = &config->keystore; + ts = &config->truststore; + /* first go through listen certs */ - LY_ARRAY_FOR(server_opts.config.endpts, struct nc_endpt, endpt) { + LY_ARRAY_FOR(config->endpts, u) { + endpt = &config->endpts[u]; if (endpt->ti == NC_TI_TLS) { ret = nc_server_notif_cert_exp_dates_endpt_get(NULL, endpt->name, endpt->opts.tls, intervals, interval_count, exp_dates, exp_date_count); @@ -4908,8 +5031,10 @@ nc_server_notif_cert_exp_dates_get(struct nc_cert_exp_time_interval *intervals, } /* then go through all the ch clients and their endpts */ - LY_ARRAY_FOR(server_opts.config.ch_clients, struct nc_ch_client, ch_client) { - LY_ARRAY_FOR(ch_client->ch_endpts, struct nc_ch_endpt, ch_endpt) { + LY_ARRAY_FOR(config->ch_clients, u) { + ch_client = &config->ch_clients[u]; + LY_ARRAY_FOR(ch_client->ch_endpts, v) { + ch_endpt = &ch_client->ch_endpts[v]; if (ch_endpt->ti == NC_TI_TLS) { ret = nc_server_notif_cert_exp_dates_endpt_get(ch_client->name, ch_endpt->name, ch_endpt->opts.tls, intervals, interval_count, exp_dates, exp_date_count); @@ -4943,8 +5068,7 @@ nc_server_notif_cert_exp_dates_get(struct nc_cert_exp_time_interval *intervals, } cleanup: - /* CONFIG READ UNLOCK */ - nc_rwlock_unlock(&server_opts.config_lock, __func__); + nc_server_config_release(config); return ret; } @@ -5055,16 +5179,17 @@ nc_server_notif_cert_exp_intervals_get(struct nc_cert_exp_time_interval *default struct nc_cert_exp_time_interval **intervals, uint32_t *interval_count) { int rc = 0; + const struct nc_server_config *config; *intervals = NULL; *interval_count = 0; - /* CONFIG LOCK */ - if (nc_rwlock_lock(&server_opts.config_lock, NC_RWLOCK_READ, NC_CONFIG_LOCK_TIMEOUT, __func__) != 1) { + config = nc_server_config_acquire(); + if (!config) { return 1; } - if (!server_opts.config.cert_exp_notif_intervals) { + if (!config->cert_exp_notif_intervals) { /* dup the default intervals */ *intervals = malloc(default_interval_count * sizeof **intervals); NC_CHECK_ERRMEM_GOTO(!*intervals, rc = 1, cleanup); @@ -5072,16 +5197,15 @@ nc_server_notif_cert_exp_intervals_get(struct nc_cert_exp_time_interval *default *interval_count = default_interval_count; } else { /* dup the configured intervals */ - *intervals = malloc(LY_ARRAY_COUNT(server_opts.config.cert_exp_notif_intervals) * sizeof **intervals); + *intervals = malloc(LY_ARRAY_COUNT(config->cert_exp_notif_intervals) * sizeof **intervals); NC_CHECK_ERRMEM_GOTO(!*intervals, rc = 1, cleanup); - memcpy(*intervals, server_opts.config.cert_exp_notif_intervals, - LY_ARRAY_COUNT(server_opts.config.cert_exp_notif_intervals) * sizeof **intervals); - *interval_count = LY_ARRAY_COUNT(server_opts.config.cert_exp_notif_intervals); + memcpy(*intervals, config->cert_exp_notif_intervals, + LY_ARRAY_COUNT(config->cert_exp_notif_intervals) * sizeof **intervals); + *interval_count = LY_ARRAY_COUNT(config->cert_exp_notif_intervals); } cleanup: - /* CONFIG UNLOCK */ - nc_rwlock_unlock(&server_opts.config_lock, __func__); + nc_server_config_release(config); return rc; } @@ -5279,31 +5403,21 @@ nc_server_notif_cert_expiration_thread_stop(int wait) #endif /* NC_ENABLED_SSH_TLS */ int -nc_server_is_mod_ignored(const char *mod_name, int config_locked) +nc_server_is_mod_ignored(const struct nc_server_config *config, const char *mod_name) { - int ignored = 0; - LY_ARRAY_COUNT_TYPE i; + LY_ARRAY_COUNT_TYPE u; - if (!config_locked) { - /* LOCK */ - if (nc_rwlock_lock(&server_opts.config_lock, NC_RWLOCK_READ, NC_CONFIG_LOCK_TIMEOUT, __func__) != 1) { - return 0; - } + if (!config) { + return 0; } - LY_ARRAY_FOR(server_opts.config.ignored_modules, i) { - if (!strcmp(server_opts.config.ignored_modules[i], mod_name)) { - ignored = 1; - break; + LY_ARRAY_FOR(config->ignored_modules, u) { + if (!strcmp(config->ignored_modules[u], mod_name)) { + return 1; } } - if (!config_locked) { - /* UNLOCK */ - nc_rwlock_unlock(&server_opts.config_lock, __func__); - } - - return ignored; + return 0; } API int diff --git a/src/session_server_ssh.c b/src/session_server_ssh.c index d40f35a8..67818832 100644 --- a/src/session_server_ssh.c +++ b/src/session_server_ssh.c @@ -90,7 +90,7 @@ nc_ssh_find_auth_client(struct nc_server_ssh_opts *opts, const char *user, struc /* client not known by the endpt, but it references another one so try it */ if (opts->referenced_endpt_name) { - if (nc_server_endpt_get(opts->referenced_endpt_name, &referenced_endpt)) { + if (nc_server_endpt_get(session->opts.server.config, opts->referenced_endpt_name, &referenced_endpt)) { ERR(session, "Referenced endpoint \"%s\" not found.", opts->referenced_endpt_name); return NULL; } @@ -284,7 +284,8 @@ nc_server_ssh_auth_pubkey_check(struct nc_session *session, ssh_key pubkey, pubkey_count = LY_ARRAY_COUNT(auth_client->pubkeys); } else if (auth_client->pubkey_store == NC_STORE_TRUSTSTORE) { /* need to fetch from the truststore */ - ret = nc_server_ssh_ts_ref_get_keys(auth_client->ts_ref, &pubkeys, &pubkey_count); + ret = nc_server_ssh_ts_ref_get_keys(session->opts.server.config, auth_client->ts_ref, + &pubkeys, &pubkey_count); if (ret) { goto cleanup; } @@ -657,15 +658,23 @@ nc_server_ssh_privkey_data_to_tmp_file(const char *in, const char *privkey_forma /** * @brief Get asymmetric key from the keystore. * + * @param[in] config Pinned server configuration to search. * @param[in] referenced_name Name of the asymmetric key in the keystore. * @param[out] askey Referenced asymmetric key. * @return 0 on success, 1 on error. */ static int -nc_server_ssh_ks_ref_get_key(const char *referenced_name, struct nc_asymmetric_key **askey) +nc_server_ssh_ks_ref_get_key(const struct nc_server_config *config, const char *referenced_name, + struct nc_asymmetric_key **askey) { LY_ARRAY_COUNT_TYPE i; - struct nc_keystore *ks = &server_opts.config.keystore; + const struct nc_keystore *ks; + + if (!config) { + ERR(NULL, "No server configuration to get the keystore entry \"%s\" from.", referenced_name); + return 1; + } + ks = &config->keystore; *askey = NULL; @@ -680,7 +689,7 @@ nc_server_ssh_ks_ref_get_key(const char *referenced_name, struct nc_asymmetric_k return 1; } - *askey = &ks->entries[i].asym_key; + *askey = (struct nc_asymmetric_key *)&ks->entries[i].asym_key; /* check if the referenced public key is SubjectPublicKeyInfo */ if ((*askey)->pubkey.data && nc_is_pk_subject_public_key_info((*askey)->pubkey.data)) { @@ -693,15 +702,21 @@ nc_server_ssh_ks_ref_get_key(const char *referenced_name, struct nc_asymmetric_k } int -nc_server_ssh_ts_ref_get_keys(const char *referenced_name, struct nc_public_key **pubkeys, uint32_t *pubkey_count) +nc_server_ssh_ts_ref_get_keys(const struct nc_server_config *config, const char *referenced_name, + struct nc_public_key **pubkeys, uint32_t *pubkey_count) { - LY_ARRAY_COUNT_TYPE i; - struct nc_public_key *pubkey; - struct nc_truststore *ts = &server_opts.config.truststore; + LY_ARRAY_COUNT_TYPE i, u; + const struct nc_truststore *ts; *pubkeys = NULL; *pubkey_count = 0; + if (!config) { + ERR(NULL, "No server configuration to get the truststore entry \"%s\" from.", referenced_name); + return 1; + } + ts = &config->truststore; + /* lookup name */ LY_ARRAY_FOR(ts->pubkey_bags, i) { if (!strcmp(referenced_name, ts->pubkey_bags[i].name)) { @@ -714,8 +729,8 @@ nc_server_ssh_ts_ref_get_keys(const char *referenced_name, struct nc_public_key } /* check if any of the referenced public keys is SubjectPublicKeyInfo */ - LY_ARRAY_FOR(ts->pubkey_bags[i].pubkeys, struct nc_public_key, pubkey) { - if (nc_is_pk_subject_public_key_info(pubkey->data)) { + LY_ARRAY_FOR(ts->pubkey_bags[i].pubkeys, u) { + if (nc_is_pk_subject_public_key_info(ts->pubkey_bags[i].pubkeys[u].data)) { ERR(NULL, "A public key of the referenced public key bag \"%s\" is in the SubjectPublicKeyInfo format, " "which is not allowed in SSH!", referenced_name); return 1; @@ -1663,12 +1678,13 @@ nc_accept_ssh_session_open_netconf_channel(struct nc_session *session, struct nc /** * @brief Set hostkeys to be used for an SSH bind. * + * @param[in] config Pinned server configuration the options belong to. * @param[in] sbind SSH bind to use. * @param[in] opts SSH server options. * @return 0 on success, -1 on error. */ static int -nc_ssh_bind_add_hostkeys(ssh_bind sbind, struct nc_server_ssh_opts *opts) +nc_ssh_bind_add_hostkeys(const struct nc_server_config *config, ssh_bind sbind, struct nc_server_ssh_opts *opts) { int rc; char *privkey_path; @@ -1684,7 +1700,7 @@ nc_ssh_bind_add_hostkeys(ssh_bind sbind, struct nc_server_ssh_opts *opts) key = &hostkey->key; } else { /* keystore reference, need to get it */ - NC_CHECK_RET(nc_server_ssh_ks_ref_get_key(hostkey->ks_ref, &key), -1); + NC_CHECK_RET(nc_server_ssh_ks_ref_get_key(config, hostkey->ks_ref, &key), -1); } privkey_path = nc_server_ssh_privkey_data_to_tmp_file(key->privkey.data, nc_privkey_format_to_str(key->privkey.type)); @@ -1852,7 +1868,7 @@ nc_accept_ssh_session(struct nc_session *session, struct nc_server_ssh_opts *opt } /* configure host keys */ - if (nc_ssh_bind_add_hostkeys(sbind, opts)) { + if (nc_ssh_bind_add_hostkeys(session->opts.server.config, sbind, opts)) { rc = -1; goto cleanup; } @@ -1989,6 +2005,14 @@ nc_accept_ssh_session(struct nc_session *session, struct nc_server_ssh_opts *opt } cleanup: +#if LIBSSH_0_12 + /* the transport options belong to a configuration generation that is released once the handshake + * is over, the callbacks running afterwards must not reach them */ + if (session->ti.libssh.cb_data) { + ((struct nc_server_ssh_cb_data *)session->ti.libssh.cb_data)->opts = NULL; + } +#endif /* LIBSSH_0_12 */ + if (sock > -1) { close(sock); } diff --git a/src/session_server_ssh_wrapper.h b/src/session_server_ssh_wrapper.h index 8a68eb5b..827445d2 100644 --- a/src/session_server_ssh_wrapper.h +++ b/src/session_server_ssh_wrapper.h @@ -128,7 +128,16 @@ void nc_server_ssh_cb_pam_cancel(struct nc_server_ssh_cb_pam_data *data); struct nc_server_ssh_cb_data { struct ssh_server_callbacks_struct server_cb; /**< libssh server callbacks. */ struct nc_session *session; /**< The current session. */ - struct nc_server_ssh_opts *opts; /**< SSH server options. */ + + /** + * @brief SSH server options, a pointer into a configuration generation. + * + * Only valid during the transport handshake - it is dereferenced solely by + * ::nc_server_ssh_cb_auth_common_setup(), reached only from the four authentication callbacks. + * The long-lived channel callbacks use only @p session. It is cleared at the end of the + * handshake so that a later dereference fails immediately instead of reading freed memory. + */ + struct nc_server_ssh_opts *opts; struct nc_auth_state auth_state; /**< Tracks multi-method authentication state. */ struct nc_ssh_channel_cb_data *channels; /**< List of additional channel callback data, tracked so non-netconf channels can be freed. */ @@ -295,12 +304,14 @@ int nc_server_ssh_auth_pubkey_compare_key(ssh_key key, struct nc_public_key *pub /** * @brief Get public keys from the truststore. * + * @param[in] config Pinned server configuration to search. * @param[in] referenced_name Name of the public key bag in the truststore. * @param[out] pubkeys Referenced public keys. * @param[out] pubkey_count Referenced public key count. * @return 0 on success, 1 on error. */ -int nc_server_ssh_ts_ref_get_keys(const char *referenced_name, struct nc_public_key **pubkeys, uint32_t *pubkey_count); +int nc_server_ssh_ts_ref_get_keys(const struct nc_server_config *config, const char *referenced_name, + struct nc_public_key **pubkeys, uint32_t *pubkey_count); /** * @brief Get user's public keys from the system. diff --git a/src/session_server_tls.c b/src/session_server_tls.c index 08cf0aa6..9e01e0aa 100644 --- a/src/session_server_tls.c +++ b/src/session_server_tls.c @@ -36,6 +36,7 @@ /** * @brief Get certificate and private key data from keystore. * + * @param[in] config Pinned server configuration to search. * @param[in] referenced_key_name Name of the asymmetric key in the keystore. * @param[in] referenced_cert_name Name of the certificate in the keystore. * @param[out] privkey_data Retrieved private key data. @@ -44,15 +45,21 @@ * @return 0 on success, -1 on error. */ static int -nc_server_tls_ks_ref_get_cert_key(const char *referenced_key_name, const char *referenced_cert_name, - char **privkey_data, enum nc_privkey_format *privkey_type, char **cert_data) +nc_server_tls_ks_ref_get_cert_key(const struct nc_server_config *config, const char *referenced_key_name, + const char *referenced_cert_name, char **privkey_data, enum nc_privkey_format *privkey_type, char **cert_data) { LY_ARRAY_COUNT_TYPE i, j; - struct nc_keystore *ks = &server_opts.config.keystore; + const struct nc_keystore *ks; *privkey_data = NULL; *cert_data = NULL; + if (!config) { + ERR(NULL, "No server configuration to get the keystore entry \"%s\" from.", referenced_key_name); + return -1; + } + ks = &config->keystore; + /* lookup key */ LY_ARRAY_FOR(ks->entries, i) { if (!strcmp(referenced_key_name, ks->entries[i].asym_key.name)) { @@ -84,20 +91,28 @@ nc_server_tls_ks_ref_get_cert_key(const char *referenced_key_name, const char *r /** * @brief Get certificates from truststore. * + * @param[in] config Pinned server configuration to search. * @param[in] referenced_name Name of the certificate bag in the truststore. * @param[out] certs Retrieved certificates. * @param[out] cert_count Number of retrieved certificates. * @return 0 on success, -1 on error. */ static int -nc_server_tls_truststore_ref_get_certs(const char *referenced_name, struct nc_certificate **certs, uint32_t *cert_count) +nc_server_tls_truststore_ref_get_certs(const struct nc_server_config *config, const char *referenced_name, + struct nc_certificate **certs, uint32_t *cert_count) { LY_ARRAY_COUNT_TYPE i; - struct nc_truststore *ts = &server_opts.config.truststore; + const struct nc_truststore *ts; *certs = NULL; *cert_count = 0; + if (!config) { + ERR(NULL, "No server configuration to get the truststore bag \"%s\" from.", referenced_name); + return -1; + } + ts = &config->truststore; + /* lookup name */ LY_ARRAY_FOR(ts->cert_bags, i) { if (!strcmp(referenced_name, ts->cert_bags[i].name)) { @@ -500,13 +515,15 @@ nc_server_tls_cert_to_name(struct nc_ctn *ctn, void *cert_chain, char **username /** * @brief Resolve username from cert-to-name entries of endpoint and referenced endpoint. * + * @param[in] config Pinned server configuration the options belong to. * @param[in] opts TLS options of the endpoint. * @param[in] cert_chain Presented certificate chain, peer certificate first. * @param[out] username Resolved username. * @return 0 on success, 1 if no entry matched, -1 on error. */ static int -_nc_server_tls_cert_to_name(struct nc_server_tls_opts *opts, void *cert_chain, char **username) +_nc_server_tls_cert_to_name(const struct nc_server_config *config, struct nc_server_tls_opts *opts, + void *cert_chain, char **username) { int rc = 1; struct nc_endpt *referenced_endpt; @@ -522,7 +539,7 @@ _nc_server_tls_cert_to_name(struct nc_server_tls_opts *opts, void *cert_chain, c /* do the same for referenced endpoint's ctn entries */ if (opts->referenced_endpt_name) { - if (nc_server_endpt_get(opts->referenced_endpt_name, &referenced_endpt)) { + if (nc_server_endpt_get(config, opts->referenced_endpt_name, &referenced_endpt)) { ERR(NULL, "Referenced endpoint \"%s\" not found.", opts->referenced_endpt_name); ERRINT; rc = -1; @@ -543,7 +560,8 @@ _nc_server_tls_cert_to_name(struct nc_server_tls_opts *opts, void *cert_chain, c } static int -_nc_server_tls_verify_peer_cert(void *peer_cert, struct nc_server_tls_client_auth *client_auth) +_nc_server_tls_verify_peer_cert(const struct nc_server_config *config, void *peer_cert, + struct nc_server_tls_client_auth *client_auth) { int rc; void *cert; @@ -556,7 +574,7 @@ _nc_server_tls_verify_peer_cert(void *peer_cert, struct nc_server_tls_client_aut cert_count = LY_ARRAY_COUNT(client_auth->ee_certs); } else if (client_auth->ee_certs_store == NC_STORE_TRUSTSTORE) { /* truststore reference */ - if (nc_server_tls_truststore_ref_get_certs(client_auth->ee_cert_bag_ts_ref, &certs, &cert_count)) { + if (nc_server_tls_truststore_ref_get_certs(config, client_auth->ee_cert_bag_ts_ref, &certs, &cert_count)) { ERR(NULL, "Error getting end-entity certificates from the truststore reference \"%s\".", client_auth->ee_cert_bag_ts_ref); return -1; } @@ -580,24 +598,26 @@ _nc_server_tls_verify_peer_cert(void *peer_cert, struct nc_server_tls_client_aut } int -nc_server_tls_verify_peer_cert(void *peer_cert, struct nc_server_tls_opts *opts) +nc_server_tls_verify_peer_cert(void *peer_cert, struct nc_tls_verify_cb_data *cb_data) { int rc; struct nc_endpt *referenced_endpt; + struct nc_server_tls_opts *opts = cb_data->opts; + const struct nc_server_config *config = cb_data->session->opts.server.config; - rc = _nc_server_tls_verify_peer_cert(peer_cert, &opts->client_auth); + rc = _nc_server_tls_verify_peer_cert(config, peer_cert, &opts->client_auth); if (!rc) { return 0; } if (opts->referenced_endpt_name) { - if (nc_server_endpt_get(opts->referenced_endpt_name, &referenced_endpt)) { + if (nc_server_endpt_get(config, opts->referenced_endpt_name, &referenced_endpt)) { ERR(NULL, "Referenced endpoint \"%s\" not found.", opts->referenced_endpt_name); ERRINT; return -1; } - rc = _nc_server_tls_verify_peer_cert(peer_cert, &referenced_endpt->opts.tls->client_auth); + rc = _nc_server_tls_verify_peer_cert(config, peer_cert, &referenced_endpt->opts.tls->client_auth); if (!rc) { return 0; } @@ -614,6 +634,7 @@ nc_server_tls_verify_cert(void *cert, int depth, int trusted, struct nc_tls_veri struct nc_server_tls_opts *opts = cb_data->opts; struct nc_session *session = cb_data->session; void *cert_chain = cb_data->chain; + const struct nc_server_config *config = cb_data->session->opts.server.config; int (*user_verify_clb)(const struct nc_session *session); @@ -638,7 +659,7 @@ nc_server_tls_verify_cert(void *cert, int depth, int trusted, struct nc_tls_veri if (!trusted) { /* peer cert is not trusted, so it must match any configured end-entity cert * on the given endpoint in order for the client to be authenticated */ - rc = nc_server_tls_verify_peer_cert(cert, opts); + rc = nc_server_tls_verify_peer_cert(cert, cb_data); if (rc) { ERR(session, "Cert verify: fail (Client certificate not trusted and does not match any configured end-entity certificate)."); goto cleanup; @@ -649,7 +670,7 @@ nc_server_tls_verify_cert(void *cert, int depth, int trusted, struct nc_tls_veri * the whole chain is needed in order to comply with the following issue: * https://github.com/CESNET/netopeer2/issues/1596 */ - rc = _nc_server_tls_cert_to_name(opts, cert_chain, &session->username); + rc = _nc_server_tls_cert_to_name(config, opts, cert_chain, &session->username); if (rc == -1) { /* fatal error */ goto cleanup; @@ -712,7 +733,8 @@ nc_server_tls_set_verify_clb(int (*verify_clb)(const struct nc_session *session) } int -nc_server_tls_load_server_cert_key(struct nc_server_tls_opts *opts, void **srv_cert, void **srv_pkey) +nc_server_tls_load_server_cert_key(const struct nc_server_config *config, struct nc_server_tls_opts *opts, + void **srv_cert, void **srv_pkey) { char *privkey_data = NULL, *cert_data = NULL; enum nc_privkey_format privkey_type; @@ -729,7 +751,8 @@ nc_server_tls_load_server_cert_key(struct nc_server_tls_opts *opts, void **srv_c privkey_type = opts->local.key.privkey.type; } else if (opts->cert_store == NC_STORE_KEYSTORE) { /* keystore */ - if (nc_server_tls_ks_ref_get_cert_key(opts->keystore.asym_key_ref, opts->keystore.cert_ref, &privkey_data, &privkey_type, &cert_data)) { + if (nc_server_tls_ks_ref_get_cert_key(config, opts->keystore.asym_key_ref, opts->keystore.cert_ref, + &privkey_data, &privkey_type, &cert_data)) { ERR(NULL, "Getting server certificate from the keystore reference \"%s\" failed.", opts->keystore.asym_key_ref); return 1; } @@ -756,7 +779,8 @@ nc_server_tls_load_server_cert_key(struct nc_server_tls_opts *opts, void **srv_c } int -nc_server_tls_load_trusted_certs(struct nc_server_tls_client_auth *client_auth, void *cert_store) +nc_server_tls_load_trusted_certs(const struct nc_server_config *config, + struct nc_server_tls_client_auth *client_auth, void *cert_store) { struct nc_certificate *certs; uint32_t i, cert_count = 0; @@ -768,7 +792,7 @@ nc_server_tls_load_trusted_certs(struct nc_server_tls_client_auth *client_auth, cert_count = LY_ARRAY_COUNT(client_auth->ca_certs); } else if (client_auth->ca_certs_store == NC_STORE_TRUSTSTORE) { /* truststore */ - if (nc_server_tls_truststore_ref_get_certs(client_auth->ca_cert_bag_ts_ref, &certs, &cert_count)) { + if (nc_server_tls_truststore_ref_get_certs(config, client_auth->ca_cert_bag_ts_ref, &certs, &cert_count)) { ERR(NULL, "Error getting certificate-authority certificates from the truststore reference \"%s\".", client_auth->ca_cert_bag_ts_ref); return 1; } @@ -817,12 +841,14 @@ nc_server_tls_accept_check(int accept_ret, void *tls_session) /** * @brief Get the number of certificates in a certificate grouping. * + * @param[in] config Pinned server configuration to resolve truststore references in. * @param[in] client_auth Client authentication data to get the number of certificates from. * @param[out] cert_count Number of certificates in the grouping. * @return 0 on success, -1 on error. */ static int -nc_server_tls_get_num_certs(struct nc_server_tls_client_auth *client_auth, uint32_t *cert_count) +nc_server_tls_get_num_certs(const struct nc_server_config *config, struct nc_server_tls_client_auth *client_auth, + uint32_t *cert_count) { uint32_t ca_count = 0, ee_count = 0; struct nc_certificate *certs; @@ -832,7 +858,7 @@ nc_server_tls_get_num_certs(struct nc_server_tls_client_auth *client_auth, uint3 if (client_auth->ca_certs_store == NC_STORE_LOCAL) { ca_count = LY_ARRAY_COUNT(client_auth->ca_certs); } else if (client_auth->ca_certs_store == NC_STORE_TRUSTSTORE) { - if (nc_server_tls_truststore_ref_get_certs(client_auth->ca_cert_bag_ts_ref, &certs, &ca_count)) { + if (nc_server_tls_truststore_ref_get_certs(config, client_auth->ca_cert_bag_ts_ref, &certs, &ca_count)) { ERR(NULL, "Getting CA certificates from the truststore reference \"%s\" failed.", client_auth->ca_cert_bag_ts_ref); return -1; } @@ -841,7 +867,7 @@ nc_server_tls_get_num_certs(struct nc_server_tls_client_auth *client_auth, uint3 if (client_auth->ee_certs_store == NC_STORE_LOCAL) { ee_count += LY_ARRAY_COUNT(client_auth->ee_certs); } else if (client_auth->ee_certs_store == NC_STORE_TRUSTSTORE) { - if (nc_server_tls_truststore_ref_get_certs(client_auth->ee_cert_bag_ts_ref, &certs, &ee_count)) { + if (nc_server_tls_truststore_ref_get_certs(config, client_auth->ee_cert_bag_ts_ref, &certs, &ee_count)) { ERR(NULL, "Getting end-entity certificates from the truststore reference \"%s\" failed.", client_auth->ee_cert_bag_ts_ref); return -1; } @@ -860,6 +886,7 @@ nc_accept_tls_session(struct nc_session *session, struct nc_server_tls_opts *opt struct nc_endpt *referenced_endpt; void *tls_cfg, *srv_cert, *srv_pkey, *cert_store, *cipher_suites; uint32_t cert_count = 0, ref_cert_count = 0; + const struct nc_server_config *config = session->opts.server.config; tls_cfg = srv_cert = srv_pkey = cert_store = cipher_suites = NULL; @@ -880,25 +907,25 @@ nc_accept_tls_session(struct nc_session *session, struct nc_server_tls_opts *opt } /* load server's key and certificate */ - if (nc_server_tls_load_server_cert_key(opts, &srv_cert, &srv_pkey)) { + if (nc_server_tls_load_server_cert_key(config, opts, &srv_cert, &srv_pkey)) { ERR(session, "Loading server certificate and/or private key failed."); goto fail; } /* load trusted CA certificates */ - if (nc_server_tls_load_trusted_certs(&opts->client_auth, cert_store)) { + if (nc_server_tls_load_trusted_certs(config, &opts->client_auth, cert_store)) { ERR(session, "Loading server CA certs failed."); goto fail; } /* load referenced endpoint's trusted CA certs if set */ if (opts->referenced_endpt_name) { - if (nc_server_endpt_get(opts->referenced_endpt_name, &referenced_endpt)) { + if (nc_server_endpt_get(config, opts->referenced_endpt_name, &referenced_endpt)) { ERR(session, "Referenced endpoint \"%s\" not found.", opts->referenced_endpt_name); goto fail; } - if (nc_server_tls_load_trusted_certs(&referenced_endpt->opts.tls->client_auth, cert_store)) { + if (nc_server_tls_load_trusted_certs(config, &referenced_endpt->opts.tls->client_auth, cert_store)) { ERR(session, "Loading server CA certs from referenced endpoint failed."); goto fail; } @@ -906,11 +933,11 @@ nc_accept_tls_session(struct nc_session *session, struct nc_server_tls_opts *opt /* Check if there are no CA/end entity certs configured, which is a valid config. * However, that would imply not using TLS for auth, which is not (yet) supported */ - if (nc_server_tls_get_num_certs(&opts->client_auth, &cert_count)) { + if (nc_server_tls_get_num_certs(config, &opts->client_auth, &cert_count)) { goto fail; } if (opts->referenced_endpt_name) { - if (nc_server_tls_get_num_certs(&referenced_endpt->opts.tls->client_auth, &ref_cert_count)) { + if (nc_server_tls_get_num_certs(config, &referenced_endpt->opts.tls->client_auth, &ref_cert_count)) { goto fail; } cert_count += ref_cert_count; diff --git a/src/session_wrapper.h b/src/session_wrapper.h index 36713cd4..c4f1032a 100644 --- a/src/session_wrapper.h +++ b/src/session_wrapper.h @@ -248,10 +248,10 @@ int nc_server_tls_verify_cert(void *cert, int depth, int trusted, struct nc_tls_ * @brief Check if the peer certificate matches any configured ee certs. * * @param[in] peer_cert Peer certificate. - * @param[in] opts TLS options. + * @param[in] cb_data Verify callback data with the session and the TLS options. * @return 0 on success, non-zero on fail. */ -int nc_server_tls_verify_peer_cert(void *peer_cert, struct nc_server_tls_opts *opts); +int nc_server_tls_verify_peer_cert(void *peer_cert, struct nc_tls_verify_cb_data *cb_data); /** * @brief Get the subject of the certificate. diff --git a/tests/test_config.c b/tests/test_config.c index 96413d0f..78fb0b11 100644 --- a/tests/test_config.c +++ b/tests/test_config.c @@ -15,13 +15,17 @@ #define _GNU_SOURCE +#include #include +#include #include #include #include #include #include #include +#include +#include #include #include @@ -1125,11 +1129,20 @@ test_ordered_list_move(void **state) /** * @brief Time in seconds the client stalls in its password callback. * - * The server waits for the authentication while holding the configuration READ lock, so this has to be - * longer than ::NC_CONFIG_LOCK_TIMEOUT (10 s) for the test to be meaningful. + * Has to be longer than ::NC_CONFIG_LOCK_TIMEOUT (10 s) so that a configuration update waiting for + * the whole authentication would be dropped instead of applied. */ #define TEST_STALL_AUTH_SLEEP 13 +/** @brief Time in seconds the client stalls when only an in-flight handshake is needed. */ +#define TEST_STALL_AUTH_SLEEP_SHORT 5 + +/** @brief Maximum time in msec anything done while a handshake is stalled may take. */ +#define TEST_NO_BLOCK_TIMEOUT 2000 + +/** @brief Time in seconds the client stalls in its password callback, set by each test. */ +static unsigned int test_stall_auth_sleep = TEST_STALL_AUTH_SLEEP; + /** @brief Time in seconds to wait for a Call Home client to report failed connection attempts. */ #define TEST_CH_WATCH_TIME 4 @@ -1142,6 +1155,7 @@ struct test_ch_threads { pthread_t tids[TEST_CH_TID_MAX]; uint32_t tid_count; char endpt[64]; + char last_endpt[64]; }; /* acquire ctx cb for the Call Home dispatch */ @@ -1191,6 +1205,8 @@ test_ch_new_session_fail_cb(const char *client_name, const char *endpt_name, uin /* the endpoint of the very first failed attempt is the first one in the configuration */ strncpy(threads->endpt, endpt_name, sizeof threads->endpt - 1); } + memset(threads->last_endpt, 0, sizeof threads->last_endpt); + strncpy(threads->last_endpt, endpt_name, sizeof threads->last_endpt - 1); for (i = 0; i < threads->tid_count; ++i) { if (pthread_equal(threads->tids[i], self)) { break; @@ -1390,8 +1406,8 @@ test_stall_auth_password(const char *username, const char *hostname, void *priv) (void) hostname; (void) priv; - /* keep the server waiting for the authentication, it holds the configuration READ lock meanwhile */ - sleep(TEST_STALL_AUTH_SLEEP); + /* keep the server waiting for the authentication */ + sleep(test_stall_auth_sleep); /* a wrong password, the connection is expected to fail */ return strdup("wrong"); @@ -1455,6 +1471,10 @@ test_config_update_during_auth(void **state) struct lyd_node *tree = NULL, *diff = NULL; struct ln2_test_ctx *test_ctx = *state; const struct lys_module *yang_mod; + struct timespec ts_start, ts_end; + int64_t elapsed_ms; + + test_stall_auth_sleep = TEST_STALL_AUTH_SLEEP; yang_mod = ly_ctx_get_module_implemented(test_ctx->ctx, "yang"); assert_non_null(yang_mod); @@ -1489,9 +1509,15 @@ test_config_update_during_auth(void **state) * while holding the configuration READ lock */ sleep(2); - /* this must not be silently dropped */ + /* this must neither be silently dropped nor wait out the stalled authentication */ + clock_gettime(CLOCK_MONOTONIC, &ts_start); ret = nc_server_config_setup_diff(diff); assert_int_equal(ret, 0); + clock_gettime(CLOCK_MONOTONIC, &ts_end); + + elapsed_ms = ((int64_t)(ts_end.tv_sec - ts_start.tv_sec) * 1000) + + ((ts_end.tv_nsec - ts_start.tv_nsec) / 1000000); + assert_true(elapsed_ms < TEST_NO_BLOCK_TIMEOUT); for (i = 0; i < 2; i++) { pthread_join(tids[i], NULL); @@ -1501,6 +1527,305 @@ test_config_update_during_auth(void **state) lyd_free_all(tree); } +/** + * @brief Create the YANG data of a listening SSH endpoint with a password-authenticated user. + * + * @param[in] ctx libyang context. + * @param[in] endpt_name Name of the endpoint. + * @param[in] port Port to listen on. + * @param[out] tree Created YANG data. + */ +static void +test_create_stall_endpt_data(const struct ly_ctx *ctx, const char *endpt_name, uint16_t port, + struct lyd_node **tree) +{ + int ret; + + ret = nc_server_config_add_address_port(ctx, endpt_name, NC_TI_SSH, "127.0.0.1", port, tree); + assert_int_equal(ret, 0); + ret = nc_server_config_add_ssh_hostkey(ctx, endpt_name, "hostkey", TESTS_DIR "/data/key_ecdsa", + NULL, tree); + assert_int_equal(ret, 0); + ret = nc_server_config_add_ssh_user_password(ctx, endpt_name, "stall", "correct", tree); + assert_int_equal(ret, 0); + + /* add all the default nodes, the authentication timeout has to be longer than the stall */ + ret = lyd_new_implicit_tree(*tree, LYD_IMPLICIT_NO_STATE, NULL); + assert_int_equal(ret, 0); +} + +/** + * @brief Try to establish a TCP connection to a local port. + * + * @param[in] port Port to connect to. + * @return 0 if the connection was established, -1 if it was refused. + */ +static int +test_tcp_connect(uint16_t port) +{ + int sock, r; + struct sockaddr_in addr = {0}; + + sock = socket(AF_INET, SOCK_STREAM, 0); + assert_true(sock > -1); + + addr.sin_family = AF_INET; + addr.sin_port = htons(port); + addr.sin_addr.s_addr = inet_addr("127.0.0.1"); + + r = connect(sock, (struct sockaddr *)&addr, sizeof addr); + close(sock); + + return r ? -1 : 0; +} + +/** + * @brief Removing an endpoint must stop its listening socket right away. + * + * A stalled handshake keeps a reference to the configuration generation the endpoint belongs to, but + * the listening socket lives outside of it, so it is closed as soon as the update is applied. + */ +static void +test_removed_endpt_stops_listening(void **state) +{ + int ret, i; + pthread_t tids[2]; + struct lyd_node *tree = NULL; + struct ln2_test_ctx *test_ctx = *state; + + test_stall_auth_sleep = TEST_STALL_AUTH_SLEEP_SHORT; + + test_create_stall_endpt_data(test_ctx->ctx, "endpt", TEST_PORT, &tree); + ret = nc_server_config_setup_data(tree); + assert_int_equal(ret, 0); + + /* the endpoint is listening now */ + assert_int_equal(test_tcp_connect(TEST_PORT), 0); + + ret = pthread_create(&tids[0], NULL, test_stall_auth_client_thread, test_ctx); + assert_int_equal(ret, 0); + ret = pthread_create(&tids[1], NULL, test_stall_auth_server_thread, test_ctx); + assert_int_equal(ret, 0); + + /* let the key exchange finish, the server is now stalled in the authentication */ + sleep(2); + + /* remove all the endpoints, only the keystore and the truststore are left */ + ret = nc_server_config_setup_data(test_ctx->test_data); + assert_int_equal(ret, 0); + + /* the socket must be gone even though the stalled handshake still uses the old generation */ + assert_int_equal(test_tcp_connect(TEST_PORT), -1); + + for (i = 0; i < 2; i++) { + pthread_join(tids[i], NULL); + } + + lyd_free_all(tree); +} + +/** + * @brief The API-settable options must be settable while a handshake is in flight. + */ +static void +test_api_setters_during_auth(void **state) +{ + int ret, i; + pthread_t tids[2]; + struct lyd_node *tree = NULL; + struct ln2_test_ctx *test_ctx = *state; + struct timespec ts_start, ts_end; + int64_t elapsed_ms; + + test_stall_auth_sleep = TEST_STALL_AUTH_SLEEP_SHORT; + + test_create_stall_endpt_data(test_ctx->ctx, "endpt", TEST_PORT, &tree); + ret = nc_server_config_setup_data(tree); + assert_int_equal(ret, 0); + + ret = pthread_create(&tids[0], NULL, test_stall_auth_client_thread, test_ctx); + assert_int_equal(ret, 0); + ret = pthread_create(&tids[1], NULL, test_stall_auth_server_thread, test_ctx); + assert_int_equal(ret, 0); + + /* let the key exchange finish, the server is now stalled in the authentication */ + sleep(2); + + clock_gettime(CLOCK_MONOTONIC, &ts_start); + + ret = nc_server_ssh_set_protocol_string("test"); + assert_int_equal(ret, 0); + nc_server_tls_set_verify_clb(NULL); + /* returns an error without libpam support, which is fine, it must just not block */ + nc_server_ssh_set_pam_conf_filename("netconf"); + ret = nc_server_ssh_set_authkey_path_format("/tmp/%u/authorized_keys"); + assert_int_equal(ret, 0); + ret = nc_server_set_unix_socket_dir("/tmp"); + assert_int_equal(ret, 0); + + clock_gettime(CLOCK_MONOTONIC, &ts_end); + elapsed_ms = ((int64_t)(ts_end.tv_sec - ts_start.tv_sec) * 1000) + + ((ts_end.tv_nsec - ts_start.tv_nsec) / 1000000); + assert_true(elapsed_ms < TEST_NO_BLOCK_TIMEOUT); + + for (i = 0; i < 2; i++) { + pthread_join(tids[i], NULL); + } + + lyd_free_all(tree); +} + +/** + * @brief Wait until the Call Home client reports a failed attempt on the given endpoint. + * + * @param[in] threads Call Home thread tracking data. + * @param[in] endpt_name Expected endpoint name. + */ +static void +test_ch_wait_for_endpt(struct test_ch_threads *threads, const char *endpt_name) +{ + int ret; + struct timespec ts; + + pthread_mutex_lock(&threads->lock); + while (strcmp(threads->last_endpt, endpt_name)) { + clock_gettime(CLOCK_REALTIME, &ts); + ts.tv_sec += 10; + ret = pthread_cond_timedwait(&threads->cond, &threads->lock, &ts); + assert_int_equal(ret, 0); + } + pthread_mutex_unlock(&threads->lock); +} + +/** + * @brief A running Call Home thread must survive a configuration swap and pick up the new endpoints. + */ +static void +test_ch_survives_config_swap(void **state) +{ + int ret; + uint32_t tid_count; + struct lyd_node *tree = NULL, *tree2 = NULL; + struct ln2_test_ctx *test_ctx = *state; + struct test_ch_threads threads = {0}; + + pthread_mutex_init(&threads.lock, NULL); + pthread_cond_init(&threads.cond, NULL); + + /* a client with a single endpoint that can never connect anywhere */ + test_create_ch_endpt_data(test_ctx->ctx, "ch", "first", &tree); + ret = nc_server_config_add_ch_persistent(test_ctx->ctx, "ch", &tree); + assert_int_equal(ret, 0); + ret = nc_server_config_add_ch_reconnect_strategy(test_ctx->ctx, "ch", NC_CH_FIRST_LISTED, 1, 3, &tree); + assert_int_equal(ret, 0); + + nc_server_ch_set_dispatch_data(test_ch_acquire_ctx_cb, test_ch_release_ctx_cb, test_ctx, + test_ch_new_session_cb, NULL); + nc_server_ch_set_new_session_fail_cb(test_ch_new_session_fail_cb, &threads); + + ret = nc_server_config_setup_data(tree); + assert_int_equal(ret, 0); + + /* the thread is running and attempting to connect to the only endpoint */ + test_ch_wait_for_endpt(&threads, "first"); + + pthread_mutex_lock(&threads.lock); + assert_int_equal(threads.tid_count, 1); + pthread_mutex_unlock(&threads.lock); + + /* replace the whole configuration, the client keeps its name but gets a different endpoint */ + test_create_ch_endpt_data(test_ctx->ctx, "ch", "second", &tree2); + ret = nc_server_config_add_ch_persistent(test_ctx->ctx, "ch", &tree2); + assert_int_equal(ret, 0); + ret = nc_server_config_add_ch_reconnect_strategy(test_ctx->ctx, "ch", NC_CH_FIRST_LISTED, 1, 3, &tree2); + assert_int_equal(ret, 0); + + ret = nc_server_config_setup_data(tree2); + assert_int_equal(ret, 0); + + /* the very same thread must pick the new endpoint up */ + test_ch_wait_for_endpt(&threads, "second"); + + pthread_mutex_lock(&threads.lock); + tid_count = threads.tid_count; + pthread_mutex_unlock(&threads.lock); + assert_int_equal(tid_count, 1); + + lyd_free_all(tree2); + lyd_free_all(tree); + pthread_cond_destroy(&threads.cond); + pthread_mutex_destroy(&threads.lock); +} + +/** @brief Number of threads applying the configuration concurrently. */ +#define TEST_APPLY_THREAD_COUNT 4 + +/** @brief Number of configuration updates each applying thread performs. */ +#define TEST_APPLY_COUNT 10 + +struct test_apply_arg { + struct ln2_test_ctx *test_ctx; + struct lyd_node *tree; +}; + +static void * +test_apply_thread(void *arg) +{ + struct test_apply_arg *apply_arg = arg; + int ret, i; + + for (i = 0; i < TEST_APPLY_COUNT; ++i) { + ret = nc_server_config_setup_data(apply_arg->tree); + assert_int_equal(ret, 0); + } + + return NULL; +} + +/** + * @brief Several threads applying the configuration while a handshake is stalled. + * + * Every apply publishes a new configuration generation while the stalled handshake holds a reference + * to an older one, so the valgrind twin of this test is what actually checks the refcounting. + */ +static void +test_concurrent_apply_and_accept(void **state) +{ + int ret, i; + pthread_t tids[2 + TEST_APPLY_THREAD_COUNT]; + struct lyd_node *tree = NULL; + struct ln2_test_ctx *test_ctx = *state; + struct test_apply_arg apply_arg; + + test_stall_auth_sleep = TEST_STALL_AUTH_SLEEP_SHORT; + + test_create_stall_endpt_data(test_ctx->ctx, "endpt", TEST_PORT, &tree); + ret = nc_server_config_setup_data(tree); + assert_int_equal(ret, 0); + + apply_arg.test_ctx = test_ctx; + apply_arg.tree = tree; + + ret = pthread_create(&tids[0], NULL, test_stall_auth_client_thread, test_ctx); + assert_int_equal(ret, 0); + ret = pthread_create(&tids[1], NULL, test_stall_auth_server_thread, test_ctx); + assert_int_equal(ret, 0); + + /* let the key exchange finish, the server is now stalled in the authentication */ + sleep(2); + + for (i = 0; i < TEST_APPLY_THREAD_COUNT; ++i) { + ret = pthread_create(&tids[2 + i], NULL, test_apply_thread, &apply_arg); + assert_int_equal(ret, 0); + } + + for (i = 0; i < 2 + TEST_APPLY_THREAD_COUNT; i++) { + pthread_join(tids[i], NULL); + } + + lyd_free_all(tree); +} + static void test_config_data_free(void *data) { @@ -1559,6 +1884,10 @@ main(void) cmocka_unit_test_setup_teardown(test_ch_dispatch_not_duplicated, setup_f, ln2_glob_test_teardown), cmocka_unit_test_setup_teardown(test_ch_endpoint_order, setup_f, ln2_glob_test_teardown), cmocka_unit_test_setup_teardown(test_config_update_during_auth, setup_f, ln2_glob_test_teardown), + cmocka_unit_test_setup_teardown(test_removed_endpt_stops_listening, setup_f, ln2_glob_test_teardown), + cmocka_unit_test_setup_teardown(test_api_setters_during_auth, setup_f, ln2_glob_test_teardown), + cmocka_unit_test_setup_teardown(test_ch_survives_config_swap, setup_f, ln2_glob_test_teardown), + cmocka_unit_test_setup_teardown(test_concurrent_apply_and_accept, setup_f, ln2_glob_test_teardown), }; /* try to get ports from the environment, otherwise use the default */ From 388a966b6ad36d5702c73f58cc943a7bd68d268b Mon Sep 17 00:00:00 2001 From: Roman Janota Date: Tue, 25 Aug 2026 15:26:45 +0200 Subject: [PATCH 04/10] session server BUGFIX fix Call Home thread races The thread was appended to the registry only after pthread_create(), so a concurrent config apply could miss it and dispatch a second thread for the same client. Register and create the thread atomically under ch_threads_lock and reject a client that already has one, which restores the exclusivity the config write lock used to provide. A thread that terminated for any other reason than being told to left its registry entry behind, so every later apply considered the client running and never dispatched it again. Define the registry entry as the ownership token of the thread argument - whoever removes it joins the thread and frees the argument - and have an abnormally exiting thread unregister, detach and free itself. The thread also gave up when the configuration could not be acquired, which is a transient lock timeout; retry a few times instead. And do not log through the session in the thread cleanup, at that point it belongs to the user and may already have been freed. Finally, keep the rollback of the dispatch reconcile from walking a NULL entry: LY_ARRAY_NEW_GOTO() counts the new element in before the strdup() of the client name, so an allocation failure left a NULL in the array that the rollback then passed to dispatch_stop(). --- src/server_config.c | 29 ++++-- src/session_p.h | 15 ++- src/session_server.c | 206 +++++++++++++++++++++++++++------------- src/session_server_ch.h | 4 + 4 files changed, 183 insertions(+), 71 deletions(-) diff --git a/src/server_config.c b/src/server_config.c index 3a4a9a00..1a8668ba 100644 --- a/src/server_config.c +++ b/src/server_config.c @@ -5481,10 +5481,16 @@ nc_server_config_new_ch_clients_created(const struct nc_server_config *new_cfg, } /** - * @brief Atomically dispatch new Call Home clients and keep the already running ones. + * @brief Dispatch new Call Home clients, keep the already running ones and stop the removed ones. * * The running clients are learned from the Call Home thread registry, not from any configuration. * + * Starting the new clients is atomic - if any of them fails to start, the ones started by this call + * are stopped again and no client is stopped at all. Stopping the removed clients afterwards is + * not: if it fails halfway through, some removed clients are already stopped and the error is + * simply returned. That is enough because the only caller reacts to the error by reconciling + * against the generation that stays published, which dispatches the stopped clients again. + * * @param[in] new_cfg New server configuration currently being applied. * @return 0 on success, 1 on error. */ @@ -5493,7 +5499,7 @@ nc_server_config_reconcile_chclients_dispatch(const struct nc_server_config *new { int rc = 0; LY_ARRAY_COUNT_TYPE u; - char **running = NULL, **started = NULL, **started_name; + char **running = NULL, **started = NULL, **started_name, *name = NULL; struct nc_server_ch_dispatch_data dispatch_data; int dispatch_new_clients = 1; @@ -5534,15 +5540,25 @@ nc_server_config_reconcile_chclients_dispatch(const struct nc_server_config *new rc = _nc_connect_ch_client_dispatch(new_cfg->ch_clients[u].name, dispatch_data.acquire_ctx_cb, dispatch_data.release_ctx_cb, dispatch_data.ctx_cb_data, dispatch_data.new_session_cb, dispatch_data.new_session_cb_data); - if (rc) { + if (rc == 1) { + /* the client was dispatched through the API right after we learned the running ones, + * which is exactly the state we wanted, so leave the thread to its dispatcher */ + VRB(NULL, "Call Home client \"%s\" already has a running thread, skipping its dispatch.", + new_cfg->ch_clients[u].name); + rc = 0; + continue; + } else if (rc) { /* FAILURE! trigger rollback */ goto rollback; } - /* successfully started, track the client for a potential rollback */ + /* successfully started, track the client for a potential rollback, the name must be + * ready before the array grows so that the rollback never sees a NULL entry */ + name = strdup(new_cfg->ch_clients[u].name); + NC_CHECK_ERRMEM_GOTO(!name, rc = 1, rollback); LY_ARRAY_NEW_GOTO(NULL, started, started_name, rc, rollback); - *started_name = strdup(new_cfg->ch_clients[u].name); - NC_CHECK_ERRMEM_GOTO(!*started_name, rc = 1, rollback); + *started_name = name; + name = NULL; } } @@ -5579,6 +5595,7 @@ nc_server_config_reconcile_chclients_dispatch(const struct nc_server_config *new /* rc is already set to non-zero from the failure point */ cleanup: + free(name); nc_server_ch_thread_names_free(running); nc_server_ch_thread_names_free(started); return rc ? 1 : 0; diff --git a/src/session_p.h b/src/session_p.h index b09d0eaf..8b92c615 100644 --- a/src/session_p.h +++ b/src/session_p.h @@ -105,6 +105,13 @@ extern struct nc_server_opts server_opts; */ #define NC_CH_NO_ENDPT_WAIT 1000 +/** + * Number of consecutive failed attempts to acquire the server configuration after which a Call Home + * client thread gives up and terminates. A single failed attempt means the configuration lock timed + * out or the server was destroyed without stopping the thread first. + */ +#define NC_CH_CONFIG_ACQUIRE_ATTEMPTS 3 + /** * Time slept in msec between Call Home thread session idle timeout checks. */ @@ -1532,13 +1539,19 @@ int nc_session_server_ch_client_dispatch_stop(const char *client_name); /** * @brief Dispatch a thread connecting to a listening NETCONF client and creating Call Home sessions. * + * The thread is added to the Call Home thread registry and created atomically, so it is findable by + * ::nc_session_server_ch_client_dispatch_stop() and by a concurrent configuration apply from the + * moment it exists. There is never more than one thread per Call Home client. + * * @param[in] client_name Name of the Call Home client to dispatch the thread for. * @param[in] acquire_ctx_cb Callback for acquiring new session context. * @param[in] release_ctx_cb Callback for releasing session context. * @param[in] ctx_cb_data Arbitrary user data passed to @p acquire_ctx_cb and @p release_ctx_cb. * @param[in] new_session_cb Callback called for every established session on the client. * @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. + * @return 0 if the thread was successfully created. + * @return 1 if a thread is already running for the client and nothing was done. + * @return -1 on error. */ int _nc_connect_ch_client_dispatch(const char *client_name, 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, diff --git a/src/session_server.c b/src/session_server.c index 260adbd2..c58765eb 100644 --- a/src/session_server.c +++ b/src/session_server.c @@ -67,34 +67,33 @@ static nc_rpc_clb global_rpc_clb = NULL; #ifdef NC_ENABLED_SSH_TLS /** - * @brief Add a Call Home thread argument to the thread registry. + * @brief Free a Call Home thread argument. * - * @param[in] thread_arg Thread argument to register. - * @return 0 on success, 1 on error. + * @param[in] thread_arg Thread argument to free, may be NULL. */ -static int -nc_server_ch_thread_reg_add(struct nc_server_ch_thread_arg *thread_arg) +static void +nc_server_ch_thread_arg_free(struct nc_server_ch_thread_arg *thread_arg) { - int rc = 0; - struct nc_server_ch_thread_arg **item; - - /* CH THREADS LOCK */ - if (nc_mutex_lock(&server_opts.ch_threads_lock, NC_CH_THREADS_LOCK_TIMEOUT, __func__) != 1) { - return 1; + if (!thread_arg) { + return; } - LY_ARRAY_NEW_GOTO(NULL, server_opts.ch_threads, item, rc, cleanup); - *item = thread_arg; - -cleanup: - /* CH THREADS UNLOCK */ - nc_mutex_unlock(&server_opts.ch_threads_lock, __func__); - return rc ? 1 : 0; + free(thread_arg->client_name); + if (thread_arg->notify_pipe[0] != -1) { + close(thread_arg->notify_pipe[0]); + } + if (thread_arg->notify_pipe[1] != -1) { + close(thread_arg->notify_pipe[1]); + } + free(thread_arg); } /** * @brief Remove a Call Home thread argument from the thread registry. * + * The registry entry is the ownership token of the thread argument, whoever removes it becomes + * responsible for terminating the thread and freeing the argument. + * * @param[in] client_name Name of the Call Home client to unregister the thread of. * @param[out] thread_arg Unregistered thread argument, NULL if the client had no thread registered. * @return 0 on success, 1 on error. @@ -129,6 +128,58 @@ nc_server_ch_thread_reg_del(const char *client_name, struct nc_server_ch_thread_ return 0; } +/** + * @brief Unregister a Call Home thread that is terminating on its own and free its argument. + * + * Called by the Call Home thread itself right before it returns. Normally the thread only ever + * terminates because ::nc_session_server_ch_client_dispatch_stop() told it to, in which case the + * stopper has already removed the registry entry and does all the cleanup itself. If the thread + * terminates for any other reason (an unrecoverable error), it has to take itself out of the + * registry, otherwise every later configuration apply would believe the client is still running + * and would never dispatch it again. + * + * The registry entry is the ownership token of the thread argument, so the entry removal decides + * who cleans up and the ::nc_server_opts.ch_threads_lock makes that decision atomic. + * + * @param[in] thread_arg Argument of the calling thread. + */ +static void +nc_server_ch_thread_unreg_self(struct nc_server_ch_thread_arg *thread_arg) +{ + LY_ARRAY_COUNT_TYPE u; + int found = 0; + + /* CH THREADS LOCK */ + if (nc_mutex_lock(&server_opts.ch_threads_lock, NC_CH_THREADS_LOCK_TIMEOUT, __func__) != 1) { + return; + } + + LY_ARRAY_FOR(server_opts.ch_threads, u) { + if (server_opts.ch_threads[u] != thread_arg) { + continue; + } + + found = 1; + + /* swap the last entry into the hole, the order of the registry is irrelevant */ + server_opts.ch_threads[u] = server_opts.ch_threads[LY_ARRAY_COUNT(server_opts.ch_threads) - 1]; + LY_ARRAY_DECREMENT_FREE(server_opts.ch_threads); + break; + } + + /* CH THREADS UNLOCK */ + nc_mutex_unlock(&server_opts.ch_threads_lock, __func__); + + if (!found) { + /* someone else owns us now and will join us, nothing to do */ + return; + } + + /* nobody is going to join us anymore, so make sure our resources are reclaimed */ + pthread_detach(thread_arg->tid); + nc_server_ch_thread_arg_free(thread_arg); +} + void nc_server_ch_thread_names_free(char **names) { @@ -4064,36 +4115,45 @@ nc_server_ch_client_thread_wait(struct nc_session *session, struct nc_server_ch_ * @brief Acquire a configuration in which the Call Home client has at least one endpoint defined. * * A client that is missing from the published configuration is waited for the same way as a client - * with no endpoints - it may simply not have been published yet. The thread only ever stops because - * ::nc_server_ch_thread_arg.thread_running was cleared. + * with no endpoints - it may simply not have been published yet, so the thread is normally only + * ever stopped by clearing ::nc_server_ch_thread_arg.thread_running. A configuration that cannot be + * acquired at all is retried a few times as well, but not forever - it means either a wedged + * configuration lock or a server destroyed without stopping this thread first. * * @param[in] data Call Home client thread argument. * @param[out] client Found Call Home client of the returned configuration. * @return Pinned server configuration, the caller must release it. - * @return NULL if the thread should stop running or the configuration could not be acquired. + * @return NULL if the thread should stop running. */ static const struct nc_server_config * nc_server_ch_client_acquire_with_endpt(struct nc_server_ch_thread_arg *data, const struct nc_ch_client **client) { const struct nc_server_config *config; + uint32_t failed_attempts = 0; *client = NULL; while (ATOMIC_LOAD_RELAXED(data->thread_running)) { config = nc_server_config_acquire(); - if (!config) { - return NULL; - } + if (config) { + failed_attempts = 0; + + *client = nc_server_ch_client_get_pinned(config, data->client_name); + if (*client && (*client)->ch_endpts) { + /* the client is configured and has at least one endpoint */ + return config; + } - *client = nc_server_ch_client_get_pinned(config, data->client_name); - if (*client && (*client)->ch_endpts) { - /* the client is configured and has at least one endpoint */ - return config; + /* not configured (yet) or no endpoints defined yet */ + nc_server_config_release(config); + *client = NULL; + } else if (++failed_attempts == NC_CH_CONFIG_ACQUIRE_ATTEMPTS) { + ERR(NULL, "Call Home client \"%s\" failed to acquire the server configuration %d times, " + "terminating its thread.", data->client_name, NC_CH_CONFIG_ACQUIRE_ATTEMPTS); + return NULL; } - /* not configured (yet) or no endpoints defined yet, wait a little bit */ - nc_server_config_release(config); - *client = NULL; + /* the configuration is not usable (yet), wait a little bit and try again */ usleep(NC_CH_NO_ENDPT_WAIT * 1000); } @@ -4104,6 +4164,9 @@ nc_server_ch_client_acquire_with_endpt(struct nc_server_ch_thread_arg *data, con /** * @brief Call Home client management thread. * + * Runs until ::nc_server_ch_thread_arg.thread_running is cleared or an unrecoverable error occurs. + * In the latter case it unregisters itself, see ::nc_server_ch_thread_unreg_self(). + * * @param[in] arg CH client thread argument. * @return NULL. */ @@ -4230,6 +4293,9 @@ nc_ch_client_thread(void *arg) } else { /* session was not created, wait a little bit and try again */ ++cur_attempts; + + /* copy what is needed after the configuration is released, the user callback and the + * wait must not run with a generation pinned */ max_wait = client->max_wait; max_attempts = client->max_attempts; @@ -4313,13 +4379,19 @@ nc_ch_client_thread(void *arg) } cleanup: - VRB(session, "Call Home client \"%s\" thread exit.", data->client_name); + /* the session, if there still is one, belongs to the user and may have been freed already, + * so it must not be logged through */ + VRB(NULL, "Call Home client \"%s\" thread exit.", data->client_name); nc_server_config_release(config); free(cur_endpt_name); if (cur_sock_pending != -1) { close(cur_sock_pending); } + /* if we are terminating on our own, take ourselves out of the registry so that the client can + * be dispatched again, otherwise this is a no-op and whoever stopped us cleans up after us */ + nc_server_ch_thread_unreg_self(data); + return NULL; } @@ -4357,11 +4429,8 @@ nc_session_server_ch_client_dispatch_stop(const char *client_name) return 1; } - /* free the thread data */ - free(thread_arg->client_name); - close(thread_arg->notify_pipe[0]); - close(thread_arg->notify_pipe[1]); - free(thread_arg); + /* the registry entry was ours, so is the cleanup */ + nc_server_ch_thread_arg_free(thread_arg); return 0; } @@ -4407,7 +4476,9 @@ _nc_connect_ch_client_dispatch(const char *client_name, nc_server_ch_session_acq { int rc = 0, r; int flags; - struct nc_server_ch_thread_arg *arg = NULL; + LY_ERR lyrc = LY_SUCCESS; + struct nc_server_ch_thread_arg *arg = NULL, **item; + LY_ARRAY_COUNT_TYPE u; /* create the thread argument */ arg = calloc(1, sizeof *arg); @@ -4452,44 +4523,46 @@ _nc_connect_ch_client_dispatch(const char *client_name, nc_server_ch_session_acq /* mark the thread as running before it is created, so that it can be stopped right away */ ATOMIC_STORE_RELAXED(arg->thread_running, 1); - /* 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)); + /* CH THREADS LOCK - the registration and the thread creation must be atomic, the registry entry + * is what makes the thread findable and joinable, so it must exist before the thread does but + * it must never refer to a thread that was not created yet */ + if (nc_mutex_lock(&server_opts.ch_threads_lock, NC_CH_THREADS_LOCK_TIMEOUT, __func__) != 1) { rc = -1; goto cleanup; } - /* register the thread, only now can anyone else find and stop it */ - if (nc_server_ch_thread_reg_add(arg)) { - /* stop the thread we have just created */ - ATOMIC_STORE_RELAXED(arg->thread_running, 0); - if (write(arg->notify_pipe[1], "x", 1) == -1) { - if (errno != EAGAIN) { - ERR(NULL, "Writing to the notify pipe failed (%s).", strerror(errno)); - } - } - if (pthread_join(arg->tid, NULL)) { - /* cannot free the thread data safely */ - arg = NULL; + /* there must never be two threads dispatched for a single Call Home client */ + LY_ARRAY_FOR(server_opts.ch_threads, u) { + if (!strcmp(server_opts.ch_threads[u]->client_name, client_name)) { + rc = 1; + goto unlock; } + } + + /* register the thread first, the array cannot fail to grow once the thread is running */ + LY_ARRAY_NEW_GOTO(NULL, server_opts.ch_threads, item, lyrc, unlock); + *item = 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)); + LY_ARRAY_DECREMENT_FREE(server_opts.ch_threads); rc = -1; - goto cleanup; + goto unlock; } /* arg is now owned by the thread and the registry */ arg = NULL; -cleanup: - if (arg) { - free(arg->client_name); - if (arg->notify_pipe[0] != -1) { - close(arg->notify_pipe[0]); - } - if (arg->notify_pipe[1] != -1) { - close(arg->notify_pipe[1]); - } - free(arg); +unlock: + /* CH THREADS UNLOCK */ + nc_mutex_unlock(&server_opts.ch_threads_lock, __func__); + if (lyrc) { + rc = -1; } + +cleanup: + nc_server_ch_thread_arg_free(arg); return rc; } @@ -4519,6 +4592,11 @@ nc_connect_ch_client_dispatch(const char *client_name, nc_server_ch_session_acqu rc = _nc_connect_ch_client_dispatch(client_name, acquire_ctx_cb, release_ctx_cb, ctx_cb_data, new_session_cb, new_session_cb_data); + if (rc == 1) { + /* a thread is already running for this client, do not silently ignore that */ + ERR(NULL, "Call Home client \"%s\" is already being dispatched.", client_name); + rc = -1; + } cleanup: nc_server_config_release(config); diff --git a/src/session_server_ch.h b/src/session_server_ch.h index 91c0b7d0..302bdbe3 100644 --- a/src/session_server_ch.h +++ b/src/session_server_ch.h @@ -106,6 +106,10 @@ typedef void (*nc_server_ch_new_session_fail_cb)(const char *client_name, const /** * @brief Dispatch a thread connecting to a listening NETCONF client and creating Call Home sessions. * + * There is at most one thread per Call Home client, so dispatching a client that already has a + * running thread (either from a previous call or automatically, when its configuration was applied) + * does nothing and is reported as an error. + * * @param[in] client_name Existing client name. * @param[in] acquire_ctx_cb Callback for acquiring new session context. * @param[in] release_ctx_cb Callback for releasing session context. From 05e01af1d2432a50ec02764ec9ca36ba93a4599d Mon Sep 17 00:00:00 2001 From: Roman Janota Date: Tue, 25 Aug 2026 15:27:38 +0200 Subject: [PATCH 05/10] session server BUGFIX fix bind registry reconcile A connection accepted on a socket whose endpoint the pinned config does not have was accepted and then dropped. That also happened for a plain endpoint rename, which changes nothing about the socket. Resolve the endpoint of every bind while the poll set is built and skip the ones the pinned configuration does not know, so the pending connections stay in the listen backlog for a call with a newer configuration instead of being reset. The reuse pass also wrote the new endpoint name into the live registry entry right away, so a later failure of the apply left the registry carrying a name the published generation does not have - with the change above that socket would then never be polled again. Stage the rename in the bind description and store it only once nothing can fail, which is what the function documented all along. Since the registry lock is not held while polling, an apply may close a socket meanwhile. Accepting on a closed descriptor is a normal outcome here, so skip it instead of failing the whole nc_accept(). Also move struct nc_bind_desc to the private header and document how it differs from a configured bind and a registry entry. --- src/session_p.h | 39 +++++++++++++- src/session_server.c | 119 ++++++++++++++++++++++++------------------- 2 files changed, 105 insertions(+), 53 deletions(-) diff --git a/src/session_p.h b/src/session_p.h index 8b92c615..c2d7419d 100644 --- a/src/session_p.h +++ b/src/session_p.h @@ -848,10 +848,11 @@ struct nc_server_opts { pthread_mutex_t binds_lock; /**< Lock for the listening socket registry. */ /** - * @brief Entry of the listening socket registry. + * @brief Entry of the listening socket registry, one per socket the server is listening on. * * A listening socket is runtime state, not configuration, so it is kept out of * ::nc_server_config, which must not be written to while it is being read by an accept path. + * See ::nc_bind_desc for the difference between a registry entry and a bind description. */ struct nc_bind_entry { char *endpt_name; /**< Name of the endpoint the listening socket belongs to. */ @@ -933,6 +934,42 @@ struct nc_server_opts { configuration update to complete before accepting a new one. */ }; +/** + * @brief Description of a single listening socket required by a server configuration generation. + * + * There are three representations of a listening socket in the server, each with a different + * lifetime and owner: + * + * - ::nc_bind is the configured one. It is part of ::nc_server_config, so it is immutable and it + * only holds what the YANG data say - for a UNIX endpoint that is a possibly relative socket path + * and no port at all. + * - ::nc_bind_entry is the live one. It is an entry of the listening socket registry in + * ::nc_server_opts.binds and it owns the open socket FD. Since sockets must survive a + * configuration change untouched (a session may be in the middle of being accepted on one), they + * cannot live in a configuration generation that is replaced on every apply. + * - ::nc_bind_desc, this structure, is the transient one. It is the resolved and flattened form of + * all the ::nc_bind of a single generation, built by an applier and thrown away once the apply is + * over. + * + * The description exists because reconciling the registry with a new generation needs data that a + * ::nc_bind does not have and that must not be computed with the registry lock held. + * + * So an apply (::nc_server_binds_reconcile()) builds the descriptions of the new generation with no lock held, matches + * them against the registry entries, opens the sockets that are missing and closes the entries that no description matches. + * Everything that can fail is done into the descriptions first, the registry itself is only ever modified by steps + * that cannot fail anymore, so a failed apply leaves the registry exactly as it was. + */ +struct nc_bind_desc { + const struct nc_endpt *endpt; /**< Endpoint the listening socket belongs to. */ + char *address; /**< Resolved address, the full socket path for a UNIX endpoint. */ + uint16_t port; /**< Port number, 0 for a UNIX socket. */ + int reused; /**< Whether an already registered socket is being reused. */ + LY_ARRAY_COUNT_TYPE entry_idx; /**< Index of the reused registry entry, valid only if @p reused. */ + char *rename; /**< New endpoint name to store into the reused registry entry, + NULL if the endpoint was not renamed. */ + int sock; /**< Newly opened listening socket, -1 if none was opened. */ +}; + /** * @brief Type of the session */ diff --git a/src/session_server.c b/src/session_server.c index c58765eb..9dac72b3 100644 --- a/src/session_server.c +++ b/src/session_server.c @@ -987,6 +987,12 @@ nc_sock_accept_first(struct pollfd *pfd, uint16_t pfd_count, int *client_sock, /* another thread already accepted the connection, try another one */ continue; } + if ((errno == EBADF) || (errno == ENOTSOCK)) { + /* the listening socket was closed by a configuration apply after we copied it + * out of the registry, which is a normal outcome here, try another one */ + DBG(NULL, "Accept on an already closed listening socket, skipping it."); + continue; + } ERR(NULL, "Accept failed (%s).", strerror(errno)); return -1; } @@ -1103,8 +1109,16 @@ nc_sock_accept_pollfds(struct pollfd *pollfds, uint16_t pollfd_count, const char * The listening socket registry is only read to build the local poll arrays, the ::poll() itself * and the ::accept() run with no lock held. * - * @note A connection accepted on a bind that @p config does not contain is dropped. That can only - * happen for a bind registered after @p config was read, in which case the next call accepts it. + * @note Only the sockets of endpoints that @p config contains are polled. A socket registered for + * an endpoint that @p config does not know (it was registered or its endpoint renamed after + * @p config was read) is skipped, its pending connections are left in the listen backlog for a + * call with a newer configuration pinned. That way no connection is ever accepted just to be + * dropped again because there is no endpoint to serve it with. + * + * @note Since the registry lock is not held while polling, a configuration apply may close one of + * the sockets meanwhile. Polling and accepting a closed descriptor is handled (the socket is simply + * skipped), but the descriptor number may also have been reused by then, in which case the + * connection is accepted on and logged with whatever the endpoint of the new socket is. * * @param[in] config Pinned server configuration used to look the accepting endpoint up. * @param[in] timeout Timeout for accepting a connection. @@ -1122,8 +1136,9 @@ nc_server_accept_binds(const struct nc_server_config *config, int timeout, char uint16_t pollfd_count = 0, fd_idx = 0, i, bind_count = 0; LY_ARRAY_COUNT_TYPE u; int ret = 1, binds_locked = 0; - char **addr_map = NULL, **name_map = NULL; + char **addr_map = NULL; uint16_t *port_map = NULL; + LY_ARRAY_COUNT_TYPE *endpt_map = NULL; /* BINDS LOCK */ if (nc_mutex_lock(&server_opts.binds_lock, NC_BINDS_LOCK_TIMEOUT, __func__) != 1) { @@ -1145,19 +1160,30 @@ nc_server_accept_binds(const struct nc_server_config *config, int timeout, char NC_CHECK_ERRMEM_GOTO(!addr_map, ret = -1, cleanup); port_map = malloc(bind_count * sizeof *port_map); NC_CHECK_ERRMEM_GOTO(!port_map, ret = -1, cleanup); - name_map = calloc(bind_count, sizeof *name_map); - NC_CHECK_ERRMEM_GOTO(!name_map, ret = -1, cleanup); + endpt_map = malloc(bind_count * sizeof *endpt_map); + NC_CHECK_ERRMEM_GOTO(!endpt_map, ret = -1, cleanup); for (i = 0; i < bind_count; ++i) { + /* resolve the endpoint of the bind in the pinned configuration, it is immutable so the + * index stays valid for as long as the configuration is pinned */ + LY_ARRAY_FOR(config->endpts, u) { + if (!strcmp(config->endpts[u].name, server_opts.binds[i].endpt_name)) { + break; + } + } + if (u == LY_ARRAY_COUNT(config->endpts)) { + /* we would have no endpoint to serve a connection accepted here with, do not poll it */ + continue; + } + endpt_map[pollfd_count] = u; + pollfds[pollfd_count].fd = server_opts.binds[i].sock; pollfds[pollfd_count].events = POLLIN; pollfds[pollfd_count].revents = 0; - /* the registry entries may be freed once the lock is released, so copy the strings */ + /* the registry entries may be freed once the lock is released, so copy the address */ addr_map[pollfd_count] = strdup(server_opts.binds[i].address); NC_CHECK_ERRMEM_GOTO(!addr_map[pollfd_count], ret = -1, cleanup); - name_map[pollfd_count] = strdup(server_opts.binds[i].endpt_name); - NC_CHECK_ERRMEM_GOTO(!name_map[pollfd_count], ret = -1, cleanup); port_map[pollfd_count] = server_opts.binds[i].port; ++pollfd_count; @@ -1167,29 +1193,19 @@ nc_server_accept_binds(const struct nc_server_config *config, int timeout, char nc_mutex_unlock(&server_opts.binds_lock, __func__); binds_locked = 0; + if (!pollfd_count) { + /* every registered socket belongs to an endpoint the pinned configuration does not have, + * report a timeout right away and let the caller retry with a newer configuration */ + VRB(NULL, "No listening socket of the pinned configuration to accept on."); + ret = 0; + goto cleanup; + } + /* accept a new connection on any of the sockets */ ret = nc_sock_accept_pollfds(pollfds, pollfd_count, (const char **)addr_map, port_map, timeout, host, port, &fd_idx, sock); - if (ret > 0) { - /* map the endpoint name back to an endpoint of the pinned configuration */ - LY_ARRAY_FOR(config->endpts, u) { - if (!strcmp(config->endpts[u].name, name_map[fd_idx])) { - break; - } - } - if (u == LY_ARRAY_COUNT(config->endpts)) { - /* the endpoint is not in the configuration we are working with, drop the connection */ - VRB(NULL, "Endpoint \"%s\" not found, dropping the accepted connection.", name_map[fd_idx]); - close(*sock); - *sock = -1; - if (host) { - free(*host); - *host = NULL; - } - ret = 0; - } else if (idx) { - *idx = u; - } + if ((ret > 0) && idx) { + *idx = endpt_map[fd_idx]; } cleanup: @@ -1197,18 +1213,15 @@ nc_server_accept_binds(const struct nc_server_config *config, int timeout, char /* BINDS UNLOCK */ nc_mutex_unlock(&server_opts.binds_lock, __func__); } - for (i = 0; i < bind_count; ++i) { - if (addr_map) { + if (addr_map) { + for (i = 0; i < bind_count; ++i) { free(addr_map[i]); } - if (name_map) { - free(name_map[i]); - } } free(pollfds); free(addr_map); free(port_map); - free(name_map); + free(endpt_map); return ret; } @@ -3024,17 +3037,6 @@ nc_ps_clear(struct nc_pollsession *ps, int all, void (*data_free)(void *)) nc_ps_unlock(ps, q_id, __func__); } -/** - * @brief Description of a listening socket required by a server configuration. - */ -struct nc_bind_desc { - const struct nc_endpt *endpt; /**< Endpoint the listening socket belongs to. */ - char *address; /**< Resolved address, the full socket path for a UNIX endpoint. */ - uint16_t port; /**< Port number, 0 for a UNIX socket. */ - int reused; /**< Whether an already registered socket is being reused. */ - int sock; /**< Newly opened listening socket, -1 if none was opened. */ -}; - /** * @brief Start listening on a socket of an endpoint bind. * @@ -3205,6 +3207,7 @@ nc_server_bind_descs_free(struct nc_bind_desc *descs) LY_ARRAY_FOR(descs, u) { nc_server_bind_desc_close(&descs[u]); free(descs[u].address); + free(descs[u].rename); } LY_ARRAY_FREE(descs); } @@ -3236,15 +3239,15 @@ nc_server_binds_reconcile(const struct nc_server_config *config) continue; } - /* the socket stays open, but the endpoint owning it may have been renamed */ + descs[u].reused = 1; + descs[u].entry_idx = v; + + /* the socket stays open, but the endpoint owning it may have been renamed, prepare the + * new name and store it only once nothing can fail anymore */ if (strcmp(server_opts.binds[v].endpt_name, descs[u].endpt->name)) { - endpt_name = strdup(descs[u].endpt->name); - NC_CHECK_ERRMEM_GOTO(!endpt_name, rc = 1, cleanup); - free(server_opts.binds[v].endpt_name); - server_opts.binds[v].endpt_name = endpt_name; + descs[u].rename = strdup(descs[u].endpt->name); + NC_CHECK_ERRMEM_GOTO(!descs[u].rename, rc = 1, cleanup); } - - descs[u].reused = 1; break; } @@ -3301,6 +3304,18 @@ nc_server_binds_reconcile(const struct nc_server_config *config) ++added; } + /* the registry entries did not move, so store the new endpoint names now that nothing can fail */ + LY_ARRAY_FOR(descs, u) { + if (!descs[u].rename) { + continue; + } + + entry = &server_opts.binds[descs[u].entry_idx]; + free(entry->endpt_name); + entry->endpt_name = descs[u].rename; + descs[u].rename = NULL; + } + /* stop listening on the sockets the configuration no longer contains */ v = 0; while (v < LY_ARRAY_COUNT(server_opts.binds)) { From 9da75af7f0f372a371d8cf161eec696922ce4c1f Mon Sep 17 00:00:00 2001 From: Roman Janota Date: Tue, 25 Aug 2026 15:28:31 +0200 Subject: [PATCH 06/10] session server BUGFIX fix apply and init errors A failed Call Home dispatch reconcile jumped to cleanup instead of rollback. The listening sockets were already reconciled against the generation being applied at that point, so a deleted endpoint stayed closed while the published configuration still advertised it and newly opened sockets accepted connections with no endpoint to serve them. nc_server_init() left the initial configuration generation allocated when a later init step failed, which both leaked it and made the server look initialized. Release it and clear the initialized flag. The options lock must not be held across anything slow, but the UNIX socket path resolution ran two realpath() calls under it. Copy the base directory and the hidden path mapping out and resolve the path unlocked. Also report a failed strdup() of the system public keys path format as an allocation error instead of "path format not set", and reject a NULL directory in the UNIX socket dir setter and getter. --- src/server_config.c | 16 +++++++--- src/session_p.h | 10 +++--- src/session_server.c | 67 ++++++++++++++++++++++++++++------------ src/session_server.h | 7 +++++ src/session_server_ssh.c | 6 ++-- 5 files changed, 75 insertions(+), 31 deletions(-) diff --git a/src/server_config.c b/src/server_config.c index 1a8668ba..49e56439 100644 --- a/src/server_config.c +++ b/src/server_config.c @@ -504,6 +504,9 @@ nc_server_config_truststore_free(struct nc_truststore *ts) /** * @brief Free the data of a server configuration generation. * + * @note Never call this directly on a published generation, dropping the last reference with + * ::nc_server_config_release() is the only way a generation may be freed. + * * @param[in] config Server configuration to free. */ static void @@ -6348,9 +6351,10 @@ nc_server_config_setup_diff(const struct lyd_node *data) ERR(NULL, "Starting to listen on new endpoints failed."), cleanup); #ifdef NC_ENABLED_SSH_TLS - /* dispatch new call-home threads */ + /* dispatch new call-home threads, the listening sockets are already reconciled with the new + * generation so a failure here has to be rolled back */ NC_CHECK_ERR_GOTO(ret = nc_server_config_reconcile_chclients_dispatch(config_copy), - ERR(NULL, "Dispatching new call-home threads failed."), cleanup); + ERR(NULL, "Dispatching new call-home threads failed."), rollback); #endif /* NC_ENABLED_SSH_TLS */ /* CONFIG WR LOCK - only the pointer swap */ @@ -6460,9 +6464,10 @@ nc_server_config_setup_data(const struct lyd_node *data) ERR(NULL, "Starting to listen on new endpoints failed."), cleanup); #ifdef NC_ENABLED_SSH_TLS - /* dispatch new call-home connections */ + /* dispatch new call-home connections, the listening sockets are already reconciled with the new + * generation so a failure here has to be rolled back */ NC_CHECK_ERR_GOTO(ret = nc_server_config_reconcile_chclients_dispatch(config), - ERR(NULL, "Dispatching new call-home connections failed."), cleanup); + ERR(NULL, "Dispatching new call-home connections failed."), rollback); #endif /* NC_ENABLED_SSH_TLS */ /* CONFIG WR LOCK - only the pointer swap */ @@ -6498,10 +6503,11 @@ nc_server_config_setup_data(const struct lyd_node *data) #ifdef NC_ENABLED_SSH_TLS nc_server_config_reconcile_chclients_dispatch(cur_config); #endif /* NC_ENABLED_SSH_TLS */ - nc_server_config_release(cur_config); } cleanup: + nc_server_config_release(cur_config); + /* release the new generation, it was either not published or it is NULL */ nc_server_config_release(config); diff --git a/src/session_p.h b/src/session_p.h index c2d7419d..8b9b7b8b 100644 --- a/src/session_p.h +++ b/src/session_p.h @@ -871,11 +871,11 @@ struct nc_server_opts { * ::nc_server_opts.ssh_protocol_string, ::nc_server_opts.user_verify_clb, * ::nc_server_opts.unix_socket_dir and ::nc_server_opts.unix_paths. * - * It is a leaf lock, never acquire another lock while holding it. Only ::nc_server_opts.config_lock - * may be held while acquiring it, never the other way around. Since it is also held on the - * authentication path, it must never be held across anything slow and, most importantly, never - * across a call to a user callback - read the callback and its data pointer as a pair, unlock, - * and only then call it. + * It is a leaf lock, never acquire another lock while holding it, and no other lock is held + * while acquiring it either. Since it is also held on the authentication path, it must never be + * held across anything slow (not even filesystem access) and, most importantly, never across a + * call to a user callback - read the callback and its data pointer as a pair, unlock, and only + * then call it. */ pthread_rwlock_t opts_lock; diff --git a/src/session_server.c b/src/session_server.c index 9dac72b3..650b4b8d 100644 --- a/src/session_server.c +++ b/src/session_server.c @@ -578,25 +578,25 @@ nc_sock_listen_inet(const char *address, uint16_t port) /** * @brief Construct the full path to the UNIX socket. * - * @note The options read lock must be held. + * @note Resolves the paths on the filesystem, so no lock may be held. * + * @param[in] dir Base directory the socket must reside in, NULL if none is set. * @param[in] filename Name of the socket file. * @param[out] path Constructed full path to the UNIX socket (must be freed by the caller). * @return 0 on success, 1 on error. */ static int -nc_session_unix_construct_socket_path(const char *filename, char **path) +nc_session_unix_construct_socket_path(const char *dir, const char *filename, char **path) { int rc = 0, is_prefix, is_subdir, is_exact; char *full_path = NULL, *real_base_dir = NULL, *last_slash = NULL, *sock_dir_path = NULL; char *real_target_dir = NULL; struct sockaddr_un sun; size_t dir_len, base_len; - const char *dir = server_opts.unix_socket_dir; if (!dir) { ERR(NULL, "Cannot construct UNIX socket path \"%s\"" - " (no base directory set, see nc_set_unix_socket_dir()).", filename); + " (no base directory set, see nc_server_set_unix_socket_dir()).", filename); return 1; } @@ -686,27 +686,28 @@ nc_session_unix_construct_socket_path(const char *filename, char **path) static char * nc_server_unix_get_socket_path(const struct nc_endpt *endpt) { + int rc = 0; LY_ARRAY_COUNT_TYPE i; const char *p = NULL; - char *path = NULL; + char *path = NULL, *sock_dir = NULL; /* OPTS READ LOCK */ if (nc_rwlock_lock(&server_opts.opts_lock, NC_RWLOCK_READ, NC_OPTS_LOCK_TIMEOUT, __func__) != 1) { return NULL; } - /* check the endpoints options for type of socket path */ - if (endpt->opts.unix->path_type == NC_UNIX_SOCKET_PATH_FILE) { - /* UNIX socket endpoints always have only one bind, get its address */ - p = endpt->binds[0].address; - - /* it is relative, we need to construct the full path */ - if (nc_session_unix_construct_socket_path(p, &path)) { - path = NULL; - goto cleanup; + /* only copy what is needed out of the options, resolving the path touches the filesystem and + * the options lock must not be held for that */ + switch (endpt->opts.unix->path_type) { + case NC_UNIX_SOCKET_PATH_FILE: + /* the address in the bind is relative to the base directory */ + if (server_opts.unix_socket_dir) { + sock_dir = strdup(server_opts.unix_socket_dir); + NC_CHECK_ERRMEM_GOTO(!sock_dir, rc = 1, cleanup); } - } else if (endpt->opts.unix->path_type == NC_UNIX_SOCKET_PATH_HIDDEN) { - /* search the mappings, no need to construct the path */ + break; + case NC_UNIX_SOCKET_PATH_HIDDEN: + /* search the mappings, they store the full path so there is nothing to construct */ LY_ARRAY_FOR(server_opts.unix_paths, i) { if (!strcmp(server_opts.unix_paths[i].endpt_name, endpt->name)) { p = server_opts.unix_paths[i].path; @@ -715,18 +716,38 @@ nc_server_unix_get_socket_path(const struct nc_endpt *endpt) } if (!p) { ERR(NULL, "UNIX socket path mapping for endpoint \"%s\" not found.", endpt->name); + rc = 1; goto cleanup; } path = strdup(p); - NC_CHECK_ERRMEM_GOTO(!path, path = NULL, cleanup); - } else { + NC_CHECK_ERRMEM_GOTO(!path, rc = 1, cleanup); + break; + default: ERRINT; + rc = 1; + break; } cleanup: /* OPTS READ UNLOCK */ nc_rwlock_unlock(&server_opts.opts_lock, __func__); + + if (rc) { + free(sock_dir); + free(path); + return NULL; + } + if (path) { + /* the hidden path is used as it is */ + return path; + } + + /* UNIX socket endpoints always have only one bind, its address is the socket file name */ + if (nc_session_unix_construct_socket_path(sock_dir, endpt->binds[0].address, &path)) { + path = NULL; + } + free(sock_dir); return path; } @@ -1533,7 +1554,7 @@ nc_server_init(void) if (nc_tls_backend_init_wrap()) { ERR(NULL, "%s: failed to init the SSL library backend.", __func__); - return -1; + goto error; } /* optional for dynamic library, mandatory for static */ @@ -1558,6 +1579,10 @@ nc_server_init(void) return 0; error: + /* the server is not initialized, do not leave a configuration generation behind */ + nc_server_config_release(server_opts.config); + server_opts.config = NULL; + ATOMIC_STORE_RELAXED(server_opts.new_session_id, 0); return -1; } @@ -5595,6 +5620,8 @@ nc_server_set_unix_socket_dir(const char *dir) { int rc = 0; + NC_CHECK_ARG_RET(NULL, dir, 1); + /* OPTS WRITE LOCK */ if (nc_rwlock_lock(&server_opts.opts_lock, NC_RWLOCK_WRITE, NC_OPTS_LOCK_TIMEOUT, __func__) != 1) { return 1; @@ -5615,6 +5642,8 @@ nc_server_get_unix_socket_dir(char **dir) { int rc = 0; + NC_CHECK_ARG_RET(NULL, dir, 1); + *dir = NULL; /* OPTS READ LOCK */ diff --git a/src/session_server.h b/src/session_server.h index c4b82289..e9e94b1a 100644 --- a/src/session_server.h +++ b/src/session_server.h @@ -144,6 +144,13 @@ int nc_server_init(void); /** * @brief Destroy any dynamically allocated libssh and/or libssl/libcrypto and server resources. * + * No other server API call may run concurrently with this function and there must be no session + * being accepted or authenticated. Established sessions are not affected, but the data of the + * user callbacks set through the API (::nc_server_ssh_set_interactive_auth_clb(), + * ::nc_server_ch_set_dispatch_data(), ...) is released here, so an authentication still running in + * one of them would use freed data. Call Home client threads are stopped, but only the ones + * dispatched before this call. + * * @return 0 on success, 1 on error - failed to synchronize with other threads * (timed out waiting for locks or failed to join threads). Safe to call * again to retry freeing resources. diff --git a/src/session_server_ssh.c b/src/session_server_ssh.c index 67818832..d8a16d3e 100644 --- a/src/session_server_ssh.c +++ b/src/session_server_ssh.c @@ -821,7 +821,7 @@ nc_server_ssh_str_append(const char src_c, const char *src_str, int *size, int * static int nc_server_ssh_get_system_keys_path(const char *username, char **out_path) { - int ret = 0, i, have_percent = 0, size = 0, idx = 0; + int ret = 0, i, have_percent = 0, size = 0, idx = 0, fmt_set = 0; char *path_fmt = NULL; char *path = NULL, *buf = NULL, *uid = NULL; struct passwd *pw, pw_buf; @@ -832,15 +832,17 @@ nc_server_ssh_get_system_keys_path(const char *username, char **out_path) return 1; } if (server_opts.authkey_path_fmt) { + fmt_set = 1; path_fmt = strdup(server_opts.authkey_path_fmt); } /* OPTS READ UNLOCK */ nc_rwlock_unlock(&server_opts.opts_lock, __func__); - if (!path_fmt) { + if (!fmt_set) { ERR(NULL, "System public keys path format not set."); return 1; } + NC_CHECK_ERRMEM_RET(!path_fmt, 1); /* check if the path format contains any tokens */ if (strstr(path_fmt, "%h") || strstr(path_fmt, "%U") || strstr(path_fmt, "%u") || strstr(path_fmt, "%%")) { From ece9c28d0bc7805a3464a966e147237700897dbb Mon Sep 17 00:00:00 2001 From: Roman Janota Date: Tue, 25 Aug 2026 15:29:20 +0200 Subject: [PATCH 07/10] session REFACTOR const-correct the pinned config A published configuration generation is immutable, but the accept and Call Home paths cast the constness away to pass its keepalives to nc_sock_connect() and to hand an endpoint out of nc_server_endpt_get(). All the callees only read, so make the types say so: nc_sock_connect() takes const keepalives (nc_sock_configure_ka() already did) and nc_server_endpt_get() returns a const endpoint. No cast is then left to let a future caller mutate a generation other threads are reading. --- src/session_client.c | 2 +- src/session_p.h | 4 ++-- src/session_server.c | 8 ++++---- src/session_server_ssh.c | 2 +- src/session_server_tls.c | 6 +++--- 5 files changed, 11 insertions(+), 11 deletions(-) diff --git a/src/session_client.c b/src/session_client.c index 1b0586c8..6af5d98b 100644 --- a/src/session_client.c +++ b/src/session_client.c @@ -1665,7 +1665,7 @@ sock_connect(const char *src_addr, uint16_t src_port, int timeout_ms, int *sock_ int nc_sock_connect(const char *src_addr, uint16_t src_port, const char *dst_addr, uint16_t dst_port, int timeout_ms, - struct nc_keepalives *ka, int *sock_pending, char **ip_host) + const struct nc_keepalives *ka, int *sock_pending, char **ip_host) { int i, opt; int sock = sock_pending ? *sock_pending : -1; diff --git a/src/session_p.h b/src/session_p.h index 8b9b7b8b..14edb6b1 100644 --- a/src/session_p.h +++ b/src/session_p.h @@ -1447,7 +1447,7 @@ int nc_sock_bind_inet(int sock, const char *address, uint16_t port, int is_ipv4) * @return Connected socket or -1 on error. */ int nc_sock_connect(const char *src_addr, uint16_t src_port, const char *dst_addr, uint16_t dst_port, int timeout_ms, - struct nc_keepalives *ka, int *sock_pending, char **ip_host); + const struct nc_keepalives *ka, int *sock_pending, char **ip_host); /** * @brief Accept a new socket connection. @@ -1503,7 +1503,7 @@ int nc_connect_unix_session(struct nc_session *session, int sock, const char *us * @param[out] endpt Pointer to the endpoint structure. * @return 0 on success, 1 on failure. */ -int nc_server_endpt_get(const struct nc_server_config *config, const char *name, struct nc_endpt **endpt); +int nc_server_endpt_get(const struct nc_server_config *config, const char *name, const struct nc_endpt **endpt); /** * @brief Add a client Call Home bind, listen on it. diff --git a/src/session_server.c b/src/session_server.c index 650b4b8d..684dd928 100644 --- a/src/session_server.c +++ b/src/session_server.c @@ -251,7 +251,7 @@ nc_server_ch_client_get_pinned(const struct nc_server_config *config, const char #endif /* NC_ENABLED_SSH_TLS */ int -nc_server_endpt_get(const struct nc_server_config *config, const char *name, struct nc_endpt **endpt) +nc_server_endpt_get(const struct nc_server_config *config, const char *name, const struct nc_endpt **endpt) { LY_ARRAY_COUNT_TYPE u; @@ -263,7 +263,7 @@ nc_server_endpt_get(const struct nc_server_config *config, const char *name, str LY_ARRAY_FOR(config->endpts, u) { if (config->endpts[u].name && !strcmp(config->endpts[u].name, name)) { - *endpt = (struct nc_endpt *)&config->endpts[u]; + *endpt = &config->endpts[u]; return 0; } } @@ -3644,7 +3644,7 @@ nc_accept(int timeout, const struct ly_ctx *ctx, struct nc_session **session) } /* configure keepalives */ - if (nc_sock_configure_ka(sock, (struct nc_keepalives *)&config->endpts[endpt_idx].ka)) { + if (nc_sock_configure_ka(sock, &config->endpts[endpt_idx].ka)) { msgtype = NC_MSG_ERROR; goto cleanup; } @@ -3827,7 +3827,7 @@ nc_connect_ch_endpt(const struct nc_server_config *config, const struct nc_ch_en char *ip_host = NULL; sock = nc_sock_connect(endpt->src_addr, endpt->src_port, endpt->dst_addr, endpt->dst_port, - NC_CH_CONNECT_TIMEOUT, (struct nc_keepalives *)&endpt->ka, cur_sock_pending, &ip_host); + NC_CH_CONNECT_TIMEOUT, &endpt->ka, cur_sock_pending, &ip_host); if (sock < 0) { return NC_MSG_ERROR; } diff --git a/src/session_server_ssh.c b/src/session_server_ssh.c index d8a16d3e..ef127d5a 100644 --- a/src/session_server_ssh.c +++ b/src/session_server_ssh.c @@ -75,7 +75,7 @@ nc_ssh_check_local_user_support(struct nc_session *session) struct nc_auth_client * nc_ssh_find_auth_client(struct nc_server_ssh_opts *opts, const char *user, struct nc_session *session) { - struct nc_endpt *referenced_endpt; + const struct nc_endpt *referenced_endpt; LY_ARRAY_COUNT_TYPE u; if (!user) { diff --git a/src/session_server_tls.c b/src/session_server_tls.c index 9e01e0aa..d3a7226f 100644 --- a/src/session_server_tls.c +++ b/src/session_server_tls.c @@ -526,7 +526,7 @@ _nc_server_tls_cert_to_name(const struct nc_server_config *config, struct nc_ser void *cert_chain, char **username) { int rc = 1; - struct nc_endpt *referenced_endpt; + const struct nc_endpt *referenced_endpt; struct nc_ctn *ctn; for (ctn = opts->ctn; ctn; ctn = ctn->next) { @@ -601,7 +601,7 @@ int nc_server_tls_verify_peer_cert(void *peer_cert, struct nc_tls_verify_cb_data *cb_data) { int rc; - struct nc_endpt *referenced_endpt; + const struct nc_endpt *referenced_endpt; struct nc_server_tls_opts *opts = cb_data->opts; const struct nc_server_config *config = cb_data->session->opts.server.config; @@ -883,7 +883,7 @@ nc_accept_tls_session(struct nc_session *session, struct nc_server_tls_opts *opt int rc, timeouted = 0; struct timespec ts_timeout; struct nc_tls_verify_cb_data cb_data = {0}; - struct nc_endpt *referenced_endpt; + const struct nc_endpt *referenced_endpt; void *tls_cfg, *srv_cert, *srv_pkey, *cert_store, *cipher_suites; uint32_t cert_count = 0, ref_cert_count = 0; const struct nc_server_config *config = session->opts.server.config; From 60180d37821048ad6a827912e4575c72a37e28d5 Mon Sep 17 00:00:00 2001 From: Roman Janota Date: Tue, 25 Aug 2026 15:29:33 +0200 Subject: [PATCH 08/10] session server UPDATE document snapshot locking Explain why the published configuration pointer needs the rwlock and cannot be a plain atomic pointer: acquiring a generation is a pointer load followed by a refcount increment on what was loaded, and the read lock is what keeps an applier from publishing and freeing the old generation in between the two. Doing that lock-free would need hazard pointers, RCU or a double-width CAS, none of which the compatibility layer provides, and there is nothing to gain either. Also document why the idle timeout is mirrored into an atomic next to the generation, and that a missing generation simply means no module is ignored in the . --- src/session.c | 3 ++- src/session_p.h | 27 +++++++++++++++++++++++---- 2 files changed, 25 insertions(+), 5 deletions(-) diff --git a/src/session.c b/src/session.c index e8a92181..4d779c81 100644 --- a/src/session.c +++ b/src/session.c @@ -1354,7 +1354,8 @@ _nc_server_get_cpblts_version(const struct ly_ctx *ctx, LYS_VERSION version) NC_CHECK_ARG_RET(NULL, ctx, NULL); - /* pin the configuration, only the ignored module names are needed from it */ + /* pin the configuration, only the ignored module names are needed from it, so if there is none + * (the server is not initialized) simply no module is ignored */ config = nc_server_config_acquire(); cpblts = malloc(3 * sizeof *cpblts); diff --git a/src/session_p.h b/src/session_p.h index 14edb6b1..f601bcc2 100644 --- a/src/session_p.h +++ b/src/session_p.h @@ -824,14 +824,33 @@ 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 - the published configuration pointer is swapped under the WRITE lock, - * - a reference to it is acquired under the READ lock */ + /** + * @brief Lock for the ::nc_server_opts.config pointer. + * + * ACCESS locked - the published configuration pointer is swapped under the WRITE lock, + * - a reference to it is acquired under the READ lock. + * + * Acquiring a new config generation is: a load of the pointer followed by an increment + * of the refcount of what was loaded and these two steps must not be split. + */ pthread_rwlock_t config_lock; /**< Lock for the ::nc_server_opts.config pointer. */ struct nc_server_config *config; /**< Currently published YANG server configuration generation, NULL until ::nc_server_init(). */ - /* ACCESS unlocked - mirror of the published config->idle_timeout, stored under the config WRITE lock */ - ATOMIC_T idle_timeout; /**< Idle timeout of the server sessions in seconds, 0 for none. */ + /** + * @brief Idle timeout of the server sessions in seconds, 0 for none. + * + * ACCESS unlocked - mirror of the published config->idle_timeout, stored under the config + * WRITE lock so that it always matches the generation in ::nc_server_opts.config. + * + * It is the only piece of the configuration the session poll and paths need, and + * ::nc_ps_poll() reads it for every session on every iteration. Acquiring and releasing a + * whole generation (config lock plus two atomic refcount updates) just to read a single scalar + * that often is needlessly expensive, and mirroring it keeps the configuration out of the poll + * path completely. Reading a value one generation old is harmless here, it costs at most one + * extra poll iteration before the session times out. + */ + ATOMIC_T idle_timeout; /* ACCESS locked - CH threads lock - leaf lock, never acquire another lock while holding it */ pthread_mutex_t ch_threads_lock; /**< Lock for the Call Home thread registry. */ From 1be40855ad5b8c39d2395cf56dcca3b669ee1f16 Mon Sep 17 00:00:00 2001 From: Roman Janota Date: Fri, 28 Aug 2026 09:54:38 +0200 Subject: [PATCH 09/10] session server REFACTOR move CH client reconcile session_server.c owns the Call Home thread registry the same way it owns the listening socket registry, so the operation that reconciles that registry with a configuration generation belongs there too, next to nc_server_binds_reconcile(). Everything the function actually does already lived in session_server.c (nc_server_ch_thread_names_get(), _nc_connect_ch_client_dispatch(), nc_session_server_ch_client_dispatch_stop() and ch_dispatch_data), it only ever reads the configuration passed to it. Moving it also leaves server_config.c with no reason to touch the opts_lock at all. Rename it and its helpers into the CH namespace of the file they now live in, put the documentation into session_p.h like the bind registry does and let the two appliers call a symmetric pair: nc_server_binds_reconcile(config) nc_server_ch_clients_reconcile(config) No functional change. --- src/server_config.c | 193 +------------------------------------------ src/session_p.h | 26 ++++++ src/session_server.c | 171 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 201 insertions(+), 189 deletions(-) diff --git a/src/server_config.c b/src/server_config.c index 49e56439..22689c8b 100644 --- a/src/server_config.c +++ b/src/server_config.c @@ -5419,191 +5419,6 @@ nc_server_config_libnetconf2_netconf_server(const struct lyd_node *tree, int is_ #ifdef NC_ENABLED_SSH_TLS -/** - * @brief Check whether a Call Home client name is present in an array of names. - * - * @param[in] names Array of names (sized-array, see libyang docs). - * @param[in] name Name to look for. - * @return 1 if @p name is present, 0 otherwise. - */ -static int -nc_server_config_ch_name_found(char **names, const char *name) -{ - LY_ARRAY_COUNT_TYPE u; - - LY_ARRAY_FOR(names, u) { - if (!strcmp(names[u], name)) { - return 1; - } - } - - return 0; -} - -/** - * @brief Check whether a server configuration contains a Call Home client of the given name. - * - * @param[in] config Server configuration. - * @param[in] name Name of the Call Home client to look for. - * @return 1 if the client is configured, 0 otherwise. - */ -static int -nc_server_config_ch_client_configured(const struct nc_server_config *config, const char *name) -{ - LY_ARRAY_COUNT_TYPE u; - - LY_ARRAY_FOR(config->ch_clients, u) { - if (!strcmp(config->ch_clients[u].name, name)) { - return 1; - } - } - - return 0; -} - -/** - * @brief Check if the new configuration contains a Call Home client that has no thread running. - * - * @param[in] new_cfg New server configuration currently being applied. - * @param[in] running Names of the Call Home clients with a running thread (sized-array, see libyang docs). - * @return 1 if there are new CH clients, 0 otherwise. - */ -static int -nc_server_config_new_ch_clients_created(const struct nc_server_config *new_cfg, char **running) -{ - LY_ARRAY_COUNT_TYPE u; - - LY_ARRAY_FOR(new_cfg->ch_clients, u) { - if (!nc_server_config_ch_name_found(running, new_cfg->ch_clients[u].name)) { - return 1; - } - } - - /* no differences found */ - return 0; -} - -/** - * @brief Dispatch new Call Home clients, keep the already running ones and stop the removed ones. - * - * The running clients are learned from the Call Home thread registry, not from any configuration. - * - * Starting the new clients is atomic - if any of them fails to start, the ones started by this call - * are stopped again and no client is stopped at all. Stopping the removed clients afterwards is - * not: if it fails halfway through, some removed clients are already stopped and the error is - * simply returned. That is enough because the only caller reacts to the error by reconciling - * against the generation that stays published, which dispatches the stopped clients again. - * - * @param[in] new_cfg New server configuration currently being applied. - * @return 0 on success, 1 on error. - */ -static int -nc_server_config_reconcile_chclients_dispatch(const struct nc_server_config *new_cfg) -{ - int rc = 0; - LY_ARRAY_COUNT_TYPE u; - char **running = NULL, **started = NULL, **started_name, *name = NULL; - struct nc_server_ch_dispatch_data dispatch_data; - int dispatch_new_clients = 1; - - /* OPTS READ LOCK */ - if (nc_rwlock_lock(&server_opts.opts_lock, NC_RWLOCK_READ, NC_OPTS_LOCK_TIMEOUT, __func__) != 1) { - return 1; - } - dispatch_data = server_opts.ch_dispatch_data; - /* OPTS READ UNLOCK */ - nc_rwlock_unlock(&server_opts.opts_lock, __func__); - - /* learn which clients are running right now */ - NC_CHECK_GOTO(rc = nc_server_ch_thread_names_get(&running), cleanup); - - if (!dispatch_data.acquire_ctx_cb || !dispatch_data.release_ctx_cb || !dispatch_data.new_session_cb) { - /* 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(new_cfg, running)) { - 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 threads for potential rollback. - */ - if (dispatch_new_clients) { - /* only dispatch if all required CBs are set */ - LY_ARRAY_FOR(new_cfg->ch_clients, u) { - if (nc_server_config_ch_name_found(running, new_cfg->ch_clients[u].name)) { - /* already running */ - continue; - } - - /* this is a new Call Home client, dispatch it */ - rc = _nc_connect_ch_client_dispatch(new_cfg->ch_clients[u].name, dispatch_data.acquire_ctx_cb, - dispatch_data.release_ctx_cb, dispatch_data.ctx_cb_data, - dispatch_data.new_session_cb, dispatch_data.new_session_cb_data); - if (rc == 1) { - /* the client was dispatched through the API right after we learned the running ones, - * which is exactly the state we wanted, so leave the thread to its dispatcher */ - VRB(NULL, "Call Home client \"%s\" already has a running thread, skipping its dispatch.", - new_cfg->ch_clients[u].name); - rc = 0; - continue; - } else if (rc) { - /* FAILURE! trigger rollback */ - goto rollback; - } - - /* successfully started, track the client for a potential rollback, the name must be - * ready before the array grows so that the rollback never sees a NULL entry */ - name = strdup(new_cfg->ch_clients[u].name); - NC_CHECK_ERRMEM_GOTO(!name, rc = 1, rollback); - LY_ARRAY_NEW_GOTO(NULL, started, started_name, rc, rollback); - *started_name = name; - name = NULL; - } - } - - /* - * == PHASE 2: STOP DELETED CLIENTS (COMMIT) == - * All new clients started successfully. Now stop the running clients - * that are not present in the new configuration. - */ - LY_ARRAY_FOR(running, u) { - if (nc_server_config_ch_client_configured(new_cfg, running[u])) { - continue; - } - - /* this Call Home client was deleted, notify it to stop */ - if ((rc = nc_session_server_ch_client_dispatch_stop(running[u]))) { - ERR(NULL, "Failed to dispatch stop for Call Home client \"%s\".", running[u]); - goto rollback; - } - } - - /* success */ - rc = 0; - goto cleanup; - -rollback: - /* - * == ROLLBACK LOGIC == - * An error occurred during PHASE 1. Stop any new threads we *just* started - * to return to the pre-call state. - */ - LY_ARRAY_FOR(started, u) { - nc_session_server_ch_client_dispatch_stop(started[u]); - } - /* rc is already set to non-zero from the failure point */ - -cleanup: - free(name); - nc_server_ch_thread_names_free(running); - nc_server_ch_thread_names_free(started); - return rc ? 1 : 0; -} - /** * @brief Create a deep copy of the SSH server options. * @@ -6353,7 +6168,7 @@ nc_server_config_setup_diff(const struct lyd_node *data) #ifdef NC_ENABLED_SSH_TLS /* dispatch new call-home threads, the listening sockets are already reconciled with the new * generation so a failure here has to be rolled back */ - NC_CHECK_ERR_GOTO(ret = nc_server_config_reconcile_chclients_dispatch(config_copy), + NC_CHECK_ERR_GOTO(ret = nc_server_ch_clients_reconcile(config_copy), ERR(NULL, "Dispatching new call-home threads failed."), rollback); #endif /* NC_ENABLED_SSH_TLS */ @@ -6388,7 +6203,7 @@ nc_server_config_setup_diff(const struct lyd_node *data) if (cur_config) { nc_server_binds_reconcile(cur_config); #ifdef NC_ENABLED_SSH_TLS - nc_server_config_reconcile_chclients_dispatch(cur_config); + nc_server_ch_clients_reconcile(cur_config); #endif /* NC_ENABLED_SSH_TLS */ } @@ -6466,7 +6281,7 @@ nc_server_config_setup_data(const struct lyd_node *data) #ifdef NC_ENABLED_SSH_TLS /* dispatch new call-home connections, the listening sockets are already reconciled with the new * generation so a failure here has to be rolled back */ - NC_CHECK_ERR_GOTO(ret = nc_server_config_reconcile_chclients_dispatch(config), + NC_CHECK_ERR_GOTO(ret = nc_server_ch_clients_reconcile(config), ERR(NULL, "Dispatching new call-home connections failed."), rollback); #endif /* NC_ENABLED_SSH_TLS */ @@ -6501,7 +6316,7 @@ nc_server_config_setup_data(const struct lyd_node *data) if (cur_config) { nc_server_binds_reconcile(cur_config); #ifdef NC_ENABLED_SSH_TLS - nc_server_config_reconcile_chclients_dispatch(cur_config); + nc_server_ch_clients_reconcile(cur_config); #endif /* NC_ENABLED_SSH_TLS */ } diff --git a/src/session_p.h b/src/session_p.h index f601bcc2..414880b5 100644 --- a/src/session_p.h +++ b/src/session_p.h @@ -1255,6 +1255,32 @@ int nc_server_binds_reconcile(const struct nc_server_config *config); */ void nc_server_binds_destroy(void); +#ifdef NC_ENABLED_SSH_TLS + +/** + * @brief Reconcile the Call Home thread registry with the given server configuration. + * + * Dispatches a thread for every Call Home client of @p config that has none yet, keeps the already + * running ones and stops the threads of the clients @p config no longer contains. The running + * clients are learned from the registry itself, not from any configuration. Nothing is written + * to @p config. + * + * Starting the new clients is atomic - if any of them fails to start, the ones started by this call + * are stopped again and no client is stopped at all. Stopping the removed clients afterwards is + * not: if it fails halfway through, some removed clients are already stopped and the error is + * simply returned. That is enough because the callers react to the error by reconciling against + * the generation that stays published, which dispatches the stopped clients again. + * + * @note Only one thread may reconcile the registry at a time, the callers must be serialized by + * ::nc_server_opts.config_update_lock. + * + * @param[in] config Server configuration to reconcile the registry with. + * @return 0 on success, 1 on error. + */ +int nc_server_ch_clients_reconcile(const struct nc_server_config *config); + +#endif /* NC_ENABLED_SSH_TLS */ + /** * @brief Acquire a reference to the currently published server configuration generation. * diff --git a/src/session_server.c b/src/session_server.c index 684dd928..d437f561 100644 --- a/src/session_server.c +++ b/src/session_server.c @@ -4643,6 +4643,177 @@ nc_connect_ch_client_dispatch(const char *client_name, nc_server_ch_session_acqu return rc; } +/** + * @brief Check whether a Call Home client name is present in an array of names. + * + * @param[in] names Array of names (sized-array, see libyang docs). + * @param[in] name Name to look for. + * @return 1 if @p name is present, 0 otherwise. + */ +static int +nc_server_ch_name_found(char **names, const char *name) +{ + LY_ARRAY_COUNT_TYPE u; + + LY_ARRAY_FOR(names, u) { + if (!strcmp(names[u], name)) { + return 1; + } + } + + return 0; +} + +/** + * @brief Check whether a server configuration contains a Call Home client of the given name. + * + * @param[in] config Server configuration. + * @param[in] name Name of the Call Home client to look for. + * @return 1 if the client is configured, 0 otherwise. + */ +static int +nc_server_ch_client_configured(const struct nc_server_config *config, const char *name) +{ + LY_ARRAY_COUNT_TYPE u; + + LY_ARRAY_FOR(config->ch_clients, u) { + if (!strcmp(config->ch_clients[u].name, name)) { + return 1; + } + } + + return 0; +} + +/** + * @brief Check if the new configuration contains a Call Home client that has no thread running. + * + * @param[in] config New server configuration currently being applied. + * @param[in] running Names of the Call Home clients with a running thread (sized-array, see libyang docs). + * @return 1 if there are new CH clients, 0 otherwise. + */ +static int +nc_server_ch_new_clients_created(const struct nc_server_config *config, char **running) +{ + LY_ARRAY_COUNT_TYPE u; + + LY_ARRAY_FOR(config->ch_clients, u) { + if (!nc_server_ch_name_found(running, config->ch_clients[u].name)) { + return 1; + } + } + + /* no differences found */ + return 0; +} + +int +nc_server_ch_clients_reconcile(const struct nc_server_config *config) +{ + int rc = 0; + LY_ARRAY_COUNT_TYPE u; + char **running = NULL, **started = NULL, **started_name, *name = NULL; + struct nc_server_ch_dispatch_data dispatch_data; + int dispatch_new_clients = 1; + + /* OPTS READ LOCK */ + if (nc_rwlock_lock(&server_opts.opts_lock, NC_RWLOCK_READ, NC_OPTS_LOCK_TIMEOUT, __func__) != 1) { + return 1; + } + dispatch_data = server_opts.ch_dispatch_data; + /* OPTS READ UNLOCK */ + nc_rwlock_unlock(&server_opts.opts_lock, __func__); + + /* learn which clients are running right now */ + NC_CHECK_GOTO(rc = nc_server_ch_thread_names_get(&running), cleanup); + + if (!dispatch_data.acquire_ctx_cb || !dispatch_data.release_ctx_cb || !dispatch_data.new_session_cb) { + /* Call Home dispatch callbacks not set, we can't dispatch new clients, but we can still stop deleted ones */ + if (nc_server_ch_new_clients_created(config, running)) { + 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 config that are not already running. + * Track successfully started threads for potential rollback. + */ + if (dispatch_new_clients) { + /* only dispatch if all required CBs are set */ + LY_ARRAY_FOR(config->ch_clients, u) { + if (nc_server_ch_name_found(running, config->ch_clients[u].name)) { + /* already running */ + continue; + } + + /* this is a new Call Home client, dispatch it */ + rc = _nc_connect_ch_client_dispatch(config->ch_clients[u].name, dispatch_data.acquire_ctx_cb, + dispatch_data.release_ctx_cb, dispatch_data.ctx_cb_data, + dispatch_data.new_session_cb, dispatch_data.new_session_cb_data); + if (rc == 1) { + /* the client was dispatched through the API right after we learned the running ones, + * which is exactly the state we wanted, so leave the thread to its dispatcher */ + VRB(NULL, "Call Home client \"%s\" already has a running thread, skipping its dispatch.", + config->ch_clients[u].name); + rc = 0; + continue; + } else if (rc) { + /* FAILURE! trigger rollback */ + goto rollback; + } + + /* successfully started, track the client for a potential rollback, the name must be + * ready before the array grows so that the rollback never sees a NULL entry */ + name = strdup(config->ch_clients[u].name); + NC_CHECK_ERRMEM_GOTO(!name, rc = 1, rollback); + LY_ARRAY_NEW_GOTO(NULL, started, started_name, rc, rollback); + *started_name = name; + name = NULL; + } + } + + /* + * == PHASE 2: STOP DELETED CLIENTS (COMMIT) == + * All new clients started successfully. Now stop the running clients + * that are not present in the new configuration. + */ + LY_ARRAY_FOR(running, u) { + if (nc_server_ch_client_configured(config, running[u])) { + continue; + } + + /* this Call Home client was deleted, notify it to stop */ + if ((rc = nc_session_server_ch_client_dispatch_stop(running[u]))) { + ERR(NULL, "Failed to dispatch stop for Call Home client \"%s\".", running[u]); + goto rollback; + } + } + + /* success */ + rc = 0; + goto cleanup; + +rollback: + /* + * == ROLLBACK LOGIC == + * An error occurred during PHASE 1. Stop any new threads we *just* started + * to return to the pre-call state. + */ + LY_ARRAY_FOR(started, u) { + nc_session_server_ch_client_dispatch_stop(started[u]); + } + /* rc is already set to non-zero from the failure point */ + +cleanup: + free(name); + nc_server_ch_thread_names_free(running); + nc_server_ch_thread_names_free(started); + return rc ? 1 : 0; +} + #endif /* NC_ENABLED_SSH_TLS */ API struct timespec From b5025f72e34ea3faf283593dc124e49998f04807 Mon Sep 17 00:00:00 2001 From: Roman Janota Date: Fri, 28 Aug 2026 09:55:10 +0200 Subject: [PATCH 10/10] session server UPDATE revisit config lock timeouts NC_CONFIG_APPLY_LOCK_TIMEOUT was introduced when a reader held the config_lock for a whole transport handshake, so an applier had to wait much longer than any handshake could take. Refcounting the snapshot removed that: every config_lock section is now a pointer read plus a refcount increment, or the pointer swap. So drop NC_CONFIG_LOCK_TIMEOUT to 1000 like every other lock with a constant-time critical section. A longer timeout buys nothing when there is nothing to wait for, it only makes a real deadlock take ten times longer to surface. Use it for the pointer swap in both appliers as well, which is what nc_server_destroy() already did for the very same swap. That leaves the long timeout guarding only the config_update_lock, so rename it accordingly. Its comment claimed it can never fire, which is wrong: the lock is held across the whole apply, including joining the threads of the removed Call Home clients, and such a thread only notices that it should stop once its handshake is over. Say that instead. --- src/server_config.c | 8 ++++---- src/session_p.h | 19 ++++++++++++------- src/session_server.c | 2 +- 3 files changed, 17 insertions(+), 12 deletions(-) diff --git a/src/server_config.c b/src/server_config.c index 22689c8b..66e96982 100644 --- a/src/server_config.c +++ b/src/server_config.c @@ -6125,7 +6125,7 @@ nc_server_config_setup_diff(const struct lyd_node *data) * - avoids concurrent updates * - readers are still allowed to read the old config while we are applying the new one */ - if (nc_mutex_lock(&server_opts.config_update_lock, NC_CONFIG_APPLY_LOCK_TIMEOUT, __func__) != 1) { + if (nc_mutex_lock(&server_opts.config_update_lock, NC_CONFIG_UPDATE_LOCK_TIMEOUT, __func__) != 1) { ERR(NULL, "Timed out waiting for another configuration update to finish, " "the new configuration was not applied."); return 1; @@ -6173,7 +6173,7 @@ nc_server_config_setup_diff(const struct lyd_node *data) #endif /* NC_ENABLED_SSH_TLS */ /* CONFIG WR LOCK - only the pointer swap */ - if (nc_rwlock_lock(&server_opts.config_lock, NC_RWLOCK_WRITE, NC_CONFIG_APPLY_LOCK_TIMEOUT, __func__) != 1) { + if (nc_rwlock_lock(&server_opts.config_lock, NC_RWLOCK_WRITE, NC_CONFIG_LOCK_TIMEOUT, __func__) != 1) { ERR(NULL, "Timed out waiting for the configuration lock, the new configuration was not applied."); ret = 1; goto rollback; @@ -6232,7 +6232,7 @@ nc_server_config_setup_data(const struct lyd_node *data) * - avoids concurrent updates * - readers are still allowed to read the old config while we are applying the new one */ - if (nc_mutex_lock(&server_opts.config_update_lock, NC_CONFIG_APPLY_LOCK_TIMEOUT, __func__) != 1) { + if (nc_mutex_lock(&server_opts.config_update_lock, NC_CONFIG_UPDATE_LOCK_TIMEOUT, __func__) != 1) { ERR(NULL, "Timed out waiting for another configuration update to finish, " "the new configuration was not applied."); return 1; @@ -6286,7 +6286,7 @@ nc_server_config_setup_data(const struct lyd_node *data) #endif /* NC_ENABLED_SSH_TLS */ /* CONFIG WR LOCK - only the pointer swap */ - if (nc_rwlock_lock(&server_opts.config_lock, NC_RWLOCK_WRITE, NC_CONFIG_APPLY_LOCK_TIMEOUT, __func__) != 1) { + if (nc_rwlock_lock(&server_opts.config_lock, NC_RWLOCK_WRITE, NC_CONFIG_LOCK_TIMEOUT, __func__) != 1) { ERR(NULL, "Timed out waiting for the configuration lock, the new configuration was not applied."); ret = 1; goto rollback; diff --git a/src/session_p.h b/src/session_p.h index 414880b5..5e1c763f 100644 --- a/src/session_p.h +++ b/src/session_p.h @@ -161,18 +161,23 @@ extern struct nc_server_opts server_opts; /** * @brief Timeout in msec for acquiring the config_lock - * (only a pointer read plus a refcount increment) + * (only a pointer read plus a refcount increment on the read side, only the pointer swap on the + * write side, so this can never fire unless something is broken) */ -#define NC_CONFIG_LOCK_TIMEOUT 10000 +#define NC_CONFIG_LOCK_TIMEOUT 1000 /** - * @brief Timeout in msec for the locks acquired while applying a new configuration. + * @brief Timeout in msec for acquiring the config_update_lock. * - * The config_lock is only ever held for the pointer swap now, so this can never fire. It is kept as - * a safety net because giving up here means losing the configuration change, which the caller - * generally cannot recover from. + * Unlike the other locks this one is held for a whole configuration apply, which includes joining + * the threads of the removed Call Home clients. A thread with an established session only notices + * that it should stop every ::NC_CH_THREAD_IDLE_TIMEOUT_SLEEP and a thread stuck in a transport + * handshake does not notice at all until the endpoint's auth-timeout elapses, which is configurable + * and unlimited when set to 0. So a legitimate apply can take a long time and this timeout is only + * a last resort - giving up here means losing the configuration change, which the caller generally + * cannot recover from. */ -#define NC_CONFIG_APPLY_LOCK_TIMEOUT 300000 +#define NC_CONFIG_UPDATE_LOCK_TIMEOUT 300000 /** * @brief Timeout in msec for acquiring session's ch_lock diff --git a/src/session_server.c b/src/session_server.c index d437f561..ba94b0e1 100644 --- a/src/session_server.c +++ b/src/session_server.c @@ -1622,7 +1622,7 @@ nc_server_destroy(void) /* CONFIG UPDATE LOCK - the same timeout as the appliers use, destroying the server must not * fail just because a legitimate configuration apply is in progress */ - if (nc_mutex_lock(&server_opts.config_update_lock, NC_CONFIG_APPLY_LOCK_TIMEOUT, __func__) != 1) { + if (nc_mutex_lock(&server_opts.config_update_lock, NC_CONFIG_UPDATE_LOCK_TIMEOUT, __func__) != 1) { rc = 1; goto cleanup; }