diff --git a/Cargo.lock b/Cargo.lock index 0acf3c032b..b3142a6431 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/install/src/install.rs b/src/uu/install/src/install.rs index a42d805f04..cdf6ae3394 100644 --- a/src/uu/install/src/install.rs +++ b/src/uu/install/src/install.rs @@ -37,7 +37,7 @@ use uucore::translate; use uucore::{format_usage, show, show_error, show_if_err}; #[cfg(unix)] -use std::os::unix::fs::MetadataExt; +use std::os::unix::fs::{DirBuilderExt, MetadataExt}; #[cfg(unix)] use std::os::unix::prelude::OsStrExt; @@ -205,10 +205,12 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { let behavior = behavior(&matches, diag_args.as_deref())?; - match behavior.main_function { + // Like GNU, work with the umask cleared: new ancestor directories get + // exactly DEFAULT_MODE and the strip program sees a umask of 0. + uucore::mode::with_umask(0, || match behavior.main_function { MainFunction::Directory => directory(&paths, &behavior), MainFunction::Standard => standard(paths, &behavior), - } + }) } pub fn uu_app() -> Command { @@ -506,11 +508,11 @@ fn directory(paths: &[OsString], b: &Behavior) -> UResult<()> { // create all ancestors (or components) of a directory // regardless of the presence of the "-D" flag. // - // NOTE: the GNU "install" sets the expected mode only for the - // target directory. All created ancestor directories will have - // the default mode. Hence it is safe to use fs::create_dir_all - // and then only modify the target's dir mode. - if let Err(e) = fs::create_dir_all(&path_to_create).map_err_context( + // Only the target gets the requested mode; newly created ancestors + // use DEFAULT_MODE, which create_dir_all would request as 0777. + let mut builder = fs::DirBuilder::new(); + builder.recursive(true).mode(DEFAULT_MODE); + if let Err(e) = builder.create(&path_to_create).map_err_context( || translate!("install-error-create-dir-failed", "path" => path_to_create.quote()), ) { show!(e); @@ -713,8 +715,6 @@ fn standard(mut paths: Vec, b: &Behavior) -> UResult<()> { #[cfg(unix)] { - // Use DEFAULT_MODE (0o755) for created directories - this matches GNU install - // behavior. The actual mode will be modified by umask at the kernel level. match create_dir_all_safe(to_create, DEFAULT_MODE) { Ok(dir_fd) => { if b.target_dir.is_none() diff --git a/src/uu/mkdir/Cargo.toml b/src/uu/mkdir/Cargo.toml index f1fdc62e07..917270a009 100644 --- a/src/uu/mkdir/Cargo.toml +++ b/src/uu/mkdir/Cargo.toml @@ -21,9 +21,6 @@ clap = { workspace = true } fluent = { workspace = true } uucore = { workspace = true, features = ["fs", "fsxattr", "mode"] } -[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 3553f7e39b..9775ffd654 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,26 +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(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), @@ -272,16 +252,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))] @@ -295,8 +271,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 @@ -304,13 +279,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/sort/Cargo.toml b/src/uu/sort/Cargo.toml index 8a9f447e18..73feba4361 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 ee92d18a5c..9819784565 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); + }); } } diff --git a/src/uucore/src/lib/features/mode.rs b/src/uucore/src/lib/features/mode.rs index 77aef656d1..e2ff2ab390 100644 --- a/src/uucore/src/lib/features/mode.rs +++ b/src/uucore/src/lib/features/mode.rs @@ -355,6 +355,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 @@ -365,6 +382,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); @@ -387,6 +408,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 { @@ -526,4 +588,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_install.rs b/tests/by-util/test_install.rs index 3a5189d57c..8a1f663bd2 100644 --- a/tests/by-util/test_install.rs +++ b/tests/by-util/test_install.rs @@ -112,12 +112,9 @@ fn test_install_ancestors_mode_directories() { let target_dir = "ancestor1/ancestor2/target_dir"; let directories_arg = "-d"; let mode_arg = "--mode=200"; - let probe = "probe"; - at.mkdir(probe); - let default_perms = at.metadata(probe).permissions().mode(); - - ucmd.args(&[mode_arg, directories_arg, target_dir]) + ucmd.umask(0o077) + .args(&[mode_arg, directories_arg, target_dir]) .succeeds() .no_stderr(); @@ -125,11 +122,10 @@ fn test_install_ancestors_mode_directories() { assert!(at.dir_exists(ancestor2)); assert!(at.dir_exists(target_dir)); - assert_eq!(default_perms, at.metadata(ancestor1).permissions().mode()); - assert_eq!(default_perms, at.metadata(ancestor2).permissions().mode()); - - // Expected mode only on the target_dir. - assert_eq!(0o40_200_u32, at.metadata(target_dir).permissions().mode()); + // Ancestors use the default mode; --mode applies only to the target. + assert_eq!(0o755, at.metadata(ancestor1).permissions().mode() & 0o777); + assert_eq!(0o755, at.metadata(ancestor2).permissions().mode() & 0o777); + assert_eq!(0o200, at.metadata(target_dir).permissions().mode() & 0o777); } #[test] @@ -141,14 +137,11 @@ fn test_install_ancestors_mode_directories_with_file() { let directories_arg = "-D"; let mode_arg = "--mode=200"; let file = "file"; - let probe = "probe"; - - at.mkdir(probe); - let default_perms = at.metadata(probe).permissions().mode(); at.touch(file); - ucmd.args(&[mode_arg, directories_arg, file, target_file]) + ucmd.umask(0o077) + .args(&[mode_arg, directories_arg, file, target_file]) .succeeds() .no_stderr(); @@ -156,11 +149,10 @@ fn test_install_ancestors_mode_directories_with_file() { assert!(at.dir_exists(ancestor2)); assert!(at.file_exists(target_file)); - assert_eq!(default_perms, at.metadata(ancestor1).permissions().mode()); - assert_eq!(default_perms, at.metadata(ancestor2).permissions().mode()); - - // Expected mode only on the target_file. - assert_eq!(0o100_200_u32, at.metadata(target_file).permissions().mode()); + // Ancestors use the default mode; --mode applies only to the target. + assert_eq!(0o755, at.metadata(ancestor1).permissions().mode() & 0o777); + assert_eq!(0o755, at.metadata(ancestor2).permissions().mode() & 0o777); + assert_eq!(0o200, at.metadata(target_file).permissions().mode() & 0o777); } #[test] @@ -845,6 +837,7 @@ fn test_install_and_strip() { scene .ucmd() + .umask(0o777) .env("PATH", path) .arg("-s") .arg("source") @@ -855,6 +848,69 @@ fn test_install_and_strip() { assert_eq!(at.metadata(STRIP_TARGET_FILE).len(), 0); } +#[test] +fn test_install_strip_program_sees_zeroed_umask() { + let scene = TestScenario::new(util_name!()); + let at = &scene.fixtures; + + // Write in a child to avoid inherited writable script fds (see write_strip_program). + scene + .cmd("sh") + .env("STRIP_SCRIPT", "#!/bin/sh\numask > \"$1\"\n") + .arg("-c") + .arg("printf '%s' \"$STRIP_SCRIPT\" > probe && chmod 755 probe") + .succeeds(); + at.write("source", "file contents"); + + scene + .ucmd() + .umask(0o077) + .args(&[ + "-s", + "--strip-program", + "./probe", + "source", + STRIP_TARGET_FILE, + ]) + .succeeds() + .no_stderr(); + + // Follow GNU's behavior: the strip program sees a zeroed umask. Shells + // differ on whether they print it as 000 or 0000, so compare the value. + let reported = at.read(STRIP_TARGET_FILE); + let reported = reported.trim(); + assert_eq!( + u32::from_str_radix(reported, 8).unwrap(), + 0, + "strip program saw umask {reported}" + ); +} + +#[test] +fn test_install_and_strip_with_ancestor_directories() { + let scene = TestScenario::new(util_name!()); + let at = &scene.fixtures; + + write_strip_program(&scene, "strip"); + at.write("source", "file contents"); + let path = format!( + "{}:{}", + at.plus(".").display(), + std::env::var("PATH").unwrap_or_default() + ); + let target = "ancestor/helloworld_installed"; + + scene + .ucmd() + .umask(0o777) + .env("PATH", path) + .args(&["-s", "-D", "source", target]) + .succeeds() + .no_stderr(); + + assert_eq!(at.metadata(target).len(), 0); +} + #[test] fn test_install_no_strip_with_program() { let source = current_exe().unwrap();