Move UmaskGuard in uucore and use it for mkdir, mkfifo & mknod

src/uu/mkdir/src/mkdir.rs

@@ -3,7 +3,7 @@
 // For the full copyright and license information, please view the LICENSE
 // file that was distributed with this source code.
 
-// spell-checker:ignore (ToDO) ugoa cmode RAII
+// spell-checker:ignore (ToDO) ugoa cmode
 
 use clap::builder::ValueParser;
 use clap::parser::ValuesRef;
@@ -245,28 +245,6 @@ fn create_dir(path: &Path, is_parent: bool, config: &Config) -> UResult<()> {
     create_single_dir(path, is_parent, config)
 }
 
-/// RAII guard to restore umask on drop, ensuring cleanup even on panic.
-#[cfg(unix)]
-struct UmaskGuard(uucore::libc::mode_t);
-
-#[cfg(unix)]
-impl UmaskGuard {
-    /// Set umask to the given value and return a guard that restores the original on drop.
-    fn set(new_mask: uucore::libc::mode_t) -> Self {
-        let old_mask = unsafe { uucore::libc::umask(new_mask) };
-        Self(old_mask)
-    }
-}
-
-#[cfg(unix)]
-impl Drop for UmaskGuard {
-    fn drop(&mut self) {
-        unsafe {
-            uucore::libc::umask(self.0);
-        }
-    }
-}
-
 /// Create a directory with the exact mode specified, bypassing umask.
 ///
 /// GNU mkdir temporarily sets umask to 0 before calling mkdir(2), ensuring the
@@ -278,7 +256,7 @@ fn create_dir_with_mode(path: &Path, mode: u32) -> std::io::Result<()> {
 
     // Temporarily set umask to 0 so the directory is created with the exact mode.
     // The guard restores the original umask on drop, even if we panic.
-    let _guard = UmaskGuard::set(0);
+    let _guard = mode::UmaskGuard::set(0);
 
     std::fs::DirBuilder::new().mode(mode).create(path)
 }

src/uu/mkfifo/src/mkfifo.rs

@@ -4,10 +4,9 @@
 // file that was distributed with this source code.
 
 use clap::{Arg, ArgAction, Command, value_parser};
+use nix::libc;
 use nix::sys::stat::Mode;
 use nix::unistd::mkfifo;
-use std::fs;
-use std::os::unix::fs::PermissionsExt;
 use uucore::display::Quotable;
 use uucore::error::{UResult, USimpleError};
 use uucore::translate;
