Repository navigation
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughBlob ChangesBlob copy-on-write buffers
Suggested reviewers: Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to
|
|
Updated 8:36 PM PT - Oct 21st, 2026
✅ @robobun, your commit 9930e0ca012845e52e7ae4e51a23f6b232abd324 passed in 🧪 To try this PR locally: bunx bun-pr 45947That installs a local version of the PR into your bun-45947 --bun |
|
Reproduced on a release build of main (5f5799e), linux x64: const size = 8 * 1024 * 1024;
const res = new Response([new Uint8Array(size).fill(97)]);
const kept = new Uint8Array(await res.clone().arrayBuffer());
const mine = new Uint8Array(await res.arrayBuffer());
mine.fill(90);
console.log(kept.indexOf(90)); // 0 on main, -1 with this PR
|
There was a problem hiding this comment.
Beyond the inline findings, I also checked the other consumers of the memfd store for the same aliasing: the Lifetime::Share arm in src/runtime/webcore/Blob.rs hands out the single store view with a store ref held, so its buffers share one mapping exactly as before; to_js_buffer_from_memfd in src/jsc/array_buffer.rs (subprocess stdio) is a separate memfd that never goes through LinuxMemFdAllocator, so its MAP_SHARED is unaffected; and the Bytes doc comment at src/jsc/webcore_types.rs:613 does not describe the old flags, so nothing stale remains there.
Extended reasoning...
The diff drops the offset/flags parameters of LinuxMemFdAllocator::alloc and maps the store's view MAP_PRIVATE | MAP_NORESERVE instead of MAP_SHARED, with create as the only caller updated, plus ~160 lines of new tests in blob-cow.test.ts. No security-sensitive surface is touched. The sibling memfd mapper (ArrayBuffer__fromSharedMemfd) and the Share/Transfer arms were read to confirm no other write path reaches the file.
Findings marked 🟡 are optional suggestions and need no follow-up push.
d20af52 to
bd26d5c
Compare
MAP_PRIVATE so two reads of one large body do not share memorybd26d5c to
0dbb770
Compare
There was a problem hiding this comment.
I reviewed the new revision and did not find any bugs; because it moves ownership of a Store ref across the Rust/C++ boundary and changes what a live copy keeps open (one memfd fd and mapping per store until the copy is collected), a human look at the design tradeoff is still worthwhile.
What was reviewed:
- Ref balance for the
store.clone().into_raw()handed toArrayBuffer__fromSharedMemfd: C++ releases it on the three early returns, in the munmap lambda, and whenJSUint8Array::create/JSArrayBuffer::createfail (the movedRef<ArrayBuffer>is destroyed, which runs the SharedTask).StoreusesThreadSafeRefCount, so running the deallocator off the JS thread is sound. - The
Lifetime::Sharefold intoClone:to_array_buffer/to_uint8_arrayhave noSharecaller (read_file passesTemporary, S3 passesClone), so no behavior changes for existing callers; theTransferarm correctly falls to the copy branch once a COW copy holds a ref. - The store view stays
MAP_SHARED(theallocsignature change drops only unused parameters), and the /proc-based mapping tests are gated on a memfd probe rather thanisLinuxalone.
Extended reasoning...
The change touches the FFI declaration and definition of ArrayBuffer__fromSharedMemfd (src/jsc/array_buffer.rs, src/jsc/bindings/ZigGlobalObject.cpp), moves the memfd copy-on-write path into LinuxMemFdAllocator::copy_on_write_js (src/runtime/allocators/LinuxMemFdAllocator.rs), removes the Lifetime::Share ArrayBuffer arm in src/runtime/webcore/Blob.rs, and adds a large test block in test/js/web/fetch/blob-cow.test.ts. It touches no auth, injection, or data-exposure surface; the sensitive surface is unsafe Rust and raw-pointer ownership transfer across FFI plus mmap of a memfd. The parameter lists match on both sides, every C++ exit path releases the context exactly once, and the test probes memfd availability before reading /proc. It is still not a simple change: it alters when the store's fd is closed and relies on JSC ArrayBuffer destructor semantics, so a maintainer should weigh the fd-per-live-copy tradeoff, and no CODEOWNERS entry covers these files.
A Blob of 8 MiB or more keeps its bytes in a memfd on Linux. The store mapped that file MAP_SHARED, and a read by the store's last owner handed that mapping to JS. A write into it changed the file, and with it every earlier copy-on-write copy of the same Blob. Map the store's view MAP_PRIVATE | MAP_NORESERVE. No mapping of the file is shared, so no write reaches it after create() fills it. Remove the unused flags and offset parameters of alloc().
The byte cases also pass when no memfd store is in use. The new case reads /proc/self/fd and /proc/self/smaps: the body's Blob is one memfd, the store's view of it is private and exempt from the commit charge, and a second read adds one more private mapping. Reword the MAP_NORESERVE comment: the flag keeps the view out of Committed_AS, and vm.overcommit_memory=2 ignores it.
The last owner of a memfd store gets the store's own MAP_SHARED mapping (has_one_ref() in the Transfer arm). A copy-on-write mapping of the same memfd held no reference on the store, so that test passed while a copy was alive, and a write into the handed-out mapping changed the copy. ArrayBuffer__fromSharedMemfd takes a deallocator and a context, and runs the deallocator once: when its buffer is freed, or before it returns when it makes no buffer. The one mapper, LinuxMemFdAllocator::copy_on_write_js, passes a Store ref, so a store with a live copy is not solely owned. The store's view is MAP_SHARED again, as on main: a body that was never copied keeps the zero-copy hand-out. The Share lifetime of the array-buffer conversion had no caller and handed out the view of a shared store. It takes the copy arm now.
Each copy-on-write mapping of a memfd store now holds a reference on the store's allocator until JS frees the buffer. The last owner of a store takes the store's own shared mapping only when the allocator has no other reference. A live copy no longer keeps the store, its file descriptor or its shared mapping: the descriptor closes with the store's view. The mapping is made in Rust and handed to JS through the existing no-copy constructors, so ArrayBuffer__fromSharedMemfd is removed.
0dbb770 to
d5d5a0d
Compare
There was a problem hiding this comment.
I re-reviewed the latest revision (allocator-ref design, mapping moved into Rust) and found no bugs; because this is unsafe refcount/mmap/FFI-deallocator code on the Blob hot path, a human look is still worthwhile.
What was reviewed:
- Ref balance of
CopyOnWriteinsrc/runtime/allocators/LinuxMemFdAllocator.rs: the allocator ref is taken only aftermmapsucceeds, released exactly once viaCopyOnWrite::release(JSC runs the deallocator on both GC and theErrreturn ofto_js_with_context), andDropcloses the fd only whenfreehas not already replaced it withFd::INVALID, so no double-close. - The
owns_bytes_aloneordering insrc/runtime/webcore/Blob.rs: with one store owner, no other thread can entercopy_on_write_js, so a concurrentCopyOnWritedrop can only yield a false "has copies", which falls back to a private copy rather than the shared view. - The
Sharearm merge: no ArrayBuffer/Uint8Array caller passesLifetime::Share(onlyto_json/to_stringdo), and the OOM-pathdetach()staysClone-only, so the behavior change is confined to a dead path. - No remaining references to the removed
ArrayBuffer__fromSharedMemfd; the new tests gate onmemfd_createfrombun:internal-for-testingand/procreadability, and theafterCollectedhelper is bounded (50 gc ticks) and fails rather than hangs.
Extended reasoning...
The range reworks how a Linux memfd-backed Blob store (>= 8 MiB) hands bytes to JS: the C++ ArrayBuffer__fromSharedMemfd and its Rust wrapper are deleted, replaced by LinuxMemFdAllocator::copy_on_write_js which mmaps MAP_PRIVATE, boxes a CopyOnWrite holding an allocator ref, and hands it to JSC with a release callback; the Transfer arm now gates the zero-copy MAP_SHARED hand-out on owns_bytes_alone (store has one ref and the allocator has no outstanding copy refs). It touches no auth, injection, or data-exposure surface, but it is unsafe Rust with raw-pointer refcounting, mmap/munmap ownership, an FFI deallocator, and cross-thread atomics. The bug hunt ran dry and the inline concerns from my earlier review of the first revision (MAP_PRIVATE store view, unguarded /proc reads) are no longer present in the final code, but the change is too complex and memory-safety-sensitive to approve without a human; no CODEOWNERS entry covers these paths and no third-party CHANGES_REQUESTED review is outstanding.
The allocator no longer writes its fd field after construction. free closes the fd when it unmaps the store's view, and create closes it when the first mapping fails. The Drop impl and the write that needed an exclusive-access argument are removed.
There was a problem hiding this comment.
I reviewed this revision and didn't find any bugs. Because it moves memfd mapping lifetime into unsafe Rust (an FFI finalizer plus a cross-thread refcount gate on the zero-copy hand-out), a human look is still worthwhile.
What was reviewed:
- Allocator ref balance in
copy_on_write_js: the +1 is taken only aftermmapsucceeds and is released exactly once byCopyOnWrite::release, whichto_js_with_contextruns on collection or before returningErr;createcloses the fd onallocfailure without a double close now thatDropno longer closes it. - The
owns_bytes_aloneordering: with the store at one ref, no other holder can reach the copy path, so the allocator count can only decrease between the two reads (bothSeqCst); a stale "copies exist" read just falls back to a private mapping. freeclosing the fd while private mappings survive:CopyOnWritenever readsfd, and the only other readers (alloc,copy_on_write_js) run while the store'sBytesis still live.Lifetime::Sharefolded into the copy arm: the onlySharecallers insrc/runtime/webcore/Blob.rsare the string/JSON paths, so no caller loses the shared view.
Extended reasoning...
The change deletes the C++ ArrayBuffer__fromSharedMemfd mapper and replaces it with a Rust LinuxMemFdAllocator::copy_on_write_js that owns a MAP_PRIVATE mapping plus an allocator ref, makes the allocator's fd private and closed from the vtable free path, adds has_copies gating the Lifetime::Transfer zero-copy hand-out in src/runtime/webcore/Blob.rs, and adds a test matrix in test/js/web/fetch/blob-cow.test.ts. It touches no auth, injection or data-exposure surface, but it is unsafe memory-lifetime code at an FFI boundary with a cross-thread refcount used as a correctness gate. The bug hunt ran dry with no findings and the refcount, fd-close and race-ordering paths traced out balanced, which is why this is a defer rather than a request for changes; the depth of unsafe code and the prior design churn across revisions is why it is not an approve. No CODEOWNERS entry covers the changed files.
|
#46835 removes the This PR keeps that detach as One change here removes the conflict for both merge orders: drop only the |
Related to #22885
Problem
kept = await res.clone().bytes()andmine = await res.bytes(),mine.fill(90)changes all ofkept.MAP_SHAREDmapping (has_one_ref(),src/runtime/webcore/Blob.rs:2837). A copy is aMAP_PRIVATEmapping that this test does not count.Fix
LinuxMemFdAllocatoruntil JS frees it. The last owner gets the shared mapping only when that count is 1.ArrayBuffer__fromSharedMemfdis removed.test/js/web/fetch/blob-cow.test.ts(20 of 32 cases fail on main),blob.test.ts,body.test.tsand 12 more. Self-reviewed: 12 concerns raised, 10 addressed, 2 handed off.Background
blob.arrayBuffer()is onemmap, not a copy (Make file uploads use less memory on Linux #7938).MAP_PRIVATEpage reads the file until its first write, which copies it. AMAP_SHAREDwrite goes to the file.Downsides
mmapand +1munmap.Notes
Who hits it
#22885 reported this class for every body size: a write into the result of
clone().arrayBuffer()changed the original. #33072 fixed it with the last-owner test. This is the case that the test does not see. A fuzz run found it.Linux and Android only: macOS and Windows have no memfd path. The body must be a single store of 8 MiB or more (1 MiB with
--smol). A copy of it must be alive when a body that is the store's last owner is read. Measured at 12 MiB on main:new Response([bytes]),new Response([blob]),new Request(url, { body: [bytes] }), aDataViewbody,server.fetch(url, { body: bytes })Blob, aFile, or a file part out offormData(), once that object is collected. With the body made inside a function, that happens without a forced collection in 9 of 10 runsUint8Array,ArrayBuffer,Bufferand string bodies, a body whose Blob is alive,server.fetch(new Request(...)), a request that arrives over the networkOne store can also be reached from two threads through a
blob:URL. The copy and the last owner are then in different VMs.Strings are safe.
blob.text()returns a string that holds a reference on the store.Why this layer
The rule that broke is the last-owner test. It asks whether this Blob is the only holder of the bytes. A copy-on-write mapping of the memfd is a holder that no count saw. main already took an allocator reference in the copy branch, for the length of the call. This PR keeps that reference for the life of the buffer, and the test reads it.
owns_bytes_alonereads the store count first and the allocator count second. With one store owner left, no other thread can reachcopy_on_write_js, so the allocator count can only go down. The other order would race.allocandcopy_on_write_jsmap the memfd.copy_on_write_jsmaps first and takes the reference only when the kernel accepts the mapping. The buffer's destructor is the single place that releases it.fdfield is private, and nothing writes it after construction.allocator_interface::freecloses the fd when it unmaps the store's view, andcreatecloses it when the first mapping fails. main closed it inDrop, which ran at that same moment because nothing else held the allocator. A copy that outlives its Blob keeps its private mapping, a 24-byte record and the 16-byte allocator, and nothing else.Store::has_one_ref()are not edited.Any::to_internal_blob_if_possibleandStore::to_any_blobcopy memfd bytes to the heap before they hand them out, so they never expose the shared mapping.The
Lifetime::Sharearm of the same function had no ArrayBuffer or Uint8Array caller, and it handed out the view of a shared store. It takes the copy arm now.allocloses its unusedflagsandoffsetparameters and mapsMAP_SHAREDas before.Designs weighed:
ulimit -v 4000000(main 26)MAP_PRIVATE(the first revision)VmData, every last-owner result is copy-on-write. Numbers belowmprotectat the hand-outmmap, +1munmapand a page copy per written page on every last-owner read. No count and no gatefcntlper large Blob, and the zero-copy hand-out is gone for goodblob.arrayBuffer()The count exists to keep the zero-copy hand-out for a sole owner. That hand-out reaches only the body forms in the first two rows of the table above. It never reaches a Blob that is alive or an upload from the network. If that hand-out is not wanted, "always a private copy" is the smaller change. It costs 16384 page copies and +64 MiB of anonymous memory for each overwritten 64 MiB result of
new Response([bytes]).Self-review
The review of the second revision raised 7 concerns. This revision addresses 6 of them:
ulimit -v.ulimit -v.memfd_createand/proc), not on the behaviour under test. Three cases assert that the body is in a memfd.DataViewbody.One is handed off to a separate change: a read of a small slice maps the whole file.
The review of this revision raised 5 concerns. Four are addressed:
DataViewbodies named the wrong follow-up. Two concerns said so. It is corrected below.One is handed off: a typed array over a buffer with an unaligned base aborts in
console.log(below). The review also asked for an issue for the body init follow-up. I did not file one, because it is a design choice with a memory cost and not a defect. It is described below for the assignee.Measurements
Release builds of main 6f58f6b and of this change on that commit, linux x64, 12 vCPU, kernel 6.1.
arrayBuffer()andbytes(), both read orders, aDataViewbody, a collected Blob, a read from the Blob object first, a slice, aformData()file part,transfer(),structuredClonewith transfer. With--smolat 1 MiB: 0 of 256 (main 256 of 256). Control at 8388607 bytes: 0 on both.rw-s(main: onerw-s). Fill 7.26 ms (main 7.37 and 7.39 ms in two interleaved series of 15).memfd_create2,ftruncate1,pwrite641,mmap1,munmap1 /mmap1,munmap1 / the same as a construct. main is identical in all three. Last-owner read with a live copy: +1mmap, +1munmap. Counted with a ptrace counter, because strace is not installed in the container.Transferinstances:bytes()+4 for a heap store and +8 for a memfd store,arrayBuffer()+11 and +15. The test is 2 compares, and for a memfd store 2 loads and 1 compare more. ThearrayBuffer()instance also pushes and pops 3 more registers, because the compiler now inlines the copy arm into it. Counted withperf_event_open(user instructions, JIT off, one GC thread, slope between 10000 and 20000 reads of a 1 KiB body, median of 4 series):bytes()7985 per read on main and 7989 on the PR,arrayBuffer()8373 and 8384.ulimit -d1 GiB, 64 MiB objects: live Blobs 48 of 48 (main 48), never-copied results 40 of 40 (main 40), after a collected copy 40 of 40 (main 40), with a live copy 4 pairs thenRangeError: Out of memory, exit 0 (main 9 pairs then the same error, and each main pair is one memory).ulimit -v 4000000, kept copies of dropped Blobs: 26 at 64 MiB and 49 at 32 MiB, thenRangeError: Out of memory, exit 0. main is identical.memory.max1 GiB: 30 of 30 iterations, peak 583 MiB (main 30 of 30, 583 MiB), 5 runs each.blob:URL runs that change the earlier copy: 0 of 40 (main 40 of 40, with the collection forced)..text: -256 bytes (81695461 to 81695205,size -A).With every copy-on-write
mmaprefused through anLD_PRELOADshim, the reads fall back to heap copies, the copies stay intact, and no memfd fd is left after the Blob is collected.The last commit changes only where the fd is closed. A release build of it repeats these results for the alias, the fds, both limits, the refused mappings, the consumer script and the test file, and its
.texthas the same size. With the store's own mapping refused through a second shim,createfails 20 of 20 times, the Blobs fall back to the heap, and no memfd fd stays open.A script with 63 checks reads a 12 MiB Blob through its consumers (reads, slices, streams,
Bun.write,Bun.spawnstdin,fetchupload, FormData, Worker, object URL), with and without a live copy. Both release builds read the same bytes in all 63.The earlier revisions
The first revision changed one flag:
allocmapped the store's viewMAP_PRIVATE | MAP_NORESERVE. It removed the alias. Measured on a release build of it:ulimit -d1 GiB the 10th live 64 MiB Blob aborted the process (exit 134). main and this revision keep 48 of 48.memory.max1 GiB was killed in 5 of 5 runs (exit 137, peak 1024 MiB). main and this revision finish 30 of 30 at 583 MiB.The second revision gave each copy a store reference. It added nothing to a last-owner read. Measured on a release build of it:
ulimit -v 4000000it kept 17 copies of 64 MiB and 31 of 32 MiB. main and this revision keep 26 and 49.The Worker crash
The alias gave another thread a way to write a plain
ArrayBufferduring a native read.kept = await res.clone().arrayBuffer(),mine = await res.arrayBuffer(),worker.postMessage(mine, [mine]). The Worker rewrites its buffer in a loop, once with ASCII and once with 3-byte UTF-8 sequences. The main thread callsnew TextDecoder().decode(kept.subarray(0, 1 << 20))in a loop. main: 10 of 10 runs end inpanic: Segmentation fault at address 0x0(7 of 10 in an earlier round, where the other 3 read the Worker's bytes). PR: 0 of 10, and no foreign byte.This PR removes that route, not the crash. The UTF-8 decoder reads its input twice (
src/bun_core/string/immutable/unicode.rs), so any buffer that another thread writes can still crash it. #41574 changes the decoder and has the race fixture. The test file here does not carry this shape because it is a race.Tests
blob-cow.test.tsgets 27 cases:new Response([bytes]),new Request(url, { body: [bytes] }),new Response(new DataView(buffer))) xclone().arrayBuffer()/clone().bytes()xarrayBuffer()/bytes().buffer.transfer()and afterstructuredClonewith transfer.Blobobject. The test waits on aFinalizationRegistry, because the finalizer is what releases the reference on the bytes. The wait is bounded, so an object that is never collected fails the case and does not hang it./proc/self/fdand/proc/self/maps. They run whenmemfd_createworks and/procis readable, and they are skipped withBUN_FEATURE_FLAG_DISABLE_MEMFD. Three assert that each body form puts its bytes in a memfd, so a changed threshold or label fails them. The others: the only owner gets therw-smapping. The last owner gets a secondrw-pmapping while a copy is alive, and the fd is closed. It gets therw-smapping again after the copy was collected. A copy that outlives its Blob keeps onerw-pmapping and no fd. Nothing maps the memfd after every copy was collected.20 of the 32 cases in the file fail on a release build of main: the 19 byte cases and the second mapping case. The other 7 mapping cases hold on main too. They pin the zero-copy hand-out, the release of the reference and the close of the fd. The file passes 20 of 20 runs on the release build and 10 of 10 on the debug build.
One limit of the collection cases. With
--rerun-each=100the file fails 1 of 3200 executions, always in repetition 72 of one case: a tracked buffer is not collected within the 50 ticks. A heap snapshot at that point shows the buffer as a root of the JIT worklist. A DFG compile that is finished but not installed yet keeps the values that it was compiled against. The file passes 3200 of 3200 withBUN_JSC_useConcurrentJIT=0and withBUN_JSC_useDFGJIT=0, and 640 of 640 with--rerun-each=20. CI runs each case once per process.Suites on the debug build of this branch:
memfd-disabled.test.ts,blob-oom.test.ts,body-clone.test.ts,blob-array-fast-path.test.ts,blob-write.test.ts,body-mixin-errors.test.ts,blob.test.tsandbody.test.tsintest/js/web/fetchand intest/js/deno/fetch,FormData.test.ts,FormData-multipart-serialization.test.ts,structured-clone.test.ts,fs-oom.test.ts: 1531 pass, 1 fail. The failure is the 2 GiB case offs-oom.test.ts, which takes more than the 5 s limit under the debug build on this machine. It does not use a Blob. In one run each, with the machine under load, abeforeAllofblob-oom.test.tsand two cases ofbody.test.tsthat spawn a debug build also took more than 5 s. They passed in the runs after it.bun run rust:check-allpasses for all 12 targets, andcargo clippyis clean forbun_runtimeandbun_jsc.Left for later
ftruncateon the memfd by fd number (fs.writeSync(5, ...)) changes the Blob and every copy while the Blob is alive, and a truncate ends in a bus error. Same on main. A seal aftercreatefills the file would close it.Blob::try_createaborts when the heap refuses its fallback copy. Same on main.vm.max_map_count. Same on main.DataViewonResponseandRequest(2), an array of one buffer onResponseandRequest(5), and a buffer or an array of one buffer passed toserver.fetch(url, { body })(5). ADataViewrule beside the typed array rule would remove 2 of them. A heap copy of a one-part buffer at body init would remove all 12 and leavenew Blob([bytes])on the memfd. It has a cost: with the memfd store off, the loop that keeps 40 of 40 results of 64 MiB underulimit -d1 GiB aborts on a 64 MiB heap allocation. After that change, the count would guard only bodies whose Blob was collected, and a plain copy could replace it. This is a choice for the assignee, not a defect.new Response([blob]),new Request(url, { body: [blob] }),new Response([file])andserver.fetch(url, { body: blob }). Blob: keep the caller's Blob intact after a slice read, an array body, and a rejected bytes() #38503 changes that.console.log(new Uint16Array(await blob.slice(1, 17).arrayBuffer()))then aborts withcast_slice>TargetAlignmentGreaterAndInputNotAligned.Bun.mmapwith an offset does the same with no Blob. Handed off as a separate fix in the consumers.src/jsc/.no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/web/fetch/blob-cow.test.ts