Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
113 changes: 42 additions & 71 deletions src/jsc/SystemError.rs
Original file line number Diff line number Diff line change
@@ -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,
}
Comment on lines 8 to 22

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 #[derive(Clone)] on SystemError relies on OwnedString::clone(), but that impl (src/bun_core/string/mod.rs:1259) is Self(self.0.clone()) where bun_core::String is #[derive(Copy)] — a bitwise copy with no WTF refcount bump (its own doc comment is wrong; contrast OwnedStringCell::clone, which uses .dupe_ref()). Both the original and the clone then run OwnedString::drop → String::deref() on the same WTFStringImpl, decrementing twice for one ref → double-free/UAF. This PR replaces the previously-correct .dupe() (which did ptr::read + ref_()) with .clone() at Body.rs:630 (reachable via Response#clone() on an errored body whose SystemError carries create_format/clone_utf8-backed strings from FetchTasklet/read_file.rs) and socket_body.rs:1207. Fix: make OwnedString::clone() use self.0.dupe_ref() to match its doc and OwnedStringCell.

Extended reasoning...

What the bug is

The PR description states: "jsc::SystemError now #[derive(Clone)] (OwnedString::clone bumps the refcount)". That premise is false.

OwnedString::clone() at src/bun_core/string/mod.rs:1258-1260 is:

impl Clone for OwnedString {
    /// Bumps the WTF refcount (or copies a `ZigString` into a fresh
    /// WTF::StringImpl) and wraps the resulting +1 in a new `OwnedString`.
    #[inline]
    fn clone(&self) -> Self {
        Self(self.0.clone())
    }
}

where self.0 is bun_core::String, which at line 63-65 is:

#[repr(transparent)]
#[derive(Clone, Copy)]
pub struct String(pub bun_alloc::String);

For a Copy type, the derived Clone::clone() is a trivial bitwise copy — no refcount bump. The doc comment on OwnedString::clone is simply wrong. The correct refcount-bumping copy is String::dupe_ref() (line 519: self.ref_(); *self), which is exactly what the sibling OwnedStringCell::clone() at line 2487 uses. Meanwhile OwnedString::drop() (line 1239) calls self.0.deref().

So cloning an OwnedString produces two owners of the same WTFStringImpl pointer with only one ref between them; when both drop, deref() runs twice → the second decrement frees (or corrupts) an already-freed WTF::StringImpl.

The specific code path this PR breaks

Before this PR, jsc::SystemError had a hand-written .dupe():

pub fn dupe(&self) -> SystemError {
    let mut v: SystemError = unsafe { core::ptr::read(self) };
    v.ref_();  // bumps every string field
    v
}

which was correct: bitwise copy + one ref_() per field, so both the original and the copy each held a balanced +1 that their eventual consumers released.

