Skip to content

usockets(win): free the poll when us_poll_free runs after the uv close callback - #37105

Open
robobun wants to merge 4 commits into
mainfrom
farm/aff9046f/fix-uv-poll-closed-free
Open

robobun wants to merge 4 commits into
mainfrom
farm/aff9046f/fix-uv-poll-closed-free

Conversation

@robobun

@robobun robobun commented Aug 7, 2026 •

Copy link
Copy Markdown
Collaborator

Symptom

On Windows (libuv backend), us_poll_free leaks both the us_socket_t block and the uv_poll_t block whenever it runs after libuv has already finished closing the poll handle. Each leaked pair is 568 bytes (24 us_poll_t + 128 ext + 416 uv_poll_t); a server churning sockets through this path grows RSS without bound (measured ~6.7MB per 3000 sockets on release builds).

Cause

The stop/free handshake in packages/bun-usockets/src/eventing/libuv.c:

  • us_poll_stop nulls uv_p->data and calls uv_close(handle, close_cb_free_poll).
  • close_cb_free_poll frees both blocks only when data != 0.
  • us_poll_free checks uv_is_closing(): if true, it re-points data at the poll so the close callback frees it later; otherwise it frees both blocks directly.

uv_is_closing() returns true for a handle that is CLOSING (close requested, callback pending) and for one that is CLOSED (callback already ran). In the CLOSED case the callback ran with data == 0, freed nothing, and will never run again, so us_poll_free's deferral branch drops both allocations on the floor. The epoll/kqueue backend's us_poll_free always frees; callers (the closed-socket sweep in loop.c) rely on that contract.

This is reachable from plain JS. terminate() dispatches the JS close handler synchronously from us_internal_socket_close_raw, before the socket is linked to the sweep list. A synchronous event-loop re-entry inside that handler (anything that blocks on waitForPromise, e.g. expect().resolves in bun:test) lets libuv process the poll's cancellation completion and run its endgames phase: the handle reaches CLOSED while the socket is still waiting for the sweep. The sweep then calls us_poll_free on a CLOSED handle and leaks. Native addons driving uv callbacks can hit the same window.

Fix

close_cb_free_poll now marks the handle (h->data = h) when it runs with data == 0, recording that it ran and freed nothing. When us_poll_free sees the mark on a closing handle it frees both blocks itself instead of deferring to a callback that will never fire. The mark cannot collide with a live poll: data is otherwise 0 or the us_poll_t, which is a different live allocation than the handle, and handle reuse (us_poll_start_rc) memsets the uv_poll_t first.

For the test, the libuv backend now keeps an atomic count of live us_poll_t allocations, exposed as uvPollLiveCount via bun:internal-for-testing (same shape as the existing sslCtxLiveCount); it returns -1 on platforms without that backend. An RSS-based assertion was tried first and dropped: the probe's churn noise varies by machine (1.7MB locally, 2.9MB on the CI runners), while the count is exact.

Related: #33018 (nested-tick heap corruption) needs this same handshake repair once the closed-socket sweep is deferred on Windows; fixing the leak separately keeps that PR to its own bug.

Verification

The test chains 8 terminate() calls through close handlers (none of the chained sockets reaches the sweep list until its handler returns) with one loop re-entry at the bottom, so every chained socket's handle is CLOSED by the time the sweep frees it, 10 rounds. uvPollLiveCount must return to its pre-churn baseline. Measured on debug builds of this branch on both Windows architectures:

  • fix present: leaked: 0, test passes (windows-x64 and windows-aarch64)
  • fix disabled (same build with the rescue branch in us_poll_free neutered): leaked: 80, exactly rounds x depth, test fails (windows-x64 and windows-aarch64) - so the rescue branch is load-bearing and the probe drives every chained handle to CLOSED on both arches, not just the machine the fix was written on
  • release binary without the fix (system Bun 1.4.0-canary.1): the counter export does not exist yet, so the probe fails at import; an RSS variant of the same probe run on that binary leaks 568 bytes per pair (6.7MB over 3000 pairs), which is the user-visible form of the bug

