From 6d3c2c3733356f4bd6e349b69fde317617a10385 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Thu, 13 Aug 2026 03:17:13 +0000 Subject: [PATCH] dns: keep the c-ares uv_poll_t alive while its poll callback is on the stack (Windows) On Windows each c-ares socket is driven by a uv_poll_t embedded in a heap UvDnsPoll. c-ares closes a UDP socket as soon as its answer is processed, which happens inside on_dns_poll_uv, so uv_close() is issued from within the handle's own poll callback. The callback then drains microtasks; a promise reaction that spins the event loop (Bun.build() with a plugin whose setup() returns a pending promise, bun:test's .resolves, ...) runs a nested uv_run, whose endgame phase invoked on_close_uv and freed the UvDnsPoll. libuv's uv__fast_poll_process_poll_req frame for that handle was still suspended underneath and reads the handle again once the callback returns; with the stale bytes it re-queued the endgame, close_cb ran a second time and the struct was freed twice. A script that re-enters the loop from resolve4() reactions segfaults after about ten queries on the release build. Count the on_dns_poll_uv frames active for a handle. on_close_uv only frees the struct when none is active; otherwise it marks the handle and the outermost frame, once its microtask drain is over, hands the struct to the event loop's task queue, which is drained only after the libuv callback has returned. The common path (close callback delivered after the poll callback returned) is unchanged. The libuv side of the same scenario is the win-poll-no-reendgame-after- close patch from #33018 (identical copy): with the struct kept alive, libuv would otherwise re-queue the endgame of the already CLOSED handle from the post-callback check and invoke close_cb twice (uv__poll_endgame asserts on this in debug builds). --- .../win-poll-no-reendgame-after-close.patch | 29 ++++++ scripts/build/deps/libuv.ts | 13 ++- src/runtime/dns_jsc/dns.rs | 92 ++++++++++++++----- test/js/node/dns/node-dns.test.js | 84 +++++++++++++++++ 4 files changed, 195 insertions(+), 23 deletions(-) create mode 100644 patches/libuv/win-poll-no-reendgame-after-close.patch diff --git a/patches/libuv/win-poll-no-reendgame-after-close.patch b/patches/libuv/win-poll-no-reendgame-after-close.patch new file mode 100644 index 000000000000..6f6e536bd262 --- /dev/null +++ b/patches/libuv/win-poll-no-reendgame-after-close.patch @@ -0,0 +1,29 @@ +--- a/src/win/poll.c ++++ b/src/win/poll.c +@@ -217,8 +217,14 @@ static void uv__fast_poll_process_poll_req(uv_loop_t* loop, uv_poll_t* handle, + handle->submitted_events_2)) != 0) { + uv__fast_poll_submit_poll_req(loop, handle); + } else if ((handle->flags & UV_HANDLE_CLOSING) && ++ !(handle->flags & UV_HANDLE_CLOSED) && + handle->submitted_events_1 == 0 && + handle->submitted_events_2 == 0) { ++ /* A nested uv_run entered from inside poll_cb may have already run this ++ * handle's endgame: uv__process_endgames cleared ENDGAME_QUEUED and ++ * uv__handle_close set CLOSED. Re-queuing it here would run the endgame ++ * again, which uv__poll_endgame asserts against and which invokes ++ * close_cb a second time. */ + uv__want_endgame(loop, (uv_handle_t*) handle); + } + } +@@ -418,8 +424,11 @@ static void uv__slow_poll_process_poll_req(uv_loop_t* loop, uv_poll_t* handle, + handle->submitted_events_2)) != 0) { + uv__slow_poll_submit_poll_req(loop, handle); + } else if ((handle->flags & UV_HANDLE_CLOSING) && ++ !(handle->flags & UV_HANDLE_CLOSED) && + handle->submitted_events_1 == 0 && + handle->submitted_events_2 == 0) { ++ /* Same as the fast path: never re-queue an endgame that a nested uv_run ++ * already ran for this handle. */ + uv__want_endgame(loop, (uv_handle_t*) handle); + } + } diff --git a/scripts/build/deps/libuv.ts b/scripts/build/deps/libuv.ts index 3ff7e346c9f0..db4cd838ed01 100644 --- a/scripts/build/deps/libuv.ts +++ b/scripts/build/deps/libuv.ts @@ -53,7 +53,18 @@ export const libuv: Dependency = { // an in-process loopback fetch().abort() can fall into. To upstream: // send to libuv/libuv with the wepoll/ReactOS references in the patch // comment as the rationale. - patches: ["patches/libuv/win-poll-rearm-before-callback.patch", "patches/libuv/win-poll-abort-with-disconnect.patch"], + // + // win-poll-no-reendgame-after-close: the post-poll_cb endgame check in + // uv__fast_poll_process_poll_req (and its slow-poll sibling) re-queues a + // handle whose endgame a nested uv_run, entered from inside poll_cb, + // already ran, which double-invokes close_cb. Guard the check on + // !(flags & UV_HANDLE_CLOSED), which uv__poll_endgame already asserts. + // Nested uv_run is outside libuv's contract, so this is not upstreamable. + patches: [ + "patches/libuv/win-poll-rearm-before-callback.patch", + "patches/libuv/win-poll-abort-with-disconnect.patch", + "patches/libuv/win-poll-no-reendgame-after-close.patch", + ], build: () => ({ kind: "direct", diff --git a/src/runtime/dns_jsc/dns.rs b/src/runtime/dns_jsc/dns.rs index cb77f288f2af..2f554ecb971d 100644 --- a/src/runtime/dns_jsc/dns.rs +++ b/src/runtime/dns_jsc/dns.rs @@ -3685,6 +3685,15 @@ pub(crate) struct UvDnsPoll { // `Resolver::deref`, which may write/free `*this`. pub parent: *mut Resolver, pub socket: c_ares::ares_socket_t, + /// `on_dns_poll_uv` frames for this handle currently on the stack. libuv + /// reads `poll` again after each one returns, so while this is non-zero + /// the allocation must stay alive even if libuv has already closed the + /// handle (a nested `uv_run` spun from JS inside the callback runs the + /// endgame phase, and with it `on_close_uv`, underneath the frame). + poll_cb_depth: u32, + /// `on_close_uv` ran while `poll_cb_depth` was non-zero; the outermost + /// frame frees the allocation once it unwinds instead. + closed: bool, pub poll: libuv::uv_poll_t, } @@ -3694,6 +3703,8 @@ impl UvDnsPoll { bun_core::heap::into_raw(Box::new(Self { parent, socket, + poll_cb_depth: 0, + closed: false, poll: bun_core::ffi::zeroed(), })) } @@ -3702,6 +3713,20 @@ impl UvDnsPoll { unsafe { drop(bun_core::heap::take(this)) }; } + /// libuv's `uv__fast_poll_process_poll_req` (and its slow-poll sibling) + /// still reads `poll` after `on_dns_poll_uv` returns, so a handle whose + /// close callback already ran underneath that frame is freed from the task + /// queue, which the loop drains only after the libuv callback has returned. + fn destroy_after_poll_cb_returns(this: *mut Self, vm: &VirtualMachine) { + fn run(this: *mut UvDnsPoll) -> bun_event_loop::JsResult<()> { + UvDnsPoll::destroy(this); + Ok(()) + } + // `new_owned` also frees `this` if the VM tears down before the task runs. + vm.event_loop_mut() + .enqueue_task(jsc::ManagedTask::ManagedTask::new_owned(this, run)); + } + fn from_poll(poll: *mut libuv::uv_poll_t) -> *mut Self { // SAFETY: poll points to UvDnsPoll.poll unsafe { bun_core::from_field_ptr!(UvDnsPoll, poll, poll) } @@ -4730,44 +4755,67 @@ impl Resolver { ) { let poll = UvDnsPoll::from_poll(watcher); // SAFETY: `poll` is the live `UvDnsPoll` recovered from libuv's `watcher` - // via `from_poll` (libuv guarantees the handle outlives this callback). + // via `from_poll`; `on_close_uv` leaves it allocated while + // `poll_cb_depth` is non-zero, so it stays valid through this frame even + // if c-ares closes the socket below and JS then re-enters the loop. // `parent` is the heap-allocated Resolver back-ptr (set in // `on_dns_socket_state`); it is kept alive across `Channel::process` by the // `ref_()`/`_deref` bracket below. `channel` is non-null because c-ares // must have been initialized for this poll callback to fire. unsafe { + (*poll).poll_cb_depth += 1; let parent: *mut Resolver = (*poll).parent; let vm = (*parent).vm.get(); - let _exit = vm.enter_event_loop_scope(); - // SAFETY: `parent` is the live heap-allocated Resolver back-ptr. - let _deref = Self::ref_scope(parent); - // channel must be non-null here as c_ares must have been initialized if we're receiving callbacks - let channel = (*parent).channel.get().unwrap(); - if status < 0 { - // an error occurred. just pretend that the socket is both readable and writable. - // https://github.com/nodejs/node/blob/8a41d9b636be86350cd32847c3f89d327c4f6ff7/src/cares_wrap.cc#L93 - (*channel).process((*poll).socket, true, true); - } else { - (*channel).process( - (*poll).socket, - events & libuv::UV_READABLE != 0, - events & libuv::UV_WRITABLE != 0, - ); - } + { + let _exit = vm.enter_event_loop_scope(); + // SAFETY: `parent` is the live heap-allocated Resolver back-ptr. + let _deref = Self::ref_scope(parent); + // channel must be non-null here as c_ares must have been initialized if we're receiving callbacks + let channel = (*parent).channel.get().unwrap(); + if status < 0 { + // an error occurred. just pretend that the socket is both readable and writable. + // https://github.com/nodejs/node/blob/8a41d9b636be86350cd32847c3f89d327c4f6ff7/src/cares_wrap.cc#L93 + (*channel).process((*poll).socket, true, true); + } else { + (*channel).process( + (*poll).socket, + events & libuv::UV_READABLE != 0, + events & libuv::UV_WRITABLE != 0, + ); + } - // See `on_dns_poll` for why this re-check follows `ares_process_fd`. - if !(*parent).any_requests_pending() { - (*parent).remove_timer(); + // See `on_dns_poll` for why this re-check follows `ares_process_fd`. + if !(*parent).any_requests_pending() { + (*parent).remove_timer(); + } + // `_deref` drops here (may free the resolver), then `_exit` drains + // microtasks: JS runs, and may spin a nested `uv_run` whose endgame + // phase calls `on_close_uv` for this very handle. + } + (*poll).poll_cb_depth -= 1; + if (*poll).poll_cb_depth == 0 && (*poll).closed { + UvDnsPoll::destroy_after_poll_cb_returns(poll, vm); } } } #[cfg(windows)] pub(crate) unsafe extern "C" fn on_close_uv(watcher: *mut libuv::uv_handle_t) { + let poll = UvDnsPoll::from_poll(watcher.cast()); // SAFETY: libuv invokes the close cb with the same handle pointer passed // to `uv_close`, which was `&mut UvDnsPoll::poll` (a `uv_poll_t` whose - // header is `uv_handle_t`); `from_poll` recovers the containing struct. - let poll = UvDnsPoll::from_poll(watcher.cast()); + // header is `uv_handle_t`), and exactly once per handle + // (patches/libuv/win-poll-no-reendgame-after-close.patch covers the + // nested case). `from_poll` recovers the containing struct, which is + // still allocated: it is freed only below or, after this function + // defers, by the `on_dns_poll_uv` frame that is still on the stack. + unsafe { + debug_assert!(!(*poll).closed); + if (*poll).poll_cb_depth > 0 { + (*poll).closed = true; + return; + } + } UvDnsPoll::destroy(poll); } diff --git a/test/js/node/dns/node-dns.test.js b/test/js/node/dns/node-dns.test.js index 9f3fd2b7408c..697554f98d53 100644 --- a/test/js/node/dns/node-dns.test.js +++ b/test/js/node/dns/node-dns.test.js @@ -491,6 +491,90 @@ test.skipIf(!isLinux)("dns.lookup uses getaddrinfo, not the c-ares resolver", as expect(exitCode).toBe(0); }); +// Windows drives each c-ares socket with a libuv uv_poll_t. c-ares closes the +// UDP socket as soon as the answer arrives, i.e. from inside that handle's poll +// callback, and the callback's microtask drain then runs the query's promise +// reactions. A reaction that synchronously spins the event loop (a nested +// uv_run) used to run libuv's close callback, which freed the uv_poll_t while +// libuv's own poll-dispatch frame for it was still suspended underneath and +// reads the handle again once the callback returns. The freed handle was then +// closed a second time, and the release build crashed after a few rounds. +// POSIX polls c-ares sockets with FilePoll, which is not involved. +test.skipIf(!isWindows)("dns.Resolver: a query's promise reaction may re-enter the event loop", async () => { + const rounds = 20; + const fixture = ` + const dns = require("node:dns"); + // Echo the question back with one A record for it. + function answer(query) { + let off = 12; + while (off < query.length && query[off] !== 0) off += query[off] + 1; + off += 1 + 2 + 2; + const header = Buffer.alloc(12); + header[0] = query[0]; + header[1] = query[1]; + header[2] = 0x81; // QR=1, RD=1 + header[3] = 0x80; // RA=1 + header[5] = 1; // QDCOUNT + header[7] = 1; // ANCOUNT + const record = Buffer.from([ + 0xc0, 0x0c, // NAME: pointer to the question name + 0x00, 0x01, // TYPE A + 0x00, 0x01, // CLASS IN + 0x00, 0x00, 0x00, 0x3c, // TTL 60 + 0x00, 0x04, // RDLENGTH + 127, 0, 0, 1, + ]); + return Buffer.concat([header, query.subarray(12, off), record]); + } + const server = await Bun.udpSocket({ + hostname: "127.0.0.1", + port: 0, + socket: { data(sock, buf, port, addr) { sock.send(answer(Buffer.from(buf)), port, addr); } }, + }); + const resolver = new dns.promises.Resolver({ timeout: 5000, tries: 1 }); + resolver.setServers(["127.0.0.1:" + server.port]); + + // Bun.build() runs a plugin's setup() while it parses its options and, if + // setup() returns a pending promise, spins the event loop until it settles. + // Called from a promise reaction, that is a nested event loop tick inside + // the I/O callback that settled the promise. + let reentries = 0; + const builds = []; + function reenterEventLoop() { + const before = reentries; + builds.push(Bun.build({ + entrypoints: ["/entry.js"], + files: { "/entry.js": "" }, + plugins: [{ + name: "spin", + setup: () => new Promise(resolve => setImmediate(() => { reentries++; resolve(); })), + }], + })); + if (reentries !== before + 1) throw new Error("Bun.build() returned without spinning the event loop"); + } + + // A different name every round: c-ares answers a repeated name from its + // own cache without touching the network, and the socket is the point. + for (let i = 0; i < ${rounds}; i++) { + const addresses = await resolver.resolve4("round" + i + ".example.test").then(addresses => { + reenterEventLoop(); + return addresses; + }); + if (addresses[0] !== "127.0.0.1") throw new Error("unexpected answer: " + addresses); + } + await Promise.all(builds); + server.close(); + console.log("reentries=" + reentries); + `; + await using proc = Bun.spawn({ + cmd: [bunExe(), "-e", fixture], + env: bunEnv, + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect({ stdout, stderr, exitCode }).toEqual({ stdout: `reentries=${rounds}\n`, stderr: "", exitCode: 0 }); +}); + test("dns.getServers", () => { function parseResolvConf() { const servers = [];