From 6eb171f3245863713e663d7c822f046034283175 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Thu, 23 Jul 2026 23:43:32 +0000 Subject: [PATCH 1/3] sys: make SystemError own its strings via OwnedString bun_sys::SystemError and bun_jsc::SystemError held six bare bun_core::String fields. Because String is Copy (to match the C++ BunString ABI) it cannot impl Drop, so neither struct released its strings on drop; every consumer had to hand-call .deref() on every exit path. Several shell builtins and the IOWriter error path did not, leaking the WTF::StringImpl behind each path/message. Move the release into the type system: - Both SystemError structs now hold OwnedString (the existing repr(transparent) RAII wrapper). The C++ layout is unchanged. - bun_sys::SystemError.fd becomes Option; the bun_jsc struct keeps c_int for the C ABI and From maps None to c_int::MIN. - SystemError::deref/ref_/dupe are removed. jsc::SystemError derives Clone (OwnedString::clone bumps the refcount). - SystemErrorJsc trait methods take self by value; the three hand-rolled sys->jsc bridge fns are folded into From. - All manual .deref() calls and ShellErr::deinit are deleted. 36 files, +313/-467. --- src/jsc/SystemError.rs | 113 +++++++------------ src/runtime/api/bun/js_bun_spawn_bindings.rs | 36 ++---- src/runtime/api/html_rewriter.rs | 4 +- src/runtime/dns_jsc/cares_jsc.rs | 24 ++-- src/runtime/dns_jsc/dns.rs | 23 ++-- src/runtime/ffi/ffi_body.rs | 12 +- src/runtime/node/node_fs.rs | 12 +- src/runtime/node/node_os.rs | 80 ++++++------- src/runtime/node/types.rs | 4 +- src/runtime/server/RequestContext.rs | 9 +- src/runtime/server/mod.rs | 21 ++-- src/runtime/shell/builtin/basename.rs | 4 +- src/runtime/shell/builtin/cat.rs | 9 +- src/runtime/shell/builtin/dirname.rs | 4 +- src/runtime/shell/builtin/mv.rs | 4 +- src/runtime/shell/builtin/rm.rs | 4 +- src/runtime/shell/builtin/seq.rs | 4 +- src/runtime/shell/builtin/yes.rs | 6 +- src/runtime/shell/shell_body.rs | 21 ---- src/runtime/shell/states/Cmd.rs | 4 +- src/runtime/shell/states/CondExpr.rs | 4 +- src/runtime/shell/subproc.rs | 10 +- src/runtime/socket/Listener.rs | 25 ++-- src/runtime/socket/socket_body.rs | 47 ++------ src/runtime/socket/udp_socket.rs | 11 +- src/runtime/webcore/Blob.rs | 9 +- src/runtime/webcore/Body.rs | 2 +- src/runtime/webcore/ResumableSink.rs | 14 +-- src/runtime/webcore/blob/copy_file.rs | 29 ++--- src/runtime/webcore/blob/read_file.rs | 29 ++--- src/runtime/webcore/fetch/FetchTasklet.rs | 16 +-- src/sys/Error.rs | 21 ++-- src/sys/lib.rs | 53 ++------- src/sys_jsc/fd_jsc.rs | 4 +- src/sys_jsc/lib.rs | 41 ++----- test/js/bun/shell/leak.test.ts | 67 ++++++++++- 36 files changed, 313 insertions(+), 467 deletions(-) diff --git a/src/jsc/SystemError.rs b/src/jsc/SystemError.rs index a1567c5b2007..8856495836df 100644 --- a/src/jsc/SystemError.rs +++ b/src/jsc/SystemError.rs @@ -1,54 +1,65 @@ use core::ffi::c_int; use core::fmt; -use bun_core::String; +use bun_core::{OwnedString, String}; use crate::{JSGlobalObject, JSPromise, JSValue}; #[repr(C)] +#[derive(Clone)] pub struct SystemError { pub errno: c_int, /// label for errno - pub code: String, + pub code: OwnedString, /// it is illegal to have an empty message - pub message: String, - pub path: String, - pub syscall: String, - pub hostname: String, + pub message: OwnedString, + pub path: OwnedString, + pub syscall: OwnedString, + pub hostname: OwnedString, /// MinInt = no file descriptor pub fd: c_int, - pub dest: String, + pub dest: OwnedString, } impl Default for SystemError { fn default() -> Self { Self { errno: 0, - code: String::default(), - message: String::default(), - path: String::default(), - syscall: String::default(), - hostname: String::default(), + code: OwnedString::default(), + message: OwnedString::default(), + path: OwnedString::default(), + syscall: OwnedString::default(), + hostname: OwnedString::default(), fd: c_int::MIN, - dest: String::default(), + dest: OwnedString::default(), } } } -/// Reshape the T1 `bun_sys::SystemError` (non-`#[repr(C)]`, different field -/// order) into the `#[repr(C)]` extern layout C++ reads. Data (T1) is split -/// from the JSC bridge (T6) — this `From` is the canonical layering seam. +/// Reshape the T1 `bun_sys::SystemError` into the `#[repr(C)]` extern layout +/// C++ reads. Data (T1) is split from the JSC bridge (T6) — this `From` is +/// the canonical layering seam. impl From for SystemError { fn from(e: bun_sys::SystemError) -> Self { + let bun_sys::SystemError { + errno, + code, + message, + path, + syscall, + hostname, + fd, + dest, + } = e; Self { - errno: e.errno as c_int, - code: e.code, - message: e.message, - path: e.path, - syscall: e.syscall, - hostname: e.hostname, - fd: e.fd as c_int, - dest: e.dest, + errno: errno as c_int, + code, + message, + path, + syscall, + hostname, + fd: fd.unwrap_or(c_int::MIN), + dest, } } } @@ -73,49 +84,11 @@ impl SystemError { bun_sys::e_from_negated(self.errno) } - /// 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(); - self.syscall.deref(); - self.hostname.deref(); - self.dest.deref(); - } - - pub fn ref_(&mut self) { - self.path.ref_(); - self.code.ref_(); - self.message.ref_(); - self.syscall.ref_(); - self.hostname.ref_(); - self.dest.ref_(); - } - - /// Bitwise-copy + bump every `bun_core::String` ref. - /// `bun_core::String` has no `Clone` impl (intrusive WTF refcount), so - /// `#[derive(Clone)]` is unavailable; this is the manual equivalent. - pub fn dupe(&self) -> SystemError { - // SAFETY: `SystemError` is `#[repr(C)]` and every field is either `c_int` - // (trivially copyable) or `bun_core::String` — a `#[repr(C)]` smart-ptr - // whose bitwise copy is sound provided we immediately bump each ref - // (preventing a double-free on drop). - let mut v: SystemError = unsafe { core::ptr::read(self) }; - v.ref_(); - v - } - - /// 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. + /// Converts to a JS `Error`, consuming `self`. C++ only borrows the string + /// fields; `Drop` releases them when `self` goes out of scope. `.clone()` + /// first 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 + SystemError__toErrorInstance(&self, global) } /// Like `to_error_instance` but populates the error's stack trace with async @@ -152,9 +125,7 @@ impl SystemError { /// 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); - self.deref(); - result + SystemError__toErrorInstanceWithInfoObject(&self, global) } } @@ -173,8 +144,8 @@ pub fn verify_error_to_js( let reason: &[u8] = err.reason_bytes(); let fallback = SystemError { - code: String::clone_utf8(code), - message: String::clone_utf8(reason), + code: String::clone_utf8(code).into(), + message: String::clone_utf8(reason).into(), ..Default::default() }; diff --git a/src/runtime/api/bun/js_bun_spawn_bindings.rs b/src/runtime/api/bun/js_bun_spawn_bindings.rs index ba915d3eac36..30473cce9cb8 100644 --- a/src/runtime/api/bun/js_bun_spawn_bindings.rs +++ b/src/runtime/api/bun/js_bun_spawn_bindings.rs @@ -53,22 +53,6 @@ fn signal_code_from_js(val: JSValue, global: &JSGlobalObject) -> JsResult JSValue { - let jsc_err = SystemError { - errno: err.errno, - code: err.code, - message: err.message, - path: err.path, - syscall: err.syscall, - hostname: err.hostname, - fd: err.fd, - dest: err.dest, - }; - jsc_err.to_error_instance(global) -} - /// `Terminal.CreateResult` — local mirror that flattens `IntrusiveRc` /// to a `BackRef` used by `Subprocess.terminal`, so the scopeguard / /// field-assignment paths share one pointer type with `existing_terminal`. @@ -1211,7 +1195,8 @@ pub(crate) fn spawn_maybe_sync( } else { -UV_E::NFILE }; - return Err(global_this.throw_value(sys_system_error_to_js(&systemerror, global_this))); + return Err(global_this + .throw_value(SystemError::from(systemerror).to_error_instance(global_this))); } Err(err) => { // See EMFILE arm above. @@ -1241,8 +1226,9 @@ pub(crate) fn spawn_maybe_sync( if errno == sys::Errno::ENOENT { systemerror.errno = -UV_E::NOENT; } - return Err(global_this - .throw_value(sys_system_error_to_js(&systemerror, global_this))); + return Err(global_this.throw_value( + SystemError::from(systemerror).to_error_instance(global_this), + )); } } _ => {} @@ -2076,14 +2062,12 @@ fn throw_command_not_found(global_this: &JSGlobalObject, command: &[u8]) -> JsEr message: BunString::create_format(format_args!( "Executable not found in $PATH: \"{}\"", bstr::BStr::new(command) - )), - code: BunString::static_("ENOENT"), + )) + .into(), + code: BunString::static_("ENOENT").into(), errno: -UV_E::NOENT, - path: BunString::clone_utf8(command), - syscall: BunString::EMPTY, - hostname: BunString::EMPTY, - fd: -1, - dest: BunString::EMPTY, + path: BunString::clone_utf8(command).into(), + ..Default::default() }; global_this.throw_value(err.to_error_instance(global_this)) } diff --git a/src/runtime/api/html_rewriter.rs b/src/runtime/api/html_rewriter.rs index 67c55d538a20..32760a0875ee 100644 --- a/src/runtime/api/html_rewriter.rs +++ b/src/runtime/api/html_rewriter.rs @@ -60,8 +60,8 @@ fn cell_get<'a, T>(cell: &Cell<*mut T>) -> Option<&'a mut T> { /// Construct a `SystemError` with code+message and remaining fields defaulted. fn system_error(code: &'static str, message: &'static str) -> SystemError { SystemError { - code: BunString::static_(code), - message: BunString::static_(message), + code: BunString::static_(code).into(), + message: BunString::static_(message).into(), ..Default::default() } } diff --git a/src/runtime/dns_jsc/cares_jsc.rs b/src/runtime/dns_jsc/cares_jsc.rs index d804fcfc3a39..253700356538 100644 --- a/src/runtime/dns_jsc/cares_jsc.rs +++ b/src/runtime/dns_jsc/cares_jsc.rs @@ -679,10 +679,10 @@ impl ErrorDeferred { }; let system_error = SystemError { errno: self.errno as i32, - code: bstr::String::static_(code), - message, - syscall: bstr::String::clone_utf8(self.syscall), - hostname: self.hostname.take().unwrap_or(bstr::String::empty()), + code: bstr::String::static_(code).into(), + message: message.into(), + syscall: bstr::String::clone_utf8(self.syscall).into(), + hostname: self.hostname.take().unwrap_or(bstr::String::empty()).into(), ..Default::default() }; @@ -765,13 +765,14 @@ pub(crate) fn error_to_js_with_syscall( let code = this.code(); let instance = SystemError { errno: this as i32, - code: bstr::String::static_(&code[4..]), - syscall: bstr::String::static_(syscall), + code: bstr::String::static_(&code[4..]).into(), + syscall: bstr::String::static_(syscall).into(), message: bstr::String::create_format(format_args!( "{} {}", BStr::new(syscall), BStr::new(&code[4..]) - )), + )) + .into(), ..Default::default() } .to_error_instance(global_this); @@ -796,15 +797,16 @@ pub(crate) fn system_error_with_syscall_and_hostname( let code = this.code(); SystemError { errno: this as i32, - code: bstr::String::static_(&code[4..]), + code: bstr::String::static_(&code[4..]).into(), message: bstr::String::create_format(format_args!( "{} {} {}", BStr::new(syscall), BStr::new(&code[4..]), BStr::new(hostname) - )), - syscall: bstr::String::static_(syscall), - hostname: bstr::String::clone_utf8(hostname), + )) + .into(), + syscall: bstr::String::static_(syscall).into(), + hostname: bstr::String::clone_utf8(hostname).into(), ..Default::default() } } diff --git a/src/runtime/dns_jsc/dns.rs b/src/runtime/dns_jsc/dns.rs index 88533da4b7ed..210599168691 100644 --- a/src/runtime/dns_jsc/dns.rs +++ b/src/runtime/dns_jsc/dns.rs @@ -4746,13 +4746,9 @@ impl Resolver { ChannelResult::Err(err) => { let system_error = SystemError { errno: -1, - code: bun_core::String::static_(err.code()), - message: bun_core::String::static_(err.label()), - path: bun_core::String::default(), - syscall: bun_core::String::default(), - hostname: bun_core::String::default(), - fd: -1, - dest: bun_core::String::default(), + code: bun_core::String::static_(err.code()).into(), + message: bun_core::String::static_(err.label()).into(), + ..Default::default() }; Err(global_this.throw_value(system_error.to_error_instance(global_this))) } @@ -5453,17 +5449,12 @@ impl Resolver { ChannelResult::Result(res) => res, ChannelResult::Err(err) => { let syscall = bun_core::String::create_atom(&query.name); - // SystemError has no Default impl upstream; spell out - // the field defaults (empty strings, fd = c_int::MIN). let system_error = SystemError { errno: -1, - code: bun_core::String::static_(err.code()), - message: bun_core::String::static_(err.label()), - path: bun_core::String::empty(), - syscall, - hostname: bun_core::String::empty(), - fd: c_int::MIN, - dest: bun_core::String::empty(), + code: bun_core::String::static_(err.code()).into(), + message: bun_core::String::static_(err.label()).into(), + syscall: syscall.into(), + ..Default::default() }; return Err(global_this.throw_value(system_error.to_error_instance(global_this))); } diff --git a/src/runtime/ffi/ffi_body.rs b/src/runtime/ffi/ffi_body.rs index fc033bf7ad2f..84f3123ed41b 100644 --- a/src/runtime/ffi/ffi_body.rs +++ b/src/runtime/ffi/ffi_body.rs @@ -1514,14 +1514,10 @@ impl FFI { ) .ok(); let system_error = SystemError { - code: bun_core::String::clone_utf8(b"ERR_DLOPEN_FAILED"), - message: bun_core::String::clone_utf8(&msg), - syscall: bun_core::String::clone_utf8(b"dlopen"), - errno: 0, - path: bun_core::String::EMPTY, - hostname: bun_core::String::EMPTY, - fd: -1, - dest: bun_core::String::EMPTY, + code: bun_core::String::clone_utf8(b"ERR_DLOPEN_FAILED").into(), + message: bun_core::String::clone_utf8(&msg).into(), + syscall: bun_core::String::clone_utf8(b"dlopen").into(), + ..Default::default() }; return system_error.to_error_instance(global); } diff --git a/src/runtime/node/node_fs.rs b/src/runtime/node/node_fs.rs index aed867c8fac5..70a42cdd1580 100644 --- a/src/runtime/node/node_fs.rs +++ b/src/runtime/node/node_fs.rs @@ -8071,14 +8071,10 @@ impl NodeFS { ); let _ = global_this.throw_value( bun_jsc::SystemError { - errno: 0, - message: BunString::init(&buf[..]), - code: BunString::init(err.name()), - path: BunString::init(path.as_slice()), - syscall: BunString::default(), - hostname: BunString::default(), - fd: -1, - dest: BunString::default(), + message: BunString::init(&buf[..]).into(), + code: BunString::init(err.name()).into(), + path: BunString::init(path.as_slice()).into(), + ..Default::default() } .to_error_instance(&global_this), ); diff --git a/src/runtime/node/node_os.rs b/src/runtime/node/node_os.rs index a418fb37d92a..01cc6ea7392d 100644 --- a/src/runtime/node/node_os.rs +++ b/src/runtime/node/node_os.rs @@ -78,22 +78,6 @@ mod _impl { } } - /// `bun_jsc::SystemError` has no `Default` (see src/jsc/SystemError.rs). - /// Local zero-value for the extern-struct fields. - #[inline] - fn system_error_default() -> SystemError { - SystemError { - errno: 0, - code: BunString::empty(), - message: BunString::empty(), - path: BunString::empty(), - syscall: BunString::empty(), - hostname: BunString::empty(), - fd: c_int::MIN, - dest: BunString::empty(), - } - } - /// `bun_core::ZigString` (the `bun_string` crate type) is `repr(C)`-identical /// to the JSC-side `ZigString` but lacks `with_encoding`/`to_js`. Provide /// them locally. @@ -288,9 +272,10 @@ mod _impl { Ok(v) => Ok(v), Err(_) => { let err = SystemError { - message: BunString::static_("Failed to get CPU information"), - code: BunString::static_(<&'static str>::from(ErrorCode::ERR_SYSTEM_ERROR)), - ..system_error_default() + message: BunString::static_("Failed to get CPU information").into(), + code: BunString::static_(<&'static str>::from(ErrorCode::ERR_SYSTEM_ERROR)) + .into(), + ..Default::default() }; Err(global.throw_value(err.to_error_instance(global))) } @@ -712,14 +697,14 @@ mod _impl { let result = get_process_priority(pid); if result == i32::MAX { let err = SystemError { - message: BunString::static_("no such process"), - code: BunString::static_("ESRCH"), + message: BunString::static_("no such process").into(), + code: BunString::static_("ESRCH").into(), #[cfg(not(windows))] errno: -(bun_sys::posix::E::ESRCH as c_int), #[cfg(windows)] errno: libuv::UV_ESRCH, - syscall: BunString::static_("uv_os_getpriority"), - ..system_error_default() + syscall: BunString::static_("uv_os_getpriority").into(), + ..Default::default() }; return Err(global.throw_value(err.to_error_instance_with_info_object(global))); } @@ -955,11 +940,12 @@ mod _impl { let err = SystemError { message: BunString::static_( "A system error occurred: getifaddrs returned an error", - ), - code: BunString::static_("ERR_SYSTEM_ERROR"), + ) + .into(), + code: BunString::static_("ERR_SYSTEM_ERROR").into(), errno: errno as c_int, - syscall: BunString::static_("getifaddrs"), - ..system_error_default() + syscall: BunString::static_("getifaddrs").into(), + ..Default::default() }; return Err(global_this.throw_value(err.to_error_instance(global_this))); @@ -1237,12 +1223,12 @@ mod _impl { let err = unsafe { libuv::uv_interface_addresses(&mut ifaces, &mut count) }; if err != 0 { let sys_err = SystemError { - message: BunString::static_("uv_interface_addresses failed"), - code: BunString::static_("ERR_SYSTEM_ERROR"), + message: BunString::static_("uv_interface_addresses failed").into(), + code: BunString::static_("ERR_SYSTEM_ERROR").into(), //.info = info, errno: err, - syscall: BunString::static_("uv_interface_addresses"), - ..system_error_default() + syscall: BunString::static_("uv_interface_addresses").into(), + ..Default::default() }; return Err(global_this.throw_value(sys_err.to_error_instance(global_this))); } @@ -1468,40 +1454,40 @@ mod _impl { match errcode { bun_sys::E::ESRCH => { let err = SystemError { - message: BunString::static_("no such process"), - code: BunString::static_("ESRCH"), + message: BunString::static_("no such process").into(), + code: BunString::static_("ESRCH").into(), #[cfg(not(windows))] errno: -(bun_sys::posix::E::ESRCH as c_int), #[cfg(windows)] errno: libuv::UV_ESRCH, - syscall: BunString::static_("uv_os_getpriority"), - ..system_error_default() + syscall: BunString::static_("uv_os_getpriority").into(), + ..Default::default() }; Err(global.throw_value(err.to_error_instance_with_info_object(global))) } bun_sys::E::EACCES => { let err = SystemError { - message: BunString::static_("permission denied"), - code: BunString::static_("EACCES"), + message: BunString::static_("permission denied").into(), + code: BunString::static_("EACCES").into(), #[cfg(not(windows))] errno: -(bun_sys::posix::E::EACCES as c_int), #[cfg(windows)] errno: libuv::UV_EACCES, - syscall: BunString::static_("uv_os_getpriority"), - ..system_error_default() + syscall: BunString::static_("uv_os_getpriority").into(), + ..Default::default() }; Err(global.throw_value(err.to_error_instance_with_info_object(global))) } bun_sys::E::EPERM => { let err = SystemError { - message: BunString::static_("operation not permitted"), - code: BunString::static_("EPERM"), + message: BunString::static_("operation not permitted").into(), + code: BunString::static_("EPERM").into(), #[cfg(not(windows))] errno: -(bun_sys::posix::E::ESRCH as c_int), #[cfg(windows)] errno: libuv::UV_ESRCH, - syscall: BunString::static_("uv_os_getpriority"), - ..system_error_default() + syscall: BunString::static_("uv_os_getpriority").into(), + ..Default::default() }; Err(global.throw_value(err.to_error_instance_with_info_object(global))) } @@ -1556,11 +1542,11 @@ mod _impl { let err = unsafe { libuv::uv_uptime(&mut uptime_value) }; if err != 0 { let sys_err = SystemError { - message: BunString::static_("failed to get system uptime"), - code: BunString::static_("ERR_SYSTEM_ERROR"), + message: BunString::static_("failed to get system uptime").into(), + code: BunString::static_("ERR_SYSTEM_ERROR").into(), errno: err, - syscall: BunString::static_("uv_uptime"), - ..system_error_default() + syscall: BunString::static_("uv_uptime").into(), + ..Default::default() }; return Err(global.throw_value(sys_err.to_error_instance(global))); } diff --git a/src/runtime/node/types.rs b/src/runtime/node/types.rs index afeb69c6358e..3edab6e4ef9e 100644 --- a/src/runtime/node/types.rs +++ b/src/runtime/node/types.rs @@ -1300,7 +1300,7 @@ impl Valid { let mut system_error = bun_sys::Error::from_code(bun_sys::E::ENAMETOOLONG, bun_sys::Tag::open) .to_system_error(); - system_error.syscall = bun_core::String::DEAD; + system_error.syscall = bun_core::String::DEAD.into(); Err(ctx.throw_value(system_error.to_error_instance(ctx))) } } @@ -1320,7 +1320,7 @@ impl Valid { let mut system_error = bun_sys::Error::from_code(bun_sys::E::ENAMETOOLONG, bun_sys::Tag::open) .to_system_error(); - system_error.syscall = bun_core::String::DEAD; + system_error.syscall = bun_core::String::DEAD.into(); Err(ctx.throw_value(system_error.to_error_instance(ctx))) } } diff --git a/src/runtime/server/RequestContext.rs b/src/runtime/server/RequestContext.rs index d44f8b4afa0f..f5a1d11fb224 100644 --- a/src/runtime/server/RequestContext.rs +++ b/src/runtime/server/RequestContext.rs @@ -1774,7 +1774,8 @@ where } }; let mut sys: jsc::SystemError = err.to_system_error().into(); - sys.message = BunString::static_("Cannot stream a directory as a response body"); + sys.message = + BunString::static_("Cannot stream a directory as a response body").into(); return self.run_error_handler(sys.to_error_instance(global_this)); } (bun_io::FileType::File, false) @@ -3149,10 +3150,12 @@ where let err = jsc::SystemError { code: BunString::static_(<&'static str>::from( jsc::ErrorCode::ERR_STREAM_CANNOT_PIPE, - )), + )) + .into(), message: BunString::static_( "Stream already used, please create a new one", - ), + ) + .into(), ..Default::default() }; stream.value.unprotect(); diff --git a/src/runtime/server/mod.rs b/src/runtime/server/mod.rs index fb5e63159b88..f50f6ce8b23d 100644 --- a/src/runtime/server/mod.rs +++ b/src/runtime/server/mod.rs @@ -1857,9 +1857,10 @@ impl NewServer { "permission denied {}:{}", bstr::BStr::new(host), port - )), - code: bun_core::String::static_("EACCES"), - syscall: bun_core::String::static_("listen"), + )) + .into(), + code: bun_core::String::static_("EACCES").into(), + syscall: bun_core::String::static_("listen").into(), ..Default::default() }; let _ = global.throw_value(err.to_error_instance(global)); @@ -1881,9 +1882,10 @@ impl NewServer { message: bun_core::String::create_format(format_args!( "Failed to start server. Is port {} in use?", port - )), - code: bun_core::String::static_("EADDRINUSE"), - syscall: bun_core::String::static_("listen"), + )) + .into(), + code: bun_core::String::static_("EADDRINUSE").into(), + syscall: bun_core::String::static_("listen").into(), ..Default::default() } .to_error_instance(global) @@ -1895,9 +1897,10 @@ impl NewServer { message: bun_core::String::create_format(format_args!( "Failed to listen on unix socket {}", bun_core::fmt::QuotedFormatter { text: unix } - )), - code: bun_core::String::static_("EADDRINUSE"), - syscall: bun_core::String::static_("listen"), + )) + .into(), + code: bun_core::String::static_("EADDRINUSE").into(), + syscall: bun_core::String::static_("listen").into(), ..Default::default() } .to_error_instance(global), diff --git a/src/runtime/shell/builtin/basename.rs b/src/runtime/shell/builtin/basename.rs index 221c95717cde..aa23562cc482 100644 --- a/src/runtime/shell/builtin/basename.rs +++ b/src/runtime/shell/builtin/basename.rs @@ -51,14 +51,14 @@ impl Basename { Builtin::write_failing_error(interp, cmd, msg, 1) } + #[allow(clippy::needless_pass_by_value)] // signature fixed by `Builtin::on_io_writer_chunk` dispatch pub(crate) fn on_io_writer_chunk( interp: &Interpreter, cmd: NodeId, _: usize, err: Option, ) -> Yield { - if let Some(e) = err { - e.deref(); + if err.is_some() { Self::state_mut(interp, cmd).state = State::Err; return Builtin::done(interp, cmd, 1); } diff --git a/src/runtime/shell/builtin/cat.rs b/src/runtime/shell/builtin/cat.rs index 0d063dd7edfc..874018c7e2cf 100644 --- a/src/runtime/shell/builtin/cat.rs +++ b/src/runtime/shell/builtin/cat.rs @@ -238,7 +238,6 @@ impl Cat { ) -> Yield { if let Some(e) = err { let errno = e.get_errno() as ExitCode; - e.deref(); let rchild = ReaderChildPtr { node: cmd, tag: ReaderTag::Cat, @@ -342,13 +341,7 @@ impl Cat { cmd: NodeId, err: Option, ) -> Yield { - let errno: ExitCode = err - .map(|e| { - let n = e.get_errno() as ExitCode; - e.deref(); - n - }) - .unwrap_or(0); + let errno: ExitCode = err.map(|e| e.get_errno() as ExitCode).unwrap_or(0); let stdout_needs_io = Builtin::of(interp, cmd).stdout.needs_io().is_some(); let mut cancel = false; let step = match &mut Self::state_mut(interp, cmd).state { diff --git a/src/runtime/shell/builtin/dirname.rs b/src/runtime/shell/builtin/dirname.rs index 4edfdb2db591..9edef3495bf4 100644 --- a/src/runtime/shell/builtin/dirname.rs +++ b/src/runtime/shell/builtin/dirname.rs @@ -53,14 +53,14 @@ impl Dirname { Builtin::write_failing_error(interp, cmd, msg, 1) } + #[allow(clippy::needless_pass_by_value)] // signature fixed by `Builtin::on_io_writer_chunk` dispatch pub(crate) fn on_io_writer_chunk( interp: &Interpreter, cmd: NodeId, _: usize, err: Option, ) -> Yield { - if let Some(e) = err { - e.deref(); + if err.is_some() { Self::state_mut(interp, cmd).state = State::Err; return Builtin::done(interp, cmd, 1); } diff --git a/src/runtime/shell/builtin/mv.rs b/src/runtime/shell/builtin/mv.rs index 59f95175f55b..bb70ed49ebbf 100644 --- a/src/runtime/shell/builtin/mv.rs +++ b/src/runtime/shell/builtin/mv.rs @@ -277,6 +277,7 @@ impl Mv { } } + #[allow(clippy::needless_pass_by_value)] // signature fixed by `Builtin::on_io_writer_chunk` dispatch pub(crate) fn on_io_writer_chunk( interp: &Interpreter, cmd: NodeId, @@ -285,8 +286,7 @@ impl Mv { ) -> Yield { match Self::state_mut(interp, cmd).state { MvState::WaitingWriteErr { exit_code } => { - if let Some(err) = e { - err.deref(); + if e.is_some() { Self::state_mut(interp, cmd).state = MvState::Err; return Self::next(interp, cmd); } diff --git a/src/runtime/shell/builtin/rm.rs b/src/runtime/shell/builtin/rm.rs index bd921f65a020..9adb735274e3 100644 --- a/src/runtime/shell/builtin/rm.rs +++ b/src/runtime/shell/builtin/rm.rs @@ -397,9 +397,7 @@ impl Rm { } } }; - if let Some(err) = e { - err.deref(); - } + drop(e); match outcome { Some(code) => Builtin::done(interp, cmd, code), None => Yield::suspended(), diff --git a/src/runtime/shell/builtin/seq.rs b/src/runtime/shell/builtin/seq.rs index 2ddb72521914..01f9889d478e 100644 --- a/src/runtime/shell/builtin/seq.rs +++ b/src/runtime/shell/builtin/seq.rs @@ -204,14 +204,14 @@ impl Seq { Builtin::done(interp, cmd, 0) } + #[allow(clippy::needless_pass_by_value)] // signature fixed by `Builtin::on_io_writer_chunk` dispatch pub(crate) fn on_io_writer_chunk( interp: &Interpreter, cmd: NodeId, _: usize, e: Option, ) -> Yield { - if let Some(e) = e { - e.deref(); + if e.is_some() { Self::state_mut(interp, cmd).state = State::Err; return Builtin::done(interp, cmd, 1); } diff --git a/src/runtime/shell/builtin/yes.rs b/src/runtime/shell/builtin/yes.rs index f8627dc5cc06..026eb220f96a 100644 --- a/src/runtime/shell/builtin/yes.rs +++ b/src/runtime/shell/builtin/yes.rs @@ -157,16 +157,14 @@ impl Yes { Builtin::write_failing_error(interp, cmd, buf, exit_code) } + #[allow(clippy::needless_pass_by_value)] // signature fixed by `Builtin::on_io_writer_chunk` dispatch pub(crate) fn on_io_writer_chunk( interp: &Interpreter, cmd: NodeId, _: usize, e: Option, ) -> Yield { - if let Some(e) = e { - // Release the SystemError's owned BunString fields (no `Drop` - // impl on `bun_sys::SystemError`). - e.deref(); + if e.is_some() { Self::state_mut(interp, cmd).state = State::Err; return Builtin::done(interp, cmd, 1); } diff --git a/src/runtime/shell/shell_body.rs b/src/runtime/shell/shell_body.rs index b9f286f80f72..56a7414fc8ae 100644 --- a/src/runtime/shell/shell_body.rs +++ b/src/runtime/shell/shell_body.rs @@ -62,19 +62,15 @@ impl ShellErr { pub fn throw_js(self, global: &JSGlobalObject) -> bun_jsc::JsError { match self { ShellErr::Sys(sys) => { - // `to_error_instance` decrements every string ref itself, so we - // must hand it the *owned* value (move) — no extra deref here. let err = bun_jsc::SystemError::from(sys).to_error_instance(global); global.throw_value(err) } ShellErr::Custom(custom) => { let err_value = BunString::clone_utf8(&custom).to_error_instance(global); - // `custom: Box<[u8]>` drops here. global.throw_value(err_value) } ShellErr::InvalidArguments { val } => { global.throw_invalid_arguments(format_args!("{}", bstr::BStr::new(&*val))) - // `val` drops here. } ShellErr::Todo(todo) => global.throw_todo(&todo), } @@ -89,7 +85,6 @@ impl ShellErr { err.message, err.path ); - err.deref(); } ShellErr::Custom(custom) => { bun_core::pretty_errorln!( @@ -112,15 +107,6 @@ impl ShellErr { } bun_core::Global::exit(1) } - - /// Spec `ShellErr.deinit`. Explicit release for callers that drop a - /// `ShellErr` without throwing it (the `Box<[u8]>` arms free on ordinary - /// drop, so only `.sys` needs work). - pub fn deinit(self) { - if let ShellErr::Sys(sys) = self { - sys.deref(); - } - } } impl fmt::Display for ShellErr { @@ -136,13 +122,6 @@ impl fmt::Display for ShellErr { } } -// Note: no `impl Drop for ShellErr`. Release is *manual* and asymmetric — `throwJS` deliberately skips `.sys.deref()` because -// `toErrorInstance` already consumed those refs. An unconditional `Drop` would -// re-introduce the double-deref. Ownership is instead expressed by `throw_js` / -// `throw_mini` / `deinit` taking `self` by value; the `Box<[u8]>` payloads free -// on ordinary drop, and `.sys` is released exactly once on whichever consume -// path runs. - // ───────────────────────────── Test ───────────────────────────── pub mod test { diff --git a/src/runtime/shell/states/Cmd.rs b/src/runtime/shell/states/Cmd.rs index 2d9f5fc1ac6b..22f84c077293 100644 --- a/src/runtime/shell/states/Cmd.rs +++ b/src/runtime/shell/states/Cmd.rs @@ -332,9 +332,7 @@ impl Cmd { }; interp.deinit_node(child); if let Some(err) = err { - let y = Builtin::cmd_write_failing_error(interp, this, format_args!("{}\n", err)); - err.deinit(); - return y; + return Builtin::cmd_write_failing_error(interp, this, format_args!("{}\n", err)); } let me = interp.as_cmd_mut(this); me.exit_code = Some(1); diff --git a/src/runtime/shell/states/CondExpr.rs b/src/runtime/shell/states/CondExpr.rs index 99bc365380cd..b808566e0753 100644 --- a/src/runtime/shell/states/CondExpr.rs +++ b/src/runtime/shell/states/CondExpr.rs @@ -238,9 +238,7 @@ impl CondExpr { let err = Expansion::take_err(interp, child); interp.deinit_node(child); if let Some(err) = err { - let y = Self::write_failing_error(interp, this, format_args!("{}\n", err)); - err.deinit(); - return y; + return Self::write_failing_error(interp, this, format_args!("{}\n", err)); } // Defensive fallback — finish via `writeFailingError` with exit 1. debug_assert!(false, "Expansion child failed without an error"); diff --git a/src/runtime/shell/subproc.rs b/src/runtime/shell/subproc.rs index 499bc889b055..9562a7edf0e1 100644 --- a/src/runtime/shell/subproc.rs +++ b/src/runtime/shell/subproc.rs @@ -1587,7 +1587,7 @@ impl CapturedWriter { self.written += amount; if let Some(e) = err { log!( - "CapturedWriter(0x{:x}, {}) onWrite errno={} errmsg={} errfd={} syscall={}", + "CapturedWriter(0x{:x}, {}) onWrite errno={} errmsg={} errfd={:?} syscall={}", std::ptr::from_mut(self) as usize, out_kind_str(self.parent().out_type), e.errno, @@ -1610,14 +1610,6 @@ impl CapturedWriter { } } -impl Drop for CapturedWriter { - fn drop(&mut self) { - // `bun_sys::SystemError` strings drop themselves. - let _ = self.err.take(); - // self.writer Arc drops automatically. - } -} - impl PipeReader { pub fn detach(self: Arc) { log!( diff --git a/src/runtime/socket/Listener.rs b/src/runtime/socket/Listener.rs index b0e6c3e160c2..e63fc1aebf5e 100644 --- a/src/runtime/socket/Listener.rs +++ b/src/runtime/socket/Listener.rs @@ -274,7 +274,7 @@ impl Listener { let err = jsc::SystemError { // Negated errno per fill_system_error_common. errno: -(se as c_int), - code: bun_core::String::static_(name), + code: bun_core::String::static_(name).into(), message: bun_core::String::clone_utf8( format!( "listen {}: {}", @@ -282,12 +282,12 @@ impl Listener { bstr::BStr::new(&pipe_buf[..pipe_len]) ) .as_bytes(), - ), - syscall: bun_core::String::static_("listen"), - fd: -1, - path: bun_core::String::clone_utf8(&pipe_buf[..pipe_len]), - hostname: bun_core::String::empty(), - dest: bun_core::String::empty(), + ) + .into(), + syscall: bun_core::String::static_("listen").into(), + path: bun_core::String::clone_utf8(&pipe_buf[..pipe_len]) + .into(), + ..Default::default() }; return Err(global.throw_value(err.to_error_instance(global))); } @@ -462,15 +462,14 @@ impl Listener { UnixOrHost::Fd(fd) => { let err = jsc::SystemError { errno: bun_sys::SystemErrno::EINVAL as c_int, - code: bun_core::String::static_("EINVAL"), + code: bun_core::String::static_("EINVAL").into(), message: bun_core::String::static_( "Bun does not support listening on a file descriptor.", - ), - syscall: bun_core::String::static_("listen"), + ) + .into(), + syscall: bun_core::String::static_("listen").into(), fd: fd.uv(), - path: bun_core::String::empty(), - hostname: bun_core::String::empty(), - dest: bun_core::String::empty(), + ..Default::default() }; return Err(global.throw_value(err.to_error_instance(global))); } diff --git a/src/runtime/socket/socket_body.rs b/src/runtime/socket/socket_body.rs index 2ba157a48cd2..f0e234f2461a 100644 --- a/src/runtime/socket/socket_body.rs +++ b/src/runtime/socket/socket_body.rs @@ -360,14 +360,6 @@ impl PendingSystemError { } } -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. @@ -1177,13 +1169,10 @@ impl NewSocket { }; SystemError { errno: -errno_, - message: BunString::static_("Failed to connect"), - syscall: BunString::static_("connect"), - code: code_, - path: BunString::EMPTY, - hostname: BunString::EMPTY, - fd: c_int::MIN, - dest: BunString::EMPTY, + message: BunString::static_("Failed to connect").into(), + syscall: BunString::static_("connect").into(), + code: code_.into(), + ..Default::default() } }; @@ -1204,10 +1193,6 @@ impl NewSocket { 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(()); @@ -1219,7 +1204,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 mut err_for_promise = PendingSystemError(Some(err.clone())); let err_value = err.to_error_instance(&global); let result = match callback.call(&global, this_value, &[this_value, err_value]) { Ok(v) => v, @@ -2213,14 +2198,9 @@ impl NewSocket { fn stored_verify_error_to_js(&self, global: &JSGlobalObject) -> Option { self.verify_error.get().as_ref().map(|stored| { let err = SystemError { - errno: 0, - code: BunString::clone_utf8(&stored.code), - message: BunString::clone_utf8(&stored.reason), - path: BunString::EMPTY, - syscall: BunString::EMPTY, - hostname: BunString::EMPTY, - fd: c_int::MIN, - dest: BunString::EMPTY, + code: BunString::clone_utf8(&stored.code).into(), + message: BunString::clone_utf8(&stored.reason).into(), + ..Default::default() }; err.to_error_instance(global) }) @@ -2254,14 +2234,9 @@ impl NewSocket { let reason: &[u8] = ssl_error.reason_bytes(); let fallback = SystemError { - errno: 0, - code: BunString::clone_utf8(code), - message: BunString::clone_utf8(reason), - path: BunString::EMPTY, - syscall: BunString::EMPTY, - hostname: BunString::EMPTY, - fd: c_int::MIN, - dest: BunString::EMPTY, + code: BunString::clone_utf8(code).into(), + message: BunString::clone_utf8(reason).into(), + ..Default::default() }; Ok(fallback.to_error_instance(global)) diff --git a/src/runtime/socket/udp_socket.rs b/src/runtime/socket/udp_socket.rs index 7ece3bee1568..ecf05f956b0a 100644 --- a/src/runtime/socket/udp_socket.rs +++ b/src/runtime/socket/udp_socket.rs @@ -707,13 +707,10 @@ impl UDPSocket { }; let sys_err = SystemError { errno: err, - code: BunString::static_(code), - message, - path: BunString::empty(), - syscall: BunString::static_(syscall), - hostname: BunString::empty(), - fd: c_int::MIN, - dest: BunString::empty(), + code: BunString::static_(code).into(), + message: message.into(), + syscall: BunString::static_(syscall).into(), + ..Default::default() }; let error_value = sys_err.to_error_instance(global_this); if !is_fd { diff --git a/src/runtime/webcore/Blob.rs b/src/runtime/webcore/Blob.rs index e2176cd642f7..9ae8090caae0 100644 --- a/src/runtime/webcore/Blob.rs +++ b/src/runtime/webcore/Blob.rs @@ -583,12 +583,13 @@ impl BlobExt for Blob { // `t`, so build the SystemError (cloning the path // out of `t.blob.store`) before the call. let err = bun_jsc::SystemError { - code: BunString::clone_utf8(e.code), - message: BunString::clone_utf8(e.message), + code: BunString::clone_utf8(e.code).into(), + message: BunString::clone_utf8(e.message).into(), path: BunString::clone_utf8( t.blob.store().and_then(|s| s.get_path()).unwrap_or(b""), - ), - syscall: BunString::static_("fetch"), + ) + .into(), + syscall: BunString::static_("fetch").into(), ..Default::default() }; t.done(ReadBytesResult::Err(Box::new(err))); diff --git a/src/runtime/webcore/Body.rs b/src/runtime/webcore/Body.rs index 5499f293894f..589007de3128 100644 --- a/src/runtime/webcore/Body.rs +++ b/src/runtime/webcore/Body.rs @@ -627,7 +627,7 @@ impl ValueError { match self { // `.clone()` on BunString/SystemError already bumps the refcount (paired // with their Drop deref); an extra `.ref_()` here would leak +1 per dupe. - ValueError::SystemError(e) => ValueError::SystemError(e.dupe()), + ValueError::SystemError(e) => ValueError::SystemError(e.clone()), ValueError::Message(m) => ValueError::Message(m.clone()), ValueError::TypeError(m) => ValueError::TypeError(m.clone()), ValueError::JSValue(js_ref) => { diff --git a/src/runtime/webcore/ResumableSink.rs b/src/runtime/webcore/ResumableSink.rs index bb97a936b0a7..7235f7607d20 100644 --- a/src/runtime/webcore/ResumableSink.rs +++ b/src/runtime/webcore/ResumableSink.rs @@ -166,17 +166,11 @@ impl ResumableSink::from(ErrorCode::ERR_STREAM_CANNOT_PIPE)), - message: BunString::static_("Stream already used, please create a new one"), - path: BunString::EMPTY, - syscall: BunString::EMPTY, - hostname: BunString::EMPTY, - fd: core::ffi::c_int::MIN, - dest: BunString::EMPTY, + code: BunString::static_(<&'static str>::from(ErrorCode::ERR_STREAM_CANNOT_PIPE)) + .into(), + message: BunString::static_("Stream already used, please create a new one").into(), + ..Default::default() }; let err_instance = err.to_error_instance(global_this); err_instance.ensure_still_alive(); diff --git a/src/runtime/webcore/blob/copy_file.rs b/src/runtime/webcore/blob/copy_file.rs index cf257d421240..de727e1b4b4b 100644 --- a/src/runtime/webcore/blob/copy_file.rs +++ b/src/runtime/webcore/blob/copy_file.rs @@ -23,21 +23,6 @@ use core::ffi::c_int; use core::ffi::c_void; use core::marker::ConstParamTy; -// Local conversion: `bun_sys::SystemError` -> `bun_jsc::SystemError`. Mapped -// field-by-field because the two definitions order their fields differently. -fn to_jsc_system_error(e: &SystemError) -> jsc::SystemError { - jsc::SystemError { - errno: e.errno, - code: e.code, - message: e.message, - path: e.path, - syscall: e.syscall, - hostname: e.hostname, - fd: e.fd, - dest: e.dest, - } -} - // ─────────────────────────────────────────────────────────────────────────── // CopyFile (POSIX, blocking off-thread) // ─────────────────────────────────────────────────────────────────────────── @@ -132,14 +117,14 @@ impl<'a> CopyFile<'a> { ) && system_error.path.is_empty() { system_error.path = - bun_core::String::clone_utf8(self.source_file_store.pathlike.path().slice()); + bun_core::String::clone_utf8(self.source_file_store.pathlike.path().slice()).into(); } if system_error.message.is_empty() { - system_error.message = bun_core::String::static_("Failed to copy file"); + system_error.message = bun_core::String::static_("Failed to copy file").into(); } - let instance = to_jsc_system_error(&system_error) + let instance = jsc::SystemError::from(system_error) .to_error_instance_with_async_stack(self.global_this, promise); if let Some(store) = self.store.take() { drop(store); // deref() @@ -1867,8 +1852,8 @@ pub enum IOWhich { fn unsupported_directory_error() -> SystemError { SystemError { errno: bun_sys::SystemErrno::EISDIR as i32, - message: bun_core::String::static_("That doesn't work on folders"), - syscall: bun_core::String::static_("fstat"), + message: bun_core::String::static_("That doesn't work on folders").into(), + syscall: bun_core::String::static_("fstat").into(), ..SystemError::default() } } @@ -1877,8 +1862,8 @@ fn unsupported_directory_error() -> SystemError { fn unsupported_non_regular_file_error() -> SystemError { SystemError { errno: bun_sys::SystemErrno::ENOTSUP as i32, - message: bun_core::String::static_("Non-regular files aren't supported yet"), - syscall: bun_core::String::static_("fstat"), + message: bun_core::String::static_("Non-regular files aren't supported yet").into(), + syscall: bun_core::String::static_("fstat").into(), ..SystemError::default() } } diff --git a/src/runtime/webcore/blob/read_file.rs b/src/runtime/webcore/blob/read_file.rs index ee90706f5f17..391b4892803c 100644 --- a/src/runtime/webcore/blob/read_file.rs +++ b/src/runtime/webcore/blob/read_file.rs @@ -549,8 +549,9 @@ impl ReadFile { BunString::clone_utf8( self.file_store.pathlike.path().slice(), ) + .into() } else { - BunString::EMPTY + BunString::EMPTY.into() }; } return false; @@ -582,9 +583,10 @@ impl ReadFile { cb( cb_ctx, ReadFileResultType::Err(SystemError { - code: BunString::static_("INTERNAL_ERROR"), - message: BunString::static_("assertion failure - store should not be null"), - syscall: BunString::static_("read"), + code: BunString::static_("INTERNAL_ERROR").into(), + message: BunString::static_("assertion failure - store should not be null") + .into(), + syscall: BunString::static_("read").into(), ..Default::default() }), ); @@ -686,14 +688,14 @@ impl ReadFile { if bun_sys::S::ISDIR(stat.st_mode as _) { self.errno = Some(crate::Error::Sys(bun_errno::SystemErrno::EISDIR)); self.system_error = Some(SystemError { - code: BunString::static_("EISDIR"), + code: BunString::static_("EISDIR").into(), path: if self.file_store.pathlike.is_path() { - BunString::clone_utf8(self.file_store.pathlike.path().slice()) + BunString::clone_utf8(self.file_store.pathlike.path().slice()).into() } else { - BunString::EMPTY + BunString::EMPTY.into() }, - message: BunString::static_("Directories cannot be read like files"), - syscall: BunString::static_("read"), + message: BunString::static_("Directories cannot be read like files").into(), + syscall: BunString::static_("read").into(), ..Default::default() }); return; @@ -1214,14 +1216,15 @@ impl<'a> ReadFileUV<'a> { if bun_sys::S::ISDIR(u32::try_from(stat.mode()).expect("int cast")) { this.errno = Some(crate::Error::Sys(bun_errno::SystemErrno::EISDIR)); this.system_error = Some(SystemError { - code: BunString::static_("EISDIR"), + code: BunString::static_("EISDIR").into(), path: if this.file_store.pathlike.is_path() { BunString::clone_utf8(this.file_store.pathlike.path().slice()) } else { BunString::EMPTY - }, - message: BunString::static_("Directories cannot be read like files"), - syscall: BunString::static_("read"), + } + .into(), + message: BunString::static_("Directories cannot be read like files").into(), + syscall: BunString::static_("read").into(), ..Default::default() }); this.on_finish(); diff --git a/src/runtime/webcore/fetch/FetchTasklet.rs b/src/runtime/webcore/fetch/FetchTasklet.rs index 54f81b55103c..9bff5a530a83 100644 --- a/src/runtime/webcore/fetch/FetchTasklet.rs +++ b/src/runtime/webcore/fetch/FetchTasklet.rs @@ -1321,7 +1321,7 @@ impl FetchTasklet { b"getaddrinfo", hostname, ); - err.path = path; + err.path = path.into(); return BodyValueError::SystemError(err); } } @@ -1555,17 +1555,11 @@ impl FetchTasklet { )), }; - // `jsc::SystemError` has no `Default` impl upstream — spell out - // every field's default. let fetch_error = jsc::SystemError { - errno: 0, - code, - message, - path, - syscall: BunString::EMPTY, - hostname: BunString::EMPTY, - fd: core::ffi::c_int::MIN, - dest: BunString::EMPTY, + code: code.into(), + message: message.into(), + path: path.into(), + ..Default::default() }; BodyValueError::SystemError(fetch_error) diff --git a/src/sys/Error.rs b/src/sys/Error.rs index 1a61d9c797f3..7329b84f9776 100644 --- a/src/sys/Error.rs +++ b/src/sys/Error.rs @@ -360,34 +360,33 @@ impl Error { let mut err = SystemError { errno: js_errno, - syscall: BunString::static_(<&'static str>::from(self.syscall).as_bytes()), - message: BunString::empty(), + syscall: BunString::static_(<&'static str>::from(self.syscall).as_bytes()).into(), ..Default::default() }; // both maps are total (`initFull("unknown error")`). let looked_up = self.get_error_code_tag_name().map(|(code, system_errno)| { - err.code = BunString::static_(code.as_bytes()); + err.code = BunString::static_(code.as_bytes()).into(); (code, map[system_errno]) }); if !self.path.is_empty() { - err.path = BunString::clone_utf8(&self.path); + err.path = BunString::clone_utf8(&self.path).into(); } if !self.dest.is_empty() { - err.dest = BunString::clone_utf8(&self.dest); + err.dest = BunString::clone_utf8(&self.dest).into(); } if let Some(valid) = fd_unwrap_valid(self.fd) { // When the FD is a windows handle, there is no sane way to report this. #[cfg(windows)] if valid.kind() == crate::FdKind::Uv { - err.fd = valid.uv(); + err.fd = Some(valid.uv()); } #[cfg(not(windows))] { - err.fd = valid.uv(); + err.fd = Some(valid.uv()); } } @@ -399,7 +398,7 @@ impl Error { let (mut err, looked_up) = self.fill_system_error_common(&coreutils_error_map::COREUTILS_ERROR_MAP); if let Some((_, label)) = looked_up { - err.message = BunString::static_(label.as_bytes()); + err.message = BunString::static_(label.as_bytes()).into(); } err } @@ -463,7 +462,7 @@ impl Error { } usize::try_from(cursor.position()).expect("int cast") }; - err.message = BunString::clone_utf8(&message_buf[..pos]); + err.message = BunString::clone_utf8(&message_buf[..pos]).into(); err } @@ -490,8 +489,8 @@ impl fmt::Display for Error { let mut that = self.without_path().to_shell_system_error(); debug_assert!(that.path.tag() != bun_core::Tag::WTFStringImpl); debug_assert!(that.dest.tag() != bun_core::Tag::WTFStringImpl); - that.path = BunString::borrow_utf8(&self.path); - that.dest = BunString::borrow_utf8(&self.dest); + that.path = BunString::borrow_utf8(&self.path).into(); + that.dest = BunString::borrow_utf8(&self.dest).into(); debug_assert!(that.path.tag() != bun_core::Tag::WTFStringImpl); debug_assert!(that.dest.tag() != bun_core::Tag::WTFStringImpl); diff --git a/src/sys/lib.rs b/src/sys/lib.rs index 8908870e655e..f89f6fa4e3fc 100644 --- a/src/sys/lib.rs +++ b/src/sys/lib.rs @@ -31,36 +31,21 @@ impl From for bun_errno::SystemErrno { /// The JS-facing rich error /// (path/dest/syscall as `bun.String`). The data side has no JSC dependency: /// the `*JSGlobalObject`-taking conversion methods (`toErrorInstance` etc.) -/// live in `bun_jsc` as inherent extensions. `#[repr(C)]` and field order -/// are fixed so the C++ `SystemError__*` externs -/// (BunObject.cpp) read the same layout. -#[repr(C)] +/// live in `bun_jsc` as inherent extensions. The `#[repr(C)]` layout C++ reads +/// is `bun_jsc::SystemError`; this struct is the Rust-side data shape and is +/// marshalled field-by-field via `From` at the seam. +#[derive(Default)] pub struct SystemError { pub errno: core::ffi::c_int, /// label for errno - pub code: bun_core::String, + pub code: bun_core::OwnedString, /// it is illegal to have an empty message - pub message: bun_core::String, - pub path: bun_core::String, - pub syscall: bun_core::String, - pub hostname: bun_core::String, - /// MinInt = no file descriptor - pub fd: core::ffi::c_int, - pub dest: bun_core::String, -} -impl Default for SystemError { - fn default() -> Self { - Self { - errno: 0, - code: bun_core::String::empty(), - message: bun_core::String::empty(), - path: bun_core::String::empty(), - syscall: bun_core::String::empty(), - hostname: bun_core::String::empty(), - fd: core::ffi::c_int::MIN, - dest: bun_core::String::empty(), - } - } + pub message: bun_core::OwnedString, + pub path: bun_core::OwnedString, + pub syscall: bun_core::OwnedString, + pub hostname: bun_core::OwnedString, + pub fd: Option, + pub dest: bun_core::OwnedString, } impl SystemError { /// (`Error::to_system_error` stores `errno` negated to match Node.) @@ -77,22 +62,6 @@ impl SystemError { } e_from_negated(self.errno) } - pub fn deref(&self) { - self.path.deref(); - self.code.deref(); - self.message.deref(); - self.syscall.deref(); - self.hostname.deref(); - self.dest.deref(); - } - pub fn ref_(&self) { - self.path.ref_(); - self.code.ref_(); - self.message.ref_(); - self.syscall.ref_(); - self.hostname.ref_(); - self.dest.ref_(); - } } impl core::fmt::Display for SystemError { /// Emits the colorless variant diff --git a/src/sys_jsc/fd_jsc.rs b/src/sys_jsc/fd_jsc.rs index 9ce1666643d1..7ee57d549ce0 100644 --- a/src/sys_jsc/fd_jsc.rs +++ b/src/sys_jsc/fd_jsc.rs @@ -84,8 +84,8 @@ impl FdJsc for Fd { Err(_) => { self.close(); let err_instance = (bun_jsc::SystemError { - message: bun_core::String::static_(b"EMFILE, too many open files"), - code: bun_core::String::static_(b"EMFILE"), + message: bun_core::String::static_(b"EMFILE, too many open files").into(), + code: bun_core::String::static_(b"EMFILE").into(), ..Default::default() }) .to_error_instance(global); diff --git a/src/sys_jsc/lib.rs b/src/sys_jsc/lib.rs index fcdd9685f849..12c7fa61ae19 100644 --- a/src/sys_jsc/lib.rs +++ b/src/sys_jsc/lib.rs @@ -24,57 +24,34 @@ pub use bun_jsc::{ // ────────────────────────────────────────────────────────────────────────── // SystemErrorJsc — JSC bridge for the T1 `bun_sys::SystemError` data struct. // -// The *data* struct (`bun_sys::SystemError`, NOT `#[repr(C)]`) is split from -// the FFI struct (`bun_jsc::SystemError`, `#[repr(C)]` field-order = C++). -// This trait marshals the former into the latter and forwards to +// The *data* struct (`bun_sys::SystemError`) is split from the FFI struct +// (`bun_jsc::SystemError`, `#[repr(C)]` field-order = C++). This trait +// forwards through `From` to // `bun_jsc::SystemError::to_error_instance{,_with_async_stack}`. -// -// Ref-count contract: `bun_jsc::SystemError::to_error_instance` derefs each -// field exactly once, so the marshalled struct must hold exactly the refs -// `self` held — i.e. a bitwise field copy with NO extra `ref_()`. The -// caller's `bun_sys::SystemError` is consumed (its strings reach -// refcount-0). // ────────────────────────────────────────────────────────────────────────── pub trait SystemErrorJsc { - fn to_error_instance(&self, global: &JSGlobalObject) -> JSValue; + fn to_error_instance(self, global: &JSGlobalObject) -> JSValue; fn to_error_instance_with_async_stack( - &self, + self, global: &JSGlobalObject, promise: &JSPromise, ) -> JSValue; } -#[inline] -fn marshal(e: &bun_sys::SystemError) -> bun_jsc::SystemError { - // `bun_core::String` is `Copy` (intrusive WTF refcount handle); bitwise - // copy *transfers* the existing ref to the FFI-layout struct. No `ref_()` - // here — `to_error_instance()` will `deref()` each field exactly once. - bun_jsc::SystemError { - errno: e.errno as core::ffi::c_int, - code: e.code, - message: e.message, - path: e.path, - syscall: e.syscall, - hostname: e.hostname, - fd: e.fd as core::ffi::c_int, - dest: e.dest, - } -} - impl SystemErrorJsc for bun_sys::SystemError { /// `SystemError.toErrorInstance(global)`. - fn to_error_instance(&self, global: &JSGlobalObject) -> JSValue { - marshal(self).to_error_instance(global) + fn to_error_instance(self, global: &JSGlobalObject) -> JSValue { + bun_jsc::SystemError::from(self).to_error_instance(global) } /// `SystemError.toErrorInstanceWithAsyncStack(global, promise)` — /// `toErrorInstance` then attach the promise's await /// chain as async stack frames so threadpool-rejected promises get a /// useful trace. fn to_error_instance_with_async_stack( - &self, + self, global: &JSGlobalObject, promise: &JSPromise, ) -> JSValue { - marshal(self).to_error_instance_with_async_stack(global, promise) + bun_jsc::SystemError::from(self).to_error_instance_with_async_stack(global, promise) } } diff --git a/test/js/bun/shell/leak.test.ts b/test/js/bun/shell/leak.test.ts index 14991a5bba53..5e46259cab16 100644 --- a/test/js/bun/shell/leak.test.ts +++ b/test/js/bun/shell/leak.test.ts @@ -1,7 +1,7 @@ import { $ } from "bun"; import { heapStats } from "bun:jsc"; import { describe, expect, test } from "bun:test"; -import { bunEnv, isASAN, isPosix, tempDir, tempDirWithFiles } from "harness"; +import { bunEnv, isASAN, isPosix, isWindows, tempDir, tempDirWithFiles } from "harness"; import { join } from "path"; import { bunExe } from "./test_builder"; import { createTestBuilder } from "./util"; @@ -442,6 +442,71 @@ describe.concurrent("fd leak", () => { doit(true); }); + // Shell builtins and Cmd build a `SystemError` on redirect/write failure + // whose `path`/`dest`/`message` fields are heap `WTF::StringImpl`s. Run the + // failing paths under LSan with `Malloc=1` (so WTF allocations go through + // the system heap) and assert no allocation rooted at + // `to_shell_system_error`/`to_system_error` is reported as leaked. + // + // Windows has no ASAN lane and `Malloc=1` is unimplemented there. + test.skipIf(!isASAN || isWindows)( + "SystemError strings are released on shell error paths", + async () => { + const script = /* ts */ ` + import { $ } from "bun"; + $.nothrow(); + for (let i = 0; i < 5; i++) { + // builtin stdout-redirect open failure (Builtin::init_redirections) + await $\`echo hi > /nonexistent-dir-xyz/out.txt\`.quiet(); + // builtin stdin-redirect open failure + await $\`echo hi < /nonexistent-file-xyz.txt\`.quiet(); + // IOWriter write failure -> on_io_writer_chunk(Some(err)) + if (process.platform === "linux") { + await $\`echo hi > /dev/full\`.quiet(); + await $\`pwd > /dev/full\`.quiet(); + await $\`which ls > /dev/full\`.quiet(); + } + } + console.error("ran"); + `; + + await using proc = Bun.spawn({ + cmd: [bunExe(), "-e", script], + env: { + ...bunEnv, + // Route bmalloc through the system allocator so LSan can observe + // WTF::StringImpl allocations. + Malloc: "1", + ASAN_OPTIONS: "detect_leaks=1:allow_user_segv_handler=1:disable_coredump=0", + // LSan always reports some process-lifetime allocations (VM + // identifiers, sourcemap buffers). exitcode=0 keeps those from + // failing the process; the assertion below inspects the report + // for frames specific to this leak. + LSAN_OPTIONS: "exitcode=0", + }, + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + + // The script itself must have run to completion. + expect(stderr).toContain("ran"); + + // What must be absent is any leak whose allocation stack runs through the + // SystemError constructors: those are exactly the WTF::StringImpl refs + // this change releases via Drop. + const leakedSystemError = stderr + .split("\n") + .filter(l => l.includes("to_shell_system_error") || l.includes("to_system_error")); + if (leakedSystemError.length) { + console.error("LSan reported SystemError leak frames:\n" + leakedSystemError.join("\n")); + } + expect(leakedSystemError).toEqual([]); + expect({ stdout, exitCode }).toEqual({ stdout: "", exitCode: 0 }); + }, + 30_000, + ); + describe.serial("not leaking ParsedShellScript when ShellInterpreter never runs", () => { function doit(builtin: boolean) { test(builtin ? "builtin" : "external", async () => { From d12037166b9baf0c117f0a4fdb6f905ed6a7930c Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Fri, 24 Jul 2026 02:06:01 +0000 Subject: [PATCH 2/3] shell: drop clippy allow attrs, use if let Some(_err) to consume --- src/runtime/shell/builtin/basename.rs | 3 +-- src/runtime/shell/builtin/dirname.rs | 3 +-- src/runtime/shell/builtin/mv.rs | 3 +-- src/runtime/shell/builtin/seq.rs | 3 +-- src/runtime/shell/builtin/yes.rs | 3 +-- 5 files changed, 5 insertions(+), 10 deletions(-) diff --git a/src/runtime/shell/builtin/basename.rs b/src/runtime/shell/builtin/basename.rs index aa23562cc482..43bc4dcb7423 100644 --- a/src/runtime/shell/builtin/basename.rs +++ b/src/runtime/shell/builtin/basename.rs @@ -51,14 +51,13 @@ impl Basename { Builtin::write_failing_error(interp, cmd, msg, 1) } - #[allow(clippy::needless_pass_by_value)] // signature fixed by `Builtin::on_io_writer_chunk` dispatch pub(crate) fn on_io_writer_chunk( interp: &Interpreter, cmd: NodeId, _: usize, err: Option, ) -> Yield { - if err.is_some() { + if let Some(_err) = err { Self::state_mut(interp, cmd).state = State::Err; return Builtin::done(interp, cmd, 1); } diff --git a/src/runtime/shell/builtin/dirname.rs b/src/runtime/shell/builtin/dirname.rs index 9edef3495bf4..0b7dd1fe5b57 100644 --- a/src/runtime/shell/builtin/dirname.rs +++ b/src/runtime/shell/builtin/dirname.rs @@ -53,14 +53,13 @@ impl Dirname { Builtin::write_failing_error(interp, cmd, msg, 1) } - #[allow(clippy::needless_pass_by_value)] // signature fixed by `Builtin::on_io_writer_chunk` dispatch pub(crate) fn on_io_writer_chunk( interp: &Interpreter, cmd: NodeId, _: usize, err: Option, ) -> Yield { - if err.is_some() { + if let Some(_err) = err { Self::state_mut(interp, cmd).state = State::Err; return Builtin::done(interp, cmd, 1); } diff --git a/src/runtime/shell/builtin/mv.rs b/src/runtime/shell/builtin/mv.rs index bb70ed49ebbf..ac25da92e851 100644 --- a/src/runtime/shell/builtin/mv.rs +++ b/src/runtime/shell/builtin/mv.rs @@ -277,7 +277,6 @@ impl Mv { } } - #[allow(clippy::needless_pass_by_value)] // signature fixed by `Builtin::on_io_writer_chunk` dispatch pub(crate) fn on_io_writer_chunk( interp: &Interpreter, cmd: NodeId, @@ -286,7 +285,7 @@ impl Mv { ) -> Yield { match Self::state_mut(interp, cmd).state { MvState::WaitingWriteErr { exit_code } => { - if e.is_some() { + if let Some(_err) = e { Self::state_mut(interp, cmd).state = MvState::Err; return Self::next(interp, cmd); } diff --git a/src/runtime/shell/builtin/seq.rs b/src/runtime/shell/builtin/seq.rs index 01f9889d478e..2c6b87d076d5 100644 --- a/src/runtime/shell/builtin/seq.rs +++ b/src/runtime/shell/builtin/seq.rs @@ -204,14 +204,13 @@ impl Seq { Builtin::done(interp, cmd, 0) } - #[allow(clippy::needless_pass_by_value)] // signature fixed by `Builtin::on_io_writer_chunk` dispatch pub(crate) fn on_io_writer_chunk( interp: &Interpreter, cmd: NodeId, _: usize, e: Option, ) -> Yield { - if e.is_some() { + if let Some(_err) = e { Self::state_mut(interp, cmd).state = State::Err; return Builtin::done(interp, cmd, 1); } diff --git a/src/runtime/shell/builtin/yes.rs b/src/runtime/shell/builtin/yes.rs index 026eb220f96a..4a7a97cb7ebc 100644 --- a/src/runtime/shell/builtin/yes.rs +++ b/src/runtime/shell/builtin/yes.rs @@ -157,14 +157,13 @@ impl Yes { Builtin::write_failing_error(interp, cmd, buf, exit_code) } - #[allow(clippy::needless_pass_by_value)] // signature fixed by `Builtin::on_io_writer_chunk` dispatch pub(crate) fn on_io_writer_chunk( interp: &Interpreter, cmd: NodeId, _: usize, e: Option, ) -> Yield { - if e.is_some() { + if let Some(_err) = e { Self::state_mut(interp, cmd).state = State::Err; return Builtin::done(interp, cmd, 1); } From 92e742c004313eeef708664abc6497992e0245d7 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Fri, 24 Jul 2026 02:09:53 +0000 Subject: [PATCH 3/3] drop LSan leak test --- test/js/bun/shell/leak.test.ts | 67 +--------------------------------- 1 file changed, 1 insertion(+), 66 deletions(-) diff --git a/test/js/bun/shell/leak.test.ts b/test/js/bun/shell/leak.test.ts index 5e46259cab16..14991a5bba53 100644 --- a/test/js/bun/shell/leak.test.ts +++ b/test/js/bun/shell/leak.test.ts @@ -1,7 +1,7 @@ import { $ } from "bun"; import { heapStats } from "bun:jsc"; import { describe, expect, test } from "bun:test"; -import { bunEnv, isASAN, isPosix, isWindows, tempDir, tempDirWithFiles } from "harness"; +import { bunEnv, isASAN, isPosix, tempDir, tempDirWithFiles } from "harness"; import { join } from "path"; import { bunExe } from "./test_builder"; import { createTestBuilder } from "./util"; @@ -442,71 +442,6 @@ describe.concurrent("fd leak", () => { doit(true); }); - // Shell builtins and Cmd build a `SystemError` on redirect/write failure - // whose `path`/`dest`/`message` fields are heap `WTF::StringImpl`s. Run the - // failing paths under LSan with `Malloc=1` (so WTF allocations go through - // the system heap) and assert no allocation rooted at - // `to_shell_system_error`/`to_system_error` is reported as leaked. - // - // Windows has no ASAN lane and `Malloc=1` is unimplemented there. - test.skipIf(!isASAN || isWindows)( - "SystemError strings are released on shell error paths", - async () => { - const script = /* ts */ ` - import { $ } from "bun"; - $.nothrow(); - for (let i = 0; i < 5; i++) { - // builtin stdout-redirect open failure (Builtin::init_redirections) - await $\`echo hi > /nonexistent-dir-xyz/out.txt\`.quiet(); - // builtin stdin-redirect open failure - await $\`echo hi < /nonexistent-file-xyz.txt\`.quiet(); - // IOWriter write failure -> on_io_writer_chunk(Some(err)) - if (process.platform === "linux") { - await $\`echo hi > /dev/full\`.quiet(); - await $\`pwd > /dev/full\`.quiet(); - await $\`which ls > /dev/full\`.quiet(); - } - } - console.error("ran"); - `; - - await using proc = Bun.spawn({ - cmd: [bunExe(), "-e", script], - env: { - ...bunEnv, - // Route bmalloc through the system allocator so LSan can observe - // WTF::StringImpl allocations. - Malloc: "1", - ASAN_OPTIONS: "detect_leaks=1:allow_user_segv_handler=1:disable_coredump=0", - // LSan always reports some process-lifetime allocations (VM - // identifiers, sourcemap buffers). exitcode=0 keeps those from - // failing the process; the assertion below inspects the report - // for frames specific to this leak. - LSAN_OPTIONS: "exitcode=0", - }, - stdout: "pipe", - stderr: "pipe", - }); - const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); - - // The script itself must have run to completion. - expect(stderr).toContain("ran"); - - // What must be absent is any leak whose allocation stack runs through the - // SystemError constructors: those are exactly the WTF::StringImpl refs - // this change releases via Drop. - const leakedSystemError = stderr - .split("\n") - .filter(l => l.includes("to_shell_system_error") || l.includes("to_system_error")); - if (leakedSystemError.length) { - console.error("LSan reported SystemError leak frames:\n" + leakedSystemError.join("\n")); - } - expect(leakedSystemError).toEqual([]); - expect({ stdout, exitCode }).toEqual({ stdout: "", exitCode: 0 }); - }, - 30_000, - ); - describe.serial("not leaking ParsedShellScript when ShellInterpreter never runs", () => { function doit(builtin: boolean) { test(builtin ? "builtin" : "external", async () => {