With counters temporarily added to every branch of the handshake (an instrumented 1500-pair RSS run during development), the close callback ran with data == 0 once per chained terminate (cb_sentinel) and the rescue branch freed those handles 1:1 (rescue), with no change to the normal paths:

[pollfree] cb_defer=1500 cb_sentinel=1501 direct=0 closing=1501 rescue=1500

The off-by-one on cb_sentinel/closing is the process-exit boundary: the listen socket (and one client) closed during teardown after the final sweep, a pre-existing exit path this PR does not change.

The test is Windows-only (it.skipIf(!isWindows)); the buggy code is compiled only under LIBUS_USE_LIBUV, so there is no Linux/macOS path to exercise, and the fail-before half of the proof can only run on a Windows build. On Linux uvPollLiveCount() returns -1 (verified on a debug build).

bun bd test test/js/bun/net/socket.test.ts on Windows: 74 pass, 0 fail.


no test proof · iteration 0 · 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

robobun added 2 commits August 7, 2026 05:04
…ready ran

On the libuv backend, us_poll_free defers freeing to close_cb_free_poll
while uv_is_closing() reports the handle as closing. uv_is_closing() is
also true once the handle has finished closing, when the callback already
ran (freeing nothing, since us_poll_stop nulled handle->data) and will
never run again: us_poll_free re-pointed handle->data and returned,
leaking both the us_socket_t and the uv_poll_t allocations.

close_cb_free_poll now marks the handle when it runs with data == 0, and
us_poll_free frees both blocks itself when it sees the mark.
@github-actions github-actions Bot added the claude label Aug 7, 2026
@coderabbitai

coderabbitai Bot commented Aug 7, 2026 •

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 24 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 3d06bb1d-1971-484c-b3ea-aa153452b366

📥 Commits

Reviewing files that changed from the base of the PR and between 45eda51 and 244c398.

📒 Files selected for processing (7)
  • packages/bun-usockets/src/eventing/libuv.c
  • packages/bun-usockets/src/internal/eventing/libuv.h
  • src/js/internal-for-testing.ts
  • src/runtime/socket/mod.rs
  • src/runtime/socket/socket_body.rs
  • src/uws_sys/SocketContext.rs
  • test/js/bun/net/socket.test.ts

Comment @coderabbitai help to get the list of available commands.

@robobun

robobun commented Aug 7, 2026

Copy link
Copy Markdown
Collaborator Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Aug 7, 2026 •

Copy link
Copy Markdown
Contributor
⚠️ Action not completed

Review rate limited.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@github-actions

github-actions Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

This PR may be a duplicate of:

  1. ai slop #33018 - Contains the identical libuv.c fix (the h->data = h sentinel in close_cb_free_poll plus the p->uv_p->data == p->uv_p rescue branch in us_poll_free) as part of its three-part Windows nested-tick fix.

🤖 Generated with Claude Code

@robobun

robobun commented Aug 7, 2026

Copy link
Copy Markdown
Collaborator Author

Related but not a duplicate. #33018 fixes a different bug: STATUS_HEAP_CORRUPTION when a socket's data callback closes it and re-enters the loop. Its fix defers the closed-socket sweep on Windows (tick_depth), and that deferral is what forces the same stop/free handshake repair to appear in its diff as a supporting piece.

This PR fixes the handshake leak that is reachable on main today without any of #33018's changes (close-handler re-entry, repro and measurements in the description), and carries a test for it. #33018 is currently conflicting with main; once this lands it can rebase down to its own bug (the sweep deferral plus its libuv patch).

