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 (4)
Included review availability: Your plan provides up to 10 included reviews per hour; 4 remain after this review. WalkthroughResolver readers now read regular files to EOF when the reported size is smaller than the probe. NodeFS treats ChangesFile read size handling
Suggested reviewers: Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to The change addresses the reported procfs reads while retaining bounds for device streams. No issue identified here needs to block merging after normal checks. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Status: reproduced on bun 1.4.3. A FormData part for This PR now holds only the FormData serializer change. Earlier pushes also changed |
|
Updated 3:06 AM PT - Sep 25th, 2026
❌ @robobun, your commit 73cc514 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 41590That installs a local version of the PR into your bun-41590 --bun |
There was a problem hiding this comment.
Findings marked 🟡 are optional suggestions and need no follow-up push.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟡
src/runtime/webcore/Blob.rs— nit: The doc comment on the localFormDataEntrymirror sayson_entry"needs&mut Blobto callresolve_size()", but this PR removes theresolve_size()call fromon_entry(andresolve_sizetakes&selfanyway). The stated justification for drivingDOMFormData__forEachwith a mutable-blob variant instead of the stockbun_jsc::dom_form_data::FormDataEntryis now dead. Fix: update or delete the comment; if nothing else inon_entryneeds&mut Blob, drop the local mirror and use the immutableFormDataEntryper REVIEW.md's "delete dead code in the same PR that makes it dead".Extended reasoning...
The removed hunk at Blob.rs:3763-3766 was the only reason cited (Blob.rs:3574-3576) for the
FormDataEntry<'a>::File { blob: &'a mut Blob }local enum and its customDOMFormData__forEachdriver. After the removal,on_entryreads onlyblob.content_type_slice(),blob.store.get(),blob.offset.get()andblob.size.get()— all&self/Cellaccessors. The comment now points a future reader at a call that no longer exists, and the&mutvariant it justifies is left as unexplained duplication ofbun_jsc::dom_form_data::FormDataEntry. No runtime failure; this is documentation/dead-code debt this PR introduces (the comment's reasoning was already shaky pre-PR sinceresolve_sizeisfn(&self)at Blob.rs:2188, but the referenced call did exist on the base branch).Verification: nit — The doc comment at src/runtime/webcore/Blob.rs:3573-3576 reads "
FormDataContext::on_entryneeds&mut Blobto callresolve_size(), so we drive the C++DOMFormData__forEachdirectly with this mutable variant." This PR's only change to Blob.rs is deleting theblob.resolve_size()call at (former) lines 3764-3766 fromon_entry; the comment now cites a call that no longer exists.…
9974ddf to
ad49337
Compare
ad49337 to
a92c100
Compare
A procfs file is a regular file that reports st_size 0 and has content. The multipart serializer called resolve_size() before the read. That stored the stat size 0 as the size of the blob, so the read got max_size = Some(0) and the part was empty. The serializer no longer resolves the size. A whole-file part reaches NodeFS::read_file with the "size unknown" sentinel, as a fetch body does. read_file is unchanged: a part is still bounded by its scratch buffer when the stat size is wrong, and a device or FIFO is read as before. The FormDataEntry doc comment named the removed call and a type that does not exist.
a92c100 to
73cc514
Compare
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 finding, I also checked the new /dev/zero child test for a hang: Some(MAX_SIZE) is still is_some() at src/runtime/node/node_fs.rs:7211, so both the pre-stat scratch read and the post-fstat loop stay capped, and the sliced-part test is unaffected because get_slice_from stores an explicit offset/len that never went through resolve_size(). One scope note: the diff at HEAD contains only the Blob.rs hunk and the FormData test, so the node_fs.rs, resolver/fs.rs and fetch-upload changes the description and title cover are not in this push.
Extended reasoning...
The push removes the resolve_size() call before the FormData file-part read in src/runtime/webcore/Blob.rs and adds three FormData tests; no security-sensitive surface is touched. The scope of the diff is much narrower than the PR title and description, which still describe changes to read_file, the resolver loader and fetch uploads that are absent here, and the inline finding shows the FormData fix only covers procfs files up to the 256 KiB scratch buffer.
Still open from earlier reviews (1):
- Unresolved: 1 minor or pre-existing.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟣
src/runtime/webcore/Blob.rs— A FormData part (or fetch body) for a procfs file over 256 KiB is still cut at 262160 bytes after this change; the PR text says otherwise. Blob.rs:3683 now passes the unresolvedMAX_SIZEsentinel asmax_size, and node_fs.rs:7211has_max_size = args.max_size.is_some()treats anySomeas a real bound, so the loop at node_fs.rs:7282 never grows past the scratch read. The describedread_filechange ("reads to EOF for the whole-file sentinel when the file is regular") is not in this diff, nor are the fetch/text-loader tests. …Why this was flagged
…Fix: in
read_filetreat a whole-file sentinel bound as "no bound" for regular files (covering both Blob.rs:3683 and fetch.rs:1655), and add a >256 KiB procfs case to the test;/proc/versionis consumed entirely by the pre-stat read at node_fs.rs:7121 and cannot detect this.formData.append("f", Bun.file("/proc/kallsyms"))thennew Response(formData).bytes(), orfetch(url, { body: Bun.file("/proc/kallsyms") }), on a host where that file exceeds 256 KiB. Withresolve_size()gone, Blob.rs:3683 setsrf_args.max_size = Some(blob.size.get())where the size is stillMAX_SIZE; fetch.rs:1655 does the same on the base and on this branch. In node_fs.rs:7211has_max_size = args.max_size.is_some()is true for that sentinel, so after the 256 KiB pre-stat read (node_fs.rs:7115-7131) the post-fstat loop reads only the 16 spare bytes ofinitial_cap(node_fs.rs:7233-7237) and the growth arm at node_fs.rs:7282 is skipped because!has_max_sizeis false; the next read gets an empty slice, returns 0, and the part is 262160 bytes. The base branch serialized 0 bytes for this…Verification: normal — triggered whenever an unsliced Bun.file() FormData part refers to a regular file whose st_size is 0 (procfs) but whose content exceeds 256 KiB, e.g. /proc/kallsyms. Mechanism verified:
git diff --statshows only src/runtime/webcore/Blob.rs and test/js/web/html/FormData.test.ts changed — theread_file"regular file reads to EOF for the whole-file sentinel" change, the fetch change…
Problem
FormData.append("f", Bun.file("/proc/version"))serializes an empty part, with no error. A procfs file is a regular file that reportsst_size0 and has content.Bun.file(p).text()returns the content.resolve_size()before the read (Blob.rs:3666). That stores the stat size 0 as the size of the blob. The read then getsmax_size = Some(0)(Blob.rs:3689) and reads nothing.Fix
resolve_size(). A whole-file part reachesNodeFS::read_filewith the "size unknown" sentinel, as afetchbody does.read_fileis unchanged. A device, a FIFO and a file with no end stay bounded at one scratch buffer.test/js/web/html/FormData.test.ts, three new tests. The procfs test fails on bun 1.4.3.Background
Blob.size == MAX_SIZEmeans "unknown, the whole file".resolve_size()replaces it with the stat size and stores that on the blob.read_filefirst fills a 256 KiB scratch buffer, before anystat. A file that ends inside the buffer needs no size.read_filerule (fetch: read a Bun.file() body with st_size 0 to EOF, up to the JS size limit #43964) and the module loader (Module loader: read a file with st_size 0 to EOF when it fills the 16 KiB probe #43965) need a bound for/proc/self/pagemap, so each has its own PR.Downsides
st_size0 is cut at 262160 bytes. Before, it was empty. fetch: read a Bun.file() body with st_size 0 to EOF, up to the JS size limit #43964 removes the cut.Notes
History of this PR. The first version changed three sites: the FormData serializer,
NodeFS::read_fileand the module loader. Two problems showed up after the push.fetchand FormData withBun.file("/dev/zero")read without bound (RSS passed 3 GB in 3 s on a debug build). A condition onS_ISREGfixed that./proc/self/pagemapisS_ISREGwithst_size0 and yields hundreds of GB. With theread_fileand loader changes, an upload, a FormData part or a text import of it passed 1.5 GB RSS in 2 to 4 s. bun 1.4.3 returns at once. The same review found that.slice(1)and.slice(0, 1 << 20)of a procfs file were still cut at 262160 bytes.So this PR now holds only the serializer change, which adds no read past the bound that main has. The other two sites need a decided bound. They are #43964 (
NodeFS::read_file) and #43965 (module loader).Bytes inside the part (bun 1.4.3 canary 367d939, and a debug build of this branch):
/proc/version(219 B)/proc/kallsyms(12 MB)/proc/self/pagemap/dev/zeroSyscall counts. Counted with a ptrace counter on debug builds of main (4227e46) and of this branch, because
straceis not installed in the container. Only calls on the part file count.The
newfstatatthat goes away is thestatinresolve_size().The 16 bytes.
read_filesizes its buffer as thefstatsize plus 16 (node_fs.rs,initial_cap). With a caller bound it does not grow past that. On main the bound was the size thatresolve_size()stored, so the part ended exactly there.Related open PRs. #43910 (draft) stops
resolve_size()from storing a stat size on a file blob. The two changes are independent and merge in either order. #41488 streams a device or FIFO body offetch. #41593 is the body stream site.Not covered here.
--env-fileof a procfs file loads nothing (src/dotenv/env_loader.rs,read_env_file_contents). Node loads it. That is a separate report.Suites run on the debug build.
test/js/web/html/FormData.test.ts(152 pass), and on the earlier three-site versionblob.test.ts,bun-file-read.test.ts,fetch-file-upload.test.ts,node/fs/fs.test.ts.Self-review. Three rounds. Round 1 moved the fix from the call sites to
read_file. Round 2 (another session) found the/dev/zeroread. Round 3 found the/proc/self/pagemapread, the slices, and asked for one site per PR. This version is the result.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/html/FormData.test.ts, test/js/bun/util/text-loader.test.ts, test/js/bun/http/fetch-file-upload.test.ts