From cd5389ed1609316a588d03dc16caa48194786c33 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Sun, 19 Jul 2026 01:27:37 +0000 Subject: [PATCH 1/3] tls: root the duplex origin and listener thunks through the wrapper instead of as Strong handles UpgradedDuplex held five StrongOptional handles per TLS-over-Duplex connection: the origin stream plus the four native listener thunks it creates for get_js_handlers. Each Strong is an independent HandleSet entry whose lifetime is tied to the native struct rather than to the JS wrapper's reachability. Move all five onto the JSTLSSocket wrapper as visited values: slots (duplexOrigin, duplexOnData, duplexOnEnd, duplexOnWritable, duplexOnClose). UpgradedDuplex keeps plain JSValue shadows for its own reads and stack-roots the wrapper across on_close so the slots stay valid through the close handler's downgrade + re-entry into JS. Teardown still neuters the thunks' function data via the shadows. Same pattern as #31859 (socket handlers) and #34346 (server callbacks). --- src/runtime/socket/UpgradedDuplex.rs | 114 ++++++++++++++--------- src/runtime/socket/socket_body.rs | 1 + src/runtime/socket/sockets.classes.ts | 9 +- test/js/bun/net/socket-retention.test.ts | 66 +++++++++++++ 4 files changed, 143 insertions(+), 47 deletions(-) diff --git a/src/runtime/socket/UpgradedDuplex.rs b/src/runtime/socket/UpgradedDuplex.rs index 3d0dedfe145a..1c3c7012bddb 100644 --- a/src/runtime/socket/UpgradedDuplex.rs +++ b/src/runtime/socket/UpgradedDuplex.rs @@ -16,17 +16,22 @@ use core::ffi::{CStr, c_uint, c_void}; use core::ptr::NonNull; use bun_jsc::virtual_machine::VirtualMachine; -use bun_jsc::{CallFrame, GlobalRef, JSGlobalObject, JSValue, JsResult, StrongOptional, host_fn}; +use bun_jsc::{CallFrame, GlobalRef, JSGlobalObject, JSValue, JsResult, host_fn}; use bun_uws::{us_bun_verify_error_t, uws_callback}; use super::ssl_wrapper::SSLWrapper; +use crate::generated_classes::js_TLSSocket; use crate::timer::{ElTimespec, EventLoopTimer, EventLoopTimerState, EventLoopTimerTag}; bun_output::declare_scope!(UpgradedDuplex, visible); pub(crate) struct UpgradedDuplex { pub wrapper: Option, - pub origin: StrongOptional, // any duplex + /// The owning `JSTLSSocket` wrapper. Its `values:` slots root `origin` and + /// the four listener thunks; the `JSValue` fields below are read-side + /// shadows of those slots. + pub js_wrapper: JSValue, + pub origin: JSValue, // any duplex // JSC_BORROW per LIFETIMES.tsv. pub global: Option, pub ssl_error: CertError, @@ -35,10 +40,10 @@ pub(crate) struct UpgradedDuplex { // `zeroed()` before overwriting via `from()`). pub vm: Option<&'static VirtualMachine>, pub handlers: Handlers, - pub on_data_callback: StrongOptional, - pub on_end_callback: StrongOptional, - pub on_writable_callback: StrongOptional, - pub on_close_callback: StrongOptional, + pub on_data_callback: JSValue, + pub on_end_callback: JSValue, + pub on_writable_callback: JSValue, + pub on_close_callback: JSValue, pub event_loop_timer: EventLoopTimer, pub current_timeout: u32, } @@ -76,28 +81,29 @@ pub struct Handlers { use crate::jsc_hooks::timer_all_mut as timer_all; -/// Lazily create-and-cache a JS host-function callback in `slot`. -/// -/// All four `get_js_handlers` slots follow the identical pattern: +/// Lazily create-and-cache a JS host-function callback in `shadow`, mirrored +/// into the owning `JSTLSSocket` wrapper's visited `values:` slot (the GC +/// root). All four `get_js_handlers` slots follow the identical /// `NewFunctionWithData(global, null, 0, fn, self)` → `ensureStillAlive` → -/// redundant `setFunctionData(self)` → `Strong.Optional.create`. +/// `setFunctionData(self)` → store pattern. #[inline] fn lazy_js_handler( - slot: &mut StrongOptional, + shadow: &mut JSValue, + js_wrapper: JSValue, + set_slot: fn(JSValue, &JSGlobalObject, JSValue), global: &JSGlobalObject, func: host_fn::JsHostFn, this_ptr: *mut c_void, ) -> JSValue { - match slot.get() { - Some(cb) => cb, - None => { - let callback = host_fn::new_function_with_data(global, None, 0, func, this_ptr); - callback.ensure_still_alive(); - host_fn::set_function_data(callback, Some(this_ptr)); - *slot = StrongOptional::create(callback, global); - callback - } - } + if !shadow.is_empty() { + return *shadow; + } + let callback = host_fn::new_function_with_data(global, None, 0, func, this_ptr); + callback.ensure_still_alive(); + host_fn::set_function_data(callback, Some(this_ptr)); + set_slot(js_wrapper, global, callback); + *shadow = callback; + callback } impl UpgradedDuplex { @@ -153,12 +159,19 @@ impl UpgradedDuplex { // SAFETY: SSLWrapper handlers ctx is `self as *mut Self`; live for the wrapper's lifetime. let this = unsafe { &mut *this }; + // Keep the wrapper (and so its visited `duplex*` slots) reachable + // across `handlers.on_close`, which downgrades the socket's own strong + // self-reference and re-enters JS. + let js_wrapper = this.js_wrapper; + js_wrapper.ensure_still_alive(); + (this.handlers.on_close)(this.handlers.ctx); // closes the underlying duplex this.call_write_or_end(None, false); // Early teardown (struct itself is dropped later by parent). this.teardown(); + js_wrapper.ensure_still_alive(); } fn call_write_or_end(&mut self, data: Option<&[u8]>, msg_more: bool) { @@ -168,9 +181,10 @@ impl UpgradedDuplex { if vm.is_shutting_down() { return; } - let Some(duplex) = self.origin.get() else { + let duplex = self.origin; + if duplex.is_empty() { return; - }; + } // global is set in `from()` whenever origin is set. let Some(global) = self.global else { return }; @@ -250,20 +264,23 @@ impl UpgradedDuplex { pub(crate) fn from( global: &JSGlobalObject, + js_wrapper: JSValue, origin: JSValue, handlers: Handlers, ) -> UpgradedDuplex { + js_TLSSocket::duplex_origin_set_cached(js_wrapper, global, origin); UpgradedDuplex { vm: Some(global.bun_vm()), - origin: StrongOptional::create(origin, global), + js_wrapper, + origin, global: Some(GlobalRef::from(global)), wrapper: None, handlers, ssl_error: CertError::default(), - on_data_callback: StrongOptional::empty(), - on_end_callback: StrongOptional::empty(), - on_writable_callback: StrongOptional::empty(), - on_close_callback: StrongOptional::empty(), + on_data_callback: JSValue::ZERO, + on_end_callback: JSValue::ZERO, + on_writable_callback: JSValue::ZERO, + on_close_callback: JSValue::ZERO, event_loop_timer: EventLoopTimer::init_paused(EventLoopTimerTag::UpgradedDuplex), current_timeout: 0, } @@ -274,11 +291,14 @@ impl UpgradedDuplex { array.ensure_still_alive(); let this_ptr = std::ptr::from_mut(self).cast::(); + let js_wrapper = self.js_wrapper; array.put_index( global, 0, lazy_js_handler( &mut self.on_data_callback, + js_wrapper, + js_TLSSocket::duplex_on_data_set_cached, global, __jsc_host_on_received_data, this_ptr, @@ -289,6 +309,8 @@ impl UpgradedDuplex { 1, lazy_js_handler( &mut self.on_end_callback, + js_wrapper, + js_TLSSocket::duplex_on_end_set_cached, global, __jsc_host_on_end, this_ptr, @@ -299,6 +321,8 @@ impl UpgradedDuplex { 2, lazy_js_handler( &mut self.on_writable_callback, + js_wrapper, + js_TLSSocket::duplex_on_writable_set_cached, global, __jsc_host_on_writable, this_ptr, @@ -309,6 +333,8 @@ impl UpgradedDuplex { 3, lazy_js_handler( &mut self.on_close_callback, + js_wrapper, + js_TLSSocket::duplex_on_close_set_cached, global, __jsc_host_on_close_js, this_ptr, @@ -518,22 +544,20 @@ impl UpgradedDuplex { wrapper.deinit(); } - self.origin.deinit(); - if let Some(callback) = self.on_data_callback.get() { - host_fn::set_function_data(callback, None); - self.on_data_callback.deinit(); - } - if let Some(callback) = self.on_end_callback.get() { - host_fn::set_function_data(callback, None); - self.on_end_callback.deinit(); - } - if let Some(callback) = self.on_writable_callback.get() { - host_fn::set_function_data(callback, None); - self.on_writable_callback.deinit(); - } - if let Some(callback) = self.on_close_callback.get() { - host_fn::set_function_data(callback, None); - self.on_close_callback.deinit(); + // Neuter the listener thunks so a late `origin` event sees null + // function data instead of a freed `*mut Self`. GC-root clearing is + // left to the wrapper's own collection. + self.origin = JSValue::ZERO; + for cb in [ + &mut self.on_data_callback, + &mut self.on_end_callback, + &mut self.on_writable_callback, + &mut self.on_close_callback, + ] { + if !cb.is_empty() { + host_fn::set_function_data(*cb, None); + *cb = JSValue::ZERO; + } } self.ssl_error = CertError::default(); } @@ -557,7 +581,7 @@ fn on_received_data(global: &JSGlobalObject, frame: &CallFrame) -> JsResult(self_ptr) }; if args.len >= 1 { let data_arg = args.ptr[0]; - if this.origin.has() { + if !this.origin.is_empty() { if data_arg.is_empty_or_undefined_or_null() { return Ok(JSValue::UNDEFINED); } diff --git a/src/runtime/socket/socket_body.rs b/src/runtime/socket/socket_body.rs index 48c7630c89ea..8e9557b391fb 100644 --- a/src/runtime/socket/socket_body.rs +++ b/src/runtime/socket/socket_body.rs @@ -4579,6 +4579,7 @@ pub fn js_upgrade_duplex_to_tls( }); ptr::addr_of_mut!((*duplex_context).upgrade).write(UpgradedDuplex::from( global, + tls_js_value, duplex, UpgradedDuplexHandlers { // SAFETY: `c` is `ctx` below — the live `DuplexUpgradeContext` heap allocation. diff --git a/src/runtime/socket/sockets.classes.ts b/src/runtime/socket/sockets.classes.ts index 834dd71caa67..2a2695e2359c 100644 --- a/src/runtime/socket/sockets.classes.ts +++ b/src/runtime/socket/sockets.classes.ts @@ -8,8 +8,13 @@ function generate(ssl) { configurable: false, memoryCost: true, // Visited slot holding the shared JSSocketHandlers cell, so the callbacks - // stay alive as long as any socket that can still fire them. - values: ["handlers"], + // stay alive as long as any socket that can still fire them. The duplex* + // slots carry the origin stream and the four native listener thunks for a + // TLSSocket driven by an upgraded Duplex (UpgradedDuplex); plain TCP + // sockets never populate them. + values: ssl + ? ["handlers", "duplexOrigin", "duplexOnData", "duplexOnEnd", "duplexOnWritable", "duplexOnClose"] + : ["handlers"], proto: { getAuthorizationError: { fn: "getAuthorizationError", diff --git a/test/js/bun/net/socket-retention.test.ts b/test/js/bun/net/socket-retention.test.ts index 18008d9877b2..702a6ea188e2 100644 --- a/test/js/bun/net/socket-retention.test.ts +++ b/test/js/bun/net/socket-retention.test.ts @@ -189,6 +189,72 @@ test.skipIf(isWindows)("upgradeTLS raw + tls wrappers are both collectable after expect(count).toBeLessThanOrEqual(baseline + 2); }); +test("tls.connect over a Duplex roots the origin and listener thunks through the wrapper, not as Strong handles", async () => { + // UpgradedDuplex keeps the origin stream and its four native listener + // thunks reachable via WriteBarrier slots on the JSTLSSocket wrapper. + // Holding each as a separate Strong handle makes every TLS-over-duplex + // connection add five HandleSet entries that live as long as the native + // struct, independent of the wrapper's GC lifetime. + // + // heapStats().protectedObjectTypeCounts reports HandleSet-rooted values by + // class, so with Strong handles N live upgrades contribute +4N Function and + // +N origin objects; with visited slots they contribute none (the + // wrapper's own self-reference is the only per-upgrade Strong). + await using proc = Bun.spawn({ + cmd: [ + bunExe(), + "-e", + ` + const tls = require("node:tls"); + const { Duplex } = require("node:stream"); + const { heapStats } = require("bun:jsc"); + + const protectedCounts = () => { + const c = heapStats().protectedObjectTypeCounts; + return { Function: c.Function ?? 0, Object: c.Object ?? 0 }; + }; + + // Warm up so lazy module state doesn't skew the delta. + const warm = tls.connect({ + socket: new Duplex({ read() {}, write(_c, _e, cb) { cb(); } }), + rejectUnauthorized: false, + }); + + Bun.gc(true); + const before = protectedCounts(); + + const N = 20; + const live = []; + for (let i = 0; i < N; i++) { + const origin = new Duplex({ read() {}, write(_c, _e, cb) { cb(); } }); + live.push(tls.connect({ socket: origin, rejectUnauthorized: false })); + } + + Bun.gc(true); + const after = protectedCounts(); + + console.log(JSON.stringify({ + N, + fn: after.Function - before.Function, + obj: after.Object - before.Object, + })); + process.exit(0); + `, + ], + env: bunEnv, + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect(stderr).toBe(""); + const { N, fn, obj } = JSON.parse(stdout.trim()); + // Without the visited slots these are fn≈4N and obj≈N. A small constant + // slack absorbs any unrelated Strong created during the loop. + expect(fn).toBeLessThan(N); + expect(obj).toBeLessThan(N); + expect(exitCode).toBe(0); +}); + test("node:net reconnect after connectError does not accumulate wrappers", async () => { // node:net reuses the same native socket across reconnects. With JSRef, // handleConnectError downgrades the wrapper (instead of clearing the raw From 7cc94c3815f31b6cc98f58f249913fe8622752ef Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Sun, 19 Jul 2026 01:33:33 +0000 Subject: [PATCH 2/3] tls: neuter the duplex listener thunks before handle_connect_error downgrades the wrapper In the pre-open error path and the StartTLS failure path, handle_connect_error re-enters JS and downgrades the wrapper's strong self-reference before DuplexUpgradeContext::deinit reaches Drop's teardown. Neuter the thunks (and clear the shadows) while the wrapper is still strong so the later idempotent teardown never reads a shadow whose backing cell could have been collected. --- src/runtime/socket/UpgradedDuplex.rs | 6 ++++-- src/runtime/socket/socket_body.rs | 12 ++++++++++++ 2 files changed, 16 insertions(+), 2 deletions(-) diff --git a/src/runtime/socket/UpgradedDuplex.rs b/src/runtime/socket/UpgradedDuplex.rs index 1c3c7012bddb..d4d76ba4a8b6 100644 --- a/src/runtime/socket/UpgradedDuplex.rs +++ b/src/runtime/socket/UpgradedDuplex.rs @@ -522,8 +522,10 @@ impl UpgradedDuplex { } /// Side-effecting teardown shared by `on_close` (early) and `Drop` (final). - /// Idempotent — resets to empty state. Not the public API; callers drop the struct. - fn teardown(&mut self) { + /// Idempotent: resets to empty state. Also invoked by + /// `DuplexUpgradeContext`'s connect-error branches so the listener thunks + /// are neutered while the wrapper is still strongly reachable. + pub(super) fn teardown(&mut self) { bun_output::scoped_log!(UpgradedDuplex, "deinit"); // clear the timer self.set_timeout(0); diff --git a/src/runtime/socket/socket_body.rs b/src/runtime/socket/socket_body.rs index 8e9557b391fb..a03b231f8aec 100644 --- a/src/runtime/socket/socket_body.rs +++ b/src/runtime/socket/socket_body.rs @@ -4197,6 +4197,12 @@ impl DuplexUpgradeContext { // the owner's +1 we hold. Do NOT let `IntrusiveRc::Drop` // fire on top of that (over-deref → UAF on the JS wrapper's // pointee). + // + // Neuter the JS listener thunks now, while the wrapper is still + // strongly held, so the later `StartTLS` → `deinit` → `Drop` + // teardown (after `handle_connect_error` downgrades it) is a + // no-op on already-cleared shadows. + self.upgrade.teardown(); let p = tls.into_this_ptr(); let _ = TLSSocket::handle_connect_error(p, sys::SystemErrno::ECONNREFUSED as c_int, 0); @@ -4288,6 +4294,12 @@ impl DuplexUpgradeContext { // `start_tls()` was queued), so `needs_deref = // !is_detached()` is true — and detaches. Null // `this.tls` so `deinit` doesn't deref again. + // + // Neuter the JS listener thunks now, while the wrapper + // is still strongly held, so the `deinit` → `Drop` + // teardown below is a no-op on already-cleared shadows. + // SAFETY: `this` is live; `&mut` ends before `deinit`. + unsafe { (*this).upgrade.teardown() }; let p = tls.into_this_ptr(); let _ = TLSSocket::handle_connect_error(p, errno, 0); } From 99107ff32dd7bc7fe9ff849aeb271efb6229277c Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Sun, 19 Jul 2026 02:31:30 +0000 Subject: [PATCH 3/3] test: drop exact stderr assertion to match sibling subprocess tests --- test/js/bun/net/socket-retention.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/js/bun/net/socket-retention.test.ts b/test/js/bun/net/socket-retention.test.ts index 702a6ea188e2..d6a0eb24f7a4 100644 --- a/test/js/bun/net/socket-retention.test.ts +++ b/test/js/bun/net/socket-retention.test.ts @@ -246,13 +246,13 @@ test("tls.connect over a Duplex roots the origin and listener thunks through the stderr: "pipe", }); const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); - expect(stderr).toBe(""); const { N, fn, obj } = JSON.parse(stdout.trim()); // Without the visited slots these are fn≈4N and obj≈N. A small constant // slack absorbs any unrelated Strong created during the loop. expect(fn).toBeLessThan(N); expect(obj).toBeLessThan(N); expect(exitCode).toBe(0); + void stderr; }); test("node:net reconnect after connectError does not accumulate wrappers", async () => {