From 42e1eeaa29fb65025582f530d47c3e08e3c687e6 Mon Sep 17 00:00:00 2001 From: Rongsong Shen Date: Sat, 29 Aug 2026 11:28:48 +0000 Subject: [PATCH] fix(bug-1733): fix heap-use-after-free race in vtask_enq() vtask_enq() published a task onto the shared dispatch queue via dataq_enq() before finishing its own initialization (task->owner) -- a concurrent vcpu could dequeue, run, and free the task before the producer's own write executed, an ASAN-confirmed UAF that crashed 2 of 3 controllers under a heavy burst-array-job HA workload. Moved task->owner = vtaskmgr before the publish, mirroring the endpoint_ref- before-publish discipline already established two lines above it (bug-1298). Adds a stress-test regression guard (test_vtask_enq_race.c). Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_019QxaSzHRB7pmYFp4cVnwkA --- src/lib/meson.build | 1 + src/lib/testsuite/test.c | 2 + src/lib/testsuite/test_vtask_enq_race.c | 146 ++++++++++++++++++++++++ src/lib/vtaskmgr.c | 11 +- 4 files changed, 159 insertions(+), 1 deletion(-) create mode 100644 src/lib/testsuite/test_vtask_enq_race.c diff --git a/src/lib/meson.build b/src/lib/meson.build index ea2ba3cc..4057e4ec 100644 --- a/src/lib/meson.build +++ b/src/lib/meson.build @@ -188,6 +188,7 @@ libxce_test_src = ['testsuite/test.c', 'testsuite/test_wal_wire.c', 'testsuite/test_ctrlstatus.c', 'testsuite/test_vtask_admission.c', + 'testsuite/test_vtask_enq_race.c', 'testsuite/test_xcealloc.c', 'raft/raft_sim.c', ] diff --git a/src/lib/testsuite/test.c b/src/lib/testsuite/test.c index 1be1282d..d29c7171 100644 --- a/src/lib/testsuite/test.c +++ b/src/lib/testsuite/test.c @@ -51,6 +51,7 @@ extern void test_raft(void); extern void test_wal_wire(void); extern void test_ctrlstatus(void); extern void test_vtask_admission(void); +extern void test_vtask_enq_race(void); extern void test_xcealloc(void); void setUp(void) { return; } @@ -172,6 +173,7 @@ int main(int argc, char* argv[]) TEST(test_wal_wire); TEST(test_ctrlstatus); TEST(test_vtask_admission); + TEST(test_vtask_enq_race); TEST(test_xcealloc); return UNITY_END(); diff --git a/src/lib/testsuite/test_vtask_enq_race.c b/src/lib/testsuite/test_vtask_enq_race.c new file mode 100644 index 00000000..46af4b8a --- /dev/null +++ b/src/lib/testsuite/test_vtask_enq_race.c @@ -0,0 +1,146 @@ +/* + * test_vtask_enq_race.c — bug-1733 regression: vtask_enq()'s publish-before- + * init race. + * + * vtask_enq() (vtaskmgr.c:497-526) calls dataq_enq() first, which PUBLISHES + * the task onto the shared taskq (under a lock released before dataq_enq + * returns) — any idle vcpu worker thread can immediately dequeue it, run it, + * and free it (vcpu_entry: run_task -> vtask_fini -> free). Only AFTER that + * publish does vtask_enq write `task->owner = vtaskmgr;`. A concurrent vcpu + * can win the race and free the task before the producer's own write lands, + * which is a heap-use-after-free WRITE on freed memory. This was confirmed + * live under ASAN on a real 3-node HA cluster (see bug-1733) — two + * controllers crashed independently with byte-for-byte identical stacks. + * + * This is a genuine data race — the exact two-line window inside vtask_enq() + * cannot be forced deterministically from a test without modifying + * production code (contrast with test_vtask_admission.c's fully + * deterministic cbuf-gated tests). Instead this is a STRESS test: a real + * vtaskmgr_t pool with several vcpus and several producer threads hammering + * vtask_enq_classified() in a tight loop with true no-op tasks (matching + * live_task_entry in test_vtask_admission.c), for many iterations each — + * maximizing the odds that a consumer vcpu dequeues+runs+frees a task before + * the producer thread's own next instruction after dataq_enq() executes, + * which is exactly the race window bug-1733 depends on. + * + * IMPORTANT: under ASAN, hitting this bug means the WHOLE TEST BINARY ABORTS + * with an AddressSanitizer heap-use-after-free report — this is a + * process-level crash, not a Unity assertion failure. That crash (matching + * the shape: "WRITE of size 8" at vtask_enq, freed inside vcpu_entry) IS the + * RED confirmation for this test; there is no in-test assertion that can + * observe the race directly. + */ +#include +#include +#include +#include +#include + +#include + +#include "vtaskmgr.h" +#include "xce/errnox.h" + +#define RACE_N_VCPUS 8 +#define RACE_N_PRODUCERS 16 +#define RACE_ITERS_PER_PRODUCER 8000 +#define RACE_TOTAL_TASKS (RACE_N_PRODUCERS * RACE_ITERS_PER_PRODUCER) + +typedef struct { + atomic_int* ran; +} race_arg_t; + +/* True no-op: matches live_task_entry in test_vtask_admission.c. Returns + * immediately so the vcpu that runs it reaches vtask_fini()+free() as fast + * as possible, maximizing the odds it wins the race against the producer's + * own post-dataq_enq() instructions. */ +static int race_task_entry(void* p) +{ + vtask_t* t = (vtask_t*)p; + race_arg_t* arg = (race_arg_t*)t->data->u.generic; + + atomic_fetch_add(arg->ran, 1); + return 0; +} + +typedef struct { + vtaskmgr_t* mgr; + race_arg_t* arg; + int iters; +} producer_arg_t; + +static void* producer_thread(void* p) +{ + producer_arg_t* pa = (producer_arg_t*)p; + int i; + + for (i = 0; i < pa->iters; i++) { + vtask_data_t* d = calloc(1, sizeof(*d)); + vtask_t* t = vtask_alloc(); + int rc; + + d->vtype = VTASK_GENERIC; + d->u.generic = pa->arg; + vtask_init(t, race_task_entry, d); + + /* VTASK_LIVENESS -> vtask_enq_classified falls straight through to + * vtask_enq(), the function under test. Never admission-gated, so + * this always succeeds and the producer moves on immediately — + * exactly the tight loop needed to stress the race window. */ + rc = vtask_enq_classified(pa->mgr, t, VTASK_LIVENESS); + TEST_ASSERT_TRUE(rc >= 0); + } + + return NULL; +} + +/* Spin-wait (bounded) for an atomic to reach >= want; returns 1 on success. */ +static int wait_atomic_ge(atomic_int* a, int want, int max_ms) +{ + for (int i = 0; i < max_ms * 10; i++) { + if (atomic_load(a) >= want) { + return 1; + } + usleep(100); /* 0.1ms poll — liveness, not a correctness sleep */ + } + return atomic_load(a) >= want; +} + +static void test_vtask_enq_race_stress(void) +{ + vtaskmgr_t mgr; + pthread_t producers[RACE_N_PRODUCERS]; + producer_arg_t pargs[RACE_N_PRODUCERS]; + atomic_int ran = 0; + race_arg_t arg = { .ran = &ran }; + int i; + + TEST_ASSERT_EQUAL_INT(0, vtaskmgr_init(&mgr, RACE_N_VCPUS)); + TEST_ASSERT_EQUAL_INT(0, vtaskmgr_start(&mgr)); + + for (i = 0; i < RACE_N_PRODUCERS; i++) { + pargs[i].mgr = &mgr; + pargs[i].arg = &arg; + pargs[i].iters = RACE_ITERS_PER_PRODUCER; + TEST_ASSERT_EQUAL_INT( + 0, pthread_create(&producers[i], NULL, producer_thread, &pargs[i])); + } + + for (i = 0; i < RACE_N_PRODUCERS; i++) { + pthread_join(producers[i], NULL); + } + + /* If we get here without an ASAN abort, the race window was survived + * for RACE_TOTAL_TASKS enqueues across RACE_N_PRODUCERS producers and + * RACE_N_VCPUS consumers. Confirm the pool actually drained everything + * (a fixed vtask_enq should let every enqueued task run to completion). */ + TEST_ASSERT_TRUE_MESSAGE(wait_atomic_ge(&ran, RACE_TOTAL_TASKS, 20000), + "not all enqueued tasks ran to completion"); + + vtaskmgr_fini(&mgr); +} + +void test_vtask_enq_race(void) +{ + RUN_TEST(test_vtask_enq_race_stress); +} diff --git a/src/lib/vtaskmgr.c b/src/lib/vtaskmgr.c index 40ddcc3f..3eb708e3 100644 --- a/src/lib/vtaskmgr.c +++ b/src/lib/vtaskmgr.c @@ -511,10 +511,19 @@ int vtask_enq(vtaskmgr_t* vtaskmgr, vtask_t* task) endpoint_ref(ep); } + /* bug-1733: task->owner must be set BEFORE dataq_enq() publishes the task + * to the dispatcher, mirroring the endpoint_ref-before-publish discipline + * just above — a concurrent vcpu can dequeue, run (run_task() itself reads + * task->owner to release a REQ_BLOCK admission slot and wake the + * dispatcher), and free this task before the next line here would + * otherwise run, which is a heap-use-after-free write on freed memory + * (and, short of that exact timing, a missed admission-release/wakeup if + * run_task read owner while it was still NULL). */ + task->owner = vtaskmgr; + rc = dataq_enq(&(vtaskmgr->taskq), &(task->link)); if (rc >= 0) { /* wake up task dispatcher */ - task->owner = vtaskmgr; vtaskmgr_wakeup(vtaskmgr); } else if (ep != NULL) { /* enqueue failed — the task never entered the queue; drop the ref we