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) } diff --git a/src/uu/mkfifo/src/mkfifo.rs b/src/uu/mkfifo/src/mkfifo.rs index 740e8cdb475..80cc60212e1 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; @@ -48,7 +46,15 @@ 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()), @@ -56,14 +62,6 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { 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 +74,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/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] diff --git a/src/uucore/src/lib/features/mode.rs b/src/uucore/src/lib/features/mode.rs index d562f1fe038..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; @@ -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); + } } diff --git a/tests/by-util/test_mkfifo.rs b/tests/by-util/test_mkfifo.rs index 4d19da636a2..aa14669a9a6 100644 --- a/tests/by-util/test_mkfifo.rs +++ b/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]