Repository navigation
shell mv: trim the preallocated destination to the bytes copied on the cross-device fallback #41677
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -612,6 +612,16 @@ impl ShellMvBatchedTask { | |||||||||||||||||||||||||||
| let _ = bun_sys::unlinkat(dst_dir, dst); | ||||||||||||||||||||||||||||
| return Err(e); | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| // The source can hold fewer bytes than `st_size` said (it shrank, or it | ||||||||||||||||||||||||||||
| // is a sysfs file). The copy advanced the source position by the bytes | ||||||||||||||||||||||||||||
| // it copied; FICLONE does not move it, so fall back to the source size. | ||||||||||||||||||||||||||||
| let copied = match bun_sys::lseek(in_.fd(), 0, libc::SEEK_CUR) { | ||||||||||||||||||||||||||||
| Ok(pos) if pos > 0 => pos, | ||||||||||||||||||||||||||||
| _ => bun_sys::fstat(in_.fd()).map_or(st.st_size as i64, |s| s.st_size as i64), | ||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||
| if copied < st.st_size as i64 { | ||||||||||||||||||||||||||||
| let _ = bun_sys::ftruncate(out.fd(), copied); | ||||||||||||||||||||||||||||
|
Comment on lines
+620
to
+623
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win 🔎 Supported by static analysis🤖 get_repo_knowledge executed:
Length of output: 18943 🏁 Script executed: #!/bin/bash
set -eu
printf '%s\n' '--- outline ---'
ast-grep outline src/runtime/shell/builtin/mv.rs --match 'copy_file' --view expanded 2>/dev/null || true
printf '%s\n' '--- target ---'
sed -n '520,675p' src/runtime/shell/builtin/mv.rs
printf '%s\n' '--- related bindings and call sites ---'
rg -n -C 4 'ftruncate|unlinkat|copy_file|copied|remove.*dst|dst_dir' src/runtime/shell/builtin/mv.rsRepository: oven-sh/bun Length of output: 11937 🏁 Script executed: #!/bin/bash
set -eu
printf '%s\n' '--- function and error type context ---'
sed -n '430,535p' src/runtime/shell/builtin/mv.rs
sed -n '675,760p' src/runtime/shell/builtin/mv.rs
printf '%s\n' '--- exact bun_sys signatures/usages ---'
rg -n -C 3 'pub fn ftruncate|fn ftruncate|ftruncate\(' src | head -120
rg -n -C 3 'pub fn unlinkat|fn unlinkat|unlinkat\(' src | head -160
printf '%s\n' '--- destination ownership and source removal ---'
rg -n -C 5 'unlinkat\(src_dir|drop\(out\)|return Err' src/runtime/shell/builtin/mv.rsRepository: oven-sh/bun Length of output: 29782 🏁 Script executed: #!/bin/bash
set -eu
printf '%s\n' '--- complete relevant implementation ---'
sed -n '560,650p' src/runtime/shell/builtin/mv.rs
printf '%s\n' '--- bun_sys declarations ---'
rg -n -C 5 'ftruncate' src/runtime/bun_sys src | head -200
printf '%s\n' '--- unlinkat declarations ---'
rg -n -C 5 'unlinkat' src/runtime/bun_sys src | head -240Repository: oven-sh/bun Length of output: 31961 🏁 Script executed: #!/bin/bash
set -eu
sed -n '560,650p' src/runtime/shell/builtin/mv.rsRepository: oven-sh/bun Length of output: 4063 🏁 Script executed: #!/bin/bash
set -eu
printf '%s\n' '--- target implementation ---'
sed -n '560,650p' src/runtime/shell/builtin/mv.rs
printf '%s\n' '--- local API references ---'
rg -n -C 4 'ftruncate|unlinkat' src/runtime/shell/builtin/mv.rs src/runtime/bun_sys 2>/dev/null || trueRepository: oven-sh/bun Length of output: 7920 🏁 Script executed: #!/bin/bash
set -eu
printf '%s\n' '--- preallocation implementation and callers ---'
rg -n -C 12 'fn preallocate_file|pub fn preallocate_file|preallocate_file\(' src/sys src/runtime/shell/builtin/mv.rsRepository: oven-sh/bun Length of output: 3519 Handle truncation failures before removing the source. When Suggested fix if copied < st.st_size as i64 {
- let _ = bun_sys::ftruncate(out.fd(), copied);
+ if let Err(e) = bun_sys::ftruncate(out.fd(), copied) {
+ drop(out);
+ let _ = bun_sys::unlinkat(dst_dir, dst);
+ return Err(e);
+ }
}📝 Committable suggestion
Suggested change
🤖 Prompt for AI AgentsSource: MCP tools ✅ Confirmed as addressed by @robobun
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done in dd57331. A failed ftruncate now removes the destination and returns the error, the same as a failed copy_file.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
🧠 Learnings used✅ Review thread resolved. You are interacting with an AI system.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Correction: my reply above is wrong. Commit dd57331 never reached this branch. The push failed and I did not check the remote before I replied. The head (add2728) still discards the
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
As you explained, You are interacting with an AI system. |
||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| #[cfg(unix)] | ||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||
| // `fchown` first: Linux clears S_ISUID/S_ISGID on chown. | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,6 @@ | ||
| import { $ } from "bun"; | ||
| import { describe, expect, test } from "bun:test"; | ||
| import { isPosix } from "harness"; | ||
| import { isPosix, tempDir } from "harness"; | ||
| import { | ||
| accessSync, | ||
| chmodSync, | ||
|
|
@@ -118,6 +118,24 @@ describe("mv", async () => { | |
| } | ||
| }); | ||
|
|
||
| // The copy fallback preallocates the destination to the source's st_size. | ||
| // A sysfs file reports st_size 4096 but holds a few bytes, so the | ||
| // destination must shrink to the bytes copied. The unlink of the sysfs | ||
| // source fails, so mv exits non-zero, but the copy has already happened. | ||
| const sysfsFile = "/sys/devices/system/cpu/online"; | ||
| const hasSysfs = process.platform === "linux" && existsSync(sysfsFile); | ||
| test.skipIf(!hasSysfs)("across devices, the destination is the bytes copied, not st_size", async () => { | ||
| using dst = tempDir("bun-mv-xdev-short-read", {}); | ||
| const content = readFileSync(sysfsFile); | ||
| expect(content.length).toBeGreaterThan(0); | ||
| expect(statSync(sysfsFile).size).toBeGreaterThan(content.length); | ||
| const dstFile = join(String(dst), "online"); | ||
|
|
||
| const r = await $`mv ${sysfsFile} ${dstFile}`.quiet(); | ||
| expect(r.exitCode).not.toBe(0); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win 🔎 Supported by static analysis🏁 Script executed: #!/bin/bash
set -eu
printf '%s\n' '--- changed hunk ---'
git diff -- test/js/bun/shell/commands/mv.test.ts
printf '%s\n' '--- surrounding test ---'
sed -n '105,145p' test/js/bun/shell/commands/mv.test.ts
printf '%s\n' '--- nearby assertion-order examples ---'
rg -n -U -C 4 'readFileSync\([^)]*\)[\s\S]{0,180}exitCode|exitCode[\s\S]{0,180}readFileSync' test/js/bun/shell/commands/mv.test.ts test/js/bun/shell/commandsRepository: oven-sh/bun Length of output: 13555 Check destination content before the exit status. If 🤖 Prompt for AI AgentsSource: Learnings ✅ Confirmed as addressed by @robobun
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done in dd57331. The content assertion runs before the exit code assertion.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
🧠 Learnings used✅ Review thread resolved. You are interacting with an AI system.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Correction: my reply above is wrong. Commit dd57331 never reached this branch, so the head (add2728) still asserts the exit code before the content. I reopened the thread. The test is carried over with the content assertion first in 5276f89 (branch
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
🧠 Learnings usedYou are interacting with an AI system. |
||
| expect(readFileSync(dstFile)).toEqual(content); | ||
| }); | ||
|
|
||
| test.skipIf(skip)("file -> directory across devices", async () => { | ||
| const [src, dst] = crossDevicePair("into-dir"); | ||
| try { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 (optional) The
pos == 0fallback re-stats the source, which races with a concurrent writer: on a btrfs/XFS cross-subvolume move,FICLONEsnapshots the full source intoout, then if the source is truncated before thisfstat(in_)runs,copiedbecomes the shrunken size andftruncate(out, copied)discards the tail FICLONE just cloned — the base branch kept the full snapshot. Fix: derive the fallback length from the destination, not the source —bun_sys::fstat(out.fd())— sinceoutis a freshly-created inode only this process holds and itsst_sizeafter FICLONE is exactly the cloned length; no other case regresses.Extended reasoning...
Path:
renameatacross btrfs subvolumes → EXDEV →move_across_devices. Line 595fstat(in_)recordsst.st_size = N. Line 609 fallocatesoutto N.copy_fileat line 610 hitsioctl_ficlone(src/sys/copy_file.rs:128), which succeeds on same-superblock subvolumes and returns without touching either fd's offset —outnow holds an N-byte reflink snapshot. Line 618lseek(in_, 0, SEEK_CUR)→ 0, so the guard falls through to line 620fstat(in_.fd()). Between the ioctl and this fstat another process truncates the source to M < N.copied = M,M < N→ line 623ftruncate(out, M)drops N-M bytes that were correctly cloned, then line 632 unlinks the source. On the base branch there was no post-copy truncate, sooutstayed at the N-byte FICLONE snapshot.fstat(out.fd())avoids this becauseoutwas just created afterunlinkat(line 602) withO_CREAT|O_TRUNCand nobody else holds it; after FICLONE itsst_sizeis the snapshot length regardless of what later happens to the source, and for the non-FICLONEpos==0cases (empty file, macOS fcopyfile) it yields the same…Verification: normal — the FICLONE fallback introduces a TOCTOU data-loss regression the base branch does not have. Reachability of FICLONE in
move_across_devices: -bun_sys::copy_file(src/sys/lib.rs:7716) →copy_file::copy_file(src/sys/copy_file.rs:247-249) creates a freshCopyFileState::default()each call, soHAS_SEEN_EXDEVis NOT set even though this path is reached via a rename EXDEV. -… | nit…There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in dd57331. The fstat fallback is gone. When the position is 0 (FICLONE) the destination is left as the clone produced it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correction: my reply above is wrong. Commit dd57331 never reached this branch, so the head (add2728) still has the
fstat(in_)fallback and the race you describe. I reopened the thread. This pull request is closed in favor of #44425. That change keeps the file size when it preallocates, so the trim and its fallback are not needed.