Bun.file: stop a file's stat size from capping reads of the whole file - #33360
Bun.file: stop a file's stat size from capping reads of the whole file#33360robobun wants to merge 1 commit into
Conversation
A BunFile's `size` is both "how many bytes this view spans" and "how many bytes a read of it may return". `resolve_size()` caches the stat size onto the blob, and procfs/sysfs/cgroupfs regular files report `st_size == 0` while reading more, so `await f.exists()` or reading `f.size` turned every later read of that BunFile into an empty one. Track whether `size` is a bound the caller asked for with `Blob.slice()`, and read to EOF when it isn't. `read_file`'s "skip reading an empty regular file" fast path went with it: it consulted the store's cached mode, so it only fired once something had already statted, and it is unsound for a file whose stat size lies. `node_fs`'s `read_file` has handled this since #1220.
|
Warning Review limit reached
Next review available in: 3 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 (6)
Comment |
|
Found 4 issues this PR may fix:
🤖 Generated with Claude Code |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any bugs, but this threads a new semantic (size_is_explicit / read_limit()) through the core Blob struct and five separate file-read paths, removes a fast path in ReadFile, and explicitly overlaps with #32794 on resolve_size() — worth a human look, especially for merge-order coordination.
Extended reasoning...
Overview
This PR fixes Bun.file() reads being capped (often to zero bytes) by a cached stat size for files whose st_size lies — procfs/sysfs/cgroupfs regular files. It also fixes the inverse: sliced file blobs having their explicit bounds widened by resolve_size(). The fix adds a size_is_explicit: Cell<bool> field to the #[repr(C)] Blob struct in src/jsc/webcore_types.rs and a read_limit() accessor that returns MAX_SIZE unless the caller set an explicit bound via slice(). Every file-read path — do_read_file/do_read_file_internal in Blob.rs, FileReader::max_size in ReadableStream.rs, node_fs::read_file's max_size in both FormDataContext and fetch_impl — is switched from size.get() to read_limit(). resolve_size()/resolved_size() gain an early-return guard for explicit bounds on file stores. The POSIX ReadFile::run_async_with_fd fast path that skipped reading when size == 0 && is_regular_file(store.mode) is removed (it consulted the store's cached mode, not the fresh fstat, which is why the bug only fired after exists()). Nine new tests cover procfs reads, streaming, FormData, fetch upload, and slice bounds.
Security risks
None identified. The change loosens a read cap from a stat-derived hint to EOF for whole-file reads, and preserves caller-supplied slice bounds. There's no new user-controlled input parsing, no path handling changes, and no auth/crypto surface. The removed fast path traded correctness for one read() syscall on genuinely-empty files, which is not security-relevant.
Level of scrutiny
High. This is a semantic redesign of what Blob.size means for file-backed blobs, threaded through a #[repr(C)] struct (the JS wrapper's m_ctx payload — Rust-owned so field additions should be layout-safe on the C++ side, but every Rust-side struct-literal construction site must carry the new field, and dupe_with_content_type/structured-clone paths must propagate it correctly). It touches five runtime files across Blob, streams, fetch, and FormData. It removes an optimization. And the description explicitly calls out an overlap with open PR #32794 on the File arm of resolve_size()/resolved_size(), with a note that whichever lands second should adopt this PR's version of that arm — that's a merge-coordination decision a human should make.
Other factors
The PR description is exceptionally thorough (repro, root cause, per-path fix rationale, before/after test output, and a list of 12 adjacent test files verified green plus rust:check-all on all targets). The test coverage is good: procfs cases are skipIf(!isLinux) and the slice/empty/regular-file cases run everywhere. The Windows ReadFileUV fast path is deliberately kept (it uses the fresh fstat's mode and NTFS doesn't under-report), which is a reasonable asymmetry but worth a reviewer confirming. No prior human or bot review comments exist beyond build-status noise.
1 participant
Bun.file(p)reads nothing once anything has resolved the file's size, for any file whosestatsize is a lie. procfs, sysfs and cgroupfs regular files all reportst_size == 0and still read hundreds of bytes, andexists()-then-read is the canonical usage sequence, so this reads as "procfs is flaky in Bun".Repro
Reading
f.sizedoes the same..text(),.bytes(),.arrayBuffer(),.json()and.stream()are all affected, and three paths are broken on a fresh blob too, because they resolve the size themselves:The same conflation widens a slice.
Body::to_readable_streamcallsresolve_size()unconditionally, and theFilearm overwrites a slice's concrete length withmax_size - offset:Cause
Blob.sizemeans two different things at once: the number of bytes the view spans, and the upper bound on a read of it. For a file-backed blob the first is only ever astathint, butresolve_size()caches that hint intosize, and every file read path then usessizeas itsmax_length:Blob::do_read_file/do_read_file_internalpass it toReadFileReadableStream::from_blob_copy_refpasses it toFileReader::max_sizeFormDataContextandfetch_implpass it tonode_fs::read_file'smax_sizeReadFilecompounds it with a "skip reading an empty regular file" fast path that consults the store's cached mode rather than the mode it just fstat'd, so it only fires once something has already statted the file. That is precisely why a freshBun.file(p).text()works and a post-exists()one does not.node_fs'sread_filealready gets this right, and has since #1220:Passing
max_size: Some(..)there setshas_max_sizeand disables that tail read, which is how theFormDataandfetchupload sites lost the content.Fix
Separate the two meanings.
Blob::size_is_explicitrecords thatsizeis a bound the caller asked for withslice();Blob::read_limit()returnsMAX_SIZE("read until EOF") otherwise, and every file read path uses it.resolve_size()/resolved_size()no longer overwrite a caller-supplied bound for file stores, andReadFile's unsound fast path is gone — an empty regular file now costs oneread()returning 0, which is what the fresh-blob path already did.ReadFileUV(Windows) keeps its equivalent fast path: it tests the mode from the fstat it just performed, and NTFS regular files do not under-report their size..sizeis unchanged and still matchesfs.statSync(p).size, including0for procfs and for genuinely empty files.Relationship to #32794
#32794 fixes the slice widening above from the other side, by clamping a concrete
sizeagainst the store's size in both arms ofresolve_size(). For file stores that clamp is againstmax_size, which is the value that lies —Bun.file("/proc/version").slice(0, 10)would clamp tomin(10, 0) == 0. Leaving a caller-supplied bound alone, as this PR does, is the behaviour both want. The two PRs overlap only on theFilearm ofresolve_size()/resolved_size(); whichever lands second should take that arm from here. Everything else is disjoint.Verification
test/js/bun/util/bun-file-read.test.ts. The procfs cases areskipIf(!isLinux); the slice, empty-file and regular-file cases run everywhere.before (released 1.4.0) / after
Also green:
blob.test.ts,bun-file.test.ts,bun-file-fd-read.test.ts,empty-file.test.ts,structured-clone-blob-file.test.ts,blob-cow.test.ts,blob-write.test.ts,bun-serve-file.test.ts,fetch-file-upload.test.ts,FormData*.test.ts,body-stream.test.ts,streams.test.js.bun run rust:check-allpasses on all 10 targets.