Fix Windows heap corruption when a socket's data callback closes it and re-enters the event loop

packages/bun-usockets/src/eventing/libuv.c

@@ -181,11 +181,14 @@ 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. */
+  /* us_poll_free normally re-pointed data at the us_poll_t before this runs.
+   * If a nested tick's loop_post deferred the closed-socket sweep (see
+   * tick_depth in us_loop_run), we run first: mark the handle for us_poll_free. */
   if (h->data) {
     us_free(h->data);
     us_free(h);
+  } else {
+    h->data = h;
   }
 }
 
@@ -213,6 +216,14 @@ void us_poll_free(struct us_poll_t *p, struct us_loop_t *loop) {
     us_free(p);
     return;
   }
+  /* close_cb_free_poll already ran: a nested tick deferred this sweep to the
+   * outermost loop_post (tick_depth), so libuv finished closing the handle
+   * before we got here. It is done with uv_p; we free both. */
+  if (p->uv_p->data == (void *)p->uv_p) {
+    us_free(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.
@@ -329,10 +340,17 @@ void us_loop_pump(struct us_loop_t *loop) {
    * for unref'd handles (subprocess exit packets, socket events) and due
    * timers are never processed. Bun's outer drive loops (wait_for_promise,
    * bun:test) supply their own keep-going predicate, so force exactly one
-   * non-blocking iteration; UV_RUN_NOWAIT keeps the poll timeout at 0. */
+   * non-blocking iteration; UV_RUN_NOWAIT keeps the poll timeout at 0.
+   *
+   * This is the nested-tick entry point (wait_for_promise from inside a poll
+   * callback), and the forced iteration always reaches the check phase, i.e.
+   * us_internal_loop_post. The tick_depth bracket is what makes that post
+   * defer the closed-socket sweep to the outermost tick; see us_loop_run. */
+  loop->data.tick_depth++;
   loop->uv_loop->active_handles++;
   uv_run(loop->uv_loop, UV_RUN_NOWAIT);
   loop->uv_loop->active_handles--;
+  loop->data.tick_depth--;
 }
 
 struct us_loop_t *us_create_loop(void *hint,
@@ -401,6 +419,11 @@ void us_loop_run(struct us_loop_t *loop) {
   us_loop_integrate(loop);
   uv_update_time(loop->uv_loop);
 
+  /* us_internal_loop_post runs from the uv_check_t registered above, so it
+   * fires inside uv_run; the tick_depth bracket mirrors us_loop_run and
+   * us_loop_run_bun_tick in epoll_kqueue.c. See us_internal_loop_post. */
+  loop->data.tick_depth++;
+
   /* UV_RUN_ONCE may block in the poll phase (pending callbacks dispatch
    * first), making this the JS thread's park hook, the counterpart of
    * us_loop_run_bun_tick's. jsc_vm is only set on the JS thread's loop. */
@@ -411,6 +434,7 @@ void us_loop_run(struct us_loop_t *loop) {
   }
 
   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,

packages/bun-usockets/src/internal/loop_data.h

@@ -95,10 +95,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-run entry points (us_loop_run_bun_tick and
+     * us_loop_run on epoll/kqueue; us_loop_run and us_loop_pump on libuv).
+     * 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;
 };
 

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

scripts/build/fetch-cli.ts

@@ -242,13 +242,18 @@ function normalizeLf(s: string): string {
  * so a CRLF-mangled checkout still applies cleanly. --no-index: dest/ is
  * not a git repo. --ignore-whitespace / --ignore-space-change: patches are
  * authored against upstream which may have different trailing whitespace.
+ *
+ * GIT_CEILING_DIRECTORIES hides the enclosing repo: run from a repo subdir,
+ * `git apply` treats a git-format (`diff --git`) patch as toplevel-relative and
+ * silently skips it with exit 0. -v + LC_ALL=C make that skip detectable below.
  */
 function applyPatch(dest: string, patchPath: string, patchBody: string): void {
-  const result = spawnSync("git", ["apply", "--ignore-whitespace", "--ignore-space-change", "--no-index", "-"], {
+  const result = spawnSync("git", ["apply", "--ignore-whitespace", "--ignore-space-change", "--no-index", "-v", "-"], {
     cwd: dest,
     input: normalizeLf(patchBody),
     stdio: ["pipe", "ignore", "pipe"],
     encoding: "utf8",
+    env: { ...process.env, GIT_CEILING_DIRECTORIES: join(dest, ".."), LC_ALL: "C" },
   });
 
   if (result.error) {
@@ -264,6 +269,16 @@ function applyPatch(dest: string, patchPath: string, patchBody: string): void {
       hint: "The patch may be out of date with the pinned commit",
     });
   }
+
+  // A file git considers outside the current directory is skipped with exit 0.
+  // Nothing under patches/ should ever be skipped: a skip here means the .ref
+  // stamp would certify a tree the patch never touched.
+  if (result.stderr !== null && result.stderr.includes("Skipped patch")) {
+    throw new BuildError(`git apply skipped one or more files: ${result.stderr.trim()}`, {
+      file: patchPath,
+      hint: "GIT_CEILING_DIRECTORIES should prevent this. A skip means git resolved the patch's paths outside the dependency's source directory.",
+    });
+  }
 }
 
 // Only run if this file is the entry point (not imported as a module).

test/js/bun/net/socket.test.ts

@@ -3827,3 +3827,59 @@ describe("allowHalfOpen socket whose peer resets behind pending writes", () => {
     expect(endCount).toBe(1);
   });
 });
+
+// On the libuv (Windows) event loop backend, a synchronous re-entrant loop
+// tick from inside a socket's data callback ran the closed-socket sweep and
+// freed the us_socket_t the suspended outer dispatch frame still held.
+it("survives closing a socket and re-entering the event loop from its own data callback", async () => {
+  await using proc = Bun.spawn({
+    cmd: [
+      bunExe(),
+      "-e",
+      `
+        // The setImmediate callback only runs inside the nested autoTick that
+        // transform()'s waitForPromise performs, so reentries === 20 proves
+        // every round really re-entered the loop and didn't just terminate().
+        let reentries = 0;
+        const server = Bun.listen({
+          hostname: "127.0.0.1",
+          port: 0,
+          socket: {
+            data(sock) {
+              // Synchronously close the socket, then synchronously re-enter the
+              // event loop: the element handler returns a still-pending promise,
+              // so .transform() spins waitForPromise -> autoTick -> a nested tick.
+              sock.terminate();
+              new HTMLRewriter()
+                .on("p", { element: () => new Promise(r => setImmediate(() => { reentries++; r(); })) })
+                .transform("<p></p>");
+            },
+            close() {},
+            error() {},
+          },
+        });
+        for (let i = 0; i < 20; i++) {
+          const { promise, resolve } = Promise.withResolvers();
+          Bun.connect({
+            hostname: "127.0.0.1",
+            port: server.port,
+            socket: { open: s => s.write("x"), data() {}, close: resolve, end: resolve, error: resolve, connectError: resolve },
+          }).catch(resolve);
+          await promise;
+        }
+        server.stop(true);
+        console.log("SURVIVED " + reentries);
+      `,
+    ],
+    env: bunEnv,
+    stdout: "pipe",
+    stderr: "pipe",
+  });
+
+  const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
+  expect({ stdout: stdout.trim(), stderr, exitCode }).toEqual({
+    stdout: "SURVIVED 20",
+    stderr: expect.any(String),
+    exitCode: 0,
+  });
+});