Skip to content

mv:fix file ownership changes when a file is mv'ed by root to a different file system - #9672

Open
mattsu2020 wants to merge 6 commits into
uutils:mainfrom
mattsu2020:mv_fix
Open

mv:fix file ownership changes when a file is mv'ed by root to a different file system#9672
mattsu2020 wants to merge 6 commits into
uutils:mainfrom
mattsu2020:mv_fix

Conversation

@mattsu2020

Copy link
Copy Markdown
Contributor

Summary

Fix cross-filesystem mv (EXDEV copy+delete fallback) so that file ownership does not change to the invoking user (e.g. root) when moving a file across filesystems.

Fixes #9635.

Background / Problem

When mv cannot rename(2) across devices (EXDEV), uutils falls back to copy+delete. The copy path used std::fs::copy, which creates the destination owned by the caller. If root moves a file owned by another user to a different filesystem, the destination ends up owned by root (compatibility + security concern).

Changes

  • On Unix, preserve uid/gid and mode after the copy step in the EXDEV fallback:
    • regular files (including the hardlink-aware copy path)
    • directories (including recursively created subdirectories)
    • symlinks (use lchown, do not follow)
    • FIFOs
  • Restore mode after chown to keep correct permission bits (since chown may clear setuid/setgid).
  • Add a Linux-only regression test that runs only as root and verifies uid/gid are preserved across partitions (/dev/shm tmpfs).
  • Address clippy (bind_instead_of_map) in the copy path.

Testing

  • cargo test -p uu_mv
  • cargo clippy -p uu_mv -- -D warnings

related
#9635

@github-actions

Copy link
Copy Markdown

GNU testsuite comparison:

Skipping an intermittent issue tests/tail/overlay-headers (passes in this run but fails in the 'main' branch)

@oech3

This comment was marked as resolved.

Comment thread tests/by-util/test_mv.rs Outdated
.env("BASE", &base)
.env("UUTILS", &scene.bin_path)
.output()
.expect("failed to run unshare");

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.

It might better to create a new function for uhshare (at different PR).
Ref: #9973

@github-actions

Copy link
Copy Markdown

GNU testsuite comparison:

Skipping an intermittent issue tests/timeout/timeout (passes in this run but fails in the 'main' branch)

Comment thread src/uu/mv/src/mv.rs
@@ -816,6 +816,44 @@ fn is_fifo(_filetype: fs::FileType) -> bool {
false
}

