From e853164e3a133b134f430a4c10a9e2c9bfa0c4e2 Mon Sep 17 00:00:00 2001 From: Nikola Lukovic Date: Fri, 19 Dec 2025 11:30:40 +0100 Subject: [PATCH 1/5] cp: Preserve mode for directories when copying by default. --- src/uu/cp/src/cp.rs | 9 ++++- tests/by-util/test_cp.rs | 67 +++++++++++++++++++++++++++++++++++ tests/uutests/src/lib/util.rs | 12 +++++++ 3 files changed, 87 insertions(+), 1 deletion(-) diff --git a/src/uu/cp/src/cp.rs b/src/uu/cp/src/cp.rs index 3048f38b711..d8c5fe0aa3e 100644 --- a/src/uu/cp/src/cp.rs +++ b/src/uu/cp/src/cp.rs @@ -1690,6 +1690,13 @@ pub(crate) fn copy_attributes( let source_metadata = fs::symlink_metadata(source).map_err(|e| CpError::IoErrContext(e, context.to_owned()))?; + // if --no-preserve wasn't explicitly passed and we're copying a directory by default we should preserve mode + let mode = if dest.is_dir() && attributes.mode == (Preserve::No { explicit: false }) { + Preserve::Yes { required: false } + } else { + attributes.mode + }; + // Ownership must be changed first to avoid interfering with mode change. #[cfg(unix)] handle_preserve(&attributes.ownership, || -> CopyResult<()> { @@ -1727,7 +1734,7 @@ pub(crate) fn copy_attributes( Ok(()) })?; - handle_preserve(&attributes.mode, || -> CopyResult<()> { + handle_preserve(&mode, || -> CopyResult<()> { // The `chmod()` system call that underlies the // `fs::set_permissions()` call is unable to change the // permissions of a symbolic link. In that case, we just diff --git a/tests/by-util/test_cp.rs b/tests/by-util/test_cp.rs index 5f4a44c4aff..744d697c8b2 100644 --- a/tests/by-util/test_cp.rs +++ b/tests/by-util/test_cp.rs @@ -7444,3 +7444,70 @@ fn test_cp_archive_deref_flag_ordering() { assert_eq!(at.is_symlink(&dest), expect_symlink, "failed for {flags}"); } } + +#[cfg(not(target_os = "windows"))] +fn test_cp_preserve_directory_permissions_by_default() { + let scene = TestScenario::new(util_name!()); + let at = &scene.fixtures; + + let dir = "a/b/c/d"; + let file = "foo.txt"; + + at.mkdir_all(dir); + + let file_path = format!("{dir}/{file}"); + + at.touch(file_path); + + scene.cmd("chmod").arg("-R").arg("555").arg("a").succeeds(); + scene.cmd("cp").arg("-r").arg("a").arg("b").succeeds(); + + scene.ucmd().arg("-r").arg("a").arg("c").succeeds(); + + assert_eq!(at.get_mode("b"), 0o40555); + assert_eq!(at.get_mode("b/b"), 0o40555); + assert_eq!(at.get_mode("b/b/c"), 0o40555); + assert_eq!(at.get_mode("b/b/c/d"), 0o40555); + + assert_eq!(at.get_mode("c"), 0o40555); + assert_eq!(at.get_mode("c/b"), 0o40555); + assert_eq!(at.get_mode("c/b/c"), 0o40555); + assert_eq!(at.get_mode("c/b/c/d"), 0o40555); +} + +#[test] +#[cfg(not(target_os = "windows"))] +fn test_cp_no_preserve_directory_permissions() { + let scene = TestScenario::new(util_name!()); + let at = &scene.fixtures; + + let dir = "a/b/c/d"; + let file = "foo.txt"; + + at.mkdir_all(dir); + + let file_path = format!("{dir}/{file}"); + + at.touch(file_path); + + scene.cmd("chmod").arg("-R").arg("555").arg("a").succeeds(); + scene.cmd("cp").arg("-r").arg("a").arg("b").succeeds(); + + scene + .ucmd() + .arg("-r") + .arg("--no-preserve=mode") + .arg("a") + .arg("c") + .succeeds(); + + assert_eq!(at.get_mode("b"), 0o40555); + assert_eq!(at.get_mode("b/b"), 0o40555); + assert_eq!(at.get_mode("b/b/c"), 0o40555); + assert_eq!(at.get_mode("b/b/c/d"), 0o40555); + + assert_eq!(at.get_mode("c"), 0o40755); + assert_eq!(at.get_mode("c/b"), 0o40755); + assert_eq!(at.get_mode("c/b/c"), 0o40755); + assert_eq!(at.get_mode("c/b/c/d"), 0o40755); +} diff --git a/tests/uutests/src/lib/util.rs b/tests/uutests/src/lib/util.rs index 5c5ed3ef401..7f1b0b5a144 100644 --- a/tests/uutests/src/lib/util.rs +++ b/tests/uutests/src/lib/util.rs @@ -1324,6 +1324,18 @@ impl AtPath { perms.set_mode(mode); std::fs::set_permissions(&path, perms).unwrap(); } + + /// Get the permissions of the specified file. + /// + /// # Panics + /// + /// This function panics if there is an error loading the metadata + #[cfg(not(windows))] + pub fn get_mode(&self, filename: &str) -> u32 { + let path = self.plus(filename); + let perms = std::fs::metadata(&path).unwrap().permissions(); + perms.mode() + } } /// An environment for running a single uutils test case, serves three functions: From 02d81eb41af7b781018e486b03d19f75bc7b0f38 Mon Sep 17 00:00:00 2001 From: Nikola Lukovic Date: Sun, 28 Dec 2025 21:34:14 +0100 Subject: [PATCH 2/5] cp: Move get_mode util func to test_cp --- tests/by-util/test_cp.rs | 43 ++++++++++++++++++++++------------- tests/uutests/src/lib/util.rs | 12 ---------- 2 files changed, 27 insertions(+), 28 deletions(-) diff --git a/tests/by-util/test_cp.rs b/tests/by-util/test_cp.rs index 744d697c8b2..ccbe0bc682e 100644 --- a/tests/by-util/test_cp.rs +++ b/tests/by-util/test_cp.rs @@ -88,6 +88,17 @@ macro_rules! assert_metadata_eq { }}; } +/// Get the permissions of the specified file. +/// +/// # Panics +/// +/// This function panics if there is an error loading the metadata +#[cfg(not(windows))] +pub fn get_mode(filename: PathBuf) -> u32 { + let perms = std::fs::metadata(filename).unwrap().permissions(); + perms.mode() +} + #[test] fn test_cp_cp() { let (at, mut ucmd) = at_and_ucmd!(); @@ -7464,15 +7475,15 @@ fn test_cp_preserve_directory_permissions_by_default() { scene.ucmd().arg("-r").arg("a").arg("c").succeeds(); - assert_eq!(at.get_mode("b"), 0o40555); - assert_eq!(at.get_mode("b/b"), 0o40555); - assert_eq!(at.get_mode("b/b/c"), 0o40555); - assert_eq!(at.get_mode("b/b/c/d"), 0o40555); + assert_eq!(get_mode(at.plus("b")), 0o40555); + assert_eq!(get_mode(at.plus("b/b")), 0o40555); + assert_eq!(get_mode(at.plus("b/b/c")), 0o40555); + assert_eq!(get_mode(at.plus("b/b/c/d")), 0o40555); - assert_eq!(at.get_mode("c"), 0o40555); - assert_eq!(at.get_mode("c/b"), 0o40555); - assert_eq!(at.get_mode("c/b/c"), 0o40555); - assert_eq!(at.get_mode("c/b/c/d"), 0o40555); + assert_eq!(get_mode(at.plus("c")), 0o40555); + assert_eq!(get_mode(at.plus("c/b")), 0o40555); + assert_eq!(get_mode(at.plus("c/b/c")), 0o40555); + assert_eq!(get_mode(at.plus("c/b/c/d")), 0o40555); } #[test] @@ -7501,13 +7512,13 @@ fn test_cp_no_preserve_directory_permissions() { .arg("c") .succeeds(); - assert_eq!(at.get_mode("b"), 0o40555); - assert_eq!(at.get_mode("b/b"), 0o40555); - assert_eq!(at.get_mode("b/b/c"), 0o40555); - assert_eq!(at.get_mode("b/b/c/d"), 0o40555); + assert_eq!(get_mode(at.plus("b")), 0o40555); + assert_eq!(get_mode(at.plus("b/b")), 0o40555); + assert_eq!(get_mode(at.plus("b/b/c")), 0o40555); + assert_eq!(get_mode(at.plus("b/b/c/d")), 0o40555); - assert_eq!(at.get_mode("c"), 0o40755); - assert_eq!(at.get_mode("c/b"), 0o40755); - assert_eq!(at.get_mode("c/b/c"), 0o40755); - assert_eq!(at.get_mode("c/b/c/d"), 0o40755); + assert_eq!(get_mode(at.plus("c")), 0o40755); + assert_eq!(get_mode(at.plus("c/b")), 0o40755); + assert_eq!(get_mode(at.plus("c/b/c")), 0o40755); + assert_eq!(get_mode(at.plus("c/b/c/d")), 0o40755); } diff --git a/tests/uutests/src/lib/util.rs b/tests/uutests/src/lib/util.rs index 7f1b0b5a144..5c5ed3ef401 100644 --- a/tests/uutests/src/lib/util.rs +++ b/tests/uutests/src/lib/util.rs @@ -1324,18 +1324,6 @@ impl AtPath { perms.set_mode(mode); std::fs::set_permissions(&path, perms).unwrap(); } - - /// Get the permissions of the specified file. - /// - /// # Panics - /// - /// This function panics if there is an error loading the metadata - #[cfg(not(windows))] - pub fn get_mode(&self, filename: &str) -> u32 { - let path = self.plus(filename); - let perms = std::fs::metadata(&path).unwrap().permissions(); - perms.mode() - } } /// An environment for running a single uutils test case, serves three functions: From 9efc5bffd3fd73398efe9e463a4ccdca20d6a074 Mon Sep 17 00:00:00 2001 From: Nikola Lukovic Date: Mon, 29 Dec 2025 00:12:25 +0100 Subject: [PATCH 3/5] cp: Added additional gnu test which *has* to work in combination with needed changes for the added test --- src/uu/cp/src/copydir.rs | 6 ++--- src/uu/cp/src/cp.rs | 41 ++++++++++++++--------------- tests/by-util/test_cp.rs | 57 +++++++++++++++++++++++----------------- 3 files changed, 55 insertions(+), 49 deletions(-) diff --git a/src/uu/cp/src/copydir.rs b/src/uu/cp/src/copydir.rs index bbd3aba6297..2fe5b73e949 100644 --- a/src/uu/cp/src/copydir.rs +++ b/src/uu/cp/src/copydir.rs @@ -503,7 +503,7 @@ pub(crate) fn copy_directory( copy_attributes( &entry.source_absolute, &entry.local_to_target, - &options.attributes, + options, )?; } } @@ -520,7 +520,7 @@ pub(crate) fn copy_directory( // Fix permissions for all directories we created // This ensures that even sibling directories get their permissions fixed for (source_path, dest_path) in dirs_needing_permissions { - copy_attributes(&source_path, &dest_path, &options.attributes)?; + copy_attributes(&source_path, &dest_path, options)?; } // Also fix permissions for parent directories, @@ -529,7 +529,7 @@ pub(crate) fn copy_directory( let dest = target.join(root.file_name().unwrap()); for (x, y) in aligned_ancestors(root, dest.as_path()) { if let Ok(src) = canonicalize(x, MissingHandling::Normal, ResolveMode::Physical) { - copy_attributes(&src, y, &options.attributes)?; + copy_attributes(&src, y, options)?; } } } diff --git a/src/uu/cp/src/cp.rs b/src/uu/cp/src/cp.rs index d8c5fe0aa3e..52c8b25f022 100644 --- a/src/uu/cp/src/cp.rs +++ b/src/uu/cp/src/cp.rs @@ -1532,7 +1532,7 @@ fn copy_source( if options.parents { for (x, y) in aligned_ancestors(source, dest.as_path()) { if let Ok(src) = canonicalize(x, MissingHandling::Normal, ResolveMode::Physical) { - copy_attributes(&src, y, &options.attributes)?; + copy_attributes(&src, y, options)?; } } } @@ -1681,25 +1681,14 @@ fn copy_extended_attrs(source: &Path, dest: &Path) -> CopyResult<()> { } /// Copy the specified attributes from one path to another. -pub(crate) fn copy_attributes( - source: &Path, - dest: &Path, - attributes: &Attributes, -) -> CopyResult<()> { +pub(crate) fn copy_attributes(source: &Path, dest: &Path, options: &Options) -> CopyResult<()> { let context = &*format!("{} -> {}", source.quote(), dest.quote()); let source_metadata = fs::symlink_metadata(source).map_err(|e| CpError::IoErrContext(e, context.to_owned()))?; - // if --no-preserve wasn't explicitly passed and we're copying a directory by default we should preserve mode - let mode = if dest.is_dir() && attributes.mode == (Preserve::No { explicit: false }) { - Preserve::Yes { required: false } - } else { - attributes.mode - }; - // Ownership must be changed first to avoid interfering with mode change. #[cfg(unix)] - handle_preserve(&attributes.ownership, || -> CopyResult<()> { + handle_preserve(&options.attributes.ownership, || -> CopyResult<()> { use std::os::unix::prelude::MetadataExt; use uucore::perms::Verbosity; use uucore::perms::VerbosityLevel; @@ -1734,14 +1723,22 @@ pub(crate) fn copy_attributes( Ok(()) })?; - handle_preserve(&mode, || -> CopyResult<()> { + handle_preserve(&options.attributes.mode, || -> CopyResult<()> { // The `chmod()` system call that underlies the // `fs::set_permissions()` call is unable to change the // permissions of a symbolic link. In that case, we just // do nothing, since every symbolic link has the same // permissions. if !dest.is_symlink() { - fs::set_permissions(dest, source_metadata.permissions()) + // let dest_permissions = calculate_dest_permissions( + // fs::metadata(dest).ok().as_ref(), + // dest, + // &source_metadata, + // options, + // context, + // )?; + let dest_permissions = source_metadata.permissions(); + fs::set_permissions(dest, dest_permissions) .map_err(|e| CpError::IoErrContext(e, context.to_owned()))?; // FIXME: Implement this for windows as well #[cfg(feature = "feat_acl")] @@ -1753,7 +1750,7 @@ pub(crate) fn copy_attributes( Ok(()) })?; - handle_preserve(&attributes.timestamps, || -> CopyResult<()> { + handle_preserve(&options.attributes.timestamps, || -> CopyResult<()> { let atime = FileTime::from_last_access_time(&source_metadata); let mtime = FileTime::from_last_modification_time(&source_metadata); if dest.is_symlink() { @@ -1766,7 +1763,7 @@ pub(crate) fn copy_attributes( })?; #[cfg(feature = "selinux")] - handle_preserve(&attributes.context, || -> CopyResult<()> { + handle_preserve(&options.attributes.context, || -> CopyResult<()> { // Get the source context and apply it to the destination if let Ok(context) = selinux::SecurityContext::of_path(source, false, false) { if let Some(context) = context { @@ -1784,7 +1781,7 @@ pub(crate) fn copy_attributes( Ok(()) })?; - handle_preserve(&attributes.xattr, || -> CopyResult<()> { + handle_preserve(&options.attributes.xattr, || -> CopyResult<()> { #[cfg(all(unix, not(target_os = "android")))] { copy_extended_attrs(source, dest)?; @@ -2549,14 +2546,14 @@ fn copy_file( .ok() .filter(|p| p.exists()) .unwrap_or_else(|| source.to_path_buf()); - copy_attributes(&src_for_attrs, dest, &options.attributes)?; + copy_attributes(&src_for_attrs, dest, &options)?; } else if source_is_stream && !source.exists() { // Some stream files may not exist after we have copied it, // like anonymous pipes. Thus, we can't really copy its // attributes. However, this is already handled in the stream // copy function (see `copy_stream` under platform/linux.rs). } else { - copy_attributes(source, dest, &options.attributes)?; + copy_attributes(source, dest, options)?; } #[cfg(feature = "selinux")] @@ -2732,7 +2729,7 @@ fn copy_link( delete_path(dest, options)?; } symlink_file(&link, dest, symlinked_files)?; - copy_attributes(source, dest, &options.attributes) + copy_attributes(source, dest, options) } /// Generate an error message if `target` is not the correct `target_type` diff --git a/tests/by-util/test_cp.rs b/tests/by-util/test_cp.rs index ccbe0bc682e..9d84cb89a9b 100644 --- a/tests/by-util/test_cp.rs +++ b/tests/by-util/test_cp.rs @@ -7458,6 +7458,8 @@ fn test_cp_archive_deref_flag_ordering() { #[cfg(not(target_os = "windows"))] fn test_cp_preserve_directory_permissions_by_default() { + use std::io; + let scene = TestScenario::new(util_name!()); let at = &scene.fixtures; @@ -7473,7 +7475,13 @@ fn test_cp_preserve_directory_permissions_by_default() { scene.cmd("chmod").arg("-R").arg("555").arg("a").succeeds(); scene.cmd("cp").arg("-r").arg("a").arg("b").succeeds(); - scene.ucmd().arg("-r").arg("a").arg("c").succeeds(); + scene + .ucmd() + .arg("-r") + .arg("a") + .arg("c") + .set_stdout(io::stdout()) + .succeeds(); assert_eq!(get_mode(at.plus("b")), 0o40555); assert_eq!(get_mode(at.plus("b/b")), 0o40555); @@ -7488,37 +7496,38 @@ fn test_cp_preserve_directory_permissions_by_default() { #[test] #[cfg(not(target_os = "windows"))] -fn test_cp_no_preserve_directory_permissions() { +fn test_cp_existing_perm_dir() { + use std::io; + let scene = TestScenario::new(util_name!()); let at = &scene.fixtures; - let dir = "a/b/c/d"; - let file = "foo.txt"; - - at.mkdir_all(dir); - - let file_path = format!("{dir}/{file}"); - - at.touch(file_path); - - scene.cmd("chmod").arg("-R").arg("555").arg("a").succeeds(); - scene.cmd("cp").arg("-r").arg("a").arg("b").succeeds(); + scene + .cmd("mkdir") + .arg("-p") + .arg("-m") + .arg("ug-s,u=rwx,g=rwx,o=rx") + .arg("src/dir") + .umask(0o022) + .succeeds(); + scene + .cmd("mkdir") + .arg("-p") + .arg("-m") + .arg("ug-s,u=rwx,g=,o=") + .arg("dst/dir") + .umask(0o022) + .succeeds(); scene .ucmd() .arg("-r") - .arg("--no-preserve=mode") - .arg("a") - .arg("c") + .arg("src/.") + .arg("dst/") + .set_stdout(io::stdout()) .succeeds(); - assert_eq!(get_mode(at.plus("b")), 0o40555); - assert_eq!(get_mode(at.plus("b/b")), 0o40555); - assert_eq!(get_mode(at.plus("b/b/c")), 0o40555); - assert_eq!(get_mode(at.plus("b/b/c/d")), 0o40555); + let mode = get_mode(at.plus("dst/dir")); - assert_eq!(get_mode(at.plus("c")), 0o40755); - assert_eq!(get_mode(at.plus("c/b")), 0o40755); - assert_eq!(get_mode(at.plus("c/b/c")), 0o40755); - assert_eq!(get_mode(at.plus("c/b/c/d")), 0o40755); + assert_eq!(mode, 0o40700); } From b31ffdaafe0ae2726f675520c0c1bff13220225c Mon Sep 17 00:00:00 2001 From: Nikola Lukovic Date: Wed, 31 Dec 2025 16:53:43 +0100 Subject: [PATCH 4/5] cp: Fix default preserve when creating directories --- src/uu/cp/src/copydir.rs | 28 ++++++++++++------- src/uu/cp/src/cp.rs | 45 ++++++++++++++++++++----------- src/uucore/src/lib/features/fs.rs | 1 - tests/by-util/test_cp.rs | 27 +++++-------------- 4 files changed, 54 insertions(+), 47 deletions(-) diff --git a/src/uu/cp/src/copydir.rs b/src/uu/cp/src/copydir.rs index 2fe5b73e949..dd4531d8b58 100644 --- a/src/uu/cp/src/copydir.rs +++ b/src/uu/cp/src/copydir.rs @@ -166,6 +166,7 @@ impl<'a> Context<'a> { /// } /// ]; /// ``` +#[derive(Debug)] struct Entry { /// The absolute path to file or directory to copy. source_absolute: PathBuf, @@ -178,6 +179,9 @@ struct Entry { /// Whether the destination is a file. target_is_file: bool, + + /// Whether we created the destination dir + target_is_created: bool, } impl Entry { @@ -231,6 +235,7 @@ impl Entry { source_relative, local_to_target, target_is_file, + target_is_created: false, }) } } @@ -239,7 +244,7 @@ impl Entry { /// Copy a single entry during a directory traversal. fn copy_direntry( progress_bar: Option<&ProgressBar>, - entry: &Entry, + entry: &mut Entry, entry_is_symlink: bool, entry_is_dir_no_follow: bool, options: &Options, @@ -270,6 +275,7 @@ fn copy_direntry( options, Some(&entry.source_absolute), )?; + entry.target_is_created = true; if options.verbose { println!( "{}", @@ -421,7 +427,7 @@ pub(crate) fn copy_directory( let mut last_iter: Option = None; // Keep track of all directories we've created that need permission fixes - let mut dirs_needing_permissions: Vec<(PathBuf, PathBuf)> = Vec::new(); + let mut dirs_needing_permissions: Vec<(PathBuf, PathBuf, bool)> = Vec::new(); // Traverse the contents of the directory, copying each one. for direntry_result in WalkDir::new(root) @@ -440,11 +446,11 @@ pub(crate) fn copy_directory( } Err(_) => (direntry_type.is_symlink(), direntry_type.is_dir()), }; - let entry = Entry::new(&context, direntry_path, options.no_target_dir)?; + let mut entry = Entry::new(&context, direntry_path, options.no_target_dir)?; copy_direntry( progress_bar, - &entry, + &mut entry, entry_is_symlink, entry_is_dir_no_follow, options, @@ -470,8 +476,11 @@ pub(crate) fn copy_directory( entry_is_dir_no_follow || (options.dereference && direntry_path.is_dir()); if is_dir_for_permissions { // Add this directory to our list for permission fixing later - dirs_needing_permissions - .push((entry.source_absolute.clone(), entry.local_to_target.clone())); + dirs_needing_permissions.push(( + entry.source_absolute.clone(), + entry.local_to_target.clone(), + entry.target_is_created, + )); // If true, last_iter is not a parent of this iter. // The means we just exited a directory. @@ -504,6 +513,7 @@ pub(crate) fn copy_directory( &entry.source_absolute, &entry.local_to_target, options, + entry.target_is_created, )?; } } @@ -519,8 +529,8 @@ pub(crate) fn copy_directory( // Fix permissions for all directories we created // This ensures that even sibling directories get their permissions fixed - for (source_path, dest_path) in dirs_needing_permissions { - copy_attributes(&source_path, &dest_path, options)?; + for (source_path, dest_path, is_dir_created) in dirs_needing_permissions { + copy_attributes(&source_path, &dest_path, options, is_dir_created)?; } // Also fix permissions for parent directories, @@ -529,7 +539,7 @@ pub(crate) fn copy_directory( let dest = target.join(root.file_name().unwrap()); for (x, y) in aligned_ancestors(root, dest.as_path()) { if let Ok(src) = canonicalize(x, MissingHandling::Normal, ResolveMode::Physical) { - copy_attributes(&src, y, options)?; + copy_attributes(&src, y, options, false)?; } } } diff --git a/src/uu/cp/src/cp.rs b/src/uu/cp/src/cp.rs index 52c8b25f022..e0691723d05 100644 --- a/src/uu/cp/src/cp.rs +++ b/src/uu/cp/src/cp.rs @@ -194,7 +194,7 @@ pub enum SparseMode { } /// The expected file type of copy target -#[derive(Copy, Clone)] +#[derive(Copy, Clone, Debug)] pub enum TargetType { Directory, File, @@ -1532,7 +1532,7 @@ fn copy_source( if options.parents { for (x, y) in aligned_ancestors(source, dest.as_path()) { if let Ok(src) = canonicalize(x, MissingHandling::Normal, ResolveMode::Physical) { - copy_attributes(&src, y, options)?; + copy_attributes(&src, y, options, false)?; } } } @@ -1681,11 +1681,24 @@ fn copy_extended_attrs(source: &Path, dest: &Path) -> CopyResult<()> { } /// Copy the specified attributes from one path to another. -pub(crate) fn copy_attributes(source: &Path, dest: &Path, options: &Options) -> CopyResult<()> { +pub(crate) fn copy_attributes( + source: &Path, + dest: &Path, + options: &Options, + is_dir_created: bool, +) -> CopyResult<()> { let context = &*format!("{} -> {}", source.quote(), dest.quote()); let source_metadata = fs::symlink_metadata(source).map_err(|e| CpError::IoErrContext(e, context.to_owned()))?; + let is_preserve_required = matches!(options.attributes.mode, Preserve::Yes { required: true }); + + let mode = if !is_preserve_required && dest.is_dir() && is_dir_created { + Preserve::Yes { required: false } + } else { + options.attributes.mode + }; + // Ownership must be changed first to avoid interfering with mode change. #[cfg(unix)] handle_preserve(&options.attributes.ownership, || -> CopyResult<()> { @@ -1723,22 +1736,22 @@ pub(crate) fn copy_attributes(source: &Path, dest: &Path, options: &Options) -> Ok(()) })?; - handle_preserve(&options.attributes.mode, || -> CopyResult<()> { + handle_preserve(&mode, || -> CopyResult<()> { // The `chmod()` system call that underlies the // `fs::set_permissions()` call is unable to change the // permissions of a symbolic link. In that case, we just // do nothing, since every symbolic link has the same // permissions. if !dest.is_symlink() { - // let dest_permissions = calculate_dest_permissions( - // fs::metadata(dest).ok().as_ref(), - // dest, - // &source_metadata, - // options, - // context, - // )?; - let dest_permissions = source_metadata.permissions(); - fs::set_permissions(dest, dest_permissions) + let mut perms = source_metadata.permissions(); + if is_dir_created && !is_preserve_required { + let mode = handle_no_preserve_mode(options, perms.mode()); + use uucore::mode::get_umask; + let mode = mode & !get_umask(); + perms.set_mode(mode); + } + + fs::set_permissions(dest, perms) .map_err(|e| CpError::IoErrContext(e, context.to_owned()))?; // FIXME: Implement this for windows as well #[cfg(feature = "feat_acl")] @@ -2546,14 +2559,14 @@ fn copy_file( .ok() .filter(|p| p.exists()) .unwrap_or_else(|| source.to_path_buf()); - copy_attributes(&src_for_attrs, dest, &options)?; + copy_attributes(&src_for_attrs, dest, options, false)?; } else if source_is_stream && !source.exists() { // Some stream files may not exist after we have copied it, // like anonymous pipes. Thus, we can't really copy its // attributes. However, this is already handled in the stream // copy function (see `copy_stream` under platform/linux.rs). } else { - copy_attributes(source, dest, options)?; + copy_attributes(source, dest, options, false)?; } #[cfg(feature = "selinux")] @@ -2729,7 +2742,7 @@ fn copy_link( delete_path(dest, options)?; } symlink_file(&link, dest, symlinked_files)?; - copy_attributes(source, dest, options) + copy_attributes(source, dest, options, false) } /// Generate an error message if `target` is not the correct `target_type` diff --git a/src/uucore/src/lib/features/fs.rs b/src/uucore/src/lib/features/fs.rs index bebfd1821cf..adaec8c7286 100644 --- a/src/uucore/src/lib/features/fs.rs +++ b/src/uucore/src/lib/features/fs.rs @@ -538,7 +538,6 @@ pub fn display_permissions_unix(mode: mode_t, display_file_type: bool) -> String /// std::fs::create_dir("foo/."); fails in pure Rust pub fn dir_strip_dot_for_creation(path: &Path) -> PathBuf { let path_str = path.to_string_lossy(); - if path_str.ends_with("/.") || path_str.ends_with("/./") { // Do a simple dance to strip the "/." Path::new(&path).components().collect() diff --git a/tests/by-util/test_cp.rs b/tests/by-util/test_cp.rs index 9d84cb89a9b..c27c7104820 100644 --- a/tests/by-util/test_cp.rs +++ b/tests/by-util/test_cp.rs @@ -25,7 +25,7 @@ use std::os::unix::fs::{FileTypeExt, MetadataExt}; use std::os::windows::fs::symlink_file; #[cfg(not(windows))] use std::path::Path; -#[cfg(target_os = "linux")] + use std::path::PathBuf; #[cfg(any(target_os = "linux", target_os = "android"))] @@ -3654,8 +3654,8 @@ fn test_copy_dir_preserve_subdir_permissions() { ucmd.args(&["-p", "-r", "a1", "b1"]) .succeeds() - .no_stderr() - .no_stdout(); + .no_stdout() + .no_stderr(); // Make sure everything is preserved assert!(at.dir_exists("b1")); @@ -7456,10 +7456,9 @@ fn test_cp_archive_deref_flag_ordering() { } } +#[test] #[cfg(not(target_os = "windows"))] fn test_cp_preserve_directory_permissions_by_default() { - use std::io; - let scene = TestScenario::new(util_name!()); let at = &scene.fixtures; @@ -7475,13 +7474,7 @@ fn test_cp_preserve_directory_permissions_by_default() { scene.cmd("chmod").arg("-R").arg("555").arg("a").succeeds(); scene.cmd("cp").arg("-r").arg("a").arg("b").succeeds(); - scene - .ucmd() - .arg("-r") - .arg("a") - .arg("c") - .set_stdout(io::stdout()) - .succeeds(); + scene.ucmd().arg("-r").arg("a").arg("c").succeeds(); assert_eq!(get_mode(at.plus("b")), 0o40555); assert_eq!(get_mode(at.plus("b/b")), 0o40555); @@ -7497,8 +7490,6 @@ fn test_cp_preserve_directory_permissions_by_default() { #[test] #[cfg(not(target_os = "windows"))] fn test_cp_existing_perm_dir() { - use std::io; - let scene = TestScenario::new(util_name!()); let at = &scene.fixtures; @@ -7519,13 +7510,7 @@ fn test_cp_existing_perm_dir() { .umask(0o022) .succeeds(); - scene - .ucmd() - .arg("-r") - .arg("src/.") - .arg("dst/") - .set_stdout(io::stdout()) - .succeeds(); + scene.ucmd().arg("-r").arg("src/.").arg("dst/").succeeds(); let mode = get_mode(at.plus("dst/dir")); From f617d1278a4105eaa0cbe63488e13590d4db6449 Mon Sep 17 00:00:00 2001 From: Nikola Lukovic Date: Sun, 4 Jan 2026 16:52:29 +0100 Subject: [PATCH 5/5] cp: Fix explicit preserve mode --- src/uu/cp/src/cp.rs | 19 ++++++++++++------- tests/by-util/test_cp.rs | 28 +++++++++++++++++++++++++++- 2 files changed, 39 insertions(+), 8 deletions(-) diff --git a/src/uu/cp/src/cp.rs b/src/uu/cp/src/cp.rs index e0691723d05..9c906e758bc 100644 --- a/src/uu/cp/src/cp.rs +++ b/src/uu/cp/src/cp.rs @@ -1692,8 +1692,9 @@ pub(crate) fn copy_attributes( fs::symlink_metadata(source).map_err(|e| CpError::IoErrContext(e, context.to_owned()))?; let is_preserve_required = matches!(options.attributes.mode, Preserve::Yes { required: true }); + let is_explicit_true = matches!(options.attributes.mode, Preserve::No { explicit: true }); - let mode = if !is_preserve_required && dest.is_dir() && is_dir_created { + let mode = if !is_preserve_required && !is_explicit_true && dest.is_dir() && is_dir_created { Preserve::Yes { required: false } } else { options.attributes.mode @@ -1743,12 +1744,16 @@ pub(crate) fn copy_attributes( // do nothing, since every symbolic link has the same // permissions. if !dest.is_symlink() { - let mut perms = source_metadata.permissions(); - if is_dir_created && !is_preserve_required { - let mode = handle_no_preserve_mode(options, perms.mode()); - use uucore::mode::get_umask; - let mode = mode & !get_umask(); - perms.set_mode(mode); + let perms = source_metadata.permissions(); + #[cfg(unix)] + { + let mut perms = source_metadata.permissions(); + if is_dir_created && !is_preserve_required && !is_explicit_true { + let mode = handle_no_preserve_mode(options, perms.mode()); + use uucore::mode::get_umask; + let mode = mode & !get_umask(); + perms.set_mode(mode); + } } fs::set_permissions(dest, perms) diff --git a/tests/by-util/test_cp.rs b/tests/by-util/test_cp.rs index c27c7104820..d9f1a7b6170 100644 --- a/tests/by-util/test_cp.rs +++ b/tests/by-util/test_cp.rs @@ -25,7 +25,6 @@ use std::os::unix::fs::{FileTypeExt, MetadataExt}; use std::os::windows::fs::symlink_file; #[cfg(not(windows))] use std::path::Path; - use std::path::PathBuf; #[cfg(any(target_os = "linux", target_os = "android"))] @@ -7516,3 +7515,30 @@ fn test_cp_existing_perm_dir() { assert_eq!(mode, 0o40700); } + +#[test] +#[cfg(not(target_os = "windows"))] +fn test_cp_gnu_preserve_mode() { + use std::io; + + let scene = TestScenario::new(util_name!()); + let at = &scene.fixtures; + + scene.cmd("mkdir").arg("d1").succeeds(); + scene.cmd("mkdir").arg("d2").succeeds(); + scene.cmd("chmod").arg("705").arg("d2").succeeds(); + + scene + .ucmd() + .arg("--no-preserve=mode") + .arg("-r") + .arg("d2") + .arg("d3") + .set_stdout(io::stdout()) + .succeeds(); + + let d1_mode = get_mode(at.plus("d1")); + let d3_mode = get_mode(at.plus("d3")); + + assert_eq!(d1_mode, d3_mode); +}