cp: copy attributes after making subdir (#6884)

src/uu/cp/src/copydir.rs

@@ -79,6 +79,12 @@ fn get_local_to_root_parent(
     }
 }
 
+/// Given an iterator, return all its items except the last.
+fn skip_last<T>(mut iter: impl Iterator<Item = T>) -> impl Iterator<Item = T> {
+    let last = iter.next();
+    iter.scan(last, |state, item| std::mem::replace(state, Some(item)))
+}
+
 /// Paths that are invariant throughout the traversal when copying a directory.
 struct Context<'a> {
     /// The current working directory at the time of starting the traversal.
@@ -162,17 +168,18 @@ struct Entry {
 }
 
 impl Entry {
-    fn new(
+    fn new<A: AsRef<Path>>(
         context: &Context,
-        direntry: &DirEntry,
+        source: A,
         no_target_dir: bool,
     ) -> Result<Self, StripPrefixError> {
-        let source_relative = direntry.path().to_path_buf();
+        let source = source.as_ref();
+        let source_relative = source.to_path_buf();
         let source_absolute = context.current_dir.join(&source_relative);
         let mut descendant =
             get_local_to_root_parent(&source_absolute, context.root_parent.as_deref())?;
         if no_target_dir {
-            let source_is_dir = direntry.path().is_dir();
+            let source_is_dir = source.is_dir();
             if path_ends_with_terminator(context.target) && source_is_dir {
                 if let Err(e) = std::fs::create_dir_all(context.target) {
                     eprintln!("Failed to create directory: {e}");
@@ -213,6 +220,7 @@ where
     // `path.ends_with(".")` does not seem to work
     path.as_ref().display().to_string().ends_with("/.")
 }
+
 #[allow(clippy::too_many_arguments)]
 /// Copy a single entry during a directory traversal.
 fn copy_direntry(
@@ -246,7 +254,7 @@ fn copy_direntry(
         if target_is_file {
             return Err("cannot overwrite non-directory with directory".into());
         } else {
-            build_dir(options, &local_to_target, false)?;
+            build_dir(&local_to_target, false, options, Some(&source_absolute))?;
             if options.verbose {
                 println!("{}", context_for(&source_relative, &local_to_target));
             }
@@ -371,7 +379,7 @@ pub(crate) fn copy_directory(
     let tmp = if options.parents {
         if let Some(parent) = root.parent() {
             let new_target = target.join(parent);
-            build_dir(options, &new_target, true)?;
+            build_dir(&new_target, true, options, None)?;
             if options.verbose {
                 // For example, if copying file `a/b/c` and its parents
                 // to directory `d/`, then print
@@ -403,6 +411,9 @@ pub(crate) fn copy_directory(
         Err(e) => return Err(format!("failed to get current directory {e}").into()),
     };
 
+    // The directory we were in during the previous iteration
+    let mut last_iter: Option<DirEntry> = None;
+
     // Traverse the contents of the directory, copying each one.
     for direntry_result in WalkDir::new(root)
         .same_file_system(options.one_file_system)
@@ -410,7 +421,8 @@ pub(crate) fn copy_directory(
     {
         match direntry_result {
             Ok(direntry) => {
-                let entry = Entry::new(&context, &direntry, options.no_target_dir)?;
+                let entry = Entry::new(&context, direntry.path(), options.no_target_dir)?;
+
                 copy_direntry(
                     progress_bar,
                     entry,
@@ -420,23 +432,93 @@ pub(crate) fn copy_directory(
                     copied_destinations,
                     copied_files,
                 )?;
+
+                // We omit certain permissions when creating directories
+                // to prevent other users from accessing them before they're done.
+                // We thus need to fix the permissions of each directory we copy
+                // once it's contents are ready.
+                // This "fixup" is implemented here in a memory-efficient manner.
+                //
+                // We detect iterations where we "walk up" the directory tree,
+                // and fix permissions on all the directories we exited.
+                // (Note that there can be more than one! We might step out of
+                // `./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() {
+                    // 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 {
+                        last_iter.path().strip_prefix(direntry.path()).is_ok()
+                    } else {
+                        false
+                    };
+
+                    if went_up {
+                        // Compute the "difference" between `last_iter` and `direntry`.
+                        // For example, if...
+                        // - last_iter = `a/b/c/d`
+                        // - direntry = `a/b`
+                        // then diff = `c/d`
+                        //
+                        // All the unwraps() here are unreachable.
+                        let last_iter = last_iter.as_ref().unwrap();
+                        let diff = last_iter.path().strip_prefix(direntry.path()).unwrap();
+
+                        // Fix permissions for every entry in `diff`, inside-out.
+                        // We skip the last directory (which will be `.`) because
+                        // its permissions will be fixed when we walk _out_ of it.
+                        // (at this point, we might not be done copying `.`!)
+                        for p in skip_last(diff.ancestors()) {
+                            let src = direntry.path().join(p);
+                            let entry = Entry::new(&context, &src, options.no_target_dir)?;
+
+                            copy_attributes(
+                                &entry.source_absolute,
+                                &entry.local_to_target,
+                                &options.attributes,
+                            )?;
                         }
+                    }
+
+                    last_iter = Some(direntry);
+                }
+            }
+
             // Print an error message, but continue traversing the directory.
             Err(e) => show_error!("{}", e),
         }
     }
 
-    // Copy the attributes from the root directory to the target 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,
+            )?;
+        }
+    }
+
+    // Also fix permissions for parent directories,
+    // if we were asked to create them.
     if options.parents {
         let dest = target.join(root.file_name().unwrap());
-        copy_attributes(root, dest.as_path(), &options.attributes)?;
         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)?;
             }
         }
-    } else {
-        copy_attributes(root, target, &options.attributes)?;
     }
 
     Ok(())
@@ -469,20 +551,32 @@ pub fn path_has_prefix(p1: &Path, p2: &Path) -> io::Result<bool> {
 /// Builds a directory at the specified path with the given options.
 ///
 /// # Notes
-/// - It excludes certain permissions if ownership or special mode bits could
+/// - If `copy_attributes_from` is `Some`, the new directory's attributes will be
-///   potentially change.
+///   copied from the provided file. Otherwise, the new directory will have the default
+///   attributes for the current user.
+/// - This method excludes certain permissions if ownership or special mode bits could
+///   potentially change. (See `test_dir_perm_race_with_preserve_mode_and_ownership``)
 /// - The `recursive` flag determines whether parent directories should be created
 ///   if they do not already exist.
 // we need to allow unused_variable since `options` might be unused in non unix systems
 #[allow(unused_variables)]
-fn build_dir(options: &Options, path: &PathBuf, recursive: bool) -> CopyResult<()> {
+fn build_dir(
+    path: &PathBuf,
+    recursive: bool,
+    options: &Options,
+    copy_attributes_from: Option<&Path>,
+) -> CopyResult<()> {
     let mut builder = fs::DirBuilder::new();
     builder.recursive(recursive);
+
     // To prevent unauthorized access before the folder is ready,
     // exclude certain permissions if ownership or special mode bits
     // could potentially change.
     #[cfg(unix)]
     {
+        use crate::Preserve;
+        use std::os::unix::fs::PermissionsExt;
+
         // we need to allow trivial casts here because some systems like linux have u32 constants in
         // in libc while others don't.
         #[allow(clippy::unnecessary_cast)]
@@ -494,10 +588,22 @@ fn build_dir(options: &Options, path: &PathBuf, recursive: bool) -> CopyResult<(
             } else {
                 0
             } as u32;
-        excluded_perms |= uucore::mode::get_umask();
+
+        let umask = if copy_attributes_from.is_some()
+            && matches!(options.attributes.mode, Preserve::Yes { .. })
+        {
+            !fs::symlink_metadata(copy_attributes_from.unwrap())?
+                .permissions()
+                .mode()
+        } else {
+            uucore::mode::get_umask()
+        };
+
+        excluded_perms |= umask;
         let mode = !excluded_perms & 0o777; //use only the last three octet bits
         std::os::unix::fs::DirBuilderExt::mode(&mut builder, mode);
     }
+
     builder.create(path)?;
     Ok(())
 }

src/uu/cp/src/cp.rs

@@ -174,7 +174,7 @@ pub enum CopyMode {
 /// For full compatibility with GNU, these options should also combine. We
 /// currently only do a best effort imitation of that behavior, because it is
 /// difficult to achieve in clap, especially with `--no-preserve`.
-#[derive(Debug, PartialEq)]
+#[derive(Debug, PartialEq, Clone, Copy)]
 pub struct Attributes {
     #[cfg(unix)]
     pub ownership: Preserve,

tests/by-util/test_cp.rs

@@ -3365,6 +3365,29 @@ fn test_copy_dir_preserve_permissions() {
     assert_metadata_eq!(metadata1, metadata2);
 }
 
+/// cp should preserve attributes of subdirectories when copying recursively.
+#[cfg(all(not(windows), not(target_os = "freebsd"), not(target_os = "openbsd")))]
+#[test]
+fn test_copy_dir_preserve_subdir_permissions() {
+    let (at, mut ucmd) = at_and_ucmd!();
+    at.mkdir("a1");
+    at.mkdir("a1/a2");
+    // Use different permissions for a better test
+    at.set_mode("a1/a2", 0o0555);
+    at.set_mode("a1", 0o0777);
+
+    ucmd.args(&["-p", "-r", "a1", "b1"])
+        .succeeds()
+        .no_stderr()
+        .no_stdout();
+
+    // Make sure everything is preserved
+    assert!(at.dir_exists("b1"));
+    assert!(at.dir_exists("b1/a2"));
+    assert_metadata_eq!(at.metadata("a1"), at.metadata("b1"));
+    assert_metadata_eq!(at.metadata("a1/a2"), at.metadata("b1/a2"));
+}
+
 /// Test for preserving permissions when copying a directory, even in
 /// the face of an inaccessible file in that directory.
 #[cfg(all(not(windows), not(target_os = "freebsd"), not(target_os = "openbsd")))]