Make bun_core::String own its WTF ref

[dylan-conway]

What does this PR do?

bun_core::String (the Rust mirror of C++ BunString) was a Copy POD with no destructor, so every +1 producer — clone_utf8, create_format, to_bun_string, URL accessors, C++ toStringRef out-params — relied on the caller remembering .deref() or OwnedString. An audit found ~60 places that didn't (per-import ResolvedSource.source_url/specifier, whole HMR bundles in bake, Postgres error fields, valkey/UDP/S3 option strings, parseArgs argv, QUIC headers, …). This moves the +1/-1 rule into the type so those, and any not yet found, are released by Drop.

Ownership is the type now

// before: Copy, no Drop — caller must remember
let s = value.to_bun_string(global)?;   // +1
do_stuff(&s);
s.deref();                              // forgotten in ~60 places

// after: String owns one ref. Drop = deref, Clone = ref, moves are free.
let s = value.to_bun_string(global)?;
do_stuff(&s);                           // released at scope exit, incl. `?` paths

FFI signatures carry ownership. String has the same layout as C++ BunString, so a by-value String in an extern signature means the ref crosses (like Box<T> in FFI); &String / StringView<'_> mean borrow. C++ that only reads takes const BunString*.

safe fn URL__host(url: &URL) -> String;                        // C++: Bun::toStringRef (+1) → we own it
pub fn host(&self) -> String { URL__host(self) }

extern "C" fn BakeProdResolve(global: &JSGlobalObject, a: &String, b: &String) -> String {
    String::create_format(format_args!("..."))                 // C++: transferToWTFString()
}

safe fn ZigSourceProvider__getSourceSlice(p: &SourceProviderMap) -> StringView<'_>;   // C++: Bun::toStringView (+0)

Borrows are &String / StringView<'a> (a by-value borrow tied to whatever owns the characters: property-iterator names, substring/trunc, JsCell<String>::get), and consuming into JS is into_js(self) (was transfer_to_js(&mut self)):

return String::clone_utf8(name).into_js(global);   // ref moves into the JSString, no ref/deref pair

Reading a JS string's characters is a scoped borrow. JSValue::get_zig_string() returned a ZigString aliasing a JSString's buffer with no lifetime, no ref, and nothing keeping the cell alive. It is removed; callers either take an owned to_bun_string(), or:

let v = value.to_js_string_view(global)?;   // one FFI call: toString + JSString::view (no ref; substring ropes aren't flattened)
use(v.latin1(), v.to_utf8(), v.length());   // derefs to &String
// drop(v) keeps the JSString cell observable to the GC until here (like C++ GCOwnedDataScope)

