diff --git a/src/uu/mv/Cargo.toml b/src/uu/mv/Cargo.toml index ee0bc3b6a54..ea0d8af5dd6 100644 --- a/src/uu/mv/Cargo.toml +++ b/src/uu/mv/Cargo.toml @@ -30,6 +30,7 @@ uucore = { workspace = true, features = [ "fs", "fsxattr", "perms", + "safe-copy", "update-control", ] } fluent = { workspace = true } diff --git a/src/uu/mv/locales/en-US.ftl b/src/uu/mv/locales/en-US.ftl index 4bb5339fe32..8f0eb32bc6a 100644 --- a/src/uu/mv/locales/en-US.ftl +++ b/src/uu/mv/locales/en-US.ftl @@ -32,6 +32,7 @@ mv-error-backup-might-destroy-source = backing up {$target} might destroy source mv-error-will-not-overwrite-just-created = will not overwrite just-created {$target} with {$source} mv-error-not-replacing = not replacing {$target} mv-error-cannot-move = cannot move {$source} to {$target} +mv-error-cannot-overwrite = cannot overwrite {$target} mv-error-directory-not-empty = Directory not empty mv-error-dangling-symlink = can't determine symlink type, since it is dangling mv-error-no-symlink-support = your operating system does not support symlinks diff --git a/src/uu/mv/locales/fr-FR.ftl b/src/uu/mv/locales/fr-FR.ftl index 2c7a279b74b..ee1738ef80d 100644 --- a/src/uu/mv/locales/fr-FR.ftl +++ b/src/uu/mv/locales/fr-FR.ftl @@ -32,6 +32,7 @@ mv-error-backup-might-destroy-source = sauvegarder {$target} pourrait détruire mv-error-will-not-overwrite-just-created = ne va pas écraser le fichier qui vient d'être créé {$target} avec {$source} mv-error-not-replacing = ne remplace pas {$target} mv-error-cannot-move = impossible de déplacer {$source} vers {$target} +mv-error-cannot-overwrite = impossible d'écraser {$target} mv-error-directory-not-empty = Répertoire non vide mv-error-dangling-symlink = impossible de déterminer le type de lien symbolique, car il est suspendu mv-error-no-symlink-support = votre système d'exploitation ne prend pas en charge les liens symboliques diff --git a/src/uu/mv/src/mv.rs b/src/uu/mv/src/mv.rs index b750428f7e5..fcbe6b2155e 100644 --- a/src/uu/mv/src/mv.rs +++ b/src/uu/mv/src/mv.rs @@ -3,7 +3,7 @@ // For the full copyright and license information, please view the LICENSE // file that was distributed with this source code. -// spell-checker:ignore (ToDO) sourcepath targetpath nushell canonicalized unwriteable +// spell-checker:ignore (ToDO) sourcepath targetpath nushell canonicalized unwriteable callees mod error; #[cfg(unix)] @@ -407,14 +407,14 @@ fn handle_two_paths(source: &Path, target: &Path, opts: &Options) -> UResult<()> #[cfg(not(unix))] let hardlink_params = (None, None); - rename( + consume_already_reported(rename( source, target, opts, None, hardlink_params.0, hardlink_params.1, - ) + )) .map_err_context(|| { translate!("mv-error-cannot-move", "source" => source.quote(), "target" => target.quote()) }) @@ -449,14 +449,14 @@ fn handle_two_paths(source: &Path, target: &Path, opts: &Options) -> UResult<()> #[cfg(not(unix))] let hardlink_params = (None, None); - rename( + consume_already_reported(rename( source, target, opts, None, hardlink_params.0, hardlink_params.1, - ) + )) .map_err(|e| USimpleError::new(1, format!("{e}"))) } } @@ -658,26 +658,22 @@ fn move_files_into_dir(files: &[PathBuf], target_dir: &Path, options: &Options) #[cfg(not(unix))] let hardlink_params = (None, None); - match rename( + if let Err(e) = consume_already_reported(rename( sourcepath, &targetpath, options, display_manager.as_ref(), hardlink_params.0, hardlink_params.1, - ) { - Err(e) if e.to_string().is_empty() => set_exit_code(1), - Err(e) => { - let e = e.map_err_context(|| { - translate!("mv-error-cannot-move", "source" => sourcepath.quote(), "target" => targetpath.quote()) - }); - if let Some(ref pb) = display_manager { - pb.suspend(|| show!(e)); - } else { - show!(e); - } + )) { + let e = e.map_err_context(|| { + translate!("mv-error-cannot-move", "source" => sourcepath.quote(), "target" => targetpath.quote()) + }); + if let Some(ref pb) = display_manager { + pb.suspend(|| show!(e)); + } else { + show!(e); } - Ok(()) => (), } if let Some(ref pb) = count_progress { pb.inc(1); @@ -687,6 +683,20 @@ fn move_files_into_dir(files: &[PathBuf], target_dir: &Path, options: &Options) Ok(()) } +/// `rename()` (and its callees like `prompt_overwrite`) signals +/// "error already reported, just propagate the failure exit code" by +/// returning an io::Error whose message is empty. Convert that sentinel into +/// `Ok(())` after bumping the exit code so callers don't double-print. +fn consume_already_reported(result: io::Result<()>) -> io::Result<()> { + match result { + Err(e) if e.to_string().is_empty() => { + set_exit_code(1); + Ok(()) + } + other => other, + } +} + fn rename( from: &Path, to: &Path, @@ -750,7 +760,19 @@ fn rename( if is_empty_dir(to) { fs::remove_dir(to)?; } else { - return Err(io::Error::other(translate!("mv-error-directory-not-empty"))); + // GNU's mv reports "cannot overwrite 'TARGET': Directory not + // empty" for this case, *not* "cannot move SRC to TARGET: ...". + // Print it here and return an empty error so the caller takes + // the silent-failure path that just sets the exit code. + show!(USimpleError::new( + 1, + format!( + "{}: {}", + translate!("mv-error-cannot-overwrite", "target" => to.quote()), + translate!("mv-error-directory-not-empty"), + ), + )); + return Err(io::Error::other("")); } } } @@ -903,16 +925,84 @@ fn rename_fifo_fallback(_from: &Path, _to: &Path) -> io::Result<()> { #[cfg(unix)] fn rename_symlink_fallback(from: &Path, to: &Path) -> io::Result<()> { let path_symlink_points_to = fs::read_link(from)?; - unix::fs::symlink(path_symlink_points_to, to)?; + + // Fast path: create the symlink directly. If the destination already + // exists, fall through to the atomic temp-and-rename path. Matches GNU + // mv, which replaces the existing destination. See issue #10010. + match unix::fs::symlink(&path_symlink_points_to, to) { + Ok(()) => {} + Err(e) if e.kind() == io::ErrorKind::AlreadyExists => { + create_symlink_replace(&path_symlink_points_to, to)?; + } + Err(e) => return Err(e), + } #[cfg(not(any(target_os = "macos", target_os = "redox")))] { - let _ = copy_xattrs_if_supported(from, to); + let _ = fsxattr::copy_xattrs_ignore_unsupported(from, to); } // Preserve ownership (uid/gid) from the source symlink let _ = preserve_ownership(from, to); fs::remove_file(from) } +/// Create a symlink at `to` pointing to `target`, replacing any existing +/// file/symlink at `to` atomically. Creates the new symlink at a temp +/// name in `to`'s parent directory, then renames it into place. `rename` +/// is atomic on Linux, so a concurrent observer of `to` never sees it +/// absent. See issue #10010. +#[cfg(unix)] +fn create_symlink_replace(target: &Path, to: &Path) -> io::Result<()> { + use std::ffi::{OsStr, OsString}; + use std::os::unix::ffi::OsStrExt; + + let parent = to + .parent() + .filter(|p| !p.as_os_str().is_empty()) + .unwrap_or_else(|| Path::new(".")); + let basename = to + .file_name() + .ok_or_else(|| io::Error::new(io::ErrorKind::InvalidInput, "invalid destination path"))?; + let pid = std::process::id(); + + // Try a few times with a fresh nanos suffix on EEXIST. The pid+nanos + // pair is already unique in practice, but pid namespaces and clock + // coarsening can collide; retry rather than overwrite a stranger's + // temp file. + for _ in 0..8 { + let nanos = std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .map(|d| d.as_nanos()) + .unwrap_or(0); + + // Build the temp name as raw bytes so non-UTF-8 basenames survive + // intact (no U+FFFD substitution). Truncate the basename so the + // total length stays under a conservative NAME_MAX of 255 bytes. + let prefix = format!(".mv-tmp-{pid}-{nanos}-"); + let max_base_len = 255usize.saturating_sub(prefix.len()); + let base_bytes = basename.as_bytes(); + let base_slice = &base_bytes[..base_bytes.len().min(max_base_len)]; + let mut tmp_name = OsString::from(prefix); + tmp_name.push(OsStr::from_bytes(base_slice)); + let tmp_path = parent.join(&tmp_name); + + match unix::fs::symlink(target, &tmp_path) { + Ok(()) => { + if let Err(e) = fs::rename(&tmp_path, to) { + let _ = fs::remove_file(&tmp_path); + return Err(e); + } + return Ok(()); + } + Err(e) if e.kind() == io::ErrorKind::AlreadyExists => {} + Err(e) => return Err(e), + } + } + Err(io::Error::new( + io::ErrorKind::AlreadyExists, + "could not allocate a unique temp name in destination directory", + )) +} + #[cfg(windows)] fn rename_symlink_fallback(from: &Path, to: &Path) -> io::Result<()> { let path_symlink_points_to = fs::read_link(from)?; @@ -1171,7 +1261,7 @@ fn copy_file_with_hardlinks_helper( // Copy xattrs, ignoring ENOTSUP errors (filesystem doesn't support xattrs) #[cfg(all(unix, not(any(target_os = "macos", target_os = "redox"))))] { - let _ = copy_xattrs_if_supported(from, to); + let _ = fsxattr::copy_xattrs_ignore_unsupported(from, to); } // Preserve ownership (uid/gid) from the source let _ = preserve_ownership(from, to); @@ -1186,16 +1276,22 @@ fn rename_file_fallback( #[cfg(unix)] hardlink_tracker: Option<&mut HardlinkTracker>, #[cfg(unix)] hardlink_scanner: Option<&HardlinkGroupScanner>, ) -> io::Result<()> { - // Remove existing target file if it exists + // If the destination is a symlink, remove it first so the subsequent + // `fs::copy` does not follow the symlink and write to the target. The + // remaining race window (between this unlink and the open inside + // `fs::copy`) matches GNU mv; the separate pre-copy unlink of regular + // files was an additional window unique to uutils and has been removed. + // See issue #10015. if to.is_symlink() { fs::remove_file(to).map_err(|err| { let inter_device_msg = translate!("mv-error-inter-device-move-failed", "from" => from.quote(), "to" => to.quote(), "err" => err); io::Error::new(err.kind(), inter_device_msg) })?; - } else if to.exists() { - // For non-symlinks, just remove the file without special error handling - fs::remove_file(to)?; } + // For regular-file destinations we intentionally do NOT unlink here. + // `fs::copy` opens with `O_WRONLY|O_CREAT|O_TRUNC`, which truncates an + // existing regular file in place — matching GNU mv and avoiding the + // extra unlink/copy race window. // Check if this file is part of a hardlink group and if so, create a hardlink instead of copying #[cfg(unix)] @@ -1214,20 +1310,62 @@ fn rename_file_fallback( } } - // Regular file copy - fs::copy(from, to) - .map_err(|err| io::Error::new(err.kind(), translate!("mv-error-permission-denied")))?; - - // Copy xattrs, ignoring ENOTSUP errors (filesystem doesn't support xattrs) - #[cfg(all(unix, not(any(target_os = "macos", target_os = "redox"))))] + // Regular file copy. On Unix, we open source and destination via the + // shared `uucore::safe_copy` helpers and keep the file descriptors alive + // across the content copy, mode restoration, and xattr copy. This pins + // both inodes so a concurrent path renamer cannot redirect later xattr + // list/get/set syscalls to different inodes (issue #10014). Both opens + // use O_NOFOLLOW so the same path-swap-to-symlink TOCTOU that cp #10017 + // closes for cp's regular-file path is closed here for mv too — on the + // source so we don't read through an attacker-planted symlink, and on + // the dest so a planted symlink can't redirect the truncate and write + // to a victim file the caller has permission to write. + // + // `std::io::copy` has a File→File specialization that uses + // `copy_file_range`/`sendfile` on Linux, so this is roughly as fast as + // `fs::copy` and not a regression for large files. + #[cfg(unix)] { - let _ = copy_xattrs_if_supported(from, to); + use std::fs::Permissions; + use std::os::unix::fs::{MetadataExt, PermissionsExt}; + use uucore::safe_copy::{create_dest_restrictive, open_source}; + let src_file = open_source(from, /* nofollow */ true) + .map_err(|err| io::Error::new(err.kind(), translate!("mv-error-permission-denied")))?; + let src_mode = src_file + .metadata() + .map_err(|err| io::Error::new(err.kind(), translate!("mv-error-permission-denied")))? + .mode() + & 0o7777; + let mut dst_file = create_dest_restrictive(to, /* nofollow */ true) + .map_err(|err| io::Error::new(err.kind(), translate!("mv-error-permission-denied")))?; + io::copy(&mut &src_file, &mut dst_file) + .map_err(|err| io::Error::new(err.kind(), translate!("mv-error-permission-denied")))?; + + // Copy xattrs on the pinned fds. Errors are best-effort to match + // the previous path-based behavior; filesystems that don't support + // xattrs are a legal destination for cross-device moves. + #[cfg(not(any(target_os = "macos", target_os = "redox")))] + { + let _ = fsxattr::copy_xattrs_fd_ignore_unsupported(&src_file, &dst_file); + } + + // Preserve ownership (uid/gid) before applying the final mode. + // chown(2) clears S_ISUID/S_ISGID for non-root callers, so applying + // the mode after the chown is the only way to faithfully preserve + // setuid/setgid bits — matching GNU mv / cp's ordering. + let _ = preserve_ownership(from, to); + + // Now widen to the source's full mode (including setuid/setgid/ + // sticky). The restrictive 0o600 used at create_dest_restrictive() + // opened the dest narrowly so other users couldn't observe the + // file mid-copy. + let _ = dst_file.set_permissions(Permissions::from_mode(src_mode)); } - // Preserve ownership (uid/gid) from the source file - #[cfg(unix)] + #[cfg(not(unix))] { - let _ = preserve_ownership(from, to); + fs::copy(from, to) + .map_err(|err| io::Error::new(err.kind(), translate!("mv-error-permission-denied")))?; } fs::remove_file(from) @@ -1272,18 +1410,6 @@ fn preserve_ownership(from: &Path, to: &Path) -> io::Result<()> { Ok(()) } -/// Copy xattrs from source to destination, ignoring ENOTSUP/EOPNOTSUPP errors. -/// These errors indicate the filesystem doesn't support extended attributes, -/// which is acceptable when moving files across filesystems. -#[cfg(all(unix, not(any(target_os = "macos", target_os = "redox"))))] -fn copy_xattrs_if_supported(from: &Path, to: &Path) -> io::Result<()> { - match fsxattr::copy_xattrs(from, to) { - Ok(()) => Ok(()), - Err(e) if e.raw_os_error() == Some(libc::EOPNOTSUPP) => Ok(()), - Err(e) => Err(e), - } -} - fn is_empty_dir(path: &Path) -> bool { fs::read_dir(path).is_ok_and(|mut contents| contents.next().is_none()) } diff --git a/src/uucore/Cargo.toml b/src/uucore/Cargo.toml index 2cb8346394d..f45900eb5c2 100644 --- a/src/uucore/Cargo.toml +++ b/src/uucore/Cargo.toml @@ -138,7 +138,7 @@ extendedbigdecimal = ["bigdecimal", "num-traits"] fast-inc = [] fs = ["dunce", "libc", "winapi-util", "windows-sys"] fsext = ["libc", "windows-sys", "bstr"] -fsxattr = ["xattr", "itertools"] +fsxattr = ["xattr", "itertools", "libc"] hardware = [] lines = [] feat_systemd_logind = ["utmpx", "libc"] diff --git a/src/uucore/src/lib/features/fsxattr.rs b/src/uucore/src/lib/features/fsxattr.rs index d9683264652..7f552760011 100644 --- a/src/uucore/src/lib/features/fsxattr.rs +++ b/src/uucore/src/lib/features/fsxattr.rs @@ -3,7 +3,7 @@ // For the full copyright and license information, please view the LICENSE // file that was distributed with this source code. -// spell-checker:ignore getxattr posix_acl_default +// spell-checker:ignore getxattr posix_acl_default ENOTSUP EOPNOTSUPP //! Set of functions to manage xattr on files and dirs use itertools::Itertools; @@ -13,16 +13,37 @@ use std::ffi::{OsStr, OsString}; use std::os::unix::ffi::OsStrExt; use std::path::Path; +/// Returns true if the error is the kernel/filesystem signaling that +/// extended attributes are not supported (`ENOTSUP` / `EOPNOTSUPP`). +/// On Linux these are the same errno; on the BSDs they differ, so we +/// match on either. +#[cfg(unix)] +fn is_xattr_unsupported(err: &std::io::Error) -> bool { + matches!( + err.raw_os_error(), + Some(e) if e == libc::ENOTSUP || e == libc::EOPNOTSUPP + ) +} + +#[cfg(not(unix))] +fn is_xattr_unsupported(_err: &std::io::Error) -> bool { + false +} + /// Copies extended attributes (xattrs) from one file or directory to another. /// +/// All errors propagate, including `ENOTSUP` / `EOPNOTSUPP` from a +/// destination filesystem that does not support xattrs. Callers that +/// treat xattr preservation as best-effort (e.g. mv's cross-device +/// fallback, or cp without an explicit `--preserve=xattr`) should use +/// [`copy_xattrs_ignore_unsupported`] instead. cp's required-preserve +/// path relies on the strict behavior here to match GNU cp's exit code +/// when the destination filesystem rejects xattrs. +/// /// # Arguments /// /// * `source` - A reference to the source path. /// * `dest` - A reference to the destination path. -/// -/// # Returns -/// -/// A result indicating success or failure. pub fn copy_xattrs>(source: P, dest: P) -> std::io::Result<()> { for attr_name in xattr::list(&source)? { if let Some(value) = xattr::get(&source, &attr_name)? { @@ -32,6 +53,56 @@ pub fn copy_xattrs>(source: P, dest: P) -> std::io::Result<()> { Ok(()) } +/// Like [`copy_xattrs`], but maps `ENOTSUP` / `EOPNOTSUPP` from any of +/// the underlying list/get/set calls to `Ok(())`. Use from callers +/// where xattr preservation is best-effort and a destination filesystem +/// without xattr support is a legitimate target (mv across devices, +/// cp without explicit `--preserve=xattr`). +pub fn copy_xattrs_ignore_unsupported>(source: P, dest: P) -> std::io::Result<()> { + match copy_xattrs(source, dest) { + Ok(()) => Ok(()), + Err(e) if is_xattr_unsupported(&e) => Ok(()), + Err(e) => Err(e), + } +} + +/// Copies extended attributes between two open file descriptors. +/// +/// Unlike [`copy_xattrs`], which re-resolves `source` and `dest` paths on +/// every syscall (leaving a TOCTOU window a concurrent renamer could +/// exploit to redirect individual list/get/set calls to different +/// inodes), this variant operates on pinned file descriptors so every +/// xattr read is from the same inode and every write goes to the same +/// inode. See issue #10014. +/// +/// All errors propagate, matching [`copy_xattrs`]. Callers that want +/// `ENOTSUP` / `EOPNOTSUPP` mapped to success should use +/// [`copy_xattrs_fd_ignore_unsupported`]. +#[cfg(unix)] +pub fn copy_xattrs_fd(source: &std::fs::File, dest: &std::fs::File) -> std::io::Result<()> { + use xattr::FileExt; + for attr_name in source.list_xattr()? { + if let Some(value) = source.get_xattr(&attr_name)? { + dest.set_xattr(&attr_name, &value)?; + } + } + Ok(()) +} + +/// Like [`copy_xattrs_fd`], but maps `ENOTSUP` / `EOPNOTSUPP` to +/// `Ok(())`. The fd-based companion to [`copy_xattrs_ignore_unsupported`]. +#[cfg(unix)] +pub fn copy_xattrs_fd_ignore_unsupported( + source: &std::fs::File, + dest: &std::fs::File, +) -> std::io::Result<()> { + match copy_xattrs_fd(source, dest) { + Ok(()) => Ok(()), + Err(e) if is_xattr_unsupported(&e) => Ok(()), + Err(e) => Err(e), + } +} + /// Like `copy_xattrs`, but skips the security.selinux attribute. #[cfg(unix)] pub fn copy_xattrs_skip_selinux>(source: P, dest: P) -> std::io::Result<()> { @@ -200,6 +271,36 @@ mod tests { assert_eq!(copied_value, test_value); } + #[test] + #[cfg(unix)] + fn test_copy_xattrs_fd() { + let temp_dir = tempdir().unwrap(); + let source_path = temp_dir.path().join("source.txt"); + let dest_path = temp_dir.path().join("dest.txt"); + + File::create(&source_path).unwrap(); + File::create(&dest_path).unwrap(); + + let test_attr = "user.fd_test"; + let test_value = b"fd value"; + // If the filesystem doesn't support user xattrs (e.g. some CI runners + // on overlayfs), skip without failing. + if xattr::set(&source_path, test_attr, test_value).is_err() { + return; + } + + let src = File::open(&source_path).unwrap(); + let dst = std::fs::OpenOptions::new() + .write(true) + .open(&dest_path) + .unwrap(); + + copy_xattrs_fd(&src, &dst).unwrap(); + + let copied = xattr::get(&dest_path, test_attr).unwrap().unwrap(); + assert_eq!(copied, test_value); + } + #[test] fn test_apply_and_retrieve_xattrs() { let temp_dir = tempdir().unwrap(); diff --git a/src/uucore/src/lib/features/safe_copy.rs b/src/uucore/src/lib/features/safe_copy.rs index e3ec78f56f8..4a63282c535 100644 --- a/src/uucore/src/lib/features/safe_copy.rs +++ b/src/uucore/src/lib/features/safe_copy.rs @@ -77,11 +77,7 @@ pub fn create_dest_restrictive>(path: P, nofollow: bool) -> io::R if nofollow { flags |= OFlags::NOFOLLOW; } - let fd: OwnedFd = open( - path.as_ref(), - flags, - Mode::from_bits_truncate(DEST_INITIAL_MODE), - )?; + let fd: OwnedFd = open(path.as_ref(), flags, Mode::RUSR.union(Mode::WUSR))?; Ok(File::from(fd)) } diff --git a/tests/by-util/test_mv.rs b/tests/by-util/test_mv.rs index 33d54383620..cc6c2d43db1 100644 --- a/tests/by-util/test_mv.rs +++ b/tests/by-util/test_mv.rs @@ -1458,15 +1458,13 @@ fn test_mv_overwrite_nonempty_dir() { at.mkdir(dir_a); at.mkdir(dir_b); at.touch(dummy); - // Not same error as GNU; the error message is a rust builtin - // TODO: test (and implement) correct error message (or at least decide whether to do so) - // Current: "mv: couldn't rename path (Directory not empty; from=a; to=b)" - // GNU: "mv: cannot move 'a' to 'b': Directory not empty" - - // Verbose output for the move should not be shown on failure + // GNU 9.11: "mv: cannot overwrite 'b': Directory not empty" + // Verbose output for the move should not be shown on failure. let result = ucmd.arg("-vT").arg(dir_a).arg(dir_b).fails(); result.no_stdout(); - assert!(!result.stderr_str().is_empty()); + result.stderr_is(format!( + "mv: cannot overwrite '{dir_b}': Directory not empty\n" + )); assert!(at.dir_exists(dir_a)); assert!(at.dir_exists(dir_b)); @@ -2597,11 +2595,15 @@ fn test_special_file_different_filesystem() { /// Test cross-device move with permission denied error /// This test mimics the scenario from the GNU part-fail test where -/// a cross-device move fails due to permission errors when removing the target file +/// Cross-device move into a read-only destination directory must fail when +/// creating a new destination file there. (mv into an existing writable file +/// in a read-only dir now succeeds in place — matching GNU — because #10015 +/// removed the pre-copy unlink that previously forced directory-write +/// permission even when the target already existed.) #[test] #[cfg(target_os = "linux")] fn test_mv_cross_device_permission_denied() { - use std::fs::{set_permissions, write}; + use std::fs::set_permissions; use std::os::unix::fs::PermissionsExt; use tempfile::TempDir; use uutests::util::TestScenario; @@ -2614,15 +2616,12 @@ fn test_mv_cross_device_permission_denied() { let other_fs_tempdir = TempDir::new_in("/dev/shm/").expect("Unable to create temp directory in /dev/shm"); - let target_file_path = other_fs_tempdir.path().join("k"); - write(&target_file_path, "target content").expect("Unable to write target file"); - - // Remove write permissions from the directory to cause permission denied + // Remove write permissions from the directory so creating a new file inside fails. set_permissions(other_fs_tempdir.path(), PermissionsExt::from_mode(0o555)) .expect("Unable to set directory permissions"); - // Attempt to move file to the other filesystem - // This should fail with a permission denied error + let target_file_path = other_fs_tempdir.path().join("new_target"); + let result = scene .ucmd() .arg("-f") @@ -2630,8 +2629,6 @@ fn test_mv_cross_device_permission_denied() { .arg(target_file_path.to_str().unwrap()) .fails(); - // Check that it contains permission denied and references the file - // The exact format may vary but should contain these key elements let stderr = result.stderr_str(); assert!(stderr.contains("Permission denied") || stderr.contains("permission denied")); @@ -2639,6 +2636,49 @@ fn test_mv_cross_device_permission_denied() { .expect("Unable to restore directory permissions"); } +/// Regression for #10015: cross-device mv into a read-only destination +/// directory must succeed when overwriting an existing writable file — +/// the truncate happens in place via O_TRUNC, no directory-write +/// permission is needed. Previously uutils unlinked first, which forced +/// directory write permission and made this scenario fail. +#[test] +#[cfg(target_os = "linux")] +fn test_mv_cross_device_truncate_in_readonly_dir() { + use std::fs::{set_permissions, write}; + use std::os::unix::fs::PermissionsExt; + use tempfile::TempDir; + use uutests::util::TestScenario; + + let scene = TestScenario::new(util_name!()); + let at = &scene.fixtures; + + at.write("k", "source content"); + + let other_fs_tempdir = + TempDir::new_in("/dev/shm/").expect("Unable to create temp directory in /dev/shm"); + + let target_file_path = other_fs_tempdir.path().join("existing_target"); + write(&target_file_path, "old content").expect("Unable to write target file"); + + set_permissions(other_fs_tempdir.path(), PermissionsExt::from_mode(0o555)) + .expect("Unable to set directory permissions"); + + scene + .ucmd() + .arg("-f") + .arg("k") + .arg(target_file_path.to_str().unwrap()) + .succeeds() + .no_stderr(); + + set_permissions(other_fs_tempdir.path(), PermissionsExt::from_mode(0o755)) + .expect("Unable to restore directory permissions"); + + let copied = std::fs::read_to_string(&target_file_path).expect("read dst"); + assert_eq!(copied, "source content"); + assert!(!at.file_exists("k"), "source should be removed after mv"); +} + #[test] #[cfg(feature = "selinux")] fn test_mv_selinux_context() { @@ -2865,6 +2905,93 @@ fn test_mv_xattr_enotsup_silent() { } } +/// Regression for #10010: a cross-device mv of a symlink onto an existing +/// destination file must replace the destination, matching GNU. Previously +/// uutils failed with "File exists" because the symlink creation saw the +/// existing destination. +#[test] +#[cfg(target_os = "linux")] +fn test_mv_cross_device_symlink_onto_existing() { + use std::fs; + use std::os::unix::fs::symlink; + use tempfile::TempDir; + + let scene = TestScenario::new(util_name!()); + let at = &scene.fixtures; + + symlink("/etc/passwd", at.plus("src_link")).expect("Failed to create source symlink"); + + let other_fs_tempdir = + TempDir::new_in("/dev/shm/").expect("Unable to create temp directory in /dev/shm"); + let dst_path = other_fs_tempdir.path().join("dst_exists"); + fs::write(&dst_path, "placeholder").expect("Failed to write placeholder dst"); + + scene + .ucmd() + .arg(at.plus_as_string("src_link")) + .arg(dst_path.to_str().unwrap()) + .succeeds() + .no_stderr(); + + assert!( + dst_path.is_symlink(), + "dst_exists should now be a symlink after the cross-device move" + ); + assert_eq!( + fs::read_link(&dst_path).expect("read_link failed"), + Path::new("/etc/passwd"), + ); + assert!( + !at.symlink_exists("src_link"), + "source symlink should be gone" + ); +} + +/// Cross-device mv of a symlink onto an existing *directory* destination +/// (with -T so mv doesn't treat dst as a parent directory) must fail and +/// must not destroy the directory. Companion to #10010 — the atomic +/// temp-and-rename path can replace files/symlinks but `rename(2)` +/// refuses to replace a non-empty directory with a symlink, and even an +/// empty directory must not be silently destroyed. +#[test] +#[cfg(target_os = "linux")] +fn test_mv_cross_device_symlink_onto_existing_dir() { + use std::fs; + use std::os::unix::fs::symlink; + use tempfile::TempDir; + + let scene = TestScenario::new(util_name!()); + let at = &scene.fixtures; + + symlink("/etc/passwd", at.plus("src_link")).expect("Failed to create source symlink"); + + let other_fs_tempdir = + TempDir::new_in("/dev/shm/").expect("Unable to create temp directory in /dev/shm"); + let dst_dir = other_fs_tempdir.path().join("dst_dir"); + fs::create_dir(&dst_dir).expect("Failed to create destination directory"); + fs::write(dst_dir.join("guard"), "preserved").expect("Failed to write guard file"); + + scene + .ucmd() + .arg("-T") + .arg(at.plus_as_string("src_link")) + .arg(dst_dir.to_str().unwrap()) + .fails(); + + assert!( + dst_dir.is_dir(), + "destination directory must still exist after failed mv" + ); + assert!( + dst_dir.join("guard").is_file(), + "destination directory contents must be untouched" + ); + assert!( + at.symlink_exists("src_link"), + "source symlink must not be removed when mv fails" + ); +} + /// Test that symlinks inside directories are preserved during cross-device moves /// (not expanded into full copies of their targets) #[test] diff --git a/util/check-safe-traversal.sh b/util/check-safe-traversal.sh index 0462f6b9ef8..50df3305d49 100755 --- a/util/check-safe-traversal.sh +++ b/util/check-safe-traversal.sh @@ -219,6 +219,59 @@ if echo "$AVAILABLE_UTILS" | grep -q "mv"; then check_utility "mv" "openat,renameat,newfstatat,rename" "openat" "test_mv_src test_mv_dst" "move_directory" fi +# Test mv cross-device (EXDEV fallback): xattr preservation must use the +# fd-based *xattr variants so the source and destination inodes are +# pinned for the whole copy + xattr window. Using path-based listxattr / +# getxattr / setxattr reopens the TOCTOU described in issue #10014. +# Skip if /dev/shm is unavailable or doesn't support user xattrs. +if echo "$AVAILABLE_UTILS" | grep -q "mv" && [ -d /dev/shm ]; then + # Confirm $TEMP_DIR and /dev/shm are on different filesystems; otherwise + # mv won't take the EXDEV fallback and the strace check is a no-op. + temp_fs_id=$(stat -f -c %i "$TEMP_DIR" 2>/dev/null || echo "") + shm_fs_id=$(stat -f -c %i /dev/shm 2>/dev/null || echo "") + if [ -z "$temp_fs_id" ] || [ -z "$shm_fs_id" ] || [ "$temp_fs_id" = "$shm_fs_id" ]; then + echo "WARN: mv cross-device xattr check: TMPDIR and /dev/shm are on the same filesystem; skipped" + else + shm_probe=$(mktemp -p /dev/shm mv_xattr_probe.XXXXXX) + if setfattr -n user.probe -v ok "$shm_probe" 2>/dev/null; then + rm -f "$shm_probe" + cross_src=$(mktemp -p "$TEMP_DIR" cross_src.XXXXXX) + cross_dst=$(mktemp -u -p /dev/shm cross_dst.XXXXXX) + echo "cross-device payload" > "$cross_src" + if setfattr -n user.tag -v pinned "$cross_src" 2>/dev/null; then + if [ "$USE_MULTICALL" -eq 1 ]; then + mv_cmd="$COREUTILS_BIN mv" + else + mv_cmd="$PROJECT_ROOT/target/${PROFILE}/mv" + fi + strace -f -e trace='%file,fgetxattr,fsetxattr,flistxattr,getxattr,setxattr,listxattr' \ + -o strace_mv_xattr.log \ + $mv_cmd "$cross_src" "$cross_dst" 2>/dev/null || true + + # Path-based xattr calls against the source or destination paths + # indicate the non-atomic TOCTOU pattern we're avoiding. + cross_src_base=$(basename "$cross_src") + cross_dst_base=$(basename "$cross_dst") + if grep -qE "(listxattr|getxattr|setxattr)\([^,]*($cross_src_base|$cross_dst_base)" strace_mv_xattr.log; then + cat strace_mv_xattr.log + fail_immediately "mv cross-device must use fd-based xattr ops (issue #10014)" + fi + if grep -qE 'flistxattr|fgetxattr|fsetxattr' strace_mv_xattr.log; then + echo "OK: mv cross-device uses fd-based xattr syscalls" + else + echo "WARN: mv cross-device xattr check: no xattr syscalls observed (xattr may have been filtered - check filesystem support)" + fi + rm -f "$cross_dst" + else + echo "WARN: mv cross-device xattr check: TMPDIR does not support user xattrs; skipped" + fi + else + rm -f "$shm_probe" + echo "WARN: mv cross-device xattr check: /dev/shm does not support user xattrs; skipped" + fi + fi +fi + echo "" echo "✓ Basic safe traversal verification completed" echo ""