Repository navigation
Conversation
… a partial copy fs.copyFile and fs.cp call copy_file_range with explicit offset pointers, so the kernel does not move the fd offsets. When a later call fails with an errno that selects the fallback (EXDEV, EINVAL, ENOSYS, EOPNOTSUPP), the fallback copies the whole file again from offset 0. The fallback added its count to the bytes copy_file_range had already copied, and the final ftruncate used that sum. The copy got extra NUL bytes at the end, or kept old bytes when the old destination was longer. Start the fallback count at 0, as the fallback copies the whole file. This applies to the two fallback sites in copy_file_inner, the two in the Linux arm of copy_single_file_sync, and the one in its FreeBSD arm.
|
Updated 11:15 PM PT - Sep 18th, 2026
✅ @robobun, your commit e17cdef0ecf85742b3b29441f7b0acac95336cb8 passed in 🧪 To try this PR locally: bunx bun-pr 43398That installs a local version of the PR into your bun-43398 --bun |
|
Status
|
|
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 (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 1 remains after this review. WalkthroughLinux/Android and FreeBSD copy fallbacks now reset byte counts after partial ChangesCopy fallback handling
Suggested reviewers: Priority: ➖ Normal 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![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 (both pre-existing, not introduced here), I also checked two things about the reset-to-0 itself: the size-known branches pass the already-decremented size into the fallback, but copy_file_using_read_write_loop keeps reading until EOF after remain reaches 0, so the copy is still complete and wrote still equals the full length; and the FreeBSD site also passes explicit off_in/off_out, so the fd offsets are at 0 there as well and w = 0 is correct for that fallback too.
Extended reasoning...
The two confirmed inline findings are pre-existing gaps in the same match blocks (missing EINTR retry in the sync twin; kernels 5.3-5.11 returning 0 from copy_file_range on procfs). The five-site change itself is uniform and correct: every call site passes explicit offset pointers to copy_file_range, the sendfile/read-write fallbacks consume the untouched fd offsets from 0, and the scope-guard ftruncate uses wrote, so seeding the fallback counter with the partial count was the sole source of the padding. The reduced size argument forwarded to the fallback is harmless because the read/write loop's second phase continues to EOF. The test's env knob BUN_CONFIG_DISABLE_ioctl_ficlonerange is read in src/sys/copy_file.rs:310, the LD_PRELOAD syscall() interposition follows the existing spawn-stdio-syscall-error.test.ts pattern, and the stderr trace assertion ensures the shim actually intercepted the call.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟣
src/runtime/node/node_fs.rs— Users on Linux 5.3 to 5.11 copying a procfs or sysfs file with fs.copyFile / fs.cp still get an empty destination after this change. In the size-0 loop the first copy_file_range on such a file returns 0 on those kernels, and node_fs.rs:5091 treats 0 as EOF, so wrote stays 0 and the scope guard ftruncates the destination to 0 (:5037). The PR's new /proc/version rows only pass because the shim, not the kernel, answers the first call. Fix: when st_size is 0 and the first copy_file_range returns 0 without having copied anything, fall through to the read/write loop instead of breaking, so procfs-style sources are copied to EOF.Extended reasoning...
fs.copyFileSync('/proc/version', dest) on a kernel between 5.3 and 5.11 (golang/go#44272 documents the kernel returning 0 for procfs/sysfs sources in that range). copy_file_inner at :5001 sets size = 0 because st_size is 0. :5045 can_use_copy_file_range_syscall is true. The size-0 loop at :5056 calls copy_file_range(page_size); the kernel returns 0, no error, so :5091 breaks with wrote = 0. The scope guard at :5037 runs ftruncate(dest, 0). The user gets an empty file and exit code 0. Same loop at :8507/:8545 for cpSync. Base commit (git show 26e7a4b:src/runtime/node/node_fs.rs) has the identical break, so the dismissing finder is right that it is pre-existing, and Node writes an empty file too because libuv trusts st_size. But the PR touched exactly this loop (:5081-5085) with a comment saying the fallback restarts at 0, added tests named 'source reports size 0', and claims 'Verified: 8 new tests', which will make users and reviewers believe size-0 sources are handled; the tests are satisfied by the LD_PRELOAD shim (cp.test.ts:748) and never exercise a real kernel returning 0.…
Verification: pre-existing — acknowledged in PR description ("Not in this PR: when the first copy_file_range call returns 0 for a source that has data (a procfs file on some older kernels), the copy is empty"); the note is accurate about the bound, and the diff does not change it. Trigger: fs.copyFile/fs.cp of a procfs/sysfs source (st_size 0) on a Linux 5.3–5.11 kernel that lacks the 5.12 cross-fs EXDEV…
| if matches!(err.get_errno(), E::ENOSYS | E::EOPNOTSUPP) { | ||
| sys::copy_file::disable_copy_file_range_syscall(); | ||
| } | ||
| let mut w = wrote.get(); | ||
| // copy_file_range did not move the fd offsets, so the fallback starts again at 0. | ||
| let mut w = 0; | ||
| let r = Self::copy_file_using_sendfile_on_linux_with_read_write_fallback(src, dest, src_fd, dest_fd, size, &mut w); | ||
| wrote.set(w); | ||
| return r; |
There was a problem hiding this comment.
🟣 pre-existing, not blocking: pre-existing: a caller of fs.cpSync on one file can get a spurious EINTR error on Linux, where the async twin retries. The sync copy_file_range match at node_fs.rs:8526 and :8570 has no E::EINTR => continue arm, so EINTR falls to _ => return err and the copy aborts. The async copy_file_inner twin handles it at :5076 and :5115. Fix: handle EINTR the same way in both copy_single_file_sync loops (retry the call), so sync and async copies behave alike. Same pattern at 2 sites (node_fs.rs:8531, node_fs.rs:8575).
A small fix can ride a push you are already making; otherwise a short reply is enough.
Extended reasoning...
The base branch behaves the same way; this diff edits the fallback arm of these two match blocks but leaves the EINTR gap. A user calls fs.cpSync(srcFile, destFile) with default options on Linux, which routes to copy_single_file_sync. copy_file_range is issued at node_fs.rs:8512 or :8556. The kernel's splice-based path in copy_file_range can return -ERESTARTSYS when a signal is pending before any bytes are copied in that call, which surfaces as EINTR when the handler was installed without SA_RESTART. The match at :8526-8542 (size 0 loop) and :8570-8586 (sized loop) lists only EXDEV/ENOSYS/EINVAL/EOPNOTSUPP, so EINTR hits _ => return err at :8541 / :8585. cpSync throws an EINTR error and the destination is left partially written and ftruncated to the partial wrote by the scopeguard at :8484. The async twin in copy_file_inner has E::EINTR => continue at :5076 and :5115, so fs.promises.cp / fs.copyFile retry and succeed on the same event. A correct fix adds the EINTR retry arm at both sync sites.
Verification: pre-existing. Triggering condition: a signal whose handler was installed without SA_RESTART lands on the thread while copy_file_range is in the kernel's splice-based path with zero bytes progressed in that call (splice_from_pipe_next returns -ERESTARTSYS, surfaced as EINTR), during fs.cpSync/fs.cp of a single regular file on Linux. Mechanism verified in… | pre-existing — the base commit has the…
This branch has not been deployed
1 participant
Problem
fs.copyFileandfs.cpcan write a copy that is longer than the source: extra NUL bytes, or old bytes from a longer old destination. The trigger:copy_file_rangecopies part of the file, then a later call fails withEXDEV,EINVAL,ENOSYSorEOPNOTSUPP(Copying multiple large files results inassertion failed: (left == right)in kernel_copy.rs:592 rust-lang/rust#91152 saw this on a network share). A review on fs.copyFile: set the destination mode before the data so an unprivileged copy drops set-uid and set-gid #43325 found it, not a user report.src/runtime/node/node_fs.rs:copy_file_inner(:5081,:5119) andcopy_single_file_sync(:8533,:8576, FreeBSD:8715). The fallback copies the whole file again from offset 0, but its count starts at the bytes already copied. The finalftruncateuses that sum.Fix
copy_file_innerlines, byte-identical. The two PRs merge cleanly in either order.test/js/node/fs/cp.test.ts, one per entry point. All fail on 1.4.3 (14096 bytes for a 10000 byte source). Alsocp.test.ts, the copyFile tests infs.test.ts, and the nodetest-fs-cp-*files.Background
copy_file_rangecopies bytes inside the kernel. Bun passes explicit offset pointers, so the fd offsets stay at 0. The fallback (sendfile, thenreadandwrite) uses the fd offsets.O_TRUNCand end withftruncate(dest, wrote), sowrotedecides the final length.fs.copyFileusescopy_file_inner.fs.cpusescopy_single_file_synconly for one regular file with default options. The directory walker is in JS and callscopyFileper file.Notes
ftruncatewould cut real data.copyFileSyncandcpSync.syscall(), because Bun issuescopy_file_rangethroughsyscall(2).test/js/bun/spawn/spawn-stdio-syscall-error.test.tsuses the same method. The shim copies one chunk of at most 4096 bytes on the firstcopy_file_rangecall and fails each later call withEXDEV. It prints one line per call to stderr. The tests assert those lines, so they fail if the shim does not intercept the call.cpSync(file),copyFileSync(file),promises.cp(file),promises.copyFile(file),cpSync(dir, { recursive: true }),cpSyncandcopyFileSyncof/proc/version(it reportsst_size0, so the copy-until-EOF loop runs: 438 bytes for a 219 byte source without the fix), andcpSync(file)over a longer destination (old bytes stay without the fix).BUN_CONFIG_DISABLE_ioctl_ficlonerange=1keeps a reflink filesystem from finishing the copy beforecopy_file_rangeruns.:5046,:8383,:8495) are not changed: the count is 0 there.cargo check -p bun_runtime --target x86_64-unknown-freebsdpasses.cpbuiltin does not reach this code on Linux: it isposix_disabled(src/runtime/shell/Builtin.rs:197).copy_file_range, so the fallback continues where the copy stopped. That change is a side change in a larger restructure and has no test for this case. The shim handles null offsets too, so these tests hold for both designs.let mut w = wrote.get();. This PR conflicts with them (and with node:fs: remove the destination when copyFile/cp fails partway #41798) on those lines. The tests catch a rebase that loses the fix.copy_single_file_syncsites. The review measured thatcopyFileSync,promises.copyFileand recursivecpstill wrote 14096 bytes, so the twocopy_file_innersites and the entry-point rows were added. Other concerns addressed in this text: who found the bug, the reach ofcopy_single_file_sync, the shell builtin, and the list of suites that ran.test/js/node/fs/cp-symlink-target.test.tsand all 79test-fs-cp-*andtest-fs-copyfile*files intest/js/node/test/parallel.copy_file_rangecall returns 0 for a source that has data (a procfs file on some older kernels), the copy is empty. That is a different question. Node writes an empty copy of/proc/versionon every kernel, because libuv trustsst_size.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/node/fs/cp.test.ts