From b3cdb9d3ad75ce3773bb032ecd24e9abdae3a7d9 Mon Sep 17 00:00:00 2001 From: Justin Tracey Date: Tue, 22 Apr 2025 17:49:28 -0400 Subject: [PATCH] mv: use RENAME_NOREPLACE when we can --- Cargo.lock | 1 + src/uu/mv/Cargo.toml | 1 + src/uu/mv/src/mv.rs | 102 ++++++++++++++++++++++++++++--------------- 3 files changed, 69 insertions(+), 35 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 1234fd1647b..6535d0acc5b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3034,6 +3034,7 @@ dependencies = [ "fs_extra", "indicatif", "libc", + "nix", "thiserror 2.0.12", "uucore", "windows-sys 0.59.0", diff --git a/src/uu/mv/Cargo.toml b/src/uu/mv/Cargo.toml index 1cb30bf1473..dff646db6a4 100644 --- a/src/uu/mv/Cargo.toml +++ b/src/uu/mv/Cargo.toml @@ -22,6 +22,7 @@ clap = { workspace = true } fs_extra = { workspace = true } indicatif = { workspace = true } libc = { workspace = true } +nix = { workspace = true } thiserror = { workspace = true } uucore = { workspace = true, features = [ "backup-control", diff --git a/src/uu/mv/src/mv.rs b/src/uu/mv/src/mv.rs index acd21aa7e63..9fa47e8f413 100644 --- a/src/uu/mv/src/mv.rs +++ b/src/uu/mv/src/mv.rs @@ -3,13 +3,15 @@ // 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 +// spell-checker:ignore NOREPLACE TOCTOU canonicalized nushell renameat sourcepath targetpath mod error; use clap::builder::ValueParser; use clap::{Arg, ArgAction, ArgMatches, Command, error::ErrorKind}; use indicatif::{MultiProgress, ProgressBar, ProgressStyle}; +#[cfg(all(target_os = "linux", target_env = "gnu"))] +use nix::{errno::Errno, fcntl}; use std::collections::HashSet; use std::env; @@ -596,23 +598,42 @@ fn rename( ) -> io::Result<()> { let mut backup_path = None; - if to.exists() { - if opts.update == UpdateMode::None { - if opts.debug { - println!("skipped {}", to.quote()); - } - return Ok(()); - } - - if (opts.update == UpdateMode::IfOlder) - && fs::metadata(from)?.modified()? <= fs::metadata(to)?.modified()? - { - return Ok(()); + let already_exists = if opts.overwrite == OverwriteMode::Force + && opts.update == UpdateMode::All + && opts.backup == BackupMode::None + { + // fast path, no need to check for overwrites + rename_with_fallback(from, to, multi_progress)?; + false + } else { + // try to move without overwriting + match rename_no_replace(from, to, multi_progress) { + Ok(()) => false, + Err(e) if e.kind() == io::ErrorKind::AlreadyExists => true, + Err(e) => Err(e)?, } + }; - if opts.update == UpdateMode::NoneFail { - let err_msg = format!("not replacing {}", to.quote()); - return Err(io::Error::other(err_msg)); + if already_exists { + match opts.update { + UpdateMode::None => { + if opts.debug { + println!("skipped {}", to.quote()); + } + return Ok(()); + } + UpdateMode::IfOlder => { + if fs::metadata(from)?.modified()? <= fs::metadata(to)?.modified()? { + return Ok(()); + } + // There's a TOCTOU here, if `to` gets replaced with a newer version. + // I don't think there's a way to fix it. + } + UpdateMode::NoneFail => { + let err_msg = format!("not replacing {}", to.quote()); + return Err(io::Error::other(err_msg)); + } + UpdateMode::All => {} } match opts.overwrite { @@ -634,21 +655,17 @@ fn rename( if let Some(ref backup_path) = backup_path { rename_with_fallback(to, backup_path, multi_progress)?; } - } - // "to" may no longer exist if it was backed up - if to.exists() && to.is_dir() { - // normalize behavior between *nix and windows - if from.is_dir() { - if is_empty_dir(to) { - fs::remove_dir(to)?; - } else { - return Err(io::Error::other("Directory not empty")); - } + #[cfg(windows)] + // older versions of windows don't handle this automatically, + // see std::fs::rename docs + if to.is_dir() && from.is_dir() { + // this checks if dir is empty for us, and returns the appropriate error if not + fs::remove_dir(to)?; } - } - rename_with_fallback(from, to, multi_progress)?; + rename_with_fallback(from, to, multi_progress)?; + } if opts.verbose { let message = match backup_path { @@ -681,6 +698,28 @@ fn is_fifo(_filetype: fs::FileType) -> bool { false } +/// Attempts to use atomic renaming that avoids overwriting files, if available from the OS, +/// otherwise checks if `to` exists and calls `rename_with_fallback` (allowing races). +/// In either case, returns `io::ErrorKind::AlreadyExists` kind if `to` exists. +fn rename_no_replace( + from: &Path, + to: &Path, + multi_progress: Option<&MultiProgress>, +) -> io::Result<()> { + #[cfg(all(target_os = "linux", target_env = "gnu"))] + match fcntl::renameat2(None, from, None, to, fcntl::RenameFlags::RENAME_NOREPLACE) { + Ok(()) => return Ok(()), + Err(Errno::EINVAL) => {} // could be unsupported on platform, continue + Err(e) => Err(e)?, + } + + if to.exists() { + Err(io::Error::from(io::ErrorKind::AlreadyExists)) + } else { + rename_with_fallback(from, to, multi_progress) + } +} + /// A wrapper around `fs::rename`, so that if it fails, we try falling back on /// copying and removing. fn rename_with_fallback( @@ -855,13 +894,6 @@ fn rename_file_fallback(from: &Path, to: &Path) -> io::Result<()> { Ok(()) } -fn is_empty_dir(path: &Path) -> bool { - match fs::read_dir(path) { - Ok(contents) => contents.peekable().peek().is_none(), - Err(_e) => false, - } -} - /// Checks if a file can be deleted by attempting to open it with delete permissions. #[cfg(windows)] fn can_delete_file(path: &Path) -> Result {