Fix Windows heap corruption when a socket's data callback closes it and re-enters the event loop - #33018
Fix Windows heap corruption when a socket's data callback closes it and re-enters the event loop#33018robobun wants to merge 3 commits into
Conversation
|
Updated 6:05 PM PT - Aug 12th, 2026
❌ @robobun, your commit c46dde9 has 3 failures in
🧪 To try this PR locally: bunx bun-pr 33018That installs a local version of the PR into your bun-33018 --bun |
|
Found 3 issues this PR may fix:
🤖 Generated with Claude Code |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdjusts libuv poll teardown and nested loop handling, adds a Windows libuv poll patch and wires it into dependency setup, hardens dependency patch application, and adds a subprocess regression test for socket close during event-loop re-entry. Changeslibuv poll lifecycle and re-entrancy fix
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/bun-usockets/src/eventing/libuv.c`:
- Around line 219-226: In us_create_poll, the newly allocated p and p->uv_p are
used without checking for allocation failure, so add OOM handling before
dereferencing either pointer. Use the existing native OOM path in this
function’s flow to validate the malloc/calloc results and return or abort
consistently before assigning p->uv_p->data, keeping the fix localized to
us_create_poll and its allocation setup.
In `@test/js/bun/net/socket.test.ts`:
- Around line 1655-1657: The nested HTMLRewriter test only checks that it
doesn’t crash, so it doesn’t prove the re-entry path actually ran. Update the
HTMLRewriter::transform() repro in socket.test.ts to make the nested tick
observable by flipping a reentered flag inside the setImmediate callback and
asserting it immediately after transform() returns, and remove the setTimeout(r,
5) fallback so the test stays deterministic. Keep the focus on the nested
HTMLRewriter re-entry case rather than relying on sock.terminate() alone.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c800994f-8229-44c2-8464-7e07a1f458bc
📒 Files selected for processing (3)
packages/bun-usockets/src/eventing/libuv.cpackages/bun-usockets/src/internal/loop_data.htest/js/bun/net/socket.test.ts
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Good flag. #30028 (@Jarred-Sumner) did not surface in my pre-open duplicate search (I had scoped it to robobun-authored PRs). I've now read it and tested the relationship both ways on a Windows debug build. Summary: #30028 is one of this PR's two changes, this PR is a strict superset, and neither makes the other redundant. The overlap. Both PRs move Not a duplicate: #30028 alone does not fix the heap corruption. Verified, not just traced. I applied only #30028's Mechanism: the nested tick's The converse holds. #30028's regression test, run verbatim against this branch: so this PR covers #30028's symptom too. Remaining delta over #30028's @Jarred-Sumner this is your call on how the two should land:
I did not want to step on an open PR without flagging the overlap. |
|
Evaluated all three. I'm not adding any #22444 (crash in #32585 (Windows segfault in a long-running HTTP proxy, fault address #27692 (Windows BSOD, If a maintainer wants #22444 linked on the strength of the signature match, no objection from me, but I would rather not auto-close an issue I cannot prove. |
|
Converting to draft: CI on this branch has a Windows regression I introduced, and the verification section in the description is wrong until it's fixed. Six node-compat tests time out on all three Windows lanes ( None of them is in I'm reproducing @Jarred-Sumner heads up since I pinged you above: please hold off on the #30028 / #33018 question until this is resolved. |
84b2342 to
eb014e8
Compare
|
Un-drafting. CI was right and this PR's first revision was wrong. The correction changes the diff substantially and also explains #30028's two-month-old CI failures, so here is the complete story. What CI caught. Six node http tests ( Root cause of the hang. The first revision moved The fix now. A correction to my earlier comments on this PR. I wrote that this PR is a strict superset of #30028 and that #30028's regression test passes on this branch. Both were true of the first revision and are no longer true: the (The earlier review note about One build-system finding, for whoever owns |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/build/deps/libuv.ts`:
- Around line 51-64: The patch application flow in applyPatch is allowing a
skipped git apply to look successful, which can leave vendor/libuv unpatched
while still writing the fetch stamp. Update applyPatch in
scripts/build/fetch-cli.ts to detect when git apply skips or applies nothing and
treat that as a hard failure before the .ref file is written. Use the
applyPatch/fetch stamping path to locate the fix and ensure malformed patch
formats are rejected instead of being marked done.
In `@test/js/bun/net/socket.test.ts`:
- Around line 1685-1687: The subprocess regression assertion in socket.test.ts
is dropping native crash diagnostics by discarding stderr; update the combined
expectation around the Promise.all result so stderr remains part of the asserted
object while staying unconstrained (for example using an any-string matcher),
and remove the separate stderr discard so failures still surface diagnostics
from proc.stderr.text().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5c5e6eab-0499-46fe-a4b8-99a5021d2b5f
📒 Files selected for processing (5)
packages/bun-usockets/src/eventing/libuv.cpackages/bun-usockets/src/internal/loop_data.hpatches/libuv/win-poll-no-reendgame-after-close.patchscripts/build/deps/libuv.tstest/js/bun/net/socket.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/build/fetch-cli.ts`:
- Around line 245-254: Condense the explanatory block comment in fetch-cli.ts to
fit the repository’s 3-line limit while preserving the core rationale. Keep only
the essential points about GIT_CEILING_DIRECTORIES preventing skipped git-format
patches from being treated as successfully applied and the fail-loud backstop
using the skipped-patch check. Use the existing comment block near the
patch-application logic and shorten the wording without changing behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 90936f46-7f94-4661-84c0-3cfa451f43cb
📒 Files selected for processing (3)
scripts/build/deps/libuv.tsscripts/build/fetch-cli.tstest/js/bun/net/socket.test.ts
💤 Files with no reviewable changes (1)
- scripts/build/deps/libuv.ts
|
CI status, for whoever reviews this: the red check is infrastructure, not the diff. Across the last two builds on this branch (66519 and 66525), the only hard-failed job each time is It never ran a single test either time, and both failures were on the same agent ( For the lanes this PR can actually affect, both builds are clean: no failures in, and no annotation mentions of, |
|
One correction to my comment above. I wrote that no annotation mentions It is in the flaky (retried and passed) bucket, not a failure, and it is not my test. Two reasons it cannot be this PR:
The hard-failure picture is unchanged from my comment above: 280 jobs passed, and the only 2 failures are both |
|
The same failure is back on build 66564 (the push for the review nit above), so here is the consolidated CI picture across this branch's last three builds, for whoever can unblock it. Every lane that can exercise this change is green. On 66525, all 24 Windows The only failure is one Buildkite host. Across 66519, 66525, and now 66564, every hard-failed job is a That host cannot reach the artifact store. Re-pushing just re-dispatches onto it (66564 hit it again within minutes of my last commit), so I am going to stop pushing. My Buildkite token is read-scoped (the single-job retry endpoint returns 403), so I cannot retry it from here either. The red What this needs: someone with Buildkite access to retry this job, or to read the darwin failure as the infrastructure noise it is. The Windows fail-before and pass-after for the actual change are in the description. |
|
Build 66564 finished: 284 passed, 2 failed. Both failures are noise, but one of them disproves a claim in my previous comment, so I am correcting it here and explaining both. I was wrong that a re-push can only land on the sick agent. There are two The other failure is They cannot be caused by this diff, and that is a preprocessor fact rather than a judgment call. #if defined(_WIN32)
#define LIBUS_USE_LIBUV
#elif defined(__APPLE__) || defined(__FreeBSD__)
#define LIBUS_USE_KQUEUE
Since a re-push is not futile after all, I have pushed a single empty |
|
The retrigger (build 66646) went straight back to the sick agent. That is structural, not a run of bad luck. As long as that agent is online in the What this needs, in order of preference:
|
|
Build 66646 settled with a second failure beyond the darwin agent, and this one is on a Windows lane, so it deserves real scrutiny rather than a hand-wave: this is a Windows PR. The job is
So 66646's two failures are one persistently sick darwin build agent and one known flaky Windows install test that the retry happened not to save on one of three lanes. My one retrigger is spent and I am not pushing again. Everything in my previous comment stands: retry the darwin job, or merge on the evidence. |
fb8e250 to
c5c4804
Compare
|
Rebased onto The conflict was a pure append-vs-append in Since #31859 rewrote the Rust socket dispatch layer, I re-checked that the test's mechanism is intact: Full-file run on the rebased branch: |
|
Post-rebase CI picture (build 70743, finished: 285 passed, 1 failed; the two The darwin agent blocker is gone. Both The one failure is the current repo-wide Windows flake, not this PR. This PR's own surface is clean. 23 of 24 Windows I have already pushed this conversation's one retrigger (on the pre-rebase builds), so I will not push again for the napi flake. Anyone with Buildkite access can retry the one job; I cannot (read-scoped token). |
c5c4804 to
9d58b55
Compare
|
Rebased again for the same append-vs-append conflict in |
|
Build 71517 finished: 268 passed, 18 failed. All 18 are accounted for by three tests that are broken on main and present on other branches; none of the three is related to this PR, and none of them mentions
16 + 1 + 1 = the 18 failed shards. The other-branch hits are a TLS ALPN change, an mTLS close-notify fix, a blob deserialize fix, an ASan overflow sweep, a worker-threads ELU change, an RSA JWK validation fix, and so on: none related to each other, to uSockets, or to this PR. I am not going to fold an HTTP-client redirect fix or a darwin TLS-proxy stress-test fix into a uSockets |
9d58b55 to
2662a98
Compare
|
Rebased onto main for a conflict in This rebase also picks up #33916, the fix for the |
There was a problem hiding this comment.
No issues found, but this touches memory-safety-critical event-loop lifecycle code (uSockets libuv backend + a vendored libuv patch), so I'm deferring to a human reviewer rather than auto-approving.
What was reviewed:
- The
tick_depthbracketing inus_loop_run/us_loop_pumpmirrorsepoll_kqueue.cand sits correctly around the newBun__JSC_onBeforeWaithook from #34009. - The
close_cb_free_poll↔us_poll_freeself-marker handshake (h->data = h) — checked that the sentinel cannot collide with the normal-path values (0 fromus_poll_stop, orpfromus_poll_free's arm), and that both allocations are freed exactly once on each ordering. - The libuv
win/poll.cpatch guard matches whatuv__poll_endgamealready asserts, and covers both fast/slow siblings. applyPatchhardening:GIT_CEILING_DIRECTORIES+-vskip detection is additive; existing traditional-format patches are unaffected.
Extended reasoning...
Overview
This PR fixes a Windows-only heap corruption (UAF + double-free) that occurs when a socket's data callback synchronously closes the socket and then re-enters the event loop. It touches six files: the uSockets libuv eventing backend (libuv.c), a comment in loop_data.h, a new libuv patch file (win/poll.c), the libuv dep config, a hardening of fetch-cli.ts::applyPatch, and a new subprocess regression test.
The core fix has three cooperating pieces: (1) tick_depth bracketing around uv_run so the existing us_internal_loop_post guard actually observes nesting on Windows, (2) a three-state rendezvous between close_cb_free_poll and us_poll_free for the case where the deferred sweep means libuv's close callback fires before the sweep runs, and (3) a one-line guard in libuv's uv__fast_poll_process_poll_req tail so a handle whose endgame already ran inside a nested uv_run isn't re-queued.
Security risks
None identified. This is internal event-loop lifecycle management with no user-controlled input reaching the changed paths. The applyPatch change is build-time only and strictly tightens behavior (fails on skip instead of silently succeeding).
Level of scrutiny
High. This is native memory-lifecycle code in the Windows event loop backend, where a mistake manifests as heap corruption or hangs on every socket close. The first revision of this PR was itself wrong (moved uv_close from us_poll_stop to us_poll_free) and hung six Node http tests on all Windows CI lanes; the author caught it, root-caused it, and reverted to a minimal approach — but that history underscores how subtle this area is. The fix spans three layers (uSockets, libuv internals via patch, and Bun's re-entrant loop semantics) whose ordering invariants are not obvious from any single file.
The h->data = h self-marker is a novel sentinel handshake across libuv's async close machinery. I traced the three possible uv_p->data states at each callback ordering and believe it's correct, but this is exactly the kind of invariant a maintainer familiar with uSockets' Windows poll lifecycle (cirospaciari or Jarred-Sumner, per suggested reviewers) should confirm.
Other factors
- The PR description is exceptionally thorough: fail-before/pass-after on Windows, a 229-test no-regression sweep, and an explicit interleaving trace against the vendored libuv source.
- The bug-hunting system found no issues; all CodeRabbit comments are resolved; my earlier allocator-idiom nit is moot (that code was reverted).
- There is an acknowledged relationship with open PR #30028 (same file, different bug, and #30028's approach shares the hang mechanism this PR's first revision hit) — the author flagged this for Jarred-Sumner but there's been no maintainer response yet. That coordination is another reason a human should look.
- The vendored libuv patch is explicitly noted as not-upstreamable (nested
uv_runis outside libuv's contract), which is a maintenance-burden decision worth a maintainer's sign-off.
Given the criticality of the code path, the vendored-dependency patch, and the open coordination question with #30028, this should not be auto-approved.
|
Build 72796 finished: 283 passed, 3 failed; all four
The |
|
Independent confirmation of this diagnosis from a different symptom, plus a rebase onto current main.
Repro numbers on a Windows Server 2019 x64 box, running that bake test's child fixture in a loop:
This PR no longer applies to main ( Overlap note: #37105 (opened yesterday) contains the |
|
A third independent sighting of the same bug, with a trigger shape that differs from the two already described here, for whoever reviews this. Symptom. Trigger shape. Unlike the Read against |
|
@robobun rebase conflicts |
us_internal_loop_post defers us_internal_free_closed_sockets to the outermost loop tick via loop->data.tick_depth, because a nested tick (a poll callback re-entering the loop, e.g. waitForPromise) must not free a socket the suspended outer dispatch frame still reads after its callback returns. That counter was only maintained by the epoll/kqueue backend; on libuv (Windows) it stayed 0 forever, so every nested uv_run ran the sweep. The result is a deterministic heap corruption (STATUS_HEAP_CORRUPTION, 0xC0000374 on bun 1.3.14) when a socket's data callback closes it and then synchronously re-enters the event loop. Three pieces, all of which are no-ops for a non-nested close: - eventing/libuv.c: us_loop_run and us_loop_pump bracket uv_run with tick_depth++ and --, the direct mirror of us_loop_run and us_loop_run_bun_tick in epoll_kqueue.c. us_internal_loop_post runs from the uv_check_t registered in us_create_loop, so it fires inside uv_run and now sees the nesting depth. - eventing/libuv.c: deferring the sweep means the nested tick's uv__process_endgames can now run close_cb_free_poll before us_poll_free has re-pointed uv_p->data at the us_poll_t. That branch used to silently do nothing, which would leak both allocations: the later us_poll_free sees uv_is_closing() still true (uv_is_closing reports CLOSED as well as CLOSING) and arms a callback that already fired. close_cb_free_poll now marks the handle (h->data = h) and us_poll_free frees both when it sees the mark. - patches/libuv/win-poll-no-reendgame-after-close.patch: the post-poll_cb tail of uv__fast_poll_process_poll_req (and its slow-poll sibling) re-queues an endgame for any CLOSING handle with no outstanding AFD requests. When a nested uv_run already ran that handle's endgame, and so cleared UV_HANDLE_ENDGAME_QUEUED and set UV_HANDLE_CLOSED, the re-queue runs uv__poll_endgame a second time, which it asserts against, and invokes close_cb_free_poll twice. Guard the check on !(flags & UV_HANDLE_CLOSED). us_poll_stop, us_create_poll, and the common path of us_poll_free are unchanged from main. In particular, us_poll_stop keeps issuing the uv_close: that uv_close is what keeps uv__loop_alive() true for a closed socket (uv__handle_closing calls uv__active_handle_add even for an unref'd handle), which is what makes uv_run execute the next iteration, which is the only thing that runs the sweep. An earlier revision of this branch moved the uv_close into us_poll_free; that cycle hung six node http tests on every Windows CI lane, because a socket closed from the timer phase (after the check phase) dropped active_handles to zero and the very next uv_run returned without running a single phase. PR 30028 makes the same move for a different reason and its CI shows the same failures. Repro: a Bun.listen data handler that calls sock.terminate() and then synchronously re-enters the loop (an HTMLRewriter element handler that returns a pending promise) crashes bun 1.3.14 on Windows with STATUS_HEAP_CORRUPTION on the first hit. Either half alone does not crash. Not reproducible on Linux or macOS, where the epoll/kqueue backend already maintains tick_depth.
git apply, run from a subdirectory of a repository, only prefixes a patch's paths with that subdirectory for traditional unified diffs. A patch with a `diff --git a/X b/X` header is treated as toplevel-relative instead, falls outside the subdirectory prefix, and is silently "Skipped" with exit 0. applyPatch only checked the exit status, so the fetch went on to write the .ref stamp over a tree the patch never touched. vendor/<dep>/ sits inside this repository, so every dep fetch runs git apply from exactly that kind of subdirectory. All 18 existing files in patches/ happen to be traditional format, which is the only reason nothing has hit this before. Two-part fix in applyPatch: - GIT_CEILING_DIRECTORIES set to dest's parent stops git from discovering the enclosing repository, so the prefix logic never engages and the patch's paths resolve against dest regardless of format. - The invocation adds -v and LC_ALL=C and fails hard on "Skipped patch" in stderr, so a skipped file can never stamp the source as patched. Also from review: the regression test's spawned child now includes stderr in the combined assertion (as expect.any) so native crash diagnostics appear in the failure diff instead of being discarded, and the libuv dep's now-redundant patch-format note is removed because the applyPatch doc is the right home for it.
2662a98 to
c46dde9
Compare
|
Rebased onto current main (
Rebuilt on the rebased tree: the test passes, and the full file goes from |
|
Build 93542 (the rebase push) is red because of a CI-wide outage, not the diff: three |
alii
left a comment
There was a problem hiding this comment.
The tick_depth bracket, the closed sentinel and the libuv patch hold up against current main, and the rebase you pushed this afternoon took care of the us_free and active_handles conflicts. Two things left, plus one note.
- us_loop_pump: nothing exercises the pump bracket; add an unref'd variant of the new test.
- The fetch-cli.ts hardening is unrelated to this fix and the body still says it was left for a separate PR; split it out or update the body.
- The c-ares uv_poll_t in src/runtime/dns_jsc/dns.rs has the same shape (on_dns_socket_state uv_closes it from inside its own poll_cb, on_close_uv frees it, and the microtask drain at the end of on_dns_poll_uv can re-enter the loop). A follow-up is fine, but say so in the body.
Windows CI on the rebase died in the github.com outage, so it still needs a green Windows lane.
| * 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++; |
There was a problem hiding this comment.
Nothing covers this path. The new test keeps the listener ref'd, and uv_close bumps active_handles for the closing handle, so both the outer and the nested tick in the test go through us_loop_run and this bracket can be deleted without the test noticing. Since #34478 the pump does run the check phase, so a close from an unref'd socket's callback followed by a nested tick hits the same sweep. Add a variant of the socket.test.ts case with server.unref() and the client unref'd.
| input: normalizeLf(patchBody), | ||
| stdio: ["pipe", "ignore", "pipe"], | ||
| encoding: "utf8", | ||
| env: { ...process.env, GIT_CEILING_DIRECTORIES: join(dest, ".."), LC_ALL: "C" }, |
There was a problem hiding this comment.
This is unrelated to the fix, and the body still says applyPatch hardening is for a separate PR. It changes the fetch step for every patched dep on every platform, no patch in the tree is git-format so nothing needs it today, and GIT_CEILING_DIRECTORIES also hides the repo's .gitattributes eol=lf from git apply. It applies to main on its own; split it out, or if you want it here update the body and check the eol point.
…e 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).
|
Two more CI sightings of this bug in Sightings. Both on 2026-08-13, on unrelated branches that had just merged main:
Reproduction. On a Windows Server 2019 box where With the Confirmation that this is the whole story for that test. I tried a variant of the fix that avoids the libuv patch: keep a per-poll dispatch depth in Not opening a separate PR; this one covers it. |
|
One more trigger shape for this bug, with a different crash site than the three already described here. Posting it because it is the smallest repro so far and because the nested tick enters through Symptom. Trigger. A test body resumes inside TLS socket A's Numbers (Windows Server 2019 x64, canary
I have not built this branch on Windows; the nested tick here goes through Repro (bun test file; keys from test/js/node/test/fixtures/keys)import { expect, test } from "bun:test";
import { once } from "node:events";
import { readFileSync } from "node:fs";
import https from "node:https";
import tls from "node:tls";
const keys = "test/js/node/test/fixtures/keys/";
async function peerCN(port: number, extra = {}) {
const socket = tls.connect({ host: "127.0.0.1", port, rejectUnauthorized: false, ...extra });
const errored = once(socket, "error");
await Promise.race([once(socket, "secureConnect"), errored.then(([e]) => Promise.reject(e))]);
const cn = socket.getPeerCertificate().subject?.CN;
socket.destroy(); // closes socket A from inside its own on_data
return cn;
}
test("rejects after a socket event", async () => {
const server = https.createServer({
key: readFileSync(keys + "agent1-key.pem", "utf8"),
cert: readFileSync(keys + "agent1-cert.pem", "utf8"),
minVersion: "TLSv1.3",
});
server.listen(0);
await once(server, "listening");
const port = (server.address() as any).port;
try {
expect(await peerCN(port)).toBe("agent1"); // resumes inside A's us_internal_ssl_on_data
await expect(peerCN(port, { maxVersion: "TLSv1.2" })).rejects.toThrow(); // nested us_loop_run ticks
} finally {
server.close();
}
}); |
|
One more data point for Verification of this PR's exact diff (the
That reduced fixture is on |
|
This also fixes the intermittent Windows failure of CI symptomThe child How the fixture gets there
An instrumented build confirmed the free: Verification of this branch (Windows x64, debug builds)
For what it is worth, I independently tried the variant that moves the |
|
Arrived at the same three changes independently from build 98537 (
|
|
This is also the cause of the top hard failure on the windows-x64 CI lane: How that fixture reaches this bug: the case before it awaits the WebSocket Independent reproduction on a windows-x64 box with a For reference, branch |
Symptom
On Windows,
bun 1.3.14crashes withSTATUS_HEAP_CORRUPTION(exit0xC0000374) on the first occurrence of this sequence, all synchronous, inside aBun.listendatacallback:Neither half alone crashes (400 rounds each). Together it crashes on round 1. Not reproducible on Linux or macOS. Any synchronous loop re-entry from inside a socket callback works as the second half (
expect().toThrow()inbun:testis another; it is the one theus_internal_loop_postcomment names).Cause
us_internal_loop_postdefersus_internal_free_closed_socketsto the outermost loop tick:because a poll callback may re-enter the loop, and the suspended outer dispatch frame still reads
s->flagsright afteron_datareturns (loop.c:615).tick_depthis only maintained by the epoll/kqueue backend:epoll_kqueue.c, the only writer, sits inside#if defined(LIBUS_USE_EPOLL) || defined(LIBUS_USE_KQUEUE). Windows compilesLIBUS_USE_LIBUV, wheretick_depthstays0forever, so the guard is always satisfied and every nesteduv_runruns the closed-socket sweep.The exact interleaving, verified against the vendored libuv (oven-sh/libuv@4dcfac47 plus bun's patches)
One
uv_runiteration on Windows is (win/core.c:719-755):uv__process_reqs(poll callbacks fire here), prepare, poll,uv__process_reqs, check (runsus_internal_loop_post), endgames (runs handle close callbacks), timers. The check phase runs before the endgame phase.uv__fast_poll_process_poll_req(A.uv_p, req)clearssubmitted_events_1, re-arms it (bun's rearm patch does this before the callback), then callspoll_cb(A)->us_internal_dispatch_ready_poll->on_data-> the JSdatahandler.us_socket_close->us_poll_stop->uv_poll_stop; A.uv_p->data = 0; uv_close(A.uv_p, close_cb_free_poll). Because the re-armed AFD request is still outstanding,uv__poll_closetakes the cancel path, not the immediateuv__want_endgame.closesocket(fd)posts anAFD_POLL_LOCAL_CLOSEcompletion to the IOCP. A is pushed ontoloop->data.closed_head.uv_run(UV_RUN_NOWAIT):uv__fast_poll_process_poll_req(A.uv_p, req)-> tail ->uv__want_endgame(A.uv_p)us_internal_loop_post->tick_depth(0) <= 1->us_internal_free_closed_sockets->us_poll_free(A):uv_is_closing(A.uv_p)is true, so it armsA.uv_p->data = Auv__poll_endgame(A.uv_p)->close_cb_free_poll(A.uv_p)->h->data != 0->free(A); free(A.uv_p)on_datareturns, and the outer dispatch readsA->flags.adoptedat loop.c:615. Use after free.It then gets worse. The outer
uv__fast_poll_process_poll_reqframe's tail reads the freedA.uv_p, and sinceUV_HANDLE_CLOSINGis still set in the stale bytes andUV_HANDLE_ENDGAME_QUEUEDwas cleared by the nesteduv__process_endgames, it re-queues the freed handle for a second endgame. The secondclose_cb_free_pollreadsh->dataout of freed memory (by then the allocator's encoded freelist pointer) and passes it tofree(), and double-freesA.uv_p. That is what the allocator reports as heap corruption.Fix
us_poll_stop,us_create_poll, and the common path ofus_poll_freeare unchanged frommain. Three additions, all of which are no-ops for a non-nested close:us_loop_runandus_loop_pumpbracketuv_runwithtick_depth++/--, the direct mirror ofus_loop_runandus_loop_run_bun_tickinepoll_kqueue.c.us_internal_loop_postfires from theuv_check_tregistered inus_create_loop, so it runs insideuv_runand now sees the nesting depth. These two functions are the only live entry points touv_runon the uSockets default loop; I audited everyuv_runcaller insrc/to confirm it.close_cb_free_pollmarks a handle it ran for beforeus_poll_freearmed it. With the sweep deferred, the nested tick'suv__process_endgamescan now runclose_cb_free_pollwhileuv_p->datais still 0. That branch used to silently do nothing, which would leak both allocations: the laterus_poll_freeseesuv_is_closing()still true (it reportsCLOSEDas well asCLOSING) and arms a callback that already fired. It now setsh->data = h, andus_poll_freefrees both when it sees the mark.patches/libuv/win-poll-no-reendgame-after-close.patch. The post-poll_cbtail ofuv__fast_poll_process_poll_req(and its slow-poll sibling) re-queues an endgame for anyCLOSINGhandle with no outstanding AFD requests. When a nesteduv_runalready ran that handle's endgame, and so clearedUV_HANDLE_ENDGAME_QUEUEDand setUV_HANDLE_CLOSED, the re-queue runsuv__poll_endgamea second time, which it asserts against, and invokesclose_cb_free_polltwice. The patch guards the check on!(flags & UV_HANDLE_CLOSED).Why
us_poll_stopmust keep itsuv_closeAn earlier revision of this branch moved the
uv_closefromus_poll_stoptous_poll_free, and CI caught it:test-http-client-set-timeout.jsplus five more node http tests hung on every Windows lane, none of them intest/expectations.txtand none present on main's CI.The mechanism, from an instrumented build:
us_poll_stop'suv_closeis load-bearing foruv__loop_alive().uv_close->uv__handle_closing->uv__active_handle_addraisesloop->active_handleseven for an unref'd handle, and all of bun's socket polls are unref'd (us_poll_startcallsuv_unref). That is what makesuv_runexecute the next iteration, which is the only thing that runs theuv_check_t->us_internal_loop_post-> closed-socket sweep. Moving theuv_closeinto the sweep creates a cycle: a socket closed from the timer phase (which runs after the check phase) dropsactive_handlesto zero, the very nextuv_run(UV_RUN_ONCE)returns atwhile (r != 0)without running a single phase, the sweep never runs again, other still-open sockets' IOCP completions are never dequeued, and the process either spins at 100% CPU (bun's own liveness still says alive) or leaks the sockets.test-http-client-set-timeout.jsreproduces it deterministically: anAgent({keepAlive: true, timeout: 50})whosetimeoutcallback destroys the request, which is exactly a close from the timer phase.#30028 makes the same move, for a different reason, and its Windows CI (build 49641) has the same
test-http-client-*failures. I originally thought the two PRs overlapped; they do not. #30028's symptom is real but needs a different fix; the full analysis is on that PR.Verification (Windows x64)
Fail-before:
Pass-after on this branch:
The spawned script counts the
setImmediatecallbacks, which only run inside the nestedautoTickthattransform()'swaitForPromiseperforms, and the test asserts that count is exactly 20, so the re-entrant tick is observed rather than inferred.No-regression sweep on the same Windows debug build:
Both libuv patches were confirmed present in the built tree (
poll.chas the baselineUV_HANDLE_CLOSEDassert plus the two new guards).Note for reviewers
eventing/libuv.cis entirely inside#ifdef LIBUS_USE_LIBUV, which only Windows defines, so on Linux and macOS this diff produces a byte-identical binary and the new test passes both with and without it (the epoll/kqueue backend already maintainstick_depth). The Windows runs above are the before/after proof.diff --githeader) on purpose, matching every other file inpatches/.git applyruns fromvendor/<dep>/, a subdirectory of this repository, and only prefixes a patch's paths with that subdirectory for traditional patches; a git-format patch is treated as toplevel-relative, falls outside the prefix, and is silently skipped with exit 0, which the fetch step cannot detect. There's a note at thepatches:array indeps/libuv.ts;fetch-cli.ts::applyPatchis worth hardening separately.Related: #32233 fixes the sibling re-entrancy hazard on the epoll/kqueue backend, where a nested tick clobbers the outer tick's ready-poll batch. Same bug class, opposite backend.
no test proof · iteration 20 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/net/socket.test.ts