diff --git a/.kiro/specs/raft-drive-async/spec.json b/.kiro/specs/raft-drive-async/spec.json index 236e53af..ad9cc761 100644 --- a/.kiro/specs/raft-drive-async/spec.json +++ b/.kiro/specs/raft-drive-async/spec.json @@ -1,9 +1,9 @@ { "feature_name": "raft-drive-async", "created_at": "2026-06-25T11:37:13Z", - "updated_at": "2026-06-26T04:43:19Z", + "updated_at": "2026-07-18T14:45:00Z", "language": "en", - "phase": "tasks-generated", + "phase": "implementation-complete", "approvals": { "requirements": { "generated": true, @@ -18,5 +18,15 @@ "approved": true } }, - "ready_for_implementation": true -} \ No newline at end of file + "ready_for_implementation": true, + "validate_impl": { + "decision": "GO", + "validated_at": "2026-07-18T14:45:00Z", + "note": "Re-validated after bug-1447 (the prior blocking finding) was root-caused and FIXED. \n\nMECHANICAL: Fresh ASAN build (nix develop + xce-build.sh lib, then controller) with the fix applied. TBD/TODO grep on this spec's owned files (slowlane.c/.h, vtaskmgr.c/.h, commit.c, netio.c/.h, ha.c, config_repl.c, cluster.c, config.h, ha.h, run-ha-soak.sh) found only the one pre-existing, already-flagged item (ha.c:1844, ha-term-safety-owned stub) -- zero new TBD/TODO/FIXME introduced by the fix. Secrets grep clean. Full test suite: libxce xcelib_test 294/0/0 (ASAN), controller_unit_test 972/0/0 (ASAN), zero AddressSanitizer/heap-use-after-free/heap-buffer-overflow/double-free/SEGV hits anywhere in the full controller test log.\n\nRUNTIME LIVENESS / SMOKE (the check that previously blocked): re-ran the canonical /xce-test-ha SEQUENTIAL 10-round soak TWICE on fresh ASAN binaries built with the fix, on real ext4 (not overlayfs, bug-924). Both fully clean: run 1 (/home/claude/tmp/xce-bug1447-verify-1) 10/10 rounds, 33 jobs, zero job loss, all 3 nodes led; run 2 (/home/claude/tmp/xce-bug1447-verify-2) 10/10 rounds, 36 jobs, zero job loss, all 3 nodes led including a forced-election leadership-coverage check on ctrl-3. An explicit grep of every node's log in both runs for bug-1447's exact signature (AddressSanitizer/DEADLYSIGNAL/SEGV/ssl3_read_bytes/handle_ssl_handshake) found zero matches. This is a concurrency/UAF bug class, so two independent clean 10-round soaks (66 rounds of kill/elect/reconnect churn combined across both verification runs) plus a deterministic unit regression test (below) are treated as the evidence bar, not a single pass.\n\nROOT CAUSE (bug-1447, now closed): NOT the originally-suspected 'two io_worker threads racing the same SSL*' -- there is exactly ONE io_worker thread per iomgr_t (iomgr_start creates one pthread), so that hypothesis was retracted. The real defect: io_worker's epoll dispatch loop processed every ready fd from ONE epoll_wait() batch in a plain for-loop with no reference held on any of them. ha.c's on_standby_connected (raft-drive-async Task 2.1/2.2's own reconnect-offload completion callback) evicts a stale duplicate outbound peer channel via iomgr_unmonitor()+endpoint_unref() when a fresher reconnect completes -- but epoll_ctl(EPOLL_CTL_DEL) only stops a FUTURE epoll_wait() from returning events for that fd; it does not invalidate an event for that SAME stale endpoint already sitting in the CURRENT batch's events[] array (very plausible during the reconnect burst this spec's own faster cadence produces right after a new-primary election). endpoint_unref() reaching refcnt 0 immediately freed the endpoint (and its SSL object); the loop's next iteration then read that freed memory in io_handler -- a heap-use-after-free that (when ep->ssl still looked like a plausible pointer) manifested as a deep SEGV inside OpenSSL's ssl3_read_bytes, exactly matching the original crash.\n\nFIX: src/lib/netio.c -- io_worker's per-batch dispatch was extracted into io_worker_dispatch_batch() (~line 639), which now takes an endpoint_ref() on every endpoint in the batch BEFORE dispatching any of them and endpoint_unref()s each one only AFTER the whole batch is processed -- the same endpoint_ref/endpoint_unref bracket idiom this codebase already uses for the vtask dispatch path (bug-1298) and INVOKE_MSG_HANDLER, applied here to the one dispatcher that didn't yet have it. Lock-free atomics only, no I/O under the bracket -- satisfies the async-io steering no-blocking-in-the-hot-path rule. New regression test src/lib/testsuite/test_netio_batch_dispatch_uaf.c drives the real batch-dispatch loop with a synthetic 2-event epoll batch reproducing the exact same-batch eviction; TDD-verified RED (ASAN heap-use-after-free at netio.c:587) before the fix, GREEN after. Full detail in .wolf/buglog.json bug-1447 (status FIXED).\n\nINTEGRATION / COVERAGE / DESIGN: netio.c is an explicitly-owned Modified File in this spec's File Structure Plan; the fix sits entirely inside io_worker's own dispatch loop (\"the scheduling discipline of drive-thread work\", squarely in this spec's Boundary Commitments -- This Spec Owns). No Revalidation Trigger fired (the fix does not change the slow-lane submit contract, does not make iomgr/delaymgr execute work synchronously, does not change per-connection ordering semantics, and does not touch socket_connect). No new files, no dependency-direction change (netio lib -> ha controller unchanged). Requirements coverage unchanged from the prior GO-track pass (this was a runtime defect found BY this spec's own verification task 4.2/4.1 soak gate, not a requirements gap). No tasks are _Blocked:_; all 17 tasks (1.1-4.3) remain [x].\n\nDistinguish from the separate, already-accepted chaotic-mode finding (task 4.2's notes): chaotic-mode's round-4 committed-prefix divergence remains the documented pre-existing bug-851/795 family, diff-confirmed against pre-Phase-3 binaries, correctly out of scope for this spec. bug-1447 was different (a hard crash in the fully-supported SEQUENTIAL lifecycle) and is now closed.", + "known_limitations": [], + "ownership": "LOCAL", + "upstream_spec": "N/A", + "blocking_bug": null, + "remediation": [] + } +} diff --git a/src/include/netio.h b/src/include/netio.h index 3bc1aa0c..c4966b7b 100644 --- a/src/include/netio.h +++ b/src/include/netio.h @@ -207,4 +207,12 @@ extern int iomgr_unmonitor(iomgr_t*, endpoint_t*); extern int iomgr_start(iomgr_t*); extern int iomgr_fini(iomgr_t*); extern int iomgr_stop(iomgr_t*); + +/* bug-1447: expose io_worker's per-batch dispatch loop so a test can drive it + * directly with a synthetic epoll_event[] array (no real epoll_wait()/kernel + * needed) and verify that one event's handler cannot free an endpoint that a + * LATER event in the same batch still references. Not XCE_TESTING-gated: + * netio.c builds once into libxce.a with no per-target defines (see netio.c + * for why), matching every other function in this header. */ +extern void io_worker_dispatch_batch_for_test(struct epoll_event*, int); #endif diff --git a/src/lib/meson.build b/src/lib/meson.build index a99ab5a7..49430a4e 100644 --- a/src/lib/meson.build +++ b/src/lib/meson.build @@ -180,6 +180,7 @@ libxce_test_src = ['testsuite/test.c', 'testsuite/test_netio_arm_failure.c', 'testsuite/test_netio_close_propagation.c', 'testsuite/test_netio_bounded_connect.c', + 'testsuite/test_netio_batch_dispatch_uaf.c', 'testsuite/test_request_send_propagation.c', 'testsuite/test_raft.c', 'testsuite/test_wal_wire.c', diff --git a/src/lib/netio.c b/src/lib/netio.c index 55284904..06a41021 100644 --- a/src/lib/netio.c +++ b/src/lib/netio.c @@ -609,13 +609,70 @@ static int io_handler(struct epoll_event* p) return rc; } +/* + * io_worker_dispatch_batch: process ONE epoll_wait() batch. + * + * bug-1447: an earlier event in this SAME batch can, via its handler (e.g. + * ha.c's on_standby_connected evicting a stale duplicate outbound channel to + * the same peer), call endpoint_unref() on a DIFFERENT endpoint that ALSO has + * a ready event later in this batch — iomgr_unmonitor()'s epoll_ctl(DEL) only + * stops a FUTURE epoll_wait() from returning events for it; it does not + * invalidate an event already sitting in this stack-local `events[]` array. + * If that other endpoint's refcount was thereby dropped to 0, endpoint_unref + * fini's it (SSL_free(ep->ssl) + free(ep)) immediately — so the later + * io_handler() call in this SAME loop dereferences a freed endpoint_t (and a + * freed SSL*), a heap-use-after-free. This matched the ASAN DEADLYSIGNAL SEGV + * observed inside OpenSSL's ssl3_read_bytes (via handle_ssl_handshake) during + * raft-drive-async's reconnect-offload burst. + * + * INVOKE_MSG_HANDLER (netio.h) already refs/unrefs an endpoint around its OWN + * msg_handler call so a handler cannot free the ep it is CURRENTLY running + * on — but that bracket does not cover a DIFFERENT endpoint that a handler's + * side effects tear down. Take a reference on every endpoint in the batch + * BEFORE dispatching any of them, and release it only after the whole batch + * has been dispatched — mirroring the same endpoint_ref/endpoint_unref + * bracket idiom already used for the vtask dispatch path (bug-1298) and + * INVOKE_MSG_HANDLER, applied here to io_worker's own epoll dispatch loop. + * ref/unref are lock-free atomics (see endpoint_ref/endpoint_unref below), so + * this adds no lock and cannot block in this hot path. + */ +static void io_worker_dispatch_batch(struct epoll_event* events, int nfds) +{ + int i; + + for (i = 0; i < nfds; i++) { + endpoint_ref((endpoint_t*)events[i].data.ptr); + } + + for (i = 0; i < nfds; i++) { + io_handler(&events[i]); + } + + for (i = 0; i < nfds; i++) { + endpoint_unref((endpoint_t*)events[i].data.ptr); + } +} + +/* bug-1447: expose the batch dispatcher so a test can construct a synthetic + * epoll_event[] batch (bypassing a real epoll_wait()/kernel) and verify that + * processing one event cannot free an endpoint referenced by a LATER event + * in the same batch. netio.c is compiled once into libxce.a with no + * XCE_TESTING define (unlike controller/ha.c, which the controller test + * binary compiles directly), so — mirroring this file's own convention of + * unconditionally exporting its internals (endpoint_ref, endpoint_unref, + * endpoint_mark_tearing_down, etc. are all plain public functions, none + * XCE_TESTING-gated) — this seam is always compiled in rather than guarded. */ +void io_worker_dispatch_batch_for_test(struct epoll_event* events, int nfds) +{ + io_worker_dispatch_batch(events, nfds); +} + static void* io_worker(void* args) { iomgr_t* iomgr = (iomgr_t*)args; while (1) { int nfds = -1; - int i; struct epoll_event events[MAX_EPOLL_EVENTS]; nfds = epoll_wait(iomgr->epoll_fd, events, MAX_EPOLL_EVENTS, -1); @@ -629,9 +686,7 @@ static void* io_worker(void* args) return NULL; } - for (i = 0; i < nfds; i++) { - io_handler(&events[i]); - } + io_worker_dispatch_batch(events, nfds); pthread_testcancel(); } diff --git a/src/lib/testsuite/test.c b/src/lib/testsuite/test.c index 1ec5b869..01623008 100644 --- a/src/lib/testsuite/test.c +++ b/src/lib/testsuite/test.c @@ -43,6 +43,7 @@ extern void test_netio_on_fini(void); extern void test_netio_arm_failure(void); extern void test_netio_close_propagation(void); extern void test_netio_bounded_connect(void); +extern void test_netio_batch_dispatch_uaf(void); extern void test_request_send_propagation(void); extern void test_raft(void); extern void test_wal_wire(void); @@ -160,6 +161,7 @@ int main(int argc, char* argv[]) TEST(test_netio_arm_failure); TEST(test_netio_close_propagation); TEST(test_netio_bounded_connect); + TEST(test_netio_batch_dispatch_uaf); TEST(test_request_send_propagation); TEST(test_raft); TEST(test_wal_wire); diff --git a/src/lib/testsuite/test_netio_batch_dispatch_uaf.c b/src/lib/testsuite/test_netio_batch_dispatch_uaf.c new file mode 100644 index 00000000..465ef7b2 --- /dev/null +++ b/src/lib/testsuite/test_netio_batch_dispatch_uaf.c @@ -0,0 +1,165 @@ +/* + * test_netio_batch_dispatch_uaf.c — regression test for bug-1447. + * + * bug-1447: an ASAN DEADLYSIGNAL SEGV (near-null READ) was observed inside + * OpenSSL's ssl3_read_bytes <- tls_get_message_header <- state_machine <- + * handle_ssl_handshake (netio.c) <- io_handler <- io_worker, during a + * raft-drive-async reconnect-offload burst right after a new primary + * election. + * + * Root cause: io_worker's epoll dispatch loop processes ALL ready events + * from ONE epoll_wait() batch in a plain for-loop calling io_handler() per + * event. A handler for an EARLIER event in the batch (in production: ha.c's + * on_standby_connected(), which evicts a stale duplicate outbound channel to + * the same peer via iomgr_unmonitor() + endpoint_unref()) can drop the LAST + * reference on a DIFFERENT endpoint that also has a ready event LATER in the + * SAME batch. iomgr_unmonitor()'s epoll_ctl(EPOLL_CTL_DEL) only stops a + * FUTURE epoll_wait() from returning events for that fd — it does not + * invalidate an event already sitting in the batch's stack-local + * `events[]` array. endpoint_unref() reaching refcnt 0 immediately + * SSL_free()s ep->ssl and free()s the endpoint_t itself, so the later + * io_handler() call in the SAME loop dereferences freed memory — a + * heap-use-after-free that manifests deep inside OpenSSL when the freed + * (or poisoned/reused) ep->ssl pointer is handed to SSL_do_handshake(). + * + * This test reproduces the SAME-BATCH mechanism deterministically, without + * any real socket/SSL/epoll machinery, by driving + * io_worker_dispatch_batch_for_test() (the XCE_TESTING seam exposing + * io_worker's real batch-dispatch loop) with a synthetic 2-event batch: + * + * events[0] -> ep_evictor: a conn_handler that endpoint_unref()s ep_victim, + * simulating on_standby_connected's stale-duplicate eviction. + * events[1] -> ep_victim: refcnt==1 (its only ref is the one dropped by + * ep_evictor's handler) — a conn_handler stub that just + * records that it ran. + * + * RED (pre-fix): ep_evictor's handler drops ep_victim's refcount to 0, + * freeing it (SSL_free + free(ep)) DURING events[0]'s processing. Processing + * events[1] then dereferences the freed ep_victim inside io_handler() — under + * this project's mandatory ASAN build this aborts the process + * (heap-use-after-free), which is the expected RED failure mode for this + * exact bug class (the original bug was itself only caught by ASAN). + * + * GREEN (post-fix): io_worker_dispatch_batch takes a reference on every + * endpoint in the batch before dispatching any of them and releases it only + * after the whole batch is processed, so ep_evictor's endpoint_unref() drops + * ep_victim from 2 (owner + batch) to 1 (batch only) — not to 0. ep_victim + * survives to have events[1] safely dispatched, and is only freed by + * io_worker's own trailing unref after the whole batch completes. + */ + +#define _GNU_SOURCE +#include +#include +#include +#include + +#include +#include "netio.h" + +/* ── Static state for the injected conn_handlers ─────────────────────── */ + +static endpoint_t *g_victim_ep = NULL; +static int g_victim_ran = 0; +static int g_evictor_ran = 0; + +static void reset_state(void) +{ + g_victim_ep = NULL; + g_victim_ran = 0; + g_evictor_ran = 0; +} + +/* Simulates ha.c's on_standby_connected: while handling ITS OWN event, tears + * down a DIFFERENT, already-batched endpoint (the stale duplicate). */ +static int evictor_conn_handler(struct epoll_event *p) +{ + (void)p; + g_evictor_ran = 1; + /* Drop the victim's only ref — pre-fix this frees it immediately. */ + endpoint_unref(g_victim_ep); + return 0; +} + +/* The "stale duplicate" endpoint's handler — if this runs on a still-valid + * endpoint, the fix held. If ep was already freed, this call itself is + * running on freed memory (the crash already happened by the time we get + * here, or ASAN has already aborted the process). */ +static int victim_conn_handler(struct epoll_event *p) +{ + (void)p; + g_victim_ran = 1; + return 0; +} + +/* + * Test: processing an earlier batch event that evicts a later-batched + * endpoint's last reference must NOT free that endpoint before its own + * event is dispatched. + */ +static void test_batch_dispatch_survives_cross_endpoint_eviction(void) +{ + reset_state(); + + endpoint_t *ep_evictor = endpoint_get(); + endpoint_t *ep_victim = endpoint_get(); + TEST_ASSERT_NOT_NULL(ep_evictor); + TEST_ASSERT_NOT_NULL(ep_victim); + + /* Both endpoints are past handshake and dispatch through conn_handler + * (the msg_handler-less path in io_handler), exactly like an established + * outbound standby control channel. */ + ep_evictor->ssl_handshake_done = 1; + ep_evictor->msg_handler = NULL; + ep_evictor->conn_handler = evictor_conn_handler; + + ep_victim->ssl_handshake_done = 1; + ep_victim->msg_handler = NULL; + ep_victim->conn_handler = victim_conn_handler; + + /* ep_victim starts with exactly ONE reference (its "owner" ref from + * endpoint_get()) — the same ref on_standby_connected's eviction path + * drops via endpoint_unref() in production (ha.c:8327). */ + TEST_ASSERT_EQUAL_INT(1, atomic_load(&ep_victim->refcnt)); + + g_victim_ep = ep_victim; + + struct epoll_event events[2]; + memset(events, 0, sizeof(events)); + events[0].events = EPOLLIN; + events[0].data.ptr = ep_evictor; + events[1].events = EPOLLIN; + events[1].data.ptr = ep_victim; + + /* Drive the REAL io_worker batch-dispatch loop (via the XCE_TESTING + * seam) with this synthetic 2-event batch. */ + io_worker_dispatch_batch_for_test(events, 2); + + /* Both handlers must have run... */ + TEST_ASSERT_EQUAL_INT_MESSAGE(1, g_evictor_ran, + "the evictor's own event must have been dispatched"); + TEST_ASSERT_EQUAL_INT_MESSAGE(1, g_victim_ran, + "bug-1447: the victim endpoint's event must be safely dispatched " + "even though an EARLIER event in the same batch dropped its last " + "reference — reaching this point without an ASAN heap-use-after-free " + "abort IS the regression proof"); + + /* ...and ep_victim must be gone now (unref'd to 0 exactly once the whole + * batch finished), not leaked and not double-freed. Nothing further to + * assert on it directly (it is freed memory), but a clean process exit + * (no ASAN report) for the whole test binary is the authoritative check, + * verified by the test harness's exit code. */ + + endpoint_unref(ep_evictor); + + printf("test_batch_dispatch_survives_cross_endpoint_eviction: PASS\n"); +} + +/* ── Test suite entry point ───────────────────────────────────────── */ + +void test_netio_batch_dispatch_uaf(void) +{ + printf("\n>> netio io_worker batch-dispatch UAF tests (bug-1447)\n\n"); + + RUN_TEST(test_batch_dispatch_survives_cross_endpoint_eviction); +}