From b406dd762ad4a7848342e5a86e9817733faae040 Mon Sep 17 00:00:00 2001 From: Rongsong Shen Date: Tue, 18 Aug 2026 08:15:37 +0000 Subject: [PATCH] fix(controller): sched_worker_change() use-after-free on worker disconnect (bug-1578) sched_worker_change() aliased w->host.resvals_to_sched to a local xlist_t's backing array, pushed it to the scheduler, then freed that array via xlist_fini() without resetting the alias - leaving a dangling pointer for the rest of the worker's life. On disconnect (handle_worker_leave -> mark_worker_unavail -> persist_worker_hook_fun -> capnp_encode_worker -> encode_Worker -> encode_NameValue_list), this dereferenced freed memory and crashed the controller - but only once a scheduler had ever connected (on_scheduler_join gates the hook registration), which made it easy to miss in isolated worker-only testing. The correct reset already existed in the function but was dead-code'd behind #if 0 /*FIXME*/ - apparently from a prior fix attempt that got wrapped instead of landed. Fix mirrors the proven-correct pattern already used by the sibling apiserver_worker_change(): null the alias before xlist_fini() frees the backing array, rather than double-freeing via free_name_val_list() first. Adds test_sched_worker_change.c, a regression test driving the real production hook-dispatch path (xce_run_hooks -> sched_worker_hook_fun -> sched_worker_dead -> sched_worker_change) and the real encode_Worker() encoder the original crash traversed - confirmed to reproduce the exact ASAN heap-use-after-free when the fix is reverted, and pass clean when restored, closing the gap that let this regress silently once already. Found while live-testing the resource-value-verification feature (deploying a scheduler + injecting worker resource values via xce_test, then disconnecting) - unrelated to that feature's own changes. Co-Authored-By: Claude Sonnet 5 --- src/controller/meson.build | 30 ++ src/controller/sched.c | 7 +- .../testsuite/test_sched_worker_change.c | 360 ++++++++++++++++++ 3 files changed, 392 insertions(+), 5 deletions(-) create mode 100644 src/controller/testsuite/test_sched_worker_change.c diff --git a/src/controller/meson.build b/src/controller/meson.build index f01271bd..cfdcf51d 100644 --- a/src/controller/meson.build +++ b/src/controller/meson.build @@ -255,6 +255,36 @@ test('sched push test', sched_push_unit_test, timeout: 60, env: ['ASAN_OPTIONS=detect_leaks=0']) +# bug-1578 regression test: sched_worker_change()'s dangling-pointer fix +# (w->host.resvals_to_sched/nresvals_to_sched left aliased onto a freed +# xlist_t backing array -- see sched.c's sched_worker_change() for the full +# history). Same real-vs-fake link-collision reason as sched_push_unit_test +# just above requires its own standalone binary rather than folding into +# controller_unit_test_src: sched_worker_change() is only reachable by +# linking the REAL sched.c/dataex.c/egress.c, which collides with other +# controller_unit_test_src files' own fake on_scheduler_join()/notify() +# definitions. Mirrors sched_push_unit_test_src's exact source-set strategy +# (including opget.c's check_worker_() being stubbed locally rather than +# linking all of opget.c). +sched_worker_change_unit_test_src = [ + 'testsuite/test_sched_worker_change.c', + 'sched.c', + 'dataex.c', + 'egress.c', +] + +sched_worker_change_unit_test = executable('sched_worker_change_unit_test', + sched_worker_change_unit_test_src, + include_directories: [incdir], + dependencies: controller_unit_test_deps, + c_args: ['-DXCE_TESTING'], + install: false) + +test('sched worker change test', sched_worker_change_unit_test, + suite: 'controller', + timeout: 60, + env: ['ASAN_OPTIONS=detect_leaks=0']) + # job-array-single-wal-record task 5.2: apiserver.c's push logic # (apiserver_job_new_fun / apiserver_push_jobarray) faces the identical # real-vs-fake link collision task 5.1 hit for sched.c (test_commit_barrier_ diff --git a/src/controller/sched.c b/src/controller/sched.c index e090d7e1..c3bf4087 100644 --- a/src/controller/sched.c +++ b/src/controller/sched.c @@ -545,11 +545,8 @@ static int sched_worker_change(int type, int situation, void* data, } mylock_unlock(&(c->sched.lock)); -#if 0 - /*FIXME*/ - free_name_val_list(&(w->host.resvals_to_sched), - &(w->host.nresvals_to_sched)); -#endif + w->host.nresvals_to_sched = 0; + w->host.resvals_to_sched = NULL; xlist_fini(&resvalist); return rc; diff --git a/src/controller/testsuite/test_sched_worker_change.c b/src/controller/testsuite/test_sched_worker_change.c new file mode 100644 index 00000000..2415de83 --- /dev/null +++ b/src/controller/testsuite/test_sched_worker_change.c @@ -0,0 +1,360 @@ +/* test_sched_worker_change.c — regression test for bug-1578 + * + * bug-1578: sched_worker_change() (sched.c) aliases w->host.resvals_to_sched + * / nresvals_to_sched directly onto a local xlist_t's backing array + * (resvalist.data), pushes it to the scheduler connection, then calls + * xlist_fini(&resvalist) -- which frees that backing array -- WITHOUT first + * resetting w->host.resvals_to_sched to NULL / nresvals_to_sched to 0. That + * left a DANGLING pointer + a stale nonzero count on the worker_t struct for + * the rest of that worker's lifetime; the next code to read + * resvals_to_sched (production: persist_worker_hook_fun -> + * capnp_encode_worker -> encode_Worker -> encode_NameValue_list -> + * encode_NameValue, on worker disconnect) dereferenced freed memory -> + * SIGSEGV. + * + * This bug had ONE PRIOR fix attempt (commit e5fde7b5) that silently never + * took effect because it edited code inside the same dead "#if 0" (FIXME- + * commented) block sched_worker_change() has always had (introduced + * already-disabled by commit acfbec875) -- i.e. this exact bug + * already regressed once before, silently, for lack of test coverage. The + * real fix (2026-08-18) replaces that dead block with a direct + * `w->host.nresvals_to_sched = 0; w->host.resvals_to_sched = NULL;` before + * `xlist_fini(&resvalist);`, mirroring the sibling apiserver_worker_change() + * (apiserver.c) that never had this bug. + * + * Fixture: sched_worker_change()/sched_worker_dead() are static to sched.c, + * so this file drives them through the same public entry points production + * code uses, exactly mirroring test_job_array_sched_push.c's established + * pattern for this same standalone-binary strategy (see meson.build's + * comment on sched_push_unit_test for why sched.c/dataex.c/egress.c must be + * linked for REAL here rather than folded into controller_unit_test, and + * why check_worker_() below is stubbed rather than linking all of opget.c): + * on_scheduler_join() installs sched_worker_hook_fun into a real + * cluster_t's hooksys (once per process -- hook_installed is a static + * inside on_scheduler_join), and xce_run_hooks() fires + * XCE_CONTROLLER_HOOK_WORKER / HOOK_DETAIL_WORKER_DEAD exactly as + * handle_worker_leave() -> mark_worker_unavail() does in production. + * + * Test 1 asserts the exact condition the fix guarantees and the bug + * violated: after the hook fires, w->host.resvals_to_sched == NULL and + * w->host.nresvals_to_sched == 0 -- never a dangling pointer + stale count. + * + * Test 2 goes further and proves it under ASAN: immediately after the hook + * fires, it calls the REAL production encoder encode_Worker() (controller. + * capnp.c, the exact function bug-1578's crash backtrace bottoms out in -- + * capnp_encode_worker/persist_worker_hook_fun's own callee) directly on the + * SAME worker, exactly as persist_worker_hook_fun does on worker disconnect. + * Post-fix this is a correct, harmless 0-entry encode. If the fix ever + * regresses (nresvals_to_sched stale-nonzero, resvals_to_sched dangling), + * encode_NameValue_list's loop reads resvals_to_sched[i] out of the freed + * backing array -- an immediate ASAN heap-use-after-free, not just a failed + * assertion. + */ + +#include +#include +#include +#include +#include + +#include + +#include "cluster.h" +#include "controller.capnp.h" +#include "hook.h" +#include "kvmgr.h" +#include "netio.h" +#include "request.h" +#include "resobj.h" +#include "xce/cluster_def.h" +#include "xce/types.h" +#include "xlist.h" +#include "xstring.h" +#include "zarray.h" + +extern int on_scheduler_join(cluster_t*); + +/* This file builds a STANDALONE test binary (meson.build's + * sched_worker_change_unit_test) linking the real sched.c + its real + * dataex.c/egress.c dependencies, mirroring sched_push_unit_test exactly + * (see meson.build's comment on that target for why: several existing + * controller_unit_test_src files already provide their OWN local fake + * definitions of on_scheduler_join()/notify()/collect_confvars(), so linking + * the real sched.c/dataex.c into that shared binary collides at link time). + * opget.c's check_worker_() is sched.c's only remaining unresolved + * reference (from xcesched_push_workers(), reached only through + * on_scheduler_join()'s push-on-join branch, which this file's setup + * deliberately never triggers at runtime -- c->sched.ep is NULL during that + * call -- but the symbol must still resolve at link time). Stubbed locally + * rather than linking all of opget.c, identical to test_job_array_sched_ + * push.c's own stub. */ +int check_worker_(xce_worker_t* w) { return (w != NULL) ? 1 : 0; } + +/* ------------------------------------------------------------------ */ +/* Fixture */ +/* ------------------------------------------------------------------ */ + +static cluster_t g_swc_c; +static endpoint_t g_swc_ep; +static int g_swc_hook_wired; +static var g_swc_resmgr; + +static void swc_val_killer(void* p) { free(p); } + +static void swc_drain_ep(endpoint_t* ep) +{ + data_item_t* it; + + while ((it = endpoint_deq(ep)) != NULL) { + io_packet_t* pkt = ITPTR_TO_PACKET(it); + + free_packet(pkt); + free(pkt); + } +} + +/* Registers one resource ("mem", numeric) in a fresh resmgr and returns its + * xce_resource_t* (idx assigned by makeResource/key_add, matching production + * cluster_build_resources()'s own makeResource() call). */ +static xce_resource_t* swc_setup(void) +{ + kvmgr_params_t resmgr_params + = { .key_type = KVMGR_KEY_STRING, + .nslots = 64, + .delta = 16, + .datakiller = NULL }; + xce_resource_t rdef; + xce_resource_t* r; + + if (!g_swc_hook_wired) { + memset(&g_swc_c, 0, sizeof(g_swc_c)); + TEST_ASSERT_EQUAL_INT( + 0, xce_hook_init(&g_swc_c.hooksys, XCE_CONTROLLER_HOOK_MAX)); + /* sched.ep == NULL here so on_scheduler_join's push-on-join branch + * is a no-op -- this call's only effect is installing + * sched_worker_hook_fun (and siblings) into g_swc_c.hooksys, once, + * process-wide (hook_installed is a static inside + * on_scheduler_join). */ + g_swc_c.sched.ep = NULL; + TEST_ASSERT_EQUAL_INT(0, on_scheduler_join(&g_swc_c)); + g_swc_hook_wired = 1; + } + + g_swc_resmgr = makeInstance(&xkvmgr_type, &resmgr_params); + TEST_ASSERT_NOT_NULL(g_swc_resmgr); + + memset(&rdef, 0, sizeof(rdef)); + rdef.name = (char*)"mem"; + rdef.valtype = XCE_VAL_NUMERIC; + rdef.idx = -1; + r = makeResource(g_swc_resmgr, &rdef); + TEST_ASSERT_NOT_NULL(r); + + memset(&g_swc_ep, 0, sizeof(g_swc_ep)); + g_swc_ep.fd = -1; + + /* c->sched.ep != NULL is sched_worker_change()'s guard for actually + * pushing to the scheduler (situation == HOOK_AFTER_EVENT branch) -- + * required to reach the collect_resval()/resvalist code path under + * test at all. A zeroed, fd=-1 stack endpoint_t queues packets on + * ep->outq without touching a real socket, exactly as test_job_array_ + * sched_push.c's own fixture does for the JOB hook. */ + g_swc_c.sched.ep = &g_swc_ep; + g_swc_c.resmgr = g_swc_resmgr; + g_swc_c.running = 1; + g_swc_c.myhost = &g_swc_c.myhost_info; + g_swc_c.myhost_info.name = (char*)"ctrl-swc"; + g_swc_c.myhost_info.key = (char*)""; + + return r; +} + +static void swc_teardown(void) +{ + swc_drain_ep(&g_swc_ep); + g_swc_c.sched.ep = NULL; + g_swc_c.resmgr = NULL; + if (g_swc_resmgr != NULL) { + dereference(g_swc_resmgr); + g_swc_resmgr = NULL; + } +} + +/* Builds a plain, stack-owned worker_t with one populated resval -- NOT + * routed through makeWorker()/wobj.c's RTTI worker-object machinery + * (unneeded: sched_worker_change() only ever reads w->host.name/resvals and + * w->idx, none of which requires a real registered worker instance), + * mirroring test_job_array_sched_push.c's identical "plain struct, not + * through the real object constructor" rationale for its job_t fixtures. + * The resval is written the same way update_val_res() (worker.c) writes a + * real resupdate: a heap xce_val_t* stored via zarray_put() at the + * resource's own idx. */ +static void swc_make_worker(worker_t* w, const char* name, xce_resource_t* r, + double fval) +{ + xce_val_t* v; + + memset(w, 0, sizeof(*w)); + w->host.name = (char*)name; + w->idx = 0; + zarray_init(&(w->host.resvals), 16, swc_val_killer); + + v = (xce_val_t*)calloc(1, sizeof(*v)); + TEST_ASSERT_NOT_NULL(v); + v->which = Value_u_fvalue; + v->u.fvalue = fval; + TEST_ASSERT_TRUE(zarray_put(&(w->host.resvals), r->idx, v) >= 0); +} + +static void swc_free_worker(worker_t* w) { zarray_fini(&(w->host.resvals)); } + +static int swc_fire_worker_dead(worker_t* w) +{ + return xce_run_hooks(&g_swc_c.hooksys, XCE_CONTROLLER_HOOK_WORKER, + HOOK_AFTER_EVENT, HOOK_DETAIL_WORKER_DEAD, &g_swc_c, + w); +} + +/* Mirrors capnp_encode_worker() (persist.c) / tace_encode_worker() + * (test_apply_committed_entry.c) exactly -- the real production helper that + * wraps encode_Worker() for a malloc'd Cap'n Proto buffer. Not linking + * persist.c itself (heavy, unrelated dependency chain for this fix); calling + * encode_Worker() directly reaches the identical bug-1578 crash-site + * function (controller.capnp.c) with zero loss of fidelity -- persist.c's + * capnp_encode_worker() is a 20-line pass-through to it. */ +static int swc_encode_worker(const xce_worker_t* w, uint8_t** out, + size_t* out_len) +{ + struct capn c; + struct capn_segment* cs; + struct Worker cw; + Worker_ptr wp; + + capn_init_malloc(&c); + cs = capn_root(&c).seg; + encode_Worker(cs, &cw, (xce_worker_t*)w); + wp = new_Worker(cs); + write_Worker(&cw, wp); + capn_setp(capn_root(&c), 0, wp.p); + + *out_len = capn_size(&c); + *out = malloc(*out_len); + if (*out == NULL) { + capn_free(&c); + return -ENOMEM; + } + capn_write_mem(&c, *out, *out_len, 0); + capn_free(&c); + return 0; +} + +/* ------------------------------------------------------------------ */ +/* Test 1: the exact condition the fix guarantees */ +/* ------------------------------------------------------------------ */ + +static void test_worker_dead_leaves_resvals_to_sched_null_and_zero(void) +{ + xce_resource_t* r; + worker_t w; + int rc; + + r = swc_setup(); + + swc_make_worker(&w, "swc-worker-1", r, 42.5); + + rc = swc_fire_worker_dead(&w); + TEST_ASSERT_EQUAL_INT(0, rc); + + /* bug-1578: before the fix, sched_worker_change() aliased these two + * fields directly onto a local xlist_t's backing array, then freed that + * array (xlist_fini) WITHOUT resetting them -- leaving + * resvals_to_sched a dangling pointer and nresvals_to_sched a stale + * nonzero count. This is the exact condition the fix guarantees. */ + TEST_ASSERT_NULL_MESSAGE( + w.host.resvals_to_sched, + "bug-1578 regression: w->host.resvals_to_sched must be NULL after " + "sched_worker_change() completes -- a non-NULL value here is a " + "dangling pointer into memory sched_worker_change() already freed " + "via xlist_fini(&resvalist)"); + TEST_ASSERT_EQUAL_INT_MESSAGE( + 0, w.host.nresvals_to_sched, + "bug-1578 regression: w->host.nresvals_to_sched must be 0 after " + "sched_worker_change() completes -- a nonzero stale count paired " + "with the dangling pointer above is exactly what let production " + "code (persist_worker_hook_fun -> capnp_encode_worker) iterate " + "into freed memory and SIGSEGV"); + + swc_free_worker(&w); + swc_teardown(); + printf( + "test_worker_dead_leaves_resvals_to_sched_null_and_zero: PASS\n"); +} + +/* ------------------------------------------------------------------ */ +/* Test 2: ASAN-strength proof -- re-run the real crash-site encoder */ +/* ------------------------------------------------------------------ */ + +static void test_worker_dead_then_encode_worker_is_asan_clean(void) +{ + xce_resource_t* r; + worker_t w; + int rc; + uint8_t* buf = NULL; + size_t len = 0; + + r = swc_setup(); + + swc_make_worker(&w, "swc-worker-2", r, 7.0); + + rc = swc_fire_worker_dead(&w); + TEST_ASSERT_EQUAL_INT(0, rc); + + /* Exactly what persist_worker_hook_fun() does on the SAME + * HOOK_DETAIL_WORKER_DEAD event in production (capnp_encode_worker -> + * encode_Worker -> encode_NameValue_list -> encode_NameValue). Post-fix, + * w.host.nresvals_to_sched == 0, so encode_NameValue_list's loop never + * executes -- a correct, harmless empty encode. If sched_worker_change() + * ever regresses to the pre-fix behavior (dangling resvals_to_sched + + * stale nonzero nresvals_to_sched), this call reads resvals_to_sched[i] + * out of memory sched_worker_change() already freed -- an ASAN + * heap-use-after-free abort, not merely a failed assertion. */ + rc = swc_encode_worker(&w.host, &buf, &len); + TEST_ASSERT_EQUAL_INT_MESSAGE( + 0, rc, "encode_Worker() of the just-processed worker must succeed"); + TEST_ASSERT_NOT_NULL(buf); + TEST_ASSERT_TRUE(len > 0); + free(buf); + + swc_free_worker(&w); + swc_teardown(); + printf( + "test_worker_dead_then_encode_worker_is_asan_clean: PASS\n"); +} + +/* ------------------------------------------------------------------ */ +/* Standalone Unity main (this binary is its own test executable -- see */ +/* meson.build's sched_worker_change_unit_test) */ +/* ------------------------------------------------------------------ */ + +void setUp(void) { return; } +void tearDown(void) { return; } + +int main(void) +{ + printf("\n>> bug-1578: sched_worker_change() dangling-pointer " + "regression test\n\n"); + + /* makeResource()/makeString() (resobj.c/xstring.c) need the global + * xstring intern table initialized before first use -- same requirement + * test_worker_join.c documents at its own RUN_TEST call site. This is a + * standalone binary (no other test file shares process init with it), + * so it must be done exactly once here. */ + xstring_table_init(139); + + UNITY_BEGIN(); + + RUN_TEST(test_worker_dead_leaves_resvals_to_sched_null_and_zero); + RUN_TEST(test_worker_dead_then_encode_worker_is_asan_clean); + + return UNITY_END(); +}