sys: make SystemError own its strings via OwnedString - #35335
Conversation
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<c_int>; 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.
|
Warning Review limit reached
Next review available in: 2 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (35)
Comment |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
CI on build 79023:
Ran |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
No bugs found, but this is a 36-file refactor of a foundational refcounted FFI type (SystemError), so it's worth a human look before landing.
What was reviewed:
OwnedStringis#[repr(transparent)]overString, so the#[repr(C)]bun_jsc::SystemErrorlayout stays bit-identical toheaders-handwritten.h.Bun::toJS/toWTFStringon the C++ side add their own ref (borrow), so Drop releasing the Rust-side +1 is equivalent to the old explicitself.deref()— no double-free.- Field reassignments (
err.path = ....into()) now drop the previousOwnedString; checked each site's prior value is empty/static/intended-to-be-replaced, so no new UAF and one fewer leak inValueError::reset. fdsentinel change (-1→c_int::MINvia..Default::default()) is invisible to C++, which only checkserr.fd >= 0.
Extended reasoning...
Overview
This PR converts the six bun_core::String fields on both bun_sys::SystemError and bun_jsc::SystemError to bun_core::OwnedString, moving the WTF::StringImpl deref() from every consumer's exit path into the type's Drop. It removes SystemError::{deref, ref_, dupe}, ShellErr::deinit, and two now-empty Drop impls, collapses three hand-rolled bun_sys → bun_jsc marshallers into the existing From impl, and changes bun_sys::SystemError.fd from a c_int::MIN-sentinel to Option<c_int> (mapped back to the sentinel at the FFI boundary). The remaining ~30 files are mechanical: .into() on every field initializer, deletion of manual .deref()/err.deinit() calls, and ..Default::default() in place of spelled-out empty fields. A new LSan-based leak test exercises the shell-builtin error paths that previously leaked.
Security risks
None. This is internal refcount management with no user-facing surface, input parsing, or auth/crypto changes.
Level of scrutiny
High — this is exactly the memory-safety category REVIEW.md calls out as most-blocked. The change is well-reasoned and I traced the key invariants (#[repr(transparent)] preserves the C ABI; Bun::toJS in BunString.cpp:188 constructs WTF::String(impl.wtf) which bumps the ref, so C++ borrows and Rust's Drop is the matching release; the old SystemErrorJsc::to_error_instance(&self) already effectively consumed the strings via marshal's bitwise copy, so the self-by-value change makes the ownership contract honest rather than changing it). But the type is threaded through fetch, sockets, DNS, shell, blob I/O, and the node compat layer, and is stored in long-lived slots (ReadFile.system_error, CapturedWriter.err, PendingSystemError) some of which cross threads. That breadth plus the FFI layout dependency deserves a maintainer's eyes.
Other factors
The old marshal(&self) bitwise-copied the Copy String handles and then to_error_instance deref'd them, so a caller reusing the borrowed bun_sys::SystemError afterward would have hit UAF anyway — the &self → self signature change closes that trap rather than introducing a new constraint. The fd: -1 → ..Default::default() (→ c_int::MIN) shifts at a handful of sites are behavior-preserving because the C++ side gates on err.fd >= 0 (bindings.cpp:2309). The new test is well-targeted (LSan with Malloc=1, filters for to_shell_system_error/to_system_error frames, includes fail-before evidence) and correctly gated on isASAN && !isWindows. rust:check-all and clippy are reported clean.
| #[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, | ||
| } |
There was a problem hiding this comment.
🔴 #[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())insideValueError::dupe(), which is called fromBody::Valuecloning (e.g.Response#clone()when the body is inValue::Error(ValueError::SystemError(..))state). TheSystemErrors stored here carry heapWTFStringImpls:FetchTaskletbuildsmessage/path/hostnameviacreate_format/clone_utf8, andread_file.rssetspath: BunString::clone_utf8(...).into().src/runtime/socket/socket_body.rs:1207—PendingSystemError(Some(err.clone())). Currently benign only by accident: every field there isBunString::static_(...)(Tag::StaticZigString, whosederef()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
fetch()fails DNS;FetchTaskletbuilds ajsc::SystemErrorwithmessage = BunString::create_format(...)andhostname = BunString::clone_utf8(host)— bothTag::WTFStringImplwith refcount 1 — and stores it asBody::Value::Error(ValueError::SystemError(err))on theResponse.- User calls
response.clone().Body::Value::clone→ValueError::dupe→ValueError::SystemError(e.clone())(Body.rs:630). e.clone()runs the derived<SystemError as Clone>::clone, which callsOwnedString::clone()on each field →Self(self.0.clone())→ bitwise copy of theString.hostname'sWTFStringImplstill has refcount 1, but is now referenced by twoOwnedStrings.- The cloned
Responseis dropped (or its body error is consumed viato_error_instance, whoseselfthen drops).SystemError::drop→OwnedString::drop→String::deref()onhostname: refcount 1 → 0,WTFStringImplfreed. - The original
Responseis dropped. Same path runsderef()on the same freedWTFStringImpl→ 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 Drop for CapturedWriter { | ||
| fn drop(&mut self) { | ||
| // `bun_sys::SystemError` strings drop themselves. | ||
| let _ = self.err.take(); | ||
| // self.writer Arc drops automatically. | ||
| } | ||
| } | ||
|
|
||
| impl PipeReader { |
There was a problem hiding this comment.
🟡 This PR removes impl Drop for CapturedWriter, but leaves behind a stale reference at src/runtime/shell/subproc.rs:2088-2090: an empty if !self.captured_writer.dead { ... } block whose only content is the comment // CapturedWriter::drop handles err.deref() and writer Arc drop. — pointing at the impl this PR just deleted. Per REVIEW.md ('Delete dead code in the same PR that makes it dead'), the empty block and stale comment should be removed here.
Extended reasoning...
What the issue is
This PR deletes impl Drop for CapturedWriter (the removed hunk around src/runtime/shell/subproc.rs:1610-1616), because with SystemError's string fields now being OwnedString the manual self.err.take() in that Drop impl is a no-op — the fields release themselves via their own Drop.
However, further down in the same file there remains:
if !self.captured_writer.dead {
// CapturedWriter::drop handles err.deref() and writer Arc drop.
}(src/runtime/shell/subproc.rs:2088-2090)
Why it's a problem
The comment references CapturedWriter::drop, which no longer exists after this PR. And the if block has no body other than that comment — it is dead code (the condition read has no side effect; dead is a plain bool).
REVIEW.md is explicit on both counts:
- "Delete dead code in the same PR that makes it dead (required scope — name the deletions in the description): superseded implementations, helpers whose last caller you rewired, fields nothing reads, parameters discarded in the body, guards a new validator makes redundant."
- "Comments carry only durable non-obvious content … No narrating what the code does."
The PR description already lists CapturedWriter::drop under Removed, so this leftover was clearly an oversight rather than intentional.
Step-by-step proof
- The diff removes lines 1613-1618 of the old file:
impl Drop for CapturedWriter { fn drop(&mut self) { let _ = self.err.take(); } }. rg 'CapturedWriter::drop' src/runtime/shell/subproc.rson the PR branch yields exactly one hit at line 2089 — the comment inside the emptyifblock.- There is no other
Dropimpl forCapturedWriteranywhere in the tree, so the comment now points at nothing. - The
ifblock at 2088-2090 contains no statements — removing it is behavior-preserving.self.captured_writer.err(nowOption<bun_sys::SystemError>withOwnedStringfields) andself.captured_writer.writer(Option<Arc<_>>) both release automatically whenselfdrops, which is what the surrounding cleanup path relies on.
Impact
None at runtime — the block is a no-op and the comment is merely misleading. This is purely a code-hygiene issue: a future reader following the comment to understand who releases captured_writer.err will find no such impl and be confused.
Fix
Delete lines 2088-2090:
- if !self.captured_writer.dead {
- // CapturedWriter::drop handles err.deref() and writer Arc drop.
- }(Optionally also drop the adjacent / PipeReaderState::Done... comment at 2086 if that's a related leftover, but that's outside this finding.)
2 participants
What does this PR do?
bun_sys::SystemErrorandbun_jsc::SystemErrorheld six barebun_core::Stringfields. BecauseStringis#[derive(Copy)](so it stays bit-identical to the C++BunStringfor by-value FFI) it cannotimpl Drop, so neither struct released its strings on drop. Every consumer had to hand-call.deref()on every exit path (e.g.src/runtime/shell/builtin/yes.rshade.deref()with a comment "no Drop impl on bun_sys::SystemError"). Severalon_io_writer_chunkhandlers and theBuiltin::init_redirectionserror path did not, leaking theWTF::StringImplbehind each path/message.Fix
Move the release into the type system using the existing
bun_core::OwnedString(the#[repr(transparent)]RAII newtype whose doc says to prefer it over ad-hocscopeguardso?early returns can't skip the release):SystemErrorstructs: six string fields becomeOwnedString.#[repr(transparent)]keeps the C++SystemErrorlayout inheaders-handwritten.hbit-identical.bun_sys::SystemError.fd:c_intwithMINsentinel becomesOption<c_int>. Thebun_jscstruct keepsc_intfor the C ABI;From<bun_sys::SystemError>mapsNonetoc_int::MIN(C++ checkserr.fd >= 0).SystemError::deref,SystemError::ref_,SystemError::dupe,ShellErr::deinit,PendingSystemError::drop,CapturedWriter::drop.jsc::SystemErrornow#[derive(Clone)](OwnedString::clone bumps the refcount).SystemErrorJsctrait:&selfbecomesself. The three hand-rolledbun_sys→bun_jscbridge fns (marshal,to_jsc_system_error,sys_system_error_to_js) collapse intoFrom..deref()at every consumer is deleted, along with struct-literal fields that spelled outOwnedString::default()/fd: c_int::MIN/fd: -1where..Default::default()covers them.36 files, +313/−467.
Verification
The new test runs failing shell redirects under
Malloc=1 ASAN_OPTIONS=detect_leaks=1and asserts LSan reports no leak whose allocation stack runs throughto_shell_system_error/to_system_error.Fail-before output (src/ stashed)
bun run rust:check-allpasses on all 10 targets;cargo clippy --workspaceis clean.test/js/bun/shell/bunshell.test.ts: 414 pass, 0 fail.Supersedes #32328 (which added
.deref()calls by hand at each leak site; this change makes them structurally unnecessary).no test proof · iteration 2 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/shell/leak.test.ts