Use it for a scoped read; use to_bun_string() when the string must be kept. JSString::view() returns the same JSStringView guard; JSPropertyIterator::next() yields (StringView<'_>, JSValue) tied to the iterator. get_class_name/get_name_property return an owned String (they handed out views into C++ temporaries).

ResolvedSource is owned on both sides. Rust ResolvedSource/Errorable<T> own their strings, bytecode and module_info; C++ ErrorableResolvedSource gets a destructor, and Zig::SourceProvider::create takes what it keeps via transferToWTFString(). The unused specifier field is removed. Previously only ~SourceProvider released specifier/source_url, so every loader arm that doesn't build a provider (JSON/TOML/text/object exports, builtins, already-required CJS) leaked two strings per import. The LoaderHooks fn-pointer table between bun_jsc and bun_runtime is replaced by direct extern "Rust" calls; Errorable's error arm is just the JSValue (the code was never read).

Fixes

Leaks (a WTF::StringImpl ref — or the whole string — never released), per occurrence unless noted:

• every import/require of JSON, JSONC, TOML, YAML, text, HTML, bun:main, internal/*, builtins and already-required CJS: ResolvedSource.specifier and source_url (and source_code for the JSON/object/builtin arms)
• every async (await import) module fulfilment: source_url
• bun --hot / bake dev server: the entire server HMR patch bundle on each reload; the initial server runtime; BakeProdResolve/BakeProdLoad/BakeToWindowsPath results
• Bun.sql (Postgres): every field of every error/notice message; the JSON.stringify result for each json/jsonb bind parameter (1.4.0 regression: bun:sql leaks native memory on every query binding a jsonb parameter #40102); inline column-name atoms in CachedStructure
• Bun.sql (MySQL): JSON column values
• new Bun.RedisClient(url): the URL string and each parsed component; every RESP map key
• Bun.udpSocket({ hostname }) / connect address; Bun.serve({ unix }); the server URL passed to the inspector; error-path hostname strings
• S3 error code/message
• node:util parseArgs: one string per argv element and per short option
• node:quic: header names/values, ALPN/SNI/status strings; node:http2 custom settings keys
• HTMLRewriter element.attributes iterator name/value
• node:module new SourceMap(payload) sources/names, findSourceMap path
• expect(v, label), expect.extend matcher names, describe(fn)/test(fn) derived names, console.log table column names / class display names, console.trace() (built the exception twice)
• ResolveMessage.code, Bun.$ escape, IPC cmd strings, process.title = …, Worker creation error message, WebSocket client close reason on error paths, Bun.Transpiler transform result when warnings reject, bun:ffi cc/linkSymbols strings on error, import.meta.url define in the bundler, bun install hosted-git URL parts, bun build --bytecode chunk source URL, code-coverage source URL, fs.Dirent name/path on early return, bundler plugin onResolve/onLoad strings when the plugin is tombstoned
• worker VirtualMachine.main_resolved_path at teardown
• the .jsc bytecode buffer and ModuleInfo for already-bundled modules on some paths
• ZigException::Holder: strings of stack frames hidden by the frame filter

Use-after-free / lifetime:

• Bun.dlopen read its path argument after walking the user symbols object (getters could drop it)
• JSX tag name in console.log/expect formatting was reused after printing props/children
• Bun.YAML.stringify kept anchor names past the property iterator that owned them
• getClassName/getNameProperty returned views into function-local WTF::Strings
• os.userInfo() options were ptr::read out of C++-owned memory (double free)
• any toString()-created temporary JSString whose characters were read after a GC point (now pinned by JSStringView)

Other:

• BakeLoadInitialServerCode copied the whole server HMR runtime into a new StringImpl at dev-server start; it now wraps the embedded bytes
• non-ASCII paths in async-module load errors were formatted as Latin-1 (mojibake); encoding behaviour is otherwise unchanged
• UDP socket address/remoteAddress getters swallowed a pending exception as undefined

Also removes Bun::toString(JSGlobalObject*, JSValue) (a +1 producer named like a borrow), StringCell/OwnedString, the duplicate bun_jsc::URL wrapper (now bun_url::whatwg::URL, held via the RAII whatwg::Parsed), and to_slice_or_null/to_slice_clone (identical to to_slice).

No extra refcount traffic or allocations vs main: Drop/Clone are the same tag-check + inc/dec the manual calls were; to_js_string_view is one FFI call like get_zig_string was; and a number of ref()/dupe_ref() pairs, a full transpiler-cache output copy, and per-module specifier refs are gone.

Struct size changes (nothing grows; shared layouts are asserted on both sides):

• ResolvedSource (Rust ⇄ C++): 160 → 136 bytes — specifier and the unused allocator removed, needsDeref → bytecode_cache_owned. ErrorableResolvedSource follows (168 → 144).
• Zig::SourceProvider (C++): no longer embeds a ResolvedSource; keeps only m_moduleInfo/m_tag/m_alreadyBundled (~144 bytes smaller per loaded module).
• Rust-only: TranspilerJob drops its fetcher field; RuntimeTranspilerCache::Entry::output_code is a plain String instead of an enum; ZigException::Holder uses Option<ZigException> instead of MaybeUninit + loaded: bool.
• Unchanged: bun_core::String (24, asserted), StringView/JsCell<String> (transparent), ZigStackTrace/ZigStackFrame, SystemError, generated class payloads.

Fixes #40102
Supersedes #32293.

How did you verify your code works?

cargo check/clippy on linux/macOS/windows targets; debug+ASAN build driven through module loading (ESM/CJS/JSON/TOML/text/?query, builtins, virtual modules), console.log/Bun.inspect (errors, JSX, boxed primitives, symbols, classes), node:path, node:module SourceMap, workers, process.title, os.userInfo, WebSocket send/publish, UDP/unix-socket serve, parseArgs, TOML/YAML/XML stringify, Bun.dlopen, bake dev server startup. Ran the module, resolve, path, parse-args, worker, inspect(-error), expect, shell, transpiler-cache, websocket-server, serve, zlib, udp, dns, http2 and serve-body-leak test files locally against main's debug build. New RSS regression tests (each repeats the operation with a ~256 KiB string and fails on 1.4.0 with the growth shown): Postgres jsonb bind (166 MiB) and ErrorResponse fields (125 MiB) via the wire-frames.ts mock, util.parseArgs (77), expect(v, label) (76), module.SourceMap (158), Bun.udpSocket({hostname}) (73), HTMLRewriter attributes (90), new RedisClient(url) (145), RESP map keys (150), S3 error (47). The use-after-free fixes are not observable on a release build of 1.4.0 without ASAN, so they have no test.

[robobun]

Verified this closes the jsonb bind leak from #40102 with the regression test from #32293 (test/js/sql/sql-postgres-json-bind-leak.test.ts at df38968). It is hermetic: an in-process mock Postgres replies ParameterDescription([114]), then the fixture binds a 512 KiB JSON payload 300 times and measures RSS with the ASAN quarantine off.

Debug+ASAN build, same machine:

main (unfixed):  deltaMiB 163.4  -> fail (threshold 50)
this branch:     deltaMiB  12.6  -> pass

This PR has no test files. If you want a guard for #40102 in the same change, that file cherry-picks cleanly onto this branch. Happy to leave it to you either way.

[robobun]

^{Updated 8:56 PM PT - Aug 23rd, 2026}

❌ @dylan-conway, your commit 7007f7a has 1 failures in Build #104554 (All Failures):

• test/js/node/test/sequential/test-cpu-prof-drained.js - pid 512 segmentation fault at address 0xffffffffffffffc8 on 🪟 2019 x64

---

🧪   To try this PR locally:

bunx bun-pr 40238

That installs a local version of the PR into your bun-40238 executable, so you can run:

bun-40238 --bun

[dylan-conway]

Pulled test/js/sql/sql-postgres-json-bind-leak.test.ts in from #32293 in 07e13f7 (166 MiB on 1.4.0 → passes here).

[alii]

Awesome

[claude]

Automated review ran clean on e34fd40 — no findings this pass, and the earlier nits (stale comments, the ConsoleObject reborrow, the JSPropertyIterator lifetime note) are all addressed. Given the scope — a String ownership-model change across 225 files plus FFI-shared struct layout changes on both the Rust and C++ sides — this still warrants a human pass.

Checked: ResolvedSource/ErrorableResolvedSource layout agrees on both sides (136-byte static_assert in headers-handwritten.h ↔ assert_ffi_layout! in ResolvedSource.rs); Zig::SourceProvider::create now takes every retained field by transferToWTFString()/std::exchange so the C++ destructor and Rust Drop don't both release. Errorable<T>::Drop only drops the value arm when success, matching ~ErrorableResolvedSource. The to_error_instance family no longer consumes the caller's ref (C++ side unchanged — it never deref'd), so removing the post-call .deref() is correct. JsCell<String>::get() returning a borrow (was OwnedStringCell) — verified callers either .clone() or use it within the cell's lifetime.

Extended reasoning...

Overview

This PR converts bun_core::String from a Copy POD with manual .deref() to an owning type where Drop derefs and Clone refs. It removes OwnedString/OwnedStringCell/OwnedResolvedSource, introduces StringView<'a> for by-value borrows, and updates ~220 call sites. On the C++ side it removes the Bun::toString(JSGlobalObject*, JSValue) +1 producer, gives ErrorableResolvedSource a destructor, shrinks ResolvedSource (drops specifier/allocator/needsDeref), and makes Zig::SourceProvider take retained fields by transfer instead of storing a ResolvedSource copy. FFI signatures are updated so by-value String = ownership crosses, &String/const BunString* = borrow.

Security risks

None specific to this change. The PR is a memory-ownership refactor; the risk class is use-after-free / double-deref / leak at the FFI boundary, not injection or auth. The regression test added (postgres-json-bind-leak) guards one of the ~60 leaks the PR closes.

Level of scrutiny

High. This is a foundational memory-safety change: every bun_core::String producer/consumer's refcount contract changed, struct layouts shared with C++ changed (with static_asserts on both sides), and the module-loader's ResolvedSource ownership protocol was rewritten. A missed site means either a UAF (extra deref) or a leak (missing one). The author ran ASAN + targeted test files locally; full CI is pending on build #104335.

Other factors

Three prior automated review passes surfaced only comment/cosmetic nits, all fixed. The bug hunting system found nothing on this pass. The change is internally consistent and well-reasoned (the PR description accurately maps every struct-size change and asserts layouts on both sides). But 225 files of refcount-contract changes across an FFI boundary is exactly the category REVIEW.md's "Native code: memory safety" section flags as most-blocked — a human familiar with the ResolvedSource/SourceProvider lifecycle should confirm the C++-side transfer points (ZigSourceProvider::create, ~ErrorableResolvedSource, fetchCommonJSModule's builtin path) match the new Rust ownership.

[claude]

I've reviewed this PR across several rounds and the bug-hunting pass on the latest commits found no issues (earlier rounds surfaced only stale-comment nits, all addressed). Given the scope — 243 files rewriting bun_core::String's ownership model across the Rust/C++ FFI boundary, plus ResolvedSource/ErrorableResolvedSource gaining destructors on both sides and the removal of JSValue::get_zig_string — a human sign-off on the overall ownership design is still warranted before merge.

What was reviewed

• String Drop/Clone vs. former manual .deref() sites — checked for double-frees where old explicit derefs remained; none found (all scopeguard/.deref() calls in the diff were removed, not left alongside Drop).
• FFI signature agreement: spot-checked that by-value String returns on the Rust side pair with transferToWTFString()/Bun::toStringRef on the C++ side, and &String ↔ const BunString*.
• ErrorableResolvedSource C++ destructor vs. Rust Drop for Errorable<T> — both gate on success and release the same fields; Zig::SourceProvider::create now std::exchanges out what it keeps.
• ZigException::Holder new Drop only releases the populated ..frames_len/..source_lines_len prefixes — matches how C++ populates them.

Extended reasoning...

Overview

This PR changes bun_core::String from a Copy POD (manual .deref()) to an owning type where Drop = deref and Clone = ref, then propagates that through 243 files: every +1 producer now returns an owned String, borrows become &String/StringView<'_>, OwnedString/OwnedStringCell/OwnedResolvedSource are deleted, transfer_to_js becomes consuming into_js, and ResolvedSource/Errorable<T> gain real destructors on both the Rust and C++ sides. It also removes JSValue::get_zig_string/to_zig_string (unsound borrow with no lifetime) and reworks ZigException::Holder, Zig::SourceProvider, and the module-loader hook signatures.

Security risks

None net-new. The change is memory-ownership plumbing; it does not add parsing of untrusted input, auth, or crypto. The risk class is memory safety (double-free / UAF / leak) rather than a security boundary — and the direction of the change is toward fewer leak/UAF hazards by moving the +1/-1 rule into the type system.

Level of scrutiny

High. This is a foundational refactor of the most-used string type across the FFI boundary, with coordinated layout and destructor changes in both Rust and C++ (headers-handwritten.h, ZigSourceProvider, ~ErrorableResolvedSource). Per the approval guidelines this is squarely "complex, large, touches critical code paths" — not a candidate for bot-only approval regardless of how clean the individual hunks look.

Other factors

Four prior bug-hunting rounds surfaced only stale-comment nits (all fixed in b7691c1, e22735d, e34fd40). This run found nothing. A regression test for #40102 was pulled in. Layout is guarded by assert_ffi_layout!/static_assert. One maintainer left a positive comment but no formal approval is visible. The two newest commits (045baf2, 83737ac) are further cleanups in the same vein and were covered by this run.