cp: fix recursive copy of readonly directories

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<PathBuf>,
     copied_files: &mut HashMap<FileInformation, PathBuf>,
     created_parent_dirs: &mut HashSet<PathBuf>,
-) -> CopyResult<()> {
+) -> CopyResult<bool> {
     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<DirEntry> = 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)?;
             }
         }
     }

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`

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);
+}