Merge pull request #5353 from tommady/fix-5327

cp: fix cp -a --no-preserve=mode doesn't keep fully the mode

src/uu/cp/src/cp.rs

@@ -2,7 +2,7 @@
 //
 // For the full copyright and license information, please view the LICENSE
 // file that was distributed with this source code.
-// spell-checker:ignore (ToDO) copydir ficlone fiemap ftruncate linkgs lstat nlink nlinks pathbuf pwrite reflink strs xattrs symlinked deduplicated advcpmv nushell
+// spell-checker:ignore (ToDO) copydir ficlone fiemap ftruncate linkgs lstat nlink nlinks pathbuf pwrite reflink strs xattrs symlinked deduplicated advcpmv nushell IRWXG IRWXO IRWXU IRWXUGO IRWXU IRWXG IRWXO IRWXUGO
 #![allow(clippy::missing_safety_doc)]
 #![allow(clippy::extra_unused_lifetimes)]
 
@@ -184,7 +184,9 @@ pub struct Attributes {
 
 #[derive(Clone, Copy, Debug, PartialEq, Eq)]
 pub enum Preserve {
-    No,
+    // explicit means whether the --no-preserve flag is used or not to distinguish out the default value.
+    // e.g. --no-preserve=mode means mode = No { explicit = true }
+    No { explicit: bool },
     Yes { required: bool },
 }
 
@@ -197,9 +199,9 @@ impl PartialOrd for Preserve {
 impl Ord for Preserve {
     fn cmp(&self, other: &Self) -> Ordering {
         match (self, other) {
-            (Self::No, Self::No) => Ordering::Equal,
+            (Self::No { .. }, Self::No { .. }) => Ordering::Equal,
-            (Self::Yes { .. }, Self::No) => Ordering::Greater,
+            (Self::Yes { .. }, Self::No { .. }) => Ordering::Greater,
-            (Self::No, Self::Yes { .. }) => Ordering::Less,
+            (Self::No { .. }, Self::Yes { .. }) => Ordering::Less,
             (
                 Self::Yes { required: req_self },
                 Self::Yes {
@@ -800,7 +802,7 @@ impl Attributes {
             }
             #[cfg(not(feature = "feat_selinux"))]
             {
-                Preserve::No
+                Preserve::No { explicit: false }
             }
         },
         links: Preserve::Yes { required: true },
@@ -809,12 +811,12 @@ impl Attributes {
 
     pub const NONE: Self = Self {
         #[cfg(unix)]
-        ownership: Preserve::No,
+        ownership: Preserve::No { explicit: false },
-        mode: Preserve::No,
+        mode: Preserve::No { explicit: false },
-        timestamps: Preserve::No,
+        timestamps: Preserve::No { explicit: false },
-        context: Preserve::No,
+        context: Preserve::No { explicit: false },
-        links: Preserve::No,
+        links: Preserve::No { explicit: false },
-        xattr: Preserve::No,
+        xattr: Preserve::No { explicit: false },
     };
 
     // TODO: ownership is required if the user is root, for non-root users it's not required.
@@ -954,7 +956,9 @@ impl Options {
             if attribute_strs.len() > 0 {
                 let no_preserve_attributes = Attributes::parse_iter(attribute_strs)?;
                 if matches!(no_preserve_attributes.links, Preserve::Yes { .. }) {
-                    attributes.links = Preserve::No;
+                    attributes.links = Preserve::No { explicit: true };
+                } else if matches!(no_preserve_attributes.mode, Preserve::Yes { .. }) {
+                    attributes.mode = Preserve::No { explicit: true };
                 }
             }
         }
@@ -1050,11 +1054,22 @@ impl Options {
 
     fn preserve_hard_links(&self) -> bool {
         match self.attributes.links {
-            Preserve::No => false,
+            Preserve::No { .. } => false,
             Preserve::Yes { .. } => true,
         }
     }
 
+    #[cfg(unix)]
+    fn preserve_mode(&self) -> (bool, bool) {
+        match self.attributes.mode {
+            Preserve::No { explicit } => match explicit {
+                true => (false, true),
+                false => (false, false),
+            },
+            Preserve::Yes { .. } => (true, false),
+        }
+    }
+
     /// Whether to force overwriting the destination file.
     fn force(&self) -> bool {
         matches!(self.overwrite, OverwriteMode::Clobber(ClobberMode::Force))
@@ -1299,7 +1314,7 @@ impl OverwriteMode {
 /// If it's required, then the error is thrown.
 fn handle_preserve<F: Fn() -> CopyResult<()>>(p: &Preserve, f: F) -> CopyResult<()> {
     match p {
-        Preserve::No => {}
+        Preserve::No { .. } => {}
         Preserve::Yes { required } => {
             let result = f();
             if *required {
@@ -1736,15 +1751,10 @@ fn copy_file(
         let mut permissions = source_metadata.permissions();
         #[cfg(unix)]
         {
-            use uucore::mode::get_umask;
+            let mut mode = handle_no_preserve_mode(options, permissions.mode());
-
-            let mut mode = permissions.mode();
-
-            // remove sticky bit, suid and gid bit
-            const SPECIAL_PERMS_MASK: u32 = 0o7000;
-            mode &= !SPECIAL_PERMS_MASK;
 
             // apply umask
+            use uucore::mode::get_umask;
             mode &= !get_umask();
 
             permissions.set_mode(mode);
@@ -1873,6 +1883,49 @@ fn copy_file(
     Ok(())
 }
 
+#[cfg(unix)]
+fn handle_no_preserve_mode(options: &Options, org_mode: u32) -> u32 {
+    let (is_preserve_mode, is_explicit_no_preserve_mode) = options.preserve_mode();
+    if !is_preserve_mode {
+        use libc::{
+            S_IRGRP, S_IROTH, S_IRUSR, S_IRWXG, S_IRWXO, S_IRWXU, S_IWGRP, S_IWOTH, S_IWUSR,
+        };
+
+        #[cfg(not(any(
+            target_os = "android",
+            target_os = "macos",
+            target_os = "macos-12",
+            target_os = "freebsd",
+        )))]
+        {
+            const MODE_RW_UGO: u32 = S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP | S_IROTH | S_IWOTH;
+            const S_IRWXUGO: u32 = S_IRWXU | S_IRWXG | S_IRWXO;
+            match is_explicit_no_preserve_mode {
+                true => return MODE_RW_UGO,
+                false => return org_mode & S_IRWXUGO,
+            };
+        }
+
+        #[cfg(any(
+            target_os = "android",
+            target_os = "macos",
+            target_os = "macos-12",
+            target_os = "freebsd",
+        ))]
+        {
+            const MODE_RW_UGO: u32 =
+                (S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP | S_IROTH | S_IWOTH) as u32;
+            const S_IRWXUGO: u32 = (S_IRWXU | S_IRWXG | S_IRWXO) as u32;
+            match is_explicit_no_preserve_mode {
+                true => return MODE_RW_UGO,
+                false => return org_mode & S_IRWXUGO,
+            };
+        }
+    }
+
+    org_mode
+}
+
 /// Copy the file from `source` to `dest` either using the normal `fs::copy` or a
 /// copy-on-write scheme if --reflink is specified and the filesystem supports it.
 fn copy_helper(

tests/by-util/test_cp.rs

@@ -2,7 +2,7 @@
 //
 // For the full copyright and license information, please view the LICENSE
 // file that was distributed with this source code.
-// spell-checker:ignore (flags) reflink (fs) tmpfs (linux) rlimit Rlim NOFILE clob btrfs ROOTDIR USERDIR procfs outfile
+// spell-checker:ignore (flags) reflink (fs) tmpfs (linux) rlimit Rlim NOFILE clob btrfs ROOTDIR USERDIR procfs outfile uufs
 
 use crate::common::util::TestScenario;
 #[cfg(not(windows))]
@@ -1551,6 +1551,32 @@ fn test_cp_preserve_links_case_7() {
     assert!(at.plus("dest").join("g").exists());
 }
 
+#[test]
+#[cfg(unix)]
+fn test_cp_no_preserve_mode() {
+    use libc::umask;
+    use uucore::fs as uufs;
+    let (at, mut ucmd) = at_and_ucmd!();
+
+    at.touch("a");
+    at.set_mode("a", 0o731);
+    unsafe { umask(0o077) };
+
+    ucmd.arg("-a")
+        .arg("--no-preserve=mode")
+        .arg("a")
+        .arg("b")
+        .succeeds();
+
+    assert!(at.file_exists("b"));
+
+    let metadata_b = std::fs::metadata(at.subdir.join("b")).unwrap();
+    let permission_b = uufs::display_permissions(&metadata_b, false);
+    assert_eq!(permission_b, "rw-------".to_string());
+
+    unsafe { umask(0o022) };
+}
+
 #[test]
 // For now, disable the test on Windows. Symlinks aren't well support on Windows.
 // It works on Unix for now and it works locally when run from a powershell