From ef6e562cdebbf82bbb89431c75e5de49566a377a Mon Sep 17 00:00:00 2001 From: Rongsong Shen Date: Sat, 3 Oct 2026 13:06:58 +0000 Subject: [PATCH] fix: close controller-peers-env-fallback validation gaps (bug-1909..1912) - bug-1912: xce_parse_peers() ignores whitespace around entries and skips blank ones, matching xceadmin's resolve_controllers(), so "a=h:1, b=h:2" names the second candidate "b" (5 libxce tests + regression case). - bug-1911: xcecd frees the -l log dir before the -h / --version returns (regression case: no LeakSanitizer report, exit 0). - bug-1910: both version scripts build the apiserver with -mod=vendor (as the meson build does) instead of -mod=readonly, so they no longer need a Go module cache. - bug-1909: test-version-startup-consistency.sh accepts exactly the dirty-content-fingerprint pairing ('-<8 hex>' in --version with '-dirty' in the startup log), with self-test cases (e)/(f). Both version scripts now exit 0 on a dirty tree with XCE_CONTROLLER_PEERS set; regression script 35/35; xce-build all: C 127/0, scheduler 16/0, xceadmin 364 passed, no ASAN reports. Co-Authored-By: Claude Opus 5.5 (1M context) --- src/include/xce/cluster.h | 5 +- src/lib/cluster.c | 14 +++++ src/lib/testsuite/test_cluster.c | 62 +++++++++++++++++++ ...dirty-fingerprint-startup-log-unchanged.sh | 4 +- src/tools/test-version-startup-consistency.sh | 53 +++++++++++++--- src/tools/verify-controller-peers-env.sh | 28 +++++++++ src/worker/src/main.c | 2 + 7 files changed, 159 insertions(+), 9 deletions(-) diff --git a/src/include/xce/cluster.h b/src/include/xce/cluster.h index d4759384..61399a3a 100644 --- a/src/include/xce/cluster.h +++ b/src/include/xce/cluster.h @@ -129,7 +129,10 @@ void xce_reconnect_fini(xce_reconnect_ctx_t* rctx); /** * xce_parse_peers() — parse "--peers name=host:port,..." into cfg->candidates[]. * - * Each token may be "name=host:port" or bare "host:port". + * Each token may be "name=host:port" or bare "host:port". Whitespace around + * each comma-separated token is ignored and tokens that are blank after + * trimming are skipped (they do not use a MAX_HA_CANDIDATES slot), matching + * xceadmin's resolve_controllers() (bug-1912). * The name field is stored when present; it allows multiple daemons on the * same host to identify themselves by logical name rather than IP address. * diff --git a/src/lib/cluster.c b/src/lib/cluster.c index c963a0da..8b982bba 100644 --- a/src/lib/cluster.c +++ b/src/lib/cluster.c @@ -11,6 +11,7 @@ * Backoff sleep is skipped when s_connect_fn is set (i.e., in unit tests) * so tests do not need to wait for real timeouts. */ +#include #include #include #include @@ -35,6 +36,19 @@ int xce_parse_peers(const char* peers_str, xce_cluster_config_t* cfg) char* saveptr; char* tok = strtok_r(buf, ",", &saveptr); while (tok && cfg->candidates_count < MAX_HA_CANDIDATES) { + /* bug-1912: ignore whitespace around each entry and skip entries + * that are blank after trimming, as xceadmin's + * resolve_controllers() does, so "a=h:1, b=h:2" names "b". */ + while (isspace((unsigned char)*tok)) + tok++; + char* end = tok + strlen(tok); + while (end > tok && isspace((unsigned char)end[-1])) + *--end = '\0'; + if (*tok == '\0') { + tok = strtok_r(NULL, ",", &saveptr); + continue; + } + /* Format: name=host:port OR host:port */ char* eq = strchr(tok, '='); char* addr = eq ? eq + 1 : tok; diff --git a/src/lib/testsuite/test_cluster.c b/src/lib/testsuite/test_cluster.c index 1f23a19b..4ec4b0af 100644 --- a/src/lib/testsuite/test_cluster.c +++ b/src/lib/testsuite/test_cluster.c @@ -420,6 +420,63 @@ static void test_find_identity_empty_name_matches_empty_hostname(void) printf("test_find_identity_empty_name_matches_empty_hostname: PASS\n"); } +/* bug-1912: xce_parse_peers() trims whitespace around each comma-separated + * entry and skips entries that are blank after trimming, matching xceadmin's + * resolve_controllers(), so "a=h:1, b=h:2" names the second candidate "b". */ +static void test_parse_peers_trims_spaces_around_entries(void) +{ + xce_cluster_config_t cfg = {0}; + TEST_ASSERT_EQUAL_INT(0, xce_parse_peers("a=h1:1, b=h2:2 ,\tc=h3:3 ", &cfg)); + TEST_ASSERT_EQUAL_INT(3, cfg.candidates_count); + TEST_ASSERT_EQUAL_STRING("a", cfg.candidates[0].name); + TEST_ASSERT_EQUAL_STRING("b", cfg.candidates[1].name); + TEST_ASSERT_EQUAL_STRING("c", cfg.candidates[2].name); + TEST_ASSERT_EQUAL_STRING("h1", cfg.candidates[0].hostname); + TEST_ASSERT_EQUAL_STRING("h2", cfg.candidates[1].hostname); + TEST_ASSERT_EQUAL_STRING("h3", cfg.candidates[2].hostname); + TEST_ASSERT_EQUAL_INT(1, cfg.candidates[0].port); + TEST_ASSERT_EQUAL_INT(2, cfg.candidates[1].port); + TEST_ASSERT_EQUAL_INT(3, cfg.candidates[2].port); + printf("test_parse_peers_trims_spaces_around_entries: PASS\n"); +} + +static void test_parse_peers_trims_unnamed_entry(void) +{ + xce_cluster_config_t cfg = {0}; + TEST_ASSERT_EQUAL_INT(0, xce_parse_peers(" localhost:9991 ", &cfg)); + TEST_ASSERT_EQUAL_INT(1, cfg.candidates_count); + TEST_ASSERT_EQUAL_STRING("", cfg.candidates[0].name); + TEST_ASSERT_EQUAL_STRING("localhost", cfg.candidates[0].hostname); + TEST_ASSERT_EQUAL_INT(9991, cfg.candidates[0].port); + printf("test_parse_peers_trims_unnamed_entry: PASS\n"); +} + +static void test_parse_peers_skips_blank_entries(void) +{ + xce_cluster_config_t cfg = {0}; + TEST_ASSERT_EQUAL_INT(0, xce_parse_peers("a=h1:1, ,b=h2:2", &cfg)); + TEST_ASSERT_EQUAL_INT(2, cfg.candidates_count); + TEST_ASSERT_EQUAL_STRING("a", cfg.candidates[0].name); + TEST_ASSERT_EQUAL_STRING("b", cfg.candidates[1].name); + printf("test_parse_peers_skips_blank_entries: PASS\n"); +} + +static void test_parse_peers_all_blank_rejected(void) +{ + xce_cluster_config_t cfg = {0}; + TEST_ASSERT_EQUAL_INT(-1, xce_parse_peers(" , \t ", &cfg)); + TEST_ASSERT_EQUAL_INT(0, cfg.candidates_count); + printf("test_parse_peers_all_blank_rejected: PASS\n"); +} + +/* Unchanged behavior: an entry without a port is still a parse error. */ +static void test_parse_peers_entry_without_port_rejected(void) +{ + xce_cluster_config_t cfg = {0}; + TEST_ASSERT_EQUAL_INT(-1, xce_parse_peers("a=h1:1, b=h2", &cfg)); + printf("test_parse_peers_entry_without_port_rejected: PASS\n"); +} + /* ── Test group entry point ─────────────────────────────────────── */ void test_cluster(void) { @@ -450,4 +507,9 @@ void test_cluster(void) RUN_TEST(test_find_identity_first_duplicate_wins); RUN_TEST(test_find_identity_null_name); RUN_TEST(test_find_identity_empty_name_matches_empty_hostname); + RUN_TEST(test_parse_peers_trims_spaces_around_entries); + RUN_TEST(test_parse_peers_trims_unnamed_entry); + RUN_TEST(test_parse_peers_skips_blank_entries); + RUN_TEST(test_parse_peers_all_blank_rejected); + RUN_TEST(test_parse_peers_entry_without_port_rejected); } diff --git a/src/tools/test-dirty-fingerprint-startup-log-unchanged.sh b/src/tools/test-dirty-fingerprint-startup-log-unchanged.sh index e2c882bd..b36c0717 100755 --- a/src/tools/test-dirty-fingerprint-startup-log-unchanged.sh +++ b/src/tools/test-dirty-fingerprint-startup-log-unchanged.sh @@ -517,13 +517,15 @@ fi yellow "=== [apiserver] go build (direct invocation, see header comment re: bug-1597 / go_build's goproxy.cn hang in this sandbox) -- commit_id=$API_COMMIT_ID build_time=$API_BUILD_TIME dirty_fingerprint=$API_DIRTY_FINGERPRINT ===" APISERVER_BIN="$APILIB_DIR/frontend/apiserver/xce_apiserver" mkdir -p "$(dirname "$APISERVER_BIN")" +# bug-1910: -mod=vendor builds from the committed vendor/ tree (as the +# canonical meson build does), so no Go module cache is needed. nix develop "$ISO_SRC" --command bash -c " set -e cd '$ISO_SRC/frontend/apiserver' CGO_CFLAGS=\"-g \$(pkg-config --cflags librtti c-capnproto openssl tinylog) -I$ISO_SRC/include -I$APILIB_DIR/lib\" CGO_LDFLAGS=\"-g \$(pkg-config --libs librtti c-capnproto openssl tinylog) -lm -lrt -L$APILIB_DIR/lib -lxce\" GOPROXY=off CGO_ENABLED=1 CGO_CFLAGS=\"\$CGO_CFLAGS\" CGO_LDFLAGS=\"\$CGO_LDFLAGS\" \ - go build -mod=readonly -gcflags 'all=-N -l' -ldflags \"-X main.GitCommit=$API_COMMIT_ID -X main.BuildTime=$API_BUILD_TIME -X main.DirtyFingerprint=$API_DIRTY_FINGERPRINT\" \ + go build -mod=vendor -gcflags 'all=-N -l' -ldflags \"-X main.GitCommit=$API_COMMIT_ID -X main.BuildTime=$API_BUILD_TIME -X main.DirtyFingerprint=$API_DIRTY_FINGERPRINT\" \ -o $APISERVER_BIN ./cmd/main.go " diff --git a/src/tools/test-version-startup-consistency.sh b/src/tools/test-version-startup-consistency.sh index 6cbbaaa3..4a99a492 100755 --- a/src/tools/test-version-startup-consistency.sh +++ b/src/tools/test-version-startup-consistency.sh @@ -39,7 +39,10 @@ # so this script extracts just those two field values from each capture # and asserts the extracted VALUES are byte-identical between --version # and the startup log, for each binary. Surrounding text differing is -# expected and not a failure. +# expected and not a failure. One sanctioned exception (bug-1909): on a +# dirty tree the dirty-content-fingerprint feature shows +# '-<8-hex fingerprint>' in --version while the startup log keeps +# '-dirty'; check_consistency() accepts exactly that pairing. # # Build path notes: # - xce_scheduler is built via the exact same real `/xce-build`-equivalent @@ -63,11 +66,12 @@ # `go build` directly with the same -ldflags shape go_build/Makefile # use and the same CGO_CFLAGS/CGO_LDFLAGS pkg-config derivation # test-backend/apiserver's meson.build already use, against the -# default (already-populated) module cache -- functionally the same +# committed vendor/ tree -- functionally the same # real build (same source, same compiler, same ldflags injection # mechanism), just not routed through the specific wrapper script that # hangs in this sandbox. Neither go_build nor the Makefile is modified. -# `go build` is invoked with `-mod=readonly` explicitly (confirmed live: +# `go build` was originally invoked with `-mod=readonly` (superseded by +# `-mod=vendor`, see the bug-1910 note below; confirmed live at the time: # without it, this build silently ADDED ~65 unrelated indirect- # dependency /go.mod checksum lines to the committed # src/frontend/apiserver/go.sum as a side effect of resolving the full @@ -78,6 +82,13 @@ # check" constraint requires. The build still succeeds under # -mod=readonly against this repo's current go.sum, so nothing was # actually missing -- the extra entries were superfluous). +# bug-1910: `-mod=readonly` made Go ignore the committed +# src/frontend/apiserver/vendor/ tree and need a populated module +# cache, so with GOPROXY=off the build failed on any machine without +# one. The build now uses `-mod=vendor`, as the canonical meson build +# does (GOFLAGS=-mod=vendor in src/frontend/apiserver/meson.build); +# vendor mode never touches go.mod or go.sum either, so the bug-1607 +# no-repo-mutation property still holds. # # Usage: # bash src/tools/test-version-startup-consistency.sh [--keep-artifacts] @@ -216,7 +227,17 @@ extract_zap_fields() { # behaviour. check_consistency() { local label="$1" v_commit="$2" v_build="$3" l_commit="$4" l_build="$5" - local ok=1 + local ok=1 v_shown="$2" + + # bug-1909: dirty-content-fingerprint (requirements.md, scope note and + # 2.2) deliberately shows '-<8-hex fingerprint>' in --version on a + # dirty tree while the startup log keeps '-dirty'. Treat exactly + # that pairing -- same base commit, '-dirty' in the log -- as agreement. + if [[ "$v_commit" =~ ^(.+)-[0-9a-f]{8}$ ]] \ + && [ "$l_commit" = "${BASH_REMATCH[1]}-dirty" ]; then + yellow "[$label] --version fingerprint '$v_commit' pairs with startup log '$l_commit' (dirty-content-fingerprint)" + v_commit="$l_commit" + fi if [ -z "$v_commit" ] || [ -z "$l_commit" ]; then red "FAIL [$label]: commit id missing from one or both captures (--version='$v_commit' startup-log='$l_commit')" @@ -235,7 +256,7 @@ check_consistency() { fi if [ "$ok" = 1 ]; then - green "PASS [$label]: --version and startup log agree -- commit='$v_commit' build_time='$v_build'" + green "PASS [$label]: --version and startup log agree -- commit='$v_shown' (startup log '$l_commit') build_time='$v_build'" return 0 fi return 1 @@ -275,6 +296,24 @@ demonstrate_detection_capability() { fi green "Confirmed: an empty field on one side is correctly rejected." + yellow "--- (e) dirty tree: --version '-<8-hex fingerprint>' with startup log '-dirty' is the documented dirty-content-fingerprint pairing and must PASS ---" + if ! check_consistency "DEMO-dirty-fingerprint-pairing" "a1b2c3d4-0f1e2d3c" "$real_build" "a1b2c3d4-dirty" "$real_build"; then + red "DEMONSTRATION FAILED: incorrectly FAILED the documented dirty-fingerprint pairing." + exit 1 + fi + green "Confirmed: the documented dirty-fingerprint pairing is accepted." + + yellow "--- (f) the pairing must not hide a real mismatch: different base commit, or a fingerprint with a clean log ---" + if check_consistency "DEMO-dirty-pairing-other-sha" "a1b2c3d4-0f1e2d3c" "$real_build" "deadbeef-dirty" "$real_build"; then + red "DEMONSTRATION FAILED: incorrectly PASSED a fingerprint against a different commit's -dirty." + exit 1 + fi + if check_consistency "DEMO-fingerprint-clean-log" "a1b2c3d4-0f1e2d3c" "$real_build" "a1b2c3d4" "$real_build"; then + red "DEMONSTRATION FAILED: incorrectly PASSED a fingerprint against a clean startup-log commit." + exit 1 + fi + green "Confirmed: the pairing is accepted only for the same base commit with '-dirty' in the log." + yellow "--- (d) sanity: genuinely matching real values must PASS ---" if ! check_consistency "DEMO-sanity-real-match" "$real_commit" "$real_build" "$real_commit" "$real_build"; then red "DEMONSTRATION FAILED: incorrectly FAILED genuinely matching real values." @@ -386,7 +425,7 @@ cd '$SRC_DIR/frontend/apiserver' CGO_CFLAGS=\"-g \$(pkg-config --cflags librtti c-capnproto openssl tinylog) -I$SRC_DIR/include -I$APILIB_DIR/lib\" CGO_LDFLAGS=\"-g \$(pkg-config --libs librtti c-capnproto openssl tinylog) -lm -lrt -L$APILIB_DIR/lib -lxce\" GOPROXY=off CGO_ENABLED=1 CGO_CFLAGS=\"\$CGO_CFLAGS\" CGO_LDFLAGS=\"\$CGO_LDFLAGS\" \ - go build -mod=readonly -gcflags 'all=-N -l' -ldflags \"-X main.GitCommit=$API_COMMIT_ID -X main.BuildTime=$API_BUILD_TIME\" \ + go build -mod=vendor -gcflags 'all=-N -l' -ldflags \"-X main.GitCommit=$API_COMMIT_ID -X main.BuildTime=$API_BUILD_TIME\" \ -o $APISERVER_BIN ./cmd/main.go " @@ -502,7 +541,7 @@ else fi if [ "$overall_pass" = 1 ]; then - green "=== ALL CHECKS PASS: xce_scheduler and xce_apiserver each report byte-identical commit id + build time between --version/-version and their startup log ===" + green "=== ALL CHECKS PASS: xce_scheduler and xce_apiserver each report the same commit id (or the sanctioned dirty-fingerprint pairing) and byte-identical build time between --version/-version and their startup log ===" exit 0 else red "=== INCONSISTENCY DETECTED: see FAIL line(s) above ===" diff --git a/src/tools/verify-controller-peers-env.sh b/src/tools/verify-controller-peers-env.sh index 4e32ab57..a8ec25f7 100755 --- a/src/tools/verify-controller-peers-env.sh +++ b/src/tools/verify-controller-peers-env.sh @@ -498,6 +498,20 @@ case_controller_env_own_name_present() { add_case case_controller_env_own_name_present \ "controller: env list containing the own name is accepted (1.6)" +# bug-1912: spaces after the commas (natural in a shell variable) must not +# become part of a candidate's name, so --myname b still matches " b=...". +case_controller_env_spaced_list() { + local d; d="$(new_scratch controller-env-spaced)" + run_controller "$d" \ + "XCE_CONTROLLER_PEERS=a=localhost:1, b=localhost:2, c=localhost:3" \ + -- --myname b + _assert_stopped_early "$d" || return 1 + assert_grep "$d/controller.log" 'HA mode: 3 controller candidates, identity=b' || return 1 + assert_no_grep "$d/stderr" "$OWN_NAME_RE" "stderr" || return 1 +} +add_case case_controller_env_spaced_list \ + "controller: spaces around env entries are ignored (bug-1912)" + case_controller_flag_own_name_unchecked() { local d; d="$(new_scratch controller-own-flag)" run_controller "$d" -u XCE_CONTROLLER_PEERS -- --myname zz --peers "$ENV3" @@ -798,6 +812,20 @@ case_worker_help_names_variable() { add_case case_worker_help_names_variable \ "worker: -h documents XCE_CONTROLLER_PEERS, the opt-out and that sudo drops it (1.5, 1.7)" +# bug-1911: -h and --version after -l must exit 0 without a LeakSanitizer +# report (the -l string used to leak on those early returns). +case_worker_help_version_after_logdir_no_leak() { + local d opt + for opt in -h --version; do + d="$(new_scratch "worker-early-exit${opt}")" + run_worker "$d" -u XCE_CONTROLLER_PEERS -- "$opt" + assert_rc 0 || return 1 + assert_no_grep "$d/stderr" 'LeakSanitizer' "stderr ($opt after -l)" || return 1 + done +} +add_case case_worker_help_version_after_logdir_no_leak \ + "worker: -h / --version after -l exit 0 without a leak (bug-1911)" + # ── Task 3.3 (Req 1.1-1.5, 1.7): the harness (xce_test) takes its `masters` # default from XCE_CONTROLLER_PEERS; an explicit `set masters` wins. Always # run with --norc so the developer's own rc file cannot change the result, diff --git a/src/worker/src/main.c b/src/worker/src/main.c index 420ecfc5..1edf009f 100644 --- a/src/worker/src/main.c +++ b/src/worker/src/main.c @@ -438,6 +438,7 @@ int main(int argc, char* argv[]) * version_buf -- the startup INFO log lines further down keep * reading the original version_buf, untouched. */ printf("%s\n", version_display_buf); + FREEUP(logdir); /* bug-1911: -l may have allocated it */ return 0; case 'h': default: @@ -460,6 +461,7 @@ int main(int argc, char* argv[]) " preserved, e.g. " "sudo --preserve-env=XCE_CONTROLLER_PEERS xcecd ...\n", argv[0]); + FREEUP(logdir); /* bug-1911: -l may have allocated it */ return 0; } }