blob: delete Blob::deinit, which freed the allocation through &mut self - #37672
blob: delete Blob::deinit, which freed the allocation through &mut self#37672robobun wants to merge 1 commit into
Conversation
|
Warning Review limit reached
Next review available in: 22 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 (10)
Comment |
|
Updated 11:26 PM PT - Aug 11th, 2026
✅ @robobun, your commit afd8784144f90669bb1fd91f3253475577df36f7 passed in 🧪 To try this PR locally: bunx bun-pr 37672That installs a local version of the PR into your bun-37672 --bun |
|
Status: reproduced on Current revision (afd8784) deletes |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it changes the Blob free contract (an FFI-exported lifecycle path called from C++ BlobRefPtr) and composes with two other in-flight PRs (#37597, #37656), a human look would still be worthwhile.
What was reviewed:
- Confirmed
heap::destroy=drop(Box::from_raw), andBlob's field drops (StoreRef,OwnedStringCell,JsCell<BlobContentType>) release everything the olddeinit()released — the newBlob__dereffree path is equivalent. - Spot-checked the remaining
Blob::deinit()callers (jsc_hooks.rs,ObjectURLRegistry::Entry,Body::Value::Blob,FileRoute, the S3 read task) — all holdBlobby value withref_count == 0, so none relied on the removed free. - The by-value
blobin the deserializer's trailer section works via auto-ref onCell/JsCellaccessors; early-return drops release the store correctly.
Extended reasoning...
Overview
This PR removes the self-freeing branch from Blob::deinit(&mut self), moves the sole heap free to Blob__deref (via heap::destroy), and restructures on_structured_clone_deserialize to keep the Blob by value until the record has fully parsed (eliminating two scope guards, an unsafe reborrow, and a heap-allocation assertion). It adds a source lint banning heap::take/destroy/Box::from_raw on a pointer spelled from self, and a truncated-record ASAN test exercising every early return in the deserializer.
Security risks
The deserializer handles untrusted structured-clone bytes. The restructuring is a strict simplification — it removes unsafe and defers heap promotion until after all fallible reads — so the attack surface shrinks. No new parsing logic; the offset/size clamping is unchanged. Blob__deref is FFI-exported and called from C++ BlobRefPtr and ExternalShared<Blob>; the free now happens directly there instead of through deinit(), which is behaviorally equivalent (verified: JsCell<T> wraps UnsafeCell<T> so field drops run; OwnedStringCell::Drop derefs the string; StoreRef::Drop derefs the store).
Level of scrutiny
High. This is native memory-safety code in the most-blocked review category — it changes the ownership contract of a type used pervasively (Blob::dupe, AnyBlob, Body::Value, FileRoute, C++ bindings), and the correctness depends on a tree-wide caller audit. I spot-checked that audit against every .deinit() grep hit on a Blob-typed receiver and found all of them hold their blob by value, so the removed free branch was dead for them. The one remaining protector-model issue (Blob__deref still takes &mut Blob and frees through it) is explicitly deferred to #37597 and is not a regression — the free went through a protected reference before too, one call frame deeper.
Other factors
The PR interacts with #37597 (Blob__deref → *mut) and #37656 (same by-value deserializer change). The description documents the overlap precisely and the composition is a one-hunk rebase either way, but a maintainer should confirm the landing order. The new source lint adds a ratcheted allowlist (src/runtime/image/Image.rs: 1) whose entry needs a follow-up, and the lint's regex scope is clearly documented. Test coverage is solid (existing 4000-iteration round-trip test covers the new free path; new truncated-prefix sweep covers every deserializer early return under ASAN). The change looks correct, but the combination of FFI-visible lifecycle change + cross-PR composition + tree-wide audit makes this one for a human.
|
No line comments to act on from the review above. On its two notes for a human reviewer: the Image.rs allowlist entry is a placeholder for a separate change that converts |
…t self (#37681) ### What `ReadBytesHandler::on_read_bytes` (src/runtime/webcore/Blob.rs) took `&mut self`. Its only implementor, `BlobReadChain` in src/runtime/image/Image.rs, used that receiver to reclaim the Box it lives in: ```rust fn on_read_bytes(&mut self, result: ReadBytesResult) { let boxed = unsafe { bun_core::heap::take(std::ptr::from_mut::<Self>(self)) }; boxed.on_read_bytes_impl(result); // Box freed here, while `&mut self` is still on the stack } ``` This is the `Bun.Image` source path for `Bun.file()` / `Bun.s3()` / zero-length in-memory Blobs. No crash is known; it is an aliasing-model violation (latent UB), found while converting `Blob::deinit` (#37672), which has the same shape. ### Why it is wrong A reference argument is protected for the whole call under both aliasing models, and deallocating protected memory is UB even if the reference is never used again. `bun_runtime` cannot run under Miri, so here is a reduction with exactly this shape (a trait method `on_read_bytes(&mut self)` that `Box::from_raw`s its receiver, dispatched as `H::on_read_bytes(unsafe { &mut *ctx }, ..)` like the sites in `read_bytes_to_handler`), next to the shape this PR switches to: ``` # Tree Borrows (the flags `bun run rust:miri` uses) test tests::by_ptr_receiver ... ok test tests::by_ref_receiver ... error: Undefined Behavior: deallocation through <144833> at alloc47733[0x0] is forbidden = help: the allocation of the accessed tag <144833> also contains the strongly protected tag <144822> = help: the strongly protected tag <144822> disallows deallocations 18 | let boxed = unsafe { Box::from_raw(std::ptr::from_mut::<Self>(self)) }; help: the strongly protected tag <144822> was created here, in the initial state Reserved 17 | fn on_read_bytes(&mut self, bytes: Vec<u8>) { # Stacked Borrows test tests::by_ref_receiver ... error: Undefined Behavior: deallocating while item [Unique for <131080>] is strongly protected test tests::by_ptr_receiver ... ok ``` ### Fix The trait method becomes `unsafe fn on_read_bytes(this: *mut Self, result)`, the shape `ReadFileCompletion::run(ctx: *mut Self, ..)` in blob/read_file.rs already uses for the same job: `read_bytes_to_handler` passes the `ctx` it was given straight through at its four delivery sites (file completion, file cancel, S3 callback, synchronous in-memory), and the Image impl does `heap::take(this)`. The only frame on the stack when the Box is freed now holds a raw pointer, which is what both models allow. No behaviour change; the pointer value delivered is the same one as before. The handoff also asked whether `BlobReadChain::start` leaks the chain when `read_bytes_to_handler` returns `Err`. It does not: the only `Err` source is the S3 branch, and `execute_simple_s3_request` only returns `Err` when the callback itself did, i.e. after it has already delivered to (and consumed) the handler; the file branch is infallible and the in-memory branch delivers before returning. That "exactly one delivery, whatever the return value" contract is what the ownership transfer relies on, so it is now written down on `read_bytes_to_handler` instead of being implicit. ### Tests - `test/internal/source-lints/self-receiver-reclaim.test.ts` pins the shape tree-wide (`heap::take` / `heap::destroy` / `Box::from_raw` of `self` or a pointer spelled from `self`). It fails on main with `src/runtime/image/Image.rs:1385: heap::take(std::ptr::from_mut::<Self>(self)` and passes with this change. `Blob::deinit` in src/jsc/webcore_types.rs is the one other instance; it is allowlisted with a ratchet and is being converted in #37672. That PR adds the same lint file with the allowlist the other way round, so whichever of the two lands second drops the remaining allowlist entry (the ratchet test says so when it happens); the two PRs do not otherwise touch the same lines. - `test/js/bun/image/image.test.ts`: new case for an S3-backed source (local `Bun.serve` endpoint, success and `NoSuchKey`) and a zero-length in-memory slice (synchronous delivery), the two deliveries the existing `Bun.file()` test does not reach. The S3 client applies an ambient `HTTP_PROXY` regardless of `NO_PROXY` (#32045), so the test blanks the proxy variables around the S3 calls. - `test/js/web/workers/worker-refused-completion.test.ts`: new row with an image read in flight when the worker goes away, so the cancel delivery (handler told `ECANCELED`, frees itself) runs under the debug/ASAN build. Debug (ASAN) build: `image.test.ts` 94 pass, `image-adversarial.test.ts` 61 pass (the concurrent `BlobReadChain` case included), `worker-refused-completion.test.ts` 16 pass, `test/internal/source-lints/` 82 pass; `cargo clippy -p bun_runtime` clean on the touched files.
|
@robobun fix conflicts |
5bba815 to
0a4ddd9
Compare
|
Rebased onto main; #37597 and #37681 had landed under this PR. |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it changes the free path for heap-allocated Blobs (intrusive refcount teardown across the Rust/C++ FFI boundary), a human look would still be worthwhile.
What was reviewed:
- Confirmed
heap::destroy=drop(Box::from_raw), and thatBlob's field drops (StoreRef::drop→Store::deref,OwnedStringCell::drop,BlobContentType'sArc) release everythingdeinit()used to release explicitly, soBlob__deref's new one-liner is equivalent to the old re-arm-then-deinitpath. - Audited every remaining
blob.deinit()/Blob::deinitcall site (jsc_hooks, ObjectURLRegistry::Entry, read_bytes Task, S3BlobDownloadTask, FileRoute, Builtin) — all hold theirBlobby value inside a struct whose own allocation is freed separately; none relied ondeinit()freeing. - Checked the by-value deserializer: every field write in the trailer goes through
Cell/JsCell/OwnedStringCellon&self, so the immutablelet blob: Blobbinding compiles and every?early return drops it cleanly;BlobExt::to_json the freshBlob::newpointer hands the+1to the wrapper as before. - Verified the emptied lint allowlist is correct: #37681 already removed the Image.rs instance.
Extended reasoning...
Overview
This PR splits Blob::deinit(&mut self) into two responsibilities: the by-value resource reset (store ref, name, content type) stays on deinit, and the heap free moves exclusively into Blob__deref when the intrusive count hits zero. It also restructures on_structured_clone_deserialize to keep the blob by value until the record has fully parsed, removing two scopeguard closures and an unsafe reborrow. The source-lint allowlist for the self-receiver-reclaim pattern is emptied, and a new subprocess test sweeps every truncated prefix of a serialized blob record under ASAN.
Security risks
None user-facing. This is a soundness contract fix (a safe &mut self method could free its own receiver under Tree/Stacked Borrows protector rules, and ref_count being pub on a by-value-constructible type made an invalid free reachable from safe code). The change strictly narrows what deinit() can do; no new attack surface.
Level of scrutiny
High. This is core Blob lifetime management on the JSC FFI boundary — the intrusive refcount is shared with C++ BlobRefPtr and ExternalShared<Blob>, and the JS wrapper finalizer, FormData, WebSocket, static routes, and the SSLConfig cert/key fields all release through Blob__deref. A mistake here is a UAF or leak on a very hot path. I verified the equivalence of the new heap::destroy(this) to the old deinit()-with-free by tracing each owning field's Drop (JsCell<Option<StoreRef>> → StoreRef::drop → Store::deref; OwnedStringCell::drop → String::deref; BlobContentType::Owned(Arc) drops), and grepped every remaining .deinit() call on a Blob to confirm none depended on the free.
Other factors
- The PR description's note about an Image.rs allowlist entry is stale relative to the current diff: #37681 landed first (visible in the git log), so the merge-conflict resolution correctly emptied
ALLOWand updated the lint's comments/examples to reference the old shape as a regression fixture. - The deserializer change is a net simplification (two guards, one
unsafereborrow, and a debug assertion removed) and the new truncated-record test exercises every early return under ASAN. - Jarred asked for conflicts to be fixed and the current commit reflects that; there are no other outstanding reviewer comments.
- Given the memory-safety weight of the code path and the number of downstream consumers of
Blob__deref, I'm deferring rather than approving outright.
|
Nothing to act on from the second review pass (no line comments). The description note it calls stale was rewritten after the rebase: the Image.rs instance was converted by #37681 and the allowlist here is now empty; the remaining Image.rs mention in the description is the |
Blob::deinit(&mut self) released the store, name and content type and then, when the intrusive count was non-zero, freed the allocation it was called through. A reference proves nothing about who owns that allocation: Blob is built by value throughout the tree and ref_count is a pub field, so safe code could make deinit() free a stack address, and any &mut Blob into a wrapper-owned heap Blob let a safe deinit() free it out from under the wrapper. Freeing through the &mut self argument is also a deallocation of protected memory under Stacked and Tree Borrows even when the pointer is the right one. Without the free, deinit() was exactly what dropping a Blob already does (it has no Drop impl; StoreRef, OwnedStringCell and the content type Arc release themselves), and every remaining caller dropped the Blob right after calling it, so the method is removed rather than kept as a duplicate of the drop glue: the Body::Value, ObjectURLRegistry Entry, FileRoute, S3 download task and blob: import sites now just drop their Blob (FileRoute and Entry lose their custom destructors altogether). The one place that frees a Blob::new allocation is Blob__deref releasing the last count, which drops the box through the pointer it receives. The structured clone deserializer, the other caller that relied on deinit() freeing, keeps the blob by value until the record has fully parsed and heap-promotes it right before wrapping it, so its early returns drop a local instead of going through a raw-pointer guard. The self-receiver-reclaim source lint allowlisted this one remaining instance; the entry is removed, so the tree is at zero. A second test deserializes every truncated prefix of a serialized Blob record for each store kind so the deserializer's early returns run under the ASAN build.
0a4ddd9 to
afd8784
Compare
robobun
changed the title
|
Revised in afd8784 after a self-review pass: with the free gone, |
### Problem
The JSSink finalize chain frees the sink while reference arguments to it
are still live. At `3fc747a7da`:
* generated thunk `extern "C" fn ${name}__finalize(this: &mut ${name})`
(src/codegen/generate-jssink.ts), called from `~JS${name}`,
`~JSReadable${name}Controller` and `${name}__doClose`
* `JSSink::js_finalize(this: &mut T)` (src/runtime/webcore/Sink.rs)
* `JsSinkType::finalize(&mut self)` (src/runtime/webcore/Sink.rs), whose
impls do the actual release:
* `ArrayBufferSink` (src/runtime/webcore/ArrayBufferSink.rs):
`Self::finalize(ptr::from_mut(self))` -> `destroy` -> `heap::take`,
unconditionally. The comment on the impl said the C export owned the
free; this call is the free.
* `FileSink` (src/runtime/webcore/FileSink.rs): the inherent
`finalize(&mut self)` ends in `FileSink::deref(ptr::from_mut(self))`,
which runs `deinit` -> `heap::take` whenever the wrapper's +1 was the
last ref, i.e. on an ordinary GC sweep of a sink nothing else holds. The
header comment argued this was fine because the `&mut` carries write
provenance, which is true but is not the problem.
* `FetchRequestBodySink`
(src/runtime/webcore/fetch/FetchRequestBodySink.rs): drops the tasklet
ref taken in `start_request_stream`. The tasklet owns the sink
allocation, so if that ref is the last one, `FetchTasklet::deinit` ->
`clear_data` -> `clear_sink` -> `heap::take(sink)` frees `*self` inside
the call. That is the fallback path for a pump that never settled; it is
reachable at least on worker teardown: phase B of `VirtualMachine`
teardown releases the aborted fetch's other refs on the tasklet, and
phase C then destroys the heap, sweeping the controller with `m_sinkPtr`
still set because `JSSinkController__onClose` does not run the detaching
JS callback once termination is pending.
* `HTTPServerWritable`, `NetworkSink` and `RewriterPipe` do not free
anything here (their allocations are owned by the `RequestContext`, the
S3 wrapper and the pipe's own refcount respectively).
A reference passed as an argument has to stay dereferenceable until the
call returns. Freeing it from inside the call is undefined behaviour
under both aliasing models whether or not the reference is used again
(Stacked Borrows: `deallocating while item is strongly protected`; Tree
Borrows, which `bun run rust:miri` uses, rejects it the same way), and
that protector is the model behind the `dereferenceable` attribute rustc
puts on every `&`/`&mut` argument, so the optimizer may legitimately
move a load through any of the three frames past the free. No crash is
known from this; ASAN only has something to catch if the optimizer
actually takes that liberty, which the unoptimized debug build never
does, so it is not observable as a runtime test. Same family as #37672,
#37681, #37685, #37693, #37705 and #37551; #37705's description leaves
this chain out explicitly because it needs a change to the generated
thunk.
### Fix
The whole chain takes the raw pointer, which is what the C++ side has
anyway (`void* m_sinkPtr`):
* generate-jssink.ts emits `pub unsafe extern "C" fn
${name}__finalize(this: *mut ${name})` forwarding to `js_finalize`; the
ABI is unchanged, so JSSink.cpp is untouched.
* `JSSink::js_finalize(this: *mut T)` forwards to the trait.
* `JsSinkType::finalize` becomes `unsafe fn finalize(this: *mut Self)`,
documented as "the cell is giving up its claim; this may free the sink",
the same shape as `HTTPServerWritable::abort(this: *mut Self)` and the
FileSink PipeWriter callbacks.
* The three freeing impls release through the pointer without forming a
reference to the allocation: `ArrayBufferSink` calls `destroy` directly
(the inherent `finalize` wrapper, whose only caller was the trait impl,
is deleted); `FileSink::finalize(this: *mut FileSink)` keeps the same
body with per-statement `(*this).field` access, like `on_close` in the
same file (the file header no longer claims the `&mut` version was
sound; the rationale lives once, on the trait method);
`FetchRequestBodySink::finalize(this: *mut Self)` takes `task` out
through the pointer and does not touch it after the deref.
* `HTTPServerWritable` and `NetworkSink` reborrow inside their own impl
to call the unchanged inherent `finalize(&mut self)`; that borrow ends
before the impl returns and nothing under it frees, which the SAFETY
comments state. `RewriterPipe`'s impl stays empty.
Every impl performs the same operations in the same order as before; the
only thing that moves is the type the pointer travels as.
`js_controller_detached`, `js_close` and `js_end_with_sink` still take
`&mut`: nothing frees under them (the `controller_detached` contract on
the trait already requires deferring a last-owner free for that reason).
`FileSink::assign_to_stream`'s `FileSinkRef` guard also derefs from a
`&mut self` frame, but its ref is balanced against one it took itself
and every caller (subprocess stdin setup) holds its own ref across the
call, so it can never be the one that frees; left alone. Sites with the
same shape outside this chain
(`S3UploadStreamWrapper::handle_{resolve,reject}_stream`,
`FetchTasklet::write_end_request`) are not sink frames and are reported
separately.
### Tests
test/internal/source-lints/jssink-finalize-raw-ptr.test.ts scans every
`impl ... JsSinkType for ...` block for a `finalize` item and requires
`unsafe fn finalize(<ident>: *mut Self)`, checks the other frames by
signature (trait declaration, `js_finalize`, the codegen template, and
the three inherent methods that perform the free, which `pub` tells
apart from the trait impls in the same files), and checks its own
patterns against positive and negative spellings. With src/ restored to
`main` it reports:
```
src/runtime/api/html_rewriter.rs:1650: impl JsSinkType for RewriterPipe: fn finalize(&mut self) (line 1661)
src/runtime/webcore/ArrayBufferSink.rs:213: impl JsSinkType for ArrayBufferSink: fn finalize(&mut self) (line 221)
src/runtime/webcore/fetch/FetchRequestBodySink.rs:274: impl JsSinkType for FetchRequestBodySink: fn finalize(&mut self) (line 281)
src/runtime/webcore/FileSink.rs:1283: impl JsSinkType for FileSink: fn finalize(&mut self) (line 1294)
src/runtime/webcore/streams.rs:2104: impl JsSinkType for HTTPServerWritable: fn finalize(&mut self) (line 2119)
src/runtime/webcore/streams.rs:2523: impl JsSinkType for NetworkSink: fn finalize(&mut self) (line 2530)
src/runtime/webcore/Sink.rs: JsSinkType::finalize declaration does not take the sink as `*mut`
src/runtime/webcore/Sink.rs: JSSink::js_finalize does not take the sink as `*mut`
src/codegen/generate-jssink.ts: generated `${name}__finalize` thunk does not take the sink as `*mut`
src/runtime/webcore/FileSink.rs: FileSink::finalize does not take the sink as `*mut`
src/runtime/webcore/fetch/FetchRequestBodySink.rs: FetchRequestBodySink::finalize does not take the sink as `*mut`
```
(`ArrayBufferSink::destroy` already took `*mut` on `main`; its entry is
a ratchet.)
The behaviour itself is the existing coverage of each finalize path; see
below.
### Verification
Debug (ASAN) build on Linux: `cargo clippy -p bun_runtime` and `rustfmt
--check` on the touched files are clean; the generated thunks have the
new signature. Passing: test/internal/source-lints/ (all 18 files),
test/js/bun/util/arraybuffersink.test.ts and filesink.test.ts (wrapper
sweep and prototype `.close()` for the two Box/refcount sinks),
test/js/bun/spawn/spawn.test.ts (stdin `FileSink` via
`assign_to_stream`), test/js/web/fetch/body-stream.test.ts,
fetch-abort-stream-body.test.ts and fetch-stream-cancel-leak.test.ts
(`FetchRequestBodySink`),
test/js/bun/http/serve-response-stream-sink-leak,
serve-direct-readable-stream, serve-stream-reject-flush-leak and
serve-async-stream-client-abort (`HTTPServerWritable` controller
teardown), test/js/web/fetch/server-response-stream-leak.test.ts,
test/js/web/streams/streams.test.js,
test/js/workerd/html-rewriter.test.js and html-rewriter-leak.test.ts
(`RewriterPipe`), test/js/bun/s3/s3-stream-error-gc.test.ts and
s3-argument-validation.test.ts. The S3 upload tests that would drive
`NetworkSink` (s3.test.ts, s3-storage-class.test.ts) cannot connect from
this environment and fail identically on the released binary, so that
impl (a one-line forward to the unchanged inherent method) is left to
CI.
Overlap with the sibling lints, each of which documents these sites as
tracked separately: #37685 / #37693 / #37705 add
`self-receiver-teardown.test.ts` with
`src/runtime/webcore/ArrayBufferSink.rs: 1` allowlisted for the
`Self::finalize(ptr::from_mut(self))` line this PR removes, and #37703
adds `self-receiver-release.test.ts` with
`src/runtime/webcore/FileSink.rs: 2` allowlisted for the two derefs
inside the old `FileSink::finalize(&mut self)` (running that lint
against this branch reports FileSink.rs at 0). Whichever side lands
second deletes the entry; nothing else conflicts (#37703's
FetchRequestBodySink.rs hunk is `end_from_stream`, a different
function). #34999 and #35528 edit the body of `FileSink::finalize`
textually but keep the receiver.
---------
Co-authored-by: Jarred Sumner <jarred@jarredsumner.com>
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Since it reworks Blob teardown/refcounting across the finalizer, structured-clone deserializer, and several by-value holders (Body::Value, FileRoute, ObjectURLRegistry::Entry, the S3 task, transpile_file), a human look would still be worthwhile.
What was reviewed:
- Confirmed
heap::destroyand theCellRefCounteddefault destroy both rundrop(Box::from_raw), and thatBlob's field drops (StoreRef::Drop→Store::deref,OwnedStringCell::Drop,BlobContentType'sArc) release exactly what the olddeinit()released. - Traced each removed
deinit()call to the equivalent natural drop:FileRoute's derive default,Value::Drop's payload drop glue, thetranspile_filelocal (now dropped afterlr, which is strictly safer),Task::done'sdrop(self), and the deserializer's by-valueblobon every?early return. - Checked no
Blob::deinitcallers remain insrc/and that the lint allowlist ratchet now sits at zero.
Extended reasoning...
Overview
Deletes Blob::deinit(&mut self), which conditionally freed its own receiver via heap::take(ptr::from_mut(self)) — UB under both Miri borrow models and reachable from safe code because ref_count is a public field on a by-value-constructible type. The single legitimate free now happens in Blob__deref via heap::destroy(this: *mut Blob). Nine call sites that previously called deinit() on by-value blobs are updated to rely on Rust's field drop glue instead. on_structured_clone_deserialize is restructured to keep the blob by-value until the record is fully parsed, eliminating two scopeguards, an unsafe reborrow, and a debug assertion. Two source-lint tests and one runtime test are updated/added.
Security risks
None introduced. The change removes an unsound free-through-reference and replaces manual teardown with compiler-generated drop glue over the same fields. The truncated-record deserializer path is strictly simpler (local drop vs. raw-pointer scopeguard). No new attack surface.
Level of scrutiny
High — this is intrusive-refcount and GC-finalizer code for Blob, which backs fetch bodies, structured clone, Bun.serve file routes, the object-URL registry, S3, and the shell. REVIEW.md flags native memory safety as the most-blocked category. That said, the change is subtractive: it deletes a hand-written teardown in favour of drop glue whose field-by-field behaviour I verified against the old deinit() body (detach() → StoreRef::Drop, name.set(dead) → OwnedStringCell::Drop, content_type.set(default) → Arc drop).
Other factors
I confirmed bun_core::heap::destroy is drop(Box::from_raw) and that #[derive(CellRefCounted)] without destroy = … emits the same, so removing FileRoute::deinit is equivalent. JsCell<T> is transparent over UnsafeCell<T> so its inner value drops. The transpile_file change also fixes a minor drop-order quirk: the blob now outlives lr (which borrows into its store) at scope exit, whereas the old _blob_guard was declared after lr and so dropped first. The new truncated-record test walks every proper prefix of three record shapes under ASAN, and the ratcheted source-lint allowlist is now empty so the pattern cannot reappear. A maintainer has already engaged (rebase request) but not yet signed off on the substance, and #37656 has a known one-hunk overlap; both argue for a human pass before merge.
|
Nothing to act on from this pass either (no line comments). On the drop-order note for transpile_file: agreed it is the benign direction, and the blob is only ever read through the erased borrows during the synchronous transpile, so neither order could have been observed; the comment at that site says why the local is still there. CI for afd8784 is in progress (build 92908: 173 jobs passed, 17 running, none failed at the time of writing). |
2 participants
Problem
Blob::deinit(&mut self)released the blob's resources and then, whenever the intrusive count was non-zero, freed the allocation it had been called through. A safe method call could therefore free memory.Blobis built by value in many places andref_countis public, so a by-valueBlobwith a count set made a safedeinit()free a stack address, and a&mutinto a JS-ownedBlobfreed it out from under the wrapper,BlobRefPtrorExternalShared<Blob>holding its counts.&mut selfargument is invalid: a reduction of this shape fails under Miri withdeallocation through <tag> ... is forbidden ... the strongly protected tag disallows deallocations. This is why bun's other freeing teardowns takethis: *mut Self.Fix
deinit()is deleted. Without the free it did exactly what dropping aBlobalready does (Blobhas noDropimpl; store ref, name and content type release themselves), so the nine by-value callers now let theirBlobdrop, and two custom destructors that existed only to make this call go away.Blob::newallocation is nowBlob__derefreleasing the last count, which destroys the*mut Blobit receives. Property to check: after this PR no safe function frees aBlob, and each converted call site releases the same things at the same point as before.mainit reports exactly this line, with the fix zero); a new test deserializes every truncated prefix of three record kinds under the ASAN build, also run 20 times under leak detection; the existing suites for each converted drop site pass on the ASAN build.Background
Blobcarries an intrusiveref_count. Count zero means a by-valueBlobowned by whatever struct holds it (aBody, a route, a download task).Blob::newmoves it to the heap with one count; JS wrappers, C++BlobRefPtrandExternalShared<Blob>hold further counts throughBlob__ref/Blob__deref.Blob. Only a heap-promoted one needs an explicit free.&mut selfreceiver is undefined behaviour even when the caller holds the last count, because the reference protects the memory for the duration of the call. The tree's convention for teardowns that free is a rawthis: *mut Self.test/internal/source-lints/self-receiver-reclaim.test.tsis a ratchet: it scanssrc/for reclaiming a method's own receiver and its allowlist holds an exact count per file, so removing the last offender also requires removing its entry.Original description
Problem
Blob::deinit(&mut self)(src/jsc/webcore_types.rs) released the store ref, the name and the content type, and then ended withso it freed whatever allocation it happened to be called through whenever the intrusive count was non-zero. Nothing about a
&mut Blobsupports that:Blobis constructed by value all over the tree (Blob::dupe(),AnyBlob/Body::Value::Blobpayloads, stack locals),Defaultis public and so isref_count, so this was an invalid free with nounsafeanywhere in the caller:and a
&mut Blobinto a JS-owned heapBlob(src/runtime/image/Image.rs and src/runtime/api/Archive.rs form those today) turned a safedeinit()into freeing the object out from under the wrapper,BlobRefPtrorExternalShared<Blob>that owns its counts.Even on the intended path (
Blob__derefreleasing the last count) freeing through the&mut selfargument deallocates memory that a reference argument protects for the duration of the call. A standalone reduction of exactly this shape fails under Miri with both models: Tree Borrows (whatbun run rust:miriuses) reportsdeallocation through <tag> ... is forbidden ... the strongly protected tag disallows deallocations, pointing at the&mut selfreceiver; Stacked Borrows reportsdeallocating while item [Unique] is strongly protected. This is why the tree's other teardown functions that end in a free takethis: *mut Self(see the comments ondeinitin src/sql_jsc/postgres/PostgresSQLConnection.rs and src/sql_jsc/mysql/JSMySQLConnection.rs, and blob: hand the read handler's pointer to on_read_bytes instead of &mut self #37681 for the same conversion onReadBytesHandler).No in-tree caller misused it; this is a contract fix in the same family as #37597 (
Blob__ref/Blob__deref), #37681 and #37577, split out of #37597 because it needed a caller audit.Fix
The audit of every
Blob-typeddeinit()call found two callers that relied on the free and nine that did not, and the nine all drop theBlobimmediately after the call. Once the free is gone,deinit()is exactly what dropping aBlobalready does (Blobhas noDropimpl;StoreRef,OwnedStringCelland the content typeArcrelease themselves), so rather than keeping a method that duplicates the drop glue (Body.rs already carries three "never exposepub fn deinit(&mut self)" notes from the port), it is deleted:Blob::newallocation is freed in exactly one place:Blob__derefreleasing the last count now runsheap::destroyon the*mut Blobit receives (blob: make the Blob__ref/Blob__deref refcount exports unsafe fns #37597 gave it that signature) instead of re-arming the count and callingdeinit().Blob::newandBlob__refdocument that nothing else frees it.on_structured_clone_deserialize(src/runtime/webcore/Blob.rs), the other caller that relied on the free, heap-promoted the blob before reading the record's trailer and used a raw-pointer scope guard callingdeinit()to free it on a truncated record. It now keeps the blob by value until the record has fully parsed and callsBlob::newimmediately before wrapping it, so every early return drops a local. Both scope guards, theunsafereborrow and theis_heap_allocatedassertion go away; the rest of the function is unchanged.Blobdrop:Body::Value'sDroparm and the twoto_readable_streamscope guards (Body.rs), the S3read_bytes_to_handlertask andS3BlobDownloadTask::drop(Blob.rs), theblob:import keep-alive in jsc_hooks.rs, andObjectURLRegistry::EntryandFileRoute, whose custom destructors existed only to make this call (FileRoutegoes back toCellRefCounted's defaultdestroy, which its docs prescribe for exactly this case). Each of these released the same things at the same point before; the only ordering change isS3BlobDownloadTask, whose blob now drops afterpoll_ref.unref()instead of before, and nothing in between looks at it.ref_countstayingpubis now only a hygiene question (one struct literal in Blob.rs needs it): no safe function's free depends on it any more, so it is left alone here. #37656 (open) makes the same by-value change to the deserializer as part of replacingBlobExt::to_js; the arms here are written to match it, and whichever lands second drops its copy of that hunk.Tests
test/internal/source-lints/self-receiver-reclaim.test.ts (landed by blob: hand the read handler's pointer to on_read_bytes instead of &mut self #37681) bans reclaiming a method's own receiver through
heap::take/heap::destroy/Box::from_raw, and onmaincarries a ratcheted allowlist entry of exactly 1 for src/jsc/webcore_types.rs naming this PR. This PR deletes the entry, leaving the allowlist empty. With this test change andmain'ssrc/the lint reports exactlyand with the fix the tree is at zero (the ratchet test would also fail if the entry were left in place once the line is gone). The rationale sentence in unsafe-refcount-exports.test.ts that described the old
deinit()behaviour is updated to match.test/js/web/fetch/blob-file-name-ownership.test.ts gets a second test for the restructured deserializer: it serializes a
Filewith bytes, aBun.file()and an emptyFilethroughbun:jsc(same recordstructuredCloneproduces), deserializes every truncated prefix of each (which reaches every early return in the deserializer, including the trailer ones that previously went through the heap guard), and checks the full record still round-trips with an empty stderr, so a double free shows up under the ASAN build. The existing test in that file (4000 structuredClone round-trips, each finalized throughBlob__deref) covers the new free path.Verification
On the debug (ASAN) build: the two test files above; for the converted drop sites, test/js/web/fetch/{body,body-stream,response}.test.ts and test/js/web/fetch/wpt/textstream-wpt.test.ts (Body), test/js/node/buffer-resolveObjectURL.test.ts, test/js/web/workers/worker_blob.test.ts and test/js/web/url/url.test.ts (object URL entries and
blob:imports), test/js/bun/http/{bun-serve-file,bun-serve-static,serve-file-slice-read-error,tls-bunfile-leak}.test.ts (FileRoute,StaticRoute, the SSLConfigExternalShared<Blob>fields), test/js/bun/s3/{s3-requester-pays,s3-connection-close,s3-insecure,s3-stream-error-gc}.test.ts against their local servers (S3BlobDownloadTask), test/js/bun/image/image.test.ts (the S3/file read task), and test/js/web/fetch/blob.test.ts, test/js/web/structured-clone-blob-file.test.ts, test/js/web/html/FormData.test.ts, test/js/web/websocket/websocket-blob.test.ts (C++BlobRefPtrreleasing throughBlob__deref); plus test/js/bun/http/serve-body-leak.test.ts and theSending Blob/Body.Valuecases of test/js/web/fetch/fetch-leak.test.ts. Running the truncated-record sweep 20 times over underdetect_leaks=1reports no allocation from the blob or deserializer code.bun test test/internal/source-lints/passes, and the lint fails withmain'ssrc/as shown above.cargo clippyandcargo fmt --checkonbun_jscandbun_runtimeare clean.