blob: delete Blob::deinit, which freed the allocation through &mut self

RO
3mo ago
+116-119

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.
  • Blob is built by value in many places and ref_count is public, so a by-value Blob with a count set made a safe deinit() free a stack address, and a &mut into a JS-owned Blob freed it out from under the wrapper, BlobRefPtr or ExternalShared<Blob> holding its counts.
  • Even on the intended path (last count released) freeing through the &mut self argument is invalid: a reduction of this shape fails under Miri with deallocation through <tag> ... is forbidden ... the strongly protected tag disallows deallocations. This is why bun's other freeing teardowns take this: *mut Self.
  • No in-tree caller misused it. This is a contract fix in the same family as #37597, #37681 and #37577, split out because it needed a caller audit.

Fix

  • deinit() is deleted. Without the free it did exactly what dropping a Blob already does (Blob has no Drop impl; store ref, name and content type release themselves), so the nine by-value callers now let their Blob drop, and two custom destructors that existed only to make this call go away.
  • The one free of a Blob::new allocation is now Blob__deref releasing the last count, which destroys the *mut Blob it receives. Property to check: after this PR no safe function frees a Blob, and each converted call site releases the same things at the same point as before.
  • The structured-clone deserializer, the other caller that relied on the free, now keeps the blob by value until the record has fully parsed and heap-promotes it right before wrapping it, so every early return drops a local instead of running a raw-pointer guard.
  • Verification: the source lint's allowlist entry for this line is removed (on main it 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

  • Blob carries an intrusive ref_count. Count zero means a by-value Blob owned by whatever struct holds it (a Body, a route, a download task). Blob::new moves it to the heap with one count; JS wrappers, C++ BlobRefPtr and ExternalShared<Blob> hold further counts through Blob__ref / Blob__deref.
  • A blob's resources (store ref, name, content type) are fields with their own destructors, so ordinary Rust drop fully tears down a by-value Blob. Only a heap-promoted one needs an explicit free.
  • Freeing memory through a &mut self receiver 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 raw this: *mut Self.
  • The structured-clone deserializer reads a blob record in two halves: the store (bytes, file path, or empty), then a trailer (File flag, lastModified, name). A record cut short inside the trailer leaves a half-built blob that has to be released exactly once.
  • test/internal/source-lints/self-receiver-reclaim.test.ts is a ratchet: it scans src/ 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 with

if self.is_heap_allocated() {            // == ref_count != 0
    unsafe { drop(bun_core::heap::take(std::ptr::from_mut::<Blob>(self))) };
}

so it freed whatever allocation it happened to be called through whenever the intrusive count was non-zero. Nothing about a &mut Blob supports that:

  • Blob is constructed by value all over the tree (Blob::dupe(), AnyBlob / Body::Value::Blob payloads, stack locals), Default is public and so is ref_count, so this was an invalid free with no unsafe anywhere in the caller:

    let mut b = bun_jsc::webcore_types::Blob::default();
b.ref_count = bun_ptr::RawRefCount::init(1);
b.deinit();            // heap::take on a stack address

    and a &mut Blob into a JS-owned heap Blob (src/runtime/image/Image.rs and src/runtime/api/Archive.rs form those today) turned a safe deinit() into freeing the object out from under the wrapper, BlobRefPtr or ExternalShared<Blob> that owns its counts.

  • Even on the intended path (Blob__deref releasing the last count) freeing through the &mut self argument 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 (what bun run rust:miri uses) reports deallocation through <tag> ... is forbidden ... the strongly protected tag disallows deallocations, pointing at the &mut self receiver; Stacked Borrows reports deallocating while item [Unique] is strongly protected. This is why the tree's other teardown functions that end in a free take this: *mut Self (see the comments on deinit in src/sql_jsc/postgres/PostgresSQLConnection.rs and src/sql_jsc/mysql/JSMySQLConnection.rs, and #37681 for the same conversion on ReadBytesHandler).

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-typed deinit() call found two callers that relied on the free and nine that did not, and the nine all drop the Blob immediately after the call. Once the free is gone, deinit() is exactly what dropping a Blob already does (Blob has no Drop impl; StoreRef, OwnedStringCell and the content type Arc release themselves), so rather than keeping a method that duplicates the drop glue (Body.rs already carries three "never expose pub fn deinit(&mut self)" notes from the port), it is deleted:

  • A Blob::new allocation is freed in exactly one place: Blob__deref releasing the last count now runs heap::destroy on the *mut Blob it receives (#37597 gave it that signature) instead of re-arming the count and calling deinit(). Blob::new and Blob__ref document 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 calling deinit() to free it on a truncated record. It now keeps the blob by value until the record has fully parsed and calls Blob::new immediately before wrapping it, so every early return drops a local. Both scope guards, the unsafe reborrow and the is_heap_allocated assertion go away; the rest of the function is unchanged.
  • The by-value callers now just let their Blob drop: Body::Value's Drop arm and the two to_readable_stream scope guards (Body.rs), the S3 read_bytes_to_handler task and S3BlobDownloadTask::drop (Blob.rs), the blob: import keep-alive in jsc_hooks.rs, and ObjectURLRegistry::Entry and FileRoute, whose custom destructors existed only to make this call (FileRoute goes back to CellRefCounted's default destroy, which its docs prescribe for exactly this case). Each of these released the same things at the same point before; the only ordering change is S3BlobDownloadTask, whose blob now drops after poll_ref.unref() instead of before, and nothing in between looks at it.

ref_count staying pub is 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 replacing BlobExt::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 #37681) bans reclaiming a method's own receiver through heap::take / heap::destroy / Box::from_raw, and on main carries 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 and main's src/ the lint reports exactly

    src/jsc/webcore_types.rs:465: heap::take(std::ptr::from_mut::<Blob>(self)

    and 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 File with bytes, a Bun.file() and an empty File through bun:jsc (same record structuredClone produces), 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 through Blob__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 SSLConfig ExternalShared<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++ BlobRefPtr releasing through Blob__deref); plus test/js/bun/http/serve-body-leak.test.ts and the Sending Blob / Body.Value cases of test/js/web/fetch/fetch-leak.test.ts. Running the truncated-record sweep 20 times over under detect_leaks=1 reports no allocation from the blob or deserializer code. bun test test/internal/source-lints/ passes, and the lint fails with main's src/ as shown above. cargo clippy and cargo fmt --check on bun_jsc and bun_runtime are clean.