From 0b07d09780e7e514c6b05f1da5efb137774f1ff3 Mon Sep 17 00:00:00 2001 From: NdekoCode Date: Wed, 7 Oct 2026 23:12:37 +0200 Subject: [PATCH 1/3] cp: name both paths when a directory cannot overwrite a non-directory "cannot overwrite non-directory with directory" named neither path. A subdirectory whose destination already existed as a regular file was not caught at all: cp went on into it and failed later on the first file inside, with "cannot create regular file 'd/sub/x': Not a directory", and stopped there. A dangling symlink as the target of `cp -r` was followed, and its target created as a directory. Say which destination and which directory, as GNU does. Refuse a target that is not a directory before walking the tree, and for a subdirectory that would land on a file or a symlink, report it, leave the subtree alone and go on with the next entry, as GNU does. --- src/uu/cp/locales/en-US.ftl | 2 +- src/uu/cp/locales/fr-FR.ftl | 2 +- src/uu/cp/src/copydir.rs | 87 +++++++++++++++++++------------------ src/uu/cp/src/cp.rs | 4 ++ tests/by-util/test_cp.rs | 38 +++++++++++++++- 5 files changed, 87 insertions(+), 46 deletions(-) diff --git a/src/uu/cp/locales/en-US.ftl b/src/uu/cp/locales/en-US.ftl index 49fec90cc9c..5dff7757afe 100644 --- a/src/uu/cp/locales/en-US.ftl +++ b/src/uu/cp/locales/en-US.ftl @@ -68,7 +68,7 @@ cp-error-will-not-copy-through-symlink = will not copy { $source } through just- cp-error-will-not-overwrite-just-created = will not overwrite just-created { $dest } with { $source } cp-error-target-not-directory = target: { $target } is not a directory cp-error-cannot-overwrite-directory-with-non-directory = cannot overwrite directory { $dir } with non-directory -cp-error-cannot-overwrite-non-directory-with-directory = cannot overwrite non-directory with directory +cp-error-cannot-overwrite-non-directory-with-directory = cannot overwrite non-directory { $dest } with directory { $source } cp-error-with-parents-dest-must-be-dir = with --parents, the destination must be a directory cp-error-not-replacing = not replacing { $file } cp-error-failed-get-current-dir = failed to get current directory { $error } diff --git a/src/uu/cp/locales/fr-FR.ftl b/src/uu/cp/locales/fr-FR.ftl index 43911f8280c..133ff46b9f5 100644 --- a/src/uu/cp/locales/fr-FR.ftl +++ b/src/uu/cp/locales/fr-FR.ftl @@ -68,7 +68,7 @@ cp-error-will-not-copy-through-symlink = ne copiera pas { $source } à travers l cp-error-will-not-overwrite-just-created = n'écrasera pas le fichier tout juste créé { $dest } avec { $source } cp-error-target-not-directory = cible : { $target } n'est pas un répertoire cp-error-cannot-overwrite-directory-with-non-directory = impossible d'écraser le répertoire { $dir } avec un non-répertoire -cp-error-cannot-overwrite-non-directory-with-directory = impossible d'écraser un non-répertoire avec un répertoire +cp-error-cannot-overwrite-non-directory-with-directory = impossible d'écraser le non-répertoire { $dest } avec le répertoire { $source } cp-error-with-parents-dest-must-be-dir = avec --parents, la destination doit être un répertoire cp-error-not-replacing = ne remplace pas { $file } cp-error-failed-get-current-dir = échec de l'obtention du répertoire actuel { $error } diff --git a/src/uu/cp/src/copydir.rs b/src/uu/cp/src/copydir.rs index 37de10c0dbb..188ab0ef3f0 100644 --- a/src/uu/cp/src/copydir.rs +++ b/src/uu/cp/src/copydir.rs @@ -113,9 +113,6 @@ struct Context<'a> { /// The target path to which the directory will be copied. target: &'a Path, - /// Whether the target is an existing file. Cached to avoid repeated `stat` calls. - target_is_file: bool, - /// The source path from which the directory will be copied. root: &'a Path, } @@ -137,7 +134,6 @@ impl<'a> Context<'a> { env::current_dir()? }; let root_path = current_dir.join(root); - let target_is_file = target.is_file(); let root_parent = if target.exists() && !ends_with_curdir(root) { root_path.parent().map(ToOwned::to_owned) } else { @@ -147,7 +143,6 @@ impl<'a> Context<'a> { current_dir, root_parent, target, - target_is_file, root, }) } @@ -172,19 +167,16 @@ impl<'a> Context<'a> { /// source_absolute: "/tmp/a".into(), /// source_relative: "a".into(), /// local_to_target: "d/a".into(), -/// target_is_file: false, /// } /// Entry { /// source_absolute: "/tmp/a/b".into(), /// source_relative: "a/b".into(), /// local_to_target: "d/a/b".into(), -/// target_is_file: false, /// } /// Entry { /// source_absolute: "/tmp/a/b/c".into(), /// source_relative: "a/b/c".into(), /// local_to_target: "d/a/b/c".into(), -/// target_is_file: false, /// } /// ]; /// ``` @@ -197,9 +189,6 @@ struct Entry { /// The path to the destination, relative to the target. local_to_target: PathBuf, - - /// Whether the destination is a file. - target_is_file: bool, } impl Entry { @@ -236,12 +225,10 @@ impl Entry { } let local_to_target = context.target.join(descendant); - let target_is_file = context.target_is_file; Ok(Self { source_absolute, source_relative, local_to_target, - target_is_file, }) } } @@ -275,34 +262,27 @@ fn copy_direntry( entry_is_dir_no_follow }; - // `exists()` resolves symlinks, so a destination entry that is itself a - // symlink to a directory would look like an already-existing directory and - // be descended into -- writing the source subtree through the link and out - // of the destination tree. GNU refuses this ("cannot overwrite - // non-directory ... with directory"), so treat a symlink at the destination - // as the non-directory it is. - let dest_is_symlink = entry.local_to_target.is_symlink(); - - // If the source is a directory and the destination does not - // exist, ... - if source_is_dir && (dest_is_symlink || !entry.local_to_target.exists()) { - return if entry.target_is_file || dest_is_symlink { - Err(translate!("cp-error-cannot-overwrite-non-directory-with-directory").into()) - } else { - build_dir( - &entry.local_to_target, - false, - options, - Some(&entry.source_absolute), - )?; + if source_is_dir { + // `exists()` resolves symlinks, so a destination entry that is itself + // a symlink to a directory would look like an already-existing + // directory and be descended into -- writing the source subtree + // through the link and out of the destination tree. GNU refuses this + // ("cannot overwrite non-directory ... with directory"), so treat a + // symlink at the destination as the non-directory it is. + let dest = &entry.local_to_target; + if dest.is_symlink() || (dest.exists() && !dest.is_dir()) { + return Err(CpError::CannotOverwriteNonDirectory( + dest.clone(), + entry.source_relative.clone(), + )); + } + if !dest.exists() { + build_dir(dest, false, options, Some(&entry.source_absolute))?; if options.verbose { - println!( - "{}", - context_for(&entry.source_relative, &entry.local_to_target) - ); + println!("{}", context_for(&entry.source_relative, dest)); } - Ok(true) - }; + return Ok(true); + } } // If the source is not a directory, then we need to copy the file. @@ -405,6 +385,16 @@ pub(crate) fn copy_directory( .into()); } + // A directory cannot take the place of a file, nor of a symlink that + // does not lead to a directory: a dangling one would be followed and + // its target created. + if target.symlink_metadata().is_ok() && !target.is_dir() { + return Err(CpError::CannotOverwriteNonDirectory( + target.to_path_buf(), + root.to_path_buf(), + )); + } + // If in `--parents` mode, create all the necessary ancestor directories. // // For example, if the command is `cp --parents a/b/c d`, that @@ -469,10 +459,11 @@ pub(crate) fn copy_directory( let mut dirs_needing_permissions: Vec = Vec::new(); // Traverse the contents of the directory, copying each one. - for direntry_result in WalkDir::new(root) + let mut walker = WalkDir::new(root) .same_file_system(options.one_file_system) .follow_links(options.dereference) - { + .into_iter(); + while let Some(direntry_result) = walker.next() { match direntry_result { Ok(direntry) => { let direntry_type = direntry.file_type(); @@ -487,7 +478,7 @@ pub(crate) fn copy_directory( }; let entry = Entry::new(&context, direntry_path, options.no_target_dir)?; - let created = copy_direntry( + let created = match copy_direntry( progress_bar, &entry, entry_is_symlink, @@ -498,7 +489,17 @@ pub(crate) fn copy_directory( copied_destinations, copied_files, created_parent_dirs, - )?; + ) { + Ok(created) => created, + // The subtree has nowhere to go: report it, leave what is + // there alone, and go on with the next entry, as GNU does. + Err(err @ CpError::CannotOverwriteNonDirectory(..)) => { + show!(err); + walker.skip_current_dir(); + continue; + } + Err(err) => return Err(err), + }; // We omit certain permissions when creating directories // to prevent other users from accessing them before they're done. diff --git a/src/uu/cp/src/cp.rs b/src/uu/cp/src/cp.rs index 6f11e3d8152..0a69032c6bd 100644 --- a/src/uu/cp/src/cp.rs +++ b/src/uu/cp/src/cp.rs @@ -117,6 +117,10 @@ pub enum CpError { #[error("{}", translate!("cp-error-not-a-directory", "path" => .0.quote()))] NotADirectory(PathBuf), + + /// A directory would have to replace something that is not one. + #[error("{}", translate!("cp-error-cannot-overwrite-non-directory-with-directory", "dest" => .0.quote(), "source" => .1.quote()))] + CannotOverwriteNonDirectory(PathBuf, PathBuf), } // Manual impl for &str diff --git a/tests/by-util/test_cp.rs b/tests/by-util/test_cp.rs index c805f643ad6..b9b815e37e3 100644 --- a/tests/by-util/test_cp.rs +++ b/tests/by-util/test_cp.rs @@ -4544,7 +4544,43 @@ fn test_cp_dir_vs_file() { .arg(TEST_COPY_FROM_FOLDER) .arg(TEST_EXISTING_FILE) .fails() - .stderr_only("cp: cannot overwrite non-directory with directory\n"); + .stderr_only(format!( + "cp: cannot overwrite non-directory '{TEST_EXISTING_FILE}' with directory '{TEST_COPY_FROM_FOLDER}'\n" + )); +} + +#[test] +#[cfg(unix)] +fn test_cp_dir_vs_dangling_symlink() { + // A symlink that leads nowhere is not a directory to copy into: following + // it would create its target. + let (at, mut ucmd) = at_and_ucmd!(); + at.mkdir("src"); + at.touch("src/file"); + at.symlink_file("nowhere", "dangling"); + + ucmd.args(&["-r", "src", "dangling"]) + .fails() + .stderr_only("cp: cannot overwrite non-directory 'dangling' with directory 'src'\n"); + assert!(!at.dir_exists("nowhere")); +} + +#[test] +fn test_cp_recursive_dest_subdir_is_file() { + // A directory whose destination already exists as a file is reported + // and left alone; the other entries are still copied. + let (at, mut ucmd) = at_and_ucmd!(); + at.mkdir_all("src/sub"); + at.write("src/sub/x", "x"); + at.write("src/other", "other"); + at.mkdir_all("dst/src"); + at.write("dst/src/sub", "kept"); + + ucmd.args(&["-r", "src", "dst"]) + .fails() + .stderr_only("cp: cannot overwrite non-directory 'dst/src/sub' with directory 'src/sub'\n"); + assert_eq!(at.read("dst/src/sub"), "kept"); + assert_eq!(at.read("dst/src/other"), "other"); } #[test] From a4226c515d1fb5ed71083ebee1e5d9a363bb8e4d Mon Sep 17 00:00:00 2001 From: NdekoCode Date: Fri, 9 Oct 2026 23:44:24 +0200 Subject: [PATCH 2/3] cp: do not panic when the -v line for a directory cannot be written Go through print_verbose_output like the file lines do, so that a closed or full stdout gives "write error" instead of a panic in println!. --- src/uu/cp/src/copydir.rs | 6 +++--- src/uu/cp/src/cp.rs | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/uu/cp/src/copydir.rs b/src/uu/cp/src/copydir.rs index 188ab0ef3f0..aea33c93590 100644 --- a/src/uu/cp/src/copydir.rs +++ b/src/uu/cp/src/copydir.rs @@ -32,8 +32,8 @@ use walkdir::{DirEntry, WalkDir}; #[cfg(all(feature = "selinux", any(target_os = "linux", target_os = "android")))] use crate::set_selinux_context; use crate::{ - CopyMode, CopyResult, CpError, Options, aligned_ancestors, context_for, copy_attributes, - copy_file, create_parent_dirs, set_parent_dirs_attributes, + CopyMode, CopyResult, CpError, Options, aligned_ancestors, copy_attributes, copy_file, + create_parent_dirs, print_verbose_output, set_parent_dirs_attributes, }; /// Represents a directory that needs permission fixup after copying its contents. @@ -279,7 +279,7 @@ fn copy_direntry( if !dest.exists() { build_dir(dest, false, options, Some(&entry.source_absolute))?; if options.verbose { - println!("{}", context_for(&entry.source_relative, dest)); + print_verbose_output(false, progress_bar, &entry.source_relative, dest)?; } return Ok(true); } diff --git a/src/uu/cp/src/cp.rs b/src/uu/cp/src/cp.rs index 0a69032c6bd..9075cb95fc3 100644 --- a/src/uu/cp/src/cp.rs +++ b/src/uu/cp/src/cp.rs @@ -2469,7 +2469,7 @@ pub(crate) fn set_parent_dirs_attributes( result } -fn print_verbose_output( +pub(crate) fn print_verbose_output( parents: bool, progress_bar: Option<&ProgressBar>, source: &Path, From 090172574a790adf90a05527ccfa07bfb308a1b8 Mon Sep 17 00:00:00 2001 From: NdekoCode Date: Fri, 9 Oct 2026 23:44:24 +0200 Subject: [PATCH 3/3] tests: cp: expect the native separator in the overwrite message The walk joins paths with a backslash on Windows. --- tests/by-util/test_cp.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/tests/by-util/test_cp.rs b/tests/by-util/test_cp.rs index b9b815e37e3..d3474eeb413 100644 --- a/tests/by-util/test_cp.rs +++ b/tests/by-util/test_cp.rs @@ -4576,9 +4576,13 @@ fn test_cp_recursive_dest_subdir_is_file() { at.mkdir_all("dst/src"); at.write("dst/src/sub", "kept"); + let dest = path_concat!("dst", "src", "sub"); + let source = path_concat!("src", "sub"); ucmd.args(&["-r", "src", "dst"]) .fails() - .stderr_only("cp: cannot overwrite non-directory 'dst/src/sub' with directory 'src/sub'\n"); + .stderr_only(format!( + "cp: cannot overwrite non-directory '{dest}' with directory '{source}'\n" + )); assert_eq!(at.read("dst/src/sub"), "kept"); assert_eq!(at.read("dst/src/other"), "other"); }