Repository navigation
tls: defer upgraded-duplex teardown to the owning context's close task - #31364
Jarred-Sumner wants to merge 2 commits into
Conversation
|
Updated 9:05 PM PT - May 28th, 2026
❌ @Jarred-Sumner, your commit 1f67f36 has some failures in 🧪 To try this PR locally: bunx bun-pr 31364That installs a local version of the PR into your bun-31364 --bun |
|
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 (1)
💤 Files with no reviewable changes (1)
WalkthroughThis PR fixes HTTP/2 upgrade socket lifecycle by moving teardown from the ChangesHTTP/2 Upgrade Socket Lifecycle
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any issues — the reasoning in the PORT NOTE checks out against SSLWrapper::trigger_close_callback/flush and DuplexUpgradeContext::on_close — but this is unsafe-Rust TLS teardown that shifts when the wrapper, timer, and JS function-data are released, so it's worth a human pass from someone who owns this path.
Extended reasoning...
Overview
The Rust change is a single deletion: UpgradedDuplex::on_close no longer calls this.teardown() inline, and instead relies on the already-enqueued DuplexUpgradeContext next-tick Close task to drop the struct (whose Drop impl runs the same teardown()). The rest is a detailed PORT NOTE comment plus an updated doc comment on teardown(). Two regression tests are added to node-http2-upgrade.test.mts exercising sequential connect/close cycles and post-destroy raw-socket writes.
Security risks
This is TLS socket lifecycle code. The bug being fixed is real UB: on_close is invoked from inside SSLWrapper::trigger_close_callback, and writing self.wrapper = None there leaves the niche-optimized Option<SSLWrapper> payload uninitialized while frames like flush() (src/uws/lib.rs:657–661) still re-read self.ssl after the callback returns — a wild SSL* into BoringSSL. The fix removes that hazard rather than introducing one. I see no new injection/auth/data-exposure surface.
Level of scrutiny
High. The correctness of deferring teardown rests on cross-file invariants: (1) DuplexUpgradeContext::on_close unconditionally calls deinit_in_next_tick() (socket_body.rs:3585) and has already take()n tls; (2) closed_notified is set before the callback fires (uws/lib.rs:822), gating handle_traffic/update_handshake_state and the data/write/close trampolines; (3) the SSL* stays live until Drop, so any re-entry that does slip through operates on valid memory. I verified all three, but this is exactly the kind of raw-pointer + re-entrancy + niche-layout reasoning a maintainer should confirm.
Other factors
There is a behavioral shift worth a second pair of eyes: previously teardown() ran eagerly, so the event-loop timer was cleared and the four JS host-function data pointers were nulled inside on_close. Now those stay live until the next tick. If the JS end() call (via call_write_or_end) or TLSSocket::on_close synchronously re-enters on_received_data/on_writable, they'll now reach a live wrapper instead of bailing on null function-data — closed_notified should make those inert, but it's a different gate than before. The new tests cover the crash repro well; no CODEOWNERS entry matches these paths.
UpgradedDuplex::on_close used to tear the SSLWrapper down in place even though it is invoked from the wrapper's own close callback, with wrapper frames still on the stack below it. Defer the teardown to the owner's next-tick Close task, which already frees the containing DuplexUpgradeContext (and runs the same teardown via Drop). Adds two cases to node-http2-upgrade.test.mts covering repeated sequential connection close and post-destroy raw-socket events.
4c48d6c to
9a4f69f
Compare
|
Classified the failing check: build #57663's only hard failure is test/js/node/http/node-http-backpressure-max.test.ts timing out at 60s on the darwin-14 shard. That test is plain node:http over TCP streaming a 4GB buffer with no TLS involved, so it never reaches the UpgradedDuplex path this PR changes — it's an unrelated CI machine flake (every other failure in the build is tagged flaky and passed on retry). Rebased the branch onto current origin/main (clean — main hasn't touched UpgradedDuplex.rs or the test file since the old base, commit is now 9a4f69f), rebuilt, and re-ran the http2-upgrade and TLS upgraded-duplex tests locally; all pass. Force-pushed for a fresh CI run. |
|
Actionable comments posted: 0 |
There was a problem hiding this comment.
Thanks for tightening the PORT NOTE — the re-entry gates now read accurately. I have nothing further, but since this is a memory-safety change in unsafe Rust on the BoringSSL FFI path whose correctness leans on the DuplexUpgradeContext drop-ordering invariant, I'd like a human to give it a final look.
Extended reasoning...
Overview
The PR removes the in-place this.teardown() call from UpgradedDuplex::on_close (src/runtime/socket/UpgradedDuplex.rs) and replaces it with a detailed PORT NOTE explaining why teardown must be deferred to the owning DuplexUpgradeContext's next-tick Close task (which drops the struct and runs teardown() via Drop). The rationale is that on_close is invoked from inside SSLWrapper::trigger_close_callback with live &mut SSLWrapper frames still on the stack that re-read self.ssl after return; writing self.wrapper = None in place leaves the niche-optimized Option payload uninitialized and feeds a wild SSL* to BoringSSL. The teardown() doc comment is updated to state the same constraint. Two regression tests are added to test/js/node/http2/node-http2-upgrade.test.mts: 20 sequential connect/request/close cycles, and raw-socket writes after server-side session destroy.
I previously flagged that the comment's sent_ssl_shutdown clause overstated the re-entry gate; the author verified against src/uws/lib.rs, reworded the note to cite closed_notified + the still-valid ssl pointer as the actual gates, and resolved the thread. I spot-checked the cross-file invariant the fix relies on — DuplexUpgradeContext::on_close at socket_body.rs:3604 does unconditionally null self.tls and call deinit_in_next_tick() — so the comment's claims hold.
Security risks
No new attack surface is introduced; the change removes a use-after-uninit on the TLS path. The residual risk is the inverse: if any future caller reaches UpgradedDuplex::on_close via a handler whose on_close does not schedule a deferred drop, the wrapper would now leak instead of being torn down — but the only wired handler is DuplexUpgradeContext, which always schedules it. No auth, crypto-primitive, or permission logic is touched.
Level of scrutiny
High. This is unsafe Rust managing raw SSL* lifetime across re-entrant FFI callbacks, and correctness hinges on a cross-file drop-ordering invariant (DuplexUpgradeContext → next-tick Close → Drop for UpgradedDuplex → teardown). The diff is tiny (one removed call + comments + tests), but the blast radius of getting TLS connection teardown wrong is large — this is exactly the class of change that benefits from a second human pair of eyes on the lifecycle reasoning.
Other factors
CI build #58768's only failure is a Windows-aarch64 timeout in an unrelated plain-HTTP abort test that the author classified as a flake (passed on retry on another Windows shard). The bug-hunting system found nothing. The new tests exercise both the sequential-realloc and the post-close re-entry paths and also run under Node.js for parity. Given all that, the PR looks correct to me — I'm deferring purely on the "unsafe + TLS lifecycle" scrutiny bar, not on any open concern.
|
Looked into the red buildkite/bun status: it comes only from the darwin-14-aarch64 and darwin-26-aarch64 test shards, which expired waiting for a macOS aarch64 agent and never actually ran, so there is no test output attributable to this diff. The jobs relevant to this change (debian-13-x64-asan-test-bun, darwin-14-x64, and all Linux test shards) all passed at 1f67f36, and the only test failures in the build were known-flaky files unrelated to UpgradedDuplex.rs / node-http2-upgrade.test.mts that passed on retry. The expired darwin aarch64 shards have been retried and are queued; no code change is needed here. |
… pipe (#43806) ### Problem - A TLS server over a Duplex crashes when its `ALPNCallback` calls `socket.write()` or `socket.end(data)`: `panic(main thread): Segmentation fault at address 0xB0`. A remote client triggers it. - Over a synchronous Duplex pair, a `write()` made while the peer's last handshake flight is queued completes the handshake unreported: no `secureConnect` or `secure`, and no `rejectUnauthorized` check. - Cause: `SSLWrapper::write_data` (`src/uws/lib.rs:851`) calls `SSL_write` while the handshake is pending, and `SSL_write` runs `SSL_do_handshake` itself. Inside `ALPNCallback` that call fails, and the close frees the SSL under the running handshake. Elsewhere it can finish the handshake with no `on_handshake`. ### Fix - While the state is `HandshakePending`, `write_data` runs `handle_traffic` and returns `WantRead` without `SSL_write`. Only `update_handshake_state` drives the initial handshake, and it reports every result. - Correct because `UpgradedDuplex` and `WindowsNamedPipe` already park a `WantRead` write and retry it after `on_handshake`. Where `SSL_write` returned `WANT_READ` before, nothing changes. - Verified: 8 new tests in `test/js/node/tls/node-tls-connect.test.ts` fail on a build of `main` and pass with the change. They also pass on Windows x64. ### Background - `SSLWrapper` runs BoringSSL over memory BIOs for TLS on a `Duplex`, a Windows named pipe, or a CONNECT tunnel. - Its `handle_traffic` pass calls `SSL_do_handshake` and fires `on_handshake`, where JS emits `secureConnect` and applies `rejectUnauthorized`. A nested call only flushes, so a synchronous peer's answer stays queued. ### Downsides - `socket.destroy()` from `ALPNCallback` over a Duplex still crashes: it reaches the same free through `close`. #31364 defers that teardown. - A failed handshake with a write parked now reports like one with no write: with `rejectUnauthorized: false`, a wrong `secureConnect` before the error (#32929 corrects it). <details><summary>Notes</summary> - Found by a code audit of `SSLWrapper`. No user reported it. - The crash on main, from an ASAN debug build: the write in `ALPNCallback` reaches `SSL_write`, whose nested `SSL_do_handshake` fails in `tls_can_accept_handshake_data`. `write_data` takes its error branch: `trigger_close_callback`, `UpgradedDuplex::on_close`, `teardown`, `SSLWrapper::deinit`, `SSL_free`. The outer `SSL_do_handshake` then reads the freed ClientHello in `SSL_select_next_proto`. It happens on TLS 1.2 and 1.3, on Linux and Windows, with the in-memory pair and with a Duplex over a TCP socket. - Repro for the unreported handshake: both sides are `Duplex` streams whose `write()` pushes the chunk into the peer at once. The client issues `tlsSocket.write()` from inside its transport `write()`, after the push. With TLS 1.3 that is the ClientHello write. With TLS 1.2 it is the client's second flight. The server case needs the client created first, so that the client's engine is idle and answers from inside the server's transport write. - Stock `stream.duplexPair()` delivers synchronously too. With it, a server `keylog` listener that writes on the client socket hits the same window on TLS 1.3: the client never emits `secureConnect` on 1.4.3-canary. - Results on 1.4.3-canary, TLS 1.3, a chain that fails verification, default `rejectUnauthorized`: the server gets `secure` with the client certificate and the application data, and the client emits neither `secureConnect` nor `error`. Node v26.3.0 emits the client `error` and the server sees nothing. With the fix Bun does the same. - The measurement behind the second Downsides bullet: a client with `minVersion: "TLSv1.3", maxVersion: "TLSv1.2"` over a Duplex, `rejectUnauthorized: false`, and a write right after `tls.connect()`. 1.4.3-canary: `write callback ERR_SOCKET_CLOSED`, `error ERR_SOCKET_CLOSED`, `close`. With the change: `secureConnect authorized=false UNABLE_TO_GET_ISSUER_CERT` first, then the same three. Without the write both builds emit only that `secureConnect`. Node emits `error ERR_SSL_NO_PROTOCOLS_AVAILABLE`. - The fd-backed path already parks these writes. `us_internal_ssl_write` parks a write made inside `SSL_do_handshake` (`ssl_in_use`) and sets `ssl_read_input_length = 0`, so its `SSL_write` cannot read a peer flight. - Node does not park a write made inside `ALPNCallback`. Its handshake fails with `ERR_SSL_UNEXPECTED_MESSAGE`. Bun's fd-backed path delivers it after the handshake ("writing to the socket from inside SNICallback or ALPNCallback delivers the data after the handshake" in `node-tls-server.test.ts`). The Duplex path now does the same for `write()` and `end(data)`. - The rejected-certificate rows: the identity verdict (`servername` not in the certificate) runs on both TLS versions, the chain verdict on TLS 1.3 only. A TLS 1.2 client has the chain verdict before its second flight, so a client that rejects the chain during the handshake (#43767, merged) never reaches that write. On TLS 1.3 the early write still gets past that check on `main`: the chain row fails there with `server received secret`. - A TLS 1.2 client that rejects the chain still sends its client certificate before the `error`, as Node does. A TLS 1.2 renegotiation is still driven by reads and writes. A bare `end()` in the same window does not reach `write_data` with data, and behaves the same with and without the change. - A `debug_assert!` in `update_handshake_state` pins the invariant: the initial handshake never completes outside it. - `data` is not read after `handle_traffic` in the new branch. The nested `on_writable` flush can free the buffer that `data` points into (`buffered_data_for_node_net`). The existing `WANT_READ` branch has the same rule. A review suggested a call to `SSL_write` after `handle_traffic` when the handshake completed inside it. I built that: it is a heap-use-after-free under ASAN in `SSL_write`, and the bytes are not lost without it, because the nested flush sends them. - `ProxyTunnel` and `WebSocketProxyTunnel` write only after `on_handshake`, so the guard does not change them. - Self-reviewed. Addressed: the crash explanation, the TLS 1.2 rejected row that #43767 makes unreachable, the claim that only the two broken cases change, the `debug_assert!`, the `end(data)` and TCP rows, and the duplicate Duplex-pair helper. Not in this PR: the `destroy()` case (#31364), the failure report (#32929). - Rebased on `main` abc26b7. Suites run with the debug build on that base: every file in `test/js/node/tls/` and `test/js/bun/net/`, `proxy.test.ts`, `websocket-proxy.test.ts` and `fetch-proxy-connect-tunnel-split-envelope.test.ts`. Five tests fail, and each fails the same way on a build of `main`: `SNICallback runs even when the requested servername matches the bind hostname` (`ECONNREFUSED` in this container), `concurrent Workers all see the same CA certificate lists` (needs 5.0 to 5.2 s on `main`, over the 5 s default), `node:net handle whose socket never starts connecting is collectable`, `an unref'd Bun.listen() with no other references keeps accepting across GC`, and `should not call drain before handshake` (needs `www.example.com`). - Trial merge with #43924 (a3b00db): no conflict. On the merged tree `node-tls-connect.test.ts`, `node-tls-duplex-end-verify.test.ts`, `renegotiation.test.ts`, `tls-reject-before-client-cert.test.ts`, `node-tls-wrapped-socket-close.test.ts` and `node-tls-upgrade.test.ts` pass. - Vendored Node tests run with the debug build: `test-tls-connect-stream-writes`, `test-tls-destroy-stream`, `test-tls-destroy-stream-12`, `test-tls-js-stream`, `test-tls-on-empty-socket`, `test-tls-socket-close`, `test-tls-socket-default-options`, `test-tls-socket-destroy`, `test-tls-socket-failed-handshake-emits-error`, `test-tls-streamwrap-buffersize`, `test-tls-transport-destroy-after-own-gc`, `test-tls-wrap-econnreset-pipe`, `test-tls-wrap-event-emmiter`, `test-double-tls-server`, `test-http2-generic-streams`, `test-http2-generic-streams-sendfile`, `test-tls-inception`, `test-tls-alpn-server-client`, `test-tls-close-error`, `test-tls-handshake-error`. - Windows x64, debug build of the rebased head: `node-tls-connect.test.ts` (96 pass, 1 skip), `tls-reject-before-client-cert.test.ts` (117 pass, 2 skip), and `node-tls-namedpipes.test.ts` (6 pass, with a 60 s timeout). On the earlier base `should work with named pipes and tls` (400 connections) needed 5.8 s on that debug build with the change and 5.9 s without it, so it exceeds the 5 s default there either way. </details> <!-- robobun:evidence:begin --> --- **[human-review]** gate passed · iteration 0 · 2 files touched <details><summary>fails on main (without fix)</summary> ```console ASAN without fix: 8 failed, 18 skipped $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" test/js/node/tls/node-tls-connect.test.ts bun test v1.4.3 (367d939) test/js/node/tls/node-tls-connect.test.ts: (pass) should have checkServerIdentity [2.95ms] (pass) should thow ECONNRESET if FIN is received before handshake [321.60ms] (pass) initializes authorizationError to null in the TLSSocket constructor [6.80ms] (pass) setMaxSendFragment mirrors OpenSSL's [512, 16384] acceptance without throwing [138.65ms] (pass) should be able to grab the JSStreamSocket constructor [22.65ms] (skip) tls.connect > should work with alpnProtocols (pass) tls.connect > Bun.serve() should work with tls and Bun.file() [132.66ms] (pass) tls.connect > should have peer certificate when using self asign certificate [91.20ms] (skip) tls.connect > should have peer certificate (skip) tls.connect > getCipher, getProtocol, getEphemeralKeyInfo, getSharedSigalgs, getSession, exportKeyingMaterial and isSessionReused should work (skip) tls.connect > should process options correctly when connect is called with only options (skip) tls.connect > should process port a ... (truncated) release without fix: 15 failed, 18 skipped bun test v1.4.3-canary.1 (367d939) test/js/node/tls/node-tls-connect.test.ts: (pass) should have checkServerIdentity [0.05ms] (pass) should thow ECONNRESET if FIN is received before handshake [8.80ms] (pass) initializes authorizationError to null in the TLSSocket constructor [0.27ms] (pass) setMaxSendFragment mirrors OpenSSL's [512, 16384] acceptance without throwing [5.64ms] (pass) should be able to grab the JSStreamSocket constructor [0.37ms] (skip) tls.connect > should work with alpnProtocols (pass) tls.connect > Bun.serve() should work with tls and Bun.file() [6.71ms] (pass) tls.connect > should have peer certificate when using self asign certificate [5.79ms] (skip) tls.connect > should have peer certificate (skip) tls.connect > getCipher, getProtocol, getEphemeralKeyInfo, getSharedSigalgs, getSession, exportKeyingMaterial and isSessionReused should work (skip) tls.connect > should process options correctly when connect is called with only options (skip) tls.connect > should process port and host correctly (skip) tls.connect > should process port, host, and callback correctly (skip) tls.connect > should handle the absence of a callback gracefully (skip) tls.c ... (truncated) ``` </details> <details><summary>passes on PR (with fix)</summary> ```console ASAN with fix: 18 skipped $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" test/js/node/tls/node-tls-connect.test.ts bun test v1.4.3 (367d939) test/js/node/tls/node-tls-connect.test.ts: (pass) should have checkServerIdentity [2.73ms] (pass) should thow ECONNRESET if FIN is received before handshake [304.31ms] (pass) initializes authorizationError to null in the TLSSocket constructor [6.88ms] (pass) setMaxSendFragment mirrors OpenSSL's [512, 16384] acceptance without throwing [96.91ms] (pass) should be able to grab the JSStreamSocket constructor [12.12ms] (skip) tls.connect > should work with alpnProtocols (pass) tls.connect > Bun.serve() should work with tls and Bun.file() [73.43ms] (pass) tls.connect > should have peer certificate when using self asign certificate [74.52ms] (skip) tls.connect > should have peer certificate (skip) tls.connect > getCipher, getProtocol, getEphemeralKeyInfo, getSharedSigalgs, getSession, exportKeyingMaterial and isSessionReused should work (skip) tls.connect > should process options correctly when connect is called with only options (skip) tls.connect > should process port and ... (truncated) release with fix: 18 skipped $ bun scripts/build.ts --profile=release [configured] bun-profile → bun (stripped) target linux-x64-gnu build type Release build dir ./build/release revision b2cb31d features lto, baseline 23 deps, 136 codegen, 1176 objects in 1233ms ninja: Entering directory `/workspace/bun/build/release' [1/4] fetch lolhtml [lolhtml] up to date [2/4] fetch rust-argon2 [rust-argon2] up to date [2/4] cargo plan → /workspace/bun/build/release/rust-target/plan.json 244 units: 172 lib, 16 proc-macro (host), 19 custom-build (host), 15 run custom-build, 17 lib (host), 4 run custom-build (host), 1 rlib [3/4] reconfigure [1/1499] mkdir stamps [2/1499] mkdir codegen [3/1499] rustc unicode_xid [4/1499] rustc unicode_ident [5/1499] install /workspace/bun bun install v1.4.3-canary.1 (367d939) Checked 26 installs across 65 packages (no changes) [155.00ms] [6/1499] gen bake.{client,server,error}.js -> bake.client.js, bake.server.js, bake.error.js [7/1499] install /workspace/bun/packages/bun-error bun install v1.4.3-canary.1 (367d939) Checked 1 install across 2 packages (no changes) [4.00ms] [8/1499] gen ErrorCode+*.h [9/1499] gen string-map defines_t ... (truncated) ``` </details> <details><summary>diff hotspot</summary> ``` src/uws/lib.rs | 9 + test/js/node/tls/node-tls-connect.test.ts | 302 +++++++++++++++++++++++++++++- 2 files changed, 306 insertions(+), 5 deletions(-) ``` </details> **gate history** · 2 passed · 0 rejected · iteration 0 <details><summary>evidence per changed file</summary> ``` file reads edits tests src/uws/lib.rs 5 6 55 test/js/node/tls/node-tls-connect.test.ts 8 15 51 ``` </details> <!-- robobun:evidence:end -->
|
Closing: #32521 (46c54cd) changed I checked
Two results that were not clean:
#46522 also lists this PR as already fixed on |
UpgradedDuplex::on_closetore theSSLWrapperdown in place even though it is invoked from the wrapper's own close callback, with wrapper frames still on the stack below it that re-read the wrapper's fields after it returns. Teardown is now deferred to the owner's next-tick Close task, which already frees the containingDuplexUpgradeContextand runs the same teardown viaDrop.Adds two cases to
node-http2-upgrade.test.mts: 20 sequential connect/request/close cycles against an upgradednet.Server->Http2SecureServer, and raw-socket writes after the server-side session is destroyed. Existing cases in that file andnode-tls-connect.test.tsstill pass.