From 5d974daa271d1241ac3b3c945af8f7d23a86eb34 Mon Sep 17 00:00:00 2001 From: Roman Janota Date: Mon, 24 Aug 2026 20:01:23 +0200 Subject: [PATCH 1/3] server config BUGFIX fix Call Home redispatch nc_server_config_setup_data() rebuilds the configuration from scratch, so Call Home client thread data is NULL even for running clients. Reconciling then dispatched a second thread and leaked the first one's data. Carry running client thread data over to the new configuration by name before deciding whether to dispatch. Also clear the Call Home dispatch data in nc_server_destroy(), its callback data does not have to be valid once the server is destroyed. --- src/server_config.c | 11 +++ src/session_server.c | 3 + tests/test_config.c | 159 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 173 insertions(+) diff --git a/src/server_config.c b/src/server_config.c index db02b8a9..afd925dc 100644 --- a/src/server_config.c +++ b/src/server_config.c @@ -5434,6 +5434,17 @@ 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) { /* already running */ continue; diff --git a/src/session_server.c b/src/session_server.c index f6cfaa83..00ef5bf6 100644 --- a/src/session_server.c +++ b/src/session_server.c @@ -1379,6 +1379,9 @@ nc_server_destroy(void) } server_opts.interactive_auth_data = NULL; server_opts.interactive_auth_data_free = 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); #endif /* NC_ENABLED_SSH_TLS */ /* hidden UNIX socket paths */ diff --git a/tests/test_config.c b/tests/test_config.c index a62b7d10..8ce389ea 100644 --- a/tests/test_config.c +++ b/tests/test_config.c @@ -22,6 +22,7 @@ #include #include #include +#include #include @@ -1049,6 +1050,163 @@ test_invalid_diff(void **state) lyd_free_all(diff); } +/** @brief Time in seconds to wait for a Call Home client to report failed connection attempts. */ +#define TEST_CH_WATCH_TIME 4 + +/** @brief Maximum number of distinct Call Home threads the test keeps track of. */ +#define TEST_CH_TID_MAX 8 + +struct test_ch_threads { + pthread_mutex_t lock; + pthread_cond_t cond; + pthread_t tids[TEST_CH_TID_MAX]; + uint32_t tid_count; +}; + +/* acquire ctx cb for the Call Home dispatch */ +static const struct ly_ctx * +test_ch_acquire_ctx_cb(void *cb_data) +{ + return ((struct ln2_test_ctx *)cb_data)->ctx; +} + +/* release ctx cb for the Call Home dispatch */ +static void +test_ch_release_ctx_cb(void *cb_data) +{ + (void) cb_data; +} + +/* new session cb for the Call Home dispatch, never actually called in these tests */ +static int +test_ch_new_session_cb(const char *client_name, struct nc_session *new_session, void *user_data) +{ + (void) client_name; + (void) new_session; + (void) user_data; + return 1; +} + +/** + * @brief Record the thread of every failed Call Home connection attempt. + * + * Called by the Call Home thread itself, so the number of distinct thread IDs seen is the number + * of Call Home threads running for the client. + */ +static void +test_ch_new_session_fail_cb(const char *client_name, const char *endpt_name, uint8_t max_attempts, + uint8_t cur_attempt, void *user_data) +{ + struct test_ch_threads *threads = user_data; + pthread_t self = pthread_self(); + uint32_t i; + + (void) client_name; + (void) endpt_name; + (void) max_attempts; + (void) cur_attempt; + + pthread_mutex_lock(&threads->lock); + for (i = 0; i < threads->tid_count; ++i) { + if (pthread_equal(threads->tids[i], self)) { + break; + } + } + if ((i == threads->tid_count) && (threads->tid_count < TEST_CH_TID_MAX)) { + threads->tids[threads->tid_count++] = self; + } + pthread_cond_broadcast(&threads->cond); + pthread_mutex_unlock(&threads->lock); +} + +/** + * @brief Create the YANG data of a Call Home client that can never connect anywhere. + * + * @param[in] ctx libyang context. + * @param[in] client_name Name of the Call Home client. + * @param[out] tree Created YANG data. + */ +static void +test_create_ch_client_data(const struct ly_ctx *ctx, const char *client_name, struct lyd_node **tree) +{ + int ret; + + /* port 1 is never listening, so the connection is refused immediately */ + ret = nc_server_config_add_ch_address_port(ctx, client_name, "endpt", NC_TI_SSH, "127.0.0.1", "1", tree); + assert_int_equal(ret, 0); + + ret = nc_server_config_add_ch_persistent(ctx, client_name, tree); + assert_int_equal(ret, 0); + + /* retry quickly so that the test does not have to wait long */ + ret = nc_server_config_add_ch_reconnect_strategy(ctx, client_name, NC_CH_FIRST_LISTED, 1, 3, tree); + assert_int_equal(ret, 0); + + ret = nc_server_config_add_ch_ssh_hostkey(ctx, client_name, "endpt", "hostkey", + TESTS_DIR "/data/key_ecdsa", NULL, tree); + assert_int_equal(ret, 0); + + ret = nc_server_config_add_ch_ssh_user_pubkey(ctx, client_name, "endpt", "user", "pubkey", + TESTS_DIR "/data/id_ed25519.pub", tree); + assert_int_equal(ret, 0); +} + +/** + * @brief Applying the whole configuration data again must not dispatch a second thread + * for an already running Call Home client. + */ +static void +test_ch_dispatch_not_duplicated(void **state) +{ + int ret; + uint32_t tid_count; + struct lyd_node *tree = NULL; + struct ln2_test_ctx *test_ctx = *state; + struct test_ch_threads threads = {0}; + struct timespec ts; + + pthread_mutex_init(&threads.lock, NULL); + pthread_cond_init(&threads.cond, NULL); + + 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); + + test_create_ch_client_data(test_ctx->ctx, "ch", &tree); + + /* dispatch the Call Home client */ + ret = nc_server_config_setup_data(tree); + assert_int_equal(ret, 0); + + /* wait until its thread reports a failed connection attempt */ + pthread_mutex_lock(&threads.lock); + while (!threads.tid_count) { + 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); + + /* apply the very same data again, the client is already running */ + ret = nc_server_config_setup_data(tree); + assert_int_equal(ret, 0); + + /* give a duplicate thread enough time to report a failed connection attempt of its own */ + sleep(TEST_CH_WATCH_TIME); + + pthread_mutex_lock(&threads.lock); + tid_count = threads.tid_count; + pthread_mutex_unlock(&threads.lock); + + /* exactly one Call Home thread may be running for the client */ + assert_int_equal(tid_count, 1); + + lyd_free_all(tree); + pthread_cond_destroy(&threads.cond); + pthread_mutex_destroy(&threads.lock); +} + static void test_config_data_free(void *data) { @@ -1103,6 +1261,7 @@ main(void) cmocka_unit_test_setup_teardown(test_config_cascade_delete, setup_f, ln2_glob_test_teardown), cmocka_unit_test_setup_teardown(test_unusupported_asymkey_format, setup_f, ln2_glob_test_teardown), cmocka_unit_test_setup_teardown(test_invalid_diff, setup_f, ln2_glob_test_teardown), + cmocka_unit_test_setup_teardown(test_ch_dispatch_not_duplicated, setup_f, ln2_glob_test_teardown), }; /* try to get ports from the environment, otherwise use the default */ From 62b23b58568cbc9e85311f7d5ae02cf728643fc0 Mon Sep 17 00:00:00 2001 From: Roman Janota Date: Mon, 24 Aug 2026 20:03:03 +0200 Subject: [PATCH 2/3] session BUGFIX fix config lock timeout handling The config lock is held in read mode during the whole transport handshake (nc_accept(), Call Home thread). SSH/TLS key exchange and authentication have their own timeouts (10s, 30s, or even unlimited), so a reader often holds the lock longer than NC_CONFIG_LOCK_TIMEOUT. Applying a config then timed out on the write lock and failed silently. Wait longer than any handshake when applying a new config and report every timeout. --- src/server_config.c | 17 ++++-- src/session.c | 11 +++- src/session_p.h | 10 ++++ src/session_server.c | 15 +++-- tests/test_config.c | 128 +++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 168 insertions(+), 13 deletions(-) diff --git a/src/server_config.c b/src/server_config.c index afd925dc..b607be94 100644 --- a/src/server_config.c +++ b/src/server_config.c @@ -6182,12 +6182,15 @@ 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_LOCK_TIMEOUT, __func__) != 1) { + if (nc_mutex_lock(&server_opts.config_update_lock, NC_CONFIG_APPLY_LOCK_TIMEOUT, __func__) != 1) { + ERR(NULL, "Timed out waiting for another configuration update to finish, " + "the new configuration was not applied."); return 1; } /* CONFIG RD LOCK */ - if (nc_rwlock_lock(&server_opts.config_lock, NC_RWLOCK_READ, NC_CONFIG_LOCK_TIMEOUT, __func__) != 1) { + if (nc_rwlock_lock(&server_opts.config_lock, NC_RWLOCK_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; } @@ -6218,7 +6221,8 @@ nc_server_config_setup_diff(const struct lyd_node *data) ERR(NULL, "Applying libnetconf2-netconf-server configuration failed."), cleanup); /* CONFIG WR LOCK */ - if (nc_rwlock_lock(&server_opts.config_lock, NC_RWLOCK_WRITE, NC_CONFIG_LOCK_TIMEOUT, __func__) != 1) { + 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; } @@ -6271,7 +6275,9 @@ 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_LOCK_TIMEOUT, __func__) != 1) { + if (nc_mutex_lock(&server_opts.config_update_lock, NC_CONFIG_APPLY_LOCK_TIMEOUT, __func__) != 1) { + ERR(NULL, "Timed out waiting for another configuration update to finish, " + "the new configuration was not applied."); return 1; } @@ -6311,7 +6317,8 @@ nc_server_config_setup_data(const struct lyd_node *data) ERR(NULL, "Applying libnetconf2-netconf-server configuration failed."), cleanup); /* CONFIG LOCK */ - if (nc_rwlock_lock(&server_opts.config_lock, NC_RWLOCK_WRITE, NC_CONFIG_LOCK_TIMEOUT, __func__) != 1) { + 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; } diff --git a/src/session.c b/src/session.c index 0460716d..4e5e13ef 100644 --- a/src/session.c +++ b/src/session.c @@ -468,7 +468,11 @@ nc_rwlock_lock(pthread_rwlock_t *rwlock, enum nc_rwlock_mode mode, int timeout, if (ret) { if ((ret == EBUSY) || (ret == ETIMEDOUT)) { - /* timeout */ + /* timeout, a trylock (no timeout) being busy is a normal outcome, an expired deadline is not */ + if (timeout > 0) { + ERR(NULL, "%s: timed out after %d ms waiting for the rwlock in %s mode.", func_name, timeout, + mode == NC_RWLOCK_READ ? "read" : "write"); + } return 0; } @@ -523,7 +527,10 @@ nc_mutex_lock(pthread_mutex_t *mutex, int timeout, const char *func_name) if (ret) { if ((ret == EBUSY) || (ret == ETIMEDOUT)) { - /* timeout */ + /* timeout, a trylock (no timeout) being busy is a normal outcome, an expired deadline is not */ + if (timeout > 0) { + ERR(NULL, "%s: timed out after %d ms waiting for the mutex.", func_name, timeout); + } return 0; } diff --git a/src/session_p.h b/src/session_p.h index 8f591a73..446e2be0 100644 --- a/src/session_p.h +++ b/src/session_p.h @@ -146,6 +146,16 @@ extern struct nc_server_opts server_opts; */ #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. + */ +#define NC_CONFIG_APPLY_LOCK_TIMEOUT 300000 + /** * @brief Timeout in msec for acquiring session's ch_lock * (just simple session flag checks and updates) diff --git a/src/session_server.c b/src/session_server.c index 00ef5bf6..374f7c44 100644 --- a/src/session_server.c +++ b/src/session_server.c @@ -1341,8 +1341,9 @@ nc_server_destroy(void) } #endif /* NC_ENABLED_SSH_TLS */ - /* CONFIG UPDATE LOCK */ - if (nc_mutex_lock(&server_opts.config_update_lock, NC_CONFIG_LOCK_TIMEOUT, __func__) != 1) { + /* 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) { rc = 1; goto cleanup; } @@ -3885,11 +3886,13 @@ nc_session_server_ch_client_dispatch_stop(struct nc_ch_client *ch_client) goto cleanup; } - /* CONFIG WRITE LOCK - re-acquire to clear the thread pointer and free the thread data */ - if (nc_rwlock_lock(&server_opts.config_lock, NC_RWLOCK_WRITE, NC_CONFIG_LOCK_TIMEOUT, __func__) != 1) { + /* 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 */ - ERRINT; + ERR(NULL, "Timed out waiting for the configuration lock, Call Home client \"%s\" thread data leaked.", + ch_client_name); rc = 1; goto cleanup; } @@ -3909,7 +3912,7 @@ nc_session_server_ch_client_dispatch_stop(struct nc_ch_client *ch_client) 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_LOCK_TIMEOUT, __func__) != 1) { + if (nc_rwlock_lock(&server_opts.config_lock, NC_RWLOCK_WRITE, NC_CONFIG_APPLY_LOCK_TIMEOUT, __func__) != 1) { ERRINT; } } diff --git a/tests/test_config.c b/tests/test_config.c index 8ce389ea..41e61ce9 100644 --- a/tests/test_config.c +++ b/tests/test_config.c @@ -1050,6 +1050,14 @@ test_invalid_diff(void **state) lyd_free_all(diff); } +/** + * @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. + */ +#define TEST_STALL_AUTH_SLEEP 13 + /** @brief Time in seconds to wait for a Call Home client to report failed connection attempts. */ #define TEST_CH_WATCH_TIME 4 @@ -1207,6 +1215,125 @@ test_ch_dispatch_not_duplicated(void **state) pthread_mutex_destroy(&threads.lock); } +/* password callback of the stalling client */ +static char * +test_stall_auth_password(const char *username, const char *hostname, void *priv) +{ + (void) username; + (void) hostname; + (void) priv; + + /* keep the server waiting for the authentication, it holds the configuration READ lock meanwhile */ + sleep(TEST_STALL_AUTH_SLEEP); + + /* a wrong password, the connection is expected to fail */ + return strdup("wrong"); +} + +static void * +test_stall_auth_client_thread(void *arg) +{ + int ret; + struct nc_session *session = NULL; + struct ln2_test_ctx *test_ctx = arg; + + /* skip all hostkey and known_hosts checks */ + nc_client_ssh_set_knownhosts_mode(NC_SSH_KNOWNHOSTS_SKIP); + + ret = nc_client_set_schema_searchpath(MODULES_DIR); + assert_int_equal(ret, 0); + + ret = nc_client_ssh_set_username("stall"); + assert_int_equal(ret, 0); + + nc_client_ssh_set_auth_password_clb(test_stall_auth_password, NULL); + + /* wait for the server to be ready */ + pthread_barrier_wait(&test_ctx->barrier); + + /* the authentication is stalled and then fails */ + session = nc_connect_ssh("127.0.0.1", TEST_PORT, NULL); + assert_null(session); + + return NULL; +} + +static void * +test_stall_auth_server_thread(void *arg) +{ + NC_MSG_TYPE msgtype; + struct nc_session *session = NULL; + struct ln2_test_ctx *test_ctx = arg; + + /* wait for the client to be ready to connect */ + pthread_barrier_wait(&test_ctx->barrier); + + /* the client never authenticates, so this is expected to fail after the stall */ + msgtype = nc_accept(NC_ACCEPT_TIMEOUT, test_ctx->ctx, &session); + assert_int_not_equal(msgtype, NC_MSG_HELLO); + nc_session_free(session, NULL); + + return NULL; +} + +/** + * @brief A configuration update must not be dropped because a session handshake is holding + * the configuration lock. + */ +static void +test_config_update_during_auth(void **state) +{ + int ret, i; + pthread_t tids[2]; + struct lyd_node *tree = NULL, *diff = NULL; + struct ln2_test_ctx *test_ctx = *state; + const struct lys_module *yang_mod; + + yang_mod = ly_ctx_get_module_implemented(test_ctx->ctx, "yang"); + assert_non_null(yang_mod); + + /* a listening SSH endpoint with a password-authenticated user */ + ret = nc_server_config_add_address_port(test_ctx->ctx, "endpt", NC_TI_SSH, "127.0.0.1", TEST_PORT, &tree); + assert_int_equal(ret, 0); + ret = nc_server_config_add_ssh_hostkey(test_ctx->ctx, "endpt", "hostkey", TESTS_DIR "/data/key_ecdsa", + NULL, &tree); + assert_int_equal(ret, 0); + ret = nc_server_config_add_ssh_user_password(test_ctx->ctx, "endpt", "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); + + ret = nc_server_config_setup_data(tree); + assert_int_equal(ret, 0); + + /* prepare the configuration update in advance so that only the lock wait is measured */ + test_create_ch_client_data(test_ctx->ctx, "ch", &diff); + ret = lyd_new_meta(test_ctx->ctx, diff, yang_mod, "operation", "create", 0, NULL); + 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 waiting for the authentication + * while holding the configuration READ lock */ + sleep(2); + + /* this must not be silently dropped */ + ret = nc_server_config_setup_diff(diff); + assert_int_equal(ret, 0); + + for (i = 0; i < 2; i++) { + pthread_join(tids[i], NULL); + } + + lyd_free_all(diff); + lyd_free_all(tree); +} + static void test_config_data_free(void *data) { @@ -1262,6 +1389,7 @@ main(void) cmocka_unit_test_setup_teardown(test_unusupported_asymkey_format, setup_f, ln2_glob_test_teardown), cmocka_unit_test_setup_teardown(test_invalid_diff, setup_f, ln2_glob_test_teardown), cmocka_unit_test_setup_teardown(test_ch_dispatch_not_duplicated, setup_f, ln2_glob_test_teardown), + cmocka_unit_test_setup_teardown(test_config_update_during_auth, setup_f, ln2_glob_test_teardown), }; /* try to get ports from the environment, otherwise use the default */ From 8e8d404e9e8c1fbc47f72fac5c2325e8054f0a7a Mon Sep 17 00:00:00 2001 From: Roman Janota Date: Mon, 24 Aug 2026 20:04:33 +0200 Subject: [PATCH 3/3] server config FEATURE apply ordered-by user order "endpoints/endpoint" and "host-key" are ordered-by user and their order matters - CH endpoints are tried in sequence and the hostkey order decides the advertised algorithm. The order was ignored so far: entries were appended and a delete swapped the last entry into the freed slot. On top of that, moving an entry is reported as a replace in the diff, which no handler covered and left the entry NULL, crashing on its name. Find the existing entry on a replace and place a created or moved entry at the position from the diff "key" metadata, shifting the rest on delete. Also reject unexpected operations in the other find/create handlers. --- src/server_config.c | 218 ++++++++++++++++++++++++++++++++++++++++++-- tests/test_config.c | 191 +++++++++++++++++++++++++++++++++++--- 2 files changed, 392 insertions(+), 17 deletions(-) diff --git a/src/server_config.c b/src/server_config.c index b607be94..33b0f488 100644 --- a/src/server_config.c +++ b/src/server_config.c @@ -21,6 +21,7 @@ #include #include #include +#include #include #include #include @@ -124,6 +125,144 @@ nc_lyd_find_child_optional(const struct lyd_node *ctx_node, const char *child, s lyd_find_path(ctx_node, child, 0, match); } +/** + * @brief Get the instance a created or moved entry of an ordered-by user list should be placed after. + * + * The position is stored in the "key" metadata of the entry as a single key predicate + * (for example "[name='endpt']"), an empty value means the entry belongs first. + * + * @param[in] node List entry from the diff, the list is expected to have exactly one key. + * @param[out] anchor Name of the instance the entry should be placed after, NULL if it belongs first. + * Points into the metadata value, so it is not terminated, use @p anchor_len. + * @param[out] anchor_len Length of @p anchor. + * @param[out] move Whether the diff specifies a position at all. + * @return 0 on success, 1 on error. + */ +static int +nc_server_config_get_userord_anchor(const struct lyd_node *node, const char **anchor, uint32_t *anchor_len, int *move) +{ + struct lyd_meta *meta; + const char *val, *key_end, *val_end; + char quot; + + *anchor = NULL; + *anchor_len = 0; + *move = 0; + + meta = lyd_find_meta(node->meta, NULL, "yang:key"); + if (!meta) { + /* the diff says nothing about the position, for example because it was not created by libyang */ + return 0; + } + + *move = 1; + val = lyd_get_meta_value(meta); + if (!val[0]) { + /* the entry belongs first */ + return 0; + } + + key_end = strchr(val, '='); + if ((val[0] != '[') || !key_end) { + goto error; + } + + /* libyang always quotes the value with a quote it does not contain, so there is no escaping */ + quot = key_end[1]; + if ((quot != '\'') && (quot != '\"')) { + goto error; + } + + val_end = strchr(key_end + 2, quot); + if (!val_end || (val_end[1] != ']')) { + goto error; + } + + *anchor = key_end + 2; + *anchor_len = (uint32_t)(val_end - (key_end + 2)); + return 0; + +error: + ERR(NULL, "Invalid \"key\" metadata value \"%s\" of node \"%s\".", val, LYD_NAME(node)); + return 1; +} + +/** + * @brief Move an entry of an ordered-by user list to the position specified by the diff. + * + * The entries are stored in a sized array in the order they are configured in, so the item is + * moved within the array and the items in between are shifted. If the instance the entry should + * follow is not found, the entry is left where it is. + * + * @param[in] node List entry from the diff, the list is expected to have exactly one key. + * @param[in] index Current index of the entry in @p items. + * @param[in,out] items Sized array of the list entries. + * @param[in] item_size Size of a single item of @p items. + * @param[in] name_offset Offset of the entry name (the list key) within an item of @p items. + * @return 0 on success, 1 on error. + */ +static int +nc_server_config_move_userord_item(const struct lyd_node *node, LY_ARRAY_COUNT_TYPE index, void *items, + uint32_t item_size, uint32_t name_offset) +{ + int move = 0; + const char *anchor = NULL, *name; + uint32_t anchor_len = 0; + LY_ARRAY_COUNT_TYPE count, i, new_index; + char *item, *tmp; + + NC_CHECK_RET(nc_server_config_get_userord_anchor(node, &anchor, &anchor_len, &move)); + if (!move) { + /* keep the current position */ + return 0; + } + + count = LY_ARRAY_COUNT(items); + assert(index < count); + + if (!anchor) { + /* the entry belongs first */ + new_index = 0; + } else { + /* find the instance the entry should follow */ + for (i = 0; i < count; ++i) { + name = *(char **)((char *)items + (i * item_size) + name_offset); + if (!strncmp(name, anchor, anchor_len) && !name[anchor_len]) { + break; + } + } + if (i == count) { + WRN(NULL, "Instance \"%.*s\" to order the node \"%s\" after not found, keeping its position.", + (int)anchor_len, anchor, LYD_NAME(node)); + return 0; + } + + /* the entry is removed from its current position first, which shifts a following anchor down */ + new_index = (i < index) ? i + 1 : i; + } + + if (new_index == index) { + /* already in place */ + return 0; + } + + tmp = malloc(item_size); + NC_CHECK_ERRMEM_RET(!tmp, 1); + + item = (char *)items + (index * item_size); + memcpy(tmp, item, item_size); + if (new_index > index) { + memmove(item, item + item_size, (new_index - index) * item_size); + } else { + memmove((char *)items + ((new_index + 1) * item_size), (char *)items + (new_index * item_size), + (index - new_index) * item_size); + } + memcpy((char *)items + (new_index * item_size), tmp, item_size); + + free(tmp); + return 0; +} + #ifdef NC_ENABLED_SSH_TLS /** @@ -645,6 +784,9 @@ config_local_bind(const struct lyd_node *node, enum nc_operation parent_op, stru 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; } /* config local address */ @@ -1001,8 +1143,9 @@ config_ssh_hostkey(const struct lyd_node *node, enum nc_operation parent_op, str name = lyd_get_value(n); assert(name); - if ((op == NC_OP_DELETE) || (op == NC_OP_NONE)) { - /* find the hostkey we are deleting/modifying */ + if ((op == NC_OP_DELETE) || (op == NC_OP_NONE) || (op == NC_OP_REPLACE)) { + /* find the hostkey we are deleting/modifying/moving, the list is ordered-by user so a moved + * entry is reported as replaced, but its contents do not change */ LY_ARRAY_FOR(ssh->hostkeys, i) { if (!strcmp(ssh->hostkeys[i].name, name)) { break; @@ -1016,6 +1159,10 @@ config_ssh_hostkey(const struct lyd_node *node, enum nc_operation parent_op, str } else if (op == NC_OP_CREATE) { /* create a new hostkey */ LY_ARRAY_NEW_RET(LYD_CTX(node), ssh->hostkeys, hostkey, 1); + i = LY_ARRAY_COUNT(ssh->hostkeys) - 1; + } else { + ERR(NULL, "Unsupported operation of node \"%s\".", LYD_NAME(node)); + return 1; } /* config hostkey name */ @@ -1035,10 +1182,16 @@ config_ssh_hostkey(const struct lyd_node *node, enum nc_operation parent_op, str /* all children processed, we can now delete the hostkey */ if (op == NC_OP_DELETE) { + /* the list is ordered-by user, shift the rest instead of swapping the last hostkey in */ if (i < LY_ARRAY_COUNT(ssh->hostkeys) - 1) { - ssh->hostkeys[i] = ssh->hostkeys[LY_ARRAY_COUNT(ssh->hostkeys) - 1]; + memmove(&ssh->hostkeys[i], &ssh->hostkeys[i + 1], + (LY_ARRAY_COUNT(ssh->hostkeys) - i - 1) * sizeof *ssh->hostkeys); } LY_ARRAY_DECREMENT_FREE(ssh->hostkeys); + } else if ((op == NC_OP_CREATE) || (op == NC_OP_REPLACE)) { + /* the list is ordered-by user, place the hostkey at its configured position */ + NC_CHECK_RET(nc_server_config_move_userord_item(node, i, ssh->hostkeys, + (uint32_t)sizeof *ssh->hostkeys, (uint32_t)offsetof(struct nc_hostkey, name))); } return 0; @@ -1159,6 +1312,9 @@ config_ssh_user_public_key(const struct lyd_node *node, enum nc_operation parent } else if (op == NC_OP_CREATE) { /* create a new public key */ LY_ARRAY_NEW_RET(LYD_CTX(node), user->pubkeys, key, 1); + } else { + ERR(NULL, "Unsupported operation of node \"%s\".", LYD_NAME(node)); + return 1; } /* config public key name */ @@ -1398,6 +1554,9 @@ config_ssh_user(const struct lyd_node *node, enum nc_operation parent_op, struct } else if (op == NC_OP_CREATE) { /* create a new user */ LY_ARRAY_NEW_RET(LYD_CTX(node), ssh->auth_clients, user, 1); + } else { + ERR(NULL, "Unsupported operation of node \"%s\".", LYD_NAME(node)); + return 1; } /* config user name */ @@ -2236,6 +2395,9 @@ config_tls_client_auth_ca_cert(const struct lyd_node *node, } else if (op == NC_OP_CREATE) { /* create a new ca-cert */ LY_ARRAY_NEW_RET(LYD_CTX(node), client_auth->ca_certs, cert, 1); + } else { + ERR(NULL, "Unsupported operation of node \"%s\".", LYD_NAME(node)); + return 1; } /* config ca-cert name */ @@ -2362,6 +2524,9 @@ config_tls_client_auth_ee_cert(const struct lyd_node *node, } else if (op == NC_OP_CREATE) { /* create a new ee-cert */ LY_ARRAY_NEW_RET(LYD_CTX(node), client_auth->ee_certs, cert, 1); + } else { + ERR(NULL, "Unsupported operation of node \"%s\".", LYD_NAME(node)); + return 1; } /* config ee-cert name */ @@ -3246,6 +3411,9 @@ config_unix_user_mapping(const struct lyd_node *node, enum nc_operation parent_o } else if (op == NC_OP_CREATE) { /* create a new user mapping */ LY_ARRAY_NEW_RET(LYD_CTX(node), unix->user_mappings, mapping, 1); + } else { + ERR(NULL, "Unsupported operation of node \"%s\".", LYD_NAME(node)); + return 1; } /* config system-user */ @@ -3385,6 +3553,9 @@ config_endpoint(const struct lyd_node *node, enum nc_operation parent_op, } else if (op == NC_OP_CREATE) { /* create a new endpoint */ LY_ARRAY_NEW_RET(LYD_CTX(node), config->endpts, endpt, 1); + } else { + ERR(NULL, "Unsupported operation of node \"%s\".", LYD_NAME(node)); + return 1; } /* config name */ @@ -3724,8 +3895,9 @@ config_ch_client_endpoint(const struct lyd_node *node, enum nc_operation parent_ name = lyd_get_value(n); assert(name); - if ((op == NC_OP_DELETE) || (op == NC_OP_NONE)) { - /* find the endpoint we are deleting/modifying */ + if ((op == NC_OP_DELETE) || (op == NC_OP_NONE) || (op == NC_OP_REPLACE)) { + /* find the endpoint we are deleting/modifying/moving, the list is ordered-by user so a moved + * entry is reported as replaced, but its contents do not change */ LY_ARRAY_FOR(ch_client->ch_endpts, i) { if (!strcmp(ch_client->ch_endpts[i].name, name)) { break; @@ -3739,6 +3911,10 @@ config_ch_client_endpoint(const struct lyd_node *node, enum nc_operation parent_ } else if (op == NC_OP_CREATE) { /* create a new endpoint and init it */ LY_ARRAY_NEW_RET(LYD_CTX(node), ch_client->ch_endpts, endpt, 1); + i = LY_ARRAY_COUNT(ch_client->ch_endpts) - 1; + } else { + ERR(NULL, "Unsupported operation of node \"%s\".", LYD_NAME(node)); + return 1; } /* config name */ @@ -3760,10 +3936,16 @@ config_ch_client_endpoint(const struct lyd_node *node, enum nc_operation parent_ /* all children processed, we can now delete the endpoint */ if (op == NC_OP_DELETE) { + /* the list is ordered-by user, shift the rest instead of swapping the last endpoint in */ if (i < LY_ARRAY_COUNT(ch_client->ch_endpts) - 1) { - ch_client->ch_endpts[i] = ch_client->ch_endpts[LY_ARRAY_COUNT(ch_client->ch_endpts) - 1]; + memmove(&ch_client->ch_endpts[i], &ch_client->ch_endpts[i + 1], + (LY_ARRAY_COUNT(ch_client->ch_endpts) - i - 1) * sizeof *ch_client->ch_endpts); } LY_ARRAY_DECREMENT_FREE(ch_client->ch_endpts); + } else if ((op == NC_OP_CREATE) || (op == NC_OP_REPLACE)) { + /* the list is ordered-by user, place the endpoint at its configured position */ + NC_CHECK_RET(nc_server_config_move_userord_item(node, i, ch_client->ch_endpts, + (uint32_t)sizeof *ch_client->ch_endpts, (uint32_t)offsetof(struct nc_ch_endpt, name))); } return 0; @@ -4004,6 +4186,9 @@ config_netconf_client(const struct lyd_node *node, enum nc_operation parent_op, } else if (op == NC_OP_CREATE) { /* create a new client */ LY_ARRAY_NEW_RET(LYD_CTX(node), config->ch_clients, ch_client, 1); + } else { + ERR(NULL, "Unsupported operation of node \"%s\".", LYD_NAME(node)); + return 1; } /* config name */ @@ -4233,6 +4418,9 @@ config_asymmetric_key_cert(const struct lyd_node *node, enum nc_operation parent } else if (op == NC_OP_CREATE) { /* create a new certificate */ LY_ARRAY_NEW_RET(LYD_CTX(node), entry->certs, cert, 1); + } else { + ERR(NULL, "Unsupported operation of node \"%s\".", LYD_NAME(node)); + return 1; } /* config certificate name */ @@ -4308,6 +4496,9 @@ config_asymmetric_key(const struct lyd_node *node, enum nc_operation parent_op, } else if (op == NC_OP_CREATE) { /* create a new asymmetric key entry */ LY_ARRAY_NEW_RET(LYD_CTX(node), keystore->entries, entry, 1); + } else { + ERR(NULL, "Unsupported operation of node \"%s\".", LYD_NAME(node)); + return 1; } /* config asymmetric key name */ @@ -4530,6 +4721,9 @@ config_certificate_bag_cert(const struct lyd_node *node, enum nc_operation paren } else if (op == NC_OP_CREATE) { /* create a new certificate */ LY_ARRAY_NEW_RET(LYD_CTX(node), bag->certs, cert, 1); + } else { + ERR(NULL, "Unsupported operation of node \"%s\".", LYD_NAME(node)); + return 1; } /* config certificate name */ @@ -4591,6 +4785,9 @@ config_certificate_bag(const struct lyd_node *node, enum nc_operation parent_op, } else if (op == NC_OP_CREATE) { /* create a new certificate bag */ LY_ARRAY_NEW_RET(LYD_CTX(node), truststore->cert_bags, bag, 1); + } else { + ERR(NULL, "Unsupported operation of node \"%s\".", LYD_NAME(node)); + return 1; } /* config certificate bag name */ @@ -4725,6 +4922,9 @@ config_public_key_bag_pubkey(const struct lyd_node *node, enum nc_operation pare } else if (op == NC_OP_CREATE) { /* create a new public key */ LY_ARRAY_NEW_RET(LYD_CTX(node), bag->pubkeys, pubkey, 1); + } else { + ERR(NULL, "Unsupported operation of node \"%s\".", LYD_NAME(node)); + return 1; } /* config public key name */ @@ -4786,6 +4986,9 @@ config_public_key_bag(const struct lyd_node *node, enum nc_operation parent_op, } else if (op == NC_OP_CREATE) { /* create a new public key bag */ LY_ARRAY_NEW_RET(LYD_CTX(node), truststore->pubkey_bags, bag, 1); + } else { + ERR(NULL, "Unsupported operation of node \"%s\".", LYD_NAME(node)); + return 1; } /* config public key bag name */ @@ -5027,6 +5230,9 @@ config_cert_exp_notif_interval(const struct lyd_node *node, enum nc_operation pa } else if (op == NC_OP_CREATE) { /* create a new interval */ LY_ARRAY_NEW_RET(LYD_CTX(node), config->cert_exp_notif_intervals, interval, 1); + } else { + ERR(NULL, "Unsupported operation of node \"%s\".", LYD_NAME(node)); + return 1; } /* config anchor */ diff --git a/tests/test_config.c b/tests/test_config.c index 41e61ce9..96413d0f 100644 --- a/tests/test_config.c +++ b/tests/test_config.c @@ -1050,6 +1050,78 @@ test_invalid_diff(void **state) lyd_free_all(diff); } +/** + * @brief Moving an entry of an ordered-by user list is reported as a replace operation, + * which must be handled and must not crash. + */ +static void +test_ordered_list_move(void **state) +{ + int ret; + struct lyd_node *tree = NULL, *diff = NULL; + struct ln2_test_ctx *test_ctx = *state; + char *mem = NULL, *mem_filled = NULL; + + /* a diff that only moves entries of the two ordered-by user lists, just like sysrepo + * reports it - the moved list entry is present with its keys only, without the new position */ + const char *move_diff = + "" + " " + " " + " " + " ssh" + " " + " " + " " + " " + " ssh-rsa" + " " + " " + " " + " " + " " + " " + " " + " " + " " + " persistent" + " " + " " + " tls" + " " + " " + " " + " " + "\n"; + + /* read the config file into memory */ + read_config_file(TESTS_DIR "/data/config.xml", &mem); + + /* print the port numbers into the config */ + ret = asprintf(&mem_filled, mem, TEST_PORT_STR, TEST_PORT_2_STR, TEST_PORT_3_STR, + TEST_PORT_4_STR, TEST_PORT_5_STR, TEST_PORT_6_STR); + assert_int_not_equal(ret, -1); + + ret = lyd_parse_data_mem(test_ctx->ctx, mem_filled, LYD_XML, LYD_PARSE_STRICT, LYD_VALIDATE_PRESENT, &tree); + assert_int_equal(ret, 0); + + ret = nc_server_config_setup_data(tree); + assert_int_equal(ret, 0); + + /* the move must be handled, not crash and not fail */ + ret = lyd_parse_data_mem(test_ctx->ctx, move_diff, LYD_XML, LYD_PARSE_ONLY, 0, &diff); + assert_int_equal(ret, 0); + + ret = nc_server_config_setup_diff(diff); + assert_int_equal(ret, 0); + + lyd_free_all(diff); + lyd_free_all(tree); + free(mem); + free(mem_filled); +} + /** * @brief Time in seconds the client stalls in its password callback. * @@ -1069,6 +1141,7 @@ struct test_ch_threads { pthread_cond_t cond; pthread_t tids[TEST_CH_TID_MAX]; uint32_t tid_count; + char endpt[64]; }; /* acquire ctx cb for the Call Home dispatch */ @@ -1110,11 +1183,14 @@ test_ch_new_session_fail_cb(const char *client_name, const char *endpt_name, uin uint32_t i; (void) client_name; - (void) endpt_name; (void) max_attempts; (void) cur_attempt; pthread_mutex_lock(&threads->lock); + if (!threads->endpt[0]) { + /* the endpoint of the very first failed attempt is the first one in the configuration */ + strncpy(threads->endpt, endpt_name, sizeof threads->endpt - 1); + } for (i = 0; i < threads->tid_count; ++i) { if (pthread_equal(threads->tids[i], self)) { break; @@ -1128,34 +1204,51 @@ test_ch_new_session_fail_cb(const char *client_name, const char *endpt_name, uin } /** - * @brief Create the YANG data of a Call Home client that can never connect anywhere. + * @brief Create the YANG data of a Call Home endpoint that can never connect anywhere. * * @param[in] ctx libyang context. * @param[in] client_name Name of the Call Home client. + * @param[in] endpt_name Name of the Call Home endpoint. * @param[out] tree Created YANG data. */ static void -test_create_ch_client_data(const struct ly_ctx *ctx, const char *client_name, struct lyd_node **tree) +test_create_ch_endpt_data(const struct ly_ctx *ctx, const char *client_name, const char *endpt_name, + struct lyd_node **tree) { int ret; /* port 1 is never listening, so the connection is refused immediately */ - ret = nc_server_config_add_ch_address_port(ctx, client_name, "endpt", NC_TI_SSH, "127.0.0.1", "1", tree); + ret = nc_server_config_add_ch_address_port(ctx, client_name, endpt_name, NC_TI_SSH, "127.0.0.1", "1", tree); assert_int_equal(ret, 0); - ret = nc_server_config_add_ch_persistent(ctx, client_name, tree); + ret = nc_server_config_add_ch_ssh_hostkey(ctx, client_name, endpt_name, "hostkey", + TESTS_DIR "/data/key_ecdsa", NULL, tree); assert_int_equal(ret, 0); - /* retry quickly so that the test does not have to wait long */ - ret = nc_server_config_add_ch_reconnect_strategy(ctx, client_name, NC_CH_FIRST_LISTED, 1, 3, tree); + ret = nc_server_config_add_ch_ssh_user_pubkey(ctx, client_name, endpt_name, "user", "pubkey", + TESTS_DIR "/data/id_ed25519.pub", tree); assert_int_equal(ret, 0); +} - ret = nc_server_config_add_ch_ssh_hostkey(ctx, client_name, "endpt", "hostkey", - TESTS_DIR "/data/key_ecdsa", NULL, tree); +/** + * @brief Create the YANG data of a Call Home client that can never connect anywhere. + * + * @param[in] ctx libyang context. + * @param[in] client_name Name of the Call Home client. + * @param[out] tree Created YANG data. + */ +static void +test_create_ch_client_data(const struct ly_ctx *ctx, const char *client_name, struct lyd_node **tree) +{ + int ret; + + test_create_ch_endpt_data(ctx, client_name, "endpt", tree); + + ret = nc_server_config_add_ch_persistent(ctx, client_name, tree); assert_int_equal(ret, 0); - ret = nc_server_config_add_ch_ssh_user_pubkey(ctx, client_name, "endpt", "user", "pubkey", - TESTS_DIR "/data/id_ed25519.pub", tree); + /* retry quickly so that the test does not have to wait long */ + ret = nc_server_config_add_ch_reconnect_strategy(ctx, client_name, NC_CH_FIRST_LISTED, 1, 3, tree); assert_int_equal(ret, 0); } @@ -1215,6 +1308,80 @@ test_ch_dispatch_not_duplicated(void **state) pthread_mutex_destroy(&threads.lock); } +/** + * @brief A moved entry of an ordered-by user list must actually change its position. + * + * The Call Home thread always starts with the first endpoint of the client, so the endpoint of the + * first failed connection attempt tells which one that is. + */ +static void +test_ch_endpoint_order(void **state) +{ + int ret; + struct lyd_node *tree = NULL, *diff = NULL; + struct ln2_test_ctx *test_ctx = *state; + struct test_ch_threads threads = {0}; + struct timespec ts; + + /* move the second endpoint to the front, an empty "key" means the entry belongs first */ + const char *move_diff = + "" + " " + " " + " ch" + " " + " " + " second" + " " + " " + " " + " " + "\n"; + + pthread_mutex_init(&threads.lock, NULL); + pthread_cond_init(&threads.cond, NULL); + + /* two endpoints, "first" is listed first */ + test_create_ch_endpt_data(test_ctx->ctx, "ch", "first", &tree); + test_create_ch_endpt_data(test_ctx->ctx, "ch", "second", &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, 1, &tree); + assert_int_equal(ret, 0); + + ret = nc_server_config_setup_data(tree); + assert_int_equal(ret, 0); + + ret = lyd_parse_data_mem(test_ctx->ctx, move_diff, LYD_XML, LYD_PARSE_ONLY, 0, &diff); + assert_int_equal(ret, 0); + ret = nc_server_config_setup_diff(diff); + assert_int_equal(ret, 0); + + /* dispatch the client only now, so that it starts with the reordered endpoints */ + nc_server_ch_set_new_session_fail_cb(test_ch_new_session_fail_cb, &threads); + ret = nc_connect_ch_client_dispatch("ch", test_ch_acquire_ctx_cb, test_ch_release_ctx_cb, test_ctx, + test_ch_new_session_cb, NULL); + assert_int_equal(ret, 0); + + /* wait for the first failed connection attempt */ + pthread_mutex_lock(&threads.lock); + while (!threads.endpt[0]) { + clock_gettime(CLOCK_REALTIME, &ts); + ts.tv_sec += 10; + ret = pthread_cond_timedwait(&threads.cond, &threads.lock, &ts); + assert_int_equal(ret, 0); + } + assert_string_equal(threads.endpt, "second"); + pthread_mutex_unlock(&threads.lock); + + lyd_free_all(diff); + lyd_free_all(tree); + pthread_cond_destroy(&threads.cond); + pthread_mutex_destroy(&threads.lock); +} + /* password callback of the stalling client */ static char * test_stall_auth_password(const char *username, const char *hostname, void *priv) @@ -1388,7 +1555,9 @@ main(void) cmocka_unit_test_setup_teardown(test_config_cascade_delete, setup_f, ln2_glob_test_teardown), cmocka_unit_test_setup_teardown(test_unusupported_asymkey_format, setup_f, ln2_glob_test_teardown), cmocka_unit_test_setup_teardown(test_invalid_diff, setup_f, ln2_glob_test_teardown), + cmocka_unit_test_setup_teardown(test_ordered_list_move, setup_f, ln2_glob_test_teardown), 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), };