From 7ff74d9efe1cd81160282c90d1f75d35ae3ba521 Mon Sep 17 00:00:00 2001 From: Miroslav Lichvar Date: Mon, 11 Sep 2023 15:29:04 +0200 Subject: [PATCH] conf: fix reloading modified sources specified by IP address When reloading a modified source from sourcedir which is ordered before the original source (e.g. maxpoll was decreased), the new source is added before the original one is removed. If the source is specified by IP address, the addition fails due to the conflict with the original source. Sources specified by hostname don't conflict. They are resolved later (repeatedly if the resolver provides only conflicting addresses). Split the processing of sorted source lists into two phases, so all modified sources are removed before they are added again to avoid the conflict. Reported-by: Thomas Lange --- conf.c | 57 ++++++++++++++++++++-------------------- test/system/008-confload | 21 ++++++++++----- 2 files changed, 44 insertions(+), 34 deletions(-) diff --git a/conf.c b/conf.c index f984060..4c1d2e9 100644 --- a/conf.c +++ b/conf.c @@ -1679,7 +1679,7 @@ reload_source_dirs(void) uint32_t *prev_ids, *new_ids; char buf[MAX_LINE_LENGTH]; NSR_Status s; - int d; + int d, pass; prev_size = ARR_GetSize(ntp_source_ids); if (prev_size > 0 && ARR_GetSize(ntp_sources) != prev_size) @@ -1713,36 +1713,37 @@ reload_source_dirs(void) qsort(new_sources, new_size, sizeof (new_sources[0]), compare_sources); - for (i = j = 0; i < prev_size || j < new_size; ) { - if (i < prev_size && j < new_size) - d = compare_sources(&prev_sources[i], &new_sources[j]); - else - d = i < prev_size ? -1 : 1; + for (pass = 0; pass < 2; pass++) { + for (i = j = 0; i < prev_size || j < new_size; i += d <= 0, j += d >= 0) { + if (i < prev_size && j < new_size) + d = compare_sources(&prev_sources[i], &new_sources[j]); + else + d = i < prev_size ? -1 : 1; - if (d < 0) { - /* Remove the missing source */ - if (prev_sources[i].params.name[0] != '\0') + /* Remove missing sources before adding others to avoid conflicts */ + if (pass == 0 && d < 0 && prev_sources[i].params.name[0] != '\0') { NSR_RemoveSourcesById(prev_ids[i]); - i++; - } else if (d > 0) { - /* Add a newly configured source */ - source = &new_sources[j]; - s = NSR_AddSourceByName(source->params.name, source->params.port, source->pool, - source->type, &source->params.params, &new_ids[j]); - - if (s == NSR_UnresolvedName) { - unresolved++; - } else if (s != NSR_Success) { - LOG(LOGS_ERR, "Could not add source %s", source->params.name); - - /* Mark the source as not present */ - source->params.name[0] = '\0'; } - j++; - } else { - /* Keep the existing source */ - new_ids[j] = prev_ids[i]; - i++, j++; + + /* Add new sources */ + if (pass == 1 && d > 0) { + source = &new_sources[j]; + s = NSR_AddSourceByName(source->params.name, source->params.port, source->pool, + source->type, &source->params.params, &new_ids[j]); + + if (s == NSR_UnresolvedName) { + unresolved++; + } else if (s != NSR_Success) { + LOG(LOGS_ERR, "Could not add source %s", source->params.name); + + /* Mark the source as not present */ + source->params.name[0] = '\0'; + } + } + + /* Keep unchanged sources */ + if (pass == 1 && d == 0) + new_ids[j] = prev_ids[i]; } } diff --git a/test/system/008-confload b/test/system/008-confload index a7fc1d5..7e80698 100755 --- a/test/system/008-confload +++ b/test/system/008-confload @@ -30,6 +30,8 @@ echo "server 127.123.4.4" > $TEST_DIR/conf4.d/4.conf echo "server 127.123.5.1" > $TEST_DIR/conf5.d/1.sources echo "server 127.123.5.2" > $TEST_DIR/conf5.d/2.sources echo "server 127.123.5.3" > $TEST_DIR/conf5.d/3.sources +echo "server 127.123.5.4" > $TEST_DIR/conf5.d/4.sources +echo "server 127.123.5.5" > $TEST_DIR/conf5.d/5.sources start_chronyd || test_fail @@ -46,12 +48,16 @@ check_chronyc_output "^[^=]* .. 127\.123\.1\.2 [^^]* .. 127\.123\.5\.1 [^^]* .. 127\.123\.5\.2 [^^]* -.. 127\.123\.5\.3 [^^]*$" || test_fail +.. 127\.123\.5\.3 [^^]* +.. 127\.123\.5\.4 [^^]* +.. 127\.123\.5\.5 [^^]*$" || test_fail rm $TEST_DIR/conf5.d/1.sources -echo "server 127.123.5.2 minpoll 7" > $TEST_DIR/conf5.d/2.sources -echo > $TEST_DIR/conf5.d/3.sources -echo "server 127.123.5.4" > $TEST_DIR/conf5.d/4.sources +echo "server 127.123.5.2 minpoll 5" > $TEST_DIR/conf5.d/2.sources +echo "server 127.123.5.3 minpoll 7" > $TEST_DIR/conf5.d/3.sources +echo > $TEST_DIR/conf5.d/4.sources +echo "server 127.123.5.5" >> $TEST_DIR/conf5.d/5.sources +echo "server 127.123.5.6" > $TEST_DIR/conf5.d/6.sources run_chronyc "reload sources" || test_fail @@ -66,9 +72,12 @@ check_chronyc_output "^[^=]* .. 127\.123\.2\.3 [^^]* .. 127\.123\.4\.4 [^^]* .. 127\.123\.1\.2 *[05] 6 [^^]* -.. 127\.123\.5\.2 *[05] 7 [^^]* -.. 127\.123\.5\.4 [^^]*$" || test_fail +.. 127\.123\.5\.5 [^^]* +.. 127\.123\.5\.2 *[05] 5 [^^]* +.. 127\.123\.5\.3 *[05] 7 [^^]* +.. 127\.123\.5\.6 [^^]*$" || test_fail stop_chronyd || test_fail +check_chronyd_message_count "Could not add source" 1 1 || test_fail test_pass