@@ -48,22 +47,22 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> {
     };
 
     for f in fifos {
-        if mkfifo(f.as_str(), Mode::from_bits_truncate(0o666)).is_err() {
+        // Create FIFO with exact mode by temporarily setting umask to 0.
+        // This avoids a race condition where the FIFO briefly exists with
+        // umask-based permissions before chmod is called.
+        let result = {
+            let _guard = uucore::mode::UmaskGuard::set(0);
+            mkfifo(f.as_str(), Mode::from_bits_truncate(mode as libc::mode_t))
+        };
+
+        if result.is_err() {
             show!(USimpleError::new(
                 1,
                 translate!("mkfifo-error-cannot-create-fifo", "path" => f.quote()),
             ));
             continue;
         }
 
-        // Explicitly set the permissions to ignore umask
-        if let Err(e) = fs::set_permissions(&f, fs::Permissions::from_mode(mode)) {
-            return Err(USimpleError::new(
-                1,
-                translate!("mkfifo-error-cannot-set-permissions", "path" => f.quote(), "error" => e),
-            ));
-        }
-
         // Apply SELinux context if requested
         #[cfg(all(feature = "selinux", any(target_os = "linux", target_os = "android")))]
         {
@@ -76,7 +75,7 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> {
                 if let Err(e) =
                     uucore::selinux::set_selinux_security_context(Path::new(&f), context)
                 {
-                    let _ = fs::remove_file(f);
+                    let _ = std::fs::remove_file(f);
                     return Err(USimpleError::new(1, e.to_string()));
                 }
             }

src/uu/mknod/src/mknod.rs

@@ -64,57 +64,55 @@ pub struct Config<'a> {
 fn mknod(file_name: &str, config: Config) -> i32 {
     let c_str = CString::new(file_name).expect("Failed to convert to CString");
 
-    unsafe {
-        // set umask to 0 and store previous umask
-        let have_prev_umask = if config.use_umask {
+    let errno = {
+        // Use UmaskGuard to ensure umask is restored even on panic
+        let _guard = if config.use_umask {
             None
         } else {
-            Some(libc::umask(0))
+            Some(uucore::mode::UmaskGuard::set(0))
         };
 
-        let errno = libc::mknod(c_str.as_ptr(), config.mode, config.dev);
+        // SAFETY: mknod is a standard POSIX syscall. The c_str pointer is valid
+        // for the duration of the call.
+        unsafe { libc::mknod(c_str.as_ptr(), config.mode, config.dev) }
+    }; // Guard dropped here, restoring umask
 
-        // set umask back to original value
-        if let Some(prev_umask) = have_prev_umask {
-            libc::umask(prev_umask);
-        }
-
-        if errno == -1 {
-            let c_str = CString::new(uucore::execution_phrase().as_bytes())
-                .expect("Failed to convert to CString");
-            // shows the error from the mknod syscall
+    if errno == -1 {
+        let c_str = CString::new(uucore::execution_phrase().as_bytes())
+            .expect("Failed to convert to CString");
+        // shows the error from the mknod syscall
+        // SAFETY: perror is a standard C function that doesn't store the pointer
+        unsafe {
             libc::perror(c_str.as_ptr());
         }
+    }
 
-        // Apply SELinux context if requested
-        #[cfg(feature = "selinux")]
-        if config.set_security_context {
-            if let Err(e) = uucore::selinux::set_selinux_security_context(
-                std::path::Path::new(file_name),
-                config.context,
-            ) {
-                // if it fails, delete the file
-                let _ = std::fs::remove_dir(file_name);
-                eprintln!("{}: {}", uucore::util_name(), e);
-                return 1;
-            }
+    // Apply SELinux context if requested
+    #[cfg(feature = "selinux")]
+    if config.set_security_context {
+        if let Err(e) = uucore::selinux::set_selinux_security_context(
+            std::path::Path::new(file_name),
+            config.context,
+        ) {
+            // if it fails, delete the file
+            let _ = std::fs::remove_dir(file_name);
+            eprintln!("{}: {}", uucore::util_name(), e);
+            return 1;
         }
+    }
 
-        // Apply SMACK context if requested
-        #[cfg(feature = "smack")]
-        if config.set_security_context {
-            if let Err(e) =
-                uucore::smack::set_smack_label_and_cleanup(file_name, config.context, |p| {
-                    std::fs::remove_file(p)
-                })
-            {
-                eprintln!("{}: {}", uucore::util_name(), e);
-                return 1;
-            }
+    // Apply SMACK context if requested
+    #[cfg(feature = "smack")]
+    if config.set_security_context {
+        if let Err(e) = uucore::smack::set_smack_label_and_cleanup(file_name, config.context, |p| {
+            std::fs::remove_file(p)
+        }) {
+            eprintln!("{}: {}", uucore::util_name(), e);
+            return 1;
         }
-
-        errno
     }
+
+    errno
 }
 
 #[uucore::main]

src/uucore/src/lib/features/mode.rs

@@ -5,7 +5,7 @@
 
 //! Set of functions to parse modes
 
-// spell-checker:ignore (vars) fperm srwx
+// spell-checker:ignore (vars) fperm srwx RAII
 
 use libc::umask;
 
@@ -195,12 +195,76 @@ pub fn get_umask() -> u32 {
     return mask as u32;
 }
 
+/// RAII guard to restore umask on drop, ensuring cleanup even on panic.
+///
+/// This is useful when temporarily setting umask to 0 to create files or
+/// directories with exact permissions, bypassing umask. The guard ensures
+/// the original umask is restored even if a panic occurs.
+///
+/// # Thread Safety
+///
+/// Note: umask is process-wide, so this guard is not thread-safe.
+/// Concurrent use from multiple threads will cause race conditions.
+/// In tests, use a mutex to serialize access when testing umask-related functionality.
+///
+/// # Example
+///
+/// ```no_run
+/// use uucore::mode::UmaskGuard;
+/// use std::fs;
+///
+/// // Temporarily set umask to 0, then restore original on drop
+/// let _guard = UmaskGuard::set(0);
+/// fs::create_dir("dir_with_exact_mode").unwrap();
+/// // Original umask is restored here when _guard is dropped
+/// ```
+#[cfg(unix)]
+pub struct UmaskGuard(libc::mode_t);
+
+#[cfg(unix)]
+impl UmaskGuard {
+    /// Set umask to the given value and return a guard that restores the original on drop.
+    ///
+    /// # Safety
+    ///
+    /// This function manipulates the process-wide umask. While this is safe from a
+    /// memory safety perspective, it can affect other threads in multi-threaded programs.
+    /// The guard pattern ensures restoration even on panic.
+    pub fn set(new_mask: libc::mode_t) -> Self {
+        // SAFETY: umask always succeeds and doesn't operate on memory.
+        // The returned value is the previous umask which we store for restoration.
+        let old_mask = unsafe { libc::umask(new_mask) };
+        Self(old_mask)
+    }
+}
+
+#[cfg(unix)]
+impl Drop for UmaskGuard {
+    fn drop(&mut self) {
+        // SAFETY: umask always succeeds. We're restoring the original value.
+        unsafe {
+            libc::umask(self.0);
+        }
+    }
+}
+
 #[cfg(test)]
 mod tests {
 
     use super::parse;
     use super::parse_chmod;
 
+    #[cfg(unix)]
+    use std::sync::Mutex;
+
+    /// Mutex to serialize umask-related tests.
+    ///
+    /// umask is process-global, so parallel tests that manipulate it can
+    /// interfere with each other. This mutex ensures only one test accesses
+    /// umask at a time.
+    #[cfg(unix)]
+    static UMASK_TEST_MUTEX: Mutex<()> = Mutex::new(());
+
     #[test]
     fn test_chmod_symbolic_modes() {
         assert_eq!(parse_chmod(0o666, "u+x", false, 0).unwrap(), 0o766);
@@ -334,4 +398,76 @@ mod tests {
         // First add user write, then set to 755 (should override)
         assert_eq!(parse("u+w,755", false, 0).unwrap(), 0o755);
     }
+
+    #[test]
+    #[cfg(unix)]
+    fn test_umask_guard_basic() {
+        use super::{UmaskGuard, get_umask};
+
+        // Acquire mutex to prevent concurrent umask tests
+        let _lock = UMASK_TEST_MUTEX.lock().unwrap();
+
+        // Save original umask
+        let original = get_umask();
+
+        // Set umask to 0 and verify it's restored
+        {
+            let _guard = UmaskGuard::set(0);
+            assert_eq!(get_umask(), 0);
+        } // Guard dropped here
+
+        // Verify original umask is restored
+        assert_eq!(get_umask(), original);
+    }
+
+    #[test]
+    #[cfg(unix)]
+    fn test_umask_guard_nested() {
+        use super::{UmaskGuard, get_umask};
+
+        // Acquire mutex to prevent concurrent umask tests
+        let _lock = UMASK_TEST_MUTEX.lock().unwrap();
+
+        let original = get_umask();
+
+        // Test nested guards work correctly
+        {
+            let _guard1 = UmaskGuard::set(0o077);
+            assert_eq!(get_umask(), 0o077);
+
+            {
+                let _guard2 = UmaskGuard::set(0);
+                assert_eq!(get_umask(), 0);
+            } // guard2 dropped, should restore to 0o077
+
+            assert_eq!(get_umask(), 0o077);
+        } // guard1 dropped, should restore to original
+
+        assert_eq!(get_umask(), original);
+    }
+
+    #[test]
+    #[cfg(unix)]
+    fn test_umask_guard_panic_safety() {
+        use super::{UmaskGuard, get_umask};
+        use std::panic::{AssertUnwindSafe, catch_unwind};
+
+        // Acquire mutex to prevent concurrent umask tests
+        let _lock = UMASK_TEST_MUTEX.lock().unwrap();
+
+        let original = get_umask();
+
+        // Test that umask is restored even if code panics
+        let result = catch_unwind(AssertUnwindSafe(|| {
+            let _guard = UmaskGuard::set(0o777);
+            assert_eq!(get_umask(), 0o777);
+            panic!("Test panic");
+        }));
+
+        // Panic should have been caught
+        assert!(result.is_err());
+
+        // Umask should still be restored to original
+        assert_eq!(get_umask(), original);
+    }
 }

tests/by-util/test_mkfifo.rs

@@ -153,17 +153,16 @@ fn test_create_fifo_permission_denied() {
     at.mkdir(no_exec_dir);
     at.set_mode(no_exec_dir, 0o644);
 
-    // We no longer attempt to modify file permission if the file was failed to be created.
-    // Therefore the error message should only contain "cannot create".
-    let err_msg = format!("mkfifo: cannot create fifo '{named_pipe}': File exists\n");
-
+    // With atomic permission setting, mkfifo fails with a single error.
+    // The exact error depends on whether the FIFO already exists or the
+    // directory lacks execute permission.
     scene
         .ucmd()
         .arg(named_pipe)
         .arg("-m")
         .arg("666")
         .fails()
-        .stderr_is(err_msg.as_str());
+        .stderr_contains("mkfifo: cannot create fifo '{named_pipe}': File exists\n");
 }
 
 #[test]