Bun.file().slice(): reject delete()/writer()/write(), fix exists(); cap chardev slice reads - #35822
Bun.file().slice(): reject delete()/writer()/write(), fix exists(); cap chardev slice reads#35822robobun wants to merge 5 commits into
Conversation
|
Warning Review limit reached
Next review available in: 11 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 (5)
Comment |
|
Updated 7:46 PM PT - Jul 27th, 2026
❌ @robobun, your commit 78eecc1 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 35822That installs a local version of the PR into your bun-35822 --bun |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
On the duplicate flags:
The |
There was a problem hiding this comment.
Thanks for moving the check into write_file_internal — that closes the Bun.Image#write gap I flagged. I'm not auto-approving because this introduces a new user-facing throw (API decision: reject vs. implement positional writes) and overlaps with three open PRs (#35815/#31680/#33510) that a human should reconcile. The two comment-cop flags on the doc comment at L5310-5315 are also still outstanding.
Extended reasoning...
Overview
Two fixes for file-backed Blob.slice(): (1) a one-line read_off += read_amount in ReadFile::do_read_loop so chardev slice reads stop at exactly max_length instead of the next 64 KiB multiple, mirroring the existing Windows ReadFileUV path; (2) a new reject_sliced_file_destination guard called from write_file_internal and get_writer that throws a TypeError when the destination blob has offset != 0 on a file-backed store, preventing silent O_TRUNC clobber. Tests cover boundary sizes (1/4095/4096/4097/65535/65536/65537/1_000_000) and all three write entry points plus a false-positive guard (.size read stamping blob.size).
What changed since my last review
Commit 3f7b320 addressed my prior nit: the check moved from three JS entry points into write_file_internal's per-blob validation block, so Bun.Image#write and any future callers are covered. get_writer retains its own call since it doesn't route through write_file_internal. The redundant entry-point calls were removed.
Security risks
None identified. The read-side fix strictly reduces bytes returned (no over-read of adjacent memory — the over-read was of the device stream, not a buffer). The write-side change refuses an operation that previously silently destroyed data, which is a safety improvement.
Level of scrutiny
Medium. The read_off advance is mechanically obvious and mirrors Windows. The write-side rejection is a user-facing API behavior change — code that previously (incorrectly) succeeded now throws. That's the right call given silent data loss, but throw-vs-implement-pwrite is a design decision worth a human sign-off.
Other factors
- Three flagged overlapping PRs; the author's reply explains the non-overlap for #31680/#33510 and partial overlap with #35815, but a maintainer should decide landing order.
- The comment-cop bot flagged the 6-line doc comment on
reject_sliced_file_destinationtwice; those are unaddressed. It reads to me as legitimate why-documentation (invariant + intentionalunlink()exemption) rather than a workaround justification, but the repo's own linter disagrees. - Verified the
read_offcast is sound:read_amount: usize≤ 64 KiB per iteration, well withinSizeType. - Checked that keying only on
offset != 0(notsize) avoids the.size-read false positive the test covers, and thatslice(0, n)destinations still pass through (offset 0) — those still O_TRUNC but at least write from position 0, which is arguably intentional.
There was a problem hiding this comment.
Thanks for moving the check into write_file_internal — that closes the Bun.Image#write gap I flagged earlier, and blob.writer() keeping its own call is right since it doesn't route there. I didn't find any correctness issues in this revision, but deferring to a maintainer for the API-surface decision (new synchronous TypeError on sliced destinations) and to reconcile with the three overlapping open PRs (#35815 / #31680 / #33510).
What was reviewed:
read_off += read_amountindo_read_loop— mirrors the WindowsReadFileUV::on_readadvance;remaining_buffer'smax_length.saturating_sub(read_off)cap now shrinks correctly.reject_sliced_file_destinationplacement — now insidewrite_file_internal's per-blob block (coversBun.write,blob.write,Image#write, S3 callers unaffected since gated onstore::Data::File) plusget_writer.- Checked that keying on
offset != 0alone means.slice(0, N)destinations still pass through and O_TRUNC — deliberate per the description to avoid.size-read false positives, but worth a maintainer nod.
Extended reasoning...
Overview
Two independent fixes to file-backed Blob.slice(): (1) a one-line advance of self.read_off in the POSIX ReadFile::do_read_loop so the per-read cap max_length - read_off actually shrinks, fixing chardev slice reads that overshot to the next 64 KiB multiple; (2) a new reject_sliced_file_destination helper that throws a TypeError when a file-backed blob with offset != 0 is used as a write destination, called from write_file_internal and blob.writer(). Tests added for both in bun-file.test.ts (chardev boundary matrix over 1/4095/4096/4097/65535/65536/65537/1_000_000) and blob-write.test.ts (three entry points + a false-positive guard for unsliced files after .size is read).
Since my previous review, the author moved the sliced-destination check from three per-entry-point call sites into the shared write_file_internal helper (3f7b320), which now also covers Bun.Image#write. The doc comment on the helper was trimmed to two lines (0f3c199) after comment-cop flagged it. All prior threads are resolved.
Security risks
None identified. The read-side change tightens a bound (reads fewer bytes than before, never more). The write-side change adds validation that fails closed. No untrusted-input parsing, no auth/crypto surface.
Level of scrutiny
Moderate. The read_off fix is a one-liner that mirrors the existing Windows path and is straightforwardly correct. The write-side change is small but introduces a new user-facing error on a Bun-native API — that's an API-surface decision (throw now vs. implement positional writes later) that a maintainer should sign off on, per the repo's "API design" review guidance.
Other factors
- Overlapping PRs: the duplicate-finder flagged #35815 (same sliced-destination rejection, placed in
write_file_internal), #31680 (streaming face of the chardev bug), and #33510 (non-seekable source offsets). The author's overlap analysis is coherent, but a human needs to decide which combination to merge/close. - Intentional gap:
.slice(0, N)as a destination (offset 0, size set) still passes the check and will O_TRUNC + write past N. The description explains this is deliberate to avoid false-positiving after.sizestampsblob.size, and there's a regression test for that. Reasonable tradeoff, but worth maintainer confirmation. - Test evidence: the PR body's "release without fix" gate output was actually built from 31edb2b (this PR's first commit), so it doesn't demonstrate release-build failure on true main — a gate-tooling artifact, not a code concern. The ASAN-without-fix run does show 3 failures as expected.
There was a problem hiding this comment.
The earlier nit (Bun.Image#write bypassing the guard) is addressed — the check now lives in write_file_internal and get_writer. I didn't find further issues, but this adds a new user-facing throw on a previously-silent path and overlaps with #35815/#31680/#33510, so leaving the merge-vs-consolidate call to a maintainer.
Checked: read_off advance matches the Windows ReadFileUV path and remaining_buffer's cap now shrinks per iteration; the new guard keys on offset != 0 and store::Data::File(_) only, so S3 destinations and .size-stamped unsliced files are unaffected (covered by the new test); unlink()/delete() remain unguarded as stated.
Extended reasoning...
Overview
Two independent fixes to file-backed Blob.slice():
- read_file.rs (1 line): advance
self.read_off += read_amountper iteration indo_read_loop, soremaining_buffer'smax_length - read_offcap actually shrinks. Mirrors the existing WindowsReadFileUV::on_readpath. - Blob.rs (~15 lines): new
reject_sliced_file_destinationhelper throwing aTypeErrorwhen a file-backed blob withoffset != 0is used as a write destination. Called fromwrite_file_internal(coversBun.write,blob.write,Bun.Image#write) andget_writer(coversblob.writer()).
Tests cover the boundary lengths for the chardev over-read (/dev/zero, POSIX-only) and all three write entry points plus the .size-read false-positive.
Security risks
None identified. The read-side change tightens an over-read (returns fewer bytes than before, never more). The write-side change fails closed where the previous behavior was silent data loss.
Level of scrutiny
Medium. The mechanical changes are small and well-tested, but write_file_internal is a shared entry point for Bun.write, blob.write, and Bun.Image#write, and the new throw is a user-visible API behavior change (previously-silent code now throws synchronously). REVIEW.md flags user-facing API surface changes as needing maintainer sign-off, and the error message codifies that positional writes are out of scope for Bun.write ("use pwrite/write" instead).
Other factors
- My prior inline comment (Image#write bypass) was addressed in 3f7b320 by moving the guard into
write_file_internal; verified only two call sites remain (Blob.rs:1715, Blob.rs:5016). - The guard checks
store::Data::File(_)explicitly, so S3-backed destinations reachingwrite_file_internalfromS3File.rs/S3Client.rsare unaffected. - Three overlapping open PRs (#35815 same reject shape, #31680 streaming face, #33510 non-seekable source offsets) — a maintainer should decide which combination to land rather than an auto-approval racing them.
- Comment-cop feedback was addressed (doc comment trimmed to two lines in 0f3c199).
|
CI status: build 83732 is 192/196 green. The remaining failures are unrelated to this diff: Ready for review. |
A byte-range slice of a Bun.file() keeps the parent's pathlike store, so .delete() unlinked the whole file and .writer()/.write()/Bun.write() opened it with O_TRUNC and wrote at offset 0, ignoring the window. A slice's .exists() also returned false because resolve_size() is gated on size == MAX_SIZE and a slice has a concrete size. Add Blob.is_sliced_view, set in get_slice_from, and reject it in validate_writable_blob (covers do_unlink, get_writer, do_write, and Bun.write via write_file) and in write_file_internal for the callers that bypass validate_writable_blob. exists() now stats the file when the store has never been resolved.
…the requested length
2c90c5a to
6f06ec6
Compare
|
Reworked per discussion: the guard now lives in Supersedes #35815 (which keyed off |
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
src/runtime/webcore/Blob.rs:5028-5032— Theif blob.is_sliced_view.get() { return Err(global_this.throw_invalid_arguments(format_args!("A sliced Bun.file()..."))) }block appears verbatim here and again invalidate_writable_blob(L5331-5335). Consider extracting the ~140-char message to aconst SLICED_VIEW_MSG: &str(or a smallthrow_sliced_view(global)helper) and using it at both sites — REVIEW.md: "The second time a multi-line block appears in your diff, extract a named helper and use it at EVERY parallel site." Tests assert on the message substring, so a future wording tweak that only touches one site would silently drift.Extended reasoning...
What
This diff introduces the identical 5-line rejection block in two places:
write_file_internal(Blob.rs:5028-5032):if blob.is_sliced_view.get() { return Err(global_this.throw_invalid_arguments(format_args!( "A sliced Bun.file() is a read-only byte-range view; delete() and write() would affect the whole file. Use the un-sliced Bun.file() instead." ))); }
validate_writable_blob(Blob.rs:5331-5335):if blob.is_sliced_view.get() { return Err(global_this.throw_invalid_arguments(format_args!( "A sliced Bun.file() is a read-only byte-range view; delete() and write() would affect the whole file. Use the un-sliced Bun.file() instead." ))); }
Byte-for-byte identical, including the ~140-character error string.
Why it matters
REVIEW.md, Code style & idioms reviewers enforce: "The second time a multi-line block appears in your diff, extract a named helper and use it at EVERY parallel site." Both occurrences were added by this PR (the second one was added when the
Bun.Image#writereview comment moved a copy of the check intowrite_file_internal), so the rule applies directly.The tests in
test/js/bun/util/bun-file.test.tsassert on message content —expect((err as Error).message).toContain("sliced Bun.file()")— at four call sites. If a future wording change touches onlyvalidate_writable_blob(which covers.delete()/.writer()/.write()/Bun.write), thewrite_file_internalcopy (which coversBun.Image#write/Bun.s3.file().write) would silently drift and the tests would not catch it, since the tests only exercise thevalidate_writable_blobpaths.Step-by-step
- Both blocks are added in this diff (neither existed before).
- They are structurally identical: same guard (
blob.is_sliced_view.get()), same throw helper (throw_invalid_arguments), same literal message. - A reviewer changing the message in one place has no compile-time signal that the other exists;
rgfor the string would find both, but the repo convention is to make that unnecessary by construction. - The two sites cannot be collapsed to one call site —
write_file_internalis reached by callers (Bun.Image#write, S3) that bypassvalidate_writable_blob, per the comment at L5025-5027 — so the deduplication has to be at the message/helper level, not the check-placement level.
Fix
Either:
const SLICED_VIEW_READONLY: &str = "A sliced Bun.file() is a read-only byte-range view; delete() and write() would affect the whole file. Use the un-sliced Bun.file() instead.";
and use
format_args!("{SLICED_VIEW_READONLY}")(orformat_args!("{}", SLICED_VIEW_READONLY)) at both sites, or lift the whole guard into a small helper:fn reject_sliced_view(global: &JSGlobalObject, blob: &Blob) -> JsResult<()> { if blob.is_sliced_view.get() { return Err(global.throw_invalid_arguments(format_args!(SLICED_VIEW_READONLY))); } Ok(()) }
and call
reject_sliced_view(global_this, blob)?;in bothwrite_file_internalandvalidate_writable_blob.Impact
None on behavior — pure maintainability. Not blocking.
-
🟡
src/runtime/webcore/Blob.rs:5328-5335— The newis_sliced_viewfield is copied bydupe_with_content_typeand the L3351 struct literal, but is not written by_on_structured_clone_serialize(Blob.rs:714-782) nor set byon_structured_clone_deserialize(Blob.rs:4131-4307). SostructuredClone(Bun.file(p).slice(2, 5))— or the same viapostMessageto a Worker — yields a file-backed Blob withoffset/sizerestored butis_sliced_view = false, and.delete()/.write()/.writer()on that clone pass this guard and clobber the whole file. Not a regression (pre-PR the clobber happened in-thread too), but REVIEW.md's "new struct field → audit every serialization pair" applies; safest fix is bumpingSERIALIZATION_VERSIONto 5 and writing the flag byte.Extended reasoning...
What the bug is
The PR adds
Blob.is_sliced_view: Cell<bool>and threads it throughDefault,dupe_with_content_type, and the manual struct literal at Blob.rs:3351, then keysvalidate_writable_blob(L5331) and thewrite_file_internalguard (L5028) off it. But the structured-clone serialization pair was missed:_on_structured_clone_serializewritesversion,offset,content_type[/_was_set],store_tag,[size (v4) + store payload],is_jsdom_file,last_modified,[name]— and neveris_sliced_view.on_structured_clone_deserializereconstructs the file-backed blob viafind_or_create_file_from_path(which defaultsis_sliced_viewtofalse), then restoresoffset(L4279) andsize(L4280-4284), but never setsis_sliced_view.The specific code path
await Bun.write(p, "0123456789"); const clone = structuredClone(Bun.file(p).slice(2, 5)); await clone.delete(); // unlinks the whole file // or: await clone.write("XY") — truncates the whole file to "XY"
The same applies to
worker.postMessage(Bun.file(p).slice(2, 5))followed by.delete()/.write()/.writer()on the received value.Why the existing guards don't catch it
Both new guards test
blob.is_sliced_view.get()alone. The deserialized blob hasoffset = 2,size = 3,store = Data::File(pathlike), andis_sliced_view = false(fromDefault), so:validate_writable_blob(L5325-5335): store isFile, not detached, notBytes,is_sliced_viewis false →Ok(()).write_file_internal(L5022-5030): store isFile, not detached,is_sliced_viewis false → proceeds to open withO_TRUNC.do_unlink:validate_writable_blobpasses →file.unlink(pathlike)removes the whole path.
Step-by-step proof
Bun.file(p).slice(2, 5)→get_slice_fromdupes the parent, setsoffset=2,size=3,is_sliced_view=true(L1978-1981).structuredClone(...)→_on_structured_clone_serializewritesversion=4,offset=2, content_type,store_tag=File,size=3(L761), store payload (path),is_jsdom_file=0,last_modified.is_sliced_viewis not written.on_structured_clone_deserializereads back the same fields; at L4207/L4229 it callsBlob::new(find_or_create_file_from_path(...)), which returns a Blob withis_sliced_view: Cell::new(false)fromDefault.- L4279 sets
blob.offset = 2; L4280-4284 setsblob.size = 3(sincefile_size = Some(3) != MAX_SIZE).is_sliced_viewis never touched. clone.delete()→validate_writable_blobseesis_sliced_view == false→ passes → whole file unlinked.
Impact and severity
Same silent-clobber class this PR closes, on the structured-clone /
postMessagepath. Not a regression — before this PR,.delete()/.write()on a slicedBun.file()clobbered the whole file in-thread too, so the structured-clone path is no worse than it was. It's a gap in the same-class coverage on a narrow path (clone a sliced file-backed blob, then mutate the clone). REVIEW.md: "New enum variant or struct field → audit every switch on the discriminant, every constructor/clone site, every hasher/serialization pair."How to fix
Bump
SERIALIZATION_VERSIONto 5 and write/readis_sliced_viewas au8afteris_jsdom_file(matching the v3/v4 precedent). The alternative — reconstructing the flag on deserialize asoffset != 0 || file_size.is_some_and(|s| s != MAX_SIZE)— has the same false-positive the PR body called out for the JS-side check: if the user read.sizeon an un-slicedBun.file()before cloning,self.sizeis already the resolved stat size (notMAX_SIZE) at L761, so the heuristic would wrongly mark the un-sliced clone read-only. The version bump avoids that.
…void double-stat in exists() - Bump SERIALIZATION_VERSION to 5 and write/read is_sliced_view so a structuredClone/postMessage of a sliced Bun.file() stays read-only for delete()/write()/writer() on the receiving side. Added tests for both the sliced clone (must throw) and an un-sliced clone after .size was read (must still write). - Hoist the duplicated error message to SLICED_VIEW_READONLY_MSG so both throw sites cannot drift. - Gate the exists() fallback stat on !size_was_unresolved so a nonexistent un-sliced Bun.file() still issues exactly one stat, not two.
There was a problem hiding this comment.
The three follow-ups from the last pass (structured-clone serialization of is_sliced_view, the double-stat on ENOENT, and the duplicated error string) are all addressed in 78eecc1 and I have nothing further. Deferring to a human on the API-shape decision — making every sliced Bun.file() a hard read-only destination (including slice(0, n) and argless slice()) is a user-visible behavior change with a SERIALIZATION_VERSION bump, and it overlaps three open PRs (#35815 / #31680 / #33510) that a maintainer will want to reconcile.
What was reviewed
- v5 serialize/deserialize ladder:
is_sliced_viewbyte is written after the v3 name block and read only whenversion > 4; the== 3→<= 4gate change is behavior-preserving for v3/v4 payloads. get_exists_sync: the!size_was_unresolvedgate keeps the un-sliced ENOENT path at onestat; thedata_mut().as_file()reshuffle is guarded by the precedingmatches!(File)check.read_off += read_amountplacement: runs before theuse_stackcopy and before thebuffer.len() >= max_lengthbreak, so theremaining_buffercap shrinks on the next iteration and the final buffer is exactlymax_length.expect(() => Bun.write(slice, …)).toThrow()in the tests is sound —validate_writable_blobthrows synchronously at Blob.rs:5350, before any promise is created.
Extended reasoning...
Overview
Three related Bun.file().slice() fixes across five files: a new Blob.is_sliced_view: Cell<bool> field on the core type (webcore_types.rs) threaded through dupe(), the manual clone literal, get_slice_from, and structured-clone serialization (SERIALIZATION_VERSION 4 → 5); guards in validate_writable_blob and write_file_internal that reject a sliced file-backed destination for .delete()/.write()/.writer()/Bun.write()/Bun.Image#write(); a one-line self.read_off += read_amount in the POSIX ReadFile::do_read_loop so the per-read cap actually shrinks; and a get_exists_sync fallback stat so a sliced file's .exists() returns true. ~160 lines of new tests plus one existing test rewritten to assert the new throw.
Security risks
None material. The serialization change reads one extra byte on v5+ payloads via the existing bounds-checked reader (read_int_le::<u8>()?), so a truncated payload errors rather than reading past the buffer. The new guard fails closed (rejects write). No auth/crypto/permissions surface touched.
Level of scrutiny
Medium-high. The mechanical parts (read_off advance, exists() stat fallback) are small and well-tested, but the PR also (a) changes a user-facing API contract — code that previously called Bun.write(file.slice(0, n), src) and got a truncated copy now throws TypeError, (b) bumps the structured-clone wire-format version on Blob, and (c) adds a field to a core #[repr(C)] type. Those are design decisions a maintainer should sign off on rather than auto-merge.
Other factors
- All three of my earlier findings on this PR are resolved in 78eecc1; inline threads are marked resolved.
- Duplicate-PR bot flagged three overlapping PRs; the author acknowledged the overlap and this PR supersedes #35815 by design, but a human needs to decide which lands and close the others.
- Test coverage is thorough (variant matrix over slice shapes, negative test that reading
.sizedoesn't false-positive, structuredClone round-trip, boundary sizes for the chardev cap) and follows harness conventions (tempDir,test.concurrent, no network,skipIf(!isPosix)for /dev/zero). - One existing test in
bun-write.test.jswas rewritten (not deleted) to assert the new throw and that the source file is untouched — the intent-change is stated in a comment, which satisfies REVIEW.md's "never silently weaken an existing test" rule.
What
Three file-backed
Blob.slice()defects:Why
Over-read:
ReadFile::do_read_loopcaps each read bymax_length.saturating_sub(read_off), butread_offis initialised to 0 and never advanced on the POSIX path (the onlyread_off +=is on the WindowsReadFileUVbranch). A chardev slice sizes its initial buffer atmin(max_length, 4096), so the loop lands on the 64 KiB stack buffer and appends whole 65536-byte reads untilbuffer.len() >= max_lengthwithout the cap ever shrinking.Whole-file mutation on a slice:
.slice()dupe()s the parent's pathlike store and records onlyoffset/size.WriteFileopensO_WRONLY|O_CREAT|O_TRUNCand never threads the offset;do_unlinkcallsunlink(path);writer()opens the same path at position 0.validate_writable_blobonly rejectedBytes-backed destinations, so a file-backed slice reached every destructive path.exists() false on a slice:
get_exists_synctestsISREG(file.mode) || ISFIFO(file.mode), wherefile.modeis filled in byresolve_size(). A slice has a concretesize, so thesize == MAX_SIZEgate skippedresolve_size()andmodestayed 0.How
self.read_off += read_amountper iteration indo_read_loop, matching the Windows path.remaining_buffer's cap now shrinks correctly and the final buffer is exactlymax_lengthbytes.Blob.is_sliced_view: Cell<bool>, set byget_slice_fromand preserved bydupe(), the manual clone literal, and structured-clone serialization (SERIALIZATION_VERSIONbumped to 5).validate_writable_blobandwrite_file_internalboth reject when it is set, with the message hoisted to a singleconst. This coversBun.write(),.write(),.writer(),.delete()/.unlink(), andBun.Image#write(). An explicit flag avoids the false positive that keying onsize != MAX_SIZEwould have after.sizeis read on an un-slicedBun.file().get_exists_syncwhenseekableis stillNone, gated on!size_was_unresolvedso a nonexistent un-sliced path still issues exactly onestat.Verification
blob.test.ts(46 tests),blob-write.test.ts,bun-file-read.test.ts,structured-clone-blob-file.test.tsandblob-file-name-ownership.test.tspass with the debug build. The four timeouts inbun-write.test.jsreproduce identically on an unpatched debug/ASAN build.The streaming face of the chardev slice bug (sliced
.stream()never closes after deliveringmax_size) is covered by #31680. #35815 overlaps on the sliced-destination rejection (different shape,offset != 0only).no test proof · iteration 5 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/io/bun-write.test.js