diff --git a/Cargo.lock b/Cargo.lock index f6c59ca9a87..62f4e9e4d63 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), } } }; 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/src/uu/sort/Cargo.toml b/src/uu/sort/Cargo.toml index 2e4feaf77de..d38f9efaf28 100644 --- a/src/uu/sort/Cargo.toml +++ b/src/uu/sort/Cargo.toml @@ -52,6 +52,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 d869d0fbe7e..5638b4261b0 100644 --- a/src/uu/sort/src/tmp_dir.rs +++ b/src/uu/sort/src/tmp_dir.rs @@ -397,37 +397,20 @@ 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 other tests here that create files don't - // check modes. - 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.path().parent().unwrap()), 0o700); - assert_eq!(mode(path.path()), 0o600); + // 0600, so the assertions would hold for a broken implementation too. + // `with_umask` restores it, also on a panic, and the other tests here that + // create files don't check modes. + 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.path().parent().unwrap()), 0o700); + assert_eq!(mode(path.path()), 0o600); + }); } /// Once the directory's path leads to another directory, no file is created, 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); + } } 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() {