Comment thread test/js/bun/net/socket.test.ts Outdated
RSS noise on the CI Windows runners (2.9MB over the probe's churn) exceeds
the old absolute threshold. Count live us_poll_t allocations in the libuv
backend instead, exposed as uvPollLiveCount via bun:internal-for-testing
(same shape as sslCtxLiveCount): the probe's count must return to its
pre-churn baseline, which is exact regardless of allocator behavior.
Comment thread src/runtime/socket/socket_body.rs Outdated
Comment thread src/uws_sys/SocketContext.rs Outdated
@robobun

robobun commented Aug 7, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 12:44 AM PT - Aug 7th, 2026

✅ @robobun, your commit 244c398e2fde4dfa517c23a9f6a4d3a85d86753b passed in Build #89945! 🎉


🧪   To try this PR locally:

bunx bun-pr 37105

That installs a local version of the PR into your bun-37105 executable, so you can run:

bun-37105 --bun

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test rewrite to a live us_poll_t counter (commit 8f8ba7d) fully resolves my earlier RSS-threshold nit — expect(leaked).toBe(0) is strictly better than any RSS bound. Deferring to a human on the core libuv.c change: the sentinel handshake between close_cb_free_poll and us_poll_free is sound as far as I can trace, but it's Windows-only socket-lifecycle memory management that overlaps with #33018, and I'd like someone who owns this path to confirm the ordering assumptions.

What was reviewed:

  • Traced every increment/decrement site of uv_poll_live (create, resize, all four us_poll_free branches, the deferred-free callback path, both us_poll_start_rc failure paths) — the counter is balanced.
  • Checked the sentinel h->data = h cannot collide with a live us_poll_t pointer or reach the if (h->data) free branch on a second callback invocation (uv_close fires the callback exactly once; us_poll_start_rc memsets before reuse).
  • The two comment-cop flags look like false positives — both are short doc comments on test-only bindings, not workaround justifications.
Extended reasoning...

Overview

The PR fixes a Windows-only memory leak in packages/bun-usockets/src/eventing/libuv.c: when us_poll_free runs after libuv's close callback has already fired (handle is CLOSED, not just CLOSING), the old code deferred freeing to a callback that will never run again, leaking both the us_poll_t and uv_poll_t blocks. The fix has close_cb_free_poll mark the handle with a self-pointer sentinel when it runs without freeing, and us_poll_free recognizes the mark and frees directly.

Supporting changes: an atomic live-poll counter in libuv.c, an FFI export in libuv.h and SocketContext.rs, a Rust host function in socket_body.rs / mod.rs, a bun:internal-for-testing binding, and a Windows-only test in socket.test.ts that chains terminate() calls through close handlers with a nested loop tick and asserts the counter returns to baseline.

Security risks

None identified. The change is confined to internal allocation lifecycle in vendored usockets code; no user-controlled input reaches the new branches, and the new sentinel value is an internal pointer that never escapes.

Level of scrutiny

High. This is memory-ownership logic in native networking code — REVIEW.md flags this as "the most-blocked category." The sentinel handshake relies on ordering guarantees between uv_close, its callback, and uv_is_closing() semantics, plus the invariant that h->data is only ever 0, the us_poll_t*, or (now) the handle itself. I traced these and they hold, but this is exactly the kind of change where a maintainer who knows the Windows socket sweep (loop.c) and the interaction with #33018's deferred-sweep work should confirm.

Other factors

  • My earlier nit about the RSS threshold is moot: the test was rewritten (commit 8f8ba7d) to assert an exact live-allocation counter instead, which is both stricter and immune to allocator jitter.
  • The live-counter instrumentation follows the established pattern used by us_ssl_ctx_live_count, fetchH2Internals.liveCounts, etc. — production code carrying a testing counter exposed via bun:internal-for-testing. REVIEW.md permits this.
  • Counter balance verified across all paths including us_poll_resize (increments for the new block; the old block later decrements via the !p->uv_p fast path) and both us_poll_start_rc init-failure branches.
  • The comment-cop bot fired on socket_body.rs:4928 and SocketContext.rs:333; both are ordinary doc comments on test bindings, not workaround justifications, so they read as heuristic false positives.
  • CI build #89933 was still running at last check; the Windows lane result should be confirmed before merge since the test is skipIf(!isWindows) and the fix is under #ifdef LIBUS_USE_LIBUV.

Comment thread src/runtime/socket/socket_body.rs
Comment thread src/uws_sys/SocketContext.rs

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The RSS→exact-counter rewrite is a better answer to my earlier nit than what I asked for — expect(leaked).toBe(0) against a live-allocation counter has none of the threshold-tuning problem. The sentinel handshake in libuv.c reads correctly to me and the counter increments/decrements balance across every path I traced, but this is native memory management in the usockets libuv backend, so I'd like a human to sign off.

What was reviewed:

  • Traced every uv_poll_live increment/decrement (create, resize, all four free paths, us_poll_start_rc failure branches) — balanced.
  • Checked the h->data = h sentinel cannot collide with 0 or the us_poll_t pointer, and that us_poll_start_rc's memset clears it on handle reuse.
  • Confirmed the #[cfg(windows)] extern matches the LIBUS_USE_LIBUV-gated symbol; non-Windows returns -1 without linking it.
Extended reasoning...

Overview

Windows-only leak fix in packages/bun-usockets/src/eventing/libuv.c: when us_poll_free runs after libuv's close callback has already fired (handle is CLOSED, not merely CLOSING — uv_is_closing() cannot distinguish), the deferral branch used to leak both the us_poll_t and uv_poll_t blocks. The fix has close_cb_free_poll mark the handle with a self-referential sentinel (h->data = h) when it runs with data == 0, and us_poll_free checks for that sentinel to free directly instead of deferring to a callback that will never fire again. Supporting changes: an atomic live-poll counter with a testing binding (uvPollLiveCount), Rust FFI plumbing, and a Windows-only regression test asserting the counter returns exactly to baseline.

Changes since my last review

My earlier nit flagged the flat 2.6MB RSS threshold. The author replaced the RSS assertion entirely (commit 8f8ba7d) with a direct live-allocation counter and expect(leaked).toBe(0), which is strictly better — no build-config branching needed, and the description shows the unfixed build produces exactly leaked: 80 (rounds × depth). The comment-cop bot's verbose-doc flags were addressed in 244c398.

Security risks

None. This is internal allocator bookkeeping in the eventing layer; no user input parsing, no auth/crypto surface. The new testing binding is gated behind bun:internal-for-testing.

Level of scrutiny

High. This is the most-blocked category per REVIEW.md — native memory ownership in a forked third-party C library, on a platform-specific path (Windows/libuv only) that I cannot exercise from this environment. The sentinel trick is sound but non-obvious: it relies on h->data only ever being 0, the us_poll_t*, or (now) h itself, and on libuv's guarantee that the close callback fires exactly once. The PR description backs this with instrumented per-branch counters showing 1:1 rescue on 1500 pairs, and fail-before/pass-after runs on both Windows arches, which is strong evidence — but a maintainer familiar with the usockets/libuv handshake should confirm the state-machine reasoning.

Other factors

The counter's increment/decrement placement matches existing allocation/free sites 1:1; I traced create/resize/all four free branches/the us_poll_start_rc failure paths and found no imbalance introduced by this PR. The test follows harness conventions (tempDir, bunEnv, spawned subprocess with concurrent pipe drain, exit-code asserted last) and polls for the counter to settle rather than sleeping. The overlap with #33018 was explained by the author and makes sense — this PR carries the standalone leak fix with its own test so #33018 can rebase to just its own bug.

@robobun robobun mentioned this pull request Aug 9, 2026
@robobun

robobun commented Aug 9, 2026

Copy link
Copy Markdown
Collaborator Author

Heads-up on overlap: the close_cb_free_poll / us_poll_free marker handshake here is one of the three changes in #33018 (June), which also adds the tick_depth bracketing for the libuv backend and a vendored-libuv guard against re-queueing a CLOSED poll handle's endgame. Those other two are what stop the Windows segfault/hang in test/bake/deinitialization.test.ts (build 90708); repro numbers and a rebase of the full fix onto current main are in #33018 (comment).

@robobun

robobun commented Aug 19, 2026

Copy link
Copy Markdown
Collaborator Author

#39643 fixes the crash class behind this leak (the nested tick on the libuv backend) and replaces the data handshake with two bits on us_poll_t (released, uv_closed), so it covers the close-callback-first case from this PR as well. If #39643 lands, this one can be closed, or reduced to its uvPollLiveCount test.

cirospaciari added a commit that referenced this pull request Sep 11, 2026
…libuv are done with it (#39643)

### Problem
- Windows crashes after a handler returns: `us_socket_is_closed` or
`us_internal_socket_follow_adopted` at `0xFFFFFFFFFFFFFFFF` (BUN-43AH,
BUN-4NC8), `us_internal_socket_close_raw` on a freed socket (BUN-442H,
BUN-442Y, BUN-4NP3), a garbage `poll_cb` called from
`uv__fast_poll_process_poll_req` (BUN-442Z), mimalloc free-list crashes,
and the `test/bake/deinitialization.test.ts` failures on the Windows
lanes. The 1.4.0 release (`34cbb9a40`) reports this family from `bun
test` on Windows (as of 2026-08-21: BUN-442H 12 events, BUN-4NC8 6,
BUN-442Y 5, BUN-4NP3 2, BUN-442Z 1) and the TLS forms BUN-4MNY,
BUN-4NFZ, BUN-4NJG, BUN-4NKV and BUN-4QEY, one each. BUN-4QEV, BUN-4QHN
and BUN-4QQ6 are the reused-handle arm described in the notes.
- `loop.c:450` frees closed sockets only at `tick_depth <= 1`. The libuv
backend never counts it, so a handler that waits for a promise (a nested
`uv_run`) frees the socket the outer dispatch still reads
(`loop.c:746`).
- The nested run also finishes closing that socket's `uv_poll_t` under
libuv's outer `uv__fast_poll_process_poll_req` frame. That frame then
queues the endgame again, and `close_cb_free_poll` runs twice. `uv_run`
is documented as not reentrant, so the misuse is ours.

### Fix
- `us_loop_run` and `us_loop_pump` count `tick_depth`, as the POSIX
backend does.
- While a `poll_cb` frame is on the stack (`poll_cb_depth`),
`us_poll_stop` only disarms the handle. The outermost frame calls
`uv_close` on its way back into libuv, which libuv supports, and then
closes the socket (`us_internal_poll_close_fd`), so libuv sees the same
order as before. libuv is unchanged.
- `us_poll_free` and `close_cb_free_poll` record who ran first
(`released`, `uv_closed`). The second one frees both blocks. The old
`data = NULL` handshake leaked when the callback ran first (#37105). A
stopped poll is never re-armed or re-initialized.
- Verified: the new case in `test/js/bun/net/socket.test.ts`. On a
Windows debug build of main its child segfaults at `0xFFFFFFFFFFFFFFFF`
(the BUN-43AH signature), and the release canary crashes too. It passes
with the fix (Windows x64 and arm64 debug, Linux ASAN).
`appcontainer.test.ts` gets a step for the socket order. Notes have the
rest.

### Background
- On Windows a socket is two blocks: the `us_socket_t` (it starts with
the `us_poll_t`) and a libuv `uv_poll_t`. libuv's in-flight AFD requests
live inside the `uv_poll_t`, so it has to live until the close callback.
A closed socket waits on `loop->data.closed_head` until
`us_internal_loop_post` frees it, and `loop.c` reads it after its
handlers return.
- Endgame: `uv_close` cancels the requests. When the last one completes,
libuv queues the endgame, which unlinks the handle and runs the close
callback. `uv__fast_poll_process_poll_req` checks for it right after
`poll_cb` returns.
- Nested tick: `wait_for_promise` (`expect().resolves`, auto-install)
runs `uv_run` inside the handler.

<details><summary>Notes</summary>

**Symbolized fail-before** (Windows x64 debug build of main
`a35696478d`, a standalone copy of the new test's fixture. The test
itself fails on that build with the same fault address):

```
panic(main thread): Segmentation fault at address 0xFFFFFFFFFFFFFFFF
us_internal_socket_follow_adopted   packages/bun-usockets/src/internal/internal.h:357
us_internal_dispatch_ready_poll     packages/bun-usockets/src/loop.c:746   (after us_dispatch_data returned)
poll_cb                             packages/bun-usockets/src/eventing/libuv.c:165
uv__fast_poll_process_poll_req      vendor/libuv/src/win/poll.c:233
uv__process_reqs / uv_run           vendor/libuv/src/win/core.c
us_loop_run                         packages/bun-usockets/src/eventing/libuv.c:417
```

The freed socket is filled with mimalloc's debug poison, so
`flags.adopted` reads as set and `prev` is followed. The `open()`
variant crashes the same way at `loop.c:556`. The same fixture without
the nested wait passes on that build. The release canary
(`1.4.0-canary.1+32e87032b`) passes the first case and crashes in the
second, at `0xFFFFFFFFFFFFFFFF` in one run and `0x1AF0000002A` in
another, which is the heap corruption from the double free.

**How the three Sentry shapes follow.** In release, `mi_free` overwrites
the first word of the freed `uv_poll_t`, which is `data`. The rest stays
intact, so the outer frame sees `events == 0`, `CLOSING`, and no
requests in flight, and queues the endgame again. The second
`close_cb_free_poll` frees `h->data`, now the free-list link to another
freed block, and `h` itself again. Later allocations alias (a garbage
`poll_cb`, BUN-442Z, or `us_poll_start_rc` faulting at 0 in
`deinitialization.test.ts`), or the freed socket is reused and the outer
dispatch runs the error close on it (BUN-442Y), or `follow_adopted`
reads a reused block (BUN-43AH).


**Socket order, found by CI.** The first push closed the socket in
`close_raw` before the deferred `uv_close`. `appcontainer.test.ts`
failed on the Windows 11 arm64 lane with exit `0xC0000008`
(`STATUS_INVALID_HANDLE`), and failed 10 of 10 runs on an arm64 machine
with that build against 5 of 5 passes with main's `libuv.c` on the same
machine. `GetProcessMitigationPolicy(ProcessStrictHandleCheckPolicy)`
inside the container reports `0x3`, so a call on a closed handle raises
instead of failing, and `uv__poll_close` cancels the in-flight request
with an ioctl on the socket. A probe with four steps (listener only,
serve + fetch, terminate from `data()`, terminate from a timer with the
peer closed by its own dispatch) died in every step that closes a socket
from a dispatch. With `us_internal_poll_close_fd` the four steps and the
test pass (10 of 10 runs on arm64, and on x64). The test now also closes
a socket from its own `data()` handler, which is the shortest path to
this order. Outside a nested tick, the later socket close is not
observable from JS: the loop does not run again before the handler's
dispatch ends. Inside one, a handler that closes its socket and then
waits for the peer to notice now waits until the handler returns. That
wait was already unreliable on Windows (the peer's completion is often
in the outer run's batch) and then corrupted the heap.

**The reused-handle arm.** Between the inner run's endgame and the outer
frame's return, `us_create_poll` can hand the freed `uv_poll_t` block to
a new poll (same size class, most recent free). Then the second
`close_cb_free_poll` runs on a live handle: `h->data` is the new poll's
`us_socket_t`, so that socket is freed with no close path, and the new
`uv_poll_t` is freed under its pending request. The JS wrapper of that
socket still reports open. BUN-4QHN (`ws.send()`: `us_socket_group_ext`
on the freed socket's `group`), BUN-4QQ6 (`socket.readyState`:
`SSL_get_shutdown` on its `ssl`) and BUN-4QEV (`uv__process_poll_req`
reading the freed handle for a completed request, `poll.c:574`) are that
arm. With one `uv_close` per handle, issued after its last callback
frame, the second callback no longer exists.

**Why the hand-off is needed together with `tick_depth`.** With
`tick_depth` alone, every socket closed during a nested tick with no
live frame finishes closing in the inner run, and the deferred
`us_poll_free` then met a handle whose close callback had already run:
the old code handed it `data` and leaked both blocks. #37105 fixes that
case on its own with a marker in `data`. This PR covers it with the two
bits.

**Defensive branches with no current caller:** `us_poll_free` on a poll
that was never stopped, `us_poll_start_rc` on a registered poll (mask
change, or `UV_EBADF` once stopped), `us_poll_change` after stop. Every
current caller creates, starts, and later stops a poll exactly once. The
`uv_poll_init_socket` failure path now goes through `us_poll_stop`.
`uv_poll_stop` on a half-initialized handle only clears `events`
(checked against `uv__poll_set`). That path was reasoned about, not run:
it needs a `--socketFaultInjection=on` build, which rebuilds every Rust
crate.

**Suites run on the Windows x64 debug build with the fix**, all green:
`test/js/bun/net/socket.test.ts` (86 pass), `tcp-server`,
`socket-retention`, `socket-syscall-fault`,
`test/js/bun/udp/udp_socket.test.ts`,
`test/js/bun/websocket/websocket-server.test.ts`,
`test/js/node/net/node-net-server.test.ts`,
`test/js/node/tls/node-tls-server.test.ts`,
`test/js/node/http/node-http.test.ts`, `test/js/bun/http/serve.test.ts`
(294 pass), `test/bake/deinitialization.test.ts` 3 of 3 (the unfixed
release canary hangs for 60 s and is killed on the same machine).
`test/js/web/fetch/fetch.test.ts` had 14 timeouts of `(with gc)` body
tests on this slow debug box. The two socket tests among them pass in
isolation. Per the analysis in #33018, `deinitialization.test.ts` closes
the socket from a nested `poll_cb` on the same handle, so it also covers
`poll_cb_depth > 1`.

**Sentry groups on the 1.4.0 release** (`34cbb9a40`, all Windows):
BUN-4NC8 is `follow_adopted` (`internal.h:357`) under `loop.c:746`, the
frames of the fail-before above. BUN-442H is `close_raw` at
`socket.c:292`, the low-prio unlink writing through `s->prev` of a
reused block, reached from the eof or error close at the end of the
dispatch (BUN-442Y and BUN-4NP3 are the same call: the unlink at
`context.c:235` and `:240` writing through the reused block's `prev` and
`next`, and `us_internal_disable_sweep_timer` through its `group`).
BUN-4MNY is `ssl_retry_parked_write` (`openssl.c:2103`, `s->group` read
as NULL) in the tail of `us_internal_ssl_on_data` after
`us_dispatch_data` returned: the TLS form of the same read of a freed
socket. BUN-4NFZ is the other exit of that tail: `us_internal_ssl_close`
(`openssl.c:1927`, again `s->group` NULL) from the `ssl_close` call at
`openssl.c:2324` after the data callback. BUN-4NJG and BUN-4NKV are the
keylog and session flushes in the same tail (`openssl.c:2430` to `:351`,
and `:2429` to `:426`): `s->ssl` read from the reused block, so
`SSL_get_ex_data` returns garbage. Both flushes are guarded by `!s->ssl
|| is_closed`, which only a reused block gets past. The four TLS groups
are the four helpers of that tail, in whichever order a given reused
block fails. With the closed list left alone until the outermost tick,
`ssl_gone`, `is_closed` and `s->ssl` read the real state there and those
tails return.

**Limit.** `tick_depth` counts the ticks Bun itself runs (`us_loop_run`,
`us_loop_pump`). A native addon that calls `uv_run()` on this loop from
inside a handler is not counted. The socket whose own callback is on the
stack is still safe there, because its `uv_close` and frees are tied to
`poll_cb_depth`, but a socket that another frame holds (an accepted
socket closed from its own `open()` while the accept loop still reads
it, or a block retired by adoption) is not. Counting the frames of the
callbacks libuv runs for us (`poll_cb`, `prepare_cb`, `timer_cb`,
`async_cb`) would cover that entry as well. None of the crashes above
needed it, so it is left for a follow-up rather than re-verifying this
PR for it.

**Release-build numbers** for `test/bake/deinitialization.test.ts` are
in the comments below: on one Windows Server 2019 machine the fixture
fails 8 of 8 runs on main (6 hangs, 2 segfaults) and passes 20 of 20
with this diff.

**Related.** #33018 was an earlier attempt at this bug. It patched
libuv's post-callback check instead of moving the `uv_close`. #38024 has
the same bug class in the c-ares poll and still carries that patch.
Deferring its `uv_close` past the outermost callback frame would remove
the need for it there too. The test fixture waits on a timer on purpose:
a nested run cannot wait for an event of the peer socket, because that
completion may sit in the outer run's IOCP batch.

</details>

<!-- robobun:evidence:begin -->

---

**no test proof** · iteration 4 · platform-specific test(s) that do not
run on this machine, deferring to CI, which covers all platforms:
test/js/bun/windows/appcontainer.test.ts, test/js/bun/net/socket.test.ts

<!-- robobun:evidence:end -->

---------

Co-authored-by: Ciro Spaciari <ciro.spaciari@gmail.com>

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant