From ca4db13a799525e279a9722ff95555665b6c08d7 Mon Sep 17 00:00:00 2001 From: Rongsong Shen Date: Thu, 24 Sep 2026 08:41:55 +0000 Subject: [PATCH] fix(controller): string resvals own their bytes -- rejoin use-after-free killed new primary (bug-1851) update_val_res() memcpy'd the xce_val_t into w->host.resvals, so the "type"/"model" string resources aliased the interned xstrings owned by w->host.attrs. rejoin_worker() drops the old attrs strings, then apply_join_state() -> resync_arm_round() resolves the previous join's still-outstanding round, firing the worker hooks before add_hostattrs_resvals() re-seeds resvals; the apiserver hook's collect_resval() strdup()ed the freed hostmodel. Live: a newly promoted primary died ~8s after election during an HA soak. A string resval now stores a private strdup (taken before the old copy is freed), and the owning arrays free their entries: host.resvals gets kill_resval (replacing the FIXME NULL killer) and c->sharedinst's resval_obj_killer (previously a no-op) frees the string and the value -- both also leaked every xce_val_t before. Also closes the same alias for worker-reported string resvals and config values. Regression test: test_rejoin_worker_string_resvals_survive_outstanding_round (RED reproduced the live ASAN stack; GREEN after). meson test 109/109. Co-Authored-By: Claude Opus 5.5 (1M context) --- .wolf/buglog.json | 17 +++ src/controller/cluster.c | 10 +- src/controller/testsuite/test_worker_join.c | 134 ++++++++++++++++++++ src/controller/worker.c | 24 +++- src/lib/wobj.c | 18 ++- 5 files changed, 199 insertions(+), 4 deletions(-) diff --git a/.wolf/buglog.json b/.wolf/buglog.json index 83cd9327..ca07a6b9 100644 --- a/.wolf/buglog.json +++ b/.wolf/buglog.json @@ -25170,6 +25170,23 @@ "ha", "scheduler" ] + }, + { + "id": "bug-1851", + "timestamp": "2026-09-24T07:40:00Z", + "error_message": "Live sequential HA soak (run-ha-soak.sh --sequential 6, round 5): newly-promoted primary ctrl-3 (term 10) died ~8s after promotion with ASAN heap-use-after-free READ in strdup <- collect_resval (dataex.c:71) <- apiserver_worker_change (apiserver.c:450) <- apiserver_worker_hook_fun <- xce_run_hooks <- persist_notify_worker_status_updated (persist.c:7997) <- resync_resolve_round (resync.c:418) <- resync_arm_round (resync.c:315) <- apply_join_state (worker.c:351) <- rejoin_worker (worker.c:444) <- handle_worker_join. Freed by the same thread in rejoin_worker (worker.c:434, xstring_dereference of the old hostmodel). Soak surfaced it as 'FAIL: round 5: no new primary elected after kill (no serving primary at term>9)' -- the new primary crashed rather than failing to elect.", + "file": "src/controller/worker.c", + "root_cause": "update_val_res() (worker.c:~99) memcpy()s the xce_val_t into w->host.resvals, so the 'type'/'model' string resources seeded by add_hostattrs_resvals() ALIAS the interned xstrings owned by w->host.attrs->hosttype/hostmodel (no reference taken). rejoin_worker() dereferences (frees, at refcount 0 -- even when the new value is the same string, since it dereferences before makeString) the old hosttype/hostmodel, then apply_join_state() calls resync_arm_round() BEFORE add_hostattrs_resvals() re-seeds resvals. When the resync round resolves synchronously inside arm (resync_resolve_round -> persist_notify_worker_status_updated -> worker hooks), apiserver_worker_change -> collect_resval strdup()s the dangling svalue. Latent since the alias was introduced; made reachable by worker-rejoin-resync's synchronous resolve+notify path on rejoin (bug-1841 area).", + "fix": "FIXED (2026-09-24, TDD). update_val_res() (worker.c) now stores a private strdup() for Value_u_svalue (copy taken before freeing the previous one, so val may alias v) and frees an old string when a slot changes type; the owning arrays now free their entries: w->host.resvals gets kill_resval (lib/wobj.c, replacing the FIXME NULL killer) and c->sharedinst's resval_obj_killer (cluster.c, previously a no-op) frees svalue + the xce_val_t -- both previously also leaked every xce_val_t. Also closes the same alias hazard for worker-reported string resvals (update_resval_list aliased the decoded request) and config values (mark_host_res). Regression test test_worker_join.c::test_rejoin_worker_string_resvals_survive_outstanding_round: first join arms a round (non-terminal job on host), second rejoin_worker with new attrs resolves it and fires a registered XCE_CONTROLLER_HOOK_WORKER hook that reads the type/model resvals -- RED reproduced the live stack exactly (heap-use-after-free, freed by rejoin_worker worker.c:432 xstring_dereference); GREEN after. Full meson test 109/109 OK. Note: get_resval() still hands out a borrowed svalue pointer (opget.c), now into the resval's own copy -- same lifetime contract as before, not widened.", + "tags": [ + "controller", + "asan", + "use-after-free", + "worker-rejoin-resync", + "ha-soak", + "resvals", + "xstring" + ] } ] } diff --git a/src/controller/cluster.c b/src/controller/cluster.c index 99256c78..8d4479bf 100644 --- a/src/controller/cluster.c +++ b/src/controller/cluster.c @@ -1561,11 +1561,19 @@ static void queue_obj_killer(void* arg) } } +/* bug-1851: c->sharedinst owns each xce_val_t (update_val_res, worker.c) + * and, for a string value, a private strdup()ed copy. */ static void resval_obj_killer(void* arg) { - if (arg == NULL) { + xce_val_t* v = (xce_val_t*)arg; + + if (v == NULL) { return; } + if (v->which == Value_u_svalue) { + FREEUP(v->u.svalue); + } + free(v); } static void config_obj_killer(void* arg) diff --git a/src/controller/testsuite/test_worker_join.c b/src/controller/testsuite/test_worker_join.c index 1ff8a339..bce1b169 100644 --- a/src/controller/testsuite/test_worker_join.c +++ b/src/controller/testsuite/test_worker_join.c @@ -14,6 +14,7 @@ #include #include +#include #include #include #include @@ -28,6 +29,7 @@ * Request_body_* constants */ #include "jobj.h" /* worker-rejoin-resync task 6.1: makeJob, for a real * non-terminal job the arm-round wiring tests need */ +#include "hook.h" /* bug-1851: xce_hook_init/xce_add_hook */ #include "kvmgr.h" #include "netio.h" /* worker-rejoin-resync task 6.1: endpoint_get/ * endpoint_deq/ITPTR_TO_PACKET, for real wire capture */ @@ -1007,6 +1009,137 @@ static void test_rejoin_worker_no_round_when_only_terminal_jobs(void) printf("test_rejoin_worker_no_round_when_only_terminal_jobs: PASS\n"); } +/* + * bug-1851: a string resval ("type"/"model") must own its bytes rather + * than alias the interned xstring in w->host.attrs. rejoin_worker drops + * the old attrs strings, then apply_join_state -> resync_arm_round + * resolves the PREVIOUS join's still-outstanding round, which fires the + * worker hooks (resync_resolve_round -> persist_notify_worker_status_ + * updated) BEFORE add_hostattrs_resvals re-seeds resvals. Live, the + * apiserver hook's collect_resval strdup()ed the freed hostmodel and the + * newly-promoted primary died (ASAN heap-use-after-free). This hook stands + * in for it: it reads both string resvals at exactly that point. + */ +static char g_1851_seen_type[64]; +static char g_1851_seen_model[64]; +static int g_1851_hook_calls; + +static void copy_string_resval(worker_t* w, const char* name, char* out, + size_t out_sz) +{ + xce_resource_t* r = NULL; + xce_val_t* v; + + lock(g_tc.c.resmgr, RTTI_LOCK_READ); + key_find(g_tc.c.resmgr, (void*)name, (void**)&r); + unlock(g_tc.c.resmgr); + if (r == NULL || r->idx < 0 || r->idx >= w->host.resvals.caps) + return; + v = (xce_val_t*)w->host.resvals.data[r->idx]; + if (v != NULL && v->which == Value_u_svalue && v->u.svalue != NULL) + snprintf(out, out_sz, "%s", v->u.svalue); +} + +static int bug1851_worker_hook(int type, int situation, int detail, + void* cls, va_list ap) +{ + (void)type; + (void)situation; + (void)detail; + (void)cls; + (void)va_arg(ap, cluster_t*); + worker_t* w = va_arg(ap, worker_t*); + + g_1851_hook_calls++; + copy_string_resval(w, "type", g_1851_seen_type, sizeof(g_1851_seen_type)); + copy_string_resval(w, "model", g_1851_seen_model, + sizeof(g_1851_seen_model)); + return 0; +} + +static void test_rejoin_worker_string_resvals_survive_outstanding_round(void) +{ + setup_test_cluster(); + TEST_ASSERT_EQUAL_INT( + 0, xce_hook_init(&g_tc.c.hooksys, XCE_CONTROLLER_HOOK_MAX)); + TEST_ASSERT_EQUAL_INT(0, xce_add_hook(&g_tc.c.hooksys, + XCE_CONTROLLER_HOOK_WORKER, + bug1851_worker_hook, NULL)); + g_1851_hook_calls = 0; + g_1851_seen_type[0] = '\0'; + g_1851_seen_model[0] = '\0'; + + /* A non-terminal job on the host, so each rejoin arms a round and the + * second rejoin finds the first one still outstanding. */ + job_t* j = seed_test_job(96851, JOB_STAT_RUN); + TEST_ASSERT_NOT_NULL(j); + j->job.allocs = make_test_job_alloc(96851, "uaf-host-1851"); + TEST_ASSERT_NOT_NULL(j->job.allocs); + + hostattrs_t attrs; + memset(&attrs, 0, sizeof(attrs)); + attrs.hosttype = makeString("TYPE-1851-A"); + attrs.hostmodel = makeString("MODEL-1851-A"); + attrs.maxcpus = 4; + worker_t* w = make_test_worker("uaf-host-1851", &attrs); + TEST_ASSERT_NOT_NULL(w); + /* makeWorker took its own references; drop the test's so w->host.attrs + * holds the ONLY reference to the "A" strings. */ + dereference(attrs.hosttype); + dereference(attrs.hostmodel); + + xce_hostconfig_t hconfig; + memset(&hconfig, 0, sizeof(hconfig)); + hconfig.hosttype = (char*)"!"; + hconfig.hostmodel = (char*)"!"; + hconfig.mxj = 4; + + endpoint_t ep; + memset(&ep, 0, sizeof(ep)); + + /* First join: seeds resvals from the "A" strings, arms round #1. */ + TEST_ASSERT_GREATER_OR_EQUAL(0, apply_join_state(&g_tc.c, w, &ep, &hconfig)); + TEST_ASSERT_NOT_NULL_MESSAGE(w->resync, + "fixture: the first join must leave a round outstanding"); + + /* Second join with different strings: must resolve round #1 (firing + * the hook) while "A" is being released. */ + hostattrs_t new_attrs; + memset(&new_attrs, 0, sizeof(new_attrs)); + new_attrs.hosttype = makeString("TYPE-1851-B"); + new_attrs.hostmodel = makeString("MODEL-1851-B"); + new_attrs.maxcpus = 4; + g_1851_hook_calls = 0; + + TEST_ASSERT_GREATER_OR_EQUAL( + 0, rejoin_worker(&g_tc.c, w, &ep, &new_attrs, &hconfig)); + + TEST_ASSERT_GREATER_THAN_INT_MESSAGE(0, g_1851_hook_calls, + "fixture: resolving the outstanding round must fire the worker hook"); + TEST_ASSERT_EQUAL_STRING_MESSAGE("TYPE-1851-A", g_1851_seen_type, + "the hook must read the previous (still-valid) type resval, not " + "freed memory"); + TEST_ASSERT_EQUAL_STRING_MESSAGE("MODEL-1851-A", g_1851_seen_model, + "the hook must read the previous (still-valid) model resval, not " + "freed memory"); + + /* Ownership: the resval is a private copy, never the attrs xstring. */ + xce_val_t val; + memset(&val, 0, sizeof(val)); + TEST_ASSERT_EQUAL_INT(1, get_resval(&g_tc.c, (char*)"model", + (char*)"uaf-host-1851", &val)); + TEST_ASSERT_EQUAL_STRING("MODEL-1851-B", val.u.svalue); + TEST_ASSERT_TRUE_MESSAGE(val.u.svalue != w->host.attrs->hostmodel, + "a string resval must own its bytes, not alias w->host.attrs"); + + dereference(new_attrs.hosttype); + dereference(new_attrs.hostmodel); + xce_hook_fini(&g_tc.c.hooksys); + teardown_test_cluster(); + printf("test_rejoin_worker_string_resvals_survive_outstanding_round: " + "PASS\n"); +} + /* * Req 1.3: the SAME trigger condition -- no special-casing needed -- must * apply to a host's first-ever join (init_worker), not just a rejoin. @@ -1526,6 +1659,7 @@ void test_worker_join(void) RUN_TEST(test_rejoin_worker_arms_reconciliation_round_when_jobs_exist); RUN_TEST(test_rejoin_worker_no_round_when_no_jobs); RUN_TEST(test_rejoin_worker_no_round_when_only_terminal_jobs); + RUN_TEST(test_rejoin_worker_string_resvals_survive_outstanding_round); RUN_TEST(test_init_worker_arms_reconciliation_round_when_jobs_exist); RUN_TEST(test_worker_send_resync_query_sends_armed_round); RUN_TEST(test_worker_send_resync_query_sends_nothing_when_no_round); diff --git a/src/controller/worker.c b/src/controller/worker.c index 3e2f72bb..5ecc682a 100644 --- a/src/controller/worker.c +++ b/src/controller/worker.c @@ -96,7 +96,29 @@ static int update_val_res(cluster_t* c, worker_t* w, xce_resource_t* r, } } - memcpy(v, val, sizeof(*v)); + /* bug-1851: a stored string value owns a private copy. Aliasing the + * caller's pointer (w->host.attrs' interned hosttype/hostmodel, a + * config value, or a decoded request) left it dangling once that owner + * released it -- rejoin_worker drops the old attrs strings before the + * resvals are re-seeded, and a worker hook read them in between. The + * copy is made before the old one is freed, so val may alias v. */ + if (val->which == Value_u_svalue) { + char* s = strdup(val->u.svalue != NULL ? val->u.svalue : ""); + + if (s == NULL) { + return -ENOMEM; + } + if (v->which == Value_u_svalue) { + FREEUP(v->u.svalue); + } + v->which = Value_u_svalue; + v->u.svalue = s; + } else { + if (v->which == Value_u_svalue) { + FREEUP(v->u.svalue); + } + memcpy(v, val, sizeof(*v)); + } /* bug-1568 instrumentation: the actual VALUE being written into * w->host.resvals[idx] (or c->sharedinst[idx] for a shared resource) diff --git a/src/lib/wobj.c b/src/lib/wobj.c index 319e0487..7b2d68d0 100644 --- a/src/lib/wobj.c +++ b/src/lib/wobj.c @@ -22,6 +22,21 @@ static size_t worker_datasize(va_list ap) return sz; } +/* bug-1851: host.resvals owns each xce_val_t (update_val_res, controller + * worker.c) and, for a string value, a private strdup()ed copy. */ +static void kill_resval(void* p) +{ + xce_val_t* v = (xce_val_t*)p; + + if (v == NULL) { + return; + } + if (v->which == Value_u_svalue) { + FREEUP(v->u.svalue); + } + free(v); +} + static void kill_worker(worker_t* w) { dereference(w->host.name); @@ -66,8 +81,7 @@ static int copy_worker(worker_t* dst, xce_worker_t* src) dst->host.attrs->hostmodel = makeString(src->attrs->hostmodel); dst->host.status = src->status; - /* FIXME: */ - zarray_init(&(dst->host.resvals), 64, NULL); + zarray_init(&(dst->host.resvals), 64, kill_resval); dst->host.dead_since = src->dead_since; dst->host.pending_ctrl = src->pending_ctrl;