fix(netio): bug-1447 same-batch endpoint use-after-free in io_worker dispatch
Raft-drive-async's reconnect-offload burst (Task 2.1/2.2) exposed a latent race in io_worker's epoll dispatch loop: ha.c's on_standby_connected() evicts a stale duplicate peer channel via iomgr_unmonitor()+endpoint_unref(), but epoll_ctl(DEL) only stops FUTURE epoll_wait() calls -- it does not invalidate an event for that same endpoint already sitting in the CURRENT batch's events[] array. A later event in the same batch then dereferences the freed endpoint_t/SSL*, producing the ASAN SEGV observed deep inside OpenSSL's ssl3_read_bytes during raft-drive-async's round-9 soak. The original "concurrent SSL_do_handshake from two io_worker threads" hypothesis was wrong and is retracted -- iomgr_start() creates exactly one io_worker thread per iomgr_t. Fix: extract io_worker_dispatch_batch(), which takes endpoint_ref() on every endpoint in the batch before dispatching any of them and endpoint_unref()s each only after the whole batch completes -- the same lock-free ref/unref bracket already used for bug-1298 and INVOKE_MSG_HANDLER, just never applied to io_worker's own loop. Pure atomics, no locks, no blocking added to the hot path. Adds test_netio_batch_dispatch_uaf.c, which drives the real dispatch loop with a synthetic epoll_event[] batch (TDD RED confirmed via ASAN heap-use-after-free before the fix, GREEN after). Verified further with two independent clean 10-round live sequential HA soaks (33 and 36 jobs, zero loss) and full ASAN suite runs (xcelib_test 294/0, controller_unit_test 972/0). raft-drive-async's kiro-validate-impl re-run flips NO-GO to GO.
This commit is contained in:
parent
4d2c90389f
commit
426728bb98
6 changed files with 249 additions and 8 deletions
|
|
@ -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();
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue