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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019QxaSzHRB7pmYFp4cVnwkA

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',
                   ]

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();

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 <pthread.h>
+#include <stdatomic.h>
+#include <stdint.h>
+#include <stdlib.h>
+#include <unistd.h>
+
+#include <unity/unity.h>
+
+#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);
+}

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