diff --git a/packages/bun-usockets/src/eventing/libuv.c b/packages/bun-usockets/src/eventing/libuv.c index 3ea446c7bdc0..be0f46566ec3 100644 --- a/packages/bun-usockets/src/eventing/libuv.c +++ b/packages/bun-usockets/src/eventing/libuv.c @@ -47,8 +47,7 @@ static struct us_socket_t *us_internal_poll_cb_adopted_socket(struct us_poll_t * return us_internal_socket_follow_adopted((struct us_socket_t *)wp); } -/* uv_poll_t->data always (except for most times after calling us_poll_stop) - * points to the us_poll_t */ +/* uv_poll_t->data always points to the us_poll_t */ static void poll_cb(uv_poll_t *p, int status, int events) { /* UV_DISCONNECT (Windows AFD): the peer closed its write side. A FIN * arriving after this side already half-closed and stopped reading never @@ -136,18 +135,13 @@ static void check_cb(uv_check_t *p) { us_internal_loop_post(loop); } -/* Not used for polls, since polls need two frees */ +/* Not used for polls: their uv handle is a separate allocation */ static void close_cb_free(uv_handle_t *h) { us_free(h->data); } -/* This one is different for polls, since we need two frees here */ -static void close_cb_free_poll(uv_handle_t *h) { - /* It is only in case we called us_poll_stop then quickly us_poll_free that we - * enter this. Most of the time, actual freeing is done by us_poll_free. */ - if (h->data) { - us_free(h->data); - us_free(h); - } -} +/* A poll's uv_poll_t is a separate allocation that libuv keeps linked into + * the loop until its close completes; us_poll_free hands it over with + * uv_close and this frees it. The us_poll_t itself is freed by us_poll_free. */ +static void close_cb_free_uv_poll(uv_handle_t *h) { us_free(h); } static void timer_cb(uv_timer_t *t) { struct us_internal_callback_t *cb = t->data; @@ -168,23 +162,19 @@ void us_poll_init(struct us_poll_t *p, LIBUS_SOCKET_DESCRIPTOR fd, } void us_poll_free(struct us_poll_t *p, struct us_loop_t *loop) { - // poll was resized and dont own uv_poll_t anymore - if(!p->uv_p) { - us_free(p); - return; - } - /* The idea here is like so; in us_poll_stop we call uv_close after setting - * data of uv-poll to 0. This means that in close_cb_free we call free on 0 - * with does nothing, since us_poll_stop should not really free the poll. - * HOWEVER, if we then call us_poll_free while still closing the uv-poll, we - * simply change back the data to point to our structure so that we actually - * do free it like we should. */ - if (uv_is_closing((uv_handle_t *)p->uv_p)) { - p->uv_p->data = p; - } else { - us_free(p->uv_p); - us_free(p); + /* NULL when a resize moved the handle to the new block. */ + if (p->uv_p) { + if (p->uv_p->type == UV_POLL) { + /* us_poll_start_rc initialised it (uv__handle_init linked it into the + * loop): only libuv can unlink it, and it is freed once that completes. + * This is the one place a poll handle is uv_close'd - see us_poll_stop. */ + uv_close((uv_handle_t *)p->uv_p, close_cb_free_uv_poll); + } else { + /* Never initialised: libuv has not seen this block. */ + us_free(p->uv_p); + } } + us_free(p); } int us_poll_start_rc(struct us_poll_t *p, struct us_loop_t *loop, int events) { @@ -196,9 +186,9 @@ int us_poll_start_rc(struct us_poll_t *p, struct us_loop_t *loop, int events) { /* uv_poll_init_socket (win/poll.c) can fail either before uv__handle_init * (ioctlsocket FIONBIO) or after it (getsockopt SO_PROTOCOL_INFOW). The * latter leaves the handle linked into loop->handle_queue with - * submitted_events_* still unset. Zero first so, on failure, ->type - * distinguishes the two states and the fields uv__poll_close reads are 0 - * rather than garbage. */ + * submitted_events_* still unset. Zero first so, on failure, ->type tells + * us_poll_free which of the two states it is in and the fields + * uv__poll_close reads are 0 rather than garbage. */ memset(p->uv_p, 0, sizeof(uv_poll_t)); p->uv_p->data = p; @@ -212,20 +202,8 @@ int us_poll_start_rc(struct us_poll_t *p, struct us_loop_t *loop, int events) { #endif rc = uv_poll_init_socket(loop->uv_loop, p->uv_p, p->fd); if (rc < 0) { + /* The caller's us_poll_free disposes of uv_p either way (see there). */ int saved = LIBUS_ERR; - if (p->uv_p->type == UV_POLL) { - /* uv__handle_init ran: the handle is in loop->handle_queue. Close it - * through libuv so it is unlinked; the caller's us_poll_free sees - * uv_is_closing and hands ownership to close_cb_free_poll. */ - p->uv_p->data = 0; - uv_close((uv_handle_t *)p->uv_p, close_cb_free_poll); - } else { - /* Never reached uv__handle_init: uv_p is still our raw block. Free it - * here and null the pointer so the caller's us_poll_free takes the - * !uv_p fast path (its uv_is_closing check would read garbage). */ - us_free(p->uv_p); - p->uv_p = NULL; - } errno = saved ? saved : -rc; return rc; } @@ -261,14 +239,17 @@ int us_poll_change(struct us_poll_t *p, struct us_loop_t *loop, int events) { void us_poll_stop(struct us_poll_t *p, struct us_loop_t *loop) { if(!p->uv_p) return; + /* Stop only; the uv_close waits for us_poll_free. A socket is routinely + * closed from inside its own poll_cb, and JS run from there may drive the + * loop again (waitForPromise) before poll_cb returns. Starting the close + * here would let that nested uv_run complete it and free the uv_poll_t + * while libuv's uv__fast_poll_process_poll_req for this very handle is still + * on the stack: once poll_cb returns it reads the freed handle and queues + * its endgame a second time (double close callback, corrupted handle + * queue). us_poll_free runs from the outermost us_internal_loop_post, which + * is never inside a poll_cb. uv_poll_stop alone already guarantees poll_cb + * is not invoked for this handle again. */ uv_poll_stop(p->uv_p); - - /* We normally only want to close the poll here, not free it. But if we stop - * it, then quickly "free" it with us_poll_free, we postpone the actual - * freeing to close_cb_free_poll whenever it triggers. That's why we set data - * to null here, so that us_poll_free can reset it if needed */ - p->uv_p->data = 0; - uv_close((uv_handle_t *)p->uv_p, close_cb_free_poll); } int us_poll_events(struct us_poll_t *p) { @@ -295,7 +276,9 @@ void us_loop_pump(struct us_loop_t *loop) { * bun:test) supply their own keep-going predicate, so force exactly one * non-blocking iteration; UV_RUN_NOWAIT keeps the poll timeout at 0. */ loop->uv_loop->active_handles++; + loop->data.tick_depth++; uv_run(loop->uv_loop, UV_RUN_NOWAIT); + loop->data.tick_depth--; loop->uv_loop->active_handles--; } @@ -374,7 +357,13 @@ void us_loop_run(struct us_loop_t *loop) { Bun__JSC_onBeforeWait(loop->data.jsc_vm, (uint64_t) uv_now(loop->uv_loop) * 1000000ULL); } + /* Same bracket as us_loop_run_bun_tick on epoll/kqueue: a socket callback + * dispatched inside this uv_run may drive the loop again (waitForPromise -> + * us_loop_pump), and us_internal_loop_post only frees closed sockets from + * the outermost tick so an outer dispatch never resumes on a freed socket. */ + loop->data.tick_depth++; uv_run(loop->uv_loop, UV_RUN_ONCE); + loop->data.tick_depth--; } struct us_poll_t *us_create_poll(struct us_loop_t *loop, int fallthrough, @@ -382,6 +371,8 @@ struct us_poll_t *us_create_poll(struct us_loop_t *loop, int fallthrough, struct us_poll_t *p = (struct us_poll_t *)us_malloc(sizeof(struct us_poll_t) + ext_size); p->uv_p = us_malloc(sizeof(uv_poll_t)); + /* Not a libuv handle until us_poll_start_rc; us_poll_free checks this. */ + p->uv_p->type = UV_UNKNOWN_HANDLE; p->uv_p->data = p; return p; } diff --git a/packages/bun-usockets/src/internal/loop_data.h b/packages/bun-usockets/src/internal/loop_data.h index 959a9110204c..645654430d54 100644 --- a/packages/bun-usockets/src/internal/loop_data.h +++ b/packages/bun-usockets/src/internal/loop_data.h @@ -88,10 +88,11 @@ struct us_internal_loop_data_t { /* We do not care if this flips or not, it doesn't matter */ size_t iteration_nr; void* jsc_vm; - /* Reentrancy depth of us_loop_run_bun_tick. When >1, we are inside a - * nested tick (e.g. waitForPromise from a poll callback). Freeing closed - * sockets must be deferred to the outermost tick so the outer dispatch - * doesn't read a freed poll. */ + /* Reentrancy depth of the loop tick (us_loop_run / us_loop_run_bun_tick / + * us_loop_pump). When >1, we are inside a nested tick (e.g. + * waitForPromise from a poll callback). Freeing closed sockets must be + * deferred to the outermost tick so the outer dispatch doesn't read a + * freed poll. */ int tick_depth; }; diff --git a/test/js/bun/net/close-inside-data-reentrant-fixture.ts b/test/js/bun/net/close-inside-data-reentrant-fixture.ts new file mode 100644 index 000000000000..b56957a32542 --- /dev/null +++ b/test/js/bun/net/close-inside-data-reentrant-fixture.ts @@ -0,0 +1,49 @@ +// Spawned by socket.test.ts as `bun test `: it has to run under the +// test runner because expect(promise).resolves waits by driving the event loop +// synchronously, which is what nests event-loop ticks inside the socket's data +// callback while the dispatch for that socket is still on the stack. +import { expect, test } from "bun:test"; + +test("a socket closed inside its data callback survives nested event-loop ticks until the dispatch returns", async () => { + using server = Bun.listen({ + hostname: "127.0.0.1", + port: 0, + socket: { + open(socket) { + socket.write("x"); + }, + data() {}, + }, + }); + + for (let i = 0; i < 8; i++) { + const returned = Promise.withResolvers(); + const churn: Promise[] = []; + await Bun.connect({ + hostname: "127.0.0.1", + port: server.port, + socket: { + data(socket) { + // Closing moves the socket to the loop's closed list; it may only be + // freed once this callback (and the dispatch that called it) is done. + socket.terminate(); + // Nested ticks: timers, I/O and the loop's post phase all run here. + expect(new Promise(resolve => setTimeout(resolve, 5))).resolves.toBeUndefined(); + // Allocations of the same size class as the closed socket, so a + // prematurely freed block is likely to be handed out again before + // the outer dispatch looks at it. + for (let j = 0; j < 16; j++) { + churn.push( + Bun.connect({ hostname: "127.0.0.1", port: server.port, socket: { data() {} } }).then(s => s.terminate()), + ); + } + returned.resolve(); + }, + }, + }); + await returned.promise; + await Promise.all(churn); + // Let the outer dispatch unwind and the loop reach its post phase. + await new Promise(resolve => setImmediate(resolve)); + } +}); diff --git a/test/js/bun/net/socket.test.ts b/test/js/bun/net/socket.test.ts index 6619e16894c5..708740c584b4 100644 --- a/test/js/bun/net/socket.test.ts +++ b/test/js/bun/net/socket.test.ts @@ -4662,3 +4662,25 @@ describe.concurrent("a socket closed by data() while its peer's reset is being d expect(exitCode).toBe(0); }); }); + +describe.concurrent("a socket closed by data() which then re-enters the event loop before returning", () => { + // The fixture runs under `bun test` so that expect(promise).resolves can drive + // nested event-loop ticks from inside the data callback. The closed socket must + // stay allocated until the dispatch that invoked data() has returned; the loop + // used to free it from a nested tick on Windows, and the outer dispatch then + // read (and the allocator reused) freed memory. + it("is not freed until the dispatch that called data() has returned", async () => { + await using proc = Bun.spawn({ + cmd: [bunExe(), "test", fileURLToPath(new URL("./close-inside-data-reentrant-fixture.ts", import.meta.url))], + env: bunEnv, + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + // stdout carries only the runner's version banner; results go to stderr. + expect(stdout).toMatch(/^bun test v\S+ \(\S+\)\n$/); + expect(stderr).toContain(" 1 pass"); + expect(proc.signalCode).toBeNull(); + expect(exitCode).toBe(0); + }); +});