diff --git a/Cargo.lock b/Cargo.lock index cd07035fb068..4f88b2de6b33 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -285,7 +285,6 @@ dependencies = [ "enum-map", "enumset", "libc", - "scopeguard", "strum", ] diff --git a/src/boringssl/Cargo.toml b/src/boringssl/Cargo.toml index b10ae7c63c8b..3d18700f9d3e 100644 --- a/src/boringssl/Cargo.toml +++ b/src/boringssl/Cargo.toml @@ -12,7 +12,6 @@ workspace = true [dependencies] strum.workspace = true bstr.workspace = true -scopeguard.workspace = true const_format.workspace = true enum-map.workspace = true enumset.workspace = true diff --git a/src/boringssl/lib.rs b/src/boringssl/lib.rs index 65852405cf71..e3f6239b63e1 100644 --- a/src/boringssl/lib.rs +++ b/src/boringssl/lib.rs @@ -353,46 +353,37 @@ pub fn check_x509_server_identity(x509: &mut boring::X509, hostname: &[u8]) -> b None }; - let names_ = boring::X509V3_EXT_d2i(ext); - if !names_.is_null() { - let names = names_.cast::(); - let _guard = scopeguard::guard(names, |n| { - boring::sk_GENERAL_NAME_pop_free(n, boring::sk_GENERAL_NAME_free) - }); - for i in 0..boring::sk_GENERAL_NAME_num(names) { - let r#gen = boring::sk_GENERAL_NAME_value(names, i); - if let Some(name) = r#gen.as_ref() { - match name.name_type { - boring::GEN_URI => { - has_identifier_san = true; - } - boring::GEN_DNS => { - has_identifier_san = true; - if !host_is_ip { - let dns_name = &*name.d.dNSName; - let dns_name_slice = core::slice::from_raw_parts( - dns_name.data, - usize::try_from(dns_name.length).expect("int cast"), - ); - if match_dns_name(dns_name_slice, hostname) { - return true; - } + if let Some(names) = boring::GeneralNames::from_raw(boring::X509V3_EXT_d2i(ext)) { + for name in names.iter() { + match name.name_type { + boring::GEN_URI => { + has_identifier_san = true; + } + boring::GEN_DNS => { + has_identifier_san = true; + if !host_is_ip { + let dns_name = &*name.d.dNSName; + let dns_name_slice = core::slice::from_raw_parts( + dns_name.data, + usize::try_from(dns_name.length).expect("int cast"), + ); + if match_dns_name(dns_name_slice, hostname) { + return true; } } - boring::GEN_IPADD => { - has_identifier_san = true; - if let Some(hip) = host_ip { - if let Some(cert_ip) = - ip2_string(&*name.d.ip, &mut cert_ip_buf) - { - if hip == cert_ip { - return true; - } + } + boring::GEN_IPADD => { + has_identifier_san = true; + if let Some(hip) = host_ip { + if let Some(cert_ip) = ip2_string(&*name.d.ip, &mut cert_ip_buf) + { + if hip == cert_ip { + return true; } } } - _ => {} } + _ => {} } } } diff --git a/src/boringssl_sys/boringssl.rs b/src/boringssl_sys/boringssl.rs index c15028aa8c68..b5ce222206a5 100644 --- a/src/boringssl_sys/boringssl.rs +++ b/src/boringssl_sys/boringssl.rs @@ -280,10 +280,111 @@ pub(crate) struct OPENSSL_STACK { pub comp: OPENSSL_sk_cmp_func, } +unsafe extern "C" { + fn GENERAL_NAME_free(name: *mut GENERAL_NAME); +} + +/// Owns one `SSL_CTX` reference; `SSL_CTX_free`s it on drop. Construct from a +/// pointer that already carries a +1 (`SSL_CTX_new`, `SSL_CTX_up_ref`). +pub struct OwnedSslCtx(core::ptr::NonNull); + +impl OwnedSslCtx { + /// Takes the +1 `raw` carries; `None` when `raw` is null. + /// + /// # Safety + /// `raw` must be null or carry a reference the caller is giving up. + pub unsafe fn from_raw(raw: *mut SSL_CTX) -> Option { + core::ptr::NonNull::new(raw).map(Self) + } + + pub fn as_ptr(&self) -> *mut SSL_CTX { + self.0.as_ptr() + } + + /// Transfers the reference back out; the caller must free it. + pub fn into_raw(self) -> *mut SSL_CTX { + core::mem::ManuallyDrop::new(self).0.as_ptr() + } +} + +impl Drop for OwnedSslCtx { + fn drop(&mut self) { + // SAFETY: we own exactly one reference, released once. + unsafe { SSL_CTX_free(self.0.as_ptr()) } + } +} + +/// Owns the `STACK_OF(GENERAL_NAME)` that `X509V3_EXT_d2i` returns for a +/// subjectAltName extension. Frees every `GENERAL_NAME` and then the stack. +pub struct GeneralNames(core::ptr::NonNull); + +impl GeneralNames { + /// Takes ownership of a `STACK_OF(GENERAL_NAME)`; `None` when `raw` is null. + /// + /// # Safety + /// `raw` must be null or a stack the caller owns and does not free itself. + pub unsafe fn from_raw(raw: *mut c_void) -> Option { + core::ptr::NonNull::new(raw.cast::()).map(Self) + } + + pub fn len(&self) -> usize { + // SAFETY: we own a live stack; `sk_num` takes it as `const OPENSSL_STACK`. + unsafe { sk_num(self.0.as_ptr().cast::()) } + } + + pub fn is_empty(&self) -> bool { + self.len() == 0 + } + + /// Borrows the `i`th entry; `None` past the end. + pub fn get(&self, i: usize) -> Option<&GENERAL_NAME> { + if i >= self.len() { + return None; + } + // SAFETY: `i` is in bounds and the stack outlives the borrow, which is + // tied to `&self`. BoringSSL owns the element until our `Drop`. + unsafe { + sk_value(self.0.as_ptr().cast::(), i) + .cast::() + .as_ref() + } + } + + pub fn iter(&self) -> impl Iterator { + (0..self.len()).filter_map(|i| self.get(i)) + } +} + +impl Drop for GeneralNames { + fn drop(&mut self) { + // SAFETY: `sk_pop_free_ex` invokes the callback once per element, so it + // gets `GENERAL_NAME_free` (per element), not a stack free. + unsafe { + sk_pop_free_ex( + self.0.as_ptr().cast::(), + Some(call_general_name_free), + Some(core::mem::transmute::< + unsafe extern "C" fn(*mut GENERAL_NAME), + unsafe extern "C" fn(*mut c_void), + >(GENERAL_NAME_free)), + ) + } + } +} + +/// Restores the element type erased through `OPENSSL_sk_free_func`. +unsafe extern "C" fn call_general_name_free(free_func: OPENSSL_sk_free_func, ptr: *mut c_void) { + // SAFETY: `free_func` is `GENERAL_NAME_free` erased in `Drop` above; both + // sides are `extern "C" fn(*mut _)`, so the round-trip is ABI-sound. + let f: unsafe extern "C" fn(*mut GENERAL_NAME) = + unsafe { core::mem::transmute(free_func.expect("non-null free_func")) }; + // SAFETY: `ptr` is an element `sk_pop_free_ex` is draining from the stack. + unsafe { f(ptr.cast::()) } +} + unsafe extern "C" { fn sk_num(sk: *const OPENSSL_STACK) -> usize; fn sk_value(sk: *const OPENSSL_STACK, i: usize) -> *mut c_void; - fn sk_free(sk: *mut OPENSSL_STACK); fn sk_pop_free_ex( sk: *mut OPENSSL_STACK, call_free_func: OPENSSL_sk_call_free_func, @@ -439,9 +540,6 @@ unsafe extern "C" { // symbol — they bottom out on the untyped `sk_*` ABI above. // ═══════════════════════════════════════════════════════════════════════════ -/// Per-stack free callback type used by `sk_GENERAL_NAME_pop_free`. -pub(crate) type sk_GENERAL_NAME_free_func = unsafe extern "C" fn(*mut struct_stack_st_GENERAL_NAME); - #[inline] pub unsafe fn sk_X509_value(sk: *const struct_stack_st_X509, i: usize) -> *mut X509 { // SAFETY: Two independent type casts, not a const→mut provenance laundering: @@ -452,73 +550,6 @@ pub unsafe fn sk_X509_value(sk: *const struct_stack_st_X509, i: usize) -> *mut X unsafe { sk_value(sk.cast::(), i).cast::() } } -#[inline] -pub unsafe fn sk_GENERAL_NAME_num(sk: *const struct_stack_st_GENERAL_NAME) -> usize { - // SAFETY: const→const cast between opaque aliases — `STACK_OF(GENERAL_NAME)` - // is the same C object as `OPENSSL_STACK`. Caller's `unsafe` contract - // guarantees `sk` is NULL or a live BoringSSL stack; `sk_num` accepts both. - unsafe { sk_num(sk.cast::()) } -} - -#[inline] -pub unsafe fn sk_GENERAL_NAME_value( - sk: *const struct_stack_st_GENERAL_NAME, - i: usize, -) -> *mut GENERAL_NAME { - // SAFETY: `sk` cast is const→const between opaque stack types; the `*mut` - // return is narrowed from `sk_value`'s own `*mut c_void` result (C-heap - // provenance), not derived from `sk`. No const→mut on a single value. - unsafe { sk_value(sk.cast::(), i).cast::() } -} - -#[inline] -pub unsafe extern "C" fn sk_GENERAL_NAME_free(sk: *mut struct_stack_st_GENERAL_NAME) { - // SAFETY: mut→mut cast between opaque aliases of the same allocation. - // Caller's `unsafe` contract guarantees `sk` is NULL or an owned - // BoringSSL stack; `sk_free` is documented to accept both. - unsafe { sk_free(sk.cast::()) } -} - -unsafe extern "C" fn sk_GENERAL_NAME_call_free_func( - free_func: OPENSSL_sk_free_func, - ptr: *mut c_void, -) { - // SAFETY: `free_func` was originally an `sk_GENERAL_NAME_free_func` erased - // through `OPENSSL_sk_free_func` by `sk_GENERAL_NAME_pop_free` below; both - // are `extern "C" fn(*mut _)` so the pointer round-trip is ABI-sound. - let f: sk_GENERAL_NAME_free_func = unsafe { - core::mem::transmute::( - free_func.expect("non-null free_func"), - ) - }; - // SAFETY: `ptr` is an element handed to this trampoline by `sk_pop_free_ex` - // while draining the `STACK_OF(GENERAL_NAME)` passed in below; the cast - // restores the typed pointer `f` was declared to accept before erasure. - unsafe { f(ptr.cast::()) } -} - -#[inline] -pub unsafe fn sk_GENERAL_NAME_pop_free( - sk: *mut struct_stack_st_GENERAL_NAME, - free_func: sk_GENERAL_NAME_free_func, -) { - // SAFETY: `sk` cast is mut→mut between opaque aliases; caller guarantees it - // is NULL or an owned `STACK_OF(GENERAL_NAME)`. The transmute erases - // `free_func`'s typed arg to `*mut c_void` — both sides are - // `extern "C" fn(*mut _)` so the fn-pointer reinterpret is ABI-sound, and - // `sk_GENERAL_NAME_call_free_func` restores the type before invoking it. - unsafe { - sk_pop_free_ex( - sk.cast::(), - Some(sk_GENERAL_NAME_call_free_func), - Some(core::mem::transmute::< - sk_GENERAL_NAME_free_func, - unsafe extern "C" fn(*mut c_void), - >(free_func)), - ) - } -} - // ═══════════════════════════════════════════════════════════════════════════ // SSL / TLS — error codes, verify modes, shutdown flags, renegotiate modes // (`vendor/boringssl/include/openssl/ssl.h`) diff --git a/src/http_jsc/websocket_client.rs b/src/http_jsc/websocket_client.rs index 6a445aff62b7..b5c43240de97 100644 --- a/src/http_jsc/websocket_client.rs +++ b/src/http_jsc/websocket_client.rs @@ -543,22 +543,14 @@ impl WebSocket { self.message_is_compressed.set(false); } - // takes a raw `*mut Self` instead of `&self` because + // Takes `ThisPtr` instead of `&self` because // `handle_without_deinit()` re-enters this very function on the same // allocation through its own raw back-pointer. // // There is no `socket` parameter: the dispatch thunk wraps the same // `us_socket_t*` that `adopt_group` stored into `self.tcp`, so the parse // loop reads `self.tcp` directly. - // - /// # Safety - /// `this_ptr` must point to a live `WebSocket` allocated via - /// `heap::alloc` (see `init` / `init_with_tunnel`); no `&`/`&mut` - /// borrow of `*this_ptr` may be live across this call. - pub unsafe fn handle_data(this_ptr: *mut Self, data_: &[u8]) { - // SAFETY: caller contract — `this_ptr` is a live `heap::alloc` pointer - // with no outstanding `&`/`&mut` borrow (uWS dispatches from userdata). - let this = unsafe { ThisPtr::new(this_ptr) }; + pub fn handle_data(this: ThisPtr, data_: &[u8]) { // after receiving close we should ignore the data if this.close_received.get() { return; @@ -575,7 +567,7 @@ impl WebSocket { // We do not free the memory here since the lifetime is managed by the microtask queue (it should free when called from there) // SAFETY: `initial_handler` is valid (managed by microtask queue). // `handle_without_deinit` re-enters `Self::handle_data` via the - // `adopted` raw ptr (same `heap::alloc` provenance as `this_ptr`). + // `adopted` raw ptr (same `heap::alloc` provenance as `this`). unsafe { (*initial_handler.as_ptr()).handle_without_deinit() }; // handle_without_deinit is supposed to clear the handler from WebSocket* @@ -1708,8 +1700,9 @@ impl WebSocket { pub unsafe fn handle_tunnel_data(this_ptr: *mut Self, data: &[u8]) { // Process the decrypted data as if it came from the socket // has_tcp() now returns true for tunnel mode, so this will work correctly - // SAFETY: forwarded — see `handle_data`'s contract. - unsafe { Self::handle_data(this_ptr, data) }; + // SAFETY: caller contract — `this_ptr` is a live `heap::alloc` pointer + // with no outstanding `&`/`&mut` borrow. + Self::handle_data(unsafe { ThisPtr::new(this_ptr) }, data); } /// Called by the WebSocketProxyTunnel when the underlying socket drains. @@ -1984,10 +1977,10 @@ impl InitialDataHandler { unsafe { !(*ws_ptr).tcp.get().is_closed() || (*ws_ptr).proxy_tunnel.get().is_some() }; // SAFETY: `ws_ptr` is live; raw read of a `Copy` field. if unsafe { (*ws_ptr).outgoing_websocket.get().is_some() } && is_connected { - // SAFETY: `ws_ptr` carries `heap::alloc` provenance; `handle_data` - // takes `*mut Self` and forms its own scoped `&mut` internally. No + // SAFETY: `ws_ptr` carries `heap::alloc` provenance and is live; no // borrow of `*ws_ptr` is live in this frame across the call. - unsafe { WebSocket::::handle_data(ws_ptr, &self.slice) }; + let ws = unsafe { ThisPtr::new(ws_ptr) }; + WebSocket::::handle_data(ws, &self.slice); } } diff --git a/src/http_jsc/websocket_client/WebSocketUpgradeClient.rs b/src/http_jsc/websocket_client/WebSocketUpgradeClient.rs index 8cdbbf6bbf76..e7c63e78ba66 100644 --- a/src/http_jsc/websocket_client/WebSocketUpgradeClient.rs +++ b/src/http_jsc/websocket_client/WebSocketUpgradeClient.rs @@ -162,11 +162,11 @@ pub struct HTTPClient { // `bun_runtime::socket::uws_handlers`, which forwards to the `pub // handle_*` methods below. // -// The handlers take `*mut Self` (not `&mut Self`) because uSockets dispatches -// them from the raw userdata pointer and several of them can free `Self` (via -// `deref` reaching zero) or be re-entered synchronously by `tcp.close()` / -// C++ callbacks. Holding a `&mut Self` function-argument across either of -// those is UB under Stacked Borrows (argument protectors / aliased `&mut`). +// The handlers take `ThisPtr` (not `&mut Self`) because uSockets +// dispatches them from the raw userdata pointer and several can free `Self` +// (`deref` reaching zero) or be re-entered synchronously by `tcp.close()` / +// C++ callbacks; a `&mut Self` argument across either is UB under Stacked +// Borrows (argument protectors / aliased `&mut`). impl HTTPClient { const TYPE_NAME: &'static str = if SSL { "http.websocket_client.WebSocketUpgradeClient.NewHTTPUpgradeClient(true)" @@ -692,23 +692,21 @@ impl HTTPClient { } } - /// # Safety - /// `this` must point to a live `Self`. Takes `*mut Self` because the - /// trailing `deref` releases the socket ref and on the normal path frees - /// `this`; a `&mut self` argument would carry a Stacked Borrows protector - /// that makes deallocating its referent UB. - pub unsafe fn handle_close(this: *mut Self, _: Socket, _: c_int, _: *mut c_void) { + /// Takes `ThisPtr` because the trailing `deref` releases the socket + /// ref and on the normal path frees `this`; a `&mut self` argument would + /// carry a Stacked Borrows protector that makes deallocating it UB. + pub fn handle_close(this: ThisPtr, _: Socket, _: c_int, _: *mut c_void) { log!("onClose"); bun_jsc::mark_binding!(); // SAFETY: short-lived `&mut` borrows; each ends before the next call. - unsafe { (*this).clear_data() }; - // SAFETY: short-lived `&mut` for the field detach; `this` is live per caller contract. - unsafe { (*this).tcp.detach() }; + unsafe { (*this.as_ptr()).clear_data() }; + // SAFETY: short-lived `&mut` for the field detach; `this` is live. + unsafe { (*this.as_ptr()).tcp.detach() }; // SAFETY: forwards `this` with root provenance; no `&mut Self` is live. - unsafe { Self::dispatch_abrupt_close(this, ErrorCode::Ended) }; + unsafe { Self::dispatch_abrupt_close(this.as_ptr(), ErrorCode::Ended) }; // SAFETY: may free `this`; no `&mut Self` is live. - unsafe { Self::deref(this) }; + unsafe { Self::deref(this.as_ptr()) }; } /// # Safety @@ -719,11 +717,9 @@ impl HTTPClient { // We cannot access the pointer after fail is called. } - /// # Safety - /// `this` must point to a live `Self`. Takes `*mut Self` because `fail` - /// may free `this` / be re-entered; see `fail`. - pub unsafe fn handle_handshake( - this: *mut Self, + /// Takes `ThisPtr` because `fail` may free `this` / be re-entered. + pub fn handle_handshake( + this: ThisPtr, socket: Socket, success: i32, ssl_error: uws::us_bun_verify_error_t, @@ -734,9 +730,6 @@ impl HTTPClient { ssl_error.error_no ); - // SAFETY: caller (uWS dispatch) — `this` is a live `heap::alloc` - // pointer recovered from userdata; no Rust borrow is live. - let this = unsafe { ThisPtr::new(this) }; let handshake_success = success == 1; let mut reject_unauthorized = false; if let Some(ws) = this.outgoing_websocket { @@ -799,13 +792,11 @@ impl HTTPClient { } } - /// # Safety - /// `this` must point to a live `Self`. Takes `*mut Self` because - /// `terminate` may free `this`; see `fail`. - pub unsafe fn handle_open(this: *mut Self, socket: Socket) { + /// Takes `ThisPtr` because `terminate` may free `this`; see `fail`. + pub fn handle_open(this: ThisPtr, socket: Socket) { log!("onOpen"); // SAFETY: short-lived `&mut` for setup; ends before any reentrant call. - let me = unsafe { &mut *this }; + let me = unsafe { &mut *this.as_ptr() }; me.tcp = socket; debug_assert!(!me.input_body_buf.is_empty()); @@ -843,7 +834,7 @@ impl HTTPClient { let wrote = socket.write(&me.input_body_buf); if wrote < 0 { // SAFETY: no `&mut Self` is live across this call (`me`'s last use is above). - unsafe { Self::terminate(this, ErrorCode::FailedToWrite) }; + unsafe { Self::terminate(this.as_ptr(), ErrorCode::FailedToWrite) }; return; } @@ -855,16 +846,11 @@ impl HTTPClient { socket.get_native_handle() == self.tcp.get_native_handle() } - /// # Safety - /// `this` must point to a live `Self`. Takes `*mut Self` because - /// `socket.close()` synchronously dispatches `handle_close` (aliased - /// `&mut`), and `terminate`/`process_response`/the trailing `deref` may - /// free `this` (argument-protector UB on `&mut self`). - pub unsafe fn handle_data(this: *mut Self, socket: Socket, data: &[u8]) { + /// Takes `ThisPtr` because `socket.close()` synchronously dispatches + /// `handle_close` (aliased `&mut`), and `terminate`/`process_response`/the + /// trailing `deref` may free `this` (argument-protector UB on `&mut self`). + pub fn handle_data(this: ThisPtr, socket: Socket, data: &[u8]) { log!("onData"); - // SAFETY: caller (uWS dispatch) — `this` is a live `heap::alloc` - // pointer recovered from userdata; no Rust borrow is live. - let this = unsafe { ThisPtr::new(this) }; // For tunnel mode after successful upgrade, forward all data to the tunnel // The tunnel will decrypt and pass to the WebSocket client @@ -904,8 +890,7 @@ impl HTTPClient { // Handle proxy handshake response if this.state == State::ProxyHandshake { - // SAFETY: forwards `this` with root provenance; no `&mut Self` is live. - unsafe { Self::handle_proxy_response(this.as_ptr(), socket, data) }; + Self::handle_proxy_response(this, socket, data); return; } @@ -973,15 +958,13 @@ impl HTTPClient { // `_guard` drops here, balancing the ref above. May free `this`. } - /// # Safety - /// `this` must point to a live `Self`. Takes `*mut Self` because - /// `terminate`/`handle_data` may free `this`; see `fail`. - unsafe fn handle_proxy_response(this: *mut Self, socket: Socket, data: &[u8]) { + /// Takes `ThisPtr` because `terminate`/`handle_data` may free `this`. + fn handle_proxy_response(this: ThisPtr, socket: Socket, data: &[u8]) { log!("handleProxyResponse"); // SAFETY: short-lived `&mut` for body buffering; no reentrant calls in // this region until `terminate` below. - let me = unsafe { &mut *this }; + let me = unsafe { &mut *this.as_ptr() }; let mut body = data; if !me.body.is_empty() { me.body.extend_from_slice(data); @@ -996,7 +979,7 @@ impl HTTPClient { if !body.starts_with(HTTP_200) && !body.starts_with(HTTP_200_ALT) { // Proxy connection failed // SAFETY: `me`'s last use is above; no `&mut Self` spans this call. - unsafe { Self::terminate(this, ErrorCode::ProxyConnectFailed) }; + unsafe { Self::terminate(this.as_ptr(), ErrorCode::ProxyConnectFailed) }; return; } } @@ -1006,7 +989,7 @@ impl HTTPClient { Ok(r) => r, Err(picohttp::ParseResponseError::MalformedHttpResponse) => { // SAFETY: `me`'s last use is above; no `&mut Self` spans this call. - unsafe { Self::terminate(this, ErrorCode::InvalidResponse) }; + unsafe { Self::terminate(this.as_ptr(), ErrorCode::InvalidResponse) }; return; } Err(picohttp::ParseResponseError::ShortRead) => { @@ -1018,7 +1001,7 @@ impl HTTPClient { // total bytes received. if me.body.len() > bun_http::max_http_header_size() { // SAFETY: `me`'s last use is above; no `&mut Self` spans this call. - unsafe { Self::terminate(this, ErrorCode::InvalidResponse) }; + unsafe { Self::terminate(this.as_ptr(), ErrorCode::InvalidResponse) }; } return; } @@ -1028,10 +1011,10 @@ impl HTTPClient { if response.status_code != 200 { if response.status_code == 407 { // SAFETY: `me`'s last use is above; no `&mut Self` spans this call. - unsafe { Self::terminate(this, ErrorCode::ProxyAuthenticationRequired) }; + unsafe { Self::terminate(this.as_ptr(), ErrorCode::ProxyAuthenticationRequired) }; } else { // SAFETY: `me`'s last use is above; no `&mut Self` spans this call. - unsafe { Self::terminate(this, ErrorCode::ProxyConnectFailed) }; + unsafe { Self::terminate(this.as_ptr(), ErrorCode::ProxyConnectFailed) }; } return; } @@ -1044,7 +1027,7 @@ impl HTTPClient { let remain_buf: Vec = body[bytes_read..].to_vec(); // SAFETY: re-derive a fresh `&mut` after the `body` borrow above. - let me = unsafe { &mut *this }; + let me = unsafe { &mut *this.as_ptr() }; // Clear the body buffer for WebSocket handshake me.body.clear(); @@ -1052,14 +1035,14 @@ impl HTTPClient { // Safely unwrap proxy state - it must exist if we're in proxy_handshake state let Some(p) = &mut me.proxy else { // SAFETY: `me`'s last use is above; no `&mut Self` spans this call. - unsafe { Self::terminate(this, ErrorCode::ProxyTunnelFailed) }; + unsafe { Self::terminate(this.as_ptr(), ErrorCode::ProxyTunnelFailed) }; return; }; // For wss:// through proxy, we need to do TLS handshake inside the tunnel if p.is_target_https() { // SAFETY: `me`/`p` last used above; forwards `this` with root provenance. - unsafe { Self::start_proxy_tls_handshake(this, socket, &remain_buf) }; + unsafe { Self::start_proxy_tls_handshake(this.as_ptr(), socket, &remain_buf) }; return; } @@ -1075,7 +1058,7 @@ impl HTTPClient { let wrote = socket.write(&me.input_body_buf); if wrote < 0 { // SAFETY: `me`'s last use is above; no `&mut Self` spans this call. - unsafe { Self::terminate(this, ErrorCode::FailedToWrite) }; + unsafe { Self::terminate(this.as_ptr(), ErrorCode::FailedToWrite) }; return; } @@ -1083,8 +1066,7 @@ impl HTTPClient { // If there's remaining data after the proxy response, process it if !remain_buf.is_empty() { - // SAFETY: `me`'s last use is above; forwards `this` with root provenance. - unsafe { Self::handle_data(this, socket, &remain_buf) }; + Self::handle_data(this, socket, &remain_buf); } } @@ -1275,13 +1257,11 @@ impl HTTPClient { unsafe { Self::process_response(this, response, &remain_buf) }; } - /// # Safety - /// `this` must point to a live `Self`. Takes `*mut Self` because - /// `terminate` may free `this`; see `fail`. - pub unsafe fn handle_end(this: *mut Self, _: Socket) { + /// Takes `ThisPtr` because `terminate` may free `this`; see `fail`. + pub fn handle_end(this: ThisPtr, _: Socket) { log!("onEnd"); // SAFETY: forwards `this` with root provenance; no `&mut Self` is live. - unsafe { Self::terminate(this, ErrorCode::Ended) }; + unsafe { Self::terminate(this.as_ptr(), ErrorCode::Ended) }; } /// # Safety @@ -1720,13 +1700,9 @@ impl HTTPClient { cost } - /// # Safety - /// `this` must point to a live `Self`. Takes `*mut Self` because - /// `terminate` and the trailing `deref` may free `this`; see `fail`. - pub unsafe fn handle_writable(this: *mut Self, socket: Socket) { - // SAFETY: caller (uWS dispatch) — `this` is a live `heap::alloc` - // pointer recovered from userdata; no Rust borrow is live. - let this = unsafe { ThisPtr::new(this) }; + /// Takes `ThisPtr` because `terminate` and the trailing `deref` may + /// free `this`; see `fail`. + pub fn handle_writable(this: ThisPtr, socket: Socket) { debug_assert!(this.is_same_socket(socket)); // Forward to proxy tunnel if active @@ -1788,26 +1764,19 @@ impl HTTPClient { } } - /// # Safety - /// `this` must point to a live `Self`. Takes `*mut Self` because - /// `terminate` may free `this`; see `fail`. - pub unsafe fn handle_timeout(this: *mut Self, _: Socket) { + /// Takes `ThisPtr` because `terminate` may free `this`; see `fail`. + pub fn handle_timeout(this: ThisPtr, _: Socket) { // SAFETY: forwards `this` with root provenance; no `&mut Self` is live. - unsafe { Self::terminate(this, ErrorCode::Timeout) }; + unsafe { Self::terminate(this.as_ptr(), ErrorCode::Timeout) }; } /// In theory, this could be called immediately. /// In that case, we set `state` to `failed` and return, expecting the parent to call `destroy`. /// - /// # Safety - /// `this` must point to a live `Self`. Takes `*mut Self` because the - /// trailing `deref` releases the socket ref and may free `this`; a - /// `&mut self` argument would carry a Stacked Borrows protector that - /// makes deallocating its referent UB. - pub unsafe fn handle_connect_error(this: *mut Self, _: Socket, _: c_int) { - // SAFETY: caller (uWS dispatch) — `this` is a live `heap::alloc` - // pointer recovered from userdata; no Rust borrow is live. - let this = unsafe { ThisPtr::new(this) }; + /// Takes `ThisPtr` because the trailing `deref` releases the socket + /// ref and may free `this`; a `&mut self` argument would carry a Stacked + /// Borrows protector that makes deallocating its referent UB. + pub fn handle_connect_error(this: ThisPtr, _: Socket, _: c_int) { // SAFETY: short-lived `&mut` for detach; ends before any reentrant call. unsafe { (*this.as_ptr()).tcp.detach() }; diff --git a/src/jsc/SystemError.rs b/src/jsc/SystemError.rs index a8ab99dad8fe..a1567c5b2007 100644 --- a/src/jsc/SystemError.rs +++ b/src/jsc/SystemError.rs @@ -73,7 +73,10 @@ impl SystemError { bun_sys::e_from_negated(self.errno) } - pub fn deref(&self) { + /// Releases one ref of every string field. Prefer letting + /// [`to_error_instance`](Self::to_error_instance) consume the value: this + /// is only for the paths that build no JS error at all. + pub fn deref(self) { self.path.deref(); self.code.deref(); self.message.deref(); @@ -104,8 +107,13 @@ impl SystemError { v } - pub fn to_error_instance(&self, global: &JSGlobalObject) -> JSValue { - let result = SystemError__toErrorInstance(self, global); + /// Converts to a JS `Error`, consuming `self`: each string field's ref is + /// released here, so converting the same `SystemError` twice would free + /// strings the first `Error` still holds. Take `self` by value so the + /// compiler rejects that; call [`dupe`](Self::dupe) when two `Error`s are + /// genuinely wanted. + pub fn to_error_instance(self, global: &JSGlobalObject) -> JSValue { + let result = SystemError__toErrorInstance(&self, global); self.deref(); result } @@ -115,7 +123,7 @@ impl SystemError { /// from native code at the top of the event loop (threadpool callback) to /// reject a promise — otherwise the error will have an empty stack. pub fn to_error_instance_with_async_stack( - &self, + self, global: &JSGlobalObject, promise: &JSPromise, ) -> JSValue { @@ -143,8 +151,8 @@ impl SystemError { /// Before using this function, consider if the Node.js API it is /// implementing follows this convention. It is exclusively used /// to match the error code that `node:os` throws. - pub fn to_error_instance_with_info_object(&self, global: &JSGlobalObject) -> JSValue { - let result = SystemError__toErrorInstanceWithInfoObject(self, global); + pub fn to_error_instance_with_info_object(self, global: &JSGlobalObject) -> JSValue { + let result = SystemError__toErrorInstanceWithInfoObject(&self, global); self.deref(); result } diff --git a/src/jsc/bindings/JSSocketHandlers.cpp b/src/jsc/bindings/JSSocketHandlers.cpp new file mode 100644 index 000000000000..8ac210105262 --- /dev/null +++ b/src/jsc/bindings/JSSocketHandlers.cpp @@ -0,0 +1,104 @@ +#include "root.h" + +#include "JavaScriptCore/JSCJSValueInlines.h" +#include "JSSocketHandlers.h" +#include "ZigGlobalObject.h" +#include +#include +#include +#include "ExtendedDOMClientIsoSubspaces.h" +#include "ExtendedDOMIsoSubspaces.h" +#include "BunClientData.h" + +namespace Bun { + +using namespace JSC; + +const JSC::ClassInfo JSSocketHandlers::s_info = { "SocketHandlers"_s, &Base::s_info, nullptr, nullptr, CREATE_METHOD_TABLE(JSSocketHandlers) }; + +template +JSC::GCClient::IsoSubspace* JSSocketHandlers::subspaceFor(JSC::VM& vm) +{ + return WebCore::subspaceForImpl( + vm, + [](auto& spaces) { return spaces.m_clientSubspaceForJSSocketHandlers.get(); }, + [](auto& spaces, auto&& space) { spaces.m_clientSubspaceForJSSocketHandlers = std::forward(space); }, + [](auto& spaces) { return spaces.m_subspaceForJSSocketHandlers.get(); }, + [](auto& spaces, auto&& space) { spaces.m_subspaceForJSSocketHandlers = std::forward(space); }); +} + +JSC::Structure* JSSocketHandlers::createStructure(JSC::VM& vm, JSC::JSGlobalObject* globalObject, JSC::JSValue prototype) +{ + return Structure::create(vm, globalObject, prototype, TypeInfo(ObjectType, StructureFlags), info()); +} + +JSSocketHandlers::JSSocketHandlers(JSC::VM& vm, JSC::Structure* structure) + : Base(vm, structure) +{ +} + +void JSSocketHandlers::finishCreation(JSC::VM& vm, const JSC::EncodedJSValue* callbacks) +{ + Base::finishCreation(vm); + for (unsigned i = 0; i < numberOfCallbacks; i++) { + JSC::JSValue value = JSC::JSValue::decode(callbacks[i]); + Base::internalField(i).setWithoutWriteBarrier(value.isEmpty() ? jsUndefined() : value); + } + Base::internalField(static_cast(Field::Promise)).setWithoutWriteBarrier(jsUndefined()); +} + +template +void JSSocketHandlers::visitChildrenImpl(JSCell* cell, Visitor& visitor) +{ + auto* thisObject = uncheckedDowncast(cell); + ASSERT_GC_OBJECT_INHERITS(thisObject, info()); + Base::visitChildren(thisObject, visitor); +} + +DEFINE_VISIT_CHILDREN(JSSocketHandlers); + +JSSocketHandlers* JSSocketHandlers::create(JSC::JSGlobalObject* globalObject, const JSC::EncodedJSValue* callbacks) +{ + auto& vm = JSC::getVM(globalObject); + // Resolve the cached structure before allocateCell(): allocating any + // JSCell between it and finishCreation() is not allowed, and the lazily + // initialized structure allocates on first use. + auto* structure = defaultGlobalObject(globalObject)->JSSocketHandlersStructure(); + auto* cell = new (NotNull, allocateCell(vm)) JSSocketHandlers(vm, structure); + cell->finishCreation(vm, callbacks); + return cell; +} + +} // namespace Bun + +extern "C" JSC::EncodedJSValue Bun__SocketHandlers__create(JSC::JSGlobalObject* globalObject, const JSC::EncodedJSValue* callbacks) +{ + return JSC::JSValue::encode(Bun::JSSocketHandlers::create(globalObject, callbacks)); +} + +extern "C" JSC::EncodedJSValue Bun__SocketHandlers__getField(JSC::EncodedJSValue cellValue, uint32_t index) +{ + auto* cell = uncheckedDowncast(JSC::JSValue::decode(cellValue).asCell()); + ASSERT(index < Bun::JSSocketHandlers::numberOfInternalFields); + return JSC::JSValue::encode(cell->internalField(index).get()); +} + +extern "C" void Bun__SocketHandlers__setField(JSC::JSGlobalObject* globalObject, JSC::EncodedJSValue cellValue, uint32_t index, JSC::EncodedJSValue value) +{ + auto& vm = JSC::getVM(globalObject); + auto* cell = uncheckedDowncast(JSC::JSValue::decode(cellValue).asCell()); + ASSERT(index < Bun::JSSocketHandlers::numberOfInternalFields); + JSC::JSValue incoming = JSC::JSValue::decode(value); + cell->internalField(index).set(vm, cell, incoming.isEmpty() ? JSC::jsUndefined() : incoming); +} + +extern "C" void Bun__SocketHandlers__setCallbacks(JSC::JSGlobalObject* globalObject, JSC::EncodedJSValue cellValue, const JSC::EncodedJSValue* callbacks) +{ + auto& vm = JSC::getVM(globalObject); + auto* cell = uncheckedDowncast(JSC::JSValue::decode(cellValue).asCell()); + for (unsigned i = 0; i < Bun::JSSocketHandlers::numberOfCallbacks; i++) { + JSC::JSValue value = JSC::JSValue::decode(callbacks[i]); + cell->internalField(i).setWithoutWriteBarrier(value.isEmpty() ? JSC::jsUndefined() : value); + } + vm.writeBarrier(cell); +} diff --git a/src/jsc/bindings/JSSocketHandlers.h b/src/jsc/bindings/JSSocketHandlers.h new file mode 100644 index 000000000000..558c478a7756 --- /dev/null +++ b/src/jsc/bindings/JSSocketHandlers.h @@ -0,0 +1,57 @@ +#pragma once + +#include "root.h" +#include "headers-handwritten.h" + +#include "JavaScriptCore/JSCInlines.h" +#include "BunClientData.h" +#include + +namespace Bun { +using namespace JSC; + +// The JS callbacks of a Bun.listen / Bun.connect socket context, plus the +// pending connect promise, stored as GC-visited internal fields. The listener's +// and each socket's JS wrapper hold this cell in a visited slot, so the +// callbacks live exactly as long as something that can still invoke them. +// Replaces manual gcProtect/gcUnprotect of raw JSValues, and lets `reload` swap +// callbacks in place for live sockets. +class JSSocketHandlers final : public JSC::JSInternalFieldObjectImpl<14> { +public: + using Base = JSC::JSInternalFieldObjectImpl<14>; + + // Field order is ABI shared with src/runtime/socket/Handlers.rs. + enum class Field : uint32_t { + Open = 0, + Close, + Data, + Writable, + Timeout, + ConnectError, + End, + Error, + Handshake, + Session, + Keylog, + ServerName, + ALPNCallback, + // Not a callback: the `Bun.connect` promise, cleared once settled. + Promise, + }; + static_assert(static_cast(Field::Promise) + 1 == numberOfInternalFields); + + static constexpr unsigned numberOfCallbacks = static_cast(Field::Promise); + + template static JSC::GCClient::IsoSubspace* subspaceFor(JSC::VM& vm); + + static JSSocketHandlers* create(JSC::JSGlobalObject*, const JSC::EncodedJSValue* callbacks); + static JSC::Structure* createStructure(JSC::VM&, JSC::JSGlobalObject*, JSC::JSValue prototype); + + DECLARE_EXPORT_INFO; + DECLARE_VISIT_CHILDREN; + + JSSocketHandlers(JSC::VM&, JSC::Structure*); + void finishCreation(JSC::VM&, const JSC::EncodedJSValue* callbacks); +}; + +} // namespace Bun diff --git a/src/jsc/bindings/ZigGlobalObject.cpp b/src/jsc/bindings/ZigGlobalObject.cpp index 5be04f650868..3b54d1b5dc49 100644 --- a/src/jsc/bindings/ZigGlobalObject.cpp +++ b/src/jsc/bindings/ZigGlobalObject.cpp @@ -115,6 +115,7 @@ #include "JSMessageEvent.h" #include "JSMessagePort.h" #include "JSNextTickQueue.h" +#include "JSSocketHandlers.h" #include "JSPerformance.h" #include "JSPerformanceEntry.h" #include "JSPerformanceMark.h" @@ -2294,6 +2295,11 @@ void GlobalObject::finishCreation(VM& vm) init.set(Bun::PendingVirtualModuleResult::createStructure(init.vm, init.owner, init.owner->objectPrototype())); }); + this->m_JSSocketHandlersStructure.initLater( + [](const Initializer& init) { + init.set(Bun::JSSocketHandlers::createStructure(init.vm, init.owner, JSC::jsNull())); + }); + m_bunObject.initLater( [](const JSC::LazyProperty::Initializer& init) { init.set(Bun::createBunObject(init.vm, init.owner)); diff --git a/src/jsc/bindings/ZigGlobalObject.h b/src/jsc/bindings/ZigGlobalObject.h index fb4271ed1d0f..bc5495da8572 100644 --- a/src/jsc/bindings/ZigGlobalObject.h +++ b/src/jsc/bindings/ZigGlobalObject.h @@ -586,6 +586,7 @@ class GlobalObject : public Bun::GlobalScope { V(private, LazyPropertyOfGlobalObject, m_jsonlParseResultStructure) \ V(private, LazyPropertyOfGlobalObject, m_pathParsedObjectStructure) \ V(private, LazyPropertyOfGlobalObject, m_pendingVirtualModuleResultStructure) \ + V(private, LazyPropertyOfGlobalObject, m_JSSocketHandlersStructure) \ V(private, LazyPropertyOfGlobalObject, m_nativeMicrotaskTrampoline) \ V(private, LazyPropertyOfGlobalObject, m_performMicrotaskVariadicFunction) \ V(private, LazyPropertyOfGlobalObject, m_utilInspectFunction) \ @@ -724,6 +725,7 @@ class GlobalObject : public Bun::GlobalScope { JSC::Structure* jsonlParseResultStructure() { return m_jsonlParseResultStructure.get(this); } JSC::Structure* pathParsedObjectStructure() { return m_pathParsedObjectStructure.get(this); } JSC::Structure* pendingVirtualModuleResultStructure() { return m_pendingVirtualModuleResultStructure.get(this); } + JSC::Structure* JSSocketHandlersStructure() { return m_JSSocketHandlersStructure.get(this); } // We need to know if the napi module registered itself or we registered it. // To do that, we count the number of times we register a module. diff --git a/src/jsc/bindings/webcore/DOMClientIsoSubspaces.h b/src/jsc/bindings/webcore/DOMClientIsoSubspaces.h index e1a3c3b17012..fd53bd416930 100644 --- a/src/jsc/bindings/webcore/DOMClientIsoSubspaces.h +++ b/src/jsc/bindings/webcore/DOMClientIsoSubspaces.h @@ -52,6 +52,7 @@ class DOMClientIsoSubspaces { std::unique_ptr m_clientSubspaceForErrorCodeCache; std::unique_ptr m_clientSubspaceForBunInspectorConnection; std::unique_ptr m_clientSubspaceForJSNextTickQueue; + std::unique_ptr m_clientSubspaceForJSSocketHandlers; std::unique_ptr m_clientSubspaceForNAPIFunction; std::unique_ptr m_clientSubspaceForJSDiffieHellman; std::unique_ptr m_clientSubspaceForJSDiffieHellmanGroup; diff --git a/src/jsc/bindings/webcore/DOMIsoSubspaces.h b/src/jsc/bindings/webcore/DOMIsoSubspaces.h index 91a30452f0f1..bf2a7239d15f 100644 --- a/src/jsc/bindings/webcore/DOMIsoSubspaces.h +++ b/src/jsc/bindings/webcore/DOMIsoSubspaces.h @@ -52,6 +52,7 @@ class DOMIsoSubspaces { std::unique_ptr m_subspaceForErrorCodeCache; std::unique_ptr m_subspaceForBunInspectorConnection; std::unique_ptr m_subspaceForJSNextTickQueue; + std::unique_ptr m_subspaceForJSSocketHandlers; std::unique_ptr m_subspaceForNAPIFunction; std::unique_ptr m_subspaceForTTYWrapObject; std::unique_ptr m_subspaceForNapiHandleScopeImpl; diff --git a/src/ptr/ref_count.rs b/src/ptr/ref_count.rs index 3ea3cc36341d..e8770ed30653 100644 --- a/src/ptr/ref_count.rs +++ b/src/ptr/ref_count.rs @@ -861,6 +861,28 @@ impl RefPtr { unsafe { Self::unchecked_and_unsafe_init(raw_ptr, return_address()) } } + /// A [`ThisPtr`](crate::ThisPtr) to the pointee, for the FFI-shaped call + /// sites that take one. + /// + /// Safe: holding a `RefPtr` means we own a ref, so the pointee is live — + /// which is exactly `ThisPtr::new`'s precondition. The returned handle is + /// only valid while this `RefPtr` (or another ref) is alive. + #[inline] + pub fn this_ptr(&self) -> crate::ThisPtr { + // SAFETY: we own an outstanding ref, so `self.data` is live and non-null. + unsafe { crate::ThisPtr::new(self.data.as_ptr()) } + } + + /// Consume this `RefPtr` into a [`ThisPtr`](crate::ThisPtr), transferring + /// the ref to the callee — the counterpart of `into_raw` for the + /// `ThisPtr`-shaped dispatch entry points. Safe for the same reason + /// `into_raw` is: no ref is released, and the pointee stays live. + #[inline] + pub fn into_this_ptr(self) -> crate::ThisPtr { + // SAFETY: `into_raw` transfers our live ref; the pointee is non-null. + unsafe { crate::ThisPtr::new(self.into_raw()) } + } + /// Wrap a raw pointer whose ref is being transferred to this RefPtr /// WITHOUT incrementing the refcount. The caller gives up their ref; /// this RefPtr now owns it. Unlike `adopt_ref`, this does not assert diff --git a/src/runtime/node/node_net_binding.rs b/src/runtime/node/node_net_binding.rs index 87cf0594e3f8..a6d2da6e17c4 100644 --- a/src/runtime/node/node_net_binding.rs +++ b/src/runtime/node/node_net_binding.rs @@ -140,7 +140,7 @@ pub(crate) fn new_detached_socket(global: &JSGlobalObject, frame: &CallFrame) -> socket: Cell::new(uws::NewSocketHandler::::DETACHED), ref_count: bun_ptr::RefCount::init(), protos: JsCell::new(None), - handlers: Cell::new(None), + handlers: JsCell::new(None), local_binding: JsCell::new(None), // — defaults — owned_ssl_ctx: Cell::new(None), @@ -155,8 +155,7 @@ pub(crate) fn new_detached_socket(global: &JSGlobalObject, frame: &CallFrame) -> native_callback: JsCell::new(NativeCallbacks::None), twin: JsCell::new(None), }); - // SAFETY: `NewSocket::new` returns a live heap pointer (`heap::alloc`). - unsafe { (*socket).get_this_value(global) } + socket.get_this_value(global) } Ok(if !is_ssl { diff --git a/src/runtime/socket/Handlers.rs b/src/runtime/socket/Handlers.rs index f1640c1953a6..fd255093921c 100644 --- a/src/runtime/socket/Handlers.rs +++ b/src/runtime/socket/Handlers.rs @@ -1,4 +1,6 @@ use core::cell::Cell; +use core::ptr::NonNull; +use std::rc::Rc; use bun_core::zig_string::Slice as ZigStringSlice; use bun_jsc::array_buffer::BinaryType; @@ -6,12 +8,13 @@ use bun_jsc::generated::{ SocketConfig as GeneratedSocketConfig, SocketConfigHandlers as GeneratedSocketConfigHandlers, }; use bun_jsc::virtual_machine::VirtualMachine; -use bun_jsc::{GlobalRef, JSGlobalObject, JSValue, JsCell, JsResult, StrongOptional as Strong}; +use bun_jsc::{GlobalRef, JSGlobalObject, JSValue, JsResult, Strong}; use bun_sys::Fd; use bun_uws as uws; use super::Listener as SocketListener; use super::SocketMode; +use super::js_socket_handlers::{CALLBACK_COUNT, JSSocketHandlers}; use super::listener::ListenerType; use super::{SSLConfig, SSLConfigFromJs}; @@ -25,103 +28,170 @@ unsafe extern "C" { bun_output::declare_scope!(Listener, visible); +/// The callbacks and lifecycle bookkeeping shared by a listener and every +/// socket it accepts, or by one `Bun.connect` socket and its reconnects. +/// +/// Held as `Rc` by each owner (the `Listener`, each `NewSocket`, and +/// each in-flight callback [`Scope`]), so a socket that closes while a callback +/// frame still holds it cannot free it out from under that frame. pub struct Handlers { - pub on_open: JSValue, - pub on_close: JSValue, - pub on_data: JSValue, - pub on_writable: JSValue, - pub on_timeout: JSValue, - pub on_connect_error: JSValue, - pub on_end: JSValue, - pub on_error: JSValue, - pub on_handshake: JSValue, - pub on_session: JSValue, - pub on_keylog: JSValue, - pub on_server_name: JSValue, - pub on_alpn_callback: JSValue, + /// The cell holding every callback and the pending connect promise. Read + /// via the named accessors ([`on_data`](Self::on_data), ...); `reload` + /// rewrites it in place via [`apply_reload`](Self::apply_reload). + /// + /// See [`JSSocketHandlers`] for what keeps it alive; entry points that + /// build a `Handlers` hold a [`root_cell`](Self::root_cell) handle until + /// the first JS wrapper stores it. + cell: JSSocketHandlers, - pub binary_type: BinaryType, + pub binary_type: Cell, pub vm: &'static VirtualMachine, pub global_object: GlobalRef, - /// `Cell` so [`mark_active`](Self::mark_active) / - /// [`mark_inactive`](Self::mark_inactive) can mutate through the - /// `BackRef` every socket holds (see `NewSocket::get_handlers`) - /// without an `unsafe { &mut * }` reborrow per call site. + /// Live sockets plus in-flight callback [`Scope`]s. Drives the listener's + /// idle release; ownership itself is the `Rc`. pub active_connections: Cell, pub mode: SocketMode, - /// `JsCell` so [`resolve_promise`](Self::resolve_promise) / - /// [`reject_promise`](Self::reject_promise) can `try_swap()` through a - /// shared `&Handlers` (BackRef Deref). Single-JS-thread; the inner - /// `Strong` is never borrowed across a reentrant call. - pub promise: JsCell, // Strong.Optional → bun_jsc::Strong (Drop deallocates the slot) - - #[cfg(debug_assertions)] - pub protection_count: u32, + /// The listener that accepted these sockets, for `mode == Server`. + /// + /// Deliberately a nullable raw pointer and not a `BackRef`: a `BackRef` + /// promises the pointee outlives the holder, and this one does not. Every + /// accepted socket holds an `Rc` that routinely outlives the + /// `Listener` (uws defers the close of a force-closed socket past + /// `Listener::deinit`). What keeps it sound is that `deinit` clears this + /// field before freeing itself — so reads must go through [`listener`] and + /// handle `None`. + /// + /// [`listener`]: Self::listener + listener: Cell>>, } -// Bare JSValue fields are heap-stored here, but they are kept alive via JSC -// protect()/unprotect() (GC roots), not stack scanning — so this is sound. +/// Output of [`Handlers::prepare_reload`]: everything `reload` needs, parsed +/// and validated before any `Handlers` is touched. +pub struct ReloadedHandlers { + callbacks: [JSValue; CALLBACK_COUNT], + pub binary_type: BinaryType, +} -/// Expands `$body` once per callback field with `$f` bound to the field ident. -macro_rules! for_each_callback_field { - ($self:expr, |$f:ident| $body:block) => {{ - { - let $f = &mut $self.on_open; - $body - } - { - let $f = &mut $self.on_close; - $body - } - { - let $f = &mut $self.on_data; - $body - } - { - let $f = &mut $self.on_writable; - $body - } - { - let $f = &mut $self.on_timeout; - $body - } - { - let $f = &mut $self.on_connect_error; - $body - } - { - let $f = &mut $self.on_end; - $body - } - { - let $f = &mut $self.on_error; - $body - } - { - let $f = &mut $self.on_handshake; - $body - } - { - let $f = &mut $self.on_session; - $body - } - { - let $f = &mut $self.on_keylog; - $body - } - { - let $f = &mut $self.on_server_name; - $body - } - { - let $f = &mut $self.on_alpn_callback; - $body - } - }}; +fn binary_type_from_generated(binary_type: GeneratedBinaryType) -> BinaryType { + match binary_type { + GeneratedBinaryType::Arraybuffer => BinaryType::ArrayBuffer, + GeneratedBinaryType::Buffer => BinaryType::Buffer, + GeneratedBinaryType::Uint8array => BinaryType::Uint8Array, + } } impl Handlers { + /// The cell, to store in the listener / socket JS wrappers' visited + /// `handlers` slot so the callbacks stay reachable from every object that + /// can still invoke them. + #[inline] + pub fn cell(&self) -> JSValue { + self.cell.to_js() + } + + /// Roots the cell until the returned handle drops. Entry points that + /// construct a `Handlers` hold one across the window between the cell's + /// creation and the first JS wrapper that stores it — user JS (option + /// getters, `SSLConfig` parsing) and JSC allocations run in that window. + #[inline] + #[must_use = "the cell is collectable as soon as this drops"] + pub fn root_cell(&self, global: &JSGlobalObject) -> Strong { + self.cell.root(global) + } + + /// Records the listener that accepted these sockets (server mode only), or + /// clears it as that listener frees itself. + pub fn set_listener(&self, listener: Option>) { + debug_assert!(self.mode == SocketMode::Server || listener.is_none()); + self.listener.set(listener); + } + + /// The accepting listener, or `None` for client-mode handlers and for a + /// listener already torn down by `Listener::deinit`. + pub fn listener(&self) -> Option<&SocketListener> { + // SAFETY: `Listener::listen` stores its `heap::into_raw` allocation root + // here, and `Listener::deinit` clears it before freeing that allocation + // (after force-closing every accepted socket), so a `Some` is live. The + // borrow cannot outlive `&self`, and nothing frees a `Listener` while a + // caller holds one — `deinit` runs from GC finalize, not from dispatch. + self.listener.get().map(|l| unsafe { &*l.as_ptr() }) + } + + pub fn on_open(&self) -> JSValue { + self.cell.on_open() + } + pub fn on_close(&self) -> JSValue { + self.cell.on_close() + } + pub fn on_data(&self) -> JSValue { + self.cell.on_data() + } + pub fn on_writable(&self) -> JSValue { + self.cell.on_writable() + } + pub fn on_timeout(&self) -> JSValue { + self.cell.on_timeout() + } + pub fn on_connect_error(&self) -> JSValue { + self.cell.on_connect_error() + } + pub fn on_end(&self) -> JSValue { + self.cell.on_end() + } + pub fn on_error(&self) -> JSValue { + self.cell.on_error() + } + pub fn on_handshake(&self) -> JSValue { + self.cell.on_handshake() + } + pub fn on_session(&self) -> JSValue { + self.cell.on_session() + } + pub fn on_keylog(&self) -> JSValue { + self.cell.on_keylog() + } + pub fn on_server_name(&self) -> JSValue { + self.cell.on_server_name() + } + pub fn on_alpn_callback(&self) -> JSValue { + self.cell.on_alpn_callback() + } + + /// Drops the `open` callback for every holder of this `Handlers` — a client + /// socket does this after its first TLS handshake so renegotiations do not + /// fire it again. + pub fn clear_on_open(&self) { + self.cell.clear_on_open(&self.global_object); + } + + /// Wraps each provided callback with the current async context so it + /// dispatches in the right `AsyncLocalStorage` state. Runs before the cell + /// exists (create) or before any write to a live cell (reload). + fn wrap_with_context( + global_object: &JSGlobalObject, + callbacks: &[JSValue; CALLBACK_COUNT], + ) -> [JSValue; CALLBACK_COUNT] { + callbacks.map(|value| { + if value.is_empty() { + JSValue::ZERO + } else { + AsyncContextFrame__withAsyncContextIfNeeded(global_object, value) + } + }) + } + + /// Stores the pending `Bun.connect` promise in the cell. Rooted by the cell + /// like the callbacks, so settling it is the only release needed. + pub fn set_promise(&self, global_object: &JSGlobalObject, promise: JSValue) { + self.cell.set_promise(global_object, promise); + } + + /// Takes the pending connect promise, detaching it from the cell. + pub fn take_promise(&self) -> Option { + self.cell.take_promise(&self.global_object) + } + pub fn mark_active(&self) { bun_output::scoped_log!(Listener, "markActive"); self.active_connections @@ -129,38 +199,16 @@ impl Handlers { } /// Bumps `active_connections`, enters the JS event-loop scope, and returns - /// a `Scope` whose `exit()` undoes both. - /// - /// Takes `*mut Self` (not `&mut self`) because the matching - /// [`Scope::exit`] → [`Handlers::mark_inactive`] may **free this - /// allocation** (client mode, last ref). Storing a `&'a mut Handlers` in - /// `Scope` would leave a dangling reference after that free; a raw pointer - /// may dangle so long as it is not dereferenced. - /// - /// # Safety - /// `this` must point to a live `Handlers`. JS-thread only. - pub unsafe fn enter(this: *mut Self) -> Scope { - { - // SAFETY: caller contract — `this` is live; shared reborrow scoped - // to this block (no protector spans the later free in `exit`). - let h = unsafe { &*this }; - h.mark_active(); - h.vm.event_loop_ref().enter(); - } - Scope { handlers: this } - } - - /// Safe wrapper over [`enter`](Self::enter) for callers that already hold - /// a [`BackRef`](bun_ptr::BackRef) (i.e. every - /// `NewSocket::get_handlers()` site). The back-reference invariant - /// guarantees the pointee is live at call time, discharging `enter`'s - /// only precondition; JS-thread affinity is the same structural guarantee - /// every `BackRef` user already relies on (uws dispatch). + /// a [`Scope`] whose `exit()` undoes both. The scope holds its own `Rc`, so + /// a socket that closes and drops its reference mid-callback cannot free + /// the `Handlers` the callback is still reading from. #[inline] - pub fn enter_ref(h: bun_ptr::BackRef) -> Scope { - // SAFETY: BackRef invariant — pointee live and at a stable address - // for the holder's lifetime, so `h.as_ptr()` is dereferenceable now. - unsafe { Self::enter(h.as_ptr()) } + pub fn enter(self: &Rc) -> Scope { + self.mark_active(); + self.vm.event_loop_ref().enter(); + Scope { + handlers: Rc::clone(self), + } } // corker: Corker = .{}, @@ -171,7 +219,7 @@ impl Handlers { return Ok(()); } - let Some(promise) = self.promise.with_mut(|p| p.try_swap()) else { + let Some(promise) = self.take_promise() else { return Ok(()); }; let Some(any_promise) = promise.as_any_promise() else { @@ -187,7 +235,7 @@ impl Handlers { return Ok(true); } - let Some(promise) = self.promise.with_mut(|p| p.try_swap()) else { + let Some(promise) = self.take_promise() else { return Ok(false); }; let Some(any_promise) = promise.as_any_promise() else { @@ -197,66 +245,31 @@ impl Handlers { Ok(true) } - /// Returns true when the client-mode allocation has been destroyed so the - /// caller can null any `*Handlers` it still holds (the socket's `handlers` - /// field). Without that, a subsequent `connectInner` reusing the same native - /// socket as `prev` would `deinit`/`destroy` the freed pointer. - /// - /// Takes `*mut Self` (not `&mut self`) because under Stacked Borrows a - /// `&mut self` argument carries a *protector* for the duration of the - /// call: deallocating the allocation it points into while that protector - /// is live is UB. A raw `*mut` carries no protector, and the short-lived - /// reborrows below all end before the `heap::take`. - /// - /// # Safety - /// - `this` must point to a live `Handlers`. - /// - Server mode: `this` must address the embedded `Listener.handlers` - /// field with whole-`Listener` provenance (for `from_field_ptr!`). - /// - Client mode: `this` must be the `heap::alloc` allocation root. - /// - After this returns `true`, `this` is dangling — caller must not - /// dereference it and must null any stored copy. - pub unsafe fn mark_inactive(this: *mut Self) -> bool { + /// Drops one `active_connections` reference. Returns true once none are + /// left and this is not a listener's `Handlers` — the socket's cue to drop + /// its own `Rc` so a later dispatch sees no handlers rather than a stale + /// callback table. Freeing is the `Rc`'s job, not this function's. + pub fn mark_inactive(&self) -> bool { bun_output::scoped_log!(Listener, "markInactive"); - let (remaining, mode) = { - // SAFETY: caller contract — `this` is live on entry. Shared reborrow - // scoped to this block so no `&Handlers` protector spans the - // `heap::take` in the client branch below. - let h = unsafe { &*this }; - let remaining = h.active_connections.get() - 1; - h.active_connections.set(remaining); - (remaining, h.mode) - }; - if remaining == 0 { - if mode == SocketMode::Server { - // SAFETY: server-mode caller contract — `this` addresses the - // `handlers` field of a `Listener` with whole-`Listener` - // provenance. R-2: `Listener.handlers` is `JsCell` - // (`#[repr(transparent)]`), so the field offset equals the - // inner `Handlers` address; `from_field_ptr!` arithmetic is - // unchanged. Deref as shared (`&*`) — celled fields below - // take `&self`. - let listen_socket: &SocketListener = - unsafe { &*bun_core::from_field_ptr!(SocketListener, handlers, this) }; - // allow it to be GC'd once the last connection is closed and it's not listening anymore - if matches!(listen_socket.listener.get(), ListenerType::None) { - listen_socket - .poll_ref - .with_mut(|p| p.unref(bun_io::js_vm_ctx())); - // `deinit` empties the Strong slot in place; the field stays valid. - listen_socket.strong_self.with_mut(|s| s.deinit()); - } - } else { - // Client-mode Handlers is heap-allocated per-connection - // (Listener::connect_inner via `heap::alloc`). - // Free in place so callers that only hold a `*mut` - // (and thus can't `drop(Box)`) don't leak the allocation or - // its `protect()`ed JSValues. Caller must still null its - // field when this returns true. - // SAFETY: client-mode caller contract — `this` is the - // `heap::alloc` allocation root; no live `&`/`&mut` borrow - // of it remains (all reborrows above have ended). - drop(unsafe { bun_core::heap::take(this) }); - return true; + let remaining = self.active_connections.get() - 1; + self.active_connections.set(remaining); + if remaining != 0 { + return false; + } + if self.mode != SocketMode::Server { + return true; + } + // Nothing to release once the process is exiting, and the listener's + // JS wrapper may already be gone. + if self.vm.is_shutting_down() { + return false; + } + // Let the listener's JS wrapper be GC'd once the last connection is + // closed and it's not listening anymore. + if let Some(listener) = self.listener() { + if matches!(listener.listener.get(), ListenerType::None) { + listener.poll_ref.with_mut(|p| p.unref(bun_io::js_vm_ctx())); + listener.this_value.with_mut(|r| r.downgrade()); } } false @@ -269,7 +282,7 @@ impl Handlers { } let global_object = self.global_object; - let on_error = self.on_error; + let on_error = self.on_error(); if on_error.is_empty() { // SAFETY: `bun_vm()` is non-null for a Bun-owned global; single JS thread. @@ -291,163 +304,124 @@ impl Handlers { pub fn from_js( global_object: &JSGlobalObject, opts: JSValue, - is_server: bool, - ) -> JsResult { + mode: SocketMode, + ) -> JsResult> { let generated = GeneratedSocketConfigHandlers::from_js(global_object, opts)?; - Self::from_generated(global_object, &generated, is_server) + Self::from_generated(global_object, &generated, mode) } pub fn from_generated( global_object: &JSGlobalObject, generated: &GeneratedSocketConfigHandlers, - is_server: bool, - ) -> JsResult { - let mut result = Handlers { - on_open: JSValue::ZERO, - on_close: JSValue::ZERO, - on_data: JSValue::ZERO, - on_writable: JSValue::ZERO, - on_timeout: JSValue::ZERO, - on_connect_error: JSValue::ZERO, - on_end: JSValue::ZERO, - on_error: JSValue::ZERO, - on_handshake: JSValue::ZERO, - on_session: JSValue::ZERO, - on_keylog: JSValue::ZERO, - on_server_name: JSValue::ZERO, - on_alpn_callback: JSValue::ZERO, - binary_type: match generated.binary_type { - GeneratedBinaryType::Arraybuffer => BinaryType::ArrayBuffer, - GeneratedBinaryType::Buffer => BinaryType::Buffer, - GeneratedBinaryType::Uint8array => BinaryType::Uint8Array, - }, + mode: SocketMode, + ) -> JsResult> { + let callbacks = Self::validate_callbacks(global_object, generated)?; + let wrapped = Self::wrap_with_context(global_object, &callbacks); + + // Everything fallible is done; the cell is infallible, so a constructed + // `Handlers` is always fully initialized. + Ok(Rc::new(Handlers { + cell: JSSocketHandlers::create(global_object, &wrapped), + binary_type: Cell::new(binary_type_from_generated(generated.binary_type)), // SAFETY: `bun_vm()` never returns null for a Bun-owned global; the // VM outlives every `Handlers` (process-lifetime singleton). vm: global_object.bun_vm(), global_object: GlobalRef::from(global_object), active_connections: Cell::new(0), - mode: if is_server { - SocketMode::Server - } else { - SocketMode::Client - }, - promise: JsCell::new(Strong::empty()), - #[cfg(debug_assertions)] - protection_count: 0, - }; + mode, + listener: Cell::new(None), + })) + } - // inline for (callback_fields) |field| { ... @field(generated, field) ... } - macro_rules! assign_callback { + /// Validates the user-supplied callbacks without constructing or storing + /// anything. Callbacks the user did not provide come back as + /// `JSValue::ZERO`. Array order matches `Bun::JSSocketHandlers::Field`. + fn validate_callbacks( + global_object: &JSGlobalObject, + generated: &GeneratedSocketConfigHandlers, + ) -> JsResult<[JSValue; CALLBACK_COUNT]> { + macro_rules! validated_callback { ($field:ident, $name:literal) => {{ let value = generated.$field; if value.is_undefined_or_null() { + JSValue::ZERO } else if !value.is_callable() { return Err(global_object.throw_invalid_arguments(format_args!( "Expected \"{}\" callback to be a function", $name ))); } else { - result.$field = value; + value } }}; } - assign_callback!(on_open, "onOpen"); - assign_callback!(on_close, "onClose"); - assign_callback!(on_data, "onData"); - assign_callback!(on_writable, "onWritable"); - assign_callback!(on_timeout, "onTimeout"); - assign_callback!(on_connect_error, "onConnectError"); - assign_callback!(on_end, "onEnd"); - assign_callback!(on_error, "onError"); - assign_callback!(on_handshake, "onHandshake"); - assign_callback!(on_session, "onSession"); - assign_callback!(on_keylog, "onKeylog"); - assign_callback!(on_server_name, "onServerName"); - assign_callback!(on_alpn_callback, "onALPNCallback"); - - if result.on_data.is_empty() && result.on_writable.is_empty() { + let on_open = validated_callback!(on_open, "onOpen"); + let on_close = validated_callback!(on_close, "onClose"); + let on_data = validated_callback!(on_data, "onData"); + let on_writable = validated_callback!(on_writable, "onWritable"); + let on_timeout = validated_callback!(on_timeout, "onTimeout"); + let on_connect_error = validated_callback!(on_connect_error, "onConnectError"); + let on_end = validated_callback!(on_end, "onEnd"); + let on_error = validated_callback!(on_error, "onError"); + let on_handshake = validated_callback!(on_handshake, "onHandshake"); + let on_session = validated_callback!(on_session, "onSession"); + let on_keylog = validated_callback!(on_keylog, "onKeylog"); + let on_server_name = validated_callback!(on_server_name, "onServerName"); + let on_alpn_callback = validated_callback!(on_alpn_callback, "onALPNCallback"); + + if on_data.is_empty() && on_writable.is_empty() { return Err(global_object.throw_invalid_arguments(format_args!( "Expected at least \"data\" or \"drain\" callback" ))); } - result.with_async_context_if_needed(global_object); - result.protect(); - Ok(result) - } - - fn unprotect(&mut self) { - if self.vm.is_shutting_down() { - return; - } - #[cfg(debug_assertions)] - { - debug_assert!(self.protection_count > 0); - self.protection_count -= 1; - } - self.on_open.unprotect(); - self.on_close.unprotect(); - self.on_data.unprotect(); - self.on_writable.unprotect(); - self.on_timeout.unprotect(); - self.on_connect_error.unprotect(); - self.on_end.unprotect(); - self.on_error.unprotect(); - self.on_handshake.unprotect(); - self.on_session.unprotect(); - self.on_keylog.unprotect(); - self.on_server_name.unprotect(); - self.on_alpn_callback.unprotect(); - } - - fn with_async_context_if_needed(&mut self, global_object: &JSGlobalObject) { - for_each_callback_field!(self, |f| { - if !f.is_empty() { - // SAFETY: FFI — `global_object` is a live JSGlobalObject*, `*f` is a - // protect()-rooted callable JSValue; returns the (possibly wrapped) value. - *f = AsyncContextFrame__withAsyncContextIfNeeded(global_object, *f); - } - }); + Ok([ + on_open, + on_close, + on_data, + on_writable, + on_timeout, + on_connect_error, + on_end, + on_error, + on_handshake, + on_session, + on_keylog, + on_server_name, + on_alpn_callback, + ]) } - fn protect(&mut self) { - #[cfg(debug_assertions)] - { - self.protection_count += 1; - } - self.on_open.protect(); - self.on_close.protect(); - self.on_data.protect(); - self.on_writable.protect(); - self.on_timeout.protect(); - self.on_connect_error.protect(); - self.on_end.protect(); - self.on_error.protect(); - self.on_handshake.protect(); - self.on_session.protect(); - self.on_keylog.protect(); - self.on_server_name.protect(); - self.on_alpn_callback.protect(); + /// Parses and validates `opts` for `reload` without touching any + /// `Handlers`: the option getters run user JS that can close a socket and + /// free or repoint its `Handlers`, so callers must re-check liveness + /// before [`apply_reload`](Self::apply_reload). On error nothing is + /// modified. + pub fn prepare_reload( + global_object: &JSGlobalObject, + opts: JSValue, + ) -> JsResult { + let generated = GeneratedSocketConfigHandlers::from_js(global_object, opts)?; + let callbacks = Self::validate_callbacks(global_object, &generated)?; + Ok(ReloadedHandlers { + callbacks, + binary_type: binary_type_from_generated(generated.binary_type), + }) } -} -impl Drop for Handlers { - fn drop(&mut self) { - self.unprotect(); - if self.vm.is_shutting_down() { - // `~VM` may have already torn down the HandleSet that - // `Strong::drop` writes back into; the slot is bulk-freed by the - // VM destructor, so leaking it here is correct. - let _ = core::mem::ManuallyDrop::new(self.promise.replace(Strong::empty())); - } + /// Writes the validated callbacks into the existing cell, so the listener + /// and every live socket sharing it pick them up in place. Runs no user JS. + pub fn apply_reload(&self, global_object: &JSGlobalObject, reloaded: &ReloadedHandlers) { + let wrapped = Self::wrap_with_context(global_object, &reloaded.callbacks); + self.cell.set_callbacks(global_object, &wrapped); + self.binary_type.set(reloaded.binary_type); } } -/// Holds a raw `*mut Handlers` (not `&mut`) because [`Scope::exit`] may free -/// the backing allocation (client mode, last ref). A `&mut` field would dangle -/// after that — UB even if never dereferenced. A raw pointer may dangle. +/// One in-flight dispatch into JS. Holds an `Rc` so the callbacks it is about +/// to invoke outlive a `close()` from inside them. pub struct Scope { - pub handlers: *mut Handlers, + pub handlers: Rc, } impl Scope { @@ -455,33 +429,22 @@ impl Scope { /// [`Handlers::enter`]. Split from the `active_connections` bookkeeping /// because draining microtasks here can synchronously reconnect or /// `upgradeTLS` the socket, so the caller must observe the resulting - /// `handlers` state before deciding whether to decrement/free. Must be - /// followed by exactly one [`mark_inactive`](Self::mark_inactive), or by - /// dropping the scope when a new owner took over the handlers. + /// `handlers` state before deciding whether to decrement. pub fn exit_event_loop(&self) { - // SAFETY: no decrement has run yet, so `handlers` is still live (caller - // contract of `Handlers::enter`). `event_loop_ref()` returns a non-null - // self-pointer into the VM; single JS thread, no aliasing `&mut - // EventLoop` outlives this call. - unsafe { (*self.handlers).vm }.event_loop_ref().exit(); + self.handlers.vm.event_loop_ref().exit(); } - /// The `active_connections` half: decrements and, on reaching zero, frees - /// the client-mode allocation (or releases the listener, server mode). - /// Returns true if the client-mode allocation was destroyed; callers that - /// also hold the pointer in a socket field must then null it. + /// The `active_connections` half: decrements and, on reaching zero, + /// releases the listener (server mode). Returns true when the socket + /// should drop its own `Rc` — see [`Handlers::mark_inactive`]. /// - /// Consumes `self`: a `Scope` is single-use (one `enter` ↔ one exit), and - /// after a `true` return `self.handlers` is dangling. + /// Consumes `self`: a `Scope` is single-use (one `enter` ↔ one exit). pub fn mark_inactive(self) -> bool { - // SAFETY: `handlers` satisfies `mark_inactive`'s contract by - // construction in `Handlers::enter` (caller passed the - // server-embedded / client-heap-root pointer). - unsafe { Handlers::mark_inactive(self.handlers) } + self.handlers.mark_inactive() } /// Event-loop exit + `mark_inactive` in one step, for callers that cannot - /// observe an intervening handlers transfer. Returns true if destroyed. + /// observe an intervening handlers transfer. pub fn exit(self) -> bool { self.exit_event_loop(); self.mark_inactive() @@ -490,13 +453,12 @@ impl Scope { use bun_jsc::generated::SocketConfigHandlersBinaryType as GeneratedBinaryType; -/// `handlers` is always `protect`ed in this struct. pub struct SocketConfig { pub hostname_or_unix: ZigStringSlice, pub port: Option, pub fd: Option, pub ssl: Option, - pub handlers: Handlers, + pub handlers: Rc, pub default_data: JSValue, pub exclusive: bool, pub allow_half_open: bool, @@ -506,17 +468,6 @@ pub struct SocketConfig { impl SocketConfig { // Full teardown is handled by Drop (all owned fields impl Drop). - // `deinit_excluding_handlers` preserves `handlers` at the same address so - // outstanding `*Handlers` stay valid. - - /// Deinitializes everything except `handlers`. - pub fn deinit_excluding_handlers(&mut self) { - // Drops the owned non-handlers fields in place; `handlers` is left - // untouched so pointers into it remain valid. - self.hostname_or_unix = ZigStringSlice::empty(); - self.ssl = None; - // other scalar fields need no cleanup - } pub fn socket_flags(&self) -> i32 { let mut flags: i32 = if self.exclusive { @@ -541,7 +492,7 @@ impl SocketConfig { _vm: &'static VirtualMachine, global: &JSGlobalObject, generated: &GeneratedSocketConfig, - is_server: bool, + mode: SocketMode, ) -> JsResult { let mut result: SocketConfig = 'blk: { let ssl: Option = match &generated.tls { @@ -565,7 +516,7 @@ impl SocketConfig { port: None, fd: generated.fd.map(Fd::from_uv), ssl, - handlers: Handlers::from_generated(global, &generated.handlers, is_server)?, + handlers: Handlers::from_generated(global, &generated.handlers, mode)?, default_data: if generated.data.is_undefined() { JSValue::ZERO } else { @@ -577,8 +528,8 @@ impl SocketConfig { ipv6_only: false, }; }; - // On any `?` below, `result` drops and `Handlers::Drop` unprotects its - // JSValues — no manual error-path cleanup needed. + // On any `?` below, `result` drops and releases what it owns — no + // manual error-path cleanup needed. if result.fd.is_some() { // If a user passes a file descriptor then prefer it over hostname or unix @@ -635,10 +586,10 @@ impl SocketConfig { vm: &'static VirtualMachine, opts: JSValue, global_object: &JSGlobalObject, - is_server: bool, + mode: SocketMode, ) -> JsResult { let generated = GeneratedSocketConfig::from_js(global_object, opts)?; - Self::from_generated(vm, global_object, &generated, is_server) + Self::from_generated(vm, global_object, &generated, mode) } } diff --git a/src/runtime/socket/JSSocketHandlers.rs b/src/runtime/socket/JSSocketHandlers.rs new file mode 100644 index 000000000000..6b928a56867f --- /dev/null +++ b/src/runtime/socket/JSSocketHandlers.rs @@ -0,0 +1,183 @@ +//! Rust view of `Bun::JSSocketHandlers` (`src/jsc/bindings/JSSocketHandlers.cpp`): +//! a GC-visited internal-fields cell holding a socket context's JS callbacks +//! and its pending `Bun.connect` promise. +//! +//! The cell is what keeps those values alive. It is stored in the visited +//! `handlers` slot of the listener's JS wrapper and of every socket's wrapper, +//! so the callbacks live exactly as long as something that can still invoke +//! them — no `gcProtect` bookkeeping to unbalance. + +use bun_jsc::{JSGlobalObject, JSValue, Strong}; + +unsafe extern "C" { + /// Allocates the cell with the 13 callback fields populated barrier-free + /// (the cell is not yet GC-visible); the promise field starts `undefined`. + safe fn Bun__SocketHandlers__create( + global: &JSGlobalObject, + callbacks: *const JSValue, + ) -> JSValue; + /// `cell` must come from [`Bun__SocketHandlers__create`]; `index` must be + /// < `numberOfInternalFields` (asserted in debug C++). + safe fn Bun__SocketHandlers__getField(cell: JSValue, index: u32) -> JSValue; + safe fn Bun__SocketHandlers__setField( + global: &JSGlobalObject, + cell: JSValue, + index: u32, + value: JSValue, + ); + /// Overwrites all 13 callback fields on a live cell with one trailing + /// write barrier. + safe fn Bun__SocketHandlers__setCallbacks( + global: &JSGlobalObject, + cell: JSValue, + callbacks: *const JSValue, + ); +} + +/// A field of the cell. Discriminants are ABI shared with +/// `Bun::JSSocketHandlers::Field` in `src/jsc/bindings/JSSocketHandlers.h`. +/// An implementation detail of this module: callers name fields through the +/// accessors below. +#[repr(u32)] +#[derive(Clone, Copy)] +enum Field { + Open = 0, + Close, + Data, + Writable, + Timeout, + ConnectError, + End, + Error, + Handshake, + Session, + Keylog, + ServerName, + AlpnCallback, + /// Not a callback: the pending `Bun.connect` promise, cleared once settled. + Promise, +} + +/// Number of callback fields (everything before `Promise`). Matches +/// `Bun::JSSocketHandlers::numberOfCallbacks`. +pub const CALLBACK_COUNT: usize = Field::Promise as usize; + +/// A `Bun::JSSocketHandlers` cell. +/// +/// Unrooted: a `JSSocketHandlers` is only valid while some JS wrapper holds it +/// in a visited slot, or while a [`root`](Self::root) handle is alive. It is +/// `Copy` because it is just the cell's `JSValue`. +#[derive(Clone, Copy)] +pub struct JSSocketHandlers(JSValue); + +/// Defines a getter per callback field. +macro_rules! callback_getters { + ($($name:ident => $field:ident),* $(,)?) => { + $( + /// The callback, or `JSValue::ZERO` if unset. + #[inline] + pub fn $name(self) -> JSValue { + self.get(Field::$field) + } + )* + }; +} + +impl JSSocketHandlers { + /// Allocates the cell with `callbacks` stored via the barrier-free + /// early-init path. `JSValue::ZERO` entries are stored as `undefined`. + pub fn create(global: &JSGlobalObject, callbacks: &[JSValue; CALLBACK_COUNT]) -> Self { + Self(Bun__SocketHandlers__create(global, callbacks.as_ptr())) + } + + /// The cell as a `JSValue`, to store in a wrapper's visited slot. + #[inline] + pub fn to_js(self) -> JSValue { + self.0 + } + + /// Roots the cell until the returned handle drops. Callers hold one across + /// the window between creating the cell and the first JS wrapper that + /// stores it in a visited slot: option getters run user JS in that window, + /// and the only copy of the cell lives in a heap-allocated `Handlers`, which + /// the GC does not scan. Conservative stack scanning happens to cover the + /// current call shapes, which is why nothing observably breaks without this + /// — that is not a guarantee the compiler owes us. + #[inline] + #[must_use = "the cell is collectable as soon as this drops"] + pub fn root(self, global: &JSGlobalObject) -> Strong { + Strong::create(self.0, global) + } + + callback_getters! { + on_open => Open, + on_close => Close, + on_data => Data, + on_writable => Writable, + on_timeout => Timeout, + on_connect_error => ConnectError, + on_end => End, + on_error => Error, + on_handshake => Handshake, + on_session => Session, + on_keylog => Keylog, + on_server_name => ServerName, + on_alpn_callback => AlpnCallback, + } + + /// Replaces every callback on a live cell. `JSValue::ZERO` entries clear + /// the field, so `socket.reload()` also drops callbacks the new options + /// omit. One write barrier for the whole batch. + pub fn set_callbacks(self, global: &JSGlobalObject, callbacks: &[JSValue; CALLBACK_COUNT]) { + Bun__SocketHandlers__setCallbacks(global, self.0, callbacks.as_ptr()); + } + + /// Drops the `open` callback: a client socket clears it after its first TLS + /// handshake so renegotiations do not fire it again. + #[inline] + pub fn clear_on_open(self, global: &JSGlobalObject) { + self.set(global, Field::Open, JSValue::ZERO); + } + + /// Stores the pending `Bun.connect` promise. + #[inline] + pub fn set_promise(self, global: &JSGlobalObject, promise: JSValue) { + self.set(global, Field::Promise, promise); + } + + /// Takes the pending connect promise and detaches it from the cell, so a + /// settled promise — which resolves to the socket's JS wrapper, the object + /// holding this very cell — is not kept alive by the connection it + /// completed. + pub fn take_promise(self, global: &JSGlobalObject) -> Option { + let promise = self.get(Field::Promise); + if promise.is_empty() { + return None; + } + self.set(global, Field::Promise, JSValue::ZERO); + Some(promise) + } + + /// Reads a field. Unset fields (stored as `undefined`) read back as + /// `JSValue::ZERO` so call sites keep their `is_empty()` checks. + #[inline] + fn get(self, field: Field) -> JSValue { + let value = Bun__SocketHandlers__getField(self.0, field as u32); + if value.is_undefined() { + JSValue::ZERO + } else { + value + } + } + + /// Writes a field. `JSValue::ZERO` clears it. + #[inline] + fn set(self, global: &JSGlobalObject, field: Field, value: JSValue) { + let value = if value.is_empty() { + JSValue::UNDEFINED + } else { + value + }; + Bun__SocketHandlers__setField(global, self.0, field as u32, value); + } +} diff --git a/src/runtime/socket/Listener.rs b/src/runtime/socket/Listener.rs index 7776a42f700a..7f19e2ccdf49 100644 --- a/src/runtime/socket/Listener.rs +++ b/src/runtime/socket/Listener.rs @@ -4,6 +4,7 @@ use core::cell::Cell; use core::ffi::{c_int, c_void}; use core::mem::size_of; use core::ptr::NonNull; +use std::rc::Rc; use bun_boringssl_sys as boring_sys; use bun_io::KeepAlive; @@ -11,7 +12,7 @@ use bun_jsc::ZigStringJsc as _; use bun_jsc::strong::Optional as Strong; use bun_jsc::virtual_machine::VirtualMachine; use bun_jsc::zig_string::ZigString; -use bun_jsc::{self as jsc, CallFrame, JSGlobalObject, JSValue, JsCell, JsClass, JsResult}; +use bun_jsc::{self as jsc, CallFrame, JSGlobalObject, JSValue, JsCell, JsRef, JsResult}; use bun_sys::{self, Fd}; use bun_uws as uws; use bun_uws_sys as uws_sys; @@ -42,19 +43,21 @@ use bun_sys::windows::libuv as uv; bun_output::define_scoped_log!(log, Listener, visible); -/// Bridge to the per-VM digest-keyed weak `SSL_CTX*` cache. The -/// `bun_jsc::rare_data::SSLContextCache` slot is an opaque cycle-break stub; -/// the concrete cache lives on `crate::jsc_hooks::RuntimeState`. +/// Runs `f` against this thread's `SSL_CTX` cache. Takes a callback rather than +/// handing out a `&'static mut`, which two callers could hold at once. #[inline] -fn vm_ssl_ctx_cache() -> *mut crate::api::SSLContextCache::SSLContextCache { +fn with_ssl_ctx_cache( + f: impl FnOnce(&mut crate::api::SSLContextCache::SSLContextCache) -> R, +) -> R { let state = crate::jsc_hooks::runtime_state(); debug_assert!( !state.is_null(), "runtime_state() before init_runtime_state" ); // SAFETY: `state` is the per-thread `RuntimeState` boxed in - // `init_runtime_state`; address-stable until VM teardown. - unsafe { core::ptr::addr_of_mut!((*state).ssl_ctx_cache) } + // `init_runtime_state`, address-stable until VM teardown, and only the JS + // thread reaches here — so this `&mut` is unique for `f`'s duration. + f(unsafe { &mut (*state).ssl_ctx_cache }) } // Route through the codegen'd `toJS` wrapper so we @@ -70,7 +73,7 @@ use crate::generated_classes::js_Listener; // so the impls below compile against either. #[bun_jsc::JsClass(no_constructor)] pub struct Listener { - pub handlers: JsCell, + pub handlers: Rc, pub listener: Cell, pub poll_ref: JsCell, @@ -87,7 +90,9 @@ pub struct Listener { pub protos: Option>, pub strong_data: JsCell, - pub strong_self: JsCell, + /// Reference to this listener's JS wrapper. Strong while it is listening or + /// has connections, downgraded to weak once idle so GC can reclaim it. + pub this_value: JsCell, } #[derive(Clone, Copy, Default)] @@ -137,7 +142,7 @@ impl Listener { if args.len < 1 || (matches!(this.listener.get(), ListenerType::None) - && this.handlers.get().active_connections.get() == 0) + && this.handlers.active_connections.get() == 0) { return Err(global.throw(format_args!("Expected 1 argument"))); } @@ -152,21 +157,12 @@ impl Listener { None => return Err(global.throw(format_args!("Expected \"socket\" object"))), }; - let handlers = Handlers::from_js( - global, - socket_obj, - this.handlers.get().mode == SocketMode::Server, - )?; - // Preserve the live connection count across the struct assignment. `Handlers.fromJS` - // returns `active_connections = 0`, but existing accepted sockets each hold a +1 via - // `markActive`. Without this, closing any of them after reload would underflow the - // counter (panic in safe builds, wrap in release). - // Note: Drop handles unprotect; assignment below drops old. - this.handlers.with_mut(|h| { - let active_connections = h.active_connections.get(); - *h = handlers; - h.active_connections.set(active_connections); - }); + // Validates like construction (the option getters run user JS), then + // updates the callbacks of the existing cell in place, so the + // listener and every live socket sharing it pick them up with no swap + // of the `Handlers` itself. + let reloaded = Handlers::prepare_reload(global, socket_obj)?; + this.handlers.apply_reload(global, &reloaded); Ok(JSValue::UNDEFINED) } @@ -182,18 +178,13 @@ impl Listener { // SAFETY: VirtualMachine::get() returns the per-thread VM; valid for program lifetime. let vm = VirtualMachine::get().as_mut(); - let socket_config = SocketConfig::from_js(vm, opts, global, true)?; - #[cfg(windows)] - let mut socket_config = socket_config; - // Teardown handled by Drop on SocketConfig - // (excluding handlers, which are moved out below). Verified: on early-error - // paths the whole SocketConfig drops (Handlers::drop unprotects); - // on success both arms move `handlers` out via `ptr::read` and suppress - // the source's drop (`mem::forget` / `ManuallyDrop`) after extracting the - // other owned fields, so handlers are dropped exactly once. - - // Only deinit handlers if there's an error; otherwise we put them in a `Listener` and - // need them to stay alive. + let mut socket_config = SocketConfig::from_js(vm, opts, global, SocketMode::Server)?; + // Teardown handled by Drop on SocketConfig; `handlers` is an `Rc` the + // `Listener` clones out of it. + // + // The handlers cell has no JS wrapper holding it yet — root it until + // `js_Listener::handlers_set_cached` below. + let _cell_root = socket_config.handlers.root_cell(global); let port = socket_config.port; let ssl_enabled = socket_config.ssl.is_some(); @@ -207,13 +198,14 @@ impl Listener { normalize_pipe_name(socket_config.hostname_or_unix.slice(), buf.as_mut_slice()) { // Note: reshaped — `pipe_name` borrows `buf`; copy to an owned - // buffer so the borrow ends before we move `socket_config` below. + // buffer so the borrow ends before we `mem::take` from + // `socket_config` below. let mut pipe_buf = PathBuffer::uninit(); let pipe_len = pipe_name.len(); pipe_buf[..pipe_len].copy_from_slice(pipe_name); - // Transfer the allocation out of `socket_config` so the - // `mem::forget` below doesn't leak it. + // Move the hostname bytes into `connection`; `socket_config` + // drops the emptied slice. let connection = UnixOrHost::Unix( core::mem::take(&mut socket_config.hostname_or_unix) .into_vec() @@ -222,17 +214,13 @@ impl Listener { vm.event_loop_ref().ensure_waker(); - // Note: by-value move of Handlers — see the non-pipe arm below - // for rationale on `ptr::read` + `mem::forget`. - // SAFETY: socket_config.handlers is valid; we forget socket_config to avoid double-drop. - let handlers_moved: Handlers = unsafe { core::ptr::read(&socket_config.handlers) }; + let handlers = Rc::clone(&socket_config.handlers); let protos_taken = socket_config.ssl.as_mut().and_then(|s| s.take_protos()); let default_data = socket_config.default_data; let ssl_cfg_taken = socket_config.ssl.take(); - core::mem::forget(socket_config); let this: *mut Listener = bun_core::heap::into_raw(Box::new(Listener { - handlers: JsCell::new(handlers_moved), + handlers, connection, ssl: ssl_enabled, listener: Cell::new(ListenerType::None), @@ -241,7 +229,7 @@ impl Listener { group: JsCell::new(uws::SocketGroup::default()), secure_ctx: None, strong_data: JsCell::new(Strong::empty()), - strong_self: JsCell::new(Strong::empty()), + this_value: JsCell::new(JsRef::empty()), })); // SAFETY: just allocated, non-null, exclusive let this_ref = unsafe { &mut *this }; @@ -269,15 +257,9 @@ impl Listener { )); } Err(_) => { - // On error, clean up everything `this` owns *except* `this.handlers`: - // those JSValues must only be unprotected once, so calling - // `this.deinit()` here would unprotect the same callbacks a second - // time. `handlers` was *moved* into the box, so we - // recover it from the box before freeing and let it drop here for a - // single-unprotect effect. this_ref.strong_data.with_mut(|s| s.deinit()); // SAFETY: reclaim the Box we leaked via into_raw; drops connection, - // protos, and (the moved) handlers exactly once. + // protos, and the handlers `Rc`. drop(unsafe { bun_core::heap::take(this) }); return Err(global.throw_invalid_arguments(format_args!( "Failed to listen at {}", @@ -289,7 +271,13 @@ impl Listener { // SAFETY: `global` is live; ownership of `this` (heap-allocated above) // transfers to the C++ wrapper. let this_value = js_Listener::to_js(this, global); - this_ref.strong_self.with_mut(|s| s.set(global, this_value)); + // The listener holds the handlers cell in a visited slot; every + // accepted socket shares the same cell. + js_Listener::handlers_set_cached(this_value, global, this_ref.handlers.cell()); + this_ref.handlers.set_listener(NonNull::new(this)); + this_ref + .this_value + .with_mut(|r| r.set_strong(this_value, global)); this_ref.poll_ref.with_mut(|p| p.ref_(bun_io::js_vm_ctx())); return Ok(this_value); } @@ -300,17 +288,9 @@ impl Listener { // Allocate the Listener up front so the embedded `group` has its final // address before we hand it to listen() (it's linked into the loop's // intrusive list). - // Note: by-value move of Handlers. Read the handlers - // out by raw ptr and prevent double-drop by clearing the source via - // `deinit_excluding_handlers` + `mem::forget`. - let mut socket_config = core::mem::ManuallyDrop::new(socket_config); - // SAFETY: socket_config.handlers is valid; ManuallyDrop suppresses the second drop. - let handlers_moved: Handlers = - unsafe { core::ptr::read(&raw const socket_config.handlers) }; + let handlers = Rc::clone(&socket_config.handlers); let protos_taken = socket_config.ssl.as_mut().and_then(|s| s.take_protos()); let default_data = socket_config.default_data; - // Transfer the allocation out of `socket_config` so the `mem::forget` - // below doesn't leak it. let hostname_owned: Box<[u8]> = core::mem::take(&mut socket_config.hostname_or_unix) .into_vec() .into_boxed_slice(); @@ -318,7 +298,7 @@ impl Listener { let ssl_cfg_taken = socket_config.ssl.take(); let this: *mut Listener = bun_core::heap::into_raw(Box::new(Listener { - handlers: JsCell::new(handlers_moved), + handlers, // Placeholder until `this_ref.connection = connection` below. // Cannot `mem::zeroed()` a Rust enum (UB). connection: UnixOrHost::Fd(Fd::invalid()), @@ -329,7 +309,7 @@ impl Listener { group: JsCell::new(uws::SocketGroup::default()), secure_ctx: None, strong_data: JsCell::new(Strong::empty()), - strong_self: JsCell::new(Strong::empty()), + this_value: JsCell::new(JsRef::empty()), })); // SAFETY: just allocated, non-null, exclusive let this_ref = unsafe { &mut *this }; @@ -525,11 +505,7 @@ impl Listener { // null return falls back to the static tree (bind hostname + // addContext entries), then the default context; an asynchronous // resolution suspends the handshake until resumeSNI. - // SAFETY: `handlers` is embedded in the live Listener. - if !unsafe { &*this_ref.handlers.as_ptr() } - .on_server_name - .is_empty() - { + if !this_ref.handlers.on_server_name().is_empty() { // S008: `ListenSocket` is an `opaque_ffi!` ZST - safe deref. bun_opaque::opaque_deref_mut(listen_socket).on_server_name(us_dispatch_server_name); } @@ -540,18 +516,26 @@ impl Listener { // transfers to the C++ wrapper (freed via `ListenerClass__finalize` → // `Listener::finalize` → `deinit`). let this_value = js_Listener::to_js(this, global); - this_ref.strong_self.with_mut(|s| s.set(global, this_value)); + // The listener holds the handlers cell in a visited slot; every + // accepted socket shares the same cell. + js_Listener::handlers_set_cached(this_value, global, this_ref.handlers.cell()); + this_ref.handlers.set_listener(NonNull::new(this)); + this_ref + .this_value + .with_mut(|r| r.set_strong(this_value, global)); this_ref.poll_ref.with_mut(|p| p.ref_(bun_io::js_vm_ctx())); Ok(this_value) } - pub fn on_name_pipe_created(listener: &Listener) -> *mut NewSocket { + pub fn on_name_pipe_created( + listener: &Listener, + ) -> bun_ptr::ThisPtr> { debug_assert!(SSL == listener.ssl); let this_socket = NewSocket::::new(NewSocket:: { ref_count: bun_ptr::RefCount::init(), - handlers: Cell::new(NonNull::new(listener.handlers.as_ptr())), + handlers: JsCell::new(Some(Rc::clone(&listener.handlers))), socket: Cell::new(uws::NewSocketHandler::::DETACHED), protos: JsCell::new(listener.protos.clone()), // `protos` is `Option>` so we clone the listener's slice. @@ -568,15 +552,13 @@ impl Listener { native_callback: JsCell::new(crate::socket::NativeCallbacks::None), twin: JsCell::new(None), }); - // SAFETY: `NewSocket::new` returns a non-null live heap pointer - // (refcount==1); single JS thread, no other borrow exists yet. - let s = unsafe { bun_ptr::ThisPtr::new(this_socket) }; + let s = this_socket; s.ref_(); if let Some(default_data) = listener.strong_data.get().get() { - let global = listener.handlers.get().global_object; + let global = listener.handlers.global_object; NewSocket::::data_set_cached(s.get_this_value(&global), &global, default_data); } - this_socket + s } /// Called from `BunListener::on_open` (uws dispatch) for every accepted socket. @@ -586,7 +568,7 @@ impl Listener { pub fn on_create( listener: &Listener, socket: uws::NewSocketHandler, - ) -> *mut NewSocket { + ) -> bun_ptr::ThisPtr> { jsc::mark_binding!(); log!("onCreate"); @@ -594,7 +576,7 @@ impl Listener { let this_socket = NewSocket::::new(NewSocket:: { ref_count: bun_ptr::RefCount::init(), - handlers: Cell::new(NonNull::new(listener.handlers.as_ptr())), + handlers: JsCell::new(Some(Rc::clone(&listener.handlers))), socket: Cell::new(socket), protos: JsCell::new(listener.protos.clone()), // `protos` is `Option>` so each accepted socket clones @@ -612,18 +594,16 @@ impl Listener { native_callback: JsCell::new(crate::socket::NativeCallbacks::None), twin: JsCell::new(None), }); - // SAFETY: `NewSocket::new` returns a non-null live heap pointer - // (refcount==1); single JS thread, no other borrow exists yet. - let s = unsafe { bun_ptr::ThisPtr::new(this_socket) }; + let s = this_socket; s.ref_(); let default_data = listener.strong_data.get().get(); if let Some(default_data) = default_data { - let global = listener.handlers.get().global_object; + let global = listener.handlers.global_object; NewSocket::::data_set_cached(s.get_this_value(&global), &global, default_data); } if let Some(ctx) = socket.ext::<*mut c_void>() { // SAFETY: ext storage is at least pointer-sized; we stash *mut NewSocket - unsafe { *ctx = this_socket.cast::() }; + unsafe { *ctx = this_socket.as_ptr().cast::() }; } if let uws::InternalSocket::Connected(s) = socket.socket { // S008: `us_socket_t` is an `opaque_ffi!` ZST — safe deref. @@ -675,38 +655,35 @@ impl Listener { // node:tls passes the native SecureContext (already-built SSL_CTX*) — no // re-parse. Bun.listen({tls}) callers may still pass a raw options dict. - let sni_ctx: *mut boring_sys::SSL_CTX = if let Some(sc) = SecureContext::from_js(tls) { - // SAFETY: from_js returned non-null; SecureContext is live for the call. - unsafe { (*sc).borrow() } - } else if let Some(ssl_config) = { - // SAFETY: per-thread VM; valid for program lifetime. - let vm = VirtualMachine::get().as_mut(); - SSLConfig::from_js(vm, global, tls)? - } { - // Note: `cfg` cleanup handled by Drop on SSLConfig - let mut create_err = uws::create_bun_socket_error_t::none; - // SAFETY: `vm_ssl_ctx_cache()` returns the per-thread cache; only - // touched from the JS thread so the `&mut` is unique. - let cache = unsafe { &mut *vm_ssl_ctx_cache() }; - match cache.get_or_create(&ssl_config, &mut create_err) { - Some(ctx) => ctx, - None => { - if create_err != uws::create_bun_socket_error_t::none { - return Err(global.throw_value( - crate::socket::uws_jsc::create_bun_socket_error_to_js( - create_err, global, - ), - )); + let sni_ctx: *mut boring_sys::SSL_CTX = + if let Some(sc) = tls.as_class_ref::() { + sc.borrow() + } else if let Some(ssl_config) = { + // SAFETY: per-thread VM; valid for program lifetime. + let vm = VirtualMachine::get().as_mut(); + SSLConfig::from_js(vm, global, tls)? + } { + // Note: `cfg` cleanup handled by Drop on SSLConfig + let mut create_err = uws::create_bun_socket_error_t::none; + match with_ssl_ctx_cache(|cache| cache.get_or_create(&ssl_config, &mut create_err)) + { + Some(ctx) => ctx, + None => { + if create_err != uws::create_bun_socket_error_t::none { + return Err(global.throw_value( + crate::socket::uws_jsc::create_bun_socket_error_to_js( + create_err, global, + ), + )); + } + let code = boring_sys::ERR_get_error(); + return Err(global + .throw_value(crate::crypto::boringssl_jsc::err_to_js(global, code))); } - let code = boring_sys::ERR_get_error(); - return Err( - global.throw_value(crate::crypto::boringssl_jsc::err_to_js(global, code)) - ); } - } - } else { - return Ok(JSValue::UNDEFINED); - }; + } else { + return Ok(JSValue::UNDEFINED); + }; // The C SNI tree SSL_CTX_up_ref()s; drop our build/borrow ref once added. // S008: `ListenSocket` is an `opaque_ffi!` ZST — safe deref. @@ -767,10 +744,9 @@ impl Listener { Self::unlink_unix_socket_path(this); } - if this.handlers.get().active_connections.get() == 0 { + if this.handlers.active_connections.get() == 0 { this.poll_ref.with_mut(|p| p.unref(bun_io::js_vm_ctx())); - this.strong_self - .with_mut(|s| s.clear_without_deallocation()); + this.this_value.with_mut(|r| r.downgrade()); this.strong_data .with_mut(|s| s.clear_without_deallocation()); } else if force_close { @@ -838,15 +814,16 @@ impl Listener { log!("deinit"); // SAFETY: `this` is a Box leaked via into_raw; sole owner here let this_ref = unsafe { &mut *this }; - this_ref.strong_self.with_mut(|s| s.deinit()); + this_ref.this_value.with_mut(|r| r.finalize()); this_ref.strong_data.with_mut(|s| s.deinit()); this_ref.poll_ref.with_mut(|p| p.unref(bun_io::js_vm_ctx())); debug_assert!(matches!(this_ref.listener.get(), ListenerType::None)); - // Any still-open accepted sockets hold a `&listener.handlers` pointer, so - // we cannot free `this` while they're alive. Force-close them; their - // onClose paths will markInactive against handlers we drop right after. - if this_ref.handlers.get().active_connections.get() > 0 { + // Clear the back-pointer before force-closing: this listener is already + // releasing its own `poll_ref`/`this_value`, so an accepted socket's + // `on_close` must not reach back in and release them a second time. + this_ref.handlers.set_listener(None); + if this_ref.handlers.active_connections.get() > 0 { this_ref.group.with_mut(|g| g.close_all()); } bun_core::asan::unregister_root_region( @@ -860,15 +837,14 @@ impl Listener { unsafe { boring_sys::SSL_CTX_free(ctx.as_ptr()) }; } - // connection / protos: dropped by heap::take below - // Drop on Handlers handles unprotect. + // connection / protos / the handlers `Rc`: dropped by heap::take below // SAFETY: reclaim the Box allocated in listen() drop(unsafe { bun_core::heap::take(this) }); } #[bun_jsc::host_fn(getter)] pub fn get_connections_count(this: &Self, _global: &JSGlobalObject) -> JSValue { - JSValue::js_number(this.handlers.get().active_connections.get() as f64) + JSValue::js_number(this.handlers.active_connections.get() as f64) } #[bun_jsc::host_fn(getter)] @@ -919,7 +895,8 @@ impl Listener { return Ok(JSValue::UNDEFINED); } this.poll_ref.with_mut(|p| p.ref_(bun_io::js_vm_ctx())); - this.strong_self.with_mut(|s| s.set(global, this_value)); + this.this_value + .with_mut(|r| r.set_strong(this_value, global)); Ok(JSValue::UNDEFINED) } @@ -934,9 +911,8 @@ impl Listener { #[bun_jsc::host_fn(method)] pub fn unref(this: &Self, _global: &JSGlobalObject, _frame: &CallFrame) -> JsResult { this.poll_ref.with_mut(|p| p.unref(bun_io::js_vm_ctx())); - if this.handlers.get().active_connections.get() == 0 { - this.strong_self - .with_mut(|s| s.clear_without_deallocation()); + if this.handlers.active_connections.get() == 0 { + this.this_value.with_mut(|r| r.downgrade()); } Ok(JSValue::UNDEFINED) } @@ -958,13 +934,13 @@ impl Listener { } let vm = VirtualMachine::get().as_mut(); - // is_server=false: this is the client connect path. Handlers.mode must be - // .client so markInactive() takes the destroy branch — the - // .server branch recovers the containing Listener from the handlers - // field pointer, but here handlers live in a standalone heap - // allocation (see below), so that would read past the allocation. - let mut socket_config = SocketConfig::from_js(vm, opts, global, false)?; - // Note: `socket_config` cleanup (excluding handlers) handled by Drop on SocketConfig + // Client mode: these handlers have no owning listener, so + // `mark_inactive` skips the listener-release branch. + let mut socket_config = SocketConfig::from_js(vm, opts, global, SocketMode::Client)?; + // No JS wrapper holds the handlers cell until `connect_finish` creates + // the socket's; the option getters below run user JS that can GC. + let handlers = Rc::clone(&socket_config.handlers); + let _cell_root = handlers.root_cell(global); let port = socket_config.port; let ssl_enabled = socket_config.ssl.is_some(); @@ -979,8 +955,8 @@ impl Listener { break 'blk UnixOrHost::Fd(fd); } } - // Transfer the allocation out of `socket_config` so the later - // `mem::forget` doesn't leak it. + // Move the hostname bytes into `host`; `socket_config` drops the + // emptied slice. let host: Box<[u8]> = core::mem::take(&mut socket_config.hostname_or_unix) .into_vec() .into_boxed_slice(); @@ -1019,7 +995,7 @@ impl Listener { // `tls.secureContext` so we share its already-built SSL_CTX. let mut owned_ssl_ctx: Option> = None; if ssl_enabled { - let native_sc: Option<*mut SecureContext> = 'blk: { + let native_sc: Option<&SecureContext> = 'blk: { let Some(tls_js) = opts.get_truthy(global, "tls")? else { break 'blk None; }; @@ -1029,11 +1005,10 @@ impl Listener { let Some(sc_js) = tls_js.get_truthy(global, "secureContext")? else { break 'blk None; }; - SecureContext::from_js(sc_js) + sc_js.as_class_ref::() }; if let Some(sc) = native_sc { - // SAFETY: from_js returned non-null; SecureContext is live for the call. - owned_ssl_ctx = NonNull::new(unsafe { (*sc).borrow() }); + owned_ssl_ctx = NonNull::new(sc.borrow()); } } let mut ssl_ctx_guard = scopeguard::guard(owned_ssl_ctx, |c| { @@ -1090,44 +1065,18 @@ impl Listener { if is_named_pipe { default_data.ensure_still_alive(); - // Note: by-value move of Handlers — see `listen()` for rationale. - // SAFETY: socket_config.handlers is valid; we forget socket_config below. - let handlers_moved: Handlers = unsafe { core::ptr::read(&socket_config.handlers) }; let mut ssl_taken = socket_config.ssl.take(); - core::mem::forget(socket_config); - - let mut handlers_box = Box::new(handlers_moved); - handlers_box.mode = SocketMode::Client; let promise = jsc::JSPromise::create(global); let promise_value = promise.to_js(); - // Set on the `Box` before `into_raw` so no raw-deref is needed. - handlers_box - .promise - .with_mut(|p| p.set(global, promise_value)); - let handlers_ptr: *mut Handlers = bun_core::heap::into_raw(handlers_box); + handlers.set_promise(global, promise_value); if ssl_enabled { - let tls: *mut TLSSocket = if let Some(prev_ptr) = prev_maybe_tls { - // SAFETY: caller passes a live TLSSocket - let prev = unsafe { &*prev_ptr }; - if let Some(prev_handlers) = prev.handlers.get() { - if prev.flags.get().contains(SocketFlags::OWNS_HANDLERS) - // SAFETY: prev_handlers was heap-allocated; shared - // reborrow is scoped to this expression. - && unsafe { (*prev_handlers.as_ptr()).active_connections.get() } - == 0 - { - // SAFETY: prev_handlers was heap-allocated and unreferenced. - unsafe { drop(bun_core::heap::take(prev_handlers.as_ptr())) }; - } - } + let tls: bun_ptr::ThisPtr = if let Some(prev_ptr) = prev_maybe_tls { + // SAFETY: caller passes a live TLSSocket, owned by its JS wrapper. + let prev = unsafe { bun_ptr::ThisPtr::new(prev_ptr) }; debug_assert!(!prev.this_value.get().is_empty()); - prev.handlers.set(NonNull::new(handlers_ptr)); - // Same ownership rationale as `connect_finish`'s prev - // branch — see the comment there. - prev.flags - .set(prev.flags.get() | SocketFlags::OWNS_HANDLERS); + prev.set_handlers(global, Some(Rc::clone(&handlers))); debug_assert!(matches!( prev.socket.get().socket, uws::InternalSocket::Detached @@ -1143,11 +1092,11 @@ impl Listener { .set(ssl_taken.as_mut().and_then(|s| s.take_protos())); prev.server_name .set(ssl_taken.as_mut().and_then(|s| s.take_server_name())); - prev_ptr + prev } else { TLSSocket::new(TLSSocket { ref_count: bun_ptr::RefCount::init(), - handlers: Cell::new(NonNull::new(handlers_ptr)), + handlers: JsCell::new(Some(Rc::clone(&handlers))), socket: Cell::new(uws::NewSocketHandler::::DETACHED), connection: JsCell::new(Some(connection)), local_binding: JsCell::new(local_binding.clone()), @@ -1156,7 +1105,7 @@ impl Listener { ssl_taken.as_mut().and_then(|s| s.take_server_name()), ), owned_ssl_ctx: Cell::new(None), - flags: Cell::new(SocketFlags::default() | SocketFlags::OWNS_HANDLERS), + flags: Cell::new(SocketFlags::default()), this_value: JsCell::new(jsc::JsRef::empty()), poll_ref: JsCell::new(KeepAlive::init()), ref_pollref_on_connect: Cell::new(true), @@ -1166,8 +1115,7 @@ impl Listener { twin: JsCell::new(None), }) }; - // SAFETY: tls is a valid heap pointer - let tls_ref = unsafe { &*tls }; + let tls_ref = tls; TLSSocket::data_set_cached( tls_ref.get_this_value(global), global, @@ -1190,14 +1138,14 @@ impl Listener { &buf[..pipe_name_len.unwrap()], ssl_taken.take(), ctx_for_pipe, - PipeSocketType::Tls(tls), + PipeSocketType::Tls(tls_ref), ), UnixOrHost::Fd(fd) => WindowsNamedPipeContext::open( global, *fd, ssl_taken.take(), ctx_for_pipe, - PipeSocketType::Tls(tls), + PipeSocketType::Tls(tls_ref), ), _ => unreachable!(), }; @@ -1209,26 +1157,11 @@ impl Listener { socket: uws::InternalSocket::Pipe(named_pipe.cast()), }); } else { - let tcp: *mut TCPSocket = if let Some(prev_ptr) = prev_maybe_tcp { - // SAFETY: caller passes a live TCPSocket - let prev = unsafe { &*prev_ptr }; + let tcp: bun_ptr::ThisPtr = if let Some(prev_ptr) = prev_maybe_tcp { + // SAFETY: caller passes a live TCPSocket, owned by its JS wrapper. + let prev = unsafe { bun_ptr::ThisPtr::new(prev_ptr) }; debug_assert!(!prev.this_value.get().is_empty()); - if let Some(prev_handlers) = prev.handlers.get() { - if prev.flags.get().contains(SocketFlags::OWNS_HANDLERS) - // SAFETY: prev_handlers was heap-allocated; shared - // reborrow is scoped to this expression. - && unsafe { (*prev_handlers.as_ptr()).active_connections.get() } - == 0 - { - // SAFETY: prev_handlers was heap-allocated and unreferenced. - unsafe { drop(bun_core::heap::take(prev_handlers.as_ptr())) }; - } - } - prev.handlers.set(NonNull::new(handlers_ptr)); - // Same ownership rationale as `connect_finish`'s prev - // branch — see the comment there. - prev.flags - .set(prev.flags.get() | SocketFlags::OWNS_HANDLERS); + prev.set_handlers(global, Some(Rc::clone(&handlers))); debug_assert!(matches!( prev.socket.get().socket, uws::InternalSocket::Detached @@ -1241,18 +1174,18 @@ impl Listener { prev.local_binding.set(local_binding.clone()); debug_assert!(prev.protos.get().is_none()); debug_assert!(prev.server_name.get().is_none()); - prev_ptr + prev } else { TCPSocket::new(TCPSocket { ref_count: bun_ptr::RefCount::init(), - handlers: Cell::new(NonNull::new(handlers_ptr)), + handlers: JsCell::new(Some(Rc::clone(&handlers))), socket: Cell::new(uws::NewSocketHandler::::DETACHED), connection: JsCell::new(Some(connection)), local_binding: JsCell::new(local_binding.clone()), protos: JsCell::new(None), server_name: JsCell::new(None), owned_ssl_ctx: Cell::new(None), - flags: Cell::new(SocketFlags::default() | SocketFlags::OWNS_HANDLERS), + flags: Cell::new(SocketFlags::default()), this_value: JsCell::new(jsc::JsRef::empty()), poll_ref: JsCell::new(KeepAlive::init()), ref_pollref_on_connect: Cell::new(true), @@ -1262,8 +1195,7 @@ impl Listener { twin: JsCell::new(None), }) }; - // SAFETY: tcp is a valid heap pointer - let tcp_ref = unsafe { &*tcp }; + let tcp_ref = tcp; tcp_ref.ref_(); TCPSocket::data_set_cached( tcp_ref.get_this_value(global), @@ -1278,14 +1210,14 @@ impl Listener { &buf[..pipe_name_len.unwrap()], None, None, - PipeSocketType::Tcp(tcp), + PipeSocketType::Tcp(tcp_ref), ), UnixOrHost::Fd(fd) => WindowsNamedPipeContext::open( global, *fd, None, None, - PipeSocketType::Tcp(tcp), + PipeSocketType::Tcp(tcp_ref), ), _ => unreachable!(), }; @@ -1313,10 +1245,7 @@ impl Listener { // `requires_custom_request_ctx` gate is gone; the cache makes the // default-vs-custom distinction by content. let mut create_err = uws::create_bun_socket_error_t::none; - // SAFETY: `vm_ssl_ctx_cache()` returns the per-thread cache field - // inside the boxed `RuntimeState`; address-stable until VM teardown. - let cache = unsafe { &mut *vm_ssl_ctx_cache() }; - match cache.get_or_create(ssl_cfg, &mut create_err) { + match with_ssl_ctx_cache(|cache| cache.get_or_create(ssl_cfg, &mut create_err)) { Some(ctx) => { *ssl_ctx_guard = NonNull::new(ctx.cast::()); } @@ -1335,24 +1264,12 @@ impl Listener { default_data.ensure_still_alive(); - // Note: by-value move of Handlers. See `listen()` for rationale. - let mut socket_config = core::mem::ManuallyDrop::new(socket_config); - // SAFETY: socket_config.handlers is valid; ManuallyDrop suppresses the second drop. - let handlers_moved: Handlers = - unsafe { core::ptr::read(&raw const socket_config.handlers) }; let allow_half_open = socket_config.allow_half_open; let mut ssl_taken = socket_config.ssl.take(); - let mut handlers_box = Box::new(handlers_moved); - handlers_box.mode = SocketMode::Client; - let promise = jsc::JSPromise::create(global); let promise_value = promise.to_js(); - // Set on the `Box` before `into_raw` so no raw-deref is needed. - handlers_box - .promise - .with_mut(|p| p.set(global, promise_value)); - let handlers_ptr: *mut Handlers = bun_core::heap::into_raw(handlers_box); + handlers.set_promise(global, promise_value); // Ownership of the SSL_CTX is about to move into the socket; disarm the guard. let owned_ssl_ctx = scopeguard::ScopeGuard::into_inner(ssl_ctx_guard); @@ -1363,7 +1280,7 @@ impl Listener { connect_finish::( global, prev_maybe_tls, - handlers_ptr, + handlers, connection, local_binding, ssl_taken.as_mut(), @@ -1377,7 +1294,7 @@ impl Listener { connect_finish::( global, prev_maybe_tcp, - handlers_ptr, + handlers, connection, local_binding, ssl_taken.as_mut(), @@ -1407,8 +1324,8 @@ impl Listener { let mut buf = [0u8; 64]; let mut text_buf = [0u8; 512]; - // SAFETY: socket is non-null (Uws variant invariant). - let socket_ref = unsafe { &mut *socket }; + // S008: `ListenSocket` is an `opaque_ffi!` ZST — safe deref. + let socket_ref = bun_opaque::opaque_deref_mut(socket); let address_bytes: &[u8] = match socket_ref.get_local_address(&mut buf) { Ok(b) => b, Err(_) => return Ok(JSValue::UNDEFINED), @@ -1455,7 +1372,7 @@ impl Listener { fn connect_finish( global: &JSGlobalObject, maybe_previous: Option<*mut NewSocket>, - handlers_ptr: *mut Handlers, + handlers: Rc, connection: UnixOrHost, local_binding: Option<(Box<[u8]>, u16)>, mut ssl: Option<&mut SSLConfig>, @@ -1465,37 +1382,19 @@ fn connect_finish( port: Option, promise_value: JSValue, ) -> JsResult { - let socket: *mut NewSocket = if let Some(prev_ptr) = maybe_previous { - // SAFETY: caller passes a live NewSocket - let prev = unsafe { &*prev_ptr }; + let socket: bun_ptr::ThisPtr> = if let Some(prev_ptr) = maybe_previous { + // SAFETY: caller passes a live NewSocket, owned by its JS wrapper. + let prev = unsafe { bun_ptr::ThisPtr::new(prev_ptr) }; debug_assert!(prev.this_value.get().is_not_empty()); // `node:net` allows `socket.connect()` on an already-connected / // still-connecting socket. Close the previous native socket before // reusing this wrapper so `do_connect` does not alias two native // sockets onto one ext slot. prev.detach_for_reconnect(); - if let Some(prev_handlers) = prev.handlers.get() { - // Only free the previous Handlers when no callback scope is still - // holding it. If a `data`/`close` handler synchronously re-entered - // `connect`, `Scope::exit` (via `Handlers::mark_inactive`) frees it - // once the in-flight callback unwinds; freeing here would be a UAF. - if prev.flags.get().contains(SocketFlags::OWNS_HANDLERS) - // SAFETY: prev_handlers was heap-allocated; shared reborrow is - // scoped to this expression. - && unsafe { (*prev_handlers.as_ptr()).active_connections.get() } == 0 - { - // SAFETY: prev_handlers was heap-allocated and unreferenced. - unsafe { drop(bun_core::heap::take(prev_handlers.as_ptr())) }; - } - } - prev.handlers.set(NonNull::new(handlers_ptr)); - // `handlers_ptr` is a fresh `heap::alloc` box from `connect_inner`; - // this socket now owns it. Without the flag, `deinit_and_destroy` and - // `mark_inactive`'s shutdown gate skip the free and the box leaks - // (`node:net`'s `new_detached_socket` creates `prev` with default - // flags and no handlers). - prev.flags - .set(prev.flags.get() | SocketFlags::OWNS_HANDLERS); + // Dropping the previous `Rc` here is safe even mid-callback: a `Scope` + // from a `data`/`close` handler that synchronously re-entered `connect` + // still holds its own reference. + prev.set_handlers(global, Some(handlers)); debug_assert!(prev.socket.get().is_detached()); // Free old resources before reassignment to prevent memory leaks // when sockets are reused for reconnection (common with MongoDB driver) @@ -1512,18 +1411,18 @@ fn connect_finish( unsafe { boring_sys::SSL_CTX_free(old) }; } prev.owned_ssl_ctx.set(owned_ssl_ctx.map(|p| p.as_ptr())); - prev_ptr + prev } else { NewSocket::::new(NewSocket:: { ref_count: bun_ptr::RefCount::init(), - handlers: Cell::new(NonNull::new(handlers_ptr)), + handlers: JsCell::new(Some(handlers)), socket: Cell::new(uws::NewSocketHandler::::DETACHED), connection: JsCell::new(Some(connection)), local_binding: JsCell::new(local_binding), protos: JsCell::new(ssl.as_mut().and_then(|s| s.take_protos())), server_name: JsCell::new(ssl.as_mut().and_then(|s| s.take_server_name())), owned_ssl_ctx: Cell::new(owned_ssl_ctx.map(|p| p.as_ptr())), - flags: Cell::new(SocketFlags::default() | SocketFlags::OWNS_HANDLERS), + flags: Cell::new(SocketFlags::default()), this_value: JsCell::new(jsc::JsRef::empty()), poll_ref: JsCell::new(KeepAlive::init()), ref_pollref_on_connect: Cell::new(true), @@ -1533,10 +1432,8 @@ fn connect_finish( twin: JsCell::new(None), }) }; - // Ownership moved into `socket`; disarm the guard. - // (owned_ssl_ctx consumed above) - // SAFETY: socket is a valid heap pointer - let socket_ref = unsafe { &*socket }; + // Either the caller's JS-owned socket (reconnect) or the fresh one above. + let socket_ref = socket; socket_ref.ref_(); NewSocket::::data_set_cached(socket_ref.get_this_value(global), global, default_data); // On the reuse-prev path, `prev.this_value` was downgraded to Weak by the @@ -1593,13 +1490,11 @@ fn connect_finish( bun_sys::SystemErrno::ECONNREFUSED as c_int } }; - // SAFETY: `socket` is the live heap pointer; `socket_ref`'s `&mut` is no - // longer used on this branch. `handle_connect_error` takes `*mut Self` - // (noalias re-entrancy) — no `&mut NewSocket` held across its JS call. - unsafe { - let _ = NewSocket::::handle_connect_error(socket, errno, 0); + { + let this = socket; + let _ = NewSocket::::handle_connect_error(this, errno, 0); // Balance the unconditional `socket_ref.ref_()` above. - (*socket).deref(); + NewSocket::deref(&this); } return Ok(promise_value); } @@ -1624,15 +1519,8 @@ pub(crate) fn js_add_server_name(global: &JSGlobalObject, frame: &CallFrame) -> return Err(global.throw_not_enough_arguments("addServerName", 3, arguments.len)); } let listener = arguments.ptr[0]; - if let Some(this) = Listener::from_js(listener) { - // SAFETY: from_js returned a non-null *mut Listener; the JS wrapper holds it. - // R-2: deref as shared (`&*`) — `add_server_name` takes `&Self`. - return Listener::add_server_name( - unsafe { &*this }, - global, - arguments.ptr[1], - arguments.ptr[2], - ); + if let Some(this) = listener.as_class_ref::() { + return Listener::add_server_name(this, global, arguments.ptr[1], arguments.ptr[2]); } Err(global.throw(format_args!("Expected a Listener instance"))) } @@ -1882,30 +1770,29 @@ pub(crate) extern "C" fn us_dispatch_server_name( if ls.is_null() || hostname.is_null() { return core::ptr::null_mut(); } - // SAFETY: `ls` is live per the fn contract; the accept group's ext holds - // the owning `*mut Listener` for the lifetime of the listen socket. - let listener_ptr: *mut Listener = unsafe { (*ls).group().owner::() }; + // The accept group's ext holds the owning `*mut Listener` for the lifetime + // of the listen socket. S008: `ListenSocket` is an `opaque_ffi!` ZST. + let listener_ptr: *mut Listener = bun_opaque::opaque_deref_mut(ls).group().owner::(); if listener_ptr.is_null() { return core::ptr::null_mut(); } - // SAFETY: see above. - let listener: &Listener = unsafe { &*listener_ptr }; - // SAFETY: `handlers` is embedded in the live Listener. - let handlers = unsafe { &*listener.handlers.as_ptr() }; + // SAFETY: see above — the listen socket keeps the `Listener` alive for the + // duration of this synchronous handshake dispatch. + let listener = unsafe { bun_ptr::ThisPtr::new(listener_ptr) }; + let handlers = &listener.handlers; if handlers.vm.is_shutting_down() { return core::ptr::null_mut(); } - let callback = handlers.on_server_name; + let callback = handlers.on_server_name(); if callback.is_empty() { return core::ptr::null_mut(); } // No `Handlers::enter`/`exit` scope here: that protocol tracks the - // accepted-socket callback lifecycle (an exit returning true means "the - // socket died during the callback, free the handlers"), and running it - // against the listener's own handlers from inside the handshake corrupts - // their refcount for every subsequent accept. The listener and its - // embedded handlers are structurally alive for the duration of this - // synchronous dispatch - the listen socket cannot be freed mid-handshake. + // accepted-socket callback lifecycle, and running it against the listener's + // own handlers from inside the handshake corrupts `active_connections` for + // every subsequent accept. The listener and its handlers are structurally + // alive for this synchronous dispatch - the listen socket cannot be freed + // mid-handshake. let global = handlers.global_object; // Pass the listener's `data` (the owning net.Server) rather than minting a // JS wrapper for the Listener itself - `to_js` here would create a second @@ -1931,12 +1818,9 @@ pub(crate) extern "C" fn us_dispatch_server_name( // TLSSocket wrapper. let s_ref = uws_sys::us_socket_t::opaque_mut(socket.cast()); if s_ref.kind() == uws_sys::SocketKind::BunSocketTls { - let tls_ptr: *mut TLSSocket = *s_ref.ext::<*mut TLSSocket>(); - if tls_ptr.is_null() { - JSValue::UNDEFINED - } else { - // SAFETY: ext slot holds a live TLSSocket; single-threaded dispatch. - unsafe { &*tls_ptr }.get_this_value(&global) + match *s_ref.ext::>>() { + Some(tls) => tls.get_this_value(&global), + None => JSValue::UNDEFINED, } } else { JSValue::UNDEFINED @@ -1973,10 +1857,9 @@ pub(crate) extern "C" fn us_dispatch_server_name( if result.is_undefined_or_null() { return core::ptr::null_mut(); } - if let Some(sc) = SecureContext::from_js(result) { - // SAFETY: from_js returned non-null; the SecureContext is live for the - // call and SSL_set_SSL_CTX takes its own reference to the SSL_CTX. - return unsafe { (*sc).borrow() }.cast(); + if let Some(sc) = result.as_class_ref::() { + // `SSL_set_SSL_CTX` takes its own reference to the returned SSL_CTX. + return sc.borrow().cast(); } // Anything else is not a SecureContext: Node treats this as an invalid SNI // context and drops the connection. diff --git a/src/runtime/socket/WindowsNamedPipeContext.rs b/src/runtime/socket/WindowsNamedPipeContext.rs index f11d805c61fb..c421fa568fb7 100644 --- a/src/runtime/socket/WindowsNamedPipeContext.rs +++ b/src/runtime/socket/WindowsNamedPipeContext.rs @@ -46,10 +46,19 @@ pub struct WindowsNamedPipeContext { // `ref_()`/`deref()` are provided by `#[derive(CellRefCounted)]` above. pub type RefCount = bun_ptr::IntrusiveRc; +/// Reached from `on_close` → `Self::deref` while `WindowsNamedPipe::on_close` +/// still holds a live `&mut (*this).named_pipe` and uses it after we return, so +/// project raw fields only — same constraint as the `on_*` handlers below. fn schedule_deinit(this: *mut WindowsNamedPipeContext) { - // SAFETY: called from deref() when count hits zero; `this` still live until deinit_in_next_tick fires. - // Forward the raw pointer — do NOT autoref to `&mut *this` (see `deinit_in_next_tick`). - unsafe { WindowsNamedPipeContext::deinit_in_next_tick(this) }; + // SAFETY: called from `deref()` at count zero; `this` is live until the task fires. + // `task_event`/`vm`/`task` are disjoint from the caller's `&mut named_pipe`, and + // `vm` is `&'static` (JSC_BORROW) so `enqueue_task`'s `&mut` goes through a raw cast. + unsafe { + debug_assert!((*this).task_event != EventState::Deinit); + (*this).task_event = EventState::Deinit; + let vm = ptr::from_ref::((*this).vm).cast_mut(); + (*vm).enqueue_task(Task::init(ptr::addr_of_mut!((*this).task))); + } } #[repr(u8)] @@ -59,13 +68,13 @@ pub enum EventState { None, } -/// Raw -/// intrusive-refcounted pointers (see `NewSocket::ref_`/`deref`). `Copy` so -/// matching by value avoids `&self.socket` aliasing `&mut self.named_pipe`. +/// Intrusive-refcounted self-pointers into the wrapped JS socket (a *different* +/// allocation from this context, so `ThisPtr`'s `Deref` is sound on them). +/// `Copy` so matching by value avoids `&self.socket` aliasing `&mut self.named_pipe`. #[derive(Copy, Clone)] pub enum SocketType { - Tls(*mut TLSSocket), - Tcp(*mut TCPSocket), + Tls(bun_ptr::ThisPtr), + Tcp(bun_ptr::ThisPtr), None, } @@ -97,7 +106,7 @@ fn socket_from_named_pipe( } /// Dispatch a `SocketType` value to a single body written generically over -/// `NewSocket`. Binds the inner `*mut NewSocket<{true|false}>` as `$s` +/// `NewSocket`. Binds the inner `ThisPtr>` as `$s` /// and a per-arm `const $ssl: bool` so the body can call /// `NewSocket::on_x($s, socket_from_named_pipe::<$ssl>(..), ..)` once instead /// of hand-duplicating the `Tls`/`Tcp` arms. `SocketType::None` is a no-op. @@ -139,119 +148,134 @@ macro_rules! match_socket { // Instead each handler projects only the disjoint fields it needs (`socket`, // `is_open`, `global_this`) via raw-pointer place expressions, and passes // `addr_of_mut!((*this).named_pipe)` as a raw pointer without retagging. +/// Fails the pending connect and releases `create()`'s sole ref, unless +/// `disarm()` runs first. +struct FailAndRelease(Option<*mut WindowsNamedPipeContext>); + +impl FailAndRelease { + fn get(&mut self) -> *mut WindowsNamedPipeContext { + self.0.expect("guard already disarmed") + } + + fn disarm(mut self) -> *mut WindowsNamedPipeContext { + self.0.take().expect("guard already disarmed") + } +} + +impl Drop for FailAndRelease { + fn drop(&mut self) { + if let Some(this) = self.0.take() { + WindowsNamedPipeContext::fail_and_release(this); + } + } +} + impl WindowsNamedPipeContext { fn on_open(this: *mut Self) { - // SAFETY: `this` is the live ctx ptr registered in `create()`; `is_open` - // and `socket` are disjoint from the caller's `&mut named_pipe`. - unsafe { (*this).is_open = true }; - // SAFETY: `this` is live (see above); addr_of_mut! computes a raw field address without forming a reference. - let pipe = unsafe { ptr::addr_of_mut!((*this).named_pipe) }; - // SAFETY: `s` is kept alive by the +1 ref taken in `create()`. - // `on_open` takes `*mut Self` (noalias re-entrancy) — no `&mut`. - match_socket!(unsafe { (*this).socket }, |s: NewSocket| unsafe { - NewSocket::on_open(s, socket_from_named_pipe::(pipe)) - }); + // SAFETY: `this` is the live ctx ptr registered in `create()`; `is_open`, + // `socket` and the `named_pipe` field *address* are all reachable without + // forming a reference that overlaps the caller's `&mut named_pipe`. + let (socket, pipe) = unsafe { + (*this).is_open = true; + ((*this).socket, ptr::addr_of_mut!((*this).named_pipe)) + }; + match_socket!(socket, |s: NewSocket| NewSocket::on_open( + s, + socket_from_named_pipe::(pipe) + )); } fn on_data(this: *mut Self, decoded_data: &[u8]) { // SAFETY: see `on_open`. - let pipe = unsafe { ptr::addr_of_mut!((*this).named_pipe) }; - // SAFETY: see `on_open`. - match_socket!(unsafe { (*this).socket }, |s: NewSocket| unsafe { - NewSocket::on_data(s, socket_from_named_pipe::(pipe), decoded_data) - }); + let (socket, pipe) = unsafe { ((*this).socket, ptr::addr_of_mut!((*this).named_pipe)) }; + match_socket!(socket, |s: NewSocket| NewSocket::on_data( + s, + socket_from_named_pipe::(pipe), + decoded_data + )); } fn on_session(this: *mut Self, session: &[u8]) { // Only the TLS wrapper parks sessions; the TCP arm can never get here. // SAFETY: see `on_open`. if let SocketType::Tls(s) = unsafe { (*this).socket } { - // SAFETY: see `on_data`; `on_session` takes `*mut Self` - // (noalias re-entrancy) and routes JS errors internally. - let _ = unsafe { TLSSocket::on_session(s, session) }; + let _ = TLSSocket::on_session(s, session); } } fn on_keylog(this: *mut Self, line: &[u8]) { - // SAFETY: same as `on_session` above. + // SAFETY: see `on_open`. if let SocketType::Tls(s) = unsafe { (*this).socket } { - // SAFETY: same as `on_session` above. - let _ = unsafe { TLSSocket::on_keylog(s, line) }; + let _ = TLSSocket::on_keylog(s, line); } } fn on_handshake(this: *mut Self, success: bool, ssl_error: us_bun_verify_error_t) { // SAFETY: see `on_open`. - let pipe = unsafe { ptr::addr_of_mut!((*this).named_pipe) }; - // SAFETY: see `on_open`. - match_socket!(unsafe { (*this).socket }, |s: NewSocket| unsafe { - _ = NewSocket::on_handshake( - s, - socket_from_named_pipe::(pipe), - success as i32, - ssl_error, - ) - }); + let (socket, pipe) = unsafe { ((*this).socket, ptr::addr_of_mut!((*this).named_pipe)) }; + match_socket!(socket, |s: NewSocket| _ = NewSocket::on_handshake( + s, + socket_from_named_pipe::(pipe), + success as i32, + ssl_error + )); } fn on_end(this: *mut Self) { // SAFETY: see `on_open`. - let pipe = unsafe { ptr::addr_of_mut!((*this).named_pipe) }; - // SAFETY: see `on_open`. - match_socket!(unsafe { (*this).socket }, |s: NewSocket| unsafe { - NewSocket::on_end(s, socket_from_named_pipe::(pipe)) - }); + let (socket, pipe) = unsafe { ((*this).socket, ptr::addr_of_mut!((*this).named_pipe)) }; + match_socket!(socket, |s: NewSocket| NewSocket::on_end( + s, + socket_from_named_pipe::(pipe) + )); } fn on_writable(this: *mut Self) { // SAFETY: see `on_open`. - let pipe = unsafe { ptr::addr_of_mut!((*this).named_pipe) }; - // SAFETY: see `on_open`. - match_socket!(unsafe { (*this).socket }, |s: NewSocket| unsafe { - NewSocket::on_writable(s, socket_from_named_pipe::(pipe)) - }); + let (socket, pipe) = unsafe { ((*this).socket, ptr::addr_of_mut!((*this).named_pipe)) }; + match_socket!(socket, |s: NewSocket| NewSocket::on_writable( + s, + socket_from_named_pipe::(pipe) + )); } fn on_error(this: *mut Self, err: &SysError) { - // SAFETY: see `on_open`. `is_open`/`socket` are Copy; `global_this` is a - // disjoint field so a short-lived `&` does not touch `named_pipe`'s stack. - if unsafe { (*this).is_open } { - // SAFETY: see `on_open`. - match_socket!(unsafe { (*this).socket }, |s: NewSocket| { - // SAFETY: `this` is live; `global_this` is disjoint from the caller's `&mut named_pipe`. + // SAFETY: see `on_open`. `is_open`/`socket` are Copy field reads. + let (is_open, socket) = unsafe { ((*this).is_open, (*this).socket) }; + if is_open { + match_socket!(socket, |s: NewSocket| { + // SAFETY: `this` is live; `global_this` is disjoint from the caller's + // `&mut named_pipe` and the borrow ends before `handle_error` runs JS. let js_err = err.to_js(unsafe { &(*this).global_this }); - // SAFETY: see `on_open`. - unsafe { (*s).handle_error(js_err) }; + s.handle_error(js_err); }); } else { - // SAFETY: see `on_open`. - match_socket!(unsafe { (*this).socket }, |s: NewSocket| unsafe { - _ = NewSocket::handle_connect_error(s, err.errno as i32, 0) - }); + match_socket!(socket, |s: NewSocket| _ = + NewSocket::handle_connect_error(s, err.errno as i32, 0)); } } fn on_timeout(this: *mut Self) { // SAFETY: see `on_open`. - let pipe = unsafe { ptr::addr_of_mut!((*this).named_pipe) }; - // SAFETY: see `on_open`. - match_socket!(unsafe { (*this).socket }, |s: NewSocket| unsafe { - NewSocket::on_timeout(s, socket_from_named_pipe::(pipe)) - }); + let (socket, pipe) = unsafe { ((*this).socket, ptr::addr_of_mut!((*this).named_pipe)) }; + match_socket!(socket, |s: NewSocket| NewSocket::on_timeout( + s, + socket_from_named_pipe::(pipe) + )); } fn on_close(this: *mut Self) { // SAFETY: see `on_open`. Snapshot `socket` BEFORE clearing it, then match // the snapshot — the macro must not read `(*this).socket` directly here. - let socket = unsafe { (*this).socket }; - // SAFETY: `this` is live; `socket` is disjoint from the caller's `&mut named_pipe`. - unsafe { (*this).socket = SocketType::None }; - // SAFETY: `this` is live; addr_of_mut! computes a raw field address without forming a reference. - let pipe = unsafe { ptr::addr_of_mut!((*this).named_pipe) }; - // SAFETY: `s` held a +1 ref from `create()`; release it after dispatch. - match_socket!(socket, |s: NewSocket| unsafe { + let (socket, pipe) = unsafe { + let socket = (*this).socket; + (*this).socket = SocketType::None; + (socket, ptr::addr_of_mut!((*this).named_pipe)) + }; + match_socket!(socket, |s: NewSocket| { _ = NewSocket::on_close(s, socket_from_named_pipe::(pipe), 0, None); - (*s).deref(); + // Release the +1 ref taken in `create()`. + s.get().deref(); }); // SAFETY: `this` is the live ctx pointer registered in create(); // releasing the named-pipe's ref may schedule deinit. @@ -271,28 +295,22 @@ impl WindowsNamedPipeContext { } } - /// # Safety - /// `this` must be live. Takes a raw `*mut Self` (NOT `&mut self`): this is - /// reached from `on_close` → `Self::deref` → `schedule_deinit` while - /// `WindowsNamedPipe::on_close` still holds a live `&mut (*this).named_pipe` - /// and touches it again after the handler returns (`self.release_resources()`). - /// Forming `&mut *this` here would retag from the allocation root and pop - /// the caller's Unique tag — same Stacked-Borrows constraint as the eight - /// `on_*` handlers above. - unsafe fn deinit_in_next_tick(this: *mut Self) { - // SAFETY: `this` is live; `task_event`/`vm`/`task` are disjoint from - // the caller's `&mut named_pipe`. - debug_assert!(unsafe { (*this).task_event } != EventState::Deinit); - // SAFETY: `this` is live; `task_event` is disjoint from the caller's `&mut named_pipe`. - unsafe { (*this).task_event = EventState::Deinit }; - // SAFETY: `vm` is the process-global VirtualMachine; `enqueue_task` mutates - // its task queue. We hold `&'static VirtualMachine` (JSC_BORROW) so cast - // through a raw pointer to obtain the `&mut` the upstream API requires. - let vm = ptr::from_ref::(unsafe { (*this).vm }).cast_mut(); - // SAFETY: `this` is live; addr_of_mut! computes a raw field address without forming a reference. - let task = unsafe { ptr::addr_of_mut!((*this).task) }; - // SAFETY: `vm` points at the process-global VirtualMachine which outlives this call. - unsafe { (*vm).enqueue_task(Task::init(task)) }; + /// Owns the freshly-`create()`d context until `disarm()`: on any early + /// return it fails the pending connect and releases the sole ref. + fn armed(this: *mut Self) -> FailAndRelease { + FailAndRelease(Some(this)) + } + + /// errdefer shared by `open`/`connect`: fail the wrapped JS socket, then + /// release the only ref `create()` handed us. + fn fail_and_release(this: *mut Self) { + // SAFETY: `this` is live; `create()` returned it and no deref has fired yet. + // +1 ref held on the inner socket; live until `Self::deref` below. + match_socket!(unsafe { (*this).socket }, |s: NewSocket| _ = + NewSocket::handle_connect_error(s, SystemErrno::ENOENT as i32, 0)); + // SAFETY: `this` was just returned from `create()` (refcount==1); + // release the only ref on the errdefer path. + unsafe { Self::deref(this) }; } pub fn create( @@ -317,14 +335,10 @@ impl WindowsNamedPipeContext { // SAFETY: `p` is the `ctx` set above (`this.cast()`); the // WindowsNamedPipe never invokes a handler after `on_close` // schedules deinit, so the allocation is live for the call. - // Project `ref_count` directly via raw place — `(*p).ref_()` would + // `rc_ref` projects `ref_count` via raw place — `(*p).ref_()` would // autoref `&Self` over the whole struct, but `WindowsNamedPipe::r#ref` - // holds `&mut (*this).named_pipe` across this callback (same - // Stacked-Borrows constraint as the `on_*` handlers above). - ref_ctx: |p| unsafe { - let rc = &*ptr::addr_of!((*p.cast::()).ref_count); - rc.set(rc.get() + 1); - }, + // holds `&mut (*this).named_pipe` across this callback. + ref_ctx: |p| unsafe { ::rc_ref(p.cast::()) }, // SAFETY: `p` is the `ctx` set above (`this.cast()`); the allocation is live for the call (see `ref_ctx`). deref_ctx: |p| unsafe { Self::deref(p.cast::()) }, on_open: |p| Self::on_open(p.cast::()), @@ -380,8 +394,7 @@ impl WindowsNamedPipeContext { } // Take a +1 intrusive ref so the wrapped JS socket outlives this context. - // SAFETY: caller passes a live socket pointer; `ref_` only bumps the count. - match_socket!(socket, |s: NewSocket| unsafe { (*s).ref_() }); + match_socket!(socket, |s: NewSocket| s.ref_()); this } @@ -403,23 +416,13 @@ impl WindowsNamedPipeContext { let this = WindowsNamedPipeContext::create(global_this, socket); - // The error-path guard reaches `socket` through `this` because it was - // moved into `this` by `create()`. - let mut guard = scopeguard::guard(this, |this| { - // SAFETY: `this` is live; create() returned it and no deref has fired yet. - // +1 ref held on the inner socket; live until `Self::deref` below. - match_socket!(unsafe { (*this).socket }, |s: NewSocket| unsafe { - _ = NewSocket::handle_connect_error(s, SystemErrno::ENOENT as i32, 0) - }); - // SAFETY: `this` was just returned from `create()` (refcount==1); - // release the only ref on the errdefer path. - unsafe { Self::deref(this) }; - }); + // The guard reaches `socket` through `this`: `create()` moved it there. + let mut guard = Self::armed(this); // SAFETY: `this` is live and exclusively accessed here - unsafe { (**guard).named_pipe.open(fd, ssl_config, owned_ctx) }?; + unsafe { (*guard.get()).named_pipe.open(fd, ssl_config, owned_ctx) }?; - let this = scopeguard::ScopeGuard::into_inner(guard); + let this = guard.disarm(); // SAFETY: `this` is live; returning interior pointer to heap-allocated field (BACKREF) Ok(unsafe { ptr::addr_of_mut!((*this).named_pipe) }) } @@ -435,19 +438,10 @@ impl WindowsNamedPipeContext { // TODO: reuse the same context for multiple connections when possibles let this = WindowsNamedPipeContext::create(global_this, socket); - let mut guard = scopeguard::guard(this, |this| { - // SAFETY: `this` is live; create() returned it and no deref has fired yet. - // +1 ref held on the inner socket; live until `Self::deref` below. - match_socket!(unsafe { (*this).socket }, |s: NewSocket| unsafe { - _ = NewSocket::handle_connect_error(s, SystemErrno::ENOENT as i32, 0) - }); - // SAFETY: `this` was just returned from `create()` (refcount==1); - // release the only ref on the errdefer path. - unsafe { Self::deref(this) }; - }); + let mut guard = Self::armed(this); // SAFETY: `this` is live and exclusively accessed here - let named_pipe = unsafe { &mut (**guard).named_pipe }; + let named_pipe = unsafe { &mut (*guard.get()).named_pipe }; if path[path.len() - 1] == 0 { // is already null terminated @@ -466,7 +460,7 @@ impl WindowsNamedPipeContext { named_pipe.connect(slice_z, ssl_config, owned_ctx)?; } - let this = scopeguard::ScopeGuard::into_inner(guard); + let this = guard.disarm(); // SAFETY: `this` is live; returning interior pointer to heap-allocated field (BACKREF) Ok(unsafe { ptr::addr_of_mut!((*this).named_pipe) }) } @@ -478,8 +472,8 @@ impl Drop for WindowsNamedPipeContext { // Deref the wrapped socket, then let `named_pipe` drop. match_socket!( core::mem::replace(&mut self.socket, SocketType::None), - // SAFETY: +1 ref taken in `create()`; this is the matching release. - |s: NewSocket| unsafe { (*s).deref() } + // +1 ref taken in `create()`; this is the matching release. + |s: NewSocket| s.get().deref() ); // `named_pipe` drops via field destructor after this. } diff --git a/src/runtime/socket/mod.rs b/src/runtime/socket/mod.rs index 619807f2f15f..69b58abf4d79 100644 --- a/src/runtime/socket/mod.rs +++ b/src/runtime/socket/mod.rs @@ -15,6 +15,9 @@ pub mod socket_address; #[path = "Handlers.rs"] pub mod handlers; +#[path = "JSSocketHandlers.rs"] +pub mod js_socket_handlers; + #[path = "Listener.rs"] pub mod listener; @@ -115,71 +118,61 @@ pub mod socket { // ─── RawSocketEvents glue ──────────────────────────────────────────────────── // `uws_handlers::RawSocketEvents` is the raw-pointer dispatch trait the // vtable layer requires of `api::NewSocket` (routed via `RawPtrHandler`, -// not `PtrHandler`). Noalias re-entrancy: the inherent `on_*` -// methods take `this: *mut Self` precisely so no `&mut NewSocket` is held -// across `callback.call` (JS can re-derive `&mut Self` via the wrapper's -// `m_ptr` and mutate `flags`/`handlers`/`ref_count`); a `&mut self` argument -// formed here from the ext slot and protected through the dispatch frame would -// be aliasing UB. Bridge them here so the trait impl and the struct definition -// stay in their respective files. +// not `PtrHandler`). The handlers take `ThisPtr` rather than `&mut self`: +// a JS callback can close the socket and drop its last ref mid-dispatch, and a +// `&mut` argument protector outliving the allocation is UB. impl uws_handlers::RawSocketEvents for NewSocket { const HAS_ON_OPEN: bool = true; #[inline] - unsafe fn on_open(this: *mut Self, s: bun_uws::NewSocketHandler) { - // SAFETY: caller (RawPtrHandler) passes the live ext-slot pointer. - unsafe { NewSocket::on_open(this, s) }; + fn on_open(this: bun_ptr::ThisPtr, s: bun_uws::NewSocketHandler) { + NewSocket::on_open(this, s); } #[inline] - unsafe fn on_data(this: *mut Self, s: bun_uws::NewSocketHandler, data: &[u8]) { - // SAFETY: see `on_open`. - unsafe { NewSocket::on_data(this, s, data) }; + fn on_data(this: bun_ptr::ThisPtr, s: bun_uws::NewSocketHandler, data: &[u8]) { + NewSocket::on_data(this, s, data); } #[inline] - unsafe fn on_writable(this: *mut Self, s: bun_uws::NewSocketHandler) { - // SAFETY: see `on_open`. - unsafe { NewSocket::on_writable(this, s) }; + fn on_writable(this: bun_ptr::ThisPtr, s: bun_uws::NewSocketHandler) { + NewSocket::on_writable(this, s); } #[inline] - unsafe fn on_close( - this: *mut Self, + fn on_close( + this: bun_ptr::ThisPtr, s: bun_uws::NewSocketHandler, code: i32, reason: *mut core::ffi::c_void, ) { - // SAFETY: see `on_open`. - let _ = unsafe { - NewSocket::on_close( - this, - s, - code, - if reason.is_null() { None } else { Some(reason) }, - ) - }; + let _ = NewSocket::on_close( + this, + s, + code, + if reason.is_null() { None } else { Some(reason) }, + ); } #[inline] - unsafe fn on_timeout(this: *mut Self, s: bun_uws::NewSocketHandler) { - // SAFETY: see `on_open`. - unsafe { NewSocket::on_timeout(this, s) }; + fn on_timeout(this: bun_ptr::ThisPtr, s: bun_uws::NewSocketHandler) { + NewSocket::on_timeout(this, s); } #[inline] - unsafe fn on_end(this: *mut Self, s: bun_uws::NewSocketHandler) { - // SAFETY: see `on_open`. - unsafe { NewSocket::on_end(this, s) }; + fn on_end(this: bun_ptr::ThisPtr, s: bun_uws::NewSocketHandler) { + NewSocket::on_end(this, s); } #[inline] - unsafe fn on_connect_error(this: *mut Self, s: bun_uws::NewSocketHandler, code: i32) { - // SAFETY: see `on_open`. - let _ = unsafe { NewSocket::on_connect_error(this, s, code) }; + fn on_connect_error( + this: bun_ptr::ThisPtr, + s: bun_uws::NewSocketHandler, + code: i32, + ) { + let _ = NewSocket::on_connect_error(this, s, code); } #[inline] - unsafe fn on_handshake( - this: *mut Self, + fn on_handshake( + this: bun_ptr::ThisPtr, s: bun_uws::NewSocketHandler, ok: i32, err: bun_uws_sys::us_bun_verify_error_t, ) { - // SAFETY: see `on_open`. - let _ = unsafe { NewSocket::on_handshake(this, s, ok, err) }; + let _ = NewSocket::on_handshake(this, s, ok, err); } } diff --git a/src/runtime/socket/socket_body.rs b/src/runtime/socket/socket_body.rs index 05dee27b3a64..87aeb928f003 100644 --- a/src/runtime/socket/socket_body.rs +++ b/src/runtime/socket/socket_body.rs @@ -15,9 +15,7 @@ use bun_ptr::IntrusiveRc; use bun_boringssl_sys::SSL_CTX; use bun_collections::VecExt; use bun_core::{self, fmt as bun_fmt}; -use bun_jsc::{ - self as jsc, CallFrame, JSGlobalObject, JSValue, JsClass, JsRef, JsResult, SystemError, -}; +use bun_jsc::{self as jsc, CallFrame, JSGlobalObject, JSValue, JsRef, JsResult, SystemError}; // `err.to_js(global)` on `sys::Error` (the `SysErrorJsc` trait method) is only // reached from `#[cfg(not(windows))]` / `#[cfg(unix)]` blocks below. #[cfg(not(windows))] @@ -57,7 +55,7 @@ fn js_loop_ctx() -> bun_io::EventLoopCtx { // ────────────────────────────────────────────────────────────────────────── pub(super) use super::handlers::Handlers; -pub(super) use super::listener::Listener; +use std::rc::Rc; mod tls_socket_functions; use crate::api::bun::h2_frame_parser::H2FrameParser; @@ -91,13 +89,14 @@ extern "C" fn select_alpn_callback( if this_ptr.is_null() { return boringssl_sys::SSL_TLSEXT_ERR_NOACK; } - // SAFETY: ex_data slot 0 holds a `*mut TLSSocket` (set in on_open). - let this: &TLSSocket = unsafe { &*this_ptr.cast::() }; + // SAFETY: ex_data slot 0 holds a `*mut TLSSocket` (set in on_open), kept + // live for this handshake callback by the JS wrapper's ref. + let this = unsafe { bun_ptr::ThisPtr::new(this_ptr.cast::()) }; // Same handlers-presence guard as every other dispatch entry point: - // mark_inactive frees the per-connection Handlers, and the ALPN selection + // an idle socket has dropped its Handlers, and the ALPN selection // callback can still fire for a connection JS already detached - // get_handlers() would panic. NOACK falls through to the static list. - if this.handlers.get().is_none() { + if !this.has_handlers() { return boringssl_sys::SSL_TLSEXT_ERR_NOACK; } // Dynamic per-connection ALPN: when the listener's config carries an @@ -109,16 +108,16 @@ extern "C" fn select_alpn_callback( // same contract as Node's ALPNCallback. { let handlers = this.get_handlers(); - let callback = handlers.on_alpn_callback; + let callback = handlers.on_alpn_callback(); if !callback.is_empty() && !handlers.vm.is_shutting_down() && !in_.is_null() && inlen > 0 { - let scope = Handlers::enter_ref(handlers); + let scope = handlers.enter(); let global = handlers.global_object; let this_value = this.get_this_value(&global); let wire_len = inlen as usize; let buffer = match JSValue::create_buffer_from_length(&global, wire_len) { Ok(b) => b, Err(_) => { - this.exit_scope(scope, handlers); + this.exit_scope(scope); return boringssl_sys::SSL_TLSEXT_ERR_ALERT_FATAL; } }; @@ -159,13 +158,13 @@ extern "C" fn select_alpn_callback( tls_socket_functions::ffi::us_internal_ssl_loop_state_restore( saved_loop_state.as_mut_ptr(), ); - this.exit_scope(scope, handlers); + this.exit_scope(scope); return boringssl_sys::SSL_TLSEXT_ERR_ALERT_FATAL; } tls_socket_functions::ffi::us_internal_ssl_loop_state_restore( saved_loop_state.as_mut_ptr(), ); - this.exit_scope(scope, handlers); + this.exit_scope(scope); if !result.is_boolean() || result.to_boolean() { // The server has an ALPNCallback and it answered: a string // selects that protocol for this connection; anything else @@ -254,23 +253,16 @@ pub struct NewSocket { pub flags: Cell, pub ref_count: bun_ptr::RefCount, // intrusive — see `bun_ptr::IntrusiveRc` - // OWNERSHIP: in **server** mode this points at `&mut listener.handlers` - // (the embedded `Listener.handlers` field — Listener.rs:34) so - // `container_of`-style offset arithmetic in `get_listener` and - // `Handlers::mark_inactive` can recover the parent `Listener`. In - // **client** mode it is `heap::alloc(Box::new(Handlers))` and - // `Handlers::mark_inactive` frees it via `heap::take` once the last - // connection drops. - // - // ALIASING: this is intentionally a raw pointer, NOT `&mut`/`Rc`/ - // `Box`. JS dispatch is reentrant (`socket.reload()` overwrites the - // pointee while a callback frame still holds the pointer), so Rust's - // `&mut` exclusivity cannot be upheld across `callback.call()`. A raw - // pointer carries no aliasing guarantee to violate; callers reborrow - // `unsafe { &mut *p }` only for the exact field access they need and - // never across a reentrant JS call. See `get_handlers` for the access - // contract. - pub handlers: Cell>>, + /// The callbacks this socket dispatches to: shared with its listener and + /// sibling sockets (server), or with its own reconnects and TLS twin + /// (client). `None` once the socket has gone idle or been detached, which + /// every dispatch entry point treats as "nothing left to call". + /// + /// `Rc`, not a raw pointer: JS dispatch is reentrant (a `close` handler can + /// re-enter `connect` and repoint this field) and every in-flight callback + /// [`Scope`] holds its own reference, so the callbacks outlive the frame + /// that is running them. + pub handlers: JsCell>>, /// Reference to the JS wrapper. Held strong while the socket is active so the /// wrapper cannot be garbage-collected out from under in-flight callbacks, and /// downgraded to weak once the socket is closed/inactive so GC can reclaim it. @@ -315,6 +307,103 @@ impl bun_ptr::RefCounted for NewSocket { } } +/// Settles `IS_ACTIVE` against the `Handlers` the close callback entered with — +/// it may have synchronously reconnected onto a fresh set — then consumes the +/// +1 the caller transferred into `on_close`. +struct CloseTeardown { + socket: bun_ptr::ThisPtr>, + entered: Rc, +} + +impl Drop for CloseTeardown { + fn drop(&mut self) { + let this = self.socket; + if this.handlers_are(&self.entered) { + this.mark_inactive(); + } else if this.flags.get().contains(Flags::IS_ACTIVE) { + // Reconnected: `connect_finish` re-armed `this_value`/`poll_ref`, so + // skip the idle teardown and only release what we took. + this.update_flags(|f| f.remove(Flags::IS_ACTIVE)); + if !VirtualMachine::get().is_shutting_down() { + self.entered.mark_inactive(); + } + } + // Last: this can be the final ref, freeing the socket read above. + this.get().deref(); + } +} + +/// Drains the thread's BoringSSL error queue on scope exit, whichever way the +/// scope is left. +struct ClearErrorQueue(bool); + +impl Drop for ClearErrorQueue { + fn drop(&mut self) { + if self.0 { + boringssl_sys::ERR_clear_error(); + } + } +} + +/// The extra `SystemError` ref taken for the promise. `to_error_instance*` +/// consumes one ref of every string, so the promise needs its own copy; this +/// releases that copy on the paths that never build an error out of it. +struct PendingSystemError(Option); + +impl PendingSystemError { + fn take(&mut self) -> jsc::SystemError { + self.0.take().expect("PendingSystemError consumed twice") + } +} + +impl Drop for PendingSystemError { + fn drop(&mut self) { + if let Some(err) = self.0.take() { + err.deref(); + } + } +} + +/// `needs_deref` releases the ref the now-detached native socket held. The idle +/// teardown is gated on the socket still holding the `Handlers` we entered with: +/// `onConnectError` can reconnect, and we must not tear that connection down. +struct ConnectErrorTeardown { + socket: bun_ptr::ThisPtr>, + entered: Rc, + needs_deref: bool, +} + +impl Drop for ConnectErrorTeardown { + fn drop(&mut self) { + let this = self.socket; + // `deref` before `mark_inactive`, as the hand-rolled guard did. It + // cannot free the socket here: `handle_connect_error`'s `_keepalive` + // is declared before this guard, so it outlives it. + if self.needs_deref { + this.get().deref(); + } + if this.handlers_are(&self.entered) { + this.mark_inactive(); + } + } +} + +/// Balances a [`Handlers::enter`] on every exit path, including `?` returns. +/// Bind it to a named local — `let _ = ...` drops at the end of the statement, +/// running the exit before the user's callback. +struct ScopeExit { + socket: bun_ptr::ThisPtr>, + scope: Option, +} + +impl Drop for ScopeExit { + fn drop(&mut self) { + if let Some(scope) = self.scope.take() { + self.socket.exit_scope(scope); + } + } +} + impl NewSocket { // ─── R-2 interior-mutability helpers ───────────────────────────────────── @@ -399,9 +488,20 @@ impl NewSocket { js_TCPSocket::data_get_cached(this) } } + pub fn handlers_set_cached(this: JSValue, global: &JSGlobalObject, value: JSValue) { + jsc::mark_binding!(); + if SSL { + js_TLSSocket::handlers_set_cached(this, global, value); + } else { + js_TCPSocket::handlers_set_cached(this, global, value); + } + } - pub fn new(init: Self) -> *mut Self { - bun_core::heap::into_raw(Box::new(init)) + /// Heap-allocates the socket; ownership passes to the intrusive refcount. + /// The returned handle is live by construction. + pub fn new(init: Self) -> bun_ptr::ThisPtr { + // SAFETY: freshly allocated, non-null. + unsafe { bun_ptr::ThisPtr::new(bun_core::heap::into_raw(Box::new(init))) } } pub fn memory_cost(&self) -> usize { @@ -446,14 +546,10 @@ impl NewSocket { /// rather than taking it by-ref so the single caller in `connect_finish` /// doesn't need a disjoint borrow. pub fn do_connect(&self) -> Result<(), bun_core::Error> { - // Keep `self` alive across the - // re-entrant connect path. `ScopedRef` stores a raw `*mut Self` (no - // borrow held across the body) and derefs on Drop. - // SAFETY: `self` is live until guard drop; all writes go through - // interior-mutable cells. - let _guard = unsafe { bun_ptr::ScopedRef::new(self.as_ctx_ptr()) }; - // Stash the raw `*mut Self` for the uSockets ext slot. - let self_ptr: *mut Self = self.as_ctx_ptr(); + // Keep `self` alive across the re-entrant connect path. + // SAFETY: `self` is live for this call and outlives the sockets below. + let this = unsafe { bun_ptr::ThisPtr::new(self.as_ctx_ptr()) }; + let _guard = this.ref_guard(); let vm = self.get_handlers().vm; // SAFETY: per-thread VM singleton; `VirtualMachine::get()` yields the @@ -516,13 +612,11 @@ impl NewSocket { return Err(bun_core::err!("FailedToOpenSocket")); } uws::ConnectResult::Socket(s) => { - // SAFETY: ext slot is sized for `*mut Self`. - unsafe { *(*s).ext::<*mut Self>() = self_ptr }; + *uws::us_socket_t::opaque_mut(s).ext() = Some(this); SocketHandler::::from(s) } uws::ConnectResult::Connecting(c) => { - // SAFETY: ext slot is sized for `*mut Self`. - unsafe { *(*c).ext::<*mut Self>() = self_ptr }; + *uws::ConnectingSocket::opaque_mut(c).ext() = Some(this); SocketHandler::::from_connecting(c) } }, @@ -539,8 +633,7 @@ impl NewSocket { if s.is_null() { return Err(bun_core::err!("FailedToOpenSocket")); } - // SAFETY: ext slot is sized for `*mut Self`. - unsafe { *(*s).ext::<*mut Self>() = self_ptr }; + *uws::us_socket_t::opaque_mut(s).ext() = Some(this); self.socket.set(SocketHandler::::from(s)); } Some(UnixOrHost::Fd(f)) => { @@ -557,14 +650,10 @@ impl NewSocket { if s.is_null() { return Err(bun_core::err!("ConnectionFailed")); } - // SAFETY: ext slot is sized for `*mut Self`. - unsafe { *(*s).ext::<*mut Self>() = self_ptr }; + *uws::us_socket_t::opaque_mut(s).ext() = Some(this); let sock = SocketHandler::::from(s); self.socket.set(sock); - // SAFETY: the `&self.connection` match borrow has ended (NLL — - // `f` is unused past `from_fd`); `self_ptr` is the live - // `*mut Self`. `on_open` takes `*mut Self` (noalias re-entrancy). - unsafe { Self::on_open(self_ptr, sock) }; + Self::on_open(this, sock); } None => unreachable!("do_connect requires self.connection to be set"), } @@ -747,9 +836,10 @@ impl NewSocket { // owned SSL_CTX reference that us_socket_sni_resolve consumes) or null // to fall through to the listener's default context. let ctx_ptr = if args.len >= 1 && !is_error { - if let Some(sc) = crate::api::bun_secure_context::SecureContext::from_js(args.ptr[0]) { - // SAFETY: from_js returned a live SecureContext. - unsafe { (*sc).borrow() } + if let Some(sc) = + args.ptr[0].as_class_ref::() + { + sc.borrow() } else { core::ptr::null_mut() } @@ -769,37 +859,27 @@ impl NewSocket { } // the handlers must be kept alive for the duration of the function call // that way if we need to call the error handler, we can - let scope = Handlers::enter_ref(handlers); + let scope = handlers.enter(); let global = handlers.global_object; let this_value = self.get_this_value(&global); let _ = handlers.call_error_handler(this_value, &[this_value, err_value]); - self.exit_scope(scope, handlers); + self.exit_scope(scope); } - /// Noalias re-entrancy: takes `this: *mut Self`, NOT - /// `&mut self`. `callback.call(...)` re-enters JS which can call - /// `socket.write()`/`socket.end()`/`socket.reload()` on this same wrapper - /// via the JS object's `m_ptr`, re-deriving a `&mut NewSocket` and mutating + /// Takes `ThisPtr`, not `&mut self`: `callback.call(...)` re-enters + /// JS which can call `socket.write()`/`end()`/`reload()` on this same + /// wrapper via the JS object's `m_ptr`, re-deriving a borrow and mutating /// `flags`/`handlers`/`ref_count`/`buffered_data_for_node_net`. A live - /// noalias `&mut self` across that call lets LLVM cache those fields and - /// dead-store the re-entrant write (and is plain aliasing UB). Each - /// `(*this).foo()` materialises a short-lived borrow scoped to one - /// statement; none span `callback.call`. - /// - /// # Safety - /// `this` points at a live `NewSocket` (uws dispatch contract: the ext - /// slot holds the unique heap allocation); JS-thread only. - pub unsafe fn on_writable(this: *mut Self, _socket: SocketHandler) { + /// `&mut self` across that call is aliasing UB and lets LLVM cache those + /// fields and dead-store the re-entrant write. `ThisPtr` derefs yield a + /// short-lived shared borrow per access; none span `callback.call`. + pub fn on_writable(this: bun_ptr::ThisPtr, _socket: SocketHandler) { jsc::mark_binding!(); - // SAFETY: per fn contract; R-2 — every field is - // `Cell`/`JsCell`, so a single shared reborrow is sufficient and no - // borrow spans `callback.call`. - let this: &Self = unsafe { &*this }; - // A late event on a socket whose Handlers were already torn down - // (mark_inactive freed them through a path that did not route back - // through this dispatch - e.g. a JS-side destroy on a TLS socket - // driven by an upgraded duplex). There is nothing to dispatch to. - if this.handlers.get().is_none() { + // A late event on a socket that already released its Handlers through + // a path that did not route back through this dispatch - e.g. a + // JS-side destroy on a TLS socket driven by an upgraded duplex. There + // is nothing to dispatch to. + if !this.has_handlers() { return; } if this.socket.get().is_detached() { @@ -809,7 +889,7 @@ impl NewSocket { return; } let handlers = this.get_handlers(); - let callback = handlers.on_writable; + let callback = handlers.on_writable(); if callback.is_empty() { return; } @@ -818,8 +898,9 @@ impl NewSocket { if vm.is_shutting_down() { return; } - this.ref_(); - // reshaped for borrowck — explicit deref at end instead of a scope guard. + // Hold the socket alive for the rest of the dispatch: `internal_flush` + // and the drain callback can both re-enter JS and close it. + let _keepalive = this.ref_guard(); // NOTE: the drain dispatch deliberately does not depend on whether the // flush hit a fatal send error. Skipping it on fatal (tried in // f0325bddf2) made Windows servers reset FIN-terminated responses: @@ -835,36 +916,29 @@ impl NewSocket { ); // is not writable if we have buffered data or if we are already detached if this.buffered_data_for_node_net.get().len() > 0 || this.socket.get().is_detached() { - this.deref(); return; } // the handlers must be kept alive for the duration of the function call // that way if we need to call the error handler, we can - let scope = Handlers::enter_ref(handlers); + let scope = handlers.enter(); let global = handlers.global_object; let this_value = this.get_this_value(&global); if let Err(err) = callback.call(&global, this_value, &[this_value]) { let _ = handlers.call_error_handler(this_value, &[this_value, global.take_error(err)]); } - this.exit_scope(scope, handlers); - this.deref(); + this.exit_scope(scope); } - /// `*mut Self` for the same noalias-reentry reason as `on_writable`. - /// - /// # Safety - /// `this` points at a live `NewSocket`; JS-thread only. - pub unsafe fn on_timeout(this: *mut Self, _socket: SocketHandler) { + /// Takes `ThisPtr` for the same re-entrancy reason as `on_writable`. + pub fn on_timeout(this: bun_ptr::ThisPtr, _socket: SocketHandler) { jsc::mark_binding!(); - // SAFETY: per fn contract; R-2 shared reborrow. - let this: &Self = unsafe { &*this }; - // A late event on a socket whose Handlers were already torn down - // (mark_inactive freed them through a path that did not route back - // through this dispatch - e.g. a JS-side destroy on a TLS socket - // driven by an upgraded duplex). There is nothing to dispatch to. - if this.handlers.get().is_none() { + // A late event on a socket that already released its Handlers through + // a path that did not route back through this dispatch - e.g. a + // JS-side destroy on a TLS socket driven by an upgraded duplex. There + // is nothing to dispatch to. + if !this.has_handlers() { return; } if this.socket.get().is_detached() { @@ -879,7 +953,7 @@ impl NewSocket { "C" } ); - let callback = handlers.on_timeout; + let callback = handlers.on_timeout(); if callback.is_empty() || this.flags.get().contains(Flags::FINALIZING) { return; } @@ -889,62 +963,62 @@ impl NewSocket { // the handlers must be kept alive for the duration of the function call // that way if we need to call the error handler, we can - let scope = Handlers::enter_ref(handlers); + let scope = handlers.enter(); let global = handlers.global_object; let this_value = this.get_this_value(&global); if let Err(err) = callback.call(&global, this_value, &[this_value]) { let _ = handlers.call_error_handler(this_value, &[this_value, global.take_error(err)]); } - this.exit_scope(scope, handlers); + this.exit_scope(scope); } - /// Returns the raw, freely-aliased - /// pointer. **Do not** materialise a long-lived `&mut Handlers` from this - /// across any `callback.call(...)` / `resolve_promise` / `reject_promise` - /// boundary: JS may synchronously reenter `socket.reload()` which - /// `drop_in_place`s + `ptr::write`s the pointee, invalidating any - /// outstanding `&mut` under Stacked Borrows. Reborrow `unsafe { &mut *p }` - /// per field access (or re-derive after every reentrant call) instead. - /// - /// Server-mode: the returned pointer addresses the embedded - /// `Listener.handlers` field, so `container_of` arithmetic on it is - /// valid. Client-mode: the pointer is a `heap::alloc` allocation that - /// `Handlers::mark_inactive` may free — callers null `self.handlers` when - /// `mark_inactive`/`scope.exit()` returns `true`. + /// This socket's callbacks. Panics if it has none — every dispatch entry + /// point checks [`has_handlers`](Self::has_handlers) first. /// - /// Returned as a [`BackRef`](bun_ptr::BackRef) so the ~40 read-only field - /// projections at call sites go through `Deref` (one short-lived `&Handlers` - /// per expression — same Stacked-Borrows footprint as the previous manual - /// `unsafe { (*p).field }`). Mutating sites use `.as_ptr()` and reborrow - /// `&mut` explicitly. - pub fn get_handlers(&self) -> bun_ptr::BackRef { - self.handlers - .get() - .expect("No handlers set on Socket") - .into() + /// Returns a fresh `Rc`, so JS re-entrancy from the caller (a `close` + /// handler that reconnects, an `upgradeTLS` that transfers the handlers to + /// a twin) cannot free the callbacks the caller is still reading. + pub fn get_handlers(&self) -> Rc { + self.handlers_opt().expect("No handlers set on Socket") + } + + #[inline] + pub fn handlers_opt(&self) -> Option> { + self.handlers.get().clone() + } + + #[inline] + pub fn has_handlers(&self) -> bool { + self.handlers.get().is_some() + } + + /// True when this socket still points at `handlers` — false once a + /// re-entrant reconnect or `upgradeTLS` repointed it. + #[inline] + fn handlers_are(&self, handlers: &Rc) -> bool { + matches!(self.handlers.get(), Some(h) if Rc::ptr_eq(h, handlers)) + } + + #[inline] + fn take_handlers(&self) -> Option> { + self.handlers.with_mut(|h| h.take()) } /// The event-loop exit drains microtasks, during which a synchronous - /// reconnect may repoint `self.handlers` at a fresh allocation (null the - /// cell only when it still holds the `Handlers` the scope freed) or - /// `upgradeTLS` may transfer the handlers to the raw TLS twin. + /// reconnect may repoint `self.handlers` at a fresh `Handlers` — only + /// release the socket's own reference when it still holds the one we + /// entered with, which the `Scope` itself carries. #[inline] - fn exit_scope(&self, scope: super::handlers::Scope, entered: bun_ptr::BackRef) { - let captured = entered.as_ptr(); + fn exit_scope(&self, scope: super::handlers::Scope) { + let entered = Rc::clone(&scope.handlers); scope.exit_event_loop(); - // `upgradeTLS` can transfer client-mode handlers (and their - // `OWNS_HANDLERS` free) to the raw TLS twin from inside this callback, - // leaving `handlers` None; the twin frees them, so skip to avoid a double-free. - if self.handlers.get().is_none() && self.flags.get().contains(Flags::OWNS_HANDLERS) { - return; - } - if scope.mark_inactive() && self.handlers.get().map(|n| n.as_ptr()) == Some(captured) { + if scope.mark_inactive() && self.handlers_are(&entered) { self.handlers.set(None); } } - /// `*mut Self` for the same noalias-reentry reason as `on_writable` — + /// Takes `ThisPtr` for the same re-entrancy reason as `on_writable`: /// `callback.call`/`reject` re-enter JS which can `connectInner()`/mutate /// this socket via `m_ptr` (node:net `autoSelectFamily` retries inside the /// `connectError` callback). @@ -952,17 +1026,11 @@ impl NewSocket { /// `dns_error` is the raw `getaddrinfo(3)` return code when the name /// lookup itself failed; 0 for a connect failure past name resolution /// (then `errno` carries the connect error). - /// - /// # Safety - /// `this` points at a live `NewSocket`; JS-thread only. - pub unsafe fn handle_connect_error( - this: *mut Self, + pub fn handle_connect_error( + this: bun_ptr::ThisPtr, errno: c_int, dns_error: i32, ) -> JsResult<()> { - // SAFETY: per fn contract; R-2 — shared reborrow, all - // mutated fields are `Cell`/`JsCell`. - let this: &Self = unsafe { &*this }; let handlers = this.get_handlers(); log!( "onConnectError {} ({}, {})", @@ -974,15 +1042,10 @@ impl NewSocket { errno, this.ref_count.get() ); - // Ensure the socket is still alive for any defer's we have - this.ref_(); - // Declared before clear_and_free/unrefOnNextTick — its own guard so the - // ref_() above is balanced even if those calls unwind. - let _outer_deref = scopeguard::guard(this.as_ctx_ptr(), |p| { - // SAFETY: `p` is the live `*mut Self`; shared reborrow, fields celled. - unsafe { (*p).deref() }; - }); - // reshaped for borrowck — explicit cleanup at end of fn. + // Ensure the socket is still alive for any defer's we have. Declared + // before clear_and_free/unrefOnNextTick so the ref is balanced even if + // those calls unwind. + let _keepalive = this.ref_guard(); this.buffered_data_for_node_net .with_mut(|b| b.clear_and_free()); @@ -990,59 +1053,29 @@ impl NewSocket { this.socket.set(SocketHandler::::DETACHED); let vm = handlers.vm; - let _ = vm; this.poll_ref .with_mut(|p| p.unref_on_next_tick(js_loop_ctx())); - // The deferred `mark_inactive()` is gated on the `Handlers` pointer - // captured before the user callback runs: `onConnectError` can - // synchronously re-enter `connect()` and — via `do_connect()`'s - // `UnixOrHost::Fd` branch — reach `on_open()`/`mark_active()` for a - // *fresh* `Handlers` allocation before this guard drops. Without the - // gate the deferred `mark_inactive()` would tear down that newly - // activated connection. When no reconnect happened the socket never - // opened, so `IS_ACTIVE` is unset and the call is a no-op either way. - let pre_callback_handlers = handlers.as_ptr(); - let cleanup = scopeguard::guard( - (this.as_ctx_ptr(), needs_deref, pre_callback_handlers), - |(p, nd, h)| { - // SAFETY: `p` is the live `*mut Self`; shared reborrow, fields celled. - unsafe { - // Order: needs_deref → markInactive. - if nd { - (*p).deref(); - } - if (*p).handlers.get().map(|n| n.as_ptr()) == Some(h) { - (*p).mark_inactive(); - } - } - }, - ); + // The deferred `mark_inactive()` is gated on the `Handlers` captured + // before the user callback runs: `onConnectError` can synchronously + // re-enter `connect()` and — via `do_connect()`'s `UnixOrHost::Fd` + // branch — reach `on_open()`/`mark_active()` for a *fresh* `Handlers` + // before this guard drops. Without the gate the deferred + // `mark_inactive()` would tear down that newly activated connection. + // When no reconnect happened the socket never opened, so `IS_ACTIVE` + // is unset and the call is a no-op either way. + let cleanup = ConnectErrorTeardown { + socket: this, + entered: Rc::clone(&handlers), + needs_deref, + }; if vm.is_shutting_down() { - // The `cleanup` guard's `mark_inactive()` is a no-op for a socket - // that never opened (`IS_ACTIVE` unset), and at process exit the - // JS wrapper is typically still rooted by module scope so - // `deinit_and_destroy()`'s `OWNS_HANDLERS` cleanup never runs. - // That strands the per-connection `Handlers` box allocated in - // `connect_inner()`. Free it here so a connect that's aborted by - // `close_all_socket_groups()` at shutdown doesn't leak. - let flags = this.flags.get(); - if flags.contains(Flags::OWNS_HANDLERS) && !flags.contains(Flags::IS_ACTIVE) { - if let Some(h) = this.handlers.take() { - // SAFETY: `OWNS_HANDLERS` ⇒ `h` is this socket's own - // `heap::alloc` Handlers box. `!IS_ACTIVE` ⇒ neither the - // `cleanup` guard nor `Handlers::mark_inactive()` will - // touch it after this, and `take()` nulls the cell so - // `deinit_and_destroy()` can't double-free. - drop(unsafe { bun_core::heap::take(h.as_ptr()) }); - } - } drop(cleanup); return Ok(()); } - let callback = handlers.on_connect_error; + let callback = handlers.on_connect_error(); let global = handlers.global_object; // A failed name lookup is reported as the resolver error // (`getaddrinfo ENOTFOUND `, `syscall`/`hostname` set), @@ -1118,37 +1151,10 @@ impl NewSocket { } }; - // the handlers must be kept alive for the duration of the function call - // that way if we need to call the error handler, we can - let captured_handlers = handlers.as_ptr(); - let scope = Handlers::enter_ref(handlers); - // `let _ = guard` would drop *immediately* (end of - // statement, not end of scope) and run `scope.exit()` before the - // user's onConnectError callback. Bind to a named `_`-prefixed - // local so it lives to end of scope. - let _scope_guard = scopeguard::guard( - (this.as_ctx_ptr(), scope, captured_handlers), - |(p, sc, h)| { - if sc.exit() { - // Connection never opened (`is_active == false`), so the - // scope's decrement is what brings client handlers to zero - // and frees them. Null the field so a retry via - // `connectInner` doesn't double-free — but only if the - // cell still points at the Handlers `exit()` just freed: - // the `connectError` callback may have synchronously - // re-entered `connect()` (node:net `autoSelectFamily` - // retries) which already repointed the cell at a fresh - // allocation, and nulling it here would orphan the new - // Handlers and make the retry's `on_open` panic. - // SAFETY: `p` is the live `*mut Self`. - unsafe { - if (*p).handlers.get().map(|n| n.as_ptr()) == Some(h) { - (*p).handlers.set(None); - } - } - } - }, - ); + let _scope_guard = ScopeExit { + socket: this, + scope: Some(handlers.enter()), + }; if callback.is_empty() { // Connection failed before open; allow the wrapper to be GC'd @@ -1157,19 +1163,15 @@ impl NewSocket { if !matches!(this.this_value.get(), JsRef::Finalized) { this.this_value.with_mut(|r| r.downgrade()); } - // BackRef Deref → `&Handlers`; `promise: JsCell` so the - // swap/deinit go through interior mutability — no `&mut Handlers` - // held across the reentrant `reject` below. - if let Some(promise) = handlers.promise.with_mut(|p| p.try_swap()) { - handlers.promise.with_mut(|p| p.deinit()); - + if let Some(promise) = handlers.take_promise() { // reject the promise on connect() error - let js_promise: *mut jsc::JSPromise = promise.as_promise().unwrap(); - // SAFETY: `as_promise` returned non-null; promise lives for this call. - let err_value = - err.to_error_instance_with_async_stack(&global, unsafe { &*js_promise }); - // SAFETY: same — `reject` takes &mut self. - unsafe { (*js_promise).reject(&global, Ok(err_value)) }?; + let js_promise = jsc::JSPromise::opaque_mut(promise.as_promise().unwrap()); + let err_value = err.to_error_instance_with_async_stack(&global, js_promise); + js_promise.reject(&global, Ok(err_value))?; + } else { + // No callback and no promise (the duplex TLS upgrade flow): + // nothing consumed `err`, so release the strings it holds. + err.deref(); } return Ok(()); @@ -1181,6 +1183,7 @@ impl NewSocket { // callback returns. The on-stack `this_value` keeps it alive for the call. this.this_value.with_mut(|r| r.downgrade()); + let mut err_for_promise = PendingSystemError(Some(err.dupe())); let err_value = err.to_error_instance(&global); let result = match callback.call(&global, this_value, &[this_value, err_value]) { Ok(v) => v, @@ -1193,13 +1196,13 @@ impl NewSocket { return Ok(()); } let _ = handlers.call_error_handler(this_value, &[this_value, err_val]); - } else if let Some(val) = handlers.promise.with_mut(|p| p.try_swap()) { + } else if let Some(val) = handlers.take_promise() { // They've defined a `connectError` callback // The error is effectively handled, but we should still reject the promise. - // UFCS so rustc can back-infer `val: JSValue` even if the - // `promise` field's `try_swap()` resolution is in flux upstream. let promise = jsc::JSPromise::opaque_mut(JSValue::as_promise(val).unwrap()); - let err_ = err.to_error_instance_with_async_stack(&global, promise); + let err_ = err_for_promise + .take() + .to_error_instance_with_async_stack(&global, promise); promise.reject_as_handled(&global, err_)?; } @@ -1208,18 +1211,15 @@ impl NewSocket { Ok(()) } - /// `*mut Self` for the same noalias-reentry reason as `handle_connect_error`. - /// - /// # Safety - /// `this` points at a live `NewSocket`; JS-thread only. - pub unsafe fn on_connect_error( - this: *mut Self, + /// Takes `ThisPtr` for the same re-entrancy reason as + /// `handle_connect_error`. + pub fn on_connect_error( + this: bun_ptr::ThisPtr, socket: SocketHandler, errno: c_int, ) -> JsResult<()> { jsc::mark_binding!(); - // SAFETY: per fn contract. - unsafe { Self::handle_connect_error(this, errno, socket.dns_error()) } + Self::handle_connect_error(this, errno, socket.dns_error()) } pub fn mark_active(&self) { @@ -1276,10 +1276,8 @@ impl NewSocket { self.poll_ref.with_mut(|p| p.unref(js_loop_ctx())); if self.flags.get().contains(Flags::IS_ACTIVE) { self.update_flags(|f| f.remove(Flags::IS_ACTIVE)); - if let Some(h) = self.handlers.get() { - // SAFETY: `h` is the live client-mode `Handlers` box; see - // `Handlers::mark_inactive` contract. - if unsafe { Handlers::mark_inactive(h.as_ptr()) } { + if let Some(h) = self.handlers_opt() { + if h.mark_inactive() { self.handlers.set(None); } } @@ -1300,78 +1298,45 @@ impl NewSocket { self.update_flags(|f| f.remove(Flags::IS_ACTIVE)); // Allow the JS wrapper to be GC'd now that the socket is idle. - // Do this before touching `handlers`: in client mode - // `handlers.markInactive()` frees the Handlers allocation - // entirely, and for the last server-side connection on a - // stopped listener it releases the listener's own strong ref. + // Do this before touching `handlers`: for the last server-side + // connection on a stopped listener, `mark_inactive` releases the + // listener's own strong ref. if !matches!(self.this_value.get(), JsRef::Finalized) { self.this_value.with_mut(|r| r.downgrade()); } - // During VM shutdown, the Listener (which embeds `handlers` - // for server sockets) may already have been finalized by the - // time a deferred `onClose` → `markInactive` reaches here, - // leaving `this.handlers` dangling. Active-connection - // bookkeeping is irrelevant once the process is exiting, so - // just release the event-loop ref and stop. - // - // Client-mode (`OWNS_HANDLERS`) is exempt: `handlers` is this - // socket's own `heap::alloc` box, not a field of a Listener, so - // it cannot be finalized out from under us. Skipping the - // `Handlers::mark_inactive()` free strands that box — - // `close_all_socket_groups()` reaches here for every still-open - // client connection at exit and the test runner module scope - // typically still roots the JS wrapper, so the GC sweep never - // runs the `OWNS_HANDLERS` cleanup in `deinit_and_destroy`. - if VirtualMachine::get().is_shutting_down() - && !self.flags.get().contains(Flags::OWNS_HANDLERS) - { - self.poll_ref.with_mut(|p| p.unref(js_loop_ctx())); - return; - } - let handlers = self.get_handlers(); - // SAFETY: server-mode `handlers` points at the embedded - // `Listener.handlers` field, so `mark_inactive`'s - // `container_of` arithmetic is valid; client-mode it is the - // `heap::alloc` allocation `mark_inactive` frees in place. - if unsafe { Handlers::mark_inactive(handlers.as_ptr()) } { - // Client-mode handlers are allocated per-connection and - // `Handlers.markInactive` just freed them. Null the field - // so `connectInner` (net.Socket reconnect path) and - // `getListener` don't dereference/destroy freed memory. - self.handlers.set(None); + if let Some(handlers) = self.handlers_opt() { + if handlers.mark_inactive() { + // Nothing else is using these handlers. Drop this socket's + // reference so a later dispatch sees none rather than a + // callback table for a connection that is over. + self.handlers.set(None); + } } self.poll_ref.with_mut(|p| p.unref(js_loop_ctx())); } } pub fn is_server(&self) -> bool { - // `handlers` is null on detached sockets and on closed client - // sockets (markInactive nulls it once the allocation is freed). + // `handlers` is None on detached sockets and on closed client sockets. // JS-callable TLS accessors (`setServername`, `getPeerCertificate`, // `getEphemeralKeyInfo`, `setVerifyMode`) consult this on sockets // whose connection may already be gone. - let Some(handlers) = self.handlers.get() else { - return false; - }; - bun_ptr::BackRef::from(handlers).mode.is_server() + match self.handlers.get() { + Some(handlers) => handlers.mode.is_server(), + None => false, + } } - /// `*mut Self` for the same noalias-reentry reason as `on_writable` — + /// Takes `ThisPtr` for the same re-entrancy reason as `on_writable`: /// `resolve_promise`/`callback.call` re-enter JS which can mutate this /// socket via `m_ptr`. - /// - /// # Safety - /// `this` points at a live `NewSocket`; JS-thread only. - pub unsafe fn on_open(this: *mut Self, socket: SocketHandler) { - let this_ptr = this; - // SAFETY: per fn contract; R-2 — shared reborrow, all - // mutated fields are `Cell`/`JsCell`. - let this: &Self = unsafe { &*this }; - // A late event on a socket whose Handlers were already torn down - // (mark_inactive freed them through a path that did not route back - // through this dispatch - e.g. a JS-side destroy on a TLS socket - // driven by an upgraded duplex). There is nothing to dispatch to. - if this.handlers.get().is_none() { + pub fn on_open(this: bun_ptr::ThisPtr, socket: SocketHandler) { + let this_ptr = this.as_ptr(); + // A late event on a socket that already released its Handlers through + // a path that did not route back through this dispatch - e.g. a + // JS-side destroy on a TLS socket driven by an upgraded duplex. There + // is nothing to dispatch to. + if !this.has_handlers() { return; } log!( @@ -1381,9 +1346,8 @@ impl NewSocket { this.socket.get().is_detached(), this.ref_count.get() ); - // Ensure the socket remains alive until this is finished - this.ref_(); - // reshaped for borrowck — explicit deref at end. + // Ensure the socket remains alive: the callbacks below re-enter JS. + let _keepalive = this.ref_guard(); // update the internal socket instance to the one that was just connected // This socket must be replaced because the previous one is a connecting socket not a uSockets socket @@ -1431,7 +1395,7 @@ impl NewSocket { // never returns null for a live SSL. if this.is_server() && (this.protos.get().is_some() - || !this.get_handlers().on_alpn_callback.is_empty()) + || !this.get_handlers().on_alpn_callback().is_empty()) { let ssl_ref = boringssl_sys::SSL::opaque_ref(ssl_ptr); tls_socket_functions::ffi::SSL_set_ex_data( @@ -1476,8 +1440,8 @@ impl NewSocket { } let handlers = this.get_handlers(); - let callback = handlers.on_open; - let handshake_callback = handlers.on_handshake; + let callback = handlers.on_open(); + let handshake_callback = handlers.on_handshake(); let global = handlers.global_object; let this_value = this.get_this_value(&global); @@ -1491,19 +1455,17 @@ impl NewSocket { // If handshake is provided, open is called on connection open // If is not provided, open is called after handshake if callback.is_empty() || handshake_callback.is_empty() { - this.deref(); return; } } else { if callback.is_empty() { - this.deref(); return; } } // the handlers must be kept alive for the duration of the function call // that way if we need to call the error handler, we can - let scope = Handlers::enter_ref(handlers); + let scope = handlers.enter(); let result = match callback.call(&global, this_value, &[this_value]) { Ok(v) => v, Err(err) => global.take_exception(err), @@ -1540,7 +1502,7 @@ impl NewSocket { // subscription. let _ = this.internal_flush(); if this.buffered_data_for_node_net.get().len() == 0 { - let drain_callback = handlers.on_writable; + let drain_callback = handlers.on_writable(); if !drain_callback.is_empty() { if let Err(err) = drain_callback.call(&global, this_value, &[this_value]) { let _ = handlers @@ -1549,8 +1511,7 @@ impl NewSocket { } } } - this.exit_scope(scope, handlers); - this.deref(); + this.exit_scope(scope); } pub fn get_this_value(&self, global: &JSGlobalObject) -> JSValue { @@ -1564,24 +1525,38 @@ impl NewSocket { } let value = self.to_js(global); value.ensure_still_alive(); + // The wrapper holds the shared handlers cell in a visited slot, so the + // callbacks stay reachable from every socket that can still fire them. + // A detached socket has no handlers left to root. + if let Some(handlers) = self.handlers.get() { + Self::handlers_set_cached(value, global, handlers.cell()); + } // Hold strong until the socket is closed / marked inactive. self.this_value.with_mut(|r| r.set_strong(value, global)); value } - /// `*mut Self` for the same noalias-reentry reason as `on_writable`. - /// - /// # Safety - /// `this` points at a live `NewSocket`; JS-thread only. - pub unsafe fn on_end(this: *mut Self, _socket: SocketHandler) { + /// Points this socket at `handlers` and, when its JS wrapper already + /// exists (the `node:net` prev-socket reuse paths), stores the new cell in + /// the wrapper's visited slot. Fresh wrappers get it in + /// [`get_this_value`](Self::get_this_value). + pub fn set_handlers(&self, global: &JSGlobalObject, handlers: Option>) { + self.handlers.set(handlers); + if let (Some(handlers), Some(wrapper)) = + (self.handlers.get(), self.this_value.get().try_get()) + { + Self::handlers_set_cached(wrapper, global, handlers.cell()); + } + } + + /// Takes `ThisPtr` for the same re-entrancy reason as `on_writable`. + pub fn on_end(this: bun_ptr::ThisPtr, _socket: SocketHandler) { jsc::mark_binding!(); - // SAFETY: per fn contract; R-2 shared reborrow. - let this: &Self = unsafe { &*this }; - // A late event on a socket whose Handlers were already torn down - // (mark_inactive freed them through a path that did not route back - // through this dispatch - e.g. a JS-side destroy on a TLS socket - // driven by an upgraded duplex). There is nothing to dispatch to. - if this.handlers.get().is_none() { + // A late event on a socket that already released its Handlers through + // a path that did not route back through this dispatch - e.g. a + // JS-side destroy on a TLS socket driven by an upgraded duplex. There + // is nothing to dispatch to. + if !this.has_handlers() { return; } if this.socket.get().is_detached() { @@ -1597,50 +1572,43 @@ impl NewSocket { } ); // Ensure the socket remains alive until this is finished - this.ref_(); + let _keepalive = this.ref_guard(); - let callback = handlers.on_end; + let callback = handlers.on_end(); let vm = handlers.vm; if callback.is_empty() || vm.is_shutting_down() { this.poll_ref.with_mut(|p| p.unref(js_loop_ctx())); // If you don't handle TCP fin, we assume you're done. this.mark_inactive(); - this.deref(); return; } // the handlers must be kept alive for the duration of the function call // that way if we need to call the error handler, we can - let scope = Handlers::enter_ref(handlers); + let scope = handlers.enter(); let global = handlers.global_object; let this_value = this.get_this_value(&global); if let Err(err) = callback.call(&global, this_value, &[this_value]) { let _ = handlers.call_error_handler(this_value, &[this_value, global.take_error(err)]); } - this.exit_scope(scope, handlers); - this.deref(); + this.exit_scope(scope); } - /// `*mut Self` for the same noalias-reentry reason as `on_writable`. - /// - /// # Safety - /// `this` points at a live `NewSocket`; JS-thread only. - pub unsafe fn on_handshake( - this: *mut Self, + /// Takes `ThisPtr` for the same re-entrancy reason as `on_writable`. + pub fn on_handshake( + this: bun_ptr::ThisPtr, s: SocketHandler, success: i32, ssl_error: uws::us_bun_verify_error_t, ) -> JsResult<()> { jsc::mark_binding!(); - // SAFETY: per fn contract; R-2 shared reborrow. - let this: &Self = unsafe { &*this }; - // A late event on a socket whose Handlers were already torn down - // (mark_inactive freed them through a path that did not route back - // through this dispatch - e.g. a JS-side destroy on a TLS socket - // driven by an upgraded duplex). There is nothing to dispatch to. - if this.handlers.get().is_none() { + // A late event on a socket that already released its Handlers through + // a path that did not route back through this dispatch - e.g. a + // JS-side destroy on a TLS socket driven by an upgraded duplex. There + // is nothing to dispatch to. + if !this.has_handlers() { return Ok(()); } this.update_flags(|f| f.insert(Flags::HANDSHAKE_COMPLETE)); @@ -1690,7 +1658,7 @@ impl NewSocket { f.set(Flags::HOSTNAME_MISMATCH, hostname_mismatch); }); - let mut callback = handlers.on_handshake; + let mut callback = handlers.on_handshake(); let mut is_open = false; if handlers.vm.is_shutting_down() { @@ -1699,7 +1667,7 @@ impl NewSocket { // Use open callback when handshake is not provided if callback.is_empty() { - callback = handlers.on_open; + callback = handlers.on_open(); if callback.is_empty() { return Ok(()); } @@ -1708,7 +1676,7 @@ impl NewSocket { // the handlers must be kept alive for the duration of the function call // that way if we need to call the error handler, we can - let scope = Handlers::enter_ref(handlers); + let scope = handlers.enter(); let global = handlers.global_object; let this_value = this.get_this_value(&global); @@ -1727,12 +1695,7 @@ impl NewSocket { // clean onOpen callback so only called in the first handshake and not in every renegotiation // on servers this would require a different approach but it's not needed because our servers will not call handshake multiple times // servers don't support renegotiation - // SAFETY: short-lived `&mut` write; raw-ptr access is the - // ONLY way to mutate the freely-aliased `Handlers` here. - unsafe { - (*handlers.as_ptr()).on_open.unprotect(); - (*handlers.as_ptr()).on_open = JSValue::ZERO; - } + handlers.clear_on_open(); } } else { // call handhsake callback with authorized and authorization error if has one @@ -1744,7 +1707,7 @@ impl NewSocket { Err(e) => { // `Scope` has no Drop — balance event_loop().enter() and // active_connections before propagating. - this.exit_scope(scope, handlers); + this.exit_scope(scope); return Err(e); } } @@ -1763,7 +1726,7 @@ impl NewSocket { if let Some(err_value) = result.to_error() { let _ = handlers.call_error_handler(this_value, &[this_value, err_value]); } - this.exit_scope(scope, handlers); + this.exit_scope(scope); Ok(()) } @@ -1773,35 +1736,32 @@ impl NewSocket { /// Dispatched from `ssl_flush_pending_session()` after the SSL stack has /// unwound, so the JS handler may safely destroy the socket. /// - /// # Safety - /// `this` points at a live `NewSocket`; JS-thread only. - pub unsafe fn on_session(this: *mut Self, session: &[u8]) -> JsResult<()> { + /// Takes `ThisPtr` for the same re-entrancy reason as `on_writable`. + pub fn on_session(this: bun_ptr::ThisPtr, session: &[u8]) -> JsResult<()> { jsc::mark_binding!(); - // SAFETY: per fn contract; shared reborrow only. - let this: &Self = unsafe { &*this }; if this.socket.get().is_detached() { return Ok(()); } // Same late-event guard as the other dispatch entry points: the - // Handlers may already have been freed by mark_inactive. - if this.handlers.get().is_none() { + // socket may already have released its Handlers. + if !this.has_handlers() { return Ok(()); } let handlers = this.get_handlers(); if handlers.vm.is_shutting_down() { return Ok(()); } - let callback = handlers.on_session; + let callback = handlers.on_session(); if callback.is_empty() { return Ok(()); } - let scope = Handlers::enter_ref(handlers); + let scope = handlers.enter(); let global = handlers.global_object; let this_value = this.get_this_value(&global); let buffer = match JSValue::create_buffer_from_length(&global, session.len()) { Ok(b) => b, Err(e) => { - this.exit_scope(scope, handlers); + this.exit_scope(scope); return Err(e); } }; @@ -1819,41 +1779,36 @@ impl NewSocket { if let Some(err_value) = result.to_error() { let _ = handlers.call_error_handler(this_value, &[this_value, err_value]); } - this.exit_scope(scope, handlers); + this.exit_scope(scope); Ok(()) } - /// `*mut Self` for the same noalias-reentry reason as `on_session`. - /// - /// # Safety - /// `this` points at a live `NewSocket`; JS-thread only. - pub unsafe fn on_keylog(this: *mut Self, line: &[u8]) -> JsResult<()> { + /// Takes `ThisPtr` for the same re-entrancy reason as `on_writable`. + pub fn on_keylog(this: bun_ptr::ThisPtr, line: &[u8]) -> JsResult<()> { jsc::mark_binding!(); - // SAFETY: per fn contract; shared reborrow only. - let this: &Self = unsafe { &*this }; if this.socket.get().is_detached() { return Ok(()); } // Same late-event guard as the other dispatch entry points: the - // Handlers may already have been freed by mark_inactive. - if this.handlers.get().is_none() { + // socket may already have released its Handlers. + if !this.has_handlers() { return Ok(()); } let handlers = this.get_handlers(); if handlers.vm.is_shutting_down() { return Ok(()); } - let callback = handlers.on_keylog; + let callback = handlers.on_keylog(); if callback.is_empty() { return Ok(()); } - let scope = Handlers::enter_ref(handlers); + let scope = handlers.enter(); let global = handlers.global_object; let this_value = this.get_this_value(&global); let buffer = match JSValue::create_buffer_from_length(&global, line.len()) { Ok(b) => b, Err(e) => { - this.exit_scope(scope, handlers); + this.exit_scope(scope); return Err(e); } }; @@ -1871,35 +1826,29 @@ impl NewSocket { if let Some(err_value) = result.to_error() { let _ = handlers.call_error_handler(this_value, &[this_value, err_value]); } - this.exit_scope(scope, handlers); + this.exit_scope(scope); Ok(()) } - /// `*mut Self` for the same noalias-reentry reason as `on_writable`. - /// - /// # Safety - /// `this` points at a live `NewSocket`; JS-thread only. - pub unsafe fn on_close( - this: *mut Self, + /// Takes `ThisPtr` for the same re-entrancy reason as `on_writable`. + pub fn on_close( + this: bun_ptr::ThisPtr, socket: SocketHandler, err: c_int, reason: Option<*mut c_void>, ) -> JsResult<()> { jsc::mark_binding!(); - // SAFETY: per fn contract; R-2 shared reborrow. - let this: &Self = unsafe { &*this }; - // A late close on a socket whose Handlers were already torn down - // (mark_inactive freed them through a path that did not route back - // through this dispatch - e.g. a JS-side destroy on a TLS socket - // driven by an upgraded duplex). There is nothing to dispatch to, - // but the caller transferred its +1 (the ext-slot/owner pin) - - // release it and detach so nothing further dispatches either. - // mark_inactive is not needed: handlers being null means the - // previous teardown already ran it (it is what nulls the field). - if this.handlers.get().is_none() { + // A late close on a socket that already released its Handlers through + // a path that did not route back through this dispatch - e.g. a + // JS-side destroy on a TLS socket driven by an upgraded duplex. There + // is nothing to dispatch to, but the caller transferred its +1 (the + // ext-slot/owner pin) - release it and detach so nothing further + // dispatches either. mark_inactive is not needed: handlers being + // null means the previous teardown already ran it. + if !this.has_handlers() { this.detach_native_callback(); this.socket.set(SocketHandler::::DETACHED); - this.deref(); + this.get().deref(); return Ok(()); } let handlers = this.get_handlers(); @@ -1918,60 +1867,15 @@ impl NewSocket { // here, then retire it. `raw.twin == None` so this doesn't // recurse, and `onClose` derefs the +1 we took at creation. if let Some(raw) = this.twin.with_mut(|t| t.take()) { - let raw = IntrusiveRc::into_raw(raw); - // SAFETY: twin holds a +1 intrusive ref; uniquely accessed here. - // `on_close` itself runs `this.deref()` (via the cleanup guard), - // which releases that +1 — so hand it the raw pointer instead of - // letting `IntrusiveRc::drop` release a *second* time. - unsafe { Self::on_close(raw, socket, err, reason).ok() }; - } - // reshaped for borrowck — deref + markInactive run explicitly at the end. - // Capture the `Handlers` pointer that was paired with the - // `IS_ACTIVE` flag *before* the user's close callback runs. The - // callback may synchronously re-enter `Bun.connect({ socket: this })` - // (the documented MongoDB-driver reconnect path), which makes - // `connect_finish` repoint `self.handlers` at a fresh allocation - // while keeping the old one alive for the in-flight `Scope`. If the - // deferred `mark_inactive()` re-read the cell at that point it would - // (a) underflow the new `Handlers`' counter (created with - // `active_connections == 0`) and (b) leak the old one with its - // `protect()`'d JS callbacks orphaned at count 1. - let captured_handlers = handlers.as_ptr(); - let cleanup = scopeguard::guard((this.as_ctx_ptr(), captured_handlers), |(p, h)| { - // SAFETY: `p` is the live `*mut Self`; shared reborrow, fields celled. - unsafe { - let this_ref = &*p; - if this_ref.handlers.get().map(|n| n.as_ptr()) == Some(h) { - // Normal close: the cell still points at the Handlers we - // captured; do the full idle teardown. - this_ref.mark_inactive(); - } else if this_ref.flags.get().contains(Flags::IS_ACTIVE) { - // The close callback synchronously reconnected. The - // socket is not going idle — `connect_finish` already - // re-upgraded `this_value` and re-armed `poll_ref` for - // the in-flight connect — so skip the idle teardown. - // Just clear `IS_ACTIVE` (the next `on_open` re-arms it - // against the new Handlers) and release the lifecycle - // ref `mark_active` took on the *captured* Handlers so - // it can reach zero in `Scope::exit` instead of leaking. - this_ref.update_flags(|f| f.remove(Flags::IS_ACTIVE)); - let vm = VirtualMachine::get(); - // SAFETY: VM singleton is always live once initialized. - if !(*vm).is_shutting_down() { - // SAFETY: `h` is still live. The lifecycle ref - // released here is the one `mark_active` took at - // `on_open`, so `active_connections >= 1` until this - // call. `connect_finish` saw a non-zero count and - // therefore kept `h` allocated; `scope.exit()` (which - // ran just before this guard dropped) decremented the - // `enter_ref` ref but cannot have freed `h` while this - // lifecycle ref is outstanding. - let _ = Handlers::mark_inactive(h); - } - } - (*p).deref(); - } - }); + // `on_close` consumes the twin's +1 via its `CloseTeardown`, so + // hand over the raw pointer rather than letting `IntrusiveRc::drop` + // release it a second time. + Self::on_close(raw.into_this_ptr(), socket, err, reason).ok(); + } + let cleanup = CloseTeardown { + socket: this, + entered: Rc::clone(&handlers), + }; if this.flags.get().contains(Flags::FINALIZING) { drop(cleanup); @@ -1981,7 +1885,7 @@ impl NewSocket { let vm = handlers.vm; this.poll_ref.with_mut(|p| p.unref(js_loop_ctx())); - let callback = handlers.on_close; + let callback = handlers.on_close(); if callback.is_empty() { drop(cleanup); @@ -1995,7 +1899,7 @@ impl NewSocket { // the handlers must be kept alive for the duration of the function call // that way if we need to call the error handler, we can - let scope = Handlers::enter_ref(handlers); + let scope = handlers.enter(); let global = handlers.global_object; let this_value = this.get_this_value(&global); @@ -2019,39 +1923,28 @@ impl NewSocket { if let Err(e) = callback.call(&global, this_value, &[this_value, js_error]) { let _ = handlers.call_error_handler(this_value, &[this_value, global.take_error(e)]); } - if scope.exit() { - // Only null if the cell still points at the Handlers `exit()` - // just freed — a synchronous reconnect from inside the close - // callback already repointed it at a fresh allocation, and - // nulling it here would orphan the new Handlers and make the - // pending `on_open`'s `get_handlers()` panic on `None`. - if this.handlers.get().map(|n| n.as_ptr()) == Some(captured_handlers) { - this.handlers.set(None); - } - } + this.exit_scope(scope); drop(cleanup); Ok(()) } - /// `*mut Self` for the same noalias-reentry reason as `on_writable`. - /// - /// # Safety - /// `this` points at a live `NewSocket`; JS-thread only. - pub unsafe fn on_data(this: *mut Self, s: SocketHandler, data: &[u8]) { + /// Takes `ThisPtr` for the same re-entrancy reason as `on_writable`. + pub fn on_data(this: bun_ptr::ThisPtr, s: SocketHandler, data: &[u8]) { jsc::mark_binding!(); - // SAFETY: per fn contract; R-2 shared reborrow. - let this: &Self = unsafe { &*this }; - // A late event on a socket whose Handlers were already torn down - // (mark_inactive freed them through a path that did not route back - // through this dispatch - e.g. a JS-side destroy on a TLS socket - // driven by an upgraded duplex). There is nothing to dispatch to. - if this.handlers.get().is_none() { + // A late event on a socket that already released its Handlers through + // a path that did not route back through this dispatch - e.g. a + // JS-side destroy on a TLS socket driven by an upgraded duplex. There + // is nothing to dispatch to. + if !this.has_handlers() { return; } this.socket.set(s); if this.socket.get().is_detached() { return; } + if this.native_callback.get().on_data(data) { + return; + } let handlers = this.get_handlers(); log!( "onData {} ({})", @@ -2062,11 +1955,8 @@ impl NewSocket { }, data.len() ); - if this.native_callback.get().on_data(data) { - return; - } - let callback = handlers.on_data; + let callback = handlers.on_data(); if callback.is_empty() || this.flags.get().contains(Flags::FINALIZING) { return; } @@ -2076,7 +1966,7 @@ impl NewSocket { let global = handlers.global_object; let this_value = this.get_this_value(&global); - let output_value = match handlers.binary_type.to_js(data, &global) { + let output_value = match handlers.binary_type.get().to_js(data, &global) { Ok(v) => v, Err(err) => { this.handle_error(global.take_exception(err)); @@ -2086,13 +1976,13 @@ impl NewSocket { // the handlers must be kept alive for the duration of the function call // that way if we need to call the error handler, we can - let scope = Handlers::enter_ref(handlers); + let scope = handlers.enter(); // const encoding = handlers.encoding; if let Err(err) = callback.call(&global, this_value, &[this_value, output_value]) { let _ = handlers.call_error_handler(this_value, &[this_value, global.take_error(err)]); } - this.exit_scope(scope, handlers); + this.exit_scope(scope); } #[bun_jsc::host_fn(getter)] @@ -2113,25 +2003,19 @@ impl NewSocket { let Some(handlers) = this.handlers.get() else { return JSValue::UNDEFINED; }; - let handlers = bun_ptr::BackRef::from(handlers); if handlers.mode != super::SocketMode::Server || this.socket.get().is_detached() { return JSValue::UNDEFINED; } - // Server-mode - // `this.handlers` is set to `&mut listener.handlers` (the embedded - // `Listener.handlers` field — Listener.rs:34 / Listener.rs on_create), - // so subtracting the field offset recovers the parent `Listener*`. - // This is ONLY valid because `NewSocket.handlers` is a raw - // `NonNull` pointing into the `Listener` allocation; an - // `Rc`/`Box` payload would break the invariant. - // - // SAFETY: server-mode invariant (checked above) guarantees `handlers` - // addresses `Listener.handlers`. - let l: &Listener = - unsafe { &*bun_core::from_field_ptr!(Listener, handlers, handlers.as_ptr()) }; - l.strong_self.get().get().unwrap_or(JSValue::UNDEFINED) + let Some(listener) = handlers.listener() else { + return JSValue::UNDEFINED; + }; + listener + .this_value + .get() + .try_get() + .unwrap_or(JSValue::UNDEFINED) } #[bun_jsc::host_fn(getter)] @@ -2476,8 +2360,9 @@ impl NewSocket { } let args = callframe.arguments_undef::<2>(); - this.ref_(); - // reshaped for borrowck — explicit deref at end. + // `write_or_end_buffered` reaches `internal_flush`, which re-enters JS. + // SAFETY: the JS wrapper holds a ref for the whole host-fn call. + let _keepalive = unsafe { bun_ptr::ScopedRef::new(this.as_ctx_ptr()) }; let result = match this.write_or_end_buffered::(global, args.ptr[0], args.ptr[1]) { WriteResult::Fail => JSValue::ZERO, WriteResult::Success { wrote, total } => { @@ -2488,7 +2373,6 @@ impl NewSocket { JSValue::from(usize::try_from(wrote.max(0)).expect("int cast") == total) } }; - this.deref(); Ok(result) } @@ -2965,9 +2849,9 @@ impl NewSocket { // peer's close_notify arrives — leaving `is_active` set so the eventual // `onClose` can run `handlers.markInactive()`. Without this guard a // follow-up `flush()` re-enters `markInactive`, sees the detached - // socket as closed, and frees `*Handlers` early; the deferred `onClose` - // then derefs freed memory. Every other `internalFlush` caller already - // has this check. + // socket as closed, and decrements `active_connections` a second time; + // the deferred `onClose` then underflows it. Every other + // `internalFlush` caller already has this check. if this.socket.get().is_detached() { return Ok(JSValue::UNDEFINED); } @@ -3085,9 +2969,9 @@ impl NewSocket { return Ok(JSValue::js_number(-1.0)); } - this.ref_(); - // reshaped for borrowck — explicit deref at end. - + // `write_or_end` reaches `internal_flush`, which re-enters JS. + // SAFETY: the JS wrapper holds a ref for the whole host-fn call. + let _keepalive = unsafe { bun_ptr::ScopedRef::new(this.as_ctx_ptr()) }; let result = match this.write_or_end::(global, args.mut_(), false) { WriteResult::Fail => JSValue::ZERO, WriteResult::Success { wrote, total } => { @@ -3097,7 +2981,6 @@ impl NewSocket { JSValue::js_number(wrote as f64) } }; - this.deref(); Ok(result) } @@ -3139,15 +3022,11 @@ impl NewSocket { /// intrusive refcount + `finalize()`. // SAFETY: `this` was allocated via `heap::alloc` and refcount == 0. unsafe fn deinit_and_destroy(this: *mut Self) { - // SAFETY: per fn contract — sole owner of the live `heap::alloc` allocation. + // Not a `ThisPtr`: the refcount is already zero, so `ref_guard()` here + // would be a resurrection bug. + // SAFETY: per fn contract — sole owner, live until the `heap::take` below. let this_ref: &Self = unsafe { &*this }; this_ref.mark_inactive(); - if this_ref.flags.get().contains(Flags::OWNS_HANDLERS) { - if let Some(h) = this_ref.handlers.take() { - // SAFETY: `OWNS_HANDLERS` ⇒ `h` is the unfreed `heap::alloc` root. - drop(unsafe { bun_core::heap::take(h.as_ptr()) }); - } - } this_ref.detach_native_callback(); // Reset to empty (Strong drops on assign). this_ref.this_value.set(JsRef::empty()); @@ -3221,39 +3100,17 @@ impl NewSocket { .get(global, "socket")? .ok_or_else(|| global.throw(format_args!("Expected \"socket\" option")))?; - // Overwrite the pointee so the - // listener + all sockets observe the new callbacks. `this.handlers` is - // a raw `*mut Handlers` (server: `&mut listener.handlers`; client: - // `heap::alloc`), so writing through it has valid provenance. - let p: *mut Handlers = this - .handlers - .get() - .expect("No handlers set on Socket") - .as_ptr(); - // SAFETY: `p` is the freely-aliased raw pointer; no `&Handlers` borrow - // is live across the read/writes below (single-threaded event loop). - let prev_mode = unsafe { (*p).mode }; - let handlers = - Handlers::from_js(global, socket_obj, prev_mode == super::SocketMode::Server)?; - if this.handlers.get().map(|n| n.as_ptr()) != Some(p) { + let handlers = this.get_handlers(); + // Parse and validate first: the option getters run user JS that can + // close this socket and repoint its `Handlers`. + let reloaded = Handlers::prepare_reload(global, socket_obj)?; + if !this.handlers_are(&handlers) { return Ok(JSValue::UNDEFINED); } - // Preserve runtime state across the struct assignment. `Handlers.fromJS` returns a - // fresh struct with `active_connections = 0` and `mode` limited to `.server`/`.client`, - // but this socket (and any in-flight callback scope) still holds references that were - // counted against the old value, and a duplex-upgraded server socket must keep - // `.duplex_server`. Losing the counter causes the next `markInactive` to either free - // the heap-allocated client `Handlers` while the socket still points at it, or - // underflow on the server path. - // SAFETY: `this.handlers` still points at `p` (checked above), so the - // allocation is live; raw-pointer-only access; see `get_handlers` contract. - unsafe { - let active_connections = (*p).active_connections.get(); - core::ptr::drop_in_place(p); - core::ptr::write(p, handlers); - (*p).mode = prev_mode; - (*p).active_connections.set(active_connections); - } + // Update the callbacks of the existing cell in place, so the listener + // and every socket sharing it observe them; nothing else about the + // shared `Handlers` (mode, active_connections) is touched. + handlers.apply_reload(global, &reloaded); Ok(JSValue::UNDEFINED) } @@ -3337,23 +3194,16 @@ impl NewSocket { .unwrap_or_default(), None => Vec::new(), }; - // Handlers lifecycle is always client-mode (heap-per-connection) here: a - // standalone `new TLSSocket(socket, { isServer })` is NOT a SocketListener, - // and server-mode Handlers::mark_inactive assumes its `this` is a Listener's - // embedded `handlers` field. The server-ness lives in the SSL accept state - // (adopt_tls is_client=!is_server) + the ServerHandlers JS table, not here. - let handlers = Handlers::from_js(global, socket_obj, false)?; + // Client mode: a standalone `new TLSSocket(socket, { isServer })` is NOT + // a SocketListener, so these handlers have no listener to release. The + // server-ness lives in the SSL accept state (adopt_tls + // is_client=!is_server) + the ServerHandlers JS table, not here. + let handlers = Handlers::from_js(global, socket_obj, super::SocketMode::Client)?; if global.has_exception() { return Ok(JSValue::ZERO); } - // 9 .protect()'d JS callbacks live in `handlers`; every error/throw - // from here until they're moved into `tls.handlers` would leak them. - // The flag flips once ownership transfers so the guard is a no-op - // on success. - let mut handlers_guard = scopeguard::guard(Some(handlers), |h| { - // `Drop for Handlers` (unprotect + Strong drop). Explicit drop for clarity. - drop(h); - }); + // Nothing holds the callback cell until the TLS wrapper below does. + let _cell_root = handlers.root_cell(global); // Resolve the `SSL_CTX*`. Prefer a passed `SecureContext` (the // memoised `tls.createSecureContext` path) so 10k upgrades share @@ -3361,17 +3211,7 @@ impl NewSocket { // `tls:` options. Either way `owned_ctx` holds one ref we drop in // deinit; SSL_new() takes its own. // - // The local lives INSIDE the guard so all reads/writes go - // through `*owned_ctx` (DerefMut); capturing `&mut owned_ctx as *mut _` - // and then writing the local by name would pop the guard's pointer - // tag under Stacked Borrows and make the closure deref UB on a - // `?`-error path. - let mut owned_ctx = scopeguard::guard(None::<*mut SSL_CTX>, |c| { - if let Some(c) = c { - // SAFETY: BoringSSL FFI; `c` is the +1 ref taken below. - unsafe { boringssl_sys::SSL_CTX_free(c) }; - } - }); + let owned_ctx: Option; let mut ssl_opts: Option = None; // Drop frees ssl_opts. @@ -3392,15 +3232,17 @@ impl NewSocket { JSValue::ZERO }; if !sc_js.is_empty() { - let Some(sc) = SecureContext::from_js(sc_js) else { + let Some(sc) = sc_js.as_class_ref::() else { return Err(global.throw_invalid_argument_type_value( b"secureContext", b"SecureContext", sc_js, )); }; - // SAFETY: `from_js` returns a live `*mut SecureContext`. - *owned_ctx = Some(unsafe { (*sc).borrow() }.cast::()); + // `borrow()` returns a +1 ref (it calls `SSL_CTX_up_ref`). + // SAFETY: that ref is ours to release. + owned_ctx = + unsafe { boringssl_sys::OwnedSslCtx::from_raw(sc.borrow().cast::()) }; // servername / ALPN still come from the surrounding tls config. if let Some(t) = opts.get_truthy(global, "tls")? { if !t.is_boolean() { @@ -3440,8 +3282,9 @@ impl NewSocket { // stable address for the VM's lifetime, JS-thread-only access. unsafe { &mut (*state).ssl_ctx_cache } }; - *owned_ctx = match cache.get_or_create(cfg, &mut create_err) { - Some(c) => Some(c.cast::()), + owned_ctx = match cache.get_or_create(cfg, &mut create_err) { + // SAFETY: `get_or_create` hands back a +1 ref. + Some(c) => unsafe { boringssl_sys::OwnedSslCtx::from_raw(c.cast::()) }, None => { // us_ssl_ctx_from_options only sets *err for the CA/cipher // cases; bad cert/key/DH return NULL with err==.none and the @@ -3470,22 +3313,15 @@ impl NewSocket { default_data.ensure_still_alive(); } - let handlers_taken = handlers_guard.take().unwrap(); - scopeguard::ScopeGuard::into_inner(handlers_guard); - let vm = handlers_taken.vm; - // Client-mode - // `Handlers` is a standalone heap allocation that - // `Handlers::mark_inactive` later frees via `heap::take`. - let handlers_ptr = bun_core::heap::into_raw_nn(Box::new(handlers_taken)); + let vm = handlers.vm; - // Ownership of the +1 `SSL_CTX` ref transfers into `tls.owned_ssl_ctx` - // below; defuse the guard so a later `?` doesn't double-free. - let owned_ctx_taken = scopeguard::ScopeGuard::into_inner(owned_ctx); + // The +1 `SSL_CTX` ref transfers into `tls.owned_ssl_ctx` below. + let owned_ctx_taken = owned_ctx.map(|c| c.into_raw()); let cfg = ssl_opts.as_ref(); - let tls_ptr: *mut TLSSocket = TLSSocket::new(TLSSocket { + let tls: bun_ptr::ThisPtr = TLSSocket::new(TLSSocket { ref_count: bun_ptr::RefCount::init(), - handlers: Cell::new(Some(handlers_ptr)), + handlers: JsCell::new(Some(handlers)), socket: Cell::new(SocketHandler::::DETACHED), owned_ssl_ctx: Cell::new(owned_ctx_taken), connection: JsCell::new(this.connection.get().clone()), @@ -3494,7 +3330,7 @@ impl NewSocket { server_name: JsCell::new( cfg.and_then(|c| c.server_name_bytes().map(Box::<[u8]>::from)), ), - flags: Cell::new(Flags::default() | Flags::OWNS_HANDLERS), + flags: Cell::new(Flags::default()), this_value: JsCell::new(JsRef::empty()), poll_ref: JsCell::new(KeepAlive::init()), ref_pollref_on_connect: Cell::new(true), @@ -3503,13 +3339,9 @@ impl NewSocket { native_callback: JsCell::new(NativeCallbacks::None), twin: JsCell::new(None), }); - // Do NOT shadow `tls_ptr` with a long-lived `&mut TLSSocket`: the - // allocation-root pointer (from `heap::alloc`) must be the value - // stored in the uws ext slot below so dispatch-derived `&mut`s share - // its provenance. A `&mut *tls_ptr` reborrow that outlives the - // ext-slot store and the `on_open`/`start_tls_handshake` calls would - // alias the `&mut TLSSocket` those calls materialise from ext. - // Reborrow short-lived `unsafe { &mut *tls_ptr }` per use instead. + // Never shadow this with a long-lived borrow: it would alias the + // reference dispatch materialises from the ext slot during + // `on_open`/`start_tls_handshake`. let sni: Option<&core::ffi::CStr> = cfg.and_then(|c| c.server_name_cstr()); // SAFETY: per-thread VM singleton; no aliasing `&mut` held. @@ -3524,7 +3356,7 @@ impl NewSocket { (*raw_socket).adopt_tls( group, uws::SocketKind::BunSocketTls, - &mut *((*tls_ptr).owned_ssl_ctx.get().unwrap()), + &mut *(tls.owned_ssl_ctx.get().unwrap()), sni, !is_server, core::mem::size_of::<*mut c_void>() as i32, @@ -3534,23 +3366,10 @@ impl NewSocket { Some(s) => s, None => { let err = boringssl_sys::ERR_get_error(); - scopeguard::defer! { - if err != 0 { - boringssl_sys::ERR_clear_error(); - } - } - // tls.deinit drops the owned_ctx ref. Null the handlers field - // first so `TLSSocket::deinit` doesn't double-destroy the - // `Handlers` we're about to free explicitly. - // SAFETY: sole owner of the fresh allocation. - unsafe { - (*tls_ptr).handlers.set(None); - (*tls_ptr).deref(); - } - // `Handlers` has a `Drop` impl that runs `deinit` (unprotect). - // SAFETY: `handlers_ptr` is the `heap::alloc` allocation - // created above; sole owner here. - drop(unsafe { bun_core::heap::take(handlers_ptr.as_ptr()) }); + let _clear_err = ClearErrorQueue(err != 0); + // `deref` runs `deinit_and_destroy`, which drops the owned_ctx + // ref and the handlers `Rc`. Sole owner of the fresh allocation. + tls.get().deref(); if err != 0 && !global.has_exception() { return Err(global.throw_value(boringssl_err_to_js(global, err))); } @@ -3568,7 +3387,7 @@ impl NewSocket { // *Handlers are TRANSFERRED to the raw twin (the `[raw, tls]` // contract is: index 0 keeps the pre-upgrade callbacks and sees // ciphertext, index 1 gets the new ones and sees plaintext). - let raw_handlers = this.handlers.take(); + let raw_handlers = this.take_handlers(); // Preserve `socket.unref()` across the upgrade — node:tls callers // that unref the underlying TCP socket before upgrading must not // suddenly hold the loop open via the TLS wrapper. @@ -3585,40 +3404,26 @@ impl NewSocket { // active_connections=1 it holds is transferring to `raw`. this.this_value.with_mut(|r| r.downgrade()); } - // Must run on EVERY exit past this point, - // including the `?` early-returns from `create_empty_array`/`put_index` - // below, or we leak one ref on the retired TCP wrapper. - let _this_deref = scopeguard::guard(this.as_ctx_ptr(), |p| { - // SAFETY: `this` is the JS-wrapper-owned allocation; the wrapper's - // +1 keeps it alive across the whole call regardless of which exit - // we take. Single JS thread. - unsafe { (*p).deref() }; - }); + // Release the retired TCP wrapper's ref on EVERY exit past this point, + // including the `?` early-returns below. + // SAFETY: `this` owns the outstanding ref this guard consumes; the JS + // wrapper's own +1 keeps the allocation alive across the whole call. + let _this_deref = unsafe { bun_ptr::ScopedRef::adopt(this.as_ctx_ptr()) }; this.detach_native_callback(); this.socket.set(SocketHandler::::DETACHED); // Only NOW is it safe for dispatch to fire: ext + kind point at `tls`. - // Store the allocation-root `tls_ptr` (from `heap::alloc`), NOT a - // reborrow-derived pointer, so dispatch's `&mut *ext` shares - // provenance with our per-use reborrows below. - // SAFETY: ext slot is sized for `*mut TLSSocket`; `new_raw` is the live - // adopted `us_socket_t`. - unsafe { *(*new_raw.as_ptr()).ext::<*mut TLSSocket>() = tls_ptr }; - // SAFETY: short-lived reborrows; no `&mut TLSSocket` is held across - // any dispatch boundary (`on_open`/`start_tls_handshake` below). - unsafe { - (*tls_ptr) - .socket - .set(SocketHandler::::from(new_raw.as_ptr())); - (*tls_ptr).ref_(); - } + *uws::us_socket_t::opaque_mut(new_raw.as_ptr()).ext() = Some(tls); + tls.socket + .set(SocketHandler::::from(new_raw.as_ptr())); + tls.ref_(); // The `raw` half — same `us_socket_t*`, ORIGINAL pre-upgrade // *Handlers, writes bypass SSL. Dispatch reaches it via the // `ssl_raw_tap` ciphertext hook, never via the ext slot. let raw = TLSSocket::new(TLSSocket { ref_count: bun_ptr::RefCount::init(), - handlers: Cell::new(raw_handlers), + handlers: JsCell::new(raw_handlers), socket: Cell::new(SocketHandler::::from(new_raw.as_ptr())), owned_ssl_ctx: Cell::new(None), connection: JsCell::new(None), @@ -3626,22 +3431,10 @@ impl NewSocket { protos: JsCell::new(None), server_name: JsCell::new(None), // is_active so the chained `raw.onClose` → `markInactive` path - // tears down `raw_handlers` (client-mode handlers free - // themselves there). No poll_ref — `tls` keeps the loop alive. - // active_connections=1 was already on raw_handlers from `this`. - // OWNS_HANDLERS transfers from the retired wrapper rather than - // being asserted: a client socket's Handlers are its own - // heap::alloc root and the twin must free them, but an accepted - // server socket only borrows an interior pointer into its - // listener's embedded Handlers - claiming ownership of that - // would bad-free the listener's allocation when the twin is - // finalized. - flags: Cell::new( - Flags::BYPASS_TLS - | Flags::IS_ACTIVE - | Flags::OWNED_PROTOS - | (this.flags.get() & Flags::OWNS_HANDLERS), - ), + // releases `raw_handlers`. No poll_ref — `tls` keeps the loop + // alive. active_connections=1 was already on raw_handlers from + // `this`. + flags: Cell::new(Flags::BYPASS_TLS | Flags::IS_ACTIVE | Flags::OWNED_PROTOS), this_value: JsCell::new(JsRef::empty()), poll_ref: JsCell::new(KeepAlive::init()), ref_pollref_on_connect: Cell::new(true), @@ -3650,67 +3443,50 @@ impl NewSocket { native_callback: JsCell::new(NativeCallbacks::None), twin: JsCell::new(None), }); - // SAFETY: raw just allocated via heap::alloc. - let raw_ref: &TLSSocket = unsafe { &*raw }; + let raw_ref = raw; raw_ref.ref_(); // SAFETY: `raw` came from `TLSSocket::new` (heap::alloc); intrusive +1 held. - unsafe { (*tls_ptr).twin.set(Some(IntrusiveRc::from_raw(raw))) }; - // SAFETY: `new_raw` is the live adopted `us_socket_t`. - unsafe { (*new_raw.as_ptr()).set_ssl_raw_tap(true) }; + tls.twin + .set(Some(unsafe { IntrusiveRc::from_raw(raw.as_ptr()) })); + // S008: `us_socket_t` is an `opaque_ffi!` ZST — safe deref. + bun_opaque::opaque_deref_mut(new_raw.as_ptr()).set_ssl_raw_tap(true); - // SAFETY: short-lived reborrow; no dispatch can fire until - // `on_open`/`start_tls_handshake` below. - let tls_js_value = unsafe { (*tls_ptr).get_this_value(global) }; + let tls_js_value = tls.get_this_value(global); let raw_js_value = raw_ref.get_this_value(global); TLSSocket::data_set_cached(tls_js_value, global, default_data); // `raw` keeps the pre-upgrade `data` so its callbacks emit on the // original net.Socket, not the TLS one. TLSSocket::data_set_cached(raw_js_value, global, original_data); - // SAFETY: short-lived reborrows on the allocation-root pointer. - unsafe { - (*tls_ptr).mark_active(); - if was_reffed { - (*tls_ptr).poll_ref.with_mut(|p| { - p.ref_(bun_io::posix_event_loop::get_vm_ctx( - bun_io::AllocatorType::Js, - )) - }); - } + tls.mark_active(); + if was_reffed { + tls.poll_ref.with_mut(|p| { + p.ref_(bun_io::posix_event_loop::get_vm_ctx( + bun_io::AllocatorType::Js, + )) + }); } - let _ = vm; // Fire onOpen with the right `this`, then send ClientHello. Doing // it before ext was repointed would have ALPN/onOpen land in the // dead TCPSocket. - // SAFETY: `on_open` takes `*mut Self` (noalias re-entrancy) and may - // synchronously dispatch through the ext slot (which now stores - // `tls_ptr`); passing the allocation-root pointer keeps provenance and - // no `&mut TLSSocket` is held across the call. - unsafe { - let sock = (*tls_ptr).socket.get(); - TLSSocket::on_open(tls_ptr, sock); - }; - // SAFETY: `new_raw` is the live adopted `us_socket_t`. - unsafe { (*new_raw.as_ptr()).start_tls_handshake() }; + TLSSocket::on_open(tls, tls.socket.get()); + bun_opaque::opaque_deref_mut(new_raw.as_ptr()).start_tls_handshake(); // The socket being wrapped may have had its readable interest off (an // accepted socket nobody was reading yet — its ClientHello is still in // the kernel buffer); make sure the adopted TLS socket is reading so // the handshake can be driven. A no-op when it was already reading. - // SAFETY: `new_raw` is the live adopted `us_socket_t`. - unsafe { (*new_raw.as_ptr()).resume() }; + bun_opaque::opaque_deref_mut(new_raw.as_ptr()).resume(); // Feed bytes that arrived before the upgrade (already pulled off the fd // by the plain-TCP layer) into the TLS engine exactly as if they had // just been received — for a server-side wrap this is the ClientHello. if !initial_data.is_empty() { - // SAFETY: `new_raw` is live; `initial_data` is an owned copy. - unsafe { (*new_raw.as_ptr()).tls_feed(initial_data.as_slice()) }; + bun_opaque::opaque_deref_mut(new_raw.as_ptr()).tls_feed(initial_data.as_slice()); } let array = JSValue::create_empty_array(global, 2)?; array.put_index(global, 0, raw_js_value)?; array.put_index(global, 1, tls_js_value)?; - // `this.deref()` runs via `_this_deref` scopeguard on return. Ok(array) } @@ -4055,7 +3831,6 @@ bitflags::bitflags! { /// `us_socket_raw_write` (bypassing the SSL layer) so node:net can pipe /// pre-handshake bytes / read the underlying TCP stream. const BYPASS_TLS = 1 << 9; - const OWNS_HANDLERS = 1 << 10; const HOSTNAME_MISMATCH = 1 << 11; } } @@ -4137,9 +3912,7 @@ impl DuplexUpgradeContext { let socket = self.duplex_socket(); if let Some(tls) = &mut self.tls { - // SAFETY: intrusive refcount; single-threaded dispatch. `on_open` - // takes `*mut Self` (noalias re-entrancy) — no `&mut TLSSocket` held. - unsafe { TLSSocket::on_open(tls.as_ptr(), socket) }; + TLSSocket::on_open(tls.this_ptr(), socket); } } @@ -4147,24 +3920,19 @@ impl DuplexUpgradeContext { let socket = self.duplex_socket(); if let Some(tls) = &mut self.tls { - // SAFETY: intrusive refcount; single-threaded dispatch. - unsafe { TLSSocket::on_data(tls.as_ptr(), socket, decoded_data) }; + TLSSocket::on_data(tls.this_ptr(), socket, decoded_data); } } fn on_session(&mut self, session: &[u8]) { if let Some(tls) = &mut self.tls { - // SAFETY: intrusive refcount; single-threaded dispatch. `on_session` - // takes `*mut Self` (noalias re-entrancy); JS errors land on the - // socket's error handler inside. - let _ = unsafe { TLSSocket::on_session(tls.as_ptr(), session) }; + let _ = TLSSocket::on_session(tls.this_ptr(), session); } } fn on_keylog(&mut self, line: &[u8]) { if let Some(tls) = &mut self.tls { - // SAFETY: same as `on_session` above. - let _ = unsafe { TLSSocket::on_keylog(tls.as_ptr(), line) }; + let _ = TLSSocket::on_keylog(tls.this_ptr(), line); } } @@ -4172,17 +3940,15 @@ impl DuplexUpgradeContext { let socket = self.duplex_socket(); if let Some(tls) = &mut self.tls { - // SAFETY: intrusive refcount; single-threaded dispatch. - let _ = - unsafe { TLSSocket::on_handshake(tls.as_ptr(), socket, success as i32, ssl_error) }; + let tls = tls.this_ptr(); + let _ = TLSSocket::on_handshake(tls, socket, success as i32, ssl_error); } } fn on_end(&mut self) { let socket = self.duplex_socket(); if let Some(tls) = &mut self.tls { - // SAFETY: intrusive refcount; single-threaded dispatch. - unsafe { TLSSocket::on_end(tls.as_ptr(), socket) }; + TLSSocket::on_end(tls.this_ptr(), socket); } } @@ -4190,8 +3956,7 @@ impl DuplexUpgradeContext { let socket = self.duplex_socket(); if let Some(tls) = &mut self.tls { - // SAFETY: intrusive refcount; single-threaded dispatch. - unsafe { TLSSocket::on_writable(tls.as_ptr(), socket) }; + TLSSocket::on_writable(tls.this_ptr(), socket); } } @@ -4205,10 +3970,10 @@ impl DuplexUpgradeContext { if let Some(tls) = self.tls.take() { // Pre-open error (e.g. the duplex emitted non-Buffer data // before the queued `.StartTLS` task ran). `handleConnectError` - // → `markInactive` frees `tls.handlers`; null `tls` so the + // → `markInactive` releases `tls.handlers`; null `tls` so the // still-queued `.StartTLS` → `onOpen` — and any further - // duplex events — skip the TLSSocket instead of calling - // `getHandlers()` on the freed allocation. + // duplex events — skip the TLSSocket instead of hitting + // `has_handlers() == false` in `onOpen`. // // Refcount: `tls.socket` is `InternalSocket::UpgradedDuplex` // here (assigned in `js_upgrade_duplex_to_tls` *before* @@ -4218,15 +3983,9 @@ 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). - let p = IntrusiveRc::into_raw(tls); - // SAFETY: intrusive refcount; single-threaded dispatch. The - // +1 transferred via `into_raw` is released by - // `handle_connect_error`'s `needs_deref` arm (socket is - // UpgradedDuplex, not Detached) — do NOT reconstruct the - // IntrusiveRc. `handle_connect_error` takes `*mut Self`. - let _ = unsafe { - TLSSocket::handle_connect_error(p, sys::SystemErrno::ECONNREFUSED as c_int, 0) - }; + let p = tls.into_this_ptr(); + let _ = + TLSSocket::handle_connect_error(p, sys::SystemErrno::ECONNREFUSED as c_int, 0); } } } @@ -4235,8 +3994,7 @@ impl DuplexUpgradeContext { let socket = self.duplex_socket(); if let Some(tls) = &mut self.tls { - // SAFETY: intrusive refcount; single-threaded dispatch. - unsafe { TLSSocket::on_timeout(tls.as_ptr(), socket) }; + TLSSocket::on_timeout(tls.this_ptr(), socket); } } @@ -4252,13 +4010,9 @@ impl DuplexUpgradeContext { // from `duplex.end()` (called right after this returns via // `UpgradedDuplex.onClose` → `callWriteOrEnd`) hits the null-check // in `onError` instead of reading the Handlers that `tls.onClose` - // → `markInactive` just freed. - let p = IntrusiveRc::into_raw(tls); - // SAFETY: intrusive refcount; single-threaded dispatch. `on_close` - // consumes the +1 we held via its internal `deref()`, so we do NOT - // reconstruct the IntrusiveRc (that would double-deref). `on_close` - // takes `*mut Self` (noalias re-entrancy). - let _ = unsafe { TLSSocket::on_close(p, socket, 0, None) }; + // → `markInactive` just released. + let p = tls.into_this_ptr(); + let _ = TLSSocket::on_close(p, socket, 0, None); } self.deinit_in_next_tick(); @@ -4320,12 +4074,8 @@ impl DuplexUpgradeContext { // `start_tls()` was queued), so `needs_deref = // !is_detached()` is true — and detaches. Null // `this.tls` so `deinit` doesn't deref again. - let p = IntrusiveRc::into_raw(tls); - // SAFETY: intrusive refcount; `handle_connect_error`'s - // `needs_deref` arm releases the +1 transferred via - // `into_raw` (socket is UpgradedDuplex, not Detached). - // `handle_connect_error` takes `*mut Self`. - let _ = unsafe { TLSSocket::handle_connect_error(p, errno, 0) }; + let p = tls.into_this_ptr(); + let _ = TLSSocket::handle_connect_error(p, errno, 0); } // `startTLS`/`startTLSWithCTX` failed before the // SSLWrapper was assigned, so its close callback @@ -4433,30 +4183,28 @@ pub fn js_upgrade_duplex_to_tls( if let Some(is_server_val) = opts.get_truthy(global, "isServer")? { is_server = is_server_val.to_boolean(); } - // Note: Handlers.fromJS is_server=false because these handlers are standalone - // allocations (not embedded in a Listener). The mode field on Handlers - // controls lifecycle (markInactive expects a Listener parent when .server). - // The TLS direction (client vs server) is controlled by DuplexUpgradeContext.mode. - let handlers = Handlers::from_js(global, socket_obj, false)?; - let mut handlers_guard = scopeguard::guard(Some(handlers), |h| { - drop(h); - }); + // `DuplexServer` mode makes `TLSSocket.isServer()` report the server role + // for ALPN without claiming a listener parent — these handlers have none, + // so `mark_inactive` must take the client path. The TLS direction itself is + // controlled by DuplexUpgradeContext.mode. + let handlers = Handlers::from_js( + global, + socket_obj, + if is_server { + crate::socket::SocketMode::DuplexServer + } else { + crate::socket::SocketMode::Client + }, + )?; + // Nothing holds the callback cell until the TLS wrapper below does. + let _cell_root = handlers.root_cell(global); // Resolve the `SSL_CTX*`. Prefer a passed `SecureContext` (the memoised // `tls.createSecureContext` path — what `[buntls]` now returns) so the // duplex/named-pipe path shares one `SSL_CTX_new` with everyone else. // node:net wraps `[buntls]`'s return as `opts.tls.secureContext`; userland // may also pass it top-level. Same lookup as `upgradeTLS` above. - // The local lives INSIDE the guard so all reads/writes go through - // `*owned_ctx` (DerefMut); capturing `&mut owned_ctx as *mut _` and then - // writing the local by name would invalidate the guard's pointer tag - // under Stacked Borrows. - let mut owned_ctx = scopeguard::guard(None::<*mut SSL_CTX>, |c| { - if let Some(c) = c { - // SAFETY: BoringSSL FFI; `c` is the +1 ref taken below. - unsafe { boringssl_sys::SSL_CTX_free(c) }; - } - }); + let mut owned_ctx: Option = None; let sc_js: JSValue = 'blk: { if let Some(v) = opts.get_truthy(global, "secureContext")? { break 'blk v; @@ -4471,15 +4219,16 @@ pub fn js_upgrade_duplex_to_tls( JSValue::ZERO }; if !sc_js.is_empty() { - let Some(sc) = SecureContext::from_js(sc_js) else { + let Some(sc) = sc_js.as_class_ref::() else { return Err(global.throw_invalid_argument_type_value( b"secureContext", b"SecureContext", sc_js, )); }; - // SAFETY: `from_js` returns a live `*mut SecureContext`. - *owned_ctx = Some(unsafe { (*sc).borrow() }.cast::()); + // `borrow()` returns a +1 ref (it calls `SSL_CTX_up_ref`). + // SAFETY: that ref is ours to release. + owned_ctx = unsafe { boringssl_sys::OwnedSslCtx::from_raw(sc.borrow().cast::()) }; } // Still parse SSLConfig for servername/ALPN (those live on the JS-side @@ -4504,22 +4253,9 @@ pub fn js_upgrade_duplex_to_tls( default_data.ensure_still_alive(); } - let mut handlers_taken = handlers_guard.take().unwrap(); - scopeguard::ScopeGuard::into_inner(handlers_guard); - // Set mode to duplex_server so TLSSocket.isServer() returns true for ALPN server mode - // without affecting markInactive lifecycle (which requires a Listener parent). - handlers_taken.mode = if is_server { - crate::socket::SocketMode::DuplexServer - } else { - crate::socket::SocketMode::Client - }; - // Client-mode `Handlers` - // is a standalone heap allocation that `Handlers::mark_inactive` later - // frees via `heap::take`. - let handlers_ptr = bun_core::heap::into_raw_nn(Box::new(handlers_taken)); let tls = TLSSocket::new(TLSSocket { ref_count: bun_ptr::RefCount::init(), - handlers: Cell::new(Some(handlers_ptr)), + handlers: JsCell::new(Some(handlers)), socket: Cell::new(SocketHandler::::DETACHED), owned_ssl_ctx: Cell::new(None), connection: JsCell::new(None), @@ -4530,7 +4266,7 @@ pub fn js_upgrade_duplex_to_tls( server_name: JsCell::new( socket_config.and_then(|cfg| cfg.server_name_bytes().map(Box::<[u8]>::from)), ), - flags: Cell::new(Flags::default() | Flags::OWNS_HANDLERS), + flags: Cell::new(Flags::default()), this_value: JsCell::new(JsRef::empty()), poll_ref: JsCell::new(KeepAlive::init()), ref_pollref_on_connect: Cell::new(true), @@ -4539,14 +4275,12 @@ pub fn js_upgrade_duplex_to_tls( native_callback: JsCell::new(NativeCallbacks::None), twin: JsCell::new(None), }); - // SAFETY: tls just allocated via heap::alloc. - let tls_ref: &TLSSocket = unsafe { &*tls }; + let tls_ref = tls; let tls_js_value = tls_ref.get_this_value(global); TLSSocket::data_set_cached(tls_js_value, global, default_data); - // Ownership of the +1 `SSL_CTX` ref transfers into - // `DuplexUpgradeContext.owned_ctx` below; defuse the guard. - let owned_ctx_taken = scopeguard::ScopeGuard::into_inner(owned_ctx); + // The +1 `SSL_CTX` ref transfers into `DuplexUpgradeContext.owned_ctx` below. + let owned_ctx_taken = owned_ctx.map(|c| c.into_raw()); // `DuplexUpgradeContext` is self-referential: `task.ctx` and // `upgrade.handlers.ctx` both point at the containing allocation, and @@ -4563,7 +4297,7 @@ pub fn js_upgrade_duplex_to_tls( // SAFETY: fresh heap allocation; every field is `ptr::write`-initialized // below before any read or `&mut DuplexUpgradeContext` is formed. unsafe { - ptr::addr_of_mut!((*duplex_context).tls).write(Some(IntrusiveRc::from_raw(tls))); + ptr::addr_of_mut!((*duplex_context).tls).write(Some(IntrusiveRc::from_raw(tls.as_ptr()))); ptr::addr_of_mut!((*duplex_context).vm).write(VirtualMachine::get()); // `AnyTask::New` can't take the callback as a type parameter (see the // notes in AnyTask.rs), so hand-write the `*mut c_void → run_event` shim. @@ -4754,11 +4488,9 @@ pub fn js_set_socket_options(global: &JSGlobalObject, callframe: &CallFrame) -> return Err(global.throw_not_enough_arguments("setSocketOptions", 3, arguments.len())); } - let Some(socket) = arguments[0].as_::() else { + let Some(socket) = arguments[0].as_class_ref::() else { return Err(global.throw(format_args!("Expected a SocketTCP instance"))); }; - // SAFETY: `as_` returned a non-null `*mut TCPSocket` owned by the JS wrapper. - let socket: &TCPSocket = unsafe { &*socket }; let is_for_send_buffer = arguments[1].to_int32() == 1; let is_for_recv_buffer = arguments[1].to_int32() == 2; diff --git a/src/runtime/socket/sockets.classes.ts b/src/runtime/socket/sockets.classes.ts index 9b2696bee3be..834dd71caa67 100644 --- a/src/runtime/socket/sockets.classes.ts +++ b/src/runtime/socket/sockets.classes.ts @@ -7,6 +7,9 @@ function generate(ssl) { noConstructor: true, 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"], proto: { getAuthorizationError: { fn: "getAuthorizationError", @@ -270,6 +273,9 @@ export default [ sharedThis: true, noConstructor: true, JSType: "0b11101110", + // Visited slot holding the JSSocketHandlers cell shared with every socket + // accepted by this listener. + values: ["handlers"], proto: { stop: { fn: "stop", diff --git a/src/runtime/socket/uws_dispatch.rs b/src/runtime/socket/uws_dispatch.rs index c1d4c17499ef..508b43ab029c 100644 --- a/src/runtime/socket/uws_dispatch.rs +++ b/src/runtime/socket/uws_dispatch.rs @@ -191,11 +191,10 @@ pub(crate) unsafe extern "C" fn us_dispatch_ssl_raw_tap( debug_assert!(s_ref.kind() == SocketKind::BunSocketTls); // `bun.jsc.API.NewSocket(true)` → the runtime-local `socket::NewSocket`. type TLSSocket = super::NewSocket; - let tls_ptr: *mut TLSSocket = *s_ref.ext::<*mut TLSSocket>(); - // SAFETY: ext slot for BunSocketTls always holds a non-null *mut TLSSocket - // (stamped at construction); dispatch is single-threaded so no `&mut` - // alias exists for the lifetime of this shared borrow. - let tls: &TLSSocket = unsafe { &*tls_ptr }; + // The ext slot for `BunSocketTls` always holds a live `TLSSocket`, stamped + // at construction. + let tls: bun_ptr::ThisPtr = + s_ref.ext::>>().unwrap(); if let Some(raw) = tls.twin.get().as_ref() { // `twin` is `IntrusiveRc` (intrusive ref-counted heap pointer); // grab the raw `*mut` without consuming the ref so the +1 stays put. @@ -208,10 +207,16 @@ pub(crate) unsafe extern "C" fn us_dispatch_ssl_raw_tap( // SAFETY: `data` points to `len` readable bytes from the TLS BIO; loop.c // guarantees the buffer outlives this call. let slice = unsafe { core::slice::from_raw_parts(data, len) }; - // SAFETY: `twin` holds a live +1 - // ref to the `[raw, _]` half; dispatch is single-threaded so no aliasing - // `&mut` exists. `on_data` takes `*mut Self` (noalias re-entrancy fix). - unsafe { TLSSocket::on_data(raw, NewSocketHandler::::from(s), slice) }; + // SAFETY: `twin` holds a live +1 ref to the `[raw, _]` half, so `raw` + // is live for `ThisPtr::new`; dispatch is single-threaded so no + // aliasing `&mut` exists. + unsafe { + TLSSocket::on_data( + bun_ptr::ThisPtr::new(raw), + NewSocketHandler::::from(s), + slice, + ) + }; } s } @@ -232,10 +237,9 @@ pub unsafe extern "C" fn us_dispatch_session(s: *mut us_socket_t, data: *const u return; } type TLSSocket = super::NewSocket; - let tls_ptr: *mut TLSSocket = *s_ref.ext::<*mut TLSSocket>(); - if tls_ptr.is_null() { + let Some(tls) = *s_ref.ext::>>() else { return; - } + }; // A negative length from the C side means there is nothing to deliver; // never panic across the `extern "C"` boundary. let Ok(len) = usize::try_from(len) else { @@ -244,9 +248,7 @@ pub unsafe extern "C" fn us_dispatch_session(s: *mut us_socket_t, data: *const u // SAFETY: `data` points to `len` readable bytes owned by the caller for the // duration of this call. let slice = unsafe { core::slice::from_raw_parts(data, len) }; - // SAFETY: ext slot for BunSocketTls holds a live *mut TLSSocket; dispatch is - // single-threaded. `on_session` takes `*mut Self` (noalias re-entrancy). - let _ = unsafe { TLSSocket::on_session(tls_ptr, slice) }; + let _ = TLSSocket::on_session(tls, slice); } /// Hands an NSS key-log line parked by the keylog callback to the JS @@ -262,10 +264,9 @@ pub unsafe extern "C" fn us_dispatch_keylog(s: *mut us_socket_t, data: *const u8 return; } type TLSSocket = super::NewSocket; - let tls_ptr: *mut TLSSocket = *s_ref.ext::<*mut TLSSocket>(); - if tls_ptr.is_null() { + let Some(tls) = *s_ref.ext::>>() else { return; - } + }; // A negative length from the C side means there is nothing to deliver; // never panic across the `extern "C"` boundary. let Ok(len) = usize::try_from(len) else { @@ -274,7 +275,5 @@ pub unsafe extern "C" fn us_dispatch_keylog(s: *mut us_socket_t, data: *const u8 // SAFETY: `data` points to `len` readable bytes owned by the caller for the // duration of this call. let slice = unsafe { core::slice::from_raw_parts(data, len) }; - // SAFETY: ext slot for BunSocketTls holds a live *mut TLSSocket; dispatch is - // single-threaded. `on_keylog` takes `*mut Self` (noalias re-entrancy). - let _ = unsafe { TLSSocket::on_keylog(tls_ptr, slice) }; + let _ = TLSSocket::on_keylog(tls, slice); } diff --git a/src/runtime/socket/uws_handlers.rs b/src/runtime/socket/uws_handlers.rs index 4a7b17aa257b..5293f83e79f7 100644 --- a/src/runtime/socket/uws_handlers.rs +++ b/src/runtime/socket/uws_handlers.rs @@ -8,6 +8,7 @@ //! old `NewSocketHandler.configure`/`unsafeConfigure` machinery, which built //! the same trampolines at runtime per `us_socket_context_t`. +use bun_ptr::ThisPtr; use core::ffi::{c_int, c_void}; use core::ptr::NonNull; @@ -218,31 +219,28 @@ where // ── RawSocketEvents / RawPtrHandler ───────────────────────────────────────── // -// Some consumers' handlers may free or re-enter `*Self` mid-call (refcount -// reaching zero, `tcp.close()` synchronously dispatching `on_close`, …) and -// therefore take `*mut Self` rather than `&mut self`. Dispatching those -// through `PtrHandler` would form a `&mut T` argument that outlives the -// allocation it points to (Stacked-Borrows argument-protector UB), so they -// get a raw-pointer twin of the trait/adapter pair. +// These handlers may free or re-enter `Self` mid-call (a JS callback closing +// the socket, the refcount reaching zero), so they cannot take `&mut self` — +// a `&mut` argument protector outliving the allocation is UB. They take +// [`ThisPtr`](bun_ptr::ThisPtr) instead: `Copy + Deref`, so each field +// access is its own short-lived shared borrow and none spans a callback. +// +// The ext slot stores that `ThisPtr` directly, so recovering it is safe and +// the `unsafe` lives once, in the vtable's ext read. pub trait RawSocketEvents: Sized { const HAS_ON_OPEN: bool = false; - unsafe fn on_open(_this: *mut Self, _s: NewSocketHandler) {} - unsafe fn on_data(_this: *mut Self, _s: NewSocketHandler, _data: &[u8]) {} - unsafe fn on_writable(_this: *mut Self, _s: NewSocketHandler) {} - unsafe fn on_close( - _this: *mut Self, - _s: NewSocketHandler, - _code: i32, - _reason: *mut c_void, - ) { + fn on_open(_this: ThisPtr, _s: NewSocketHandler) {} + fn on_data(_this: ThisPtr, _s: NewSocketHandler, _data: &[u8]) {} + fn on_writable(_this: ThisPtr, _s: NewSocketHandler) {} + fn on_close(_this: ThisPtr, _s: NewSocketHandler, _code: i32, _reason: *mut c_void) { } - unsafe fn on_timeout(_this: *mut Self, _s: NewSocketHandler) {} - unsafe fn on_long_timeout(_this: *mut Self, _s: NewSocketHandler) {} - unsafe fn on_end(_this: *mut Self, _s: NewSocketHandler) {} - unsafe fn on_connect_error(_this: *mut Self, _s: NewSocketHandler, _code: i32) {} - unsafe fn on_handshake( - _this: *mut Self, + fn on_timeout(_this: ThisPtr, _s: NewSocketHandler) {} + fn on_long_timeout(_this: ThisPtr, _s: NewSocketHandler) {} + fn on_end(_this: ThisPtr, _s: NewSocketHandler) {} + fn on_connect_error(_this: ThisPtr, _s: NewSocketHandler, _code: i32) {} + fn on_handshake( + _this: ThisPtr, _s: NewSocketHandler, _ok: i32, _err: bun_uws::us_bun_verify_error_t, @@ -256,7 +254,7 @@ impl VHandler for RawPtrHandler where T: RawSocketEvents + 'static, { - type Ext = Option>; + type Ext = Option>; const HAS_ON_OPEN: bool = T::HAS_ON_OPEN; const HAS_ON_DATA: bool = true; @@ -271,45 +269,36 @@ where fn on_open(ext: &mut Self::Ext, s: *mut us_socket_t, _is_client: bool, _ip: &[u8]) { let Some(this) = *ext else { return }; - // SAFETY: ext slot holds the unique heap owner; single-threaded dispatch. - unsafe { T::on_open(this.as_ptr(), wrap::(s)) }; + T::on_open(this, wrap::(s)); } fn on_data(ext: &mut Self::Ext, s: *mut us_socket_t, data: &[u8]) { let Some(this) = *ext else { return }; - // SAFETY: see `on_open`. - unsafe { T::on_data(this.as_ptr(), wrap::(s), data) }; + T::on_data(this, wrap::(s), data); } fn on_writable(ext: &mut Self::Ext, s: *mut us_socket_t) { let Some(this) = *ext else { return }; - // SAFETY: see `on_open`. - unsafe { T::on_writable(this.as_ptr(), wrap::(s)) }; + T::on_writable(this, wrap::(s)); } fn on_close(ext: &mut Self::Ext, s: *mut us_socket_t, code: i32, reason: Option<*mut c_void>) { let Some(this) = *ext else { return }; - // SAFETY: see `on_open`. - unsafe { - T::on_close( - this.as_ptr(), - wrap::(s), - code, - reason.unwrap_or(core::ptr::null_mut()), - ) - }; + T::on_close( + this, + wrap::(s), + code, + reason.unwrap_or(core::ptr::null_mut()), + ); } fn on_timeout(ext: &mut Self::Ext, s: *mut us_socket_t) { let Some(this) = *ext else { return }; - // SAFETY: see `on_open`. - unsafe { T::on_timeout(this.as_ptr(), wrap::(s)) }; + T::on_timeout(this, wrap::(s)); } fn on_long_timeout(ext: &mut Self::Ext, s: *mut us_socket_t) { let Some(this) = *ext else { return }; - // SAFETY: see `on_open`. - unsafe { T::on_long_timeout(this.as_ptr(), wrap::(s)) }; + T::on_long_timeout(this, wrap::(s)); } fn on_end(ext: &mut Self::Ext, s: *mut us_socket_t) { let Some(this) = *ext else { return }; - // SAFETY: see `on_open`. - unsafe { T::on_end(this.as_ptr(), wrap::(s)) }; + T::on_end(this, wrap::(s)); } fn on_connect_error(ext: &mut Self::Ext, s: *mut us_socket_t, code: i32) { // Close first, then notify — see `PtrHandler::on_connect_error`. @@ -318,22 +307,14 @@ where // deref (`s` is a live socket passed by the trampoline). us_socket_t::opaque_mut(s).close(CloseCode::failure); if let Some(t) = this { - // SAFETY: see `on_open`. - unsafe { T::on_connect_error(t.as_ptr(), wrap::(s), code) }; + T::on_connect_error(t, wrap::(s), code); } } fn on_connecting_error(c: *mut ConnectingSocket, code: i32) { - let Some(this) = *ConnectingSocket::opaque_mut(c).ext::>>() else { + let Some(this) = *ConnectingSocket::opaque_mut(c).ext::>>() else { return; }; - // SAFETY: see `on_open`. - unsafe { - T::on_connect_error( - this.as_ptr(), - NewSocketHandler::::from_connecting(c), - code, - ) - }; + T::on_connect_error(this, NewSocketHandler::::from_connecting(c), code); } fn on_handshake( ext: &mut Self::Ext, @@ -342,121 +323,85 @@ where err: us_bun_verify_error_t, ) { let Some(this) = *ext else { return }; - // SAFETY: see `on_open`. - unsafe { T::on_handshake(this.as_ptr(), wrap::(s), ok as i32, err) }; + T::on_handshake(this, wrap::(s), ok as i32, err); } } impl RawSocketEvents for websocket_upgrade_client::NewHttpUpgradeClient { const HAS_ON_OPEN: bool = true; - unsafe fn on_open(this: *mut Self, s: NewSocketHandler) { - // SAFETY: caller upholds the `RawSocketEvents` contract — `this` is the - // live unique ext-slot owner under single-threaded dispatch; `handle_*` - // has the same precondition on `this`. - unsafe { Self::handle_open(this, s) } + fn on_open(this: ThisPtr, s: NewSocketHandler) { + Self::handle_open(this, s) } - unsafe fn on_data(this: *mut Self, s: NewSocketHandler, data: &[u8]) { - // SAFETY: see `on_open`. - unsafe { Self::handle_data(this, s, data) } + fn on_data(this: ThisPtr, s: NewSocketHandler, data: &[u8]) { + Self::handle_data(this, s, data) } - unsafe fn on_writable(this: *mut Self, s: NewSocketHandler) { - // SAFETY: see `on_open`. - unsafe { Self::handle_writable(this, s) } + fn on_writable(this: ThisPtr, s: NewSocketHandler) { + Self::handle_writable(this, s) } - unsafe fn on_close(this: *mut Self, s: NewSocketHandler, code: i32, reason: *mut c_void) { - // SAFETY: see `on_open`. - unsafe { Self::handle_close(this, s, code, reason) } + fn on_close(this: ThisPtr, s: NewSocketHandler, code: i32, reason: *mut c_void) { + Self::handle_close(this, s, code, reason) } - unsafe fn on_timeout(this: *mut Self, s: NewSocketHandler) { - // SAFETY: see `on_open`. - unsafe { Self::handle_timeout(this, s) } + fn on_timeout(this: ThisPtr, s: NewSocketHandler) { + Self::handle_timeout(this, s) } - unsafe fn on_long_timeout(this: *mut Self, s: NewSocketHandler) { - // SAFETY: see `on_open`. - unsafe { Self::handle_timeout(this, s) } + fn on_long_timeout(this: ThisPtr, s: NewSocketHandler) { + Self::handle_timeout(this, s) } - unsafe fn on_end(this: *mut Self, s: NewSocketHandler) { - // SAFETY: see `on_open`. - unsafe { Self::handle_end(this, s) } + fn on_end(this: ThisPtr, s: NewSocketHandler) { + Self::handle_end(this, s) } - unsafe fn on_connect_error(this: *mut Self, s: NewSocketHandler, code: i32) { - // SAFETY: see `on_open`. - unsafe { Self::handle_connect_error(this, s, code) } + fn on_connect_error(this: ThisPtr, s: NewSocketHandler, code: i32) { + Self::handle_connect_error(this, s, code) } - unsafe fn on_handshake( - this: *mut Self, + fn on_handshake( + this: ThisPtr, s: NewSocketHandler, ok: i32, err: bun_uws::us_bun_verify_error_t, ) { - // SAFETY: see `on_open`. - unsafe { Self::handle_handshake(this, s, ok, err) } + Self::handle_handshake(this, s, ok, err) } } impl RawSocketEvents for websocket_client::WebSocket { // No `on_open` override — adoption of an already-connected socket. - unsafe fn on_data(this: *mut Self, _s: NewSocketHandler, data: &[u8]) { - // SAFETY: caller upholds the `RawSocketEvents` contract — `this` points - // to the live unique ext-slot owner under single-threaded dispatch, so - // it is valid to forward/dereference here. - unsafe { Self::handle_data(this, data) } - } - unsafe fn on_writable(this: *mut Self, s: NewSocketHandler) { - // SAFETY: see `on_data`. - unsafe { - let _guard = bun_ptr::ScopedRef::new(this); - (*this).handle_writable(s) - } + fn on_data(this: ThisPtr, _s: NewSocketHandler, data: &[u8]) { + Self::handle_data(this, data) } - unsafe fn on_close(this: *mut Self, s: NewSocketHandler, code: i32, reason: *mut c_void) { - // SAFETY: see `on_data`. - unsafe { - let _guard = bun_ptr::ScopedRef::new(this); - (*this).handle_close(s, code, reason) - } + fn on_writable(this: ThisPtr, s: NewSocketHandler) { + let _guard = this.ref_guard(); + this.handle_writable(s) } - unsafe fn on_timeout(this: *mut Self, s: NewSocketHandler) { - // SAFETY: see `on_data`. - unsafe { - let _guard = bun_ptr::ScopedRef::new(this); - (*this).handle_timeout(s) - } + fn on_close(this: ThisPtr, s: NewSocketHandler, code: i32, reason: *mut c_void) { + let _guard = this.ref_guard(); + this.handle_close(s, code, reason) } - unsafe fn on_long_timeout(this: *mut Self, s: NewSocketHandler) { - // SAFETY: see `on_data`. - unsafe { - let _guard = bun_ptr::ScopedRef::new(this); - (*this).handle_timeout(s) - } + fn on_timeout(this: ThisPtr, s: NewSocketHandler) { + let _guard = this.ref_guard(); + this.handle_timeout(s) } - unsafe fn on_end(this: *mut Self, s: NewSocketHandler) { - // SAFETY: see `on_data`. - unsafe { - let _guard = bun_ptr::ScopedRef::new(this); - (*this).handle_end(s) - } + fn on_long_timeout(this: ThisPtr, s: NewSocketHandler) { + let _guard = this.ref_guard(); + this.handle_timeout(s) } - unsafe fn on_connect_error(this: *mut Self, s: NewSocketHandler, code: i32) { - // SAFETY: see `on_data`. - unsafe { - let _guard = bun_ptr::ScopedRef::new(this); - (*this).handle_connect_error(s, code) - } + fn on_end(this: ThisPtr, s: NewSocketHandler) { + let _guard = this.ref_guard(); + this.handle_end(s) + } + fn on_connect_error(this: ThisPtr, s: NewSocketHandler, code: i32) { + let _guard = this.ref_guard(); + this.handle_connect_error(s, code) } - unsafe fn on_handshake( - this: *mut Self, + fn on_handshake( + this: ThisPtr, s: NewSocketHandler, ok: i32, err: bun_uws::us_bun_verify_error_t, ) { - // SAFETY: see `on_data`. - unsafe { - let _guard = bun_ptr::ScopedRef::new(this); - (*this).handle_handshake(s, ok, err) - } + let _guard = this.ref_guard(); + this.handle_handshake(s, ok, err) } } @@ -556,7 +501,7 @@ impl_ns_socket_events_forward!(js_valkey::JSValkeyClient, js_valkey::SocketHandl // re-derives `&mut NewSocket` via the wrapper's `m_ptr`; a `&mut NewSocket` // argument formed by `PtrHandler` and protected through the dispatch frame // would alias that re-entrant borrow (Stacked-Borrows UB + `noalias` -// dead-store of the re-entrant write). `RawPtrHandler` passes `*mut Self`. +// dead-store of the re-entrant write). `RawPtrHandler` passes `ThisPtr`. pub type BunSocket = RawPtrHandler, SSL>; /// Listener accept path: the ext is uninitialised at on_open time (the C accept @@ -598,58 +543,52 @@ where // `Listener::listen`; the listener strictly outlives every accepted // socket and is read-only here. let ns = api::Listener::on_create::(unsafe { &*listener }, wrap::(s)); - // SAFETY: `on_create` returns a freshly-boxed `NewSocket`; the `*mut` - // `on_*` methods hold no `&mut NewSocket` across re-entrant JS calls. - unsafe { api::NewSocket::on_open(ns, wrap::(s)) }; + api::NewSocket::on_open(ns, wrap::(s)); } // Accepted sockets reach the remaining events as `.bun_socket_*` once // on_create has restamped them; if anything fires before that, route to // the freshly stashed NewSocket. fn on_close_no_ext(s: *mut us_socket_t, code: i32, reason: Option<*mut c_void>) { - if let Some(ns) = *us_socket_t::opaque_mut(s).ext::>>>() + if let Some(ns) = *us_socket_t::opaque_mut(s).ext::>>>() { - // SAFETY: `ns` is the live heap `NewSocket` stashed by `on_create`; - // dispatch is single-threaded. The raw-pointer `on_*` may free it, - // so dispatch via `*mut` only — never form `&mut NewSocket`. - // Applies to every ext-slot read in this impl. - swallow(unsafe { api::NewSocket::on_close(ns.as_ptr(), wrap::(s), code, reason) }); + // `ns` is the live heap `NewSocket` stashed by `on_create`. The + // `on_*` handlers may free it, so they take `ThisPtr`, never `&mut`. + swallow(api::NewSocket::on_close(ns, wrap::(s), code, reason)); } } fn on_data_no_ext(s: *mut us_socket_t, data: &[u8]) { - if let Some(ns) = *us_socket_t::opaque_mut(s).ext::>>>() + if let Some(ns) = *us_socket_t::opaque_mut(s).ext::>>>() { - // SAFETY: see `on_close_no_ext`. - unsafe { api::NewSocket::on_data(ns.as_ptr(), wrap::(s), data) }; + api::NewSocket::on_data(ns, wrap::(s), data); } } fn on_writable_no_ext(s: *mut us_socket_t) { - if let Some(ns) = *us_socket_t::opaque_mut(s).ext::>>>() + if let Some(ns) = *us_socket_t::opaque_mut(s).ext::>>>() { - // SAFETY: see `on_close_no_ext`. - unsafe { api::NewSocket::on_writable(ns.as_ptr(), wrap::(s)) }; + api::NewSocket::on_writable(ns, wrap::(s)); } } fn on_end_no_ext(s: *mut us_socket_t) { - if let Some(ns) = *us_socket_t::opaque_mut(s).ext::>>>() + if let Some(ns) = *us_socket_t::opaque_mut(s).ext::>>>() { - // SAFETY: see `on_close_no_ext`. - unsafe { api::NewSocket::on_end(ns.as_ptr(), wrap::(s)) }; + api::NewSocket::on_end(ns, wrap::(s)); } } fn on_timeout_no_ext(s: *mut us_socket_t) { - if let Some(ns) = *us_socket_t::opaque_mut(s).ext::>>>() + if let Some(ns) = *us_socket_t::opaque_mut(s).ext::>>>() { - // SAFETY: see `on_close_no_ext`. - unsafe { api::NewSocket::on_timeout(ns.as_ptr(), wrap::(s)) }; + api::NewSocket::on_timeout(ns, wrap::(s)); } } fn on_handshake_no_ext(s: *mut us_socket_t, ok: bool, err: us_bun_verify_error_t) { - if let Some(ns) = *us_socket_t::opaque_mut(s).ext::>>>() + if let Some(ns) = *us_socket_t::opaque_mut(s).ext::>>>() { - // SAFETY: see `on_close_no_ext`. - swallow(unsafe { - api::NewSocket::on_handshake(ns.as_ptr(), wrap::(s), ok as i32, err) - }); + swallow(api::NewSocket::on_handshake( + ns, + wrap::(s), + ok as i32, + err, + )); } } } diff --git a/src/runtime/webcore/Body.rs b/src/runtime/webcore/Body.rs index 76d07f970a08..d863dbc1fc96 100644 --- a/src/runtime/webcore/Body.rs +++ b/src/runtime/webcore/Body.rs @@ -621,7 +621,12 @@ impl ValueError { pub fn to_js(&mut self, global_object: &JSGlobalObject) -> JSValue { let js_value = match self { ValueError::AbortReason(reason) => reason.to_js(global_object), - ValueError::SystemError(system_error) => system_error.to_error_instance(global_object), + // `to_error_instance` consumes the error's string refs, and `to_js` + // takes `&mut self` — take the value out so a second call builds an + // empty error rather than releasing those refs twice. + ValueError::SystemError(system_error) => { + core::mem::take(system_error).to_error_instance(global_object) + } ValueError::Message(message) => message.to_error_instance(global_object), ValueError::TypeError(message) => message.to_type_error_instance(global_object), // do an early return in this case we don't need to create a new Strong diff --git a/test/js/bun/net/socket-dns-error.test.ts b/test/js/bun/net/socket-dns-error.test.ts index 2f8d7f1a91bd..6db4c3eda121 100644 --- a/test/js/bun/net/socket-dns-error.test.ts +++ b/test/js/bun/net/socket-dns-error.test.ts @@ -1,4 +1,5 @@ import { expect, test } from "bun:test"; +import { bunEnv, bunExe } from "harness"; // `Bun.connect` to a hostname that fails to resolve must surface the resolver // error (code `ENOTFOUND`, `syscall: "getaddrinfo"`, `hostname`), matching @@ -62,6 +63,44 @@ test("Bun.connect rejects the promise with the resolver error when connectError expect(pick(error)).toEqual(EXPECTED); }); +test("a resolver error delivered to both connectError() and the promise is not released twice", async () => { + // The resolver error owns heap-allocated strings (hostname, message). When a + // `connectError` handler is present AND the connect promise is still pending, + // the error is turned into a JS Error twice — once for the callback, once for + // the rejection. Each conversion used to release the strings, so the second + // JS Error's strings were freed while it still referenced them: a double-free + // that surfaced as a use-after-free in the next JSString sweep. + // + // A subprocess so the GC that sweeps both Errors is ours to force. + await using proc = Bun.spawn({ + cmd: [ + bunExe(), + "-e", + ` + const host = Buffer.alloc(64, "a").toString() + ".com"; + for (let i = 0; i < 5; i++) { + await Bun.connect({ + hostname: host, + port: 80, + // Returns undefined, so the promise is rejected too. + socket: { open() {}, data() {}, connectError() {} }, + }).then(() => { throw new Error("expected a rejection"); }, () => {}); + } + // Sweep the Errors from both paths, destroying every JSString they hold. + for (let i = 0; i < 10; i++) Bun.gc(true); + console.log("ok"); + `, + ], + env: bunEnv, + stdout: "pipe", + stderr: "pipe", + }); + + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect({ stdout, exitCode }).toEqual({ stdout: "ok\n", exitCode: 0 }); + void stderr; +}); + test("consecutive Bun.connect calls to the same unresolvable hostname all get the resolver error", async () => { // The second attempt exercises the in-process DNS cache, which used to take // a different code path and report a different (also wrong) error. diff --git a/test/js/bun/net/socket.test.ts b/test/js/bun/net/socket.test.ts index bd9bcf9c2dfa..436c4b5aeda8 100644 --- a/test/js/bun/net/socket.test.ts +++ b/test/js/bun/net/socket.test.ts @@ -1776,3 +1776,131 @@ it.concurrent("setTypeOfService validates its argument instead of asserting", as }); void stderr; }); + +it("socket handler validation errors throw instead of crashing", async () => { + // Handlers protects its callbacks only after validation succeeds, so the + // validation error paths must throw without tearing down a never-protected + // Handlers (debug builds assert on the protect/unprotect balance). Run in + // a subprocess so a panic is observable as a non-zero exit instead of + // killing the test runner. + await using proc = spawn({ + cmd: [ + bunExe(), + "-e", + ` + for (const socket of [{}, { data() {}, end: 123 }]) { + for (const api of ["connect", "listen"]) { + try { + Bun[api]({ hostname: "localhost", port: 0, socket }); + } catch (e) { + console.log(api + ":" + e.message); + } + } + } + Bun.gc(true); + `, + ], + env: bunEnv, + stdout: "pipe", + stderr: "pipe", + }); + + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect(stdout).toBe( + 'connect:Expected at least "data" or "drain" callback\n' + + 'listen:Expected at least "data" or "drain" callback\n' + + 'connect:Expected "onEnd" callback to be a function\n' + + 'listen:Expected "onEnd" callback to be a function\n', + ); + expect(exitCode).toBe(0); + void stderr; +}); + +// https://bun-p9.sentry.io/issues/7573683042/ (BUN-3PK7) +it("socket handler validation errors don't steal GC protection from live sockets sharing the same callbacks", async () => { + // node:net passes one module-level handler table to every connection, so + // every live socket protects the same JSFunction identities. A Handlers + // dropped on a validation error before protect() ran must not gcUnprotect + // those shared functions, or GC collects them while a live socket's + // Handlers still points at the freed cells and the socket's finalizer + // later dereferences cell->vm() inside Bun__JSValue__unprotect. + await using proc = spawn({ + cmd: [ + bunExe(), + "-e", + ` + let listener; + let errors = 0; + (function setup() { + // Function expressions (not declarations) so this IIFE is their + // only JS root; once setup() returns, the listener's Handlers' + // gcProtect is the sole thing keeping them alive. + const open = function (s) { s.write("hello"); }; + const close = function () {}; + const data = function (s) { s.end(); }; + const drain = function () {}; + const error = function () {}; + const handshake = function () {}; + + listener = Bun.listen({ + hostname: "127.0.0.1", + port: 0, + socket: { open, close, data, drain, error, handshake }, + }); + + // Each validation error drops a Handlers whose open/close/data/ + // drain/error/handshake slots were assigned from the shared + // functions but never protect()ed. On an unguarded build each + // such drop issues one gcUnprotect per shared callback and the + // first one zeroes the listener's protection. + for (let i = 0; i < 4; i++) { + for (const api of ["connect", "listen"]) { + try { + Bun[api]({ + hostname: "127.0.0.1", + port: api === "connect" ? listener.port : 0, + socket: { open, close, data, drain, error, handshake, session: 1 }, + }); + } catch { errors++; } + } + } + })(); + console.log("errors=" + errors); + + // No JS roots remain for the shared callbacks; if protection was + // stolen they are now collectible. + for (let i = 0; i < 20; i++) Bun.gc(true); + + // The listener's Handlers still holds the shared callbacks; they + // must be live for open -> data -> end to round-trip. + const { promise, resolve, reject } = Promise.withResolvers(); + Bun.connect({ + hostname: "127.0.0.1", + port: listener.port, + socket: { + open() {}, + data(s, b) { resolve(b.toString()); s.end(); }, + close() {}, + error(_s, e) { reject(e); }, + connectError(_s, e) { reject(e); }, + }, + }).then(s => { s; }, reject); + console.log("received=" + (await promise)); + + // And they must survive the listener's own teardown. + listener.stop(true); + listener = null; + for (let i = 0; i < 20; i++) Bun.gc(true); + console.log("done"); + `, + ], + env: { ...bunEnv, ...(isWindows ? {} : { Malloc: "1" }) }, + stdout: "pipe", + stderr: "pipe", + }); + + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect(stdout).toBe("errors=8\nreceived=hello\ndone\n"); + expect(exitCode).toBe(0); + void stderr; +});