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,7 @@
 
 #[derive(Clone, Copy, Debug, PartialEq, Eq)]
 pub enum Preserve {
-    No,
+    No { explicit: bool },
     Yes { required: bool },
 }
 
@@ -197,9 +197,9 @@
 impl Ord for Preserve {
     fn cmp(&self, other: &Self) -> Ordering {
         match (self, other) {
-            (Self::No, Self::No) => Ordering::Equal,
-            (Self::Yes { .. }, Self::No) => Ordering::Greater,
-            (Self::No, Self::Yes { .. }) => Ordering::Less,
+            (Self::No { .. }, Self::No { .. }) => Ordering::Equal,
+            (Self::Yes { .. }, Self::No { .. }) => Ordering::Greater,
+            (Self::No { .. }, Self::Yes { .. }) => Ordering::Less,
             (
                 Self::Yes { required: req_self },
                 Self::Yes {
@@ -800,7 +800,7 @@
             }
             #[cfg(not(feature = "feat_selinux"))]
             {
-                Preserve::No
+                Preserve::No { explicit: false }
             }
         },
         links: Preserve::Yes { required: true },
@@ -809,12 +809,12 @@
 
     pub const NONE: Self = Self {
         #[cfg(unix)]
-        ownership: Preserve::No,
-        mode: Preserve::No,
-        timestamps: Preserve::No,
-        context: Preserve::No,
-        links: Preserve::No,
-        xattr: Preserve::No,
+        ownership: Preserve::No { explicit: false },
+        mode: Preserve::No { explicit: false },
+        timestamps: Preserve::No { explicit: false },
+        context: Preserve::No { explicit: false },
+        links: Preserve::No { explicit: false },
+        xattr: Preserve::No { explicit: false },
     };
 
     // TODO: ownership is required if the user is root, for non-root users it's not required.
@@ -954,7 +954,9 @@
             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 +1052,22 @@
 
     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 +1312,7 @@
 /// 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 {
@@ -1728,15 +1741,53 @@
         let mut permissions = source_metadata.permissions();
         #[cfg(unix)]
         {
-            use uucore::mode::get_umask;
-
             let mut mode = permissions.mode();
 
-            // remove sticky bit, suid and gid bit
-            const SPECIAL_PERMS_MASK: u32 = 0o7000;
-            mode &= !SPECIAL_PERMS_MASK;
+            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;
+                #[cfg(not(any(
+                    target_os = "android",
+                    target_os = "macos",
+                    target_os = "macos-12",
+                    target_os = "freebsd",
+                )))]
+                const S_IRWXUGO: u32 = S_IRWXU | S_IRWXG | S_IRWXO;
+
+                #[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;
+                #[cfg(any(
+                    target_os = "android",
+                    target_os = "macos",
+                    target_os = "macos-12",
+                    target_os = "freebsd",
+                ))]
+                const S_IRWXUGO: u32 = (S_IRWXU | S_IRWXG | S_IRWXO) as u32;
+
+                match is_explicit_no_preserve_mode {
+                    true => mode = MODE_RW_UGO,
+                    false => mode &= S_IRWXUGO,
+                }
+            }
 
             // apply umask
+            use uucore::mode::get_umask;
             mode &= !get_umask();
 
             permissions.set_mode(mode);

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))]
@@ -1552,6 +1552,32 @@ fn test_cp_preserve_links_case_7() {
     assert!(at.plus("dest").join("g").exists());
 }
 
+#[test]
+#[cfg(all(unix))]
+fn test_cp_no_preserve_mode_case() {
+    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