This PR deletes .dupe()/.ref_() and replaces the two call sites with .clone():

  • src/runtime/webcore/Body.rs:630 — ValueError::SystemError(e.clone()) inside ValueError::dupe(), which is called from Body::Value cloning (e.g. Response#clone() when the body is in Value::Error(ValueError::SystemError(..)) state). The SystemErrors stored here carry heap WTFStringImpls: FetchTasklet builds message/path/hostname via create_format/clone_utf8, and read_file.rs sets path: BunString::clone_utf8(...).into().
  • src/runtime/socket/socket_body.rs:1207 — PendingSystemError(Some(err.clone())). Currently benign only by accident: every field there is BunString::static_(...) (Tag::StaticZigString, whose deref() is a no-op), but nothing enforces that.

Why existing code doesn't prevent it

The comment right above Body.rs:630 says ".clone() on BunString/SystemError already bumps the refcount" — that comment (and the identical claim in the PR description) is exactly the mistaken assumption. The type system has no way to catch this: OwnedString implements Clone, so #[derive(Clone)] on SystemError compiles cleanly; the refcount imbalance only manifests at runtime. The new LSan test in this PR checks for leaks on shell error paths, not double-frees on the Body/socket clone paths, so it doesn't cover this.

Step-by-step proof

  1. fetch() fails DNS; FetchTasklet builds a jsc::SystemError with message = BunString::create_format(...) and hostname = BunString::clone_utf8(host) — both Tag::WTFStringImpl with refcount 1 — and stores it as Body::Value::Error(ValueError::SystemError(err)) on the Response.
  2. User calls response.clone(). Body::Value::clone → ValueError::dupe → ValueError::SystemError(e.clone()) (Body.rs:630).
  3. e.clone() runs the derived <SystemError as Clone>::clone, which calls OwnedString::clone() on each field → Self(self.0.clone()) → bitwise copy of the String. hostname's WTFStringImpl still has refcount 1, but is now referenced by two OwnedStrings.
  4. The cloned Response is dropped (or its body error is consumed via to_error_instance, whose self then drops). SystemError::drop → OwnedString::drop → String::deref() on hostname: refcount 1 → 0, WTFStringImpl freed.
  5. The original Response is dropped. Same path runs deref() on the same freed WTFStringImpl → use-after-free / double-free.

Before this PR, step 3 went through .dupe() → ref_(), so refcount was 2 after the clone and both drops balanced.

Impact

Memory-safety regression (double-free / UAF of WTF::StringImpl) on a user-reachable path (Response#clone() on any fetch/file-read that errored). Per REVIEW.md's memory-safety section ("reference counts provably balanced on every terminal path — success, error, cancellation, finalize"), this is squarely in the most-blocked category.

Fix

Fix the root cause: change OwnedString::clone() to Self(self.0.dupe_ref()), matching its own doc comment and OwnedStringCell::clone. That makes #[derive(Clone)] on both SystemError structs correct with no further changes. (Alternatively, hand-implement Clone for jsc::SystemError with per-field .dupe_ref() — but the OwnedString fix is the right layer, since anything else that starts cloning an OwnedString will hit the same trap.)


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<bun_sys::SystemError> 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,
}
}
}
Expand All @@ -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
Expand Down Expand Up @@ -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)
}
}

Expand All @@ -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()
};

Expand Down
36 changes: 10 additions & 26 deletions src/runtime/api/bun/js_bun_spawn_bindings.rs
Original file line number Diff line number Diff line change
Expand Up @@ -53,22 +53,6 @@ fn signal_code_from_js(val: JSValue, global: &JSGlobalObject) -> JsResult<Signal
bun_sys_jsc::signal_code_jsc::from_js(val, global)
}

/// Convert a `bun_sys::SystemError` (T1 stub shape) into the C-ABI
/// `bun_jsc::SystemError` and materialize a JS Error instance.
fn sys_system_error_to_js(err: &bun_sys::SystemError, global: &JSGlobalObject) -> 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<Terminal>`
/// to a `BackRef<Terminal>` used by `Subprocess.terminal`, so the scopeguard /
/// field-assignment paths share one pointer type with `existing_terminal`.
Expand Down Expand Up @@ -1211,7 +1195,8 @@ pub(crate) fn spawn_maybe_sync<const IS_SYNC: bool>(
} 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.
Expand Down Expand Up @@ -1241,8 +1226,9 @@ pub(crate) fn spawn_maybe_sync<const IS_SYNC: bool>(
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),
));
}
}
_ => {}
Expand Down Expand Up @@ -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))
}
Expand Down
4 changes: 2 additions & 2 deletions src/runtime/api/html_rewriter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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()
}
}
Expand Down
24 changes: 13 additions & 11 deletions src/runtime/dns_jsc/cares_jsc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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()
};

Expand Down Expand Up @@ -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);
Expand All @@ -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()
}
}
Expand Down
23 changes: 7 additions & 16 deletions src/runtime/dns_jsc/dns.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)))
}
Expand Down Expand Up @@ -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)));
}
Expand Down
12 changes: 4 additions & 8 deletions src/runtime/ffi/ffi_body.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down
12 changes: 4 additions & 8 deletions src/runtime/node/node_fs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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),
);
Expand Down
Loading
Loading