diff --git a/src/uu/cp/src/copydir.rs b/src/uu/cp/src/copydir.rs index 42e30b452d4..614bd597d18 100644 --- a/src/uu/cp/src/copydir.rs +++ b/src/uu/cp/src/copydir.rs @@ -237,6 +237,7 @@ impl Entry { #[allow(clippy::too_many_arguments)] /// Copy a single entry during a directory traversal. +/// Returns a bool value indicating whether this function created a directory or not fn copy_direntry( progress_bar: Option<&ProgressBar>, entry: &Entry, @@ -248,7 +249,7 @@ fn copy_direntry( copied_destinations: &HashSet, copied_files: &mut HashMap, created_parent_dirs: &mut HashSet, -) -> CopyResult<()> { +) -> CopyResult { let source_is_symlink = entry_is_symlink; let source_is_dir = if source_is_symlink && !options.dereference { false @@ -276,7 +277,7 @@ fn copy_direntry( context_for(&entry.source_relative, &entry.local_to_target) ); } - Ok(()) + Ok(true) }; } @@ -326,7 +327,7 @@ fn copy_direntry( // In any other case, there is nothing to do, so we just return to // continue the traversal. - Ok(()) + Ok(false) } /// Read the contents of the directory `root` and recursively copy the @@ -421,7 +422,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) @@ -442,7 +443,7 @@ pub(crate) fn copy_directory( }; let entry = Entry::new(&context, direntry_path, options.no_target_dir)?; - copy_direntry( + let created = copy_direntry( progress_bar, &entry, entry_is_symlink, @@ -475,12 +476,16 @@ pub(crate) fn copy_directory( &entry.source_absolute, &entry.local_to_target, &options.attributes, + false, )?; continue; } // 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(), + created, + )); // If true, last_iter is not a parent of this iter. // The means we just exited a directory. @@ -513,6 +518,7 @@ pub(crate) fn copy_directory( &entry.source_absolute, &entry.local_to_target, &options.attributes, + false, )?; } } @@ -528,8 +534,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, created) in dirs_needing_permissions { + copy_attributes(&source_path, &dest_path, &options.attributes, created)?; } // Also fix permissions for parent directories, @@ -538,7 +544,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.attributes, false)?; } } } diff --git a/src/uu/cp/src/cp.rs b/src/uu/cp/src/cp.rs index 8fb5629fac1..ea0740b2e0d 100644 --- a/src/uu/cp/src/cp.rs +++ b/src/uu/cp/src/cp.rs @@ -1550,7 +1550,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.attributes, false)?; } } } @@ -1717,11 +1717,21 @@ pub(crate) fn copy_attributes( source: &Path, dest: &Path, attributes: &Attributes, + 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_explicit = matches!(attributes.mode, Preserve::No { explicit: true }); + + // preserve is true by default if the destination is created by us and it's a directory + let mode = if !is_explicit && dest.is_dir() && created { + 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<()> { @@ -1759,7 +1769,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 @@ -2573,14 +2583,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.attributes, 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.attributes, false)?; } #[cfg(all(feature = "selinux", any(target_os = "linux", target_os = "android")))] @@ -2762,7 +2772,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.attributes, false) } /// 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 f01619af261..43c989bb1cd 100644 --- a/tests/by-util/test_cp.rs +++ b/tests/by-util/test_cp.rs @@ -7571,3 +7571,104 @@ fn test_cp_xattr_enotsup_handling() { std::fs::remove_file(f).ok(); } } + +#[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(); + + // only verify owner bits on Android + if cfg!(target_os = "android") { + assert_eq!(at.metadata("b").mode() & 0o700, 0o500); + assert_eq!(at.metadata("b/b").mode() & 0o700, 0o500); + assert_eq!(at.metadata("b/b/c").mode() & 0o700, 0o500); + assert_eq!(at.metadata("b/b/c/d").mode() & 0o700, 0o500); + + assert_eq!(at.metadata("c").mode() & 0o700, 0o500); + assert_eq!(at.metadata("c/b").mode() & 0o700, 0o500); + assert_eq!(at.metadata("c/b/c").mode() & 0o700, 0o500); + assert_eq!(at.metadata("c/b/c/d").mode() & 0o700, 0o500); + } else { + assert_eq!(at.metadata("b").mode(), 0o40555); + assert_eq!(at.metadata("b/b").mode(), 0o40555); + assert_eq!(at.metadata("b/b/c").mode(), 0o40555); + assert_eq!(at.metadata("b/b/c/d").mode(), 0o40555); + + assert_eq!(at.metadata("c").mode(), 0o40555); + assert_eq!(at.metadata("c/b").mode(), 0o40555); + assert_eq!(at.metadata("c/b/c").mode(), 0o40555); + assert_eq!(at.metadata("c/b/c/d").mode(), 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 = at.metadata("dst/dir").mode(); + + 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 = at.metadata("d1").mode(); + let d3_mode = at.metadata("d3").mode(); + + assert_eq!(d1_mode, d3_mode); +}