lei.chen/myxce
1
0
Fork 1
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

fix(controller): sched_worker_change() use-after-free on worker disconnect (bug-1578)

Browse source 
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 <noreply@anthropic.com>
This commit is contained in:
Rongsong Shen 2026-08-18 08:15:37 +00:00
parent 23c3fd6b6a
commit b406dd762a
Signed by: rshen
SSH key fingerprint: SHA256:8Vf2pZ3h6rJa7EhGK+WaAQ9Mta/apZvteW6g9U/WKKk
3 changed files with 392 additions and 5 deletions
Download patch file Download diff file Expand all files Collapse all files

30
src/controller/meson.build
View file

@ -255,6 +255,36 @@ test('sched push test', sched_push_unit_test,
timeout: 60, timeout: 60,
env: ['ASAN_OPTIONS=detect_leaks=0']) 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 # job-array-single-wal-record task 5.2: apiserver.c's push logic
# (apiserver_job_new_fun / apiserver_push_jobarray) faces the identical # (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_ # real-vs-fake link collision task 5.1 hit for sched.c (test_commit_barrier_

7
src/controller/sched.c
View file

@ -545,11 +545,8 @@ static int sched_worker_change(int type, int situation, void* data,
} }
mylock_unlock(&(c->sched.lock)); mylock_unlock(&(c->sched.lock));
#if 0 w->host.nresvals_to_sched = 0;
/*FIXME*/ w->host.resvals_to_sched = NULL;
free_name_val_list(&(w->host.resvals_to_sched),
&(w->host.nresvals_to_sched));
#endif
xlist_fini(&resvalist); xlist_fini(&resvalist);
return rc; return rc;

360
src/controller/testsuite/test_sched_worker_change.c Normal file
View file

@ -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 <capnp_c.h>
#include <errno.h>
#include <stdint.h>
#include <stdlib.h>
#include <string.h>
#include <unity/unity.h>
#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();
}