Blob: keep File and FormData entry names on the Blob instead of the shared store by robobun · Pull Request #37680 · oven-sh/bun

robobun · 2026-08-12T00:16:17Z

Problem

new File([a], "b.txt") renames a as well: a.name becomes "b.txt" (Node and browsers keep "a.txt"). fd.append("f", blob, "entry.txt") likewise names the caller's blob, and a zero-byte blob appended with a filename reports no name at all.
Serializing a FormData after one of its files was wrapped in new File([file], other) reads freed memory. A debug build reports AddressSanitizer: heap-use-after-free in FormDataContext::on_entry, freed by jsdom_file_construct.
Cause: new File([blob], name) shares the source's byte store, and the byte-backed constructor and FormData entry naming both wrote the name into that shared store. That renamed every Blob on the store and freed the old name buffer FormData still borrowed. File- and S3-backed Files already kept the name on the Blob.
Second, pre-existing bug, reachable for every File once names live on the Blob: object URLs shared one name string across threads. If one thread had used it as a property key and another released it last, bun aborted with ASSERTION FAILED: wasRemoved (AtomStringImpl.cpp:462). On 1.4.0 this needed a file-backed File.

Fix

The File constructor and FormData entry naming set the per-Blob name for every kind of blob; a store's name is written only when the store is created and is just the fallback. Naming one Blob therefore never changes another, and nothing rewrites a live store's name, so the borrowed FormData view stays valid.
Three readers of the store's name (FormData's default filename, the loader for blob: imports, the Content-Disposition filename Bun.serve adds) now use the .name lookup, so a File wrapping a Bun.file() goes by its display name everywhere and a plain Bun.file() still uses its path. The resulting behavior changes are listed in the original below.
Registering an object URL and resolving it each take a thread-safe copy of the name, so no thread holds a name string that another thread's JS can atomize.
Verification: 25 new tests. 23 fail on 1.4.0 and on a debug build of main (the file-backed object URL case by aborting), the byte-backed object URL case aborts with only the first part of the change applied, and all pass with the change. The use-after-free test runs in a spawned process so ASAN fails the test rather than the run.

Background

A Blob is a window (offset, size) onto a refcounted store holding bytes, a file path, or an S3 key. new File([blob], name) and blob.slice() share the source's store on purpose; only the Blob-level fields differ.
A name can live in two places: a per-Blob field, or a byte copy inside a byte store (set for standalone executables' embedded files and structured clone). The .name getter prefers the per-Blob field, so the store's name is only a fallback.
FormData holds its own dupe of each blob entry. Reading an entry back into JS stamps the entry's filename onto that dupe; an entry appended without a filename defaults to the blob's name, which the C++ side takes as an uncopied view of the name bytes.
WTF strings are atomized in place when a thread uses one as a property key; the atom belongs to that thread's table, and releasing its last reference from another thread is a release assertion. URL.createObjectURL hands the registered blob to whichever thread resolves the URL, workers included.

Original description

Repro

const a = new File(["xyz"], "a.txt");
const b = new File([a], "b.txt");
console.log(a.name, b.name);        // bun 1.4.0: "b.txt b.txt"   node/browsers: "a.txt b.txt"

const blob = new Blob(["xyz"]);
new File([blob], "c.txt");
console.log(blob.name);             // bun 1.4.0: "c.txt"         expected: undefined

const fd = new FormData();
fd.append("f", blob, "entry.txt");
fd.get("f");
console.log(blob.name);             // bun 1.4.0: "entry.txt"     expected: undefined

fd.set("g", new Blob([]), "empty.txt");
console.log(fd.get("g").name);      // bun 1.4.0: undefined       expected: "empty.txt"

The rename also reaches memory that is still in use. FormData.append(name, file) takes the default filename from Blob__getFileNameString, a borrowed view of the store's name bytes that the C++ side wraps without copying. A later new File([file], other) freed those bytes, so serializing the FormData read freed memory. Released build:

const file = new File(["xyz"], "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.txt");
const fd = new FormData();
fd.append("f", file);
new File([file], "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb.txt");
(await new Response(fd).text()).match(/filename="([^"]*)"/)[1];
// '\^@\^@\^@\^@\^@\^@\^@\^@aaaaaaaaaaaaaaaaaaaaaaaa.txt'

Debug build of main on the same script:

