Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: oven-sh/bun/.coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 1 remains after this review. WalkthroughBlob construction now reads file-backed parts when their in-memory views are empty. The change handles path-backed and fd-backed files, and rejects unsupported synchronous reads. Tests cover mixed parts, file changes, slicing, and error cases. ChangesBlob part byte contribution
Suggested reviewers: Priority: ➖ Normal Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to The reported mixed-part behavior is covered by the changed implementation and tests. No identified issue currently prevents merging after normal checks. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 3:57 PM PT - Sep 24th, 2026
✅ @robobun, your commit 14a5bbd91e55da15f7bbfef3bacd1ad403f8db11 passed in 🧪 To try this PR locally: bunx bun-pr 33600That installs a local version of the PR into your bun-33600 --bun |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Status: ready for review at 14a5bbd, which includes a merge with main. CI is green: build 120442 passed all 181 jobs. Reproduce on main: Proof: Open question for a maintainer: the constructor reads a file part synchronously and throws when it cannot. The PR body gives the cost and the cases under Downsides. |
c97ea66 to
4ff7093
Compare
4a5b278 to
bfd3bc2
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
bfd3bc2 to
ee3facf
Compare
There was a problem hiding this comment.
No new issues found — all five concerns from my earlier rounds (fd cursor via pread, the size==0 ENOENT guard, the S3 error wording, Windows pread pointer semantics, and the st_size==0 procfs case) are addressed in the current diff. Deferring to a maintainer because this introduces synchronous disk I/O into the new Blob([...]) constructor path and changes observable behavior (ENOENT now thrown where file parts were previously silently dropped), which is a design call worth a human sign-off. Note comment-cop has left several unresolved flags on the new code comments; the ones I see in the current diff are 2-3 line invariant notes rather than workaround justifications, so they may just need dismissing.
What was reviewed: the borrow-vs-clone split in push_blob_part_bytes and its detach_lifetime safety contract against the caller's prescan; the Path arm's read_file ownership/free of the returned buffer; the Fd arm's pread grow-loop for cap sizing, offset arithmetic, and short-read handling; and the new tests for hermeticity and platform gating.
Extended reasoning...
Overview
The PR fixes #25851: file-backed Blob parts (Bun.file(path), Bun.file(fd), slices/clones thereof) contributed zero bytes when used as one of multiple parts in new Blob([...]) / new File([...]). The fix extracts a push_blob_part_bytes helper that dispatches on the store variant: in-memory bytes borrow or clone as before, path-backed files go through NodeFS::read_file, fd-backed files use a pread grow-until-EOF loop at the Blob's absolute offset, and S3-backed parts throw a clear error. ~120 lines of new Rust in src/runtime/webcore/Blob.rs plus ~70 lines of tests in blob.test.ts.
Security risks
None identified. Inputs are the caller's own file paths/fds; there is no untrusted-length parsing. Buffer sizing uses saturating_sub/saturating_mul/saturating_add and clamps against fstat size or the slice's concrete size. The one unsafe block (detach_lifetime on the borrowed bytes view) is gated on the same prescan invariant the pre-PR code relied on and is documented at both the call site and the helper.
Level of scrutiny
Medium-high. This is core Web API surface (Blob/File constructors) and native code with an unsafe block, cross-platform I/O, and a user-visible behavior change: constructing a Blob with a nonexistent file part now throws ENOENT synchronously instead of silently contributing nothing. It also introduces synchronous disk reads into a constructor that was previously pure over its arguments — that is the correct-per-spec behavior (the constructor is sync and "process blob parts" cannot be deferred), but it is an architectural choice a maintainer should confirm rather than something I should approve unilaterally.
Other factors
This PR has been through two rounds of my review; all five prior findings were addressed with follow-up commits and tests (fd-twice, ENOENT-after-.size, POSIX-only cursor check, procfs grow loop). The bug-hunting pass this run found nothing new. Test coverage is thorough across path/fd/slice/clone/Response.blob() variants and the error path. The outstanding comment-cop bot flags target 2-3 line invariant comments (the push_blob_part_bytes doc comment, the pread-vs-read_file rationale) that read as durable non-obvious content to me, but they are unresolved and the author should either trim or dismiss them.
ee3facf to
5e59f0f
Compare
5e59f0f to
2acd433
Compare
2acd433 to
dc6cee1
Compare
The multi-part join in from_js_without_defer_gc pushed blob.shared_view() for every Blob part, but shared_view() returns an empty slice for any non-Bytes store (Data::File, Data::S3). Every file-store Blob used as one part of a multi-part new Blob([...]) or new File([...]) therefore contributed zero bytes. The single-part fast path (new Blob([Bun.file(p)])) was unaffected because it shares the store instead of joining bytes. Fix: dispatch on the part's store. Data::Bytes keeps the shared_view() path. Data::File reads the file synchronously (the constructor is sync and the spec's "process blob parts" requires the bytes at construction time): path stores use NodeFS::read_file; fd stores use pread so repeated use of the same fd part reads the correct window regardless of cursor position. Data::S3 throws rather than silently contributing zero bytes, since there is no synchronous S3 read path. A nonexistent file part now throws ENOENT from the constructor instead of being silently ignored. Fixes #25851
dc6cee1 to
2f30b03
Compare
There was a problem hiding this comment.
The over-allocation concerns from the last round are addressed in 2f30b03 — both the empty-fd-huge-slice and sparse-file triggers now go through try_reserve* with the 8 GiB cap and surface ENOMEM instead of aborting, and the grow loop is likewise fallible. I didn't find anything further.
Deferring to a human for the design/behaviour sign-off rather than the mechanics: new Blob([...]) now does synchronous disk I/O and throws (ENOENT for a missing path, an error for S3 parts) where it previously returned successfully — that's the right fix for #25851 but is a user-visible change a maintainer should confirm.
What was reviewed:
- Fd arm buffer sizing: traced empty-file+huge-slice (starts at 8K, first pread returns 0) and sparse
st_size(capped at 8 GiB viatry_reserve_exact, grow loop viatry_reserve) — no infallible allocation remains. 8 << 30literal is inferred asusizevia.min(), so no i32 overflow on 64-bit targets.- Path arm:
push_clonedbeforebuf.destroy(), so no UAF; encoding fixed toBuffer. - Both call sites (array iterator and deferred-stack arm) route through the helper; the deferred arm passes
borrow_bytes=falsematching the prior always-copy behaviour.
Extended reasoning...
Overview
Adds push_blob_part_bytes (~110 lines) to src/runtime/webcore/Blob.rs so file-backed Blob parts in a multi-part new Blob([...]) contribute their bytes instead of being silently dropped. The Path arm delegates to NodeFS::read_file; the Fd arm hand-rolls a pread loop with fstat-derived sizing, an 8 GiB initial cap, fallible try_reserve*, and grow-until-EOF for st_size==0 virtual files. S3-backed parts throw. Adds ~70 lines of tests in blob.test.ts covering path/fd, slices, repeated fds, past-EOF slices, empty-fd huge slices, structuredClone, Response.blob(), and ENOENT.
Verification of last round's fix
I traced 2f30b03 against both triggers from my previous review:
- Empty file +
.slice(0, 1e12):file_len==0→initial = 8192.min(cap) = 8192, firstpreadreturns 0, loop breaks. Test added. - Sparse file (
st_size=1e12):initial = cap = 1e12, buttry_reserve_exact(initial.min(8<<30))caps at 8 GiB and throwsENOMEMon failure; the grow loop'stry_reserveis likewise fallible. Novec[...]or infallibleresize-past-capacity remains.
The resize calls follow a successful try_reserve* for the same delta, so they cannot reallocate. new_len - buf.len() cannot underflow because new_len = buf.len().saturating_mul(2).min(cap) and the branch is only entered when buf.len() < cap.
Security risks
The user-controlled-size → infallible-allocation abort was the security-relevant issue; it is now closed. The Path arm inherits read_file's existing safeguards. No path traversal or injection surface — inputs are already-constructed Bun.file handles.
Level of scrutiny
High. This is a Web-standard constructor (new Blob/new File) that now performs synchronous disk I/O and can throw where it previously could not. Four prior review rounds each found a real bug (Windows pread cursor semantics, procfs st_size==0, realloc churn, two over-allocation aborts), which is evidence the code is subtle enough to warrant a maintainer's eyes even though this pass found nothing.
Other factors
All prior inline threads are resolved and the comment-cop paragraph-comment flags were addressed (comments in the current diff are terse). Test coverage is broad and each earlier finding has a corresponding regression assertion. The remaining question is design intent — whether synchronous read-on-construct (and the new throw behaviour) is the approach the maintainers want, versus e.g. keeping the store lazy and materializing on first read. That's a maintainer call, not a correctness bug, so I'm deferring rather than approving.
…blocking open The in-memory arm is back at the two call sites as main has it. A part whose shared_view() is empty and whose store is a file or S3 goes to one cold function, so an in-memory part costs one emptiness check more than main. The path arm no longer calls NodeFS::read_file. It opens with O_NONBLOCK, fstats, and shares the fd arm's pread loop, so a FIFO with no writer can not block the JS thread, and the file is copied once less. A directory still throws EISDIR. A pipe, socket or device now throws a TypeError that names the async readers.
…s and the FIFO writer A path is now stat'ed first. A pipe, socket or device is refused without open(2), because the open of a FIFO wakes its writer, and the writer then fails before the async read that the error message names can run. Tests: every byte value and UTF-8 text, a part against the same part read alone (slices, a file that grows or shrinks), the path in ENOENT and EISDIR, an S3 part, a procfs file whose stat size is 0, and a FIFO whose writer still delivers to the async read after the constructor threw.
|
Pushed a rework on top of a merge with main (0832a6d, then 14a5bbd for the FIFO test on macOS).
The PR body has the measured costs (read time, peak memory, instruction counts, binary size) and the 3 review concerns that I did not change, each with the reason. |
There was a problem hiding this comment.
Beyond the inline findings, I also checked the fd lifetime in push_file_part (the opened bun_sys::File is closed on Drop on every return path, and caller-owned fds are read with pread only, never closed) and the allocation bounds from the earlier round (slice size is now clamped to st_size, the initial reservation is capped at 8 GiB and goes through try_reserve, so the over-allocation abort is closed) — both look correct.
Extended reasoning...
The change adds a synchronous file-read helper to the multi-part Blob/File constructor in src/runtime/webcore/Blob.rs and a test block in test/js/web/fetch/blob.test.ts; it touches filesystem access from user-supplied paths and fds but no auth or credential surface. Three verified findings are posted inline (synchronous throw instead of rejection for Bun.write, event-loop stall on large files, and the FormData serializer not receiving the same fix), so approval is not appropriate; this note only records what else was examined and ruled out.
Findings marked 🟡 are optional suggestions and need no follow-up push.
On macOS the child never exited after `await Bun.file(fifo).text()`, so the test timed out. The test is about the constructor: it must refuse the FIFO without an open, so that the writer keeps its bytes for the next reader. `cat` is that reader now. The pipe case (`Bun.stdin`) is a test of its own.
This branch has not been deployed
2 participants
Fixes #25851
Problem
Bun.file(p)or its slice) gives zero bytes as one part ofnew Blob([...]). No error occurs, and.sizeagrees with the wrong data.from_js_without_defer_gc(src/runtime/webcore/Blob.rs) pushesblob.shared_view(), which is empty for a store that is not in-memory bytes.Fix
push_file_part: open withO_NONBLOCK,fstat, thenpread.ENOENT,EACCESorEISDIR. A pipe, device or S3 part throws aTypeErrorand is not opened.test/js/web/fetch/blob.test.tson Linux and Windows x64 (main fails the 9 new tests), and 5 related suites.Background
Downsides
Bun.write(dest, [...]).Notes
What a user sees
Each way to get a file store is affected:
Bun.file(path),Bun.file(fd),structuredClone(Bun.file(p)), aBunFilefrompostMessage,await new Response(Bun.file(p)).blob(), and slices of these.Errors
ENOENT, syscallopen, with the pathEACCES, syscallopen, with the pathEISDIR, syscallread, with the pathBun.stdinas a pipeTypeError:Blob parts backed by a pipe, socket or device cannot be read synchronously; await .bytes() or .arrayBuffer() firstTypeError:Blob parts backed by S3 cannot be read synchronously; await .bytes() or .arrayBuffer() firstA path is checked with
statbefore it is opened. The open of a FIFO wakes a writer that waits for a reader, and the writer then fails or loses its bytes. The test for the FIFO runs a writer, lets the constructor throw, and then reads the bytes of the writer withcat.The errors are thrown by the call that joins the parts. For
new Blob,new Fileandnew Response([...])that is the constructor.Bun.write(dest, [...])joins the array before it makes a promise, so it throws synchronously and does not reject:Bun.write(out, Bun.file(missing))ENOENTENOENTBun.write(out, ["h", Bun.file(existing)])hhSRCBun.write(out, ["h", Bun.file(missing)])hENOENTsynchronously,outis not createdBun.write(out, [{ toString() { throw } }])Instruction counts (release builds of main
8d36bff51and of this branch, Linux x64, JIT off)perf_event_openis not permitted in the build container andvalgrindis not installed. The counts come from a ptrace single-step counter. It counts the instructions of the main thread between two marker syscalls. Repeated runs differ by less than 60 instructions in 6 million. Each value is the slope between 1,000 and 2,000 calls.new Blob(["a", "b"])new Blob([u8, "a"])new Blob([blob, "a"])new Blob([file, "a"])(in-memoryFile)new Blob([slice, "a"])new Blob([16 in-memory blobs])The earlier head of this PR cost 30 more for each in-memory part.
Binary size:
bun80,840,224 to 80,848,416 bytes (+8,192).bun-profile169,397,392 to 169,408,152 bytes (+10,760).Synchronous read of a file part (release, warm page cache, one constructor call for each process, 15 runs each)
An empty script has a peak RSS of 28 MB. The peak is 2 times the file: the read buffer, and the joined buffer of the new Blob.
A part against the same part read alone
A grid of 912 cells compares
await part.bytes()withawait new Blob(["", part]).bytes(). It has 8 ways to make the part, 19slice()argument lists, a nested slice, and a file that grows or shrinks after the part is made.Bun.file(p).slice(-3). The slice itself is wrong on main ("ABC"for the file above). Bun.file().slice(): count a negative index back from the file's real end #41257 corrects that at slice time.Review: not changed
ENOMEM, but Linux overcommit can let the OOM killer act first.fs.readFilehas a RAM guard for this. This PR does not add one: if that guard regresses, its test reads a file as large as the RAM.fstatgave after the open. A file that grows during the read gives a valid snapshot in both readers.bun_sys::Filedoes not close fds 0 to 2 on drop. If user code closed a stdio fd, the open for a part can get that fd and keep it. The limit is 3 fds, and the rule belongs tobun_sys::File.NodeFS::read_file: a FIFO entry blocks, an fd entry reads from the cursor, an S3 entry gives no bytes. It readsBun.stdinas a pipe today, so a move topush_file_partchanges behaviour and needs its own PR.Platform notes
preaddoes not move the cursor of the fd of the caller. On Windows, a positionedReadFileon a synchronous handle moves it, so the test asserts the cursor only on POSIX.Bun.file()of a file that is embedded in a compiled executable has an in-memory store, so it never reachespush_file_part.Bun.file("/dev/null"),NULon Windows) throws theTypeErrorat this head. main gave an empty part for it, which is the right result. Blob: a null device part is an empty part #43944 is stacked on this PR and makes the null device an empty part again.Tests
bun bd test test/js/web/fetch/blob.test.ts(debug, ASAN): 119 pass, 1 skip. The skip is theEACCEStest, which does not run as root. As uid 65534 the block gives 10 pass.await Bun.file(fifo).text(), and the child never exited there (Buildkite build 120425, both macOS lanes). The constructor is not involved, so the test now reads withcat. The lazy reader on macOS is a separate problem.blob-array-fast-path.test.ts,blob-cow.test.ts,structured-clone-blob-file.test.ts,bun-file.test.ts,body-clone.test.ts(149 pass).Earlier shape of this PR
The first version sent each Blob part through one helper,
push_blob_part_bytes. It read a path withNodeFS::read_file(a blocking open, and one more copy of the file) and an fd with thepreadloop. Review rounds on that version added:preadfor an fd so that a repeated part does not read from the end, thest_size == 0case for virtual files such as procfs, a fallible allocation with a first reserve of 8 GiB at most, and a clamp of a slice to the size of the file.no test proof · iteration 11 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/web/fetch/blob.test.ts