Repository navigation
Conversation
WalkthroughFixes ChangesFile-to-file copy correctness
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 6:18 AM PT - Aug 4th, 2026
❌ @robobun, your commit 6487977 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 32859That installs a local version of the PR into your bun-32859 --bun |
|
Found 2 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}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/runtime/webcore/blob/copy_file.rs`:
- Around line 1032-1035: The comment in copy_file’s slice-handling branch is too
long and exceeds the repository’s 3-line limit. Shorten the explanatory comment
in the bounded read/write path inside copy_file so it conveys the same point in
at most 3 lines, keeping the reference to fcopyfile and the
slice/non-truncatable destination behavior but removing any extra wording.
- Around line 928-940: The destination handling in copy_file’s overwrite path
should fail closed instead of ignoring identity/truncation errors. Update the
same-file check around bun_sys::fstat and the truncation call on
self.destination_fd so that any fstat or ftruncate failure returns an error and
aborts the copy rather than continuing. Keep the logic localized to the
copy_file flow and the do_copy_same_file/do_close path so success is only
reported after destination identity and truncation are confirmed.
In `@test/js/bun/io/bun-write.test.js`:
- Around line 613-626: The current hardlink same-inode test only covers a
full-file copy, so it misses regressions in the sliced self-copy path handled by
do_copy_same_file in Bun.write/copy_file logic. Extend the Bun.write hardlink
test by adding a case where Bun.file(originalPath).slice(...) is written to the
hardlink alias, and assert both paths still contain the original content with no
truncation or corruption. Keep the test alongside the existing same-inode
coverage in bun-write.test.js and reference the existing hardlink setup and
Bun.write/Bun.file behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3159ce2c-a058-44eb-9cce-79037ba672a1
📒 Files selected for processing (3)
src/runtime/webcore/Blob.rssrc/runtime/webcore/blob/copy_file.rstest/js/bun/io/bun-write.test.js
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/runtime/webcore/blob/copy_file.rs (1)
388-417: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winUse a bounded loop before Linux fallback writes escape the slice.
copy_file_using_read_write_loop(..., remain, ...)is not actually bounded: after consumingremain, it keeps reading to EOF. The newtotal_written.min(max_length)only fixes the reported count/truncation afterward, so sliced writes to pipes/sockets//dev/null-style destinations can still receive bytes past the slice whencopy_file_range/sendfilefalls back.Suggested direction
- #[cfg(any(target_os = "macos", target_os = "freebsd"))] + #[cfg(any(target_os = "linux", target_os = "android", target_os = "macos", target_os = "freebsd"))] fn do_copy_bounded(&mut self, limit: u64) -> Result<u64, bun_core::Error> {Then use it in each Linux fallback when
!unknown_size, before calling the unbounded read/write helper:+ if !unknown_size { + match self.do_copy_bounded(remain as u64) { + Ok(written) => { + total_written += written; + return Ok(()); + } + Err(err) => return Err(err), + } + } match node_fs::NodeFS::copy_file_using_read_write_loop(Also applies to: 455-477, 533-555, 593-615
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/runtime/webcore/blob/copy_file.rs` around lines 388 - 417, The bounded-copy path in do_copy_bounded is needed to prevent Linux fallback read/write loops from continuing past the requested slice; update the fallback logic in copy_file_using_read_write_loop and the related Linux branches so they stop after remain/max_length instead of reading to EOF. Use the existing do_copy_bounded helper (and the unknown_size guard where applicable) to enforce the limit before any unbounded read/write helper is called, ensuring pipes/sockets and similar destinations never receive bytes beyond the slice.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/runtime/webcore/blob/copy_file.rs`:
- Around line 388-417: The bounded-copy path in do_copy_bounded is needed to
prevent Linux fallback read/write loops from continuing past the requested
slice; update the fallback logic in copy_file_using_read_write_loop and the
related Linux branches so they stop after remain/max_length instead of reading
to EOF. Use the existing do_copy_bounded helper (and the unknown_size guard
where applicable) to enforce the limit before any unbounded read/write helper is
called, ensuring pipes/sockets and similar destinations never receive bytes
beyond the slice.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a326fd57-5715-4a01-8890-fc8ed26db733
📒 Files selected for processing (2)
src/runtime/webcore/blob/copy_file.rstest/js/bun/io/bun-write.test.js
|
Another report of the source-slice half of this landed today; I put together a minimal variant before spotting this PR. Branch |
|
Status: rebased onto current main, ~140 insertions, all review threads resolved.
Both reported to triage. The |
|
Heads up: #31515 (open since May 28, already reviewed) fixes the same sliced-source copy window, covers Windows as well, and was just rebased onto main. The part of this PR that #31515 does not cover is |
Jarred-Sumner
left a comment
Jarred-Sumner
left a comment
There was a problem hiding this comment.
This hsould be approximately 20% of the code. Delete.
|
Cut to the minimal fix in 5dcf6ba: 127 insertions (was 474). Kept:
Dropped: slice-onto-self, bounded copy helper, FreeBSD/Linux-fallback slice handling, and the tests that exercised those. None of that was in the original reports and all of it is no worse than current behaviour (the slice was ignored entirely before). |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/runtime/webcore/blob/copy_file.rs (1)
933-961: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winFreeBSD's sliced-source copy still does unbounded I/O before truncating.
Passing
0here selectscopy_file_using_read_write_loop's unbounded "read to EOF" fallback (per its contract insrc/runtime/node/node_fs.rs:4871-4989, where a bounded loop runswhile remain > 0then falls through to an unbounded loop that reads until EOF "we don't know the size / so we just go forever until we get an EOF"), even thoughself.max_lengthis already a concrete bound by this point (recomputed at line 828 for all platforms before reaching this block). The final content ends up correct because of the subsequent truncate, but for a small slice of a very large source file, this reads/writes the entire remainder of the source before discarding most of it — unlike the macOS branch just above, which correctly bounds the loop toself.max_length.♻️ Proposed fix
match node_fs::NodeFS::copy_file_using_read_write_loop( bun_core::ZStr::EMPTY, bun_core::ZStr::EMPTY, self.source_fd, self.destination_fd, - 0, + self.max_length as usize, &mut total_written, ) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/runtime/webcore/blob/copy_file.rs` around lines 933 - 961, Update the FreeBSD call to NodeFS::copy_file_using_read_write_loop in the sliced-source copy path to pass self.max_length as the read/write bound instead of 0. Preserve the existing total_written tracking, error handling, and final truncation behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/runtime/webcore/blob/copy_file.rs`:
- Around line 803-825: Update the same-inode handling in the copy flow to set
read_len using the existing offset/max_length clamp used later, so sliced
self-copies report only the slice length. Handle failures from bun_sys::fstat
and bun_sys::ftruncate explicitly by propagating an error or otherwise failing
closed, rather than skipping truncation or discarding the result; preserve the
early return for confirmed same-inode files.
- Around line 847-850: Handle the result of bun_sys::set_file_offset in the
offset setup within the copy flow, instead of discarding it. If repositioning
self.source_fd fails, propagate the error and stop the copy before reading,
while preserving the existing regular-file and positive-offset conditions.
---
Outside diff comments:
In `@src/runtime/webcore/blob/copy_file.rs`:
- Around line 933-961: Update the FreeBSD call to
NodeFS::copy_file_using_read_write_loop in the sliced-source copy path to pass
self.max_length as the read/write bound instead of 0. Preserve the existing
total_written tracking, error handling, and final truncation behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0e637af3-3781-4071-9d19-749f91c9b74c
📒 Files selected for processing (3)
src/runtime/webcore/Blob.rssrc/runtime/webcore/blob/copy_file.rstest/js/bun/io/bun-write.test.js
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/runtime/webcore/Blob.rs`:
- Around line 4802-4805: The source-length logic around
source_store.data.as_file().max_size must preserve whether BunFile.slice() was
explicitly bounded instead of inferring unsliced state from offset and size.
Carry an explicit unsliced/bounded marker through the call, ensure explicit
slice(0, cachedSize) remains capped at the cached size, and ensure explicit
empty slices return zero even when cached_max_size is MAX_SIZE. Add regression
coverage for these empty and boundary cases.
In `@src/runtime/webcore/blob/copy_file.rs`:
- Line 918: Update the bounded-copy cleanup around ftruncate in the relevant
copy operation to retain its syscall result instead of discarding it. When
truncation fails, close both descriptors and propagate a catchable error; only
report success after truncation succeeds, while preserving existing cleanup
behavior for successful truncation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f99dc224-6ed3-48e2-8fd9-8ccc81a49876
📒 Files selected for processing (3)
src/runtime/webcore/Blob.rssrc/runtime/webcore/blob/copy_file.rstest/js/bun/io/bun-write.test.js
There was a problem hiding this comment.
No bugs found in this pass, but deferring to a human: this reworks the core Bun.write file→file copy path (removes O_TRUNC, adds same-inode detection, changes which blob's offset/size drives the copy), drops the destination-slice-cap semantics along with its test, and overlaps with open PR #31515. The whole-file-vs-slice heuristic in Blob.rs is acknowledged as imperfect pending an explicit is-sliced bit — worth a maintainer sign-off on that trade-off and on the semantic change.
Checked: the same-inode/ftruncate(0) block only runs for path-backed destinations and is gated on S_ISREG, so fd destinations and /dev/null are unaffected; the macOS sliced branch now truncates to read_len (fixes the empty-source-slice extend); the new failed-stat disjunct in the source-len heuristic covers the ENOENT-then-created case; new tests cover self-copy, hardlink, three slice shapes, and both stale-stat cases.
Extended reasoning...
Overview
The PR touches three files: src/runtime/webcore/Blob.rs (the write_file_with_source_destination call site — switches from passing destination blob offset/size to source blob offset/size, with a three-disjunct heuristic to distinguish an explicit slice from a cached stat), src/runtime/webcore/blob/copy_file.rs (removes O_TRUNC from destination open flags; adds an fstat-based same-inode check that no-ops instead of truncating; adds explicit ftruncate(dest, 0) for regular-file destinations; fixes the max_length computation to use saturating_sub(offset).min(max_length); seeks the source fd; on macOS, routes sliced sources through a read/write loop instead of fcopyfile), and test/js/bun/io/bun-write.test.js (deletes one destination-slice-cap test, adds a describe.skipIf(isWindows) block with 8 new tests).
Security risks
None identified. This is local file I/O with no auth, crypto, network, or untrusted-input parsing. The paths and fds come from the calling JS, which already had full filesystem access. The change does not widen any attack surface.
Level of scrutiny
High. Bun.write is a core, widely-used API and the two bugs being fixed are silent data-loss bugs — the fix itself must not introduce new ones. The PR went through ~15 review iterations in which multiple real regressions were found and fixed (Windows test gating, Linux read/write-fallback overshoot, macOS read_len not set after fcopyfile, ftruncate(0) on non-regular destinations, cached-source-stat cap, empty-source-slice ftruncate extend, ENOENT-stat source). That history alone signals the code is subtle enough to warrant human eyes on the final shape.
Other factors
- Semantic change:
Bun.write(Bun.file(dst).slice(0, N), src)previously capped the write at N bytes; it now writes the whole source. The only test asserting the old behavior was deleted. The PR description now mentions this, and the rationale (same mechanism as #22456, and the old code applieddst.offsetas a source seek so it was never coherent) is sound, but it's a user-visible behavior removal that a maintainer should ratify. - Heuristic limitation: the source-len heuristic in
Blob.rscannot distinguish.slice(0, n)wheren == store.max_sizefrom a cached stat, and now also treats{offset:0, size:0, max_size:MAX_SIZE}as whole-file. The author agreed the real fix is an explicit is-sliced bit onBlob, deferred as out of scope. That's reasonable, but it's a design call. - Overlap with #31515: an open, already-reviewed PR fixes the same sliced-source window and additionally covers Windows. A robobun comment flagged that if #31515 lands first, this PR should be reduced to just the same-inode fix. A human should decide the merge order.
- Discarded syscall results:
fstat(dest),ftruncate(dest, 0), andset_file_offset(src)failures are silently ignored. This was raised by CodeRabbit, and the author intentionally dropped fail-closed handling per a scope-reduction directive. On a just-opened regular-file fd these effectively cannot fail, and pre-PRO_TRUNCnever surfaced its own errno either, so it's not a regression — but it's the kind of trade-off a maintainer should confirm. - No CODEOWNERS hit and no outstanding unresolved threads; all inline comments are marked resolved.
… fd (#36758) ## Repro ```sh head -c 200000 /dev/zero | tr '\0' S > /tmp/src.bin printf 'DDDDDDDDDDDDDDDDDDDDDDDDDDDDDD' > /tmp/dst.bin BUN_CONFIG_DISABLE_COPY_FILE_RANGE=1 bun -e ' const fs = require("fs"); const fd = fs.openSync("/tmp/dst.bin", "r+"); console.log(await Bun.write(Bun.file(fd).slice(0, 5), Bun.file("/tmp/src.bin"))); fs.closeSync(fd); ' wc -c /tmp/dst.bin # Linux (fallback): 200000 (expected 30; whole source over-copied then ftruncated) # macOS: 5 (expected 30; fcopyfile rewrote from 0 then ftruncate(5)) ``` Same for `Bun.stdout` under `>>` redirection: the pre-existing bytes in the redirected file are gone after the write. ## Cause `CopyFile::run_async` dispatched fd-backed destinations through the same path as path destinations. - **macOS:** `fcopyfile(COPYFILE_DATA)` rewrites the destination from offset 0, and when `stat.st_size > max_length` Bun then calls `ftruncate(dest, max_length)` to trim the over-copied tail. Both are safe when Bun opened the destination with `O_CREAT|O_TRUNC`, but for a caller-supplied fd they discard whatever the file already contained. - **FreeBSD** and the **Linux read/write fallback** (kernel < 4.5, `EXDEV`, `ENOTSUP`, `EINVAL`, or `BUN_CONFIG_DISABLE_COPY_FILE_RANGE`): `copy_file_using_read_write_loop` reads the source to EOF regardless of `max_length` and then `ftruncate(dest, total_written)` sets the file length, with the same effect. The Linux `copy_file_range`/`sendfile`/`splice` happy path already copies exactly `max_length` bytes and does not truncate, so this only shows up there when the kernel syscall is unavailable for the fd pair. ## Fix Add `read_write_loop_capped`, a bounded `read()`/`write()` loop that transfers exactly `cap` bytes (or to EOF) and never touches the destination's length. Route every fd-backed destination through it: - macOS/FreeBSD `run_async`: take the `fcopyfile`/`ftruncate` branch only when `destination_file_store.pathlike` is a path. - Linux `do_copy_file_range`: the three identical fallback sites are factored into `read_write_fallback`, which keeps the existing copy-to-EOF + `ftruncate` for path destinations and uses the bounded loop for fd destinations. ## Verification Two tests in `test/js/bun/io/bun-write.test.js` under `Bun.write(Bun.file(fd), Bun.file(path)) does not truncate the fd` spawn a child with `BUN_CONFIG_DISABLE_COPY_FILE_RANGE=1` so they fail-before on every POSIX lane: ``` === system bun === (fail) preserves bytes past the slice window in an r+ fd resolved: "200000", content: 200000 x "S" (expected "5", "SSSSS" + 25 x "D") (fail) preserves pre-existing bytes when stdout is redirected with >> resolved: "1000", content: 1000 bytes (expected "100", 110 bytes) === this branch === (pass) preserves bytes past the slice window in an r+ fd (pass) preserves pre-existing bytes when stdout is redirected with >> ``` The existing `Bun.write(Bun.stdout, Bun.stdin) copies the whole pipe` test is widened from Linux-only to every POSIX lane, covering the bounded loop's EOF-termination branch on macOS/FreeBSD. `cargo check -p bun_runtime --target {aarch64,x86_64}-apple-darwin` and `--target x86_64-unknown-freebsd` pass. ## Related Found during review of #36692 (which routes a FIFO source through `splice` on Linux; the `bun_opened_dest` guard it adds to the same three fallback sites is subsumed by `read_write_fallback` here). #32859 reworks the same block to honour source slices and #33715 sets `read_len` after `fcopyfile`; both are complementary and will need a small rebase against whichever lands first. <!-- robobun:evidence:begin --> --- **no test proof** · iteration 4 · 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 <!-- robobun:evidence:end --> --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
… fd (oven-sh#36758) ## Repro ```sh head -c 200000 /dev/zero | tr '\0' S > /tmp/src.bin printf 'DDDDDDDDDDDDDDDDDDDDDDDDDDDDDD' > /tmp/dst.bin BUN_CONFIG_DISABLE_COPY_FILE_RANGE=1 bun -e ' const fs = require("fs"); const fd = fs.openSync("/tmp/dst.bin", "r+"); console.log(await Bun.write(Bun.file(fd).slice(0, 5), Bun.file("/tmp/src.bin"))); fs.closeSync(fd); ' wc -c /tmp/dst.bin # Linux (fallback): 200000 (expected 30; whole source over-copied then ftruncated) # macOS: 5 (expected 30; fcopyfile rewrote from 0 then ftruncate(5)) ``` Same for `Bun.stdout` under `>>` redirection: the pre-existing bytes in the redirected file are gone after the write. ## Cause `CopyFile::run_async` dispatched fd-backed destinations through the same path as path destinations. - **macOS:** `fcopyfile(COPYFILE_DATA)` rewrites the destination from offset 0, and when `stat.st_size > max_length` Bun then calls `ftruncate(dest, max_length)` to trim the over-copied tail. Both are safe when Bun opened the destination with `O_CREAT|O_TRUNC`, but for a caller-supplied fd they discard whatever the file already contained. - **FreeBSD** and the **Linux read/write fallback** (kernel < 4.5, `EXDEV`, `ENOTSUP`, `EINVAL`, or `BUN_CONFIG_DISABLE_COPY_FILE_RANGE`): `copy_file_using_read_write_loop` reads the source to EOF regardless of `max_length` and then `ftruncate(dest, total_written)` sets the file length, with the same effect. The Linux `copy_file_range`/`sendfile`/`splice` happy path already copies exactly `max_length` bytes and does not truncate, so this only shows up there when the kernel syscall is unavailable for the fd pair. ## Fix Add `read_write_loop_capped`, a bounded `read()`/`write()` loop that transfers exactly `cap` bytes (or to EOF) and never touches the destination's length. Route every fd-backed destination through it: - macOS/FreeBSD `run_async`: take the `fcopyfile`/`ftruncate` branch only when `destination_file_store.pathlike` is a path. - Linux `do_copy_file_range`: the three identical fallback sites are factored into `read_write_fallback`, which keeps the existing copy-to-EOF + `ftruncate` for path destinations and uses the bounded loop for fd destinations. ## Verification Two tests in `test/js/bun/io/bun-write.test.js` under `Bun.write(Bun.file(fd), Bun.file(path)) does not truncate the fd` spawn a child with `BUN_CONFIG_DISABLE_COPY_FILE_RANGE=1` so they fail-before on every POSIX lane: ``` === system bun === (fail) preserves bytes past the slice window in an r+ fd resolved: "200000", content: 200000 x "S" (expected "5", "SSSSS" + 25 x "D") (fail) preserves pre-existing bytes when stdout is redirected with >> resolved: "1000", content: 1000 bytes (expected "100", 110 bytes) === this branch === (pass) preserves bytes past the slice window in an r+ fd (pass) preserves pre-existing bytes when stdout is redirected with >> ``` The existing `Bun.write(Bun.stdout, Bun.stdin) copies the whole pipe` test is widened from Linux-only to every POSIX lane, covering the bounded loop's EOF-termination branch on macOS/FreeBSD. `cargo check -p bun_runtime --target {aarch64,x86_64}-apple-darwin` and `--target x86_64-unknown-freebsd` pass. ## Related Found during review of oven-sh#36692 (which routes a FIFO source through `splice` on Linux; the `bun_opened_dest` guard it adds to the same three fallback sites is subsumed by `read_write_fallback` here). oven-sh#32859 reworks the same block to honour source slices and oven-sh#33715 sets `read_len` after `fcopyfile`; both are complementary and will need a small rebase against whichever lands first. <!-- robobun:evidence:begin --> --- **no test proof** · iteration 4 · 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 <!-- robobun:evidence:end --> --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
|
@robobun rebase or close |
db1315f to
54f8516
Compare
|
Rebased onto current main as a single commit (54f8516). Resolved a conflict in the macOS block, which now uses |
31c657b to
83df194
Compare
…File slice Fixes #20462 Fixes #22456 Bun.write(p, Bun.file(p)) opened the destination with O_TRUNC before reading the source, so when they were the same inode the file was truncated to zero bytes and the call resolved 0. Bun.write(dest, Bun.file(src).slice(a, b)) passed the destination blob's offset/size to CopyFile, so the source slice was ignored and the whole file was written; the destination's cached size also acted as a cap (#22456). Open the destination without O_TRUNC. After both fds are open, fstat the destination: if it is the same inode, resolve with the file's size as a no-op; otherwise ftruncate a regular-file destination to 0. Pass the source blob's offset/size (treating a size that matches the store's cached stat, or 0 with an unstat'd store, as whole-file) and seek the source fd before copying. On macOS, use the bounded read/write loop for sliced sources since fcopyfile ignores the fd position.
83df194 to
be367ef
Compare
There was a problem hiding this comment.
No new findings on 6487977. All prior review threads are resolved — the .lastModified desync test was added, and the two remaining items (sliced self-copy resolving with the full stat size, and the ungated size == cached disjunct matching an explicit f.slice(0, f.size) on a stat'd fd destination) are the acknowledged residuals of the deferred is-sliced marker. Deferring to a maintainer for that trade-off and for the cross-platform copy-path changes rather than auto-approving.
What was reviewed:
slice_lenheuristic against every stat/desync path I'd previously traced; be367ef'sis_sourcegating covers them.- The three #36758 fd-destination tests still pass with the new
min(slice_len(src), slice_len(dst))cap. O_TRUNC→ post-openftruncateordering: only applied to path destinations after the same-inode check, gated onISREG.
Extended reasoning...
Overview
The PR fixes two silent data-loss bugs in Bun.write's file→file path (#20462 self-copy truncation, #22456 stale-stat cap) and makes source BunFile slices actually bound the copy. It touches src/runtime/webcore/Blob.rs (a new slice_len heuristic deciding whether blob.size is a real slice bound or a cached stat), src/runtime/webcore/blob/copy_file.rs (drops O_TRUNC from the destination open, adds a post-open same-inode check + ftruncate(0), seeks the source fd, and reworks the macOS branch to use the capped read/write loop for sliced sources), and adds nine tests to test/js/bun/io/bun-write.test.js.
Security risks
None identified. The change is local to file-to-file copy semantics; no parsing of untrusted input, no auth/crypto/permissions surface. The inode comparison uses st_dev+st_ino from fstat on already-open fds, which is the standard same-file check.
Level of scrutiny
High. This is a data-integrity path where a wrong branch silently writes the wrong bytes and resolves successfully. The slice_len heuristic went through four review-driven revisions today alone (rebase drop of the #36758 fd-cap, .lastModified desync, .slice(0,0) on unstat'd dest, missing test coverage), and its remaining hole — size == cached also matches an explicit .slice(0, f.size) on a stat'd fd destination, technically regressing #36758's cap for that shape — cannot be closed without adding an explicit "was sliced" marker to Blob, which was deferred as out of scope. The change also fans out across Linux (copy_file_range/sendfile/splice), macOS (clonefile/fcopyfile), and FreeBSD branches, each with different fast-path semantics.
Other factors
- All 19 of my prior inline threads are resolved; the last requested change (the
.lastModifieddesync test) landed in 6487977. - Two acknowledged residuals are explicitly "leaving for the maintainer": the sliced-self-copy
read_lenvalue, and thesize == cacheddestination hole. Both are strict improvements over main (which destroyed the file / ignored source slices entirely), but whether to ship the heuristic-with-known-hole vs. do the structural is-sliced fix is a design call. - The maintainer engaged this morning ("rebase or close") and should see the final state.
Given the data-loss surface, platform fan-out, and the explicit maintainer-decision residual, this is not a change I'm comfortable auto-approving.
Two silent data-loss bugs in
Bun.write's file→file path, both resolving successfully while writing the wrong bytes.Fixes #20462
Fixes #22456
Repro
Cause
CopyFile::do_open_fileopens the destination withO_TRUNCbefore any same-inode check, so when source and destination resolve to the same file the open truncates the data before it is read.write_file_with_source_destinationpasseddestination_blob.offset/.sizetoCopyFile::create, so a source slice's window was never seen. Becauseblob.sizealso holds a stat size cached byresolve_size()(via.exists()/.size), a previously stat'd destinationBunFilealso acted as a cap (#22456).run_asynctreated the passed size as an end offset and never seeked the source fd.Fix
O_TRUNC. After both fds are open,fstatthe destination: if it is the same inode as the source, resolve with the file's size as a no-op; otherwiseftruncate(dest, 0)for regular-file path destinations.slice_lenfor both source and destination blobs (treating asizethat equals the store's cached stat, or0with an unstat'd store, as "whole file" so only an explicit.slice()bounds the copy), and cap the copy atmin(source_slice_len, dest_slice_len). Seek the source fd to the source offset.fcopyfileignores the fd position; setread_lenafterfcopyfile.Windows uses a separate
CopyFileWindowspath that this change does not touch; the new tests aredescribe.skipIf(isWindows). #31515 has the Windows slice plumbing.Verification
New tests in
test/js/bun/io/bun-write.test.jsunderBun.write > file to file:slice(a,b),slice(0,n),slice(a)each write exactly the slice (over a pre-existing larger destination)BunFilereused as a destination receives the full sourceBunFileused as an unsliced source is not capped at its cached size.exists()ran before the backing file existed copies the whole fileAll except the last two fail on
mainand pass with this change. The existingBun.file(fd).slice(...)destination-cap tests also still pass.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/bun/io/bun-write.test.js