Repository navigation
webcore: remove unsafe from Response/Request/Body/ReadableStream/FileReader/Sink - #40411
Jarred-Sumner wants to merge 8 commits into
Conversation
There was a problem hiding this comment.
Thanks for fixing the into_js doc comment. This run found no further issues. Given the scope — 51 files rewriting ownership/refcounting across Response/Request/Body/ReadableStream/FileReader/Sink, new primitives (OwnedThis, SourceRef, finalize_owner/destroy_weakly_held, LockedRead), and codegen changes — a maintainer look is still warranted.
What was reviewed: refcount balance across NewSource::retain/SourceRef::drop/finalize and the FileReader read_ref/reader_refs paths; weak_ptr rewrite (Cell<u32>, finalize_owner) against Request/Response finalize; WTFString ref transfer in Value/AnyBlob (clone-then-drop vs. old ManuallyDrop); RequestContext.sink OwnedThis drop ordering vs. re-entrant JS in do_render_stream/handle_resolve_stream; BodyMixin LockedRead split so readableStreamTo* runs after the body borrow releases.
Extended reasoning...
Overview
This PR is part of a stacked series removing unsafe from src/runtime/webcore/. It touches 51 files (~515KB diff) and rewrites the ownership model for Response, Request, Body, ReadableStream sources (ByteStream/ByteBlobLoader/FileReader), and JSSink. Key structural changes: Body.value becomes a JsCell<Value> accessed via closure-scoped with_mut; Value::WTFStringImpl holds an owned WTFString (Drop releases the ref) instead of a raw *mut StringImpl with hand-managed ref/deref; NewSource<C> becomes CellRefCounted with a SourceRef<C> handle and SourceContext methods flip to &self; bun_ptr::weak_ptr is rewritten around Cell<u32> with a safe HasWeakPtrData trait and finalize_owner(Box<T>); Response gains CellRefCounted + destroy_weakly_held; RequestContext.sink becomes JsCell<Option<OwnedThis<…>>>; codegen (generate-jssink.ts, generate-classes.ts) emits new __getThis/take_ptr/FinalizeReceiver/sink_handle_from_id helpers. BodyMixin readers are refactored to return LockedRead/BodyRead so JS-running work happens after the body borrow is released.
Security risks
No direct security surface change (no auth/crypto/parsing of untrusted formats). The risk profile is memory safety: refcount imbalance, use-after-free on GC finalize / re-entrant JS, double-drop of WTFString refs, and aliasing under JsCell::with_mut re-entry. The PR description explicitly flags several parity-left aliasing shapes (e.g. Value::resolve inside with_mut) as owned by sibling PRs.
Level of scrutiny
High. This is core-runtime memory-management code on GC-interacting hot paths (new Response, .text(), Bun.serve streaming). Mistakes surface as UAF/leaks under GC pressure or re-entrancy, which the test suite may not catch deterministically. The author reports extensive Debug+ASAN test coverage and perf measurements, but the sheer breadth of ownership-model changes and the stacked/shared-primitive nature make this a change a maintainer should sign off on.
Other factors
My previous run flagged only a doc-comment nit on Response::into_js; commit 9cc27334 addressed it (the diff now shows the single correct sentence). No bugs were found in the current run. The PR is stacked on #40221 and shares primitives with several sibling PRs, so a maintainer should also confirm the merge/retarget plan. I checked the drop-ordering in RequestContext where owned_sink is taken before re-entrant JS runs and dropped last — the &mut from sink_mut() into the same allocation is held across that span, which is the same aliasing shape as before (documented in the field's safety note) but worth a maintainer's eye.
e3cc592 to
957214c
Compare
9cc2733 to
cb3798d
Compare
|
Updated 8:18 AM PT - Sep 8th, 2026
❌ @Jarred-Sumner, your commit 421f258 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 40411That installs a local version of the PR into your bun-40411 --bun |
65296c6 to
6f49cdf
Compare
cb3798d to
e7200a3
Compare
6f49cdf to
178f765
Compare
4bea94f to
6af4641
Compare
aaa2ee2 to
e231232
Compare
…reams to zero unsafe
Body is now `JsCell<Value>` reached through closure-scoped borrows;
BodyMixin getters split into a borrow-scoped front half and a script
half (`LockedRead`) that runs once the borrow is released.
Response is CellRefCounted with its weak-pointer finalize protocol in
bun_ptr (`HasWeakPtrData`, `finalize_owner`, `destroy_weakly_held`);
`to_js`/`into_js`/`to_js_retained` hand a typed `RefPtr` to the wrapper
and to native holders (FetchTasklet, HTMLRewriter). The abort listener is
a `NativeAbortListener` registration. Request finalizes through the same
weak-pointer protocol and owns its pooled body as a `BodyHiveHandle`.
`NewSource<C>` is CellRefCounted with a `SelfRoot`; `SourceRef<C>` is
the counted native reference (FileReader's in-flight read ref and the
reader's re-entrancy refs are typed slots). FileReader's BufferedReader
parent is the enclosing Source (`impl_buffered_reader_parent!` borrow =
this mode with a nested reader path; `read_from`/`close_from`/
`start_from`/`unpause_from` dispatch without a live reader borrow).
JSSink host functions resolve `this` in the generated thunks
(`${name}__getThis`); `__finalize`/`__controllerFinalize` dispatch on
`JsSinkType::FINALIZE` (ThisPtr / &mut / Box); the SinkID map and
`Bun__NativeTransformSink__writeBytes` / `Bun__onSinkDestroyed` are
generated. `assign_to_stream` installs the controller through a caller
closure. HTTPServerWritable's teardown is its `Drop`, `abort` returns the
source for the caller to close, the byte-list pool slot is a `PoolGuard`,
and RequestContext owns the sink as `OwnedThis`.
BakeResponse / wasm_streaming / Crypto exports are HOST_EXPORTs.
Lower layers: `RefPtr::from_box`, `OwnedThis`, `JsCell::into_inner`,
`ObjectPool::try_get`, `String::into_wtf` / `From<WTFString>`,
`ExternalShared::into_raw`, `StoreRef::set_mime_type`,
`JSValue::as_direct` checks `is_cell`, generated `take_ptr` for class
wrappers, `boxed_taskable!`.
…e_ref, HTTP3 generic gone, JsSinkType::close_with_error takes ThisPtr with the raw-pointer split in the generated close thunk, NetworkSink fail_from_js_pump on JsCell pending
…ll sites use parent
…rough NewSource::new; HTTPServerWritable names core::ptr::NonNull
6af4641 to
421f258
Compare
…44892) ### Problem - `fetch()` keeps a string body when it ends before it queues a request: 48 rejected calls with a 4 MiB body grow RSS by 192 MiB. `Bun.spawn` and the shell do the same with a `Response` that they refuse. - Cause: the string arm of `blob::Any` (`src/runtime/webcore/Blob.rs:5998`) was a raw pointer that owns one reference, and a drop did not release it. `fetch_impl` drops it at 18 exits, `Stdio::extract_blob` at 2. ### Fix - The arm holds `bun_core::WTFString`: one reference on a `WTF::StringImpl`, released on drop. Drop glue releases it at every exit. No exit is edited. - The reference moves at three sites, with no ref and deref pair. - Verified: 26 new cases in `test/js/web/fetch/fetch-args.test.ts` and `test/js/bun/spawn/spawn-empty-arrayBufferOrBlob.test.ts` fail on main. 19 more suites ran under ASAN: no report. - Self-reviewed: 5 concerns raised, 5 addressed. ### Background - `blob::Any` is the body of `fetch`, `Bun.serve`, `Bun.spawn` and the shell: a `Blob`, bytes, or a JS string held by reference. - #40411 retypes the same arm, and its tip would release the string too. It is stacked on two open PRs and has no test for this. This PR moves the reference where #40411 clones it, and adds 9 conflict hunks to its rebase. - Considered a guard in `fetch_impl` or `Drop for HTTPRequestBody`: both leave `Bun.spawn` and the shell. ### Downsides - `.text` grows 8,448 B: LLVM inlines the smaller `Any::detach` at more call sites. Measured common paths run 7 to 24 fewer instructions. - A wrong hand-off is now a double release, not a leak. 50 refused fetches destroy 50 strings (main: 0). <details><summary>Notes</summary> **For a maintainer to decide** 1. The order with #40411. Its commit ec572ec has the same arm type and a byte-identical `ExternalShared::into_raw`, so its tip would release a dropped string too. It is stacked on #40221 and #40213, was last updated on 2026-09-08, and has no test for this leak. A merge of its head with main conflicts in 39 files. With this PR the count stays 39, and two files gain hunks: `Blob.rs` 51 to 55 and `Body.rs` 26 to 31 (`git merge-tree`). 2. Move or clone at a hand-off. #40411 clones the handle and drops the old one at each hand-off, which is one `ref` and one `deref`. This PR moves the reference, so the hand-offs add no atomic write. 3. The 8,448 bytes. `Any::detach` is 28 bytes smaller, and LLVM now inlines it at more call sites. `#[inline(never)]` on it would give the bytes back and put the call back on those paths. This PR keeps LLVM's choice. **Where the string stayed** - `fetch_impl` has 29 exits between the point where it reads the body and `FetchTasklet::queue`. 4 of them call `detach()`. 25 do not. A string body can reach 18 of the 25 (3 of them only on Windows). The other 7 need a `Blob`, `FormData` or stream body, which main releases already. - The 18 include the `?` on the headers (an invalid header name, a getter that throws), proxy with unix, `file:` with a host, a `blob:` URL that is not registered, an unsupported protocol, GET with a body, the S3 option and signing errors, and the resolved 200 for `file:` and `blob:` URLs. - `Stdio::extract_blob` takes the `Any` by value and returns an error at `stdio.rs:619` and `stdio.rs:632`. `Bun.spawn`, `Bun.spawnSync` and the shell (an external command with `>` or `2>` into a `Response`) reach it. The shell does not accept a `Request`. - The `fetch` half starts with 1.4.0: the Zig `fetchImpl` released the body in one `defer` (`fetch.zig:256` at `bun-v1.3.14`). The spawn and shell half is older: 1.3.14 keeps the string too. - A string with a character outside ASCII, bytes and a `Blob` body do not use this arm on these paths and were always released. **The three sites that move the reference** - `Value::take_wtf_string` (`Body.rs`): from `body::Value` into the arm, for `use_as_any_blob`, `try_use_as_any_blob` and `use_as_any_blob_allow_non_utf8_string`. `Value` keeps its raw arm and its `impl Drop`, so a by-value move out of it does not compile (E0509). The helper forgets the arm and adopts the pointer. That is the one new `unsafe` block. - `Any::take_string` (`Blob.rs`): from the arm into a `bun_core::String`, for `to_string`, `to_json` and `to_array_buffer_view`. It is a by-value `match` on `mem::replace`. - `Value::from(any)` (`Body.rs`): from the arm back into `body::Value`, through `into_raw`. No caller passes a string there today. **Not in this PR** - `body::Value` keeps its raw string arm. #40411 retypes it. - The explicit `detach()` calls after `use_as_any_blob*` stay. They are no longer needed for the string. - The fd of a `Bun.file()` body that `fetch` sends with sendfile is closed only by `HTTPRequestBody::detach`. No exit reaches that state today. A separate fd leak there (`Bun.file(path).slice(0, 0)` as the body) has another cause and is tracked on its own. - #41812 makes `Bun.spawn` refuse a `Request` or `Response` for stdout and stderr before it takes the body. It does not touch `fetch`. **Which thread drops the string** Every holder drops its `Any` on the JS thread: the `fetch_impl` local, `FetchOptions`, `FetchTasklet.request_body` (the tasklet hops to the JS thread for its teardown), `RequestContext.blob`, `Stdio::Blob` and the pipe writer behind it, the shell command, `HTMLRewriter`, WebAssembly streaming and `node/types.rs`. `Bun.write` and `fs.writeFile` copy the bytes into a `Blob` first. **Tests** - Each case runs one child process: 8 warm-up calls, then 48 calls with a fresh 4 MiB ASCII string, then `Bun.gc(true)` and an RSS read. A release build of main grows 192 to 194 MiB in all 26 cases. With the fix it grows 0 to 2 MiB. - A debug build with the fix grows 8 to 45 MiB. A loop that only flattens the string, with no `fetch` at all, measures the same there. So the bound is 48 MiB for release and 96 MiB for debug. - `fetch` cases (19): unsupported protocol, GET with a body, invalid header name, a headers getter that throws, proxy with unix, `file:` with a host, `blob:` not registered, `blob:` registered, `file:`, S3 signing error, S3 option of the wrong type, `Bun.fetch`, `FetchSession.fetch`, `node-fetch`, `undici.fetch`, `undici.request`, a `String` object, a `Request` that carries the body, and an init object as the first argument. - Spawn cases (7): `Bun.spawn` stdout, stderr and `stdio[3]`, `Bun.spawnSync` stdout, a `Request` for stdout, and the shell with `>` and `2>`. - The cases run one child at a time. On a debug build, 19 children at once do not finish within the default timeout. - The tests are not in `fetch-leak.test.ts`, `spawn.test.ts` or `shell/leak.test.ts`. On a debug build of main those three files take 8 to 12 minutes here and have tests that time out, so a before and after run of the whole file proves nothing. **Measurements** (release builds of main 620b50f and of this diff on it, linux x64. perf, valgrind and bloaty are not in the container: the counts are from gdb single-step, GNU objdump, nm and size) - Leak: two scripts of 8 cells each (refused `fetch` calls and refused `Bun.spawn` calls, with bodies of 256 KiB or more) go from 8 of 8 growing cells to 0 of 8. A probe of 41 rows (more `fetch` forms and the other consumers of a string body): 8 growing rows to 0. A shell redirect into a `Response`: 338 and 356 KB per call to 18 to 76 KB, which is the level of a row that only builds and drops a `Response` (13 to 80). - One release per body (gdb breakpoint on `WTF::StringImpl::~StringImpl`, bodies of 262,144 characters): 50 refused fetches destroy 0 strings on main and 50 here (control with no fetch: 50). 50 refused `Bun.spawn` calls with stdout and with `stdio[3]`: 0 and 0 on main, 49 and 49 here (control that drops the `Response`: 49). - Executed instructions, callees included (gdb `stepi` from entry to return, hits 2 and 3 identical): `new Response(string).text()` 179 to 157. `.json()` 32,459 to 32,435. `.bytes()` 1,817 to 1,794. `.arrayBuffer()` 1,996 to 1,973. `FetchTasklet` teardown for GET, a string body and a bytes body: 629, 625, 655 to 618, 614, 644. `RequestContext::finalize_without_deinit` for `Bun.serve` that returns `new Response(string)`: 266 to 256. The number of lock-prefixed instructions is equal in every row. - Executed instructions inside one function (callees stepped over, 3 of 3 identical): `Any::to_string` 58 to 49. `Any::to_json` 63 to 52. `use_as_any_blob_allow_non_utf8_string` 40 to 43: the hand-off helper checks the arm once more. - `fetch_impl` on a fetch that reaches the network: 986 to 986 instructions for a POST with a 1 KiB string body, 755 to 755 for a GET. The function is 23,718 bytes in both builds. - Lock-prefixed instructions, static: the three `use_as_any_blob*` functions 0 to 0. `Any::to_string` 1 to 1. `Any::to_json` 2 to 2. `Any::detach` 1 to 0. Drop glue of `blob::Any` 2 to 3. The one `deref` moved from `detach` into the drop glue. - `size_of` in a release profile: `blob::Any` 112 to 112 bytes, `HTTPRequestBody` 112 to 112, `Stdio` 112 to 112, `body::Value` 168 to 168, `FetchOptions` 664 to 664, `FetchTasklet` 832 to 832. Debug: 384, 384, 384, 384, 936 and 1,128, unchanged. - Release binary: `.text` 65,659,477 to 65,667,925 bytes (+8,448). File 89,142,856 to 89,151,048 bytes (+8,192). 51 symbols change size, net +9,858 bytes. `Any::detach` 196 to 168 bytes. `Stdio::extract_blob` 916 to 1,293 bytes and `Cmd::transition_to_exec` +1,392 bytes: they gain the release on their exits. The 8 `RequestContext::finalize_without_deinit` instances gain 195 bytes each from the inlined `detach`. - `unsafe` lines in `src/`: +1, -0. New `bun_core` surface: `ExternalShared::into_raw` (3 lines). **Suites run on the debug build with ASAN and `Malloc=1`** (so that ASAN sees the frees of WTF strings) `fetch-args`, `spawn-empty-arrayBufferOrBlob`, `body`, `body-clone`, `blob`, `response`, `fetch`, `fetch-leak`, `wasm-streaming`, `html-rewriter`, `html-rewriter-leak`, `bun-write`, `serve`, `bun-serve-static`, `bun-server`, `spawn`, `spawn-stdin-readable-stream`, `spawn-stdin-readable-stream-edge-cases`, `bunshell`, `shell/leak` and `shell/yield`. No ASAN report. The failures are timeouts of the debug build, and tests that need a network or a user that is not root. Also run: `cargo check -p bun_runtime` for `x86_64-pc-windows-msvc` and `aarch64-apple-darwin`, `cargo fmt`, and `bun check` in `test/` (no error in the two test files). **Self-review** 5 concerns, all addressed: three sentences of this description (how the reference left the arm on main, the shell and `Request`, and which strings use the arm), the two counts (18 of 25 exits, and three move sites, not two), `Any::take_string` as a by-value move, the measured numbers in place of a placeholder, and comment blocks cut to one line. One part was not taken as asked: the comment on `Value::take_wtf_string` gives the reason for its `unsafe` (E0509) and does not name #40411, because a comment that names an open PR goes stale. </details> Co-authored-by: Jarred Sumner <jarred@jarredsumner.com>
What
Same programme as #40055 … #40405. Stacked on #40221 (base branch
claude/blob-zero-unsafe→ #40213); retarget as those land. Shares a few primitives with #40202/#40204/#40228 (HeadersRefinbun_jsc,AbortListenerRegistration/listen_native,OwnedThis,impl_buffered_reader_parent!borrow = this,boxed_taskable!) — same hunks.src/runtime/webcore/:Response.rs30 → 0,Body.rs10 → 0,BakeResponse.rs12 → 0,ArrayBufferSink.rs3 → 0,Crypto.rs/ObjectURLRegistry.rs/ByteBlobLoader.rs→ 0;Request.rs20 → 1,ReadableStream.rs23 → 1,FileReader.rs27 → 1,Sink.rs17 → 1,wasm_streaming.rs5 → 1 (each residual is an all-safe fnextern block);streams.rs26 → 7 (see below). Collateral:FetchTasklet.rs−13,RequestContext.rs−4,s3/client.rs−2.Body { value: JsCell<Value> },BodyMixin::body(&self)/body_value()return aLockedReadso thereadableStreamTo*builtins and producer start hooks run after the body borrow is released;Value::WTFStringImpl(WTFString)owns its ref;Response::{to_js(self), into_js(Box<Self>), to_js_retained -> (JSValue, RefPtr<Response>), make_maybe_pooled}; body hive slot allocated viajsc_hooks::body_hive_alloc;HTMLRewriterholds a typedRefPtr<Response>;BakeResponseClass__*,getBodyStreamOrBytesForWasmStreaming,CryptoObject__createareHOST_EXPORTs.NewSource::new(ctx, global) -> RefPtr<Self>,SourceRef<C>,SourceContextall&self,finalize(self: Box<Self>); FileReader callbacks takeThisPtr<Source>and dispatch reads throughBufferedReader::*_from(owner);StreamResult::Pending(BackRef<JsCell<Pending>>);generate-jssink.tsemits${name}__getThis,FinalizeReceiverdispatch,sink_handle_from_id;JSSink::assign_to_stream(.., set_source: impl FnOnce(SourceHandle));HTTPServerWritable::abort(&mut self) -> SourceHandle(caller closes it),destroy→Drop;RequestContext.sink: JsCell<Option<OwnedThis<JSSink<..>>>>.bun_ptr::weak_ptrrewritten (WeakPtrData(Cell<u32>), safeHasWeakPtrData,finalize_owner(Box<T>),unsafe fn destroy_weakly_held);RefPtr::from_box;JsCell::into_inner;ObjectPool::try_get;String::into_wtf/From<WTFString>;image::codecs::Encoded::as_slice(&self).Not zero —
streams.rskeeps 6 real sites that belong to sibling PRs' types:SourceHandle::ShellWritable(BackRef<_, Mut>).get_mut()(#40228 makesShellSubprocessaRootback-ref) andNetworkSink::on_writable/end_from_stream(*mut Self)×5 (#40252 rewrites NetworkSink). They go when those land.Left at parity and noted:
Value::resolve/to_error_instancestill run inside a bodywith_mutin FetchTasklet/html_rewriter/BodyAbortListener::on_abort(same aliasing shape as before; owned by #40202/#40203);CellRefCounted::deref_nn(NonNull)is a safe fn (pre-existing). Pre-existing:JSBakeResponse.cpppassesnullptrforbake_ssr_has_jsx, which both old and new code write through.Perf (release,
taskset -c 56-63,perf stat -e instructions:u, min of 7 vs the base built identically):new Response(x)×1e6 — string −5.2 %, ArrayBuffer −4.2 %, Blob −5.9 %;Response.json×1e6 −2.4 %;req.text()64 KB ×1e4 −0.75 %; Bun.serve static 300k req (oha) −0.6 % server instructions; 256 MB Blob stream pull ×8 within noise; barenew Request(url, {body})+0.26 % (one extra 168-byte move into the pooled slot). malloc counts identical (1 per Response(string/blob), 2 per Response(ArrayBuffer), 4 per Response.json).Testing
Debug+ASAN, per-file
--timeout 120000: response, body, body-stream (9086), body-mixin-errors, fetch.stream, blob, blob-cow, wasm-streaming, stream-fast-path, fetch-abort-stream-body, fetch-stream-cancel-leak, fetch-response-finalizer-sweep, body-stream-excess, server-response-stream-leak, request/response-cyclic-reference, streams/* (compression, native-source-onclose-leak, pipeTo-, readable-stream-, streams-string-limit, streams.test.js, sync-pull-fast-path, transform-stream-leak), direct-readable-stream, serve.test (296; root-range-port is a root-user artifact), serve-body-leak, bun-serve-static, bun-serve-html, bun-server, filesink, bun-write, node-stream*, html-rewriter (+leak, +end-error), FormData, response-to-bake-response, spawn, spawn-streaming-stdin, s3, abort, inspect — pass. fetch.test.ts 359/5 (root-permission ×4 + the known abort-timing red). Hand-driven: body-type × reader matrix, clone + tee, streaming Request into a local server, mid-pull cancel for File/Blob/bytes sources, ArrayBufferSink +Bun.write(Response(stream)), 1e4 Responses + GC (collected), Worker terminated mid-read ×5 — exit 0, no ASAN output. clippy clean on bun_runtime/bun_ptr/bun_jsc/bun_io/bun_collections/bun_core/bun_event_loop;rust-check-allwindows-msvc + aarch64-darwin pass.