Merge pull request #8449 from sylvestre/issue-8407

cp: fix directory permission preservation for sibling directories with -a (Closes: #8407)

Author: drinkcat

src/uu/cp/src/copydir.rs

@@ -377,6 +377,9 @@ pub(crate) fn copy_directory(
     // The directory we were in during the previous iteration
     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();
+
     // Traverse the contents of the directory, copying each one.
     for direntry_result in WalkDir::new(root)
         .same_file_system(options.one_file_system)
@@ -408,6 +411,14 @@ pub(crate) fn copy_directory(
                 // `./a/b/c` into `./a/`, in which case we'll need to fix the
                 // permissions of both `./a/b/c` and `./a/b`, in that order.)
                 if direntry.file_type().is_dir() {
+                    // Add this directory to our list for permission fixing later
+                    let entry_for_tracking =
+                        Entry::new(&context, direntry.path(), options.no_target_dir)?;
+                    dirs_needing_permissions.push((
+                        entry_for_tracking.source_absolute,
+                        entry_for_tracking.local_to_target,
+                    ));
+
                     // If true, last_iter is not a parent of this iter.
                     // The means we just exited a directory.
                     let went_up = if let Some(last_iter) = &last_iter {
@@ -452,25 +463,10 @@ pub(crate) fn copy_directory(
         }
     }
 
-    // Handle final directory permission fixes.
-    // This is almost the same as the permission-fixing code above,
-    // with minor differences (commented)
-    if let Some(last_iter) = last_iter {
-        let diff = last_iter.path().strip_prefix(root).unwrap();
-
-        // Do _not_ skip `.` this time, since we know we're done.
-        // This is where we fix the permissions of the top-level
-        // directory we just copied.
-        for p in diff.ancestors() {
-            let src = root.join(p);
-            let entry = Entry::new(&context, &src, options.no_target_dir)?;
-
-            copy_attributes(
-                &entry.source_absolute,
-                &entry.local_to_target,
-                &options.attributes,
-            )?;
-        }
+    // 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)?;
     }
 
     // Also fix permissions for parent directories,

tests/by-util/test_cp.rs

@@ -4,7 +4,7 @@
 // file that was distributed with this source code.
 
 // spell-checker:ignore (flags) reflink (fs) tmpfs (linux) rlimit Rlim NOFILE clob btrfs neve ROOTDIR USERDIR outfile uufs xattrs
-// spell-checker:ignore bdfl hlsl IRWXO IRWXG nconfined matchpathcon libselinux-devel prwx doesnotexist
+// spell-checker:ignore bdfl hlsl IRWXO IRWXG nconfined matchpathcon libselinux-devel prwx doesnotexist reftests subdirs
 use uucore::display::Quotable;
 #[cfg(feature = "feat_selinux")]
 use uucore::selinux::get_getfattr_output;
@@ -6279,6 +6279,47 @@ fn test_cp_preserve_xattr_readonly_source() {
     );
 }
 
+#[test]
+#[cfg(unix)]
+fn test_cp_archive_preserves_directory_permissions() {
+    // Test for issue #8407
+    let (at, mut ucmd) = at_and_ucmd!();
+
+    at.mkdir("test-images");
+
+    let subdirs = ["fail", "gif-test-suite", "randomly-modified", "reftests"];
+    let mode = 0o755;
+
+    for (i, subdir) in subdirs.iter().enumerate() {
+        let path = format!("test-images/{subdir}");
+        at.mkdir(&path);
+        at.set_mode(&path, mode);
+        at.write(&format!("{path}/test{}.txt", i + 1), "test content");
+    }
+
+    ucmd.arg("-a")
+        .arg("test-images")
+        .arg("test-images-copy")
+        .succeeds();
+
+    let check_mode = |path: &str| {
+        let metadata = at.metadata(path);
+        let mode = metadata.permissions().mode();
+        // Check that the permissions are 755 (only checking the last 9 bits)
+        assert_eq!(
+            mode & 0o777,
+            0o755,
+            "Directory {} has incorrect permissions: {:o}",
+            path,
+            mode & 0o777
+        );
+    };
+
+    for subdir in subdirs {
+        check_mode(&format!("test-images-copy/{subdir}"));
+    }
+}
+
 #[test]
 #[cfg(unix)]
 fn test_cp_from_stdin() {