From 41859c325f4716e5c9e280e5766db68dfc53053f Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Thu, 6 Aug 2026 19:15:34 +0000 Subject: [PATCH] socket: keep pending exceptions out of the event loop when a promise settle or TLS dispatch buffer fails A JsResult Err from these paths means a JS exception is still pending on the VM. Several dispatch sites discarded that Err and returned to the uSockets/libuv event loop with the exception still set, so the next native JSC call in the same tick ran with it (in debug/ASAN builds that trips assertNoExceptionExceptTermination; in release the stale exception gets attributed to unrelated code). The exception has to be dealt with at the failure site, before the dispatch's scope guards drop: ScopeExit runs a microtask checkpoint on exit, and entering that checkpoint with a non-termination exception pending is itself the asserted condition. Each failed settle now reports its pending exception as unhandled right there (report_active_exception_as_unhandled, the same idiom the socket error handler, ipc, h2, fs-watcher and dns completions use). Termination exceptions stay pending, as the event loop expects. - handle_connect_error: report at the three promise-settle sites (the unwrap_or(true) swallow, the no-callback reject, reject_as_handled). - on_open: report a failed connect-promise resolve (previously swallowed with the exception left pending) and a failed reject of the open-callback error. - on_session / on_keylog: report a Buffer-allocation failure instead of returning Err into dispatch sites that discarded it. - All of the above, plus on_handshake and on_close (whose Err paths were structurally dead), become infallible so no call site can discard an Err from them again; every 'let _ =' at their dispatch call sites is gone. The session/keylog Buffer allocation only fails on JSC heap OOM, so a new session_buffer fault-injection rule (mirroring ssl_loop_buffer) simulates that throw deterministically; the fixture asserts the failure surfaces as an unhandled error and the event loop survives. Without the fix the same fixture dies on the ExceptionScope assert. --- .../bun-usockets/src/internal/fault_inject.h | 6 + src/js/internal-for-testing.ts | 9 +- src/runtime/socket/Listener.rs | 2 +- src/runtime/socket/WindowsNamedPipeContext.rs | 20 +- src/runtime/socket/mod.rs | 6 +- src/runtime/socket/socket_body.rs | 200 ++++++++----- src/runtime/socket/uws_dispatch.rs | 4 +- src/uws_sys/lib.rs | 5 + test/js/bun/net/socket-session-oom-fixture.ts | 93 ++++++ test/js/bun/net/socket.test.ts | 265 +++++++++++++++++- 10 files changed, 521 insertions(+), 89 deletions(-) create mode 100644 test/js/bun/net/socket-session-oom-fixture.ts diff --git a/packages/bun-usockets/src/internal/fault_inject.h b/packages/bun-usockets/src/internal/fault_inject.h index 9923ecd6a320..4b4496bca19d 100644 --- a/packages/bun-usockets/src/internal/fault_inject.h +++ b/packages/bun-usockets/src/internal/fault_inject.h @@ -48,6 +48,12 @@ enum us_fault_syscall { * always fresh from the kernel here, so the failure path is unreachable * without fault injection. Only US_FAULT_ERRNO applies. */ US_FAULT_POLL_START, + /* Not a syscall: the JS Buffer allocated for a TLS session/keylog payload + * in the Rust on_session/on_keylog dispatch. It only fails on JSC heap + * OOM, so the failure path is unreachable without injection. Only + * US_FAULT_ERRNO applies, and the errno value is ignored — the simulated + * failure is a thrown JS out-of-memory error, not an errno. */ + US_FAULT_SESSION_BUFFER, US_FAULT_COUNT }; diff --git a/src/js/internal-for-testing.ts b/src/js/internal-for-testing.ts index 39663d1026fb..f0a58529df38 100644 --- a/src/js/internal-for-testing.ts +++ b/src/js/internal-for-testing.ts @@ -327,8 +327,8 @@ export const setSocketOptions: setSocketOptionsFn = $newRustFunction( /** * The syscalls instrumented in bsd.c, plus non-syscall hooks whose failure * paths are otherwise unreachable without injection ("ssl_loop_buffer", - * "poll_start"; see fault_inject.h for the per-hook description). Arming - * anything else is rejected. + * "poll_start", "session_buffer"; see fault_inject.h for the per-hook + * description). Arming anything else is rejected. */ export type SocketFaultSyscall = | "recv" @@ -339,7 +339,8 @@ export type SocketFaultSyscall = | "connect" | "accept" | "ssl_loop_buffer" - | "poll_start"; + | "poll_start" + | "session_buffer"; export type SocketFaultRule = { syscall: SocketFaultSyscall; @@ -366,7 +367,7 @@ export type SocketFaultRule = { after?: number; /** fire this many times then disarm; -1 = forever. Default 1. */ repeat?: number; - /** match only this fd; -1 (default) = any. Rejected for "ssl_loop_buffer", which has no fd. */ + /** match only this fd; -1 (default) = any. Rejected for "ssl_loop_buffer" and "session_buffer", which have no fd. */ fd?: number; }; diff --git a/src/runtime/socket/Listener.rs b/src/runtime/socket/Listener.rs index 8164c943e028..046a392948e9 100644 --- a/src/runtime/socket/Listener.rs +++ b/src/runtime/socket/Listener.rs @@ -1624,7 +1624,7 @@ fn connect_finish( }; { let this = socket; - let _ = NewSocket::::handle_connect_error(this, errno, 0); + NewSocket::::handle_connect_error(this, errno, 0); // Balance the unconditional `socket_ref.ref_()` above. NewSocket::deref(&this); } diff --git a/src/runtime/socket/WindowsNamedPipeContext.rs b/src/runtime/socket/WindowsNamedPipeContext.rs index d5075acf5902..34d12c03d2ce 100644 --- a/src/runtime/socket/WindowsNamedPipeContext.rs +++ b/src/runtime/socket/WindowsNamedPipeContext.rs @@ -192,21 +192,21 @@ impl WindowsNamedPipeContext { // 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 } { - let _ = TLSSocket::on_session(s, session); + TLSSocket::on_session(s, session); } } fn on_keylog(this: *mut Self, line: &[u8]) { // SAFETY: see `on_open`. if let SocketType::Tls(s) = unsafe { (*this).socket } { - let _ = TLSSocket::on_keylog(s, line); + 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 (socket, pipe) = unsafe { ((*this).socket, ptr::addr_of_mut!((*this).named_pipe)) }; - match_socket!(socket, |s: NewSocket| _ = NewSocket::on_handshake( + match_socket!(socket, |s: NewSocket| NewSocket::on_handshake( s, socket_from_named_pipe::(pipe), success as i32, @@ -243,8 +243,11 @@ impl WindowsNamedPipeContext { s.handle_error(js_err); }); } else { - match_socket!(socket, |s: NewSocket| _ = - 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 + )); } } @@ -266,7 +269,7 @@ impl WindowsNamedPipeContext { (socket, ptr::addr_of_mut!((*this).named_pipe)) }; match_socket!(socket, |s: NewSocket| { - _ = NewSocket::on_close(s, socket_from_named_pipe::(pipe), 0, None); + NewSocket::on_close(s, socket_from_named_pipe::(pipe), 0, None); // Release the +1 ref taken in `create()`. s.get().deref(); }); @@ -302,8 +305,9 @@ impl WindowsNamedPipeContext { 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)); + 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) }; diff --git a/src/runtime/socket/mod.rs b/src/runtime/socket/mod.rs index d4986ec75dd0..648b6371098a 100644 --- a/src/runtime/socket/mod.rs +++ b/src/runtime/socket/mod.rs @@ -146,7 +146,7 @@ impl uws_handlers::RawSocketEvents for NewSocket { code: i32, reason: *mut core::ffi::c_void, ) { - let _ = NewSocket::on_close( + NewSocket::on_close( this, s, code, @@ -167,7 +167,7 @@ impl uws_handlers::RawSocketEvents for NewSocket { s: bun_uws::NewSocketHandler, code: i32, ) { - let _ = NewSocket::on_connect_error(this, s, code); + NewSocket::on_connect_error(this, s, code); } #[inline] fn on_handshake( @@ -176,6 +176,6 @@ impl uws_handlers::RawSocketEvents for NewSocket { ok: i32, err: bun_uws_sys::us_bun_verify_error_t, ) { - let _ = NewSocket::on_handshake(this, s, ok, err); + NewSocket::on_handshake(this, s, ok, err); } } diff --git a/src/runtime/socket/socket_body.rs b/src/runtime/socket/socket_body.rs index f21f78dfe632..3c4ff4cba348 100644 --- a/src/runtime/socket/socket_body.rs +++ b/src/runtime/socket/socket_body.rs @@ -1043,11 +1043,15 @@ 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). - pub(crate) fn handle_connect_error( - this: bun_ptr::ThisPtr, - errno: c_int, - dns_error: i32, - ) -> JsResult<()> { + /// Infallible by design: a failed promise settle below means a JS + /// exception is pending, and it must be dealt with *here*, before the + /// scope guards drop — `ScopeExit` → `exit_event_loop()` runs a microtask + /// checkpoint, which must not be entered with a non-termination exception + /// pending (and the event-loop dispatch callers have no JS frame to + /// propagate into anyway). `report_active_exception_as_unhandled` reports + /// and clears a real throw; a termination exception stays pending, as the + /// event loop expects. + pub(crate) fn handle_connect_error(this: bun_ptr::ThisPtr, errno: c_int, dns_error: i32) { let handlers = this.get_handlers(); log!( "onConnectError {} ({}, {})", @@ -1089,7 +1093,7 @@ impl NewSocket { if vm.script_execution_status() != jsc::ScriptExecutionStatus::Running { drop(cleanup); - return Ok(()); + return; } let callback = handlers.on_connect_error(); @@ -1184,10 +1188,12 @@ impl NewSocket { // reject the promise on connect() error 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))?; + if let Err(e) = js_promise.reject(&global, Ok(err_value)) { + global.report_active_exception_as_unhandled(e.into()); + } } - return Ok(()); + return; } let this_value = this.get_this_value(&global); @@ -1203,15 +1209,20 @@ impl NewSocket { Err(e) => global.take_exception(e), }; if global.has_exception() { - return Ok(()); + return; } if let Some(err_val) = result.to_error() { - // TODO: properly propagate exception upwards - if handlers.reject_promise(err_val).unwrap_or(true) { - return Ok(()); + match handlers.reject_promise(err_val) { + Ok(true) => return, + Ok(false) => { + let _ = handlers.call_error_handler(this_value, &[this_value, err_val]); + } + Err(e) => { + global.report_active_exception_as_unhandled(e); + return; + } } - let _ = handlers.call_error_handler(this_value, &[this_value, err_val]); } 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. @@ -1219,12 +1230,13 @@ impl NewSocket { let err_ = err_for_promise .take() .to_error_instance_with_async_stack(&global, promise); - promise.reject_as_handled(&global, err_)?; + if let Err(e) = promise.reject_as_handled(&global, err_) { + global.report_active_exception_as_unhandled(e.into()); + } } // `_scope_guard` (declared after `cleanup`) drops first → scope.exit(); // then `cleanup` → needs_deref/markInactive/deref. - Ok(()) } /// Takes `ThisPtr` for the same re-entrancy reason as @@ -1233,9 +1245,9 @@ impl NewSocket { this: bun_ptr::ThisPtr, socket: SocketHandler, errno: c_int, - ) -> JsResult<()> { + ) { jsc::mark_binding!(); - Self::handle_connect_error(this, errno, socket.dns_error()) + Self::handle_connect_error(this, errno, socket.dns_error()); } pub(crate) fn mark_active(&self) { @@ -1485,8 +1497,13 @@ impl NewSocket { let this_value = this.get_this_value(&global); this.mark_active(); - // TODO: properly propagate exception upwards - let _ = handlers.resolve_promise(this_value); + if let Err(e) = handlers.resolve_promise(this_value) { + // Event-loop dispatch: returning with the exception still pending + // would leak it into the next native JSC call in this tick. + // Reporting clears a real throw; termination stays pending and the + // check below keeps it out of the callbacks. + global.report_active_exception_as_unhandled(e); + } if global.has_exception() { return; } @@ -1519,8 +1536,15 @@ impl NewSocket { log!("Already closed"); } - // TODO: properly propagate exception upwards - let rejected = handlers.reject_promise(err).unwrap_or(true); + let rejected = match handlers.reject_promise(err) { + Ok(rejected) => rejected, + Err(e) => { + // Same discipline as `handle_connect_error`: the exception + // must not survive past this dispatch. + global.report_active_exception_as_unhandled(e); + true + } + }; if !rejected { let _ = handlers.call_error_handler(this_value, &[this_value, err]); } @@ -1644,19 +1668,19 @@ impl NewSocket { s: SocketHandler, success: i32, ssl_error: uws::us_bun_verify_error_t, - ) -> JsResult<()> { + ) { jsc::mark_binding!(); // 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(()); + return; } this.update_flags(|f| f.insert(Flags::HANDSHAKE_COMPLETE)); this.socket.set(s); if this.socket.get().is_detached() { - return Ok(()); + return; } // Keep the socket alive across the callbacks below (which re-enter JS) // and across `reject_unauthorized_connection`, whose close may @@ -1766,7 +1790,7 @@ impl NewSocket { if reject_unauthorized { this.reject_unauthorized_connection(); } - return Ok(()); + return; } // Use open callback when handshake is not provided @@ -1776,7 +1800,7 @@ impl NewSocket { if reject_unauthorized { this.reject_unauthorized_connection(); } - return Ok(()); + return; } is_open = true; } @@ -1818,16 +1842,15 @@ impl NewSocket { match super::uws_jsc::verify_error_to_js(&ssl_error, &global) { Ok(v) => v, Err(e) => { - // `Scope` has no Drop — balance event_loop().enter() and - // active_connections before propagating. + // Same discipline as `handle_connect_error`: report + // before the scope exit's microtask checkpoint, and + // before `on_close` re-enters JS. + global.report_active_exception_as_unhandled(e); this.exit_scope(scope); if reject_unauthorized { - // Take the pending exception before `on_close` re-enters JS. - let pending = global.take_exception(e); this.reject_unauthorized_connection(); - return Err(global.throw_value(pending)); } - return Err(e); + return; } } }; @@ -1849,7 +1872,6 @@ impl NewSocket { if reject_unauthorized { this.reject_unauthorized_connection(); } - Ok(()) } /// Stamps the resolved client `rejectUnauthorized` policy on a fresh or @@ -1877,6 +1899,34 @@ impl NewSocket { self.close_and_detach(uws::CloseCode::FastShutdown); } + /// The JS `Buffer` for an `on_session`/`on_keylog` payload. The + /// `session_buffer` fault rule simulates the allocation throwing: that + /// failure needs JSC heap exhaustion, so it is unreachable from a test + /// without injection. + fn create_dispatch_buffer(global: &JSGlobalObject, len: usize) -> JsResult { + #[cfg(socket_fault_injection)] + { + let mut out: isize = 0; + let mut clamp: c_int = 0; + // Deliberately skips `US_FAULT_CHECK`'s `us_fault_armed` fast-path + // load: session/keylog dispatch is a few calls per connection, not + // a per-packet hot path, so the unarmed-case lock is fine. + // SAFETY: `out`/`clamp` are valid for the duration of the call. + if unsafe { + bun_uws_sys::fault_inject::us_fault_hit( + bun_uws_sys::fault_inject::SESSION_BUFFER, + -1, + &mut out, + &mut clamp, + ) + } != 0 + { + return Err(global.throw_out_of_memory()); + } + } + JSValue::create_buffer_from_length(global, len) + } + /// A new resumable TLS session arrived (the peer's NewSessionTicket was /// processed during an earlier `SSL_read`). Hands the serialized session /// to the JS `session` handler, mirroring Node's `onnewsession` callback. @@ -1884,32 +1934,35 @@ impl NewSocket { /// unwound, so the JS handler may safely destroy the socket. /// /// Takes `ThisPtr` for the same re-entrancy reason as `on_writable`. - pub(crate) fn on_session(this: bun_ptr::ThisPtr, session: &[u8]) -> JsResult<()> { + pub(crate) fn on_session(this: bun_ptr::ThisPtr, session: &[u8]) { jsc::mark_binding!(); if this.socket.get().is_detached() { - return Ok(()); + return; } // Same late-event guard as the other dispatch entry points: the // socket may already have released its Handlers. if !this.has_handlers() { - return Ok(()); + return; } let handlers = this.get_handlers(); if handlers.vm.script_execution_status() != jsc::ScriptExecutionStatus::Running { - return Ok(()); + return; } let callback = handlers.on_session(); if callback.is_empty() { - return Ok(()); + return; } 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()) { + let buffer = match Self::create_dispatch_buffer(&global, session.len()) { Ok(b) => b, Err(e) => { + // Event-loop dispatch: same exception discipline as + // `on_connect_error`. + global.report_active_exception_as_unhandled(e); this.exit_scope(scope); - return Err(e); + return; } }; if let Some(ab) = buffer.as_array_buffer(&global) { @@ -1927,36 +1980,38 @@ impl NewSocket { let _ = handlers.call_error_handler(this_value, &[this_value, err_value]); } this.exit_scope(scope); - Ok(()) } /// Takes `ThisPtr` for the same re-entrancy reason as `on_writable`. - pub(crate) fn on_keylog(this: bun_ptr::ThisPtr, line: &[u8]) -> JsResult<()> { + pub(crate) fn on_keylog(this: bun_ptr::ThisPtr, line: &[u8]) { jsc::mark_binding!(); if this.socket.get().is_detached() { - return Ok(()); + return; } // Same late-event guard as the other dispatch entry points: the // socket may already have released its Handlers. if !this.has_handlers() { - return Ok(()); + return; } let handlers = this.get_handlers(); if handlers.vm.script_execution_status() != jsc::ScriptExecutionStatus::Running { - return Ok(()); + return; } let callback = handlers.on_keylog(); if callback.is_empty() { - return Ok(()); + return; } 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()) { + let buffer = match Self::create_dispatch_buffer(&global, line.len()) { Ok(b) => b, Err(e) => { + // Event-loop dispatch: same exception discipline as + // `on_connect_error`. + global.report_active_exception_as_unhandled(e); this.exit_scope(scope); - return Err(e); + return; } }; if let Some(ab) = buffer.as_array_buffer(&global) { @@ -1974,7 +2029,6 @@ impl NewSocket { let _ = handlers.call_error_handler(this_value, &[this_value, err_value]); } this.exit_scope(scope); - Ok(()) } /// Takes `ThisPtr` for the same re-entrancy reason as `on_writable`. @@ -1983,7 +2037,7 @@ impl NewSocket { socket: SocketHandler, err: c_int, reason: Option<*mut c_void>, - ) -> JsResult<()> { + ) { jsc::mark_binding!(); // 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 @@ -1996,7 +2050,7 @@ impl NewSocket { this.detach_native_callback(); this.socket.set(SocketHandler::::DETACHED); this.get().deref(); - return Ok(()); + return; } let handlers = this.get_handlers(); log!( @@ -2017,7 +2071,7 @@ impl NewSocket { // `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(); + Self::on_close(raw.into_this_ptr(), socket, err, reason); } let cleanup = CloseTeardown { socket: this, @@ -2026,7 +2080,7 @@ impl NewSocket { if this.flags.get().contains(Flags::FINALIZING) { drop(cleanup); - return Ok(()); + return; } let vm = handlers.vm; @@ -2036,12 +2090,12 @@ impl NewSocket { if callback.is_empty() { drop(cleanup); - return Ok(()); + return; } if vm.is_shutting_down() { drop(cleanup); - return Ok(()); + return; } // An earlier callback in this dispatch may have left a termination @@ -2049,7 +2103,7 @@ impl NewSocket { // mark_inactive(), landing here. Entering JS trips assertNoException(). if handlers.global_object.has_exception() { drop(cleanup); - return Ok(()); + return; } // the handlers must be kept alive for the duration of the function call @@ -2080,7 +2134,6 @@ impl NewSocket { } this.exit_scope(scope); drop(cleanup); - Ok(()) } /// Takes `ThisPtr` for the same re-entrancy reason as `on_writable`. @@ -4243,7 +4296,7 @@ impl DuplexUpgradeContext { // SAFETY: fn contract — `this` is the live ctx; all accesses are // scoped raw place reads/writes to disjoint fields. if let Some(tls) = unsafe { Self::tls_this_ptr(this) } { - let _ = TLSSocket::on_session(tls, session); + TLSSocket::on_session(tls, session); } } @@ -4251,7 +4304,7 @@ impl DuplexUpgradeContext { // SAFETY: fn contract — `this` is the live ctx; all accesses are // scoped raw place reads/writes to disjoint fields. if let Some(tls) = unsafe { Self::tls_this_ptr(this) } { - let _ = TLSSocket::on_keylog(tls, line); + TLSSocket::on_keylog(tls, line); } } @@ -4261,7 +4314,7 @@ impl DuplexUpgradeContext { unsafe { let socket = Self::duplex_socket(this); if let Some(tls) = Self::tls_this_ptr(this) { - let _ = TLSSocket::on_handshake(tls, socket, success as i32, ssl_error); + TLSSocket::on_handshake(tls, socket, success as i32, ssl_error); } } } @@ -4323,8 +4376,7 @@ impl DuplexUpgradeContext { // `upgrade` field, borrow ends at `;`. unsafe { (*this).upgrade.teardown() }; let p = tls.into_this_ptr(); - let _ = - TLSSocket::handle_connect_error(p, sys::SystemErrno::ECONNREFUSED as c_int, 0); + TLSSocket::handle_connect_error(p, sys::SystemErrno::ECONNREFUSED as c_int, 0); } } } @@ -4355,7 +4407,7 @@ impl DuplexUpgradeContext { // in `onError` instead of reading the Handlers that `tls.onClose` // → `markInactive` just released. let p = tls.into_this_ptr(); - let _ = TLSSocket::on_close(p, socket, 0, None); + TLSSocket::on_close(p, socket, 0, None); } // SAFETY: fn contract; may enqueue this ctx for deinit. @@ -4429,7 +4481,7 @@ impl DuplexUpgradeContext { // SAFETY: `this` is live; `&mut` ends before `deinit`. unsafe { (*this).upgrade.teardown() }; let p = tls.into_this_ptr(); - let _ = TLSSocket::handle_connect_error(p, errno, 0); + TLSSocket::handle_connect_error(p, errno, 0); } // `startTLS`/`startTLSWithCTX` failed before the // SSLWrapper was assigned, so its close callback @@ -5044,11 +5096,13 @@ pub mod testing_apis { fi::SSL_LOOP_BUFFER } else if syscall_str.eql_comptime(b"poll_start") { fi::POLL_START + } else if syscall_str.eql_comptime(b"session_buffer") { + fi::SESSION_BUFFER } else { // socket/close/shutdown have enum slots but no bsd.c hooks; // accepting them would arm rules that can never fire. return Err(global.throw(format_args!( - "rule.syscall must be one of: recv, send, writev, sendmsg, recvmsg, connect, accept, ssl_loop_buffer, poll_start" + "rule.syscall must be one of: recv, send, writev, sendmsg, recvmsg, connect, accept, ssl_loop_buffer, poll_start, session_buffer" ))); }; @@ -5133,13 +5187,19 @@ pub mod testing_apis { ))); } - // ssl_loop_buffer is an allocation, not a socket operation: its hook - // passes fd = -1, so a rule pinned to a descriptor would arm and then - // silently never fire. + // ssl_loop_buffer/session_buffer are allocations, not socket + // operations: their hooks pass fd = -1, so a rule pinned to a + // descriptor would arm and then silently never fire. let target_fd = get_i32("fd", -1)?; - if syscall == fi::SSL_LOOP_BUFFER && target_fd != -1 { + if (syscall == fi::SSL_LOOP_BUFFER || syscall == fi::SESSION_BUFFER) && target_fd != -1 + { return Err(global.throw(format_args!( - "rule.fd is not supported for syscall \"ssl_loop_buffer\"" + "rule.fd is not supported for syscall \"{}\"", + if syscall == fi::SSL_LOOP_BUFFER { + "ssl_loop_buffer" + } else { + "session_buffer" + } ))); } diff --git a/src/runtime/socket/uws_dispatch.rs b/src/runtime/socket/uws_dispatch.rs index 9870a1a9c72b..f08f6360085f 100644 --- a/src/runtime/socket/uws_dispatch.rs +++ b/src/runtime/socket/uws_dispatch.rs @@ -252,7 +252,7 @@ pub(crate) unsafe extern "C" fn us_dispatch_session( // 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) }; - let _ = TLSSocket::on_session(tls, slice); + TLSSocket::on_session(tls, slice); } /// Hands an NSS key-log line parked by the keylog callback to the JS @@ -283,5 +283,5 @@ pub(crate) unsafe extern "C" fn us_dispatch_keylog( // 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) }; - let _ = TLSSocket::on_keylog(tls, slice); + TLSSocket::on_keylog(tls, slice); } diff --git a/src/uws_sys/lib.rs b/src/uws_sys/lib.rs index 6975bc05ce29..b0da133ea77c 100644 --- a/src/uws_sys/lib.rs +++ b/src/uws_sys/lib.rs @@ -415,6 +415,9 @@ pub mod fault_inject { /// (`uv_poll_init_socket` on Windows, `EPOLL_CTL_ADD` / `kevent` on /// epoll/kqueue). pub const POLL_START: c_int = 11; + /// Not a syscall: the JS `Buffer` allocated for a TLS session/keylog + /// payload in the `on_session`/`on_keylog` dispatch. + pub const SESSION_BUFFER: c_int = 12; pub const ACTION_NONE: c_int = 0; pub const ACTION_ERRNO: c_int = 1; @@ -435,6 +438,8 @@ pub mod fault_inject { pub fn us_fault_set(syscall: c_int, rule: *const UsFaultRule); pub safe fn us_fault_clear(syscall: c_int); pub safe fn us_fault_clear_all(); + pub fn us_fault_hit(syscall: c_int, fd: c_int, out: *mut isize, clamp: *mut c_int) + -> c_int; } } pub use socket::{ diff --git a/test/js/bun/net/socket-session-oom-fixture.ts b/test/js/bun/net/socket-session-oom-fixture.ts new file mode 100644 index 000000000000..05d83102d3b5 --- /dev/null +++ b/test/js/bun/net/socket-session-oom-fixture.ts @@ -0,0 +1,93 @@ +// Fixture for the out-of-memory path of the TLS session dispatch Buffer. +// +// on_session/on_keylog allocate a JS Buffer for the payload before calling the +// handler. A throw from that allocation used to escape the dispatch with the +// exception left pending on the VM, so the next native JSC call in the same +// event-loop tick ran with a stale exception (asserts in debug builds, +// misattribution in release). The allocation only fails on JSC heap OOM, so +// the fault injector is the only way here from a test. +// +// The fixed build reports the failure as an unhandled error: the +// uncaughtException handler below receives it, the event loop stays healthy, +// and the process exits cleanly. +import { socketFaultInjection as fault } from "bun:internal-for-testing"; +import { tls as certs } from "harness"; + +if (!fault.available()) throw new Error("socket fault injection is not available in this build"); + +const { promise: gotUncaught, resolve: onUncaught } = Promise.withResolvers(); +process.on("uncaughtException", err => { + // Print every one: the driver asserts exactly one fired. + console.log("UNCAUGHT: " + String(err)); + onUncaught(err as Error); +}); + +const server = Bun.listen({ + hostname: "127.0.0.1", + port: 0, + tls: { cert: certs.cert, key: certs.key }, + socket: { + open(s) { + // The client only processes the server's NewSessionTicket during a + // read, so give it something to read. + s.write("x"); + }, + data() {}, + error() {}, + }, +}); + +// Fire on the first session dispatch only; later dispatches proceed normally. +fault.set({ syscall: "session_buffer", action: "errno", errno: "ENOMEM" }); +console.log("ARMED"); + +const socket = await Bun.connect({ + hostname: "127.0.0.1", + port: server.port, + tls: { ca: certs.cert, serverName: "localhost" }, + socket: { + data() {}, + close() {}, + error() {}, + session() {}, + }, +}); + +await gotUncaught; +fault.clear(); + +// The stale-exception bug corrupts whatever native JSC call runs next in the +// loop; prove the loop still works by finishing a full TCP round trip. +const echo = Bun.listen({ + hostname: "127.0.0.1", + port: 0, + socket: { + open(s) { + s.write("ok"); + s.end(); + }, + data() {}, + error() {}, + }, +}); +const { promise: roundTrip, resolve: onClose } = Promise.withResolvers(); +let received = ""; +await Bun.connect({ + hostname: "127.0.0.1", + port: echo.port, + socket: { + data(_s, d) { + received += d; + }, + close() { + onClose(received); + }, + error() {}, + }, +}); +console.log("ROUNDTRIP: " + (await roundTrip)); + +socket.end(); +server.stop(true); +echo.stop(true); +process.exit(0); diff --git a/test/js/bun/net/socket.test.ts b/test/js/bun/net/socket.test.ts index 7cfc7718ca80..f9e7ae1f55c1 100644 --- a/test/js/bun/net/socket.test.ts +++ b/test/js/bun/net/socket.test.ts @@ -1,6 +1,6 @@ import type { Socket } from "bun"; import { connect, fileURLToPath, SocketHandler, spawn } from "bun"; -import { createSocketPair } from "bun:internal-for-testing"; +import { createSocketPair, socketFaultInjection } from "bun:internal-for-testing"; import { describe, expect, it, jest } from "bun:test"; import { closeSync } from "fs"; import { @@ -16,6 +16,7 @@ import { tls, } from "harness"; import net from "node:net"; +import { join } from "node:path"; import { createSecureContext, connect as tlsConnect } from "node:tls"; describe.concurrent("socket", () => { it("should throw when a socket from a file descriptor has a bad file descriptor", async () => { @@ -3404,3 +3405,265 @@ it("an unref'd Bun.listen() with no other references keeps accepting across GC", exitCode: 0, }); }); + +describe.concurrent("TLS session/keylog handlers", () => { + // `session`/`keylog` dispatch from the event loop after the SSL stack + // unwinds. These pin the delivery contract for those native dispatch paths: + // Buffer payloads, and a throw from the handler is delivered to the + // socket's own error handler instead of being left pending on the VM. + function listenTls() { + return Bun.listen({ + hostname: "127.0.0.1", + port: 0, + tls, + socket: { + open(s) { + // The client only processes the server's NewSessionTicket during a + // read, so give it something to read. + s.write("x"); + }, + data() {}, + error() {}, + }, + }); + } + + it("session and keylog deliver Buffers to the client handlers", async () => { + const server = listenTls(); + const { promise, resolve, reject } = Promise.withResolvers(); + const keylogLines: Buffer[] = []; + try { + const socket = await Bun.connect({ + hostname: "127.0.0.1", + port: server.port, + tls: { ca: tls.cert, serverName: "localhost" }, + socket: { + data() {}, + error(_socket, err) { + reject(err); + }, + session(_socket, sessionBuffer) { + resolve(sessionBuffer); + }, + keylog(_socket, line) { + keylogLines.push(line); + }, + }, + }); + const session = await promise; + expect(session).toBeInstanceOf(Buffer); + expect(session.length).toBeGreaterThan(0); + // The handshake's key material was logged before the session ticket. + expect(keylogLines.length).toBeGreaterThan(0); + expect(keylogLines[0]).toBeInstanceOf(Buffer); + expect(keylogLines[0].toString()).toContain("_TRAFFIC_SECRET"); + socket.end(); + } finally { + server.stop(true); + } + }); + + it("a throw from session() is delivered to the socket's error handler", async () => { + const server = listenTls(); + const { promise, resolve } = Promise.withResolvers(); + const boom = new Error("boom-session"); + try { + const socket = await Bun.connect({ + hostname: "127.0.0.1", + port: server.port, + tls: { ca: tls.cert, serverName: "localhost" }, + socket: { + data() {}, + error(_socket, err) { + resolve(err as Error); + }, + session() { + throw boom; + }, + }, + }); + expect(await promise).toBe(boom); + socket.end(); + } finally { + server.stop(true); + } + }); + + it("a throw from keylog() is delivered to the socket's error handler", async () => { + const server = listenTls(); + const { promise, resolve } = Promise.withResolvers(); + const boom = new Error("boom-keylog"); + let thrown = false; + try { + const socket = await Bun.connect({ + hostname: "127.0.0.1", + port: server.port, + tls: { ca: tls.cert, serverName: "localhost" }, + socket: { + data() {}, + error(_socket, err) { + resolve(err as Error); + }, + keylog() { + if (!thrown) { + thrown = true; + throw boom; + } + }, + }, + }); + expect(await promise).toBe(boom); + socket.end(); + } finally { + server.stop(true); + } + }); + + // The session Buffer allocation can only fail on JSC heap OOM; the fault + // injector simulates exactly that throw. The fixture asserts the failure + // surfaces as an unhandled error (not a stale pending exception on the VM) + // and that the event loop survives it. + it.skipIf(!socketFaultInjection.available())( + "an injected session-buffer allocation failure is reported as an unhandled error", + async () => { + await using proc = Bun.spawn({ + cmd: [bunExe(), join(import.meta.dir, "socket-session-oom-fixture.ts")], + env: { ...bunEnv, BUN_CRASH_REPORT_URL: "", BUN_ENABLE_CRASH_REPORTING: "0" }, + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + const lines = stdout + .split("\n") + .map(line => line.trim()) + .filter(Boolean); + expect({ + armed: lines.includes("ARMED"), + // Exactly one: the injected failure, and nothing else leaking later. + uncaught: lines.filter(line => line.startsWith("UNCAUGHT:")), + roundTrip: lines.includes("ROUNDTRIP: ok"), + exitCode, + // Only populated when the assertion is about to fail, so the diff shows why. + stderrTail: exitCode === 0 ? "" : stderr.slice(-2000), + }).toEqual({ + armed: true, + uncaught: [expect.stringMatching(/out of memory/i)], + roundTrip: true, + exitCode: 0, + stderrTail: "", + }); + }, + // Symbolizing the crash backtrace of a debug/ASAN binary (the failure + // mode this test exists to catch) takes several seconds on its own. + 30_000, + ); +}); + +describe.concurrent("socket open() callback throw", () => { + // open() dispatch settles the connect promise before the callback runs, so + // a throw from open() must not reject the promise; it is delivered to the + // socket's own error handler. + it("client: connect() still resolves and the error handler receives the thrown error", async () => { + const boom = new Error("boom-client-open"); + const { promise, resolve } = Promise.withResolvers(); + const server = Bun.listen({ hostname: "127.0.0.1", port: 0, socket: { data() {} } }); + try { + const socket = await Bun.connect({ + hostname: "127.0.0.1", + port: server.port, + socket: { + open() { + throw boom; + }, + error(_socket, err) { + resolve(err as Error); + }, + data() {}, + }, + }); + expect(socket).toBeDefined(); + expect(await promise).toBe(boom); + socket.end(); + } finally { + server.stop(true); + } + }); + + it("server: the listener's error handler receives the thrown error", async () => { + const boom = new Error("boom-server-open"); + const { promise, resolve } = Promise.withResolvers(); + const server = Bun.listen({ + hostname: "127.0.0.1", + port: 0, + socket: { + open() { + throw boom; + }, + error(_socket, err) { + resolve(err as Error); + }, + data() {}, + }, + }); + try { + const client = await Bun.connect({ hostname: "127.0.0.1", port: server.port, socket: { data() {} } }); + expect(await promise).toBe(boom); + client.end(); + } finally { + server.stop(true); + } + }); +}); + +describe.concurrent("connect() failure promise settlement", () => { + it("a synchronous unix connect failure rejects the promise and fires connectError", async () => { + // Runs in a child with a relative unix path: an absolute tempdir path can + // exceed sun_path (104 bytes on macOS), where the usockets long-path + // fallback reports ENAMETOOLONG/EINVAL instead of the ENOENT this pins. + using dir = tempDir("socket-sync-connect-fail", {}); + await using proc = Bun.spawn({ + cmd: [ + bunExe(), + "-e", + `let cbCode = null; + try { + await Bun.connect({ + unix: "does-not-exist/sock.sock", + socket: { connectError(_s, e) { cbCode = e.code; }, data() {} }, + }); + console.log("RESOLVED"); + } catch (e) { + console.log("rejected:" + e.code + " connectError:" + cbCode); + }`, + ], + env: bunEnv, + cwd: String(dir), + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect(stdout.trim()).toBe("rejected:ENOENT connectError:ENOENT"); + expect(stderr).toBe(""); + expect(exitCode).toBe(0); + }); + + it("an error thrown by connectError() becomes the connect() rejection", async () => { + // Grab a port nothing is listening on anymore. + const probe = Bun.listen({ hostname: "127.0.0.1", port: 0, socket: { data() {} } }); + const port = probe.port; + probe.stop(true); + + const boom = new Error("boom-connect-error"); + await expect( + Bun.connect({ + hostname: "127.0.0.1", + port, + socket: { + connectError() { + throw boom; + }, + data() {}, + }, + }), + ).rejects.toBe(boom); + }); +});