#[cfg(unix)]
fn try_preserve_ownership(from_meta: &fs::Metadata, to: &Path, follow_symlinks: bool) {

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.

document this function please

Comment thread src/uu/mv/src/mv.rs Outdated

unsafe {
if follow_symlinks {
let _ = libc::chown(to_cstr.as_ptr(), uid, gid);

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.

please manage the errors

Comment thread src/uu/mv/src/mv.rs
}

#[cfg(unix)]
fn try_preserve_permissions(from_meta: &fs::Metadata, to: &Path) {

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.

same, please document it

Comment thread src/uu/mv/src/mv.rs Outdated

// Keep mode bits only (file type bits are not allowed in chmod).
let mode = from_meta.mode() & 0o7777;
let _ = fs::set_permissions(to, fs::Permissions::from_mode(mode));

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.

please manage the error

@codspeed-hq

codspeed-hq Bot commented Jan 19, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 299 untouched benchmarks
⏩ 46 skipped benchmarks1


Comparing mattsu2020:mv_fix (7058753) with main (e0f0318)

Open in CodSpeed

Footnotes

  1. 46 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

@github-actions

Copy link
Copy Markdown

GNU testsuite comparison:

Skip an intermittent issue tests/tty/tty-eof (fails in this run but passes in the 'main' branch)

Comment thread src/uu/mv/src/mv.rs
Comment thread src/uu/mv/src/mv.rs Outdated
fn copy_symlink(from: &Path, to: &Path) -> io::Result<()> {
let from_meta = from.symlink_metadata()?;
let path_symlink_points_to = fs::read_link(from)?;
unix::fs::symlink(path_symlink_points_to, to).map(|_| {

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.

it should copy xattrs like rename_symlink_fallback does. no ?

Comment thread src/uu/mv/src/mv.rs Outdated
@github-actions

Copy link
Copy Markdown

GNU testsuite comparison:

Congrats! The gnu test tests/mv/meta-to-xpart is no longer failing!

@github-actions

Copy link
Copy Markdown

GNU testsuite comparison:

GNU test failed: tests/timeout/timeout-group. tests/timeout/timeout-group is passing on 'main'. Maybe you have to rebase?
Congrats! The gnu test tests/mv/meta-to-xpart is no longer failing!

@ChrisDryden

Copy link
Copy Markdown
Collaborator

I think we have a chmod helper that we can use here that implements all of the logic in the PR already in the uucore perms lib

@sylvestre

Copy link
Copy Markdown
Contributor

and it conflicts

@mattsu2020

Copy link
Copy Markdown
Contributor Author

I think we have a chmod helper that we can use here that implements all of the logic in the PR already in the uucore perms lib

wrap_chown is too high-level for mv compatibility logic. It returns formatted Result<String, String>, so mv cannot inspect errno to implement GNU-like behavior (owner+group attempt, optional group-only fallback, and selective suppression for EPERM/EACCES/EINVAL).

To avoid behavior regressions, I’d like to add a public low-level helper in uucore::perms that returns io::Result<()> (preserving errno), then switch mv to that helper while keeping GNU-compatible branching in mv. This still removes syscall duplication, but without changing user-visible behavior.

@github-actions

Copy link
Copy Markdown

GNU testsuite comparison:

GNU test failed: tests/pr/bounded-memory. tests/pr/bounded-memory is passing on 'main'. Maybe you have to rebase?
GNU test failed: tests/rm/isatty. tests/rm/isatty is passing on 'main'. Maybe you have to rebase?
Congrats! The gnu test tests/mv/meta-to-xpart is no longer failing!
Note: The gnu test tests/printf/printf-surprise is now being skipped but was previously passing.
Congrats! The gnu test tests/expand/bounded-memory is now passing!

@github-actions

Copy link
Copy Markdown

GNU testsuite comparison:

GNU test failed: tests/cp/parent-perm-race. tests/cp/parent-perm-race is passing on 'main'. Maybe you have to rebase?
GNU test failed: tests/factor/t10. tests/factor/t10 is passing on 'main'. Maybe you have to rebase?
GNU test failed: tests/timeout/timeout-group. tests/timeout/timeout-group is passing on 'main'. Maybe you have to rebase?
Congrats! The gnu test tests/factor/t29 is no longer failing!
Congrats! The gnu test tests/mv/meta-to-xpart is no longer failing!
Congrats! The gnu test tests/pr/bounded-memory is no longer failing!
Note: The gnu test tests/dd/no-allocate is now being skipped but was previously passing.
Congrats! The gnu test tests/cut/cut-huge-range is now passing!
Congrats! The gnu test tests/rm/many-dir-entries-vs-OOM is now passing!

@github-actions

Copy link
Copy Markdown

GNU testsuite comparison:

GNU test failed: tests/date/date-locale-hour. tests/date/date-locale-hour is passing on 'main'. Maybe you have to rebase?
Congrats! The gnu test tests/cut/bounded-memory is no longer failing!
Congrats! The gnu test tests/mv/meta-to-xpart is no longer failing!
Note: The gnu test tests/basenc/bounded-memory is now being skipped but was previously passing.
Note: The gnu test tests/cp/link-heap is now being skipped but was previously passing.
Note: The gnu test tests/dd/no-allocate is now being skipped but was previously passing.

Reordered import statements in test_df.rs and test_mv.rs to follow a consistent pattern, moving conditional imports and utility functions to their proper positions within the import blocks. This improves code readability and maintains consistency across test files.
@sylvestre

Copy link
Copy Markdown
Contributor

@mattsu2020 some lint issues, sorry!

mattsu2020 and others added 2 commits March 20, 2026 19:52
- Use entry.file_type() for efficient type checking (mv_fix approach)
- Use copy_file_with_hardlinks_helper on Unix for hardlink support (main approach)
- Use rename_symlink_fallback on non-Unix (main approach)
- Symlinks handled before regular files, no need for second symlink check
@github-actions

Copy link
Copy Markdown

GNU testsuite comparison:

Skipping an intermittent issue tests/cut/bounded-memory (passes in this run but fails in the 'main' branch)
Skipping an intermittent issue tests/date/date-locale-hour (passes in this run but fails in the 'main' branch)
Congrats! The gnu test tests/mv/meta-to-xpart is no longer failing!
Note: The gnu test tests/seq/seq-epipe is now being skipped but was previously passing.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG/Compatibility BUG/Security BUG] File ownership changes when a file is mv'ed by root to a different file system.

4 participants