ERROR: AddressSanitizer: heap-use-after-free
READ of size 64
    #7 <bun_runtime::webcore::blob::FormDataContext>::on_entry src/runtime/webcore/Blob.rs:3897
freed by thread T0 here:
    #9 <alloc::boxed::Box<[u8]> as core::ops::drop::Drop>::drop
    #11 bun_runtime::webcore::blob::jsdom_file_construct src/runtime/webcore/Blob.rs:5551

Cause

A Blob is a window (store, offset, size) onto a refcounted Store, and new File([blob], name) shares the part's store rather than copying it. jsdom_file_construct stored the new name in store::Bytes::stored_name, i.e. on the object shared with the source, so every Blob viewing that store was renamed and the previous name buffer was freed under any FormData entry still pointing at it. Blob__setAsFile (which attaches a FormData entry's filename when the entry is converted to JS) wrote into the shared store the same way, and dropped the name when the blob had no store, which is the case for every zero-byte blob.

The store already has no say in a File's name for file- and S3-backed Files: that arm of the constructor set the per-Blob name field, and get_name_string() (the .name getter) prefers that field over the store's name. The byte-backed arm was the odd one out.

Fix

jsdom_file_construct sets blob.name for every kind of bits, and no longer creates an empty store whose only purpose was to carry the name of an empty File. Names are now kept as a WTF string instead of a UTF-8 copy, so the constructor converts them as a USVString explicitly; the UTF-8 copy used to do that implicitly for byte-backed Files, and file-backed Files (new File([Bun.file(p)], "a\uD800b")) did not get it at all.
Blob__setAsFile sets the per-Blob name on the entry's own dupe. The store is not touched, and a storeless (zero-byte) blob keeps its name too.
Readers that want the name for something other than the getter now use the same lookup the getter uses: Blob__getFileNameString (FormData's default filename), get_loader (blob: imports pick the loader from the File's name), and the Content-Disposition filename Bun.serve adds to new Response(file). Without this, those three would stop seeing File names once the constructor no longer writes them into the store. BlobExt::get_name_utf8 is the shared helper; AnyBlob::get_file_name becomes get_name_utf8 on top of it. It does not go through the getter's caching: a blob without a name of its own (a Bun.file() response body) still borrows the store's path exactly as before, so serving files does not gain an allocation; a File's own ASCII name is a ref-holding view, also allocation free.
store::Bytes::init_empty_with_name loses its last caller and is removed. stored_name is now only written when a store is created (standalone executables' embedded files, structured clone) and serves as the fallback for blobs with no name of their own.

The two blob: module-loading sites in jsc_hooks.rs and bundler/options.rs keep using the store-level get_file_name() on purpose: they use it as the path to read when the blob is a Bun.file(), which must stay the on-disk path even when the File was given a different display name.

Why this is the right place rather than copying the bytes on new File([blob]): sharing the store is deliberate (and #36001 shares even more), and every consumer of the name already had a per-Blob field to read; the bug was the one writer that bypassed it. It also removes the only code that reassigns a live store's stored_name, which is what made the borrowed Blob__getFileNameString view unsound.

Object URLs and other threads

Keeping the name on the Blob as a WTF::StringImpl (instead of a byte copy in the store) exposed a second pre-existing bug, so this PR also fixes it. ObjectURLRegistry stored a dupe() of the registered blob and handed dupe()s of that entry to whichever thread resolved the URL, so all of them shared one name impl. A string that a thread has used as a property key is an atom of that thread's string table (WTF atomizes the impl in place), and releasing the last reference to it from another thread hits RELEASE_ASSERT in AtomStringImpl::remove. On 1.4.0 this was reachable with a file-backed File, whose name already lived on the Blob:

let name = "report-" + id;  ({})[name];             // now an atom of this thread
const url = URL.createObjectURL(new File([Bun.file(p)], name));
// worker: held = require("node:buffer").resolveObjectURL(url)
// main: URL.revokeObjectURL(url), drop the File and the name, Bun.gc(true)
// worker: held = undefined; Bun.gc(true)
// => ASSERTION FAILED: wasRemoved  (AtomStringImpl.cpp:462), "panic: abort() called" on the release build

With this change the same would have applied to every File, so Entry::init and resolve_and_dupe now both go through dupe_with_private_name, which to_thread_safe()s (copies) the name: the entry's impl is never reachable from any thread's JS, and each resolving thread gets its own. Copying on both sides is what makes it safe; a plain impl handed to a worker could still be atomized there and then released last by the registering thread. #31487 does the same for name as part of a larger change that also snapshots the store contents; this is just the part this PR needs.

Behavior changes besides the bug itself. The first four match Node; the rest are the places where Bun used to consult the store's name and now consult the same name .name reports, so a File wrapping a Bun.file(), or a blob named through Bun's .name setter, is treated according to that name everywhere. Each has a test:

new File(bits, "").name is "" (was undefined).
fd.append(name, blob, filename) / fd.set(...) no longer name the blob that was passed in.
A zero-byte blob appended with a filename, or parsed from a zero-byte multipart part, reports that filename (was undefined). This is the bug FormData: preserve filename when parsing a zero-byte multipart file part #36435 fixes by creating a named store in Blob__setAsFile; with this change that store is not needed.
fd.append(name, file, filename) / set on a File that already has a name: get() now returns a File named filename, as the multipart body already said (it returned the File's original name; this is FormData: honor filename override in append/set when value is already a File #35079's bug).
FormData's default filename for new File([Bun.file(p)], "display.bin") is display.bin in the multipart body as well (the body used to carry the full path of p while get().name said display.bin), and a blob given a name through the .name setter gets that as its default filename (the body used to carry filename=""). A plain Bun.file(p) entry still defaults to p as before (FormData: send only basename, not full path, as Bun.file() multipart filename #35523 wants that to become the basename; it would apply on top of the new lookup).
Bun.serve responding with new File([Bun.file(p)], "display.zip") now advertises filename="display.zip" instead of the basename of p; a plain Bun.file(p) body still advertises the basename of p.
A blob: import or Worker of new File([Bun.file("x.ts")], "x.txt") now picks the loader from x.txt (text) while still reading the bytes from x.ts; it used to pick it from the path. A plain Bun.file() object URL is unchanged. The two module-loading sites that need the on-disk path keep reading it from the store and now say so.

Not observable from tests here but following from the same rule: a standalone executable's embedded files keep their names when wrapped in a new File() (the wrapper used to rename the embedded blob's store).

Unchanged here and left to their own PRs: a single-Blob part still hands its name/is_jsdom_file to the wrapper via dupe() (#33603), and file.slice() still reports the File's name (#33604). This change is what makes the store stop leaking names into those wrappers once they stop copying metadata. #35079 (Blob__setAsFile honoring a filename override) and #35523 (Blob__getFileNameString returning a basename for Bun.file) touch the same two functions; see the list above for how each case comes out here.

Verification

New tests in test/js/web/fetch/blob.test.ts (13 under "new File([blob], name) names only the new File", plus 2 under "a File's name reaches other threads through an object URL as a private copy") and test/js/web/html/FormData.test.ts (10, under "entry filenames belong to the entry ..."). They cover: File, empty File, Blob, slice, and chained sources; the source's name being read before wrapping (the dupe()d name used to win); structuredClone; the empty name; USVString conversion; the blob: loader and Content-Disposition, each for byte-backed Files, for a File wrapping a Bun.file(), and (to pin the unchanged fallback) for a plain Bun.file(); the freed FormData filename (in a spawned process, so ASAN fails the test rather than the run); the object URL scenario above for a byte-backed and a file-backed File (spawned, handshaking with the worker over postMessage so the worker provably holds the last reference); and on the FormData side append/set with a filename on a Blob and on an already named File, zero-byte blobs (appended and parsed), default filenames of wrapped Files, of a File wrapping a Bun.file() and of a setter-named blob in both get() and the multipart body, and wrapping a parsed entry.

Of the 25, 23 fail on 1.4.0 and on a debug build of main (the file-backed object URL case by aborting); the byte-backed object URL case passes there and aborts with only the first part of this change applied, and the "wrapping an empty Blob" row passes either way and is there for the matrix. All pass with the change. blob.test.ts, FormData.test.ts, FormData-multipart-serialization.test.ts, blob-file-name-ownership.test.ts, blob-array-fast-path.test.ts, structured-clone-blob-file.test.ts, structuredClone-classes.test.ts, globals.test.js, worker_blob.test.ts, buffer-resolveObjectURL.test.ts, body.test.ts, response.test.ts, bun-serve-file.test.ts, fetch-file-upload.test.ts and node's test-blob-createobjecturl.js pass on the debug build (response.test.ts's "handle stack overflow" times out there on main as well, without this change).

…ed store new File([blob], name) shares the source blob's byte store and wrote the name into it, renaming the source (and anything else sharing the store). It also freed the previous name under FormData entries, which hold a zero-copy view of the name they read at append() time. Blob__setAsFile wrote FormData entry filenames into the same shared store, and lost them entirely for zero-byte blobs, which have no store. The File constructor and Blob__setAsFile now set the per-Blob name. The consumers that read a name for something other than the .name getter (FormData's default filename, the blob: loader, Bun.serve's Content-Disposition) go through the same per-Blob-first lookup the getter uses. The store's stored_name is only written when a store is created. The File constructor also converts the name as a USVString, which the old UTF-8 copy did implicitly for byte-backed Files but not for file-backed ones.

coderabbitai · 2026-08-12T00:16:24Z

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 7 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: bff4dc4f-42de-4ef5-9e38-c7f48f220447

📥 Commits

Reviewing files that changed from the base of the PR and between b49c971 and 0aa2f22.

📒 Files selected for processing (7)

src/jsc/webcore_types.rs
src/runtime/jsc_hooks.rs
src/runtime/server/RequestContext.rs
src/runtime/webcore/Blob.rs
src/runtime/webcore/ObjectURLRegistry.rs
test/js/web/fetch/blob.test.ts
test/js/web/html/FormData.test.ts

_{Comment @coderabbitai help to get the list of available commands.}

robobun · 2026-08-12T00:16:30Z

^{Updated 5:56 AM PT - Aug 12th, 2026}

✅ @robobun, your commit 0aa2f22e1afc18b7eb151a16a8b5c22b59d1c349 passed in Build #93161! 🎉

🧪 To try this PR locally:

bunx bun-pr 37680

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

bun-37680 --bun

robobun · 2026-08-12T00:16:50Z

Status: ready for review. Reproduced on 1.4.0 and on a debug build of main with the scripts in the description: the FormData one is a heap-use-after-free under ASAN (FormDataContext::on_entry reading a name freed by jsdom_file_construct), the object URL one aborts in AtomStringImpl::remove (fixed here too, see "Object URLs and other threads"). 25 tests in test/js/web/fetch/blob.test.ts and test/js/web/html/FormData.test.ts pass with this branch and 23 of them fail on main (details under Verification); every intentional behavior change in the description has a test. CI is green on the current head (0aa2f22, build 93161, 190/190 jobs), and the earlier runs' only red lanes were worker-transfer-list.test.ts (x64-asan termination race, #37267's area) and s3.test.ts (R2 InternalError), neither touched by this diff. Open PRs touching the same code are listed in the description (#36435 is made unnecessary by this change; #35079, #35523, #33603, #33604 and #31487 are compatible and would rebase onto it).

claude

I reviewed this PR and didn't find any bugs. Because it reworks name ownership across the Rust/C++ FFI boundary for a shared refcounted type and carries several user-visible behavior changes that interact with other open PRs, a human look would still be worthwhile.

What was reviewed:

Blob__getFileNameString / Blob__setAsFile lifetime contracts against their C++ callers in blob.cpp and JSDOMFormData.cpp — dupe_ref() and toWTFString(ZeroCopy) each take their own +1, and Blob::create dupes at append time so setAsFile writes to the entry's own Blob.
get_name_utf8()'s returned ZigStringSlice is self-owning (to_utf8() is the ref-holding variant), so RequestContext and get_loader don't borrow past self.name.
OwnedString::into_inner → OwnedStringCell::set transfers the +1 without leak or double-deref; the UTF-16 USVString reassignment drops the old OwnedString.
init_empty_with_name has no remaining callers; the two intentional get_file_name() sites in jsc_hooks.rs are guarded by needs_to_read_file().

Extended reasoning...

Overview

This PR fixes a heap-use-after-free and a spec-correctness bug in File/FormData name handling by moving the per-Blob name off the shared store::Bytes::stored_name and onto Blob::name. It touches src/jsc/webcore_types.rs (comments, removes init_empty_with_name), src/runtime/webcore/Blob.rs (jsdom_file_construct, Blob__setAsFile, Blob__getFileNameString, new get_name_utf8 helper, Any::get_name_utf8), src/runtime/server/RequestContext.rs (Content-Disposition filename lookup), and adds 19 tests across blob.test.ts and FormData.test.ts.

Security risks

None identified. The change removes the only site that reassigned a live shared store's stored_name (the root cause of the borrowed-view UAF), and the new Blob__getFileNameString returns a ref-counted WTF string rather than a raw byte view. The Content-Disposition path still goes through the existing contains_any(.., b"\r\n\0\"") injection guard.

Level of scrutiny

High. This is native memory-management code in a type shared across the Rust/C++ FFI boundary (Blob__setAsFile, Blob__getFileNameString, Blob__dupe) with intrusive refcounting on both the Blob and its Store. I traced the ownership of every string handoff: jsdom_file_construct uses OwnedString::into_inner() → OwnedStringCell::set() (transfers the +1, derefs the old value); the UTF-16 branch's reassignment drops the old OwnedString before adopting the clone_utf8 result; Blob__setAsFile's dupe_ref() bumps the WTF ref so the name outlives the C++ stack local; Blob__getFileNameString returns a borrowed BunString that the C++ caller immediately toWTFString(ZeroCopy)s into its own ref while this is still live. get_name_utf8() returns a ZigStringSlice from the ref-holding to_utf8() (not _without_ref / _borrowed), so callers in RequestContext.rs and get_loader hold a self-contained slice that doesn't depend on self.name staying put. I also confirmed Blob::create(JSValue) in blob.h calls Blob__dupeFromJS, so the FormData entry's impl() is a distinct Blob from the user's — writing name on it via Blob__setAsFile does not touch the appended blob, and dupe_with_content_type's name: self.name.clone() (which dupe_refs) carries it to the JS wrapper.

Other factors

The PR carries four intentional behavior changes beyond the bug fix (new File(bits, "").name → "", FormData no longer naming the appended blob, zero-byte entries keeping their filename, and Bun.serve's Content-Disposition preferring the File's display name over the on-disk basename). All match Node per the description, and each is covered by a new test, but they are user-visible and the description names four other open PRs (#33603/#33604/#35079/#35523/#36435) that touch the same two functions or depend on this change's semantics. That coordination is exactly what a maintainer should sign off on. Test coverage is thorough (variant matrix over source kinds, structuredClone, USVString, blob: loader, Content-Disposition, spawned ASAN repro for the UAF), and the description states 18/19 fail on main and all pass with the change; CI is still building.

…me string A WTF::StringImpl becomes an atom of whichever thread first uses it as a property key, and dropping its last reference on another thread aborts in AtomStringImpl::remove. The registry shared the registered blob's name impl with every thread that resolved the URL. That was already reachable for file-backed Files and now, with names kept on the Blob, for every File, so copy the name when storing the entry and again on each resolve.

claude

Automated review ran and found no bugs. This touches memory-safety-critical native code (WTF string refcounting across the Rust/C++ FFI boundary, cross-thread StringImpl handling in ObjectURLRegistry) and changes user-visible behavior in several documented places, so a human look is still worthwhile.

What was reviewed:

Blob__getFileNameString now returns a borrowed BunString view of blob.name; verified toWTFString(ZeroCopy) at both C++ call sites (JSDOMFormData.cpp:315,478) refs the impl via WTF::String(impl.wtf), so the borrowed return is sound.
dupe_with_private_name refcount balance: replace→into_inner transfers the +1, String::to_thread_safe swaps the impl in place, set installs it over the dead() placeholder — no leak or double-free.
Remaining get_file_name() callers (jsc_hooks.rs:1506,4244) are the on-disk-path uses the description says are intentional; init_empty_with_name has no callers left.

Extended reasoning...

Overview

Moves File / FormData-entry names from the shared store::Bytes::stored_name onto the per-Blob name field, fixing a class of bugs where wrapping a Blob in new File([blob], name) or appending it to FormData renamed the source (and could UAF a borrowed name view held by FormData). Also fixes a pre-existing cross-thread AtomStringImpl release abort in ObjectURLRegistry by giving the registry entry and each resolver a private (isolated-copy) name string. Touches jsdom_file_construct, Blob__setAsFile, Blob__getFileNameString, get_loader, Any::get_file_name→get_name_utf8, RequestContext Content-Disposition, and adds 21 tests across two files.

Security risks

None identified. The change removes a heap-use-after-free reachable from user JS and a cross-thread abort. The Content-Disposition path already sanitizes control bytes (contains_any(.., b"\\r\\n\\0\\\"")) and truncates to a stack buffer; switching from store path to per-Blob name doesn't widen that surface.

Level of scrutiny

High. This is native code at the Rust↔C++ FFI boundary manipulating WTF StringImpl refcounts, changes cross-thread string ownership semantics in a process-global registry, and intentionally alters user-visible behavior (new File(bits, "").name, FormData not renaming the appended blob, zero-byte parts keeping their filename, Bun.serve Content-Disposition preferring the File's display name over the on-disk basename). The reasoning is careful and the test matrix is thorough (18/21 fail on main, spawned ASAN repros for the UAF and the atom-release abort, worker handshake for the cross-thread case), but the combination of refcount transfers, borrowed-return FFI contracts, and thread-affinity invariants is exactly the category REVIEW.md flags as most-blocked.

Other factors

I traced the borrowed-BunString contract of Blob__getFileNameString through BunString.cpp's toWTFString(ZeroCopyTag) → WTF::String(this->impl.wtf) and confirmed it refs the impl, so the C++ FormData filename outlives any later reassignment of blob.name. OwnedStringCell::set derefs the previous value, so Blob__setAsFile overwriting the entry-dupe's name and jsdom_file_construct overwriting the source-dupe's name don't leak. dupe_with_private_name's replace(dead) → into_inner → to_thread_safe → set sequence balances to exactly one +1 on the isolated copy. The Any::get_file_name → get_name_utf8 rename's only in-tree caller is the RequestContext site updated in this diff. All comment-cop threads are resolved (comments shortened in de3f1c9/ec05e1e). Given the scope and the number of intentional behavior changes interacting with several other open PRs (#33603/#33604/#35079/#35523/#36435/#31487), a maintainer should confirm the layering choice and the Content-Disposition behavior change.

The loader and Content-Disposition lookups borrow the store's name when the blob has none of its own, as they did before, instead of allocating a cached copy per response body. Pin both fallbacks with a Bun.file() case.

claude

I reviewed this PR and the automated bug hunt found no issues. Because it reworks name ownership across the Blob/Store boundary (a heap-use-after-free fix), changes cross-thread WTF::StringImpl handling in ObjectURLRegistry, and alters several user-visible behaviors while intersecting with at least six other open PRs, a maintainer look is still worthwhile.

What was reviewed:

Blob__getFileNameString now returning get_name_string() — checked the C++ callers in JSDOMFormData.cpp still ref via toWTFString(ZeroCopy), so the borrowed contract holds.
Blob__setAsFile writing this.name and both blob.cpp toJS paths — the entry's dupe inherits the name via dupe_with_content_type's name.clone().
get_name_utf8 not caching — the store-backed branch returns a never_free view of bytes the store owns, matching the prior get_file_name() lifetime at the two rewritten call sites.
dupe_with_private_name — to_thread_safe() on both registry entry and each resolve, so no thread's atom impl is shared.

Extended reasoning...

Overview

This PR moves File/FormData-entry names off the shared refcounted Store and onto the per-Blob name field. It touches four Rust files (webcore_types.rs, Blob.rs, ObjectURLRegistry.rs, RequestContext.rs) and adds 21 tests across two test files. The core changes are: jsdom_file_construct now sets blob.name unconditionally instead of mutating store::Bytes::stored_name; Blob__setAsFile writes the per-Blob name; Blob__getFileNameString and a new get_name_utf8 helper use the per-Blob lookup; ObjectURLRegistry copies the name via to_thread_safe() on both register and resolve; init_empty_with_name is deleted as dead.

Security risks

None in the classic sense (no auth/injection surface), but this is squarely in the memory-safety category the repo review guide calls out as most-blocked: it fixes a heap-use-after-free (freed stored_name bytes read by FormData serialization) and a cross-thread RELEASE_ASSERT in AtomStringImpl::remove. The fix reasoning depends on WTF string atom semantics, BunString ref ownership across the FFI boundary, and OwnedStringCell::clone behavior in dupe_with_content_type.

Level of scrutiny

High. This is native code changing ownership of a string field that crosses Rust↔C++ FFI (Blob__setAsFile, Blob__getFileNameString) and is shared across threads via ObjectURLRegistry. It also introduces four user-visible behavior changes (empty-name File returns "", FormData no longer renames the appended blob, zero-byte parts keep filenames, Bun.serve Content-Disposition uses the File's display name), and the description explicitly maps interactions with #31487, #33603, #33604, #35079, #35523, #36001, and #36435 — coordination a maintainer should sign off on.

Other factors

The PR is exceptionally well-documented and well-tested (18/21 new tests fail on main, spawned ASAN repros for both memory bugs, matrix coverage of the variant space). The comment-cop feedback was addressed in de3f1c9/ec05e1e. I traced the C++ callers of the two changed extern "C" functions and the dupe→name.clone() path and found nothing wrong, but the combination of FFI ownership, cross-thread atom-string semantics, and multi-PR coordination puts this outside what an automated review should approve on its own.

The blob: loader and FormData's default filename now follow a File's own name for a File wrapping a Bun.file() and for a blob named through the setter, and a FormData filename override applies to an already named File. Add tests for each, and note at the two module-loading sites that the path they read from the store is intentionally not the File's name.

claude

I reviewed this PR and the automated bug hunt found no issues. Because it reworks the Blob/File name ownership model across the FFI boundary (WTF string refcounts at Blob__getFileNameString/Blob__setAsFile) and adds cross-thread StringImpl isolation in the process-global ObjectURLRegistry, a human look at the refcount and thread-safety reasoning would be worthwhile.

What was reviewed:

get_name_utf8 lifetimes at the three call sites (RequestContext Content-Disposition, get_loader, Any) — the ZigStringSlice is consumed before drop.
Blob__getFileNameString → toWTFString(ZeroCopy) takes its own +1; the returned BunString borrows self.name without an extra ref, so no leak.
dupe_with_private_name: replace(dead) → to_thread_safe → set keeps the count balanced (BunString__toThreadSafe derefs the original when it swaps in the isolated copy); Dead/non-WTF tags pass the is_thread_safe debug assert.
The two remaining get_file_name() callers in jsc_hooks.rs intentionally read the store path; init_empty_with_name has no remaining callers.

Extended reasoning...

Overview

This PR moves File/FormData-entry names from the shared Store::Bytes::stored_name onto the per-Blob name field, fixing (1) new File([blob], name) renaming every Blob that shares the store, (2) a heap-use-after-free where FormData's borrowed default filename was freed by a later new File, and (3) a pre-existing cross-thread AtomStringImpl::remove abort in ObjectURLRegistry that this change would otherwise widen. It touches jsdom_file_construct, Blob__setAsFile, Blob__getFileNameString, adds BlobExt::get_name_utf8, rewires three name consumers (Content-Disposition, get_loader, Any::get_name_utf8), and adds dupe_with_private_name to the object-URL registry's store and resolve paths. 25 new tests cover the matrix.

Security risks

The change removes a user-reachable UAF and a cross-thread abort; it does not add attack surface. The Content-Disposition path already sanitizes against header injection.

Level of scrutiny

High. This is native memory-safety code at the Rust/C++ FFI boundary: WTF StringImpl refcounting at Blob__getFileNameString (borrowed BunString consumed by toWTFString(ZeroCopy)), OwnedStringCell::set/replace balancing in Blob__setAsFile and dupe_with_private_name, and isolatedCopy()-based thread isolation in a process-global mutex-guarded registry. REVIEW.md flags every one of these categories (borrowed views, provably balanced refcounts on every path, thread affinity of shared strings) as most-blocked. My trace of each path looks balanced, but this is exactly where a maintainer familiar with the WTF atom model and the overlapping PRs (#31487, #35079, #35523, #36435) should confirm the design.

Other factors

The description enumerates every intentional behavioral change and pins each with a test; 23/25 fail on main. The comment-cop feedback was addressed in de3f1c9/ec05e1e. CI on the prior head was green modulo two known-unrelated flakes. The change also intersects with several open/planned PRs on the same functions, which argues for a human coordinating the merge order.

github-actions Bot added the claude label Aug 12, 2026

robobun mentioned this pull request Aug 12, 2026

FormData: preserve filename when parsing a zero-byte multipart file part #36435

Open

claude Bot reviewed Aug 12, 2026

View reviewed changes

robobun added 2 commits August 12, 2026 00:41

test: drop the per-test timeout on the spawned FormData filename test

eb12199