util: added umask manipulation (#6404)

Co-authored-by: Terts Diepraam <terts.diepraam@gmail.com>

tests/by-util/test_chmod.rs

@@ -3,22 +3,18 @@
 // For the full copyright and license information, please view the LICENSE
 // file that was distributed with this source code.
 use crate::common::util::{AtPath, TestScenario, UCommand};
-use once_cell::sync::Lazy;
 use std::fs::{metadata, set_permissions, OpenOptions, Permissions};
 use std::os::unix::fs::{OpenOptionsExt, PermissionsExt};
-use std::sync::Mutex;
-
-use libc::umask;
 
 static TEST_FILE: &str = "file";
 static REFERENCE_FILE: &str = "reference";
 static REFERENCE_PERMS: u32 = 0o247;
-static UMASK_MUTEX: Lazy<Mutex<()>> = Lazy::new(|| Mutex::new(()));
 
 struct TestCase {
     args: Vec<&'static str>,
     before: u32,
     after: u32,
+    umask: Option<libc::mode_t>,
 }
 
 fn make_file(file: &str, mode: u32) {
@@ -45,6 +41,9 @@ fn run_single_test(test: &TestCase, at: &AtPath, mut ucmd: UCommand) {
 
     for arg in &test.args {
         ucmd.arg(arg);
+        if let Some(umask) = test.umask {
+            ucmd.umask(umask);
+        }
     }
     let r = ucmd.run();
     if !r.succeeded() {
@@ -73,46 +72,55 @@ fn test_chmod_octal() {
             args: vec!["0700", TEST_FILE],
             before: 0o100000,
             after: 0o100700,
+            umask: None,
         },
         TestCase {
             args: vec!["0070", TEST_FILE],
             before: 0o100000,
             after: 0o100070,
+            umask: None,
         },
         TestCase {
             args: vec!["0007", TEST_FILE],
             before: 0o100000,
             after: 0o100007,
+            umask: None,
         },
         TestCase {
             args: vec!["-0700", TEST_FILE],
             before: 0o100700,
             after: 0o100000,
+            umask: None,
         },
         TestCase {
             args: vec!["-0070", TEST_FILE],
             before: 0o100060,
             after: 0o100000,
+            umask: None,
         },
         TestCase {
             args: vec!["-0007", TEST_FILE],
             before: 0o100001,
             after: 0o100000,
+            umask: None,
         },
         TestCase {
             args: vec!["+0100", TEST_FILE],
             before: 0o100600,
             after: 0o100700,
+            umask: None,
         },
         TestCase {
             args: vec!["+0020", TEST_FILE],
             before: 0o100050,
             after: 0o100070,
+            umask: None,
         },
         TestCase {
             args: vec!["+0004", TEST_FILE],
             before: 0o100003,
             after: 0o100007,
+            umask: None,
         },
     ];
     run_tests(tests);
@@ -122,86 +130,94 @@ fn test_chmod_octal() {
 #[allow(clippy::unreadable_literal)]
 // spell-checker:disable-next-line
 fn test_chmod_ugoa() {
-    let _guard = UMASK_MUTEX.lock();
-
-    let last = unsafe { umask(0) };
     let tests = vec![
         TestCase {
             args: vec!["u=rwx", TEST_FILE],
             before: 0o100000,
             after: 0o100700,
+            umask: Some(0),
         },
         TestCase {
             args: vec!["g=rwx", TEST_FILE],
             before: 0o100000,
             after: 0o100070,
+            umask: Some(0),
         },
         TestCase {
             args: vec!["o=rwx", TEST_FILE],
             before: 0o100000,
             after: 0o100007,
+            umask: Some(0),
         },
         TestCase {
             args: vec!["a=rwx", TEST_FILE],
             before: 0o100000,
             after: 0o100777,
+            umask: Some(0),
         },
         TestCase {
             args: vec!["-r", TEST_FILE],
             before: 0o100777,
             after: 0o100333,
+            umask: Some(0),
         },
         TestCase {
             args: vec!["-w", TEST_FILE],
             before: 0o100777,
             after: 0o100555,
+            umask: Some(0),
         },
         TestCase {
             args: vec!["-x", TEST_FILE],
             before: 0o100777,
             after: 0o100666,
+            umask: Some(0),
         },
     ];
     run_tests(tests);
 
-    unsafe {
-        umask(0o022);
-    }
     let tests = vec![
         TestCase {
             args: vec!["u=rwx", TEST_FILE],
             before: 0o100000,
             after: 0o100700,
+            umask: Some(0o022),
         },
         TestCase {
             args: vec!["g=rwx", TEST_FILE],
             before: 0o100000,
             after: 0o100070,
+            umask: Some(0o022),
         },
         TestCase {
             args: vec!["o=rwx", TEST_FILE],
             before: 0o100000,
             after: 0o100007,
+            umask: Some(0o022),
         },
         TestCase {
             args: vec!["a=rwx", TEST_FILE],
             before: 0o100000,
             after: 0o100777,
+            umask: Some(0o022),
         },
         TestCase {
             args: vec!["+rw", TEST_FILE],
             before: 0o100000,
             after: 0o100644,
+            umask: Some(0o022),
         },
         TestCase {
             args: vec!["=rwx", TEST_FILE],
             before: 0o100000,
             after: 0o100755,
+            umask: Some(0o022),
         },
         TestCase {
             args: vec!["-x", TEST_FILE],
             before: 0o100777,
             after: 0o100666,
+            umask: Some(0o022),
         },
     ];
     run_tests(tests);
@@ -219,10 +235,6 @@ fn test_chmod_ugoa() {
         metadata(at.plus("file")).unwrap().permissions().mode(),
         0o100577
     );
-
-    unsafe {
-        umask(last);
-    }
 }
 
 #[test]
@@ -233,26 +245,31 @@ fn test_chmod_ugo_copy() {
             args: vec!["u=g", TEST_FILE],
             before: 0o100070,
             after: 0o100770,
+            umask: None,
         },
         TestCase {
             args: vec!["g=o", TEST_FILE],
             before: 0o100005,
             after: 0o100055,
+            umask: None,
         },
         TestCase {
             args: vec!["o=u", TEST_FILE],
             before: 0o100200,
             after: 0o100202,
+            umask: None,
         },
         TestCase {
             args: vec!["u-g", TEST_FILE],
             before: 0o100710,
             after: 0o100610,
+            umask: None,
         },
         TestCase {
             args: vec!["u+g", TEST_FILE],
             before: 0o100250,
             after: 0o100750,
+            umask: None,
         },
     ];
     run_tests(tests);
@@ -261,18 +278,13 @@ fn test_chmod_ugo_copy() {
 #[test]
 #[allow(clippy::unreadable_literal)]
 fn test_chmod_many_options() {
-    let _guard = UMASK_MUTEX.lock();
-
-    let original_umask = unsafe { umask(0) };
     let tests = vec![TestCase {
         args: vec!["-r,a+w", TEST_FILE],
         before: 0o100444,
         after: 0o100222,
+        umask: Some(0),
     }];
     run_tests(tests);
-    unsafe {
-        umask(original_umask);
-    }
 }
 
 #[test]
@@ -283,11 +295,13 @@ fn test_chmod_reference_file() {
             args: vec!["--reference", REFERENCE_FILE, TEST_FILE],
             before: 0o100070,
             after: 0o100247,
+            umask: None,
         },
         TestCase {
             args: vec!["a-w", "--reference", REFERENCE_FILE, TEST_FILE],
             before: 0o100070,
             after: 0o100247,
+            umask: None,
         },
     ];
     let (at, ucmd) = at_and_ucmd!();
@@ -318,14 +332,16 @@ fn test_permission_denied() {
 #[test]
 #[allow(clippy::unreadable_literal)]
 fn test_chmod_recursive() {
-    let _guard = UMASK_MUTEX.lock();
-
-    let original_umask = unsafe { umask(0) };
     let (at, mut ucmd) = at_and_ucmd!();
     at.mkdir("a");
     at.mkdir("a/b");
     at.mkdir("a/b/c");
     at.mkdir("z");
+
+    // create expected permissions by removing read bits and write bits to the current perms
+    let a_perms_expected = (at.metadata("a").permissions().mode() & !0o444) | 0o222;
+    let z_perms_expected = (at.metadata("z").permissions().mode() & !0o444) | 0o222;
+
     make_file(&at.plus_as_string("a/a"), 0o100444);
     make_file(&at.plus_as_string("a/b/b"), 0o100444);
     make_file(&at.plus_as_string("a/b/c/c"), 0o100444);
@@ -338,6 +354,7 @@ fn test_chmod_recursive() {
         .arg("-r,a+w")
         .arg("a")
         .arg("z")
+        .umask(0)
         .fails()
         .stderr_is("chmod: Permission denied\n");
 
@@ -346,12 +363,8 @@ fn test_chmod_recursive() {
     assert_eq!(at.metadata("a/b/b").permissions().mode(), 0o100444);
     assert_eq!(at.metadata("a/b/c/c").permissions().mode(), 0o100444);
     println!("mode {:o}", at.metadata("a").permissions().mode());
-    assert_eq!(at.metadata("a").permissions().mode(), 0o40333);
+    assert_eq!(at.metadata("a").permissions().mode(), a_perms_expected);
-    assert_eq!(at.metadata("z").permissions().mode(), 0o40333);
+    assert_eq!(at.metadata("z").permissions().mode(), z_perms_expected);
-
-    unsafe {
-        umask(original_umask);
-    }
 }
 
 #[test]
@@ -550,6 +563,7 @@ fn test_mode_after_dash_dash() {
             args: vec!["--", "-r", TEST_FILE],
             before: 0o100_777,
             after: 0o100_333,
+            umask: None,
         },
         &at,
         ucmd,

tests/by-util/test_cp.rs

@@ -1718,18 +1718,17 @@ fn test_cp_preserve_links_case_7() {
 #[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")
+        .umask(0o077)
         .succeeds();
 
     assert!(at.file_exists("b"));
@@ -1737,8 +1736,6 @@ fn test_cp_no_preserve_mode() {
     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]
@@ -2535,8 +2532,6 @@ fn test_copy_symlink_force() {
 fn test_no_preserve_mode() {
     use std::os::unix::prelude::MetadataExt;
 
-    use uucore::mode::get_umask;
-
     const PERMS_ALL: u32 = if cfg!(target_os = "freebsd") {
         // Only the superuser can set the sticky bit on a file.
         0o6777
@@ -2547,14 +2542,15 @@ fn test_no_preserve_mode() {
     let (at, mut ucmd) = at_and_ucmd!();
     at.touch("file");
     set_permissions(at.plus("file"), PermissionsExt::from_mode(PERMS_ALL)).unwrap();
+    let umask: u16 = 0o022;
     ucmd.arg("file")
         .arg("dest")
+        .umask(umask as libc::mode_t)
         .succeeds()
         .no_stderr()
         .no_stdout();
-    let umask = get_umask();
     // remove sticky bit, setuid and setgid bit; apply umask
-    let expected_perms = PERMS_ALL & !0o7000 & !umask;
+    let expected_perms = PERMS_ALL & !0o7000 & !umask as u32;
     assert_eq!(
         at.plus("dest").metadata().unwrap().mode() & 0o7777,
         expected_perms

tests/by-util/test_mkdir.rs

@@ -4,33 +4,22 @@
 // file that was distributed with this source code.
 use crate::common::util::TestScenario;
 #[cfg(not(windows))]
-use libc::{mode_t, umask};
+use libc::mode_t;
-use once_cell::sync::Lazy;
 #[cfg(not(windows))]
 use std::os::unix::fs::PermissionsExt;
-use std::sync::Mutex;
-
-// tests in `test_mkdir.rs` cannot run in parallel since some tests alter the umask. This may cause
-// other tests to run under a wrong set of permissions
-//
-// when writing a test case, acquire this mutex before proceeding with the main logic of the test
-static TEST_MUTEX: Lazy<Mutex<()>> = Lazy::new(|| Mutex::new(()));
 
 #[test]
 fn test_invalid_arg() {
-    let _guard = TEST_MUTEX.lock();
     new_ucmd!().arg("--definitely-invalid").fails().code_is(1);
 }
 
 #[test]
 fn test_mkdir_mkdir() {
-    let _guard = TEST_MUTEX.lock();
     new_ucmd!().arg("test_dir").succeeds();
 }
 
 #[test]
 fn test_mkdir_verbose() {
-    let _guard = TEST_MUTEX.lock();
     let expected = "mkdir: created directory 'test_dir'\n";
     new_ucmd!()
         .arg("test_dir")
@@ -41,8 +30,6 @@ fn test_mkdir_verbose() {
 
 #[test]
 fn test_mkdir_dup_dir() {
-    let _guard = TEST_MUTEX.lock();
-
     let scene = TestScenario::new(util_name!());
     let test_dir = "test_dir";
 
@@ -52,13 +39,11 @@ fn test_mkdir_dup_dir() {
 
 #[test]
 fn test_mkdir_mode() {
-    let _guard = TEST_MUTEX.lock();
     new_ucmd!().arg("-m").arg("755").arg("test_dir").succeeds();
 }
 
 #[test]
 fn test_mkdir_parent() {
-    let _guard = TEST_MUTEX.lock();
     let scene = TestScenario::new(util_name!());
     let test_dir = "parent_dir/child_dir";
 
@@ -70,14 +55,11 @@ fn test_mkdir_parent() {
 
 #[test]
 fn test_mkdir_no_parent() {
-    let _guard = TEST_MUTEX.lock();
     new_ucmd!().arg("parent_dir/child_dir").fails();
 }
 
 #[test]
 fn test_mkdir_dup_dir_parent() {
-    let _guard = TEST_MUTEX.lock();
-
     let scene = TestScenario::new(util_name!());
     let test_dir = "test_dir";
 
@@ -88,13 +70,16 @@ fn test_mkdir_dup_dir_parent() {
 #[cfg(not(windows))]
 #[test]
 fn test_mkdir_parent_mode() {
-    let _guard = TEST_MUTEX.lock();
     let (at, mut ucmd) = at_and_ucmd!();
 
     let default_umask: mode_t = 0o160;
-    let original_umask = unsafe { umask(default_umask) };
 
-    ucmd.arg("-p").arg("a/b").succeeds().no_stderr().no_stdout();
+    ucmd.arg("-p")
+        .arg("a/b")
+        .umask(default_umask)
+        .succeeds()
+        .no_stderr()
+        .no_stdout();
 
     assert!(at.dir_exists("a"));
     // parents created by -p have permissions set to "=rwx,u+wx"
@@ -108,35 +93,28 @@ fn test_mkdir_parent_mode() {
         at.metadata("a/b").permissions().mode() as mode_t,
         (!default_umask & 0o777) + 0o40000
     );
-
-    unsafe {
-        umask(original_umask);
-    }
 }
 
 #[cfg(not(windows))]
 #[test]
 fn test_mkdir_parent_mode_check_existing_parent() {
-    let _guard = TEST_MUTEX.lock();
     let (at, mut ucmd) = at_and_ucmd!();
 
     at.mkdir("a");
+    let parent_a_perms = at.metadata("a").permissions().mode();
 
     let default_umask: mode_t = 0o160;
-    let original_umask = unsafe { umask(default_umask) };
 
     ucmd.arg("-p")
         .arg("a/b/c")
+        .umask(default_umask)
         .succeeds()
         .no_stderr()
         .no_stdout();
 
     assert!(at.dir_exists("a"));
     // parent dirs that already exist do not get their permissions modified
-    assert_eq!(
+    assert_eq!(at.metadata("a").permissions().mode(), parent_a_perms);
-        at.metadata("a").permissions().mode() as mode_t,
-        (!original_umask & 0o777) + 0o40000
-    );
     assert!(at.dir_exists("a/b"));
     assert_eq!(
         at.metadata("a/b").permissions().mode() as mode_t,
@@ -147,16 +125,10 @@ fn test_mkdir_parent_mode_check_existing_parent() {
         at.metadata("a/b/c").permissions().mode() as mode_t,
         (!default_umask & 0o777) + 0o40000
     );
-
-    unsafe {
-        umask(original_umask);
-    }
 }
 
 #[test]
 fn test_mkdir_dup_file() {
-    let _guard = TEST_MUTEX.lock();
-
     let scene = TestScenario::new(util_name!());
     let test_file = "test_file.txt";
 
@@ -171,7 +143,6 @@ fn test_mkdir_dup_file() {
 #[test]
 #[cfg(not(windows))]
 fn test_symbolic_mode() {
-    let _guard = TEST_MUTEX.lock();
     let (at, mut ucmd) = at_and_ucmd!();
     let test_dir = "test_dir";
 
@@ -183,24 +154,23 @@ fn test_symbolic_mode() {
 #[test]
 #[cfg(not(windows))]
 fn test_symbolic_alteration() {
-    let _guard = TEST_MUTEX.lock();
     let (at, mut ucmd) = at_and_ucmd!();
     let test_dir = "test_dir";
 
     let default_umask = 0o022;
-    let original_umask = unsafe { umask(default_umask) };
 
-    ucmd.arg("-m").arg("-w").arg(test_dir).succeeds();
+    ucmd.arg("-m")
+        .arg("-w")
+        .arg(test_dir)
+        .umask(default_umask)
+        .succeeds();
     let perms = at.metadata(test_dir).permissions().mode();
     assert_eq!(perms, 0o40577);
-
-    unsafe { umask(original_umask) };
 }
 
 #[test]
 #[cfg(not(windows))]
 fn test_multi_symbolic() {
-    let _guard = TEST_MUTEX.lock();
     let (at, mut ucmd) = at_and_ucmd!();
     let test_dir = "test_dir";
 
@@ -211,7 +181,6 @@ fn test_multi_symbolic() {
 
 #[test]
 fn test_recursive_reporting() {
-    let _guard = TEST_MUTEX.lock();
     let test_dir = "test_dir/test_dir_a/test_dir_b";
 
     new_ucmd!()
@@ -238,8 +207,6 @@ fn test_recursive_reporting() {
 
 #[test]
 fn test_mkdir_trailing_dot() {
-    let _guard = TEST_MUTEX.lock();
-
     new_ucmd!().arg("-p").arg("-v").arg("test_dir").succeeds();
 
     new_ucmd!()
@@ -265,20 +232,13 @@ fn test_mkdir_trailing_dot() {
 #[cfg(not(windows))]
 fn test_umask_compliance() {
     fn test_single_case(umask_set: mode_t) {
-        let _guard = TEST_MUTEX.lock();
-
         let test_dir = "test_dir";
         let (at, mut ucmd) = at_and_ucmd!();
 
-        let original_umask = unsafe { umask(umask_set) };
+        ucmd.arg(test_dir).umask(umask_set).succeeds();
-
-        ucmd.arg(test_dir).succeeds();
         let perms = at.metadata(test_dir).permissions().mode() as mode_t;
 
         assert_eq!(perms, (!umask_set & 0o0777) + 0o40000); // before compare, add the set GUID, UID bits
-        unsafe {
-            umask(original_umask);
-        } // set umask back to original
     }
 
     for i in 0o0..0o027 {

tests/common/util.rs

@@ -8,6 +8,8 @@
 
 #![allow(dead_code)]
 
+#[cfg(unix)]
+use libc::mode_t;
 #[cfg(unix)]
 use nix::pty::OpenptyResult;
 use pretty_assertions::assert_eq;
@@ -1253,6 +1255,8 @@ pub struct UCommand {
     #[cfg(unix)]
     terminal_simulation: Option<TerminalSimulation>,
     tmpd: Option<Rc<TempDir>>, // drop last
+    #[cfg(unix)]
+    umask: Option<mode_t>,
 }
 
 impl UCommand {
@@ -1418,6 +1422,13 @@ impl UCommand {
         self
     }
 
+    #[cfg(unix)]
+    /// The umask is a value that restricts the permissions of newly created files and directories.
+    pub fn umask(&mut self, umask: mode_t) -> &mut Self {
+        self.umask = Some(umask);
+        self
+    }
+
     /// Set the timeout for [`UCommand::run`] and similar methods in [`UCommand`].
     ///
     /// After the timeout elapsed these `run` methods (besides [`UCommand::run_no_wait`]) will
@@ -1708,6 +1719,16 @@ impl UCommand {
             }
         }
 
+        #[cfg(unix)]
+        if let Some(umask) = self.umask {
+            unsafe {
+                command.pre_exec(move || {
+                    libc::umask(umask);
+                    Ok(())
+                });
+            }
+        }
+
         (command, captured_stdout, captured_stderr, stdin_pty)
     }
 
@@ -3872,4 +3893,32 @@ mod tests {
             .no_stderr()
             .stdout_is("8\n16\n");
     }
+
+    #[cfg(unix)]
+    #[test]
+    fn test_altering_umask() {
+        use uucore::mode::get_umask;
+        let p_umask = get_umask();
+        // make sure we are not testing against the same umask
+        let c_umask = if p_umask == 0o002 { 0o007 } else { 0o002 };
+        let expected = if cfg!(target_os = "android") {
+            if p_umask == 0o002 {
+                "007\n"
+            } else {
+                "002\n"
+            }
+        } else if p_umask == 0o002 {
+            "0007\n"
+        } else {
+            "0002\n"
+        };
+
+        let ts = TestScenario::new("util");
+        ts.cmd("sh")
+            .args(&["-c", "umask"])
+            .umask(c_umask)
+            .succeeds()
+            .stdout_is(expected);
+        std::assert_eq!(p_umask, get_umask()); // make sure parent umask didn't change
+    }
 }