tls: root the duplex origin and listener thunks through the wrapper instead of as Strong handles

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<WrapperType>,
-    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<GlobalRef>,
     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::<c_void>();
+        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<JSVa
         let this = unsafe { bun_ptr::callback_ctx::<UpgradedDuplex>(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);
                 }

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.

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",

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