Skip to content

shell mv: keep timestamps and the symlink owner on the cross-device fallback - #43397

Open
robobun wants to merge 1 commit into
robobun/fdea49ec/mv-exdev-drop-setidfrom
robobun/b652ccdd/mv-exdev-times
Open

robobun wants to merge 1 commit into
robobun/fdea49ec/mv-exdev-drop-setidfrom
robobun/b652ccdd/mv-exdev-times

Conversation

@robobun

@robobun robobun commented Sep 19, 2026 •

Copy link
Copy Markdown
Collaborator

Stacked on #43394. The base of this PR is that branch. The diff here is the last commit only.

Problem

Fix

  • copy_metadata (the copy_owner_and_mode helper of shell mv: drop set-uid and set-gid across devices when the owner cannot be kept #43394, renamed) calls futimens with the atime and mtime of the source, then fchown, then fchmod. Times come first: after fchown gives the copy to another user, the mover may lack the right to set them. coreutils uses the same order. For a directory the helper runs after the children are copied, because each child copy bumps the mtime of the new directory.
  • The symlink branch calls utimensat and fchownat, both with AT_SYMLINK_NOFOLLOW, after symlinkat.
  • Every new call is best effort, like the existing fchown and fchmod. bun_sys gets fchownat and a flag-taking utimensat. lutimens now calls the latter, with the same arguments as before.
  • Verified: test/js/bun/shell/commands/mv.test.ts, two new cases in cross-device (EXDEV). Both fail on bun 1.4.3 and on the base branch. Also the rest of test/js/bun/shell/commands/, and cargo check for aarch64-apple-darwin and x86_64-pc-windows-msvc.

Background

  • EXDEV is the errno for a rename across filesystems. The kernel cannot move the inode, so mv has to copy the data and every attribute it can, then remove the source.
  • futimens sets the access and modification times of an open file. utimensat does the same through a directory fd and a name. With AT_SYMLINK_NOFOLLOW both utimensat and fchownat act on the link itself, not on its target. A symbolic link has no fd, so the link branch needs the at forms.
  • An explicit time needs the caller to own the file, or to have CAP_FOWNER. The owner of the copy is the mover until fchown runs.
Notes

Not in this PR: POSIX ACLs and extended attributes. #43393 names them. They need new bun_sys wrappers with different signatures on Linux and macOS, so they are a separate change.

Repro on 1.4.3 (Linux, /tmp and /dev/shm on different filesystems):

mkdir -p /tmp/mvsrc && echo DATA > /tmp/mvsrc/f && touch -d '2001-02-03 04:05:06' /tmp/mvsrc/f
bun -e 'import {$} from "bun"; await $`mv /tmp/mvsrc/f /dev/shm/f`'
stat -c '%y %n' /dev/shm/f

Before: the time of the copy. After: 2001-02-03 04:05:06. The same holds for a directory (touch -d on the directory) and a symbolic link (touch -h -d).

The symlink owner test runs as root only, because only root can give a link to another user. It sets the owner of the source link to 65534 with lchownSync and checks lstat of the copy.

The atime of a regular file comes from the fstat taken before copy_file reads the data, so the copy gets the atime from before the move.

The test file also has two pre-existing ls failures when run as root (permission denied directory). They fail on main as root too and are unrelated.

Self-reviewed: the review found the conflict with #43394 on the same fchown/fchmod lines and asked for the times-first order. Both are addressed by the stack and the helper order.


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/bun/shell/commands/mv.test.ts

…allback

When renameat returns EXDEV, the mv builtin copies the source and
removes it. The copy now gets the atime and mtime of the source, for a
regular file, a directory, and a symbolic link. The new symbolic link
also gets the owner of the source.

Adds bun_sys::fchownat and bun_sys::utimensat.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Beyond the inline findings, I also checked the lutimens refactor in src/sys/lib.rs for behavior drift — Fd::cwd() is Fd(libc::AT_FDCWD) on unix and the path/flags/timespec arguments are unchanged, so existing callers still issue the same utimensat(AT_FDCWD, ..., AT_SYMLINK_NOFOLLOW) call.

Extended reasoning...

Findings were reported inline, so this body only records what else was examined. The lutimens -> utimensat refactor was the one non-mv change that could silently alter existing behavior; tracing Fd::cwd() to src/bun_core/util.rs:980 confirms it wraps AT_FDCWD on unix, and the new wrapper passes the caller's flags verbatim, so the refactor is behavior-preserving. The mv.rs call sites are all #[cfg(unix)]-gated and the new wrappers live in posix_impl, consistent with the PR's claim of a Windows cargo check. The inline findings (symlink chown race, unix-only coverage) still warrant a human look.

Comment thread src/runtime/shell/builtin/mv.rs Resolved
Comment thread src/runtime/shell/builtin/mv.rs Resolved
@robobun

robobun commented Sep 19, 2026

Copy link
Copy Markdown
Collaborator Author

Both inline findings are answered in their threads. No code change.

  • The by-name owner call on the new symlink is the sequence coreutils mv uses (symlinkat, then lchownat, then utimensat with AT_SYMLINK_NOFOLLOW). The file branch of the fallback already trusts the destination directory more than this call (O_CREAT | O_TRUNC after an unlink, no O_EXCL). A hardening against a hostile destination directory is a separate change for every branch.
  • The Windows copy path keeps the behaviour of the base branch. bun_sys has no futimens for Windows.

@robobun

robobun commented Sep 19, 2026

Copy link
Copy Markdown
Collaborator Author
Updated 11:16 PM PT - Sep 18th, 2026

✅ @robobun, your commit 9f237fba3e665999781a05ab47afdbec1172623f passed in Build #118178! 🎉


🧪   To try this PR locally:

bunx bun-pr 43397

That installs a local version of the PR into your bun-43397 executable, so you can run:

bun-43397 --bun

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant