From 90f77561da80b2a7fa8ec878ee0cd469e6ed3e44 Mon Sep 17 00:00:00 2001 From: Sylvestre Ledru Date: Sun, 18 Jan 2026 13:59:48 +0100 Subject: [PATCH 1/5] uucore: add UmaskGuard RAII helper for safe umask manipulation --- src/uucore/src/lib/features/mode.rs | 110 ++++++++++++++++++++++++++++ 1 file changed, 110 insertions(+) diff --git a/src/uucore/src/lib/features/mode.rs b/src/uucore/src/lib/features/mode.rs index d562f1fe038..bfaa06df3f4 100644 --- a/src/uucore/src/lib/features/mode.rs +++ b/src/uucore/src/lib/features/mode.rs @@ -195,6 +195,53 @@ 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. +/// +/// # 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 { @@ -334,4 +381,67 @@ 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}; + + // 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}; + + 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}; + + 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); + } } From 39fa9def6fc2efca90432074fca4b53b91dd4410 Mon Sep 17 00:00:00 2001 From: Sylvestre Ledru Date: Sun, 18 Jan 2026 14:00:08 +0100 Subject: [PATCH 2/5] mkdir: use uucore::mode::UmaskGuard instead of local implementation --- src/uu/mkdir/src/mkdir.rs | 26 ++------------------------ 1 file changed, 2 insertions(+), 24 deletions(-) diff --git a/src/uu/mkdir/src/mkdir.rs b/src/uu/mkdir/src/mkdir.rs index d08640ff08b..2978953e4b2 100644 --- a/src/uu/mkdir/src/mkdir.rs +++ b/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) } From de567c4a0c51c8ea06d47ac2d5f250543c4db40b Mon Sep 17 00:00:00 2001 From: Sylvestre Ledru Date: Sun, 18 Jan 2026 14:00:30 +0100 Subject: [PATCH 3/5] mkfifo: fix race condition by using UmaskGuard for atomic permissions --- src/uu/mkfifo/src/mkfifo.rs | 22 ++++++++++------------ tests/by-util/test_mkfifo.rs | 11 ++++------- 2 files changed, 14 insertions(+), 19 deletions(-) diff --git a/src/uu/mkfifo/src/mkfifo.rs b/src/uu/mkfifo/src/mkfifo.rs index 3586eb7c37c..d91f3e39d06 100644 --- a/src/uu/mkfifo/src/mkfifo.rs +++ b/src/uu/mkfifo/src/mkfifo.rs @@ -6,8 +6,6 @@ use clap::{Arg, ArgAction, Command, value_parser}; 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; @@ -39,21 +37,21 @@ 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()), )); } - // 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", target_os = "linux"))] { @@ -66,7 +64,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())); } } diff --git a/tests/by-util/test_mkfifo.rs b/tests/by-util/test_mkfifo.rs index ac0b78b3a3b..fa3582fcb04 100644 --- a/tests/by-util/test_mkfifo.rs +++ b/tests/by-util/test_mkfifo.rs @@ -137,19 +137,16 @@ fn test_create_fifo_permission_denied() { at.mkdir(no_exec_dir); at.set_mode(no_exec_dir, 0o644); - let err_msg = format!( - "mkfifo: cannot create fifo '{named_pipe}': File exists -mkfifo: cannot set permissions on '{named_pipe}': Permission denied (os error 13) -" - ); - + // 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"); } #[test] From c20327c22422a050568e1cb9dd4453a3197a3288 Mon Sep 17 00:00:00 2001 From: Sylvestre Ledru Date: Sun, 18 Jan 2026 14:00:50 +0100 Subject: [PATCH 4/5] mknod: make umask manipulation panic-safe with UmaskGuard --- src/uu/mknod/src/mknod.rs | 76 +++++++++++++++++++-------------------- 1 file changed, 37 insertions(+), 39 deletions(-) diff --git a/src/uu/mknod/src/mknod.rs b/src/uu/mknod/src/mknod.rs index 558717a8d24..62b77a7676f 100644 --- a/src/uu/mknod/src/mknod.rs +++ b/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] From 29061b6a920f99fb1c453dbb733217d4508ad4c9 Mon Sep 17 00:00:00 2001 From: Sylvestre Ledru Date: Mon, 19 Jan 2026 10:45:58 +0100 Subject: [PATCH 5/5] Update spell-checker ignore list in mode.rs --- src/uucore/src/lib/features/mode.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/uucore/src/lib/features/mode.rs b/src/uucore/src/lib/features/mode.rs index bfaa06df3f4..806614d4148 100644 --- a/src/uucore/src/lib/features/mode.rs +++ b/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;