Merge pull request #3973 from jfinkels/cp-directory-preserve-permissions

cp: preserve permissions when copying directory and don't terminate early on inaccessible file

src/uu/cp/src/cp.rs

@@ -38,7 +38,7 @@ use libc::mkfifo;
 use quick_error::ResultExt;
 use uucore::backup_control::{self, BackupMode};
 use uucore::display::Quotable;
-use uucore::error::{set_exit_code, UClapError, UError, UResult, UUsageError};
+use uucore::error::{set_exit_code, UClapError, UError, UIoError, UResult, UUsageError};
 use uucore::format_usage;
 use uucore::fs::{
     canonicalize, paths_refer_to_same_file, FileInformation, MissingHandling, ResolveMode,
@@ -1173,17 +1173,36 @@ fn copy_directory(
                     }?;
                 }
             } else {
-                copy_file(
+                // At this point, `path` is just a plain old file.
+                // Terminate this function immediately if there is any
+                // kind of error *except* a "permission denied" error.
+                //
+                // TODO What other kinds of errors, if any, should
+                // cause us to continue walking the directory?
+                match copy_file(
                     path.as_path(),
                     local_to_target.as_path(),
                     options,
                     symlinked_files,
                     false,
-                )?;
+                ) {
+                    Ok(_) => {}
+                    Err(Error::IoErrContext(e, _))
+                        if e.kind() == std::io::ErrorKind::PermissionDenied =>
+                    {
+                        show!(uio_error!(
+                            e,
+                            "cannot open {} for reading",
+                            p.path().quote()
+                        ));
+                    }
+                    Err(e) => return Err(e),
+                }
             }
         }
     }
-
+    // Copy the attributes from the root directory to the target directory.
+    copy_attributes(root, target, &options.preserve_attributes)?;
     Ok(())
 }
 
@@ -1206,6 +1225,14 @@ impl OverwriteMode {
     }
 }
 
+/// Copy the specified attributes from one path to another.
+fn copy_attributes(source: &Path, dest: &Path, attributes: &[Attribute]) -> CopyResult<()> {
+    for attribute in attributes {
+        copy_attribute(source, dest, attribute)?;
+    }
+    Ok(())
+}
+
 fn copy_attribute(source: &Path, dest: &Path, attribute: &Attribute) -> CopyResult<()> {
     let context = &*format!("{} -> {}", source.quote(), dest.quote());
     let source_metadata = fs::symlink_metadata(source).context(context)?;
@@ -1554,9 +1581,7 @@ fn copy_file(
         // the user does not have permission to write to the file.
         fs::set_permissions(dest, dest_permissions).ok();
     }
-    for attribute in &options.preserve_attributes {
+    copy_attributes(source, dest, &options.preserve_attributes)?;
-        copy_attribute(source, dest, attribute)?;
-    }
     Ok(())
 }
 

tests/by-util/test_cp.rs

@@ -48,6 +48,27 @@ static TEST_MOUNT_OTHER_FILESYSTEM_FILE: &str = "mount/DO_NOT_copy_me.txt";
 #[cfg(unix)]
 static TEST_NONEXISTENT_FILE: &str = "nonexistent_file.txt";
 
