dns: keep the c-ares uv_poll_t alive while its poll callback is on the stack (Windows)

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);
+   }
+ }

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",

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

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 = [];