diff --git a/src/uu/cp/src/copydir.rs b/src/uu/cp/src/copydir.rs index bbd3aba6297..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. @@ -503,7 +512,8 @@ pub(crate) fn copy_directory( copy_attributes( &entry.source_absolute, &entry.local_to_target, - &options.attributes, + 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.attributes)?; + 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.attributes)?; + copy_attributes(&src, y, options, false)?; } } } diff --git a/src/uu/cp/src/cp.rs b/src/uu/cp/src/cp.rs index 3048f38b711..9c906e758bc 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.attributes)?; + copy_attributes(&src, y, options, false)?; } } } @@ -1684,15 +1684,25 @@ fn copy_extended_attrs(source: &Path, dest: &Path) -> CopyResult<()> { pub(crate) fn copy_attributes( source: &Path, dest: &Path, - attributes: &Attributes, + 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 is_explicit_true = matches!(options.attributes.mode, Preserve::No { explicit: true }); + + let mode = if !is_preserve_required && !is_explicit_true && 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(&attributes.ownership, || -> CopyResult<()> { + handle_preserve(&options.attributes.ownership, || -> CopyResult<()> { use std::os::unix::prelude::MetadataExt; use uucore::perms::Verbosity; use uucore::perms::VerbosityLevel; @@ -1727,14 +1737,26 @@ 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 // do nothing, since every symbolic link has the same // permissions. if !dest.is_symlink() { - fs::set_permissions(dest, source_metadata.permissions()) + 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) .map_err(|e| CpError::IoErrContext(e, context.to_owned()))?; // FIXME: Implement this for windows as well #[cfg(feature = "feat_acl")] @@ -1746,7 +1768,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() { @@ -1759,7 +1781,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 { @@ -1777,7 +1799,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)?; @@ -2542,14 +2564,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, 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.attributes)?; + copy_attributes(source, dest, options, false)?; } #[cfg(feature = "selinux")] @@ -2725,7 +2747,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, 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 5f4a44c4aff..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; -#[cfg(target_os = "linux")] use std::path::PathBuf; #[cfg(any(target_os = "linux", target_os = "android"))] @@ -88,6 +87,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!(); @@ -3643,8 +3653,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")); @@ -7444,3 +7454,91 @@ fn test_cp_archive_deref_flag_ordering() { assert_eq!(at.is_symlink(&dest), expect_symlink, "failed for {flags}"); } } + +#[test] +#[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!(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!(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] +#[cfg(not(target_os = "windows"))] +fn test_cp_existing_perm_dir() { + let scene = TestScenario::new(util_name!()); + let at = &scene.fixtures; + + 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("src/.").arg("dst/").succeeds(); + + let mode = get_mode(at.plus("dst/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); +}