Repository navigation
Conversation
|
Status: reproduced on bun 1.4.3 (canary 367d939). |
|
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; 4 remain after this review. WalkthroughRegular files that report a size of zero now use adjusted size limits and JavaScript-size checks during reads. Upload tests cover procfs files, ChangesZero-Size File Uploads
Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to The zero-size file upload change is ready to merge after normal checks; no actionable issue remains from this review. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
A procfs file is a regular file that reports st_size 0 and has content. fetch() passes the size of the blob as max_size, and any max_size stopped the read at the 256 KiB scratch buffer plus 16 bytes. An upload of a 12 MB procfs file sent 262160 bytes with a matching Content-Length. Slices were cut at the same place. For a caller with max_size, a regular file with st_size 0 now has an unknown size: the read continues until EOF, clamped by the caller's window. That read has the size limit of a JS buffer, which fs.readFile already has, so a file with no practical end (/proc/self/pagemap) fails with ENOMEM. A file with st_size above 0, a device and a FIFO take the same path as before.
f8ea6fc to
67904c0
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline findings, I also checked that the new size_unknown gate at node_fs.rs:7212 is keyed on S_ISREG, so device and FIFO bodies (/dev/zero, pipes) and regular files with a non-zero st_size keep the exact pre-PR has_max_size behavior, and that fs.readFile/readFileSync (no max_size) are unaffected since both new conditions require args.max_size.is_some().
Extended reasoning...
The change is confined to the post-fstat sizing logic of NodeFS::read_file in src/runtime/node/node_fs.rs plus three subprocess tests in test/js/bun/http/fetch-file-upload.test.ts; it touches no auth, crypto, or injection surface. Inline findings were posted on the slice/growth-arm interaction and on serial test execution, and one further verified finding was withheld, so approval is not appropriate; this note only records the sibling paths that were checked and found unchanged.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟡
src/runtime/node/node_fs.rs— Uploading a slice of a procfs file can now fail with ENOMEM after every byte of the window was already read, and always pays one useless buffer doubling. At node_fs.rs:7286 the growth arm runs whentotal == max_sizebecausehas_max_sizeis false for a size-0 regular file, sotry_reserve(8192)at :7293 doubles a full buffer (e.g. 512 MiB to 1 GiB) althoughupperat :7271 can never exceedmax_size. If that allocation fails the read returns ENOMEM/read instead of the complete window. Fix: skip the growth arm (and break) oncetotal >= max_size, i.e. only grow when the window still has room, so a slice window on a size-0 file never allocates beyondmax_size.Why this was flagged
Trigger:
fetch(url, { body: Bun.file(procfsPath).slice(0, N) })(fetch.rs:1655 passesmax_size = Some(N)) or a FormData part with a window (Blob.rs:3689) on a regular file whosefstatreportsst_size == 0.size_unknownat node_fs.rs:7212 makeshas_max_sizefalse at :7213. The read that fills the buffer to exactlymax_sizereturnsamt != 0withtotal > size(size is the pre-stat total, :7217), so the arm at :7286 runs:truncatethentry_reserve(8192)at :7293 withlen == capacitytriggers Vec amortized growth to 2x capacity.upperat :7271 is capped atmax_size, so the new capacity is never used; the next read is empty and breaks at :7304. On the base branchhas_max_sizewas true for every blob caller, so this arm never ran and no allocation beyondmax_size + 16happened. Consequence: transient 2x peak memory for the window, a shrinking realloc ininto_boxed_sliceat :7341, and, when the doubled allocation fails, a spurious ENOMEM for an upload whose bytes were all already read. The per-read check at :7275 does not guard this because it measurestotal, not…Verification: nit. Trigger: a blob caller (fetch.rs:1655
rf_args.max_size = Some(blob_size), or Blob.rs:3689) reading a regular file whose fstat reports st_size 0 (procfs) with a slice window W larger than the 256 KiB pre-stat scratch and file content >= W, when W leaves less than 8192 bytes of spare capacity in the doubling sequence (all power-of-two windows from 1 MiB to 64 MiB qualify). Mechanism…
| // it on the JS thread. The read must go to EOF, or to the end of a slice, and | ||
| // not stop at the 256 KiB scratch buffer. The child reads its own | ||
| // /proc/self/environ, which three 100 KB variables make larger than that. | ||
| test.skipIf(!isLinux)("uploads a procfs file and its slices, not the stat size", async () => { |
There was a problem hiding this comment.
🟡 nit (optional): the three new subprocess tests run serially, so the file gets slower on the debug/ASAN lane than it needs to be. Each spawns its own child and Bun.serve({port: 0}) and shares no state, and the sibling slice tests in this file at test/js/bun/http/fetch-file-upload.test.ts:168 already use test.concurrent. Fix: declare the three new cases with test.concurrent (and test.concurrent.skipIf(...)), covering the tests at lines 257, 307 and 325.
Why this was flagged
The new tests at test/js/bun/http/fetch-file-upload.test.ts:257, :307 and :325 each spawn a child through runChild and wait for it to finish before the next starts. The procfs child reads and uploads a >270 KB body four times and the pagemap child reads 1 MiB from /proc/self/pagemap before failing; on a debug+ASAN build these serial waits add up. test/CLAUDE.md asks that independent subprocess tests in one file be test.concurrent, and the existing slice tests in this file at :168 and :201 follow that. Nothing is functionally wrong; the base file simply has fewer tests.
Verification: nit. Triggering condition: any run of this file (notably the debug/ASAN lane), where the three new subprocess tests execute one after another. Verified in the diff of /home/claude/bun/test/js/bun/http/fetch-file-upload.test.ts: the new cases are declared test.skipIf(!isLinux)("uploads a procfs file and its slices, ...") (L257), test.skipIf(isWindows)("upload of a device with no EOF ...")…
| let size_unknown = sys::S::ISREG(stat_.st_mode as u32) && stat_.st_size == 0; | ||
| let has_max_size = args.max_size.is_some() && !size_unknown; |
There was a problem hiding this comment.
🟣 pre-existing, not blocking: pre-existing: a user who touches file.size (or .exists()) before uploading a procfs file still sends an empty body, exactly as on the base branch. The blob getter calls resolve_size, which sets size to the stat's 0 for a regular file, so fetch.rs:1655 passes max_size = Some(0). At node_fs.rs:7213 size_unknown is then true but max_size 0 still caps upper at 7271, so zero bytes are read. Fix: for a size_unknown regular file treat a caller bound that came from st_size 0 (max_size == Some(0), or resolve to MAX_SIZE in Blob.rs:5805 for procfs) as unbounded, so both the resolved and unresolved blob reach EOF, while keeping a genuine slice(0,0) empty.
A small fix can ride a push you are already making; otherwise a short reply is enough.
Why this was flagged
A script does const f = Bun.file("/proc/kallsyms"); f.size; await fetch(url, { method: "POST", body: f }). Blob.rs:2081 get_size calls resolve_size; Blob.rs:5805 sets file.max_size = st_size = 0 because the file is S_ISREG, and Blob.rs:2133 sets blob.size to 0. fetch.rs:1655 passes rf_args.max_size = Some(0). In read_file, node_fs.rs:7117 makes pre_stat_len 0 so nothing is read before fstat; node_fs.rs:7212-7213 compute size_unknown = true and has_max_size = false, but max_size stays 0 so node_fs.rs:7271 gives upper = 0 and the read returns 0 bytes, ending the loop with total 0. The server receives content-length 0. The base branch does the same, and the PR's own description only claims the unresolved case; the new size_unknown logic is the natural place where this sibling input is decided, and it still yields an empty upload.
Verification: pre-existing (base fails identically by the same route; the PR's fix does not reach this path). Trigger: any read of file.size (or file.exists(), which calls the same resolve_size) on a Bun.file() over a procfs-style regular file before it is used as a fetch body. Mechanism verified: - A fresh Bun.file() blob starts with size = MAX_SIZE… | pre-existing. Trigger: the script observes…
Problem
fetch(url, { body: Bun.file("/proc/kallsyms") })uploads exactly 262160 bytes of a 12 MB file, with no error. A procfs file is a regular file that reportsst_size0..slice(1)and.slice(0, 1 << 20)are cut at the same place.max_size = Some(blob.size)toNodeFS::read_file(fetch.rs:1655). AnySomesetshas_max_size(node_fs.rs:7211), which stops the read at the 256 KiB scratch buffer.Fix
read_filereads a regular file withst_size0 to EOF. The caller's window still clamps a slice.fs.readFilehas./proc/self/pagemaphas no practical end and fails withENOMEM.st_sizeabove 0, a device and a FIFO are read as before.test/js/bun/http/fetch-file-upload.test.ts. Two of three new tests fail on bun 1.4.3.Background
read_filefills a 256 KiB scratch buffer, callsfstat, then readsst_sizebytes. Without a caller bound it continues to EOF (readFileSync not reading values in /proc. #1220).MAX_SIZEsentinel". It left slices cut and unbounded every regular file. Node'sreadFileuses thest_size == 0rule.Downsides
/proc/self/pagemapreads 4 GiB on the JS thread, then fails (7.4 s, 4.7 GB RSS, debug build). Before, it sent 262160 bytes at once.fs.readFileSyncdoes the same on bun 1.4.3.Notes
Where this comes from. #41590 first held three sites. A review of it found that "read a regular file to EOF" has no bound for
/proc/self/pagemap, that slices were still cut, and asked for one site per PR. This PR is theread_filesite. #41590 keeps the FormData serializer and #43965 has the module loader.Upload sizes (bytes the server receives. bun 1.4.3 canary 367d939, and a debug build of this branch):
/proc/kallsyms(12340250 B)/proc/kallsyms.slice(1)/proc/kallsyms.slice(0, 1 << 20)/proc/version(219 B)/dev/zero/proc/self/pagemapENOMEMafter 4 GiBThe bound. A blob caller that reaches the new path gets
limit_size_for_javascript, the check thatfs.readFilealready uses:ENOMEMwhen the bytes read pass the size limit of a JS buffer (u32::MAX), or on Linux the size of the memory. The test lowers the limit to 1 MiB withsetSyntheticAllocationLimitForTesting. A smaller fixed bound for this path is a policy choice and is one line to change.Sibling readers and
/proc/self/pagemapon bun 1.4.3:fs.readFileSyncthrowsENOMEMafter 6.8 s at 4.0 GB RSS.await Bun.file(p).bytes()has no bound (killed at 11.7 GB RSS after 16 s).No cancel. The two blob callers read on the JS thread with no
AbortSignaland no VM stop check. #36259 and #40207 add those tofs.readFile. They do not reach the blob callers.FormData. The serializer calls
resolve_size()first, so on this branch alone a procfs part is still empty. With #41590 and this PR together, a part over 256 KiB has its full content, and a part for/proc/self/pagemapthrowsENOMEMfrom the serialize. A test for that pair goes into the PR that lands second.#41488 changes the same line to
max_size != BLOB_SIZE_MAXand streams a device or FIFO body before it reachesread_file. It is in conflict with main. The/dev/zerotest here pins the bound that a device has when it does reachread_file.Syscall counts. Counted with a ptrace counter on debug builds, because
straceis not installed in the container. An upload of an ordinary 20000 B file: openat, fstat, read x2, close on both builds. An upload of an empty file: openat, fstat, read, close on both builds.Suites run on the debug build.
test/js/bun/http/fetch-file-upload.test.ts(14 pass),test/js/node/fs/fs.test.ts,test/js/web/fetch/blob.test.ts,test/js/bun/util/bun-file-read.test.ts,test/js/web/html/FormData.test.ts.cargo check -p bun_runtimepasses for linux, aarch64-apple-darwin and x86_64-pc-windows-msvc.