diff --git a/src/runtime/socket/UpgradedDuplex.rs b/src/runtime/socket/UpgradedDuplex.rs index 3d0dedfe145a..d4d76ba4a8b6 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, @@ -496,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); @@ -518,22 +546,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 +583,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..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); } @@ -4579,6 +4591,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..d6a0eb24f7a4 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]); + 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 () => { // node:net reuses the same native socket across reconnects. With JSRef, // handleConnectError downgrades the wrapper (instead of clearing the raw