From 6635595ba7f706650917bf91a1fcd6b64dad5844 Mon Sep 17 00:00:00 2001 From: Jake Abendroth Date: Tue, 6 Oct 2026 16:55:10 -0700 Subject: [PATCH 1/4] uucore: add mode::with_umask Utilities that create files or directories at an exact mode, whatever the caller's umask, change the process umask around the call. Done by hand, overlapping calls can restore each other's mask and leave the process at the wrong value, and an unwind between the set and the restore leaks the temporary mask. with_umask runs an operation with a temporary umask, holds a mutex shared with get_umask, and restores the previous mask on return or unwind. Calls nest: the thread that holds the mutex skips it, and get_umask there returns the mask in effect. Only Unix sets the umask this way, so Windows' get_umask takes no lock. --- src/uucore/src/lib/features/mode.rs | 90 +++++++++++++++++++++++++++++ 1 file changed, 90 insertions(+) diff --git a/src/uucore/src/lib/features/mode.rs b/src/uucore/src/lib/features/mode.rs index 05867346331..b6ce07aa9d5 100644 --- a/src/uucore/src/lib/features/mode.rs +++ b/src/uucore/src/lib/features/mode.rs @@ -397,6 +397,23 @@ pub fn parse(mode_string: &str, considering_dir: bool, umask: u32) -> Result = std::sync::Mutex::new(()); + +#[cfg(unix)] +thread_local! { + /// The umask set by a [`with_umask`] running on this thread, which holds + /// `UMASK_LOCK`. + static HELD_UMASK: std::cell::Cell> = const { std::cell::Cell::new(None) }; +} + +#[cfg(unix)] +fn lock_umask() -> std::sync::MutexGuard<'static, ()> { + UMASK_LOCK + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner) +} + pub fn get_umask() -> u32 { // There's no portable way to read the umask without changing it. // We have to replace it and then quickly set it back, hopefully before @@ -407,6 +424,10 @@ pub fn get_umask() -> u32 { { use rustix::fs::Mode; use rustix::process::umask; + if let Some(mask) = HELD_UMASK.get() { + return mask; + } + let _lock = lock_umask(); let mask = umask(Mode::empty()); let _ = umask(mask); @@ -429,6 +450,47 @@ pub fn get_umask() -> u32 { } } +#[cfg(unix)] +struct UmaskGuard { + previous: rustix::fs::Mode, + held: Option, +} + +#[cfg(unix)] +impl UmaskGuard { + fn set(mask: u32) -> Self { + // `rustix::fs::RawMode` is u16 on some targets and u32 on others. + let mode = rustix::fs::Mode::from_bits_truncate(mask as rustix::fs::RawMode); + Self { + previous: rustix::process::umask(mode), + held: HELD_UMASK.replace(Some(mask & 0o777)), + } + } +} + +#[cfg(unix)] +impl Drop for UmaskGuard { + fn drop(&mut self) { + rustix::process::umask(self.previous); + HELD_UMASK.set(self.held); + } +} + +/// Run an operation with a temporary process umask. +/// +/// The previous umask is restored when the operation returns or unwinds. +/// Calls through this module are serialized because the umask is process-wide. +/// They nest: inside `operation`, [`get_umask`] returns `mask`, and `with_umask` +/// can be called again. Other threads wait until `operation` returns, so it +/// must not wait for one of them. +#[cfg(unix)] +pub fn with_umask(mask: u32, operation: impl FnOnce() -> T) -> T { + // Only the outermost call on this thread takes the lock. + let _lock = HELD_UMASK.get().is_none().then(lock_umask); + let _guard = UmaskGuard::set(mask); + operation() +} + #[cfg(test)] mod tests { @@ -568,4 +630,32 @@ mod tests { // First add user write, then set to 755 (should override) assert_eq!(parse("u+w,755", false, 0).unwrap(), 0o755); } + + /// Reads the umask without going through the helpers under test. + #[cfg(unix)] + fn raw_umask() -> u32 { + let mask = rustix::process::umask(rustix::fs::Mode::empty()); + rustix::process::umask(mask); + mask.bits() as u32 + } + + #[cfg(unix)] + #[test] + fn test_with_umask_sets_and_restores_mask() { + let before = super::get_umask(); + assert_eq!(super::with_umask(0o027, raw_umask), 0o027); + assert_eq!(super::get_umask(), before); + } + + #[cfg(unix)] + #[test] + fn test_with_umask_nests() { + let before = super::get_umask(); + super::with_umask(0o027, || { + assert_eq!(super::get_umask(), 0o027); + assert_eq!(super::with_umask(0o077, raw_umask), 0o077); + assert_eq!(raw_umask(), 0o027); + }); + assert_eq!(super::get_umask(), before); + } } From 782ad282b64926d2767e4dd8bd86f4dfbd11afd9 Mon Sep 17 00:00:00 2001 From: Jake Abendroth Date: Tue, 6 Oct 2026 16:55:52 -0700 Subject: [PATCH 2/4] mkdir: set the umask through mode::with_umask Replace mkdir's own umask guard with the shared helper, so its change is serialized with get_umask and other callers. The shaped umask is computed as a plain mask, which drops mkdir's rustix dependency. --- Cargo.lock | 1 - src/uu/mkdir/Cargo.toml | 3 --- src/uu/mkdir/src/mkdir.rs | 43 ++++++++------------------------------- 3 files changed, 9 insertions(+), 38 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 19f1ac4e3fd..a512ae59d38 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3671,7 +3671,6 @@ version = "0.13.0" dependencies = [ "clap", "fluent", - "rustix", "uucore", ] diff --git a/src/uu/mkdir/Cargo.toml b/src/uu/mkdir/Cargo.toml index 7af96649850..37b4ef66b63 100644 --- a/src/uu/mkdir/Cargo.toml +++ b/src/uu/mkdir/Cargo.toml @@ -26,9 +26,6 @@ uucore = { workspace = true, features = [ "quoting-style", ] } -[target.'cfg(unix)'.dependencies] -rustix = { workspace = true, features = ["process", "fs"] } - [features] diagnostics = ["uucore/diagnostics"] selinux = ["uucore/selinux"] diff --git a/src/uu/mkdir/src/mkdir.rs b/src/uu/mkdir/src/mkdir.rs index ef3b59c5892..a9e372dde13 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; @@ -251,26 +251,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(rustix::fs::Mode); - -#[cfg(unix)] -impl UmaskGuard { - /// Set umask to the given value and return a guard that restores the original on drop. - fn set(new_mask: rustix::fs::Mode) -> Self { - let old_mask = rustix::process::umask(new_mask); - Self(old_mask) - } -} - -#[cfg(unix)] -impl Drop for UmaskGuard { - fn drop(&mut self) { - rustix::process::umask(self.0); - } -} - /// Create a directory with the exact mode specified, bypassing umask. /// /// GNU mkdir temporarily sets umask to a shaped umask before calling mkdir(2), @@ -278,16 +258,12 @@ impl Drop for UmaskGuard { /// This avoids a race condition where the directory briefly exists with /// umask-based permissions. #[cfg(unix)] -fn create_dir_with_mode( - path: &Path, - mode: u32, - shaped_umask: rustix::fs::Mode, -) -> std::io::Result<()> { +fn create_dir_with_mode(path: &Path, mode: u32, shaped_umask: u32) -> std::io::Result<()> { use std::os::unix::fs::DirBuilderExt; - let _guard = UmaskGuard::set(shaped_umask); - - std::fs::DirBuilder::new().mode(mode).create(path) + mode::with_umask(shaped_umask, || { + std::fs::DirBuilder::new().mode(mode).create(path) + }) } #[cfg(not(unix))] @@ -301,8 +277,7 @@ fn create_dir_with_mode(path: &Path, _mode: u32, _shaped_umask: u32) -> std::io: fn create_single_dir(path: &Path, is_parent: bool, config: &Config) -> UResult<()> { #[cfg(unix)] let (mkdir_mode, shaped_umask) = { - let mode_bits = |x: u32| rustix::fs::Mode::from_bits_truncate(x as rustix::fs::RawMode); - let umask_bits = mode_bits(mode::get_umask()); + let umask = mode::get_umask(); if is_parent { // Parent directories are never affected by -m (matches GNU behavior). // We pass 0o777 as the mode and shape the umask so it cannot block @@ -310,13 +285,13 @@ fn create_single_dir(path: &Path, is_parent: bool, config: &Config) -> UResult<( // write into the parent to create children. All other umask bits are // preserved so the kernel applies them — and any default ACL on the // grandparent — through the normal mkdir(2) path. - (DEFAULT_PERM, umask_bits & !mode_bits(0o300)) + (DEFAULT_PERM, umask & !0o300) } else { match config.mode { // Explicit -m: shape umask so it cannot block explicitly requested bits. - Some(m) => (m, umask_bits & !mode_bits(m)), + Some(m) => (m, umask & !m), // No -m: leave umask fully intact; kernel applies umask + ACL naturally. - None => (DEFAULT_PERM, umask_bits), + None => (DEFAULT_PERM, umask), } } }; From b434a5c7c2fd67093b80a54b1bc1c4999579e99b Mon Sep 17 00:00:00 2001 From: Jake Abendroth Date: Tue, 6 Oct 2026 16:56:03 -0700 Subject: [PATCH 3/4] sort: pin the tmp_dir test's umask with mode::with_umask The test set and restored the umask with its own guard, outside the lock that get_umask and with_umask share. --- src/uu/sort/Cargo.toml | 1 + src/uu/sort/src/tmp_dir.rs | 39 +++++++++++--------------------------- 2 files changed, 12 insertions(+), 28 deletions(-) diff --git a/src/uu/sort/Cargo.toml b/src/uu/sort/Cargo.toml index 8a9f447e18f..73feba43615 100644 --- a/src/uu/sort/Cargo.toml +++ b/src/uu/sort/Cargo.toml @@ -51,6 +51,7 @@ tempfile = { workspace = true } uucore = { workspace = true, features = [ "benchmark", "fs", + "mode", "parser-size", "version-cmp", "i18n-collator", diff --git a/src/uu/sort/src/tmp_dir.rs b/src/uu/sort/src/tmp_dir.rs index ee92d18a5c1..98197845651 100644 --- a/src/uu/sort/src/tmp_dir.rs +++ b/src/uu/sort/src/tmp_dir.rs @@ -221,36 +221,19 @@ mod tests { std::fs::metadata(path).unwrap().permissions().mode() & 0o777 } - /// Restores the process umask on drop, so a panic in the test cannot leak the - /// value into the rest of the binary. - struct UmaskGuard(libc::mode_t); - - impl UmaskGuard { - fn set(mask: libc::mode_t) -> Self { - // SAFETY: umask(2) has no failure mode; it returns the previous value. - Self(unsafe { libc::umask(mask) }) - } - } - - impl Drop for UmaskGuard { - fn drop(&mut self) { - unsafe { libc::umask(self.0) }; - } - } - #[test] fn tmp_files_are_private_regardless_of_umask() { // Pin a permissive umask: under 0077 the umask alone would produce 0700 and - // 0600, so the assertions would hold for a broken implementation too. The - // guard restores it, and the only other test here that creates files sets - // the modes it cares about explicitly. - let _umask = UmaskGuard::set(0o022); - - let parent = tempfile::tempdir().unwrap(); - let mut wrapper = TmpDirWrapper::new(parent.path().to_owned()); - let (_file, path) = wrapper.next_file().unwrap(); - - assert_eq!(mode(path.parent().unwrap()), 0o700); - assert_eq!(mode(&path), 0o600); + // 0600, so the assertions would hold for a broken implementation too. + // `with_umask` restores it, also on a panic, and the only other test here + // that creates files sets the modes it cares about explicitly. + uucore::mode::with_umask(0o022, || { + let parent = tempfile::tempdir().unwrap(); + let mut wrapper = TmpDirWrapper::new(parent.path().to_owned()); + let (_file, path) = wrapper.next_file().unwrap(); + + assert_eq!(mode(path.parent().unwrap()), 0o700); + assert_eq!(mode(&path), 0o600); + }); } } From 484803e54f036af49ff86e4a38946dff5b18cece Mon Sep 17 00:00:00 2001 From: Jake Abendroth Date: Fri, 9 Oct 2026 19:44:57 -0700 Subject: [PATCH 4/4] mkfifo, mknod: set the umask through mode::with_umask Both changed the process-wide umask directly around a single syscall. Overlapping calls can restore each other's mask and leave the process at the wrong value, and a panic between the set and the restore leaks the temporary mask into the rest of the run. Use uucore::mode::with_umask, which serializes the change with get_umask and restores the mask on unwind. No behavior change: modes for mkfifo -m and plain mkfifo match GNU 9.12 at umasks 022, 077 and 002. test_mknod.rs now pins the umask with and without -m, as test_mkfifo.rs does. Narrow rustix to the fs feature in uu_mkfifo, which no longer needs process. --- src/uu/mkfifo/Cargo.toml | 2 +- src/uu/mkfifo/src/mkfifo.rs | 10 ++++----- src/uu/mknod/src/mknod.rs | 31 +++++++++++--------------- tests/by-util/test_mknod.rs | 43 +++++++++++++++++++++++++++++++++++++ 4 files changed, 61 insertions(+), 25 deletions(-) diff --git a/src/uu/mkfifo/Cargo.toml b/src/uu/mkfifo/Cargo.toml index 9957defd58f..97b3429871d 100644 --- a/src/uu/mkfifo/Cargo.toml +++ b/src/uu/mkfifo/Cargo.toml @@ -22,7 +22,7 @@ fluent = { workspace = true } uucore = { workspace = true, features = ["fs", "mode"] } [target.'cfg(unix)'.dependencies] -rustix = { workspace = true, features = ["fs", "process"] } +rustix = { workspace = true, features = ["fs"] } [target.'cfg(target_vendor = "apple")'.dependencies] libc = { workspace = true } diff --git a/src/uu/mkfifo/src/mkfifo.rs b/src/uu/mkfifo/src/mkfifo.rs index 0a94ea8a8b1..5656a578469 100644 --- a/src/uu/mkfifo/src/mkfifo.rs +++ b/src/uu/mkfifo/src/mkfifo.rs @@ -4,8 +4,7 @@ // file that was distributed with this source code. use clap::{Arg, ArgAction, Command, value_parser}; -use rustix::fs::{Mode, RawMode}; -use rustix::process::umask; +use rustix::fs::RawMode; use std::ffi::OsString; use uucore::display::Quotable; use uucore::error::{ExitCode, UResult, USimpleError, strip_errno}; @@ -81,9 +80,8 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { // that used to follow this call closes the TOCTOU window an // attacker could use to swap the FIFO for a symlink between // mkfifo and chmod (issue #10020). - let prev_umask = umask(Mode::empty()); - let mkfifo_result = create_fifo(f.as_str(), mode as RawMode); - umask(prev_umask); + let mkfifo_result = + uucore::mode::with_umask(0, || create_fifo(f.as_str(), mode as RawMode)); if let Err(e) = mkfifo_result { show!(USimpleError::new( @@ -156,7 +154,7 @@ pub fn uu_app() -> Command { #[cfg(not(target_vendor = "apple"))] fn create_fifo(path: &str, mode: RawMode) -> std::io::Result<()> { use rustix::fs; - fs::mkfifoat(fs::CWD, path, Mode::from_bits_truncate(mode)).map_err(Into::into) + fs::mkfifoat(fs::CWD, path, fs::Mode::from_bits_truncate(mode)).map_err(Into::into) } #[cfg(target_vendor = "apple")] diff --git a/src/uu/mknod/src/mknod.rs b/src/uu/mknod/src/mknod.rs index 2a91f98ba0d..547a0b7e6f4 100644 --- a/src/uu/mknod/src/mknod.rs +++ b/src/uu/mknod/src/mknod.rs @@ -7,7 +7,7 @@ use clap::{Arg, ArgAction, Command, value_parser}; use nix::libc::{S_IRGRP, S_IROTH, S_IRUSR, S_IWGRP, S_IWOTH, S_IWUSR, mode_t}; -use nix::sys::stat::{Mode, SFlag, dev_t, mknod as nix_mknod, umask as nix_umask}; +use nix::sys::stat::{Mode, SFlag, dev_t, mknod as nix_mknod}; use std::ffi::OsString; use std::io::{self, Write as _}; @@ -93,27 +93,22 @@ fn mknod(file_name: &str, config: Config) -> i32 { None }; - // set umask to 0 and store previous umask - let have_prev_umask = if config.use_umask { - None + let create_node = || { + nix_mknod( + file_name, + config.file_type.as_sflag(), + config.mode, + config.dev, + ) + .err() + }; + let mknod_err = if config.use_umask { + create_node() } else { - Some(nix_umask(Mode::empty())) + uucore::mode::with_umask(0, create_node) }; - - let mknod_err = nix_mknod( - file_name, - config.file_type.as_sflag(), - config.mode, - config.dev, - ) - .err(); let errno = if mknod_err.is_some() { -1 } else { 0 }; - // set umask back to original value - if let Some(prev_umask) = have_prev_umask { - nix_umask(prev_umask); - } - if let Some(err) = mknod_err { let _ = writeln!( io::stderr(), diff --git a/tests/by-util/test_mknod.rs b/tests/by-util/test_mknod.rs index 49fdbc243f9..1fae6d1e092 100644 --- a/tests/by-util/test_mknod.rs +++ b/tests/by-util/test_mknod.rs @@ -183,6 +183,49 @@ fn test_mknod_mode_comma_separated() { ); } +#[test] +fn test_mknod_fifo_with_mode_and_umask() { + use uucore::fs::display_permissions; + let ts = TestScenario::new(util_name!()); + + let test_fifo_creation = |mode: &str, umask: u16, expected: &str| { + ts.ucmd() + .args(&["-m", mode, "fifo_test", "p"]) + .umask(libc::mode_t::from(umask)) + .succeeds(); + + let metadata = ts.fixtures.metadata("fifo_test"); + assert_eq!(display_permissions(&metadata, true), expected); + ts.fixtures.remove("fifo_test"); + }; + + test_fifo_creation("734", 0o077, "prwx-wxr--"); // spell-checker:disable-line + test_fifo_creation("706", 0o777, "prwx---rw-"); // spell-checker:disable-line + test_fifo_creation("a=r", 0o022, "pr--r--r--"); // spell-checker:disable-line + test_fifo_creation("u+x", 0o022, "prwxrw-rw-"); // spell-checker:disable-line +} + +#[test] +fn test_mknod_fifo_with_umask() { + use uucore::fs::display_permissions; + let ts = TestScenario::new(util_name!()); + + let test_fifo_creation = |umask: u16, expected: &str| { + ts.ucmd() + .args(&["fifo_test", "p"]) + .umask(libc::mode_t::from(umask)) + .succeeds(); + + let metadata = ts.fixtures.metadata("fifo_test"); + assert_eq!(display_permissions(&metadata, true), expected); + ts.fixtures.remove("fifo_test"); + }; + + test_fifo_creation(0o022, "prw-r--r--"); // spell-checker:disable-line + test_fifo_creation(0o077, "prw-------"); // spell-checker:disable-line + test_fifo_creation(0o777, "p---------"); // spell-checker:disable-line +} + #[test] #[cfg(all(feature = "selinux", any(target_os = "linux", target_os = "android")))] fn test_mknod_selinux() {