+/// Assert that mode, ownership, and permissions of two metadata objects match.
+#[cfg(not(windows))]
+macro_rules! assert_metadata_eq {
+    ($m1:expr, $m2:expr) => {{
+        assert_eq!($m1.mode(), $m2.mode(), "mode is different");
+        assert_eq!($m1.uid(), $m2.uid(), "uid is different");
+        assert_eq!($m1.atime(), $m2.atime(), "atime is different");
+        assert_eq!(
+            $m1.atime_nsec(),
+            $m2.atime_nsec(),
+            "atime_nsec is different"
+        );
+        assert_eq!($m1.mtime(), $m2.mtime(), "mtime is different");
+        assert_eq!(
+            $m1.mtime_nsec(),
+            $m2.mtime_nsec(),
+            "mtime_nsec is different"
+        );
+    }};
+}
+
 #[test]
 fn test_cp_cp() {
     let (at, mut ucmd) = at_and_ucmd!();
@@ -1787,20 +1808,7 @@ fn test_copy_through_dangling_symlink_no_dereference_permissions() {
     {
         let metadata1 = at.symlink_metadata("dangle");
         let metadata2 = at.symlink_metadata("d2");
-        assert_eq!(metadata1.mode(), metadata2.mode(), "mode is different");
+        assert_metadata_eq!(metadata1, metadata2);
-        assert_eq!(metadata1.uid(), metadata2.uid(), "uid is different");
-        assert_eq!(metadata1.atime(), metadata2.atime(), "atime is different");
-        assert_eq!(
-            metadata1.atime_nsec(),
-            metadata2.atime_nsec(),
-            "atime_nsec is different"
-        );
-        assert_eq!(metadata1.mtime(), metadata2.mtime(), "mtime is different");
-        assert_eq!(
-            metadata1.mtime_nsec(),
-            metadata2.mtime_nsec(),
-            "mtime_nsec is different"
-        );
     }
 }
 
@@ -2136,3 +2144,67 @@ fn test_copy_nested_directory_to_itself_disallowed() {
         .fails()
         .stderr_only(expected);
 }
+
+/// Test for preserving permissions when copying a directory.
+#[cfg(not(windows))]
+#[test]
+fn test_copy_dir_preserve_permissions() {
+    // Create a directory that has some non-default permissions.
+    let (at, mut ucmd) = at_and_ucmd!();
+    at.mkdir("d1");
+    at.set_mode("d1", 0o0500);
+
+    // Copy the directory, preserving those permissions.
+    //
+    //         preserve permissions (mode, ownership, timestamps)
+    //            |    copy directories recursively
+    //            |      |   from this source directory
+    //            |      |    |   to this destination
+    //            |      |    |     |
+    //            V      V    V     V
+    ucmd.args(&["-p", "-R", "d1", "d2"])
+        .succeeds()
+        .no_stderr()
+        .no_stdout();
+    assert!(at.dir_exists("d2"));
+
+    // Assert that the permissions are preserved.
+    let metadata1 = at.metadata("d1");
+    let metadata2 = at.metadata("d2");
+    assert_metadata_eq!(metadata1, metadata2);
+}
+
+/// Test for preserving permissions when copying a directory, even in
+/// the face of an inaccessible file in that directory.
+#[cfg(not(windows))]
+#[test]
+fn test_copy_dir_preserve_permissions_inaccessible_file() {
+    // Create a directory that has some non-default permissions and
+    // contains an inaccessible file.
+    let (at, mut ucmd) = at_and_ucmd!();
+    at.mkdir("d1");
+    at.touch("d1/f");
+    at.set_mode("d1/f", 0);
+    at.set_mode("d1", 0o0500);
+
+    // Copy the directory, preserving those permissions. There should
+    // be an error message that the file `d1/f` is inaccessible.
+    //
+    //         preserve permissions (mode, ownership, timestamps)
+    //            |    copy directories recursively
+    //            |      |   from this source directory
+    //            |      |    |   to this destination
+    //            |      |    |     |
+    //            V      V    V     V
+    ucmd.args(&["-p", "-R", "d1", "d2"])
+        .fails()
+        .status_code(1)
+        .stderr_only("cp: cannot open 'd1/f' for reading: Permission denied");
+    assert!(at.dir_exists("d2"));
+    assert!(!at.file_exists("d2/f"));
+
+    // Assert that the permissions are preserved.
+    let metadata1 = at.metadata("d1");
+    let metadata2 = at.metadata("d2");
+    assert_metadata_eq!(metadata1, metadata2);
+}

tests/common/util.rs

@@ -19,7 +19,7 @@ use std::ffi::OsStr;
 use std::fs::{self, hard_link, File, OpenOptions};
 use std::io::{BufWriter, Read, Result, Write};
 #[cfg(unix)]
-use std::os::unix::fs::{symlink as symlink_dir, symlink as symlink_file};
+use std::os::unix::fs::{symlink as symlink_dir, symlink as symlink_file, PermissionsExt};
 #[cfg(windows)]
 use std::os::windows::fs::{symlink_dir, symlink_file};
 #[cfg(windows)]
@@ -821,6 +821,20 @@ impl AtPath {
             s
         }
     }
+
+    /// Set the permissions of the specified file.
+    ///
+    /// # Panics
+    ///
+    /// This function panics if there is an error loading the metadata
+    /// or setting the permissions of the file.
+    #[cfg(not(windows))]
+    pub fn set_mode(&self, filename: &str, mode: u32) {
+        let path = self.plus(filename);
+        let mut perms = std::fs::metadata(&path).unwrap().permissions();
+        perms.set_mode(mode);
+        std::fs::set_permissions(&path, perms).unwrap();
+    }
 }
 
 /// An environment for running a single uutils test case, serves three functions: