socket: guard flush() with isDetached() to prevent UAF on TLS close - #30136
Conversation
On a TLS client, end() → internalFlush → markInactive → closeAndDetach(.normal) detaches this.socket and sends close_notify, but defers the raw close until the peer replies — leaving is_active set so the eventual onClose can release the Handlers allocation. flush() was the only internalFlush() caller without an isDetached() guard, so calling flush() in that window re-entered markInactive, saw the detached socket as closed, and ran the full teardown — vm.allocator.destroy(handlers). When the peer's close_notify then arrived, onClose dereferenced the freed *Handlers (ASAN use-after-poison at socket.zig onClose). Add the same isDetached() early-return that end(), endBuffered(), onWritable, etc. already have.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThis PR fixes a re-entrancy bug in ChangesSocket Flush Re-entrancy Fix
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Review rate limit: 3/5 reviews remaining, refill in 23 minutes and 19 seconds. Comment |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — one-line guard matching the established pattern in every other internalFlush() caller, with a clear ASAN repro and regression test.
Extended reasoning...
Overview
Adds a single if (this.socket.isDetached()) return .js_undefined; guard to the JS-exposed flush() method in src/bun.js/api/bun/socket.zig, plus a subprocess-isolated regression test in test/js/bun/net/socket.test.ts. The PR description includes an ASAN trace and a precise root-cause analysis of how end() → deferred TLS close leaves is_active set, allowing a follow-up flush() to re-enter markInactive() and free *Handlers before the deferred onClose runs.
Verification
I confirmed the PR's claim by grepping all internalFlush() call sites: onWritable (guarded at :250), writeBuffered/endBuffered (guarded at :948/:962), and end (guarded at :1337) all already early-return on isDetached(). flush() was the only outlier. The added guard is byte-for-byte the same idiom used in ~25 other methods in this file.
Security risks
None introduced. The change strictly narrows behavior — calling flush() on a detached socket now no-ops instead of proceeding into internalFlush() → markInactive(). This closes a use-after-free, so the security posture only improves.
Level of scrutiny
Low-to-moderate. The source change is a one-line defensive guard following an established, repeated pattern in the same file. The test is additive, runs in a subprocess (so an ASAN abort can't take down the test runner), and uses existing harness helpers (tempDir, bunExe, bunEnv, tls).
Other factors
No prior reviewer comments to address. The bug-hunting system found no issues. The PR description demonstrates the test fails without the fix (ASAN abort) and passes with it.
…ven-sh#30136) ## Repro ```js const client = await Bun.connect({ hostname, port, tls, socket: { ... } }); // after handshake: client.end("x"); client.flush(); // ← second markInactive frees *Handlers // peer replies close_notify → onClose derefs freed Handlers ``` ASAN on debug build: ``` ==4075==ERROR: AddressSanitizer: use-after-poison on address 0x7aff355e0469 READ of size 1 at 0x7aff355e0469 thread T0 #0 bun.js.api.bun.socket.NewSocket(true).onClose src/bun.js/api/bun/socket.zig:661:46 oven-sh#1 deps.uws.handlers.PtrHandler(...).onClose src/deps/uws/handlers.zig:49:61 ... oven-sh#5 us_internal_ssl_on_close packages/bun-usockets/src/crypto/openssl.c:940:29 ``` ## Cause `end()` → `internalFlush` → `canEndAfterFlush()` → `markInactive()` → `closeAndDetach(.normal)` detaches `this.socket` and calls `us_socket_close(code=0)`. For TLS with `code==0`, `us_internal_ssl_close` sends close_notify and **defers** the raw close until the peer replies (so the loop stays alive to receive it). `markInactive` returns early without clearing `is_active`, relying on the eventual `onClose` → `markInactive` to run `handlers.markInactive()` and free the client-mode `*Handlers`. `flush()` was the only `internalFlush()` caller without an `isDetached()` guard. Calling it in that window re-enters `canEndAfterFlush()` (still `is_active && end_after_flush`) → `markInactive()`, which now sees the detached socket as closed and runs the **full** teardown: `handlers.markInactive()` → `active_connections == 0` → `vm.allocator.destroy(handlers)`. When the peer's close_notify later arrives, `onClose` calls `this.getHandlers()` on freed memory. ## Fix Add the same `isDetached()` early-return to `flush()` that `end()`, `endBuffered()`, `onWritable`, and every other `internalFlush()` caller already have. ## Verification New test in `test/js/bun/net/socket.test.ts` spawns a TLS client that does `end("x"); flush(); flush();` after handshake and awaits `close`. - Without fix (`git stash -- src/`): subprocess aborts with the ASAN trace above; test fails. - With fix: subprocess prints `OK` and exits 0; test passes. Co-authored-by: robobun <robobun@users.noreply.github.com>
2 participants
Repro
ASAN on debug build:
Cause
end()→internalFlush→canEndAfterFlush()→markInactive()→closeAndDetach(.normal)detachesthis.socketand callsus_socket_close(code=0). For TLS withcode==0,us_internal_ssl_closesends close_notify and defers the raw close until the peer replies (so the loop stays alive to receive it).markInactivereturns early without clearingis_active, relying on the eventualonClose→markInactiveto runhandlers.markInactive()and free the client-mode*Handlers.flush()was the onlyinternalFlush()caller without anisDetached()guard. Calling it in that window re-enterscanEndAfterFlush()(stillis_active && end_after_flush) →markInactive(), which now sees the detached socket as closed and runs the full teardown:handlers.markInactive()→active_connections == 0→vm.allocator.destroy(handlers). When the peer's close_notify later arrives,onClosecallsthis.getHandlers()on freed memory.Fix
Add the same
isDetached()early-return toflush()thatend(),endBuffered(),onWritable, and every otherinternalFlush()caller already have.Verification
New test in
test/js/bun/net/socket.test.tsspawns a TLS client that doesend("x"); flush(); flush();after handshake and awaitsclose.git stash -- src/): subprocess aborts with the ASAN trace above; test fails.OKand exits 0; test passes.