diff --git a/src/uu/mkdir/src/mkdir.rs b/src/uu/mkdir/src/mkdir.rs index 76262f4bdbc..cad8f5fb64a 100644 --- a/src/uu/mkdir/src/mkdir.rs +++ b/src/uu/mkdir/src/mkdir.rs @@ -3,14 +3,14 @@ // 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 +// spell-checker:ignore (ToDO) ugoa cmode RAII use clap::builder::ValueParser; use clap::parser::ValuesRef; use clap::{Arg, ArgAction, ArgMatches, Command}; use std::ffi::OsString; use std::path::{Path, PathBuf}; -#[cfg(not(windows))] +#[cfg(all(unix, target_os = "linux"))] use uucore::error::FromIo; use uucore::error::{UResult, USimpleError}; use uucore::translate; @@ -191,7 +191,8 @@ pub fn mkdir(path: &Path, config: &Config) -> UResult<()> { create_dir(path, false, config) } -#[cfg(any(unix, target_os = "redox"))] +/// Only needed on Linux to add ACL permission bits after directory creation. +#[cfg(all(unix, target_os = "linux"))] fn chmod(path: &Path, mode: u32) -> UResult<()> { use std::fs::{Permissions, set_permissions}; use std::os::unix::fs::PermissionsExt; @@ -201,12 +202,6 @@ fn chmod(path: &Path, mode: u32) -> UResult<()> { ) } -#[cfg(windows)] -fn chmod(_path: &Path, _mode: u32) -> UResult<()> { - // chmod on Windows only sets the readonly flag, which isn't even honored on directories - Ok(()) -} - // Create a directory at the given path. // Uses iterative approach instead of recursion to avoid stack overflow with deep nesting. fn create_dir(path: &Path, is_parent: bool, config: &Config) -> UResult<()> { @@ -250,13 +245,67 @@ 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 +/// directory is created atomically with the correct permissions. 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) -> std::io::Result<()> { + use std::os::unix::fs::DirBuilderExt; + + // 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); + + std::fs::DirBuilder::new().mode(mode).create(path) +} + +#[cfg(not(unix))] +fn create_dir_with_mode(path: &Path, _mode: u32) -> std::io::Result<()> { + std::fs::create_dir(path) +} + // Helper function to create a single directory with appropriate permissions // `is_parent` argument is not used on windows #[allow(unused_variables)] fn create_single_dir(path: &Path, is_parent: bool, config: &Config) -> UResult<()> { let path_exists = path.exists(); - match std::fs::create_dir(path) { + // Calculate the mode to use for directory creation + #[cfg(unix)] + let create_mode = if is_parent { + // For parent directories with -p, use umask-derived mode with u+wx + (!mode::get_umask() & 0o777) | 0o300 + } else { + config.mode + }; + #[cfg(not(unix))] + let create_mode = config.mode; + + match create_dir_with_mode(path, create_mode) { Ok(()) => { if config.verbose { println!( @@ -265,30 +314,17 @@ fn create_single_dir(path: &Path, is_parent: bool, config: &Config) -> UResult<( ); } + // On Linux, we may need to add ACL permission bits via chmod. + // On other Unix systems, the directory was already created with the correct mode. #[cfg(all(unix, target_os = "linux"))] - let new_mode = if path_exists { - config.mode - } else { + if !path_exists { // TODO: Make this macos and freebsd compatible by creating a function to get permission bits from // acl in extended attributes let acl_perm_bits = uucore::fsxattr::get_acl_perm_bits_from_xattr(path); - - if is_parent { - (!mode::get_umask() & 0o777) | 0o300 | acl_perm_bits - } else { - config.mode | acl_perm_bits + if acl_perm_bits != 0 { + chmod(path, create_mode | acl_perm_bits)?; } - }; - #[cfg(all(unix, not(target_os = "linux")))] - let new_mode = if is_parent { - (!mode::get_umask() & 0o777) | 0o300 - } else { - config.mode - }; - #[cfg(windows)] - let new_mode = config.mode; - - chmod(path, new_mode)?; + } // Apply SELinux context if requested #[cfg(feature = "selinux")] diff --git a/tests/by-util/test_mkdir.rs b/tests/by-util/test_mkdir.rs index 2ccfbb44aaa..5d68fadfd10 100644 --- a/tests/by-util/test_mkdir.rs +++ b/tests/by-util/test_mkdir.rs @@ -788,6 +788,126 @@ fn test_mkdir_environment_expansion() { } } +/// Test that mkdir -m creates directories with the exact requested mode, +/// bypassing umask. This verifies the fix for issue #10022. +/// +/// Previously, mkdir would create the directory with umask-based permissions +/// and then chmod afterward, leaving a brief window with wrong permissions. +/// Now it temporarily sets umask to 0 and creates with the exact mode. +#[cfg(not(windows))] +#[test] +fn test_mkdir_mode_ignores_umask() { + // Test that -m 0700 with restrictive umask still creates 0700 + { + let (at, mut ucmd) = at_and_ucmd!(); + let restrictive_umask: mode_t = 0o077; // Would normally block group/other + + ucmd.arg("-m") + .arg("0700") + .arg("test_700") + .umask(restrictive_umask) + .succeeds(); + + let perms = at.metadata("test_700").permissions().mode() as mode_t; + assert_eq!(perms, 0o40700, "Expected 0700, got {:o}", perms & 0o777); + } + + // Test that -m 0777 is honored even with umask 022 + // This is the key test: without the fix, 0777 & ~022 = 0755 + { + let (at, mut ucmd) = at_and_ucmd!(); + let common_umask: mode_t = 0o022; + + ucmd.arg("-m") + .arg("0777") + .arg("test_777") + .umask(common_umask) + .succeeds(); + + let perms = at.metadata("test_777").permissions().mode() as mode_t; + assert_eq!( + perms, + 0o40777, + "Expected 0777 (umask should be ignored with -m), got {:o}", + perms & 0o777 + ); + } + + // Test that -m 0755 with umask 077 still creates 0755 + { + let (at, mut ucmd) = at_and_ucmd!(); + let very_restrictive_umask: mode_t = 0o077; + + ucmd.arg("-m") + .arg("0755") + .arg("test_755") + .umask(very_restrictive_umask) + .succeeds(); + + let perms = at.metadata("test_755").permissions().mode() as mode_t; + assert_eq!(perms, 0o40755, "Expected 0755, got {:o}", perms & 0o777); + } + + // Test symbolic mode also ignores umask + { + let (at, mut ucmd) = at_and_ucmd!(); + let umask: mode_t = 0o022; + + ucmd.arg("-m") + .arg("a=rwx") + .arg("test_symbolic") + .umask(umask) + .succeeds(); + + let perms = at.metadata("test_symbolic").permissions().mode() as mode_t; + assert_eq!(perms, 0o40777, "Expected 0777, got {:o}", perms & 0o777); + } +} + +/// Test that mkdir -p -m applies mode correctly: +/// - Parent directories use umask-derived permissions (with u+wx) +/// - Final directory uses the exact requested mode (ignoring umask) +#[cfg(not(windows))] +#[test] +fn test_mkdir_parent_mode_with_explicit_mode() { + let (at, mut ucmd) = at_and_ucmd!(); + let umask: mode_t = 0o022; + + ucmd.arg("-p") + .arg("-m") + .arg("0700") + .arg("parent/child/target") + .umask(umask) + .succeeds(); + + // Parent directories created by -p use umask-derived mode with u+wx + let parent_perms = at.metadata("parent").permissions().mode() as mode_t; + let expected_parent = ((!umask & 0o777) | 0o300) + 0o40000; + assert_eq!( + parent_perms, + expected_parent, + "Parent should have umask-derived mode, got {:o}", + parent_perms & 0o777 + ); + + let child_perms = at.metadata("parent/child").permissions().mode() as mode_t; + assert_eq!( + child_perms, + expected_parent, + "Intermediate dir should have umask-derived mode, got {:o}", + child_perms & 0o777 + ); + + // Final directory should have exactly the requested mode + let target_perms = at.metadata("parent/child/target").permissions().mode() as mode_t; + assert_eq!( + target_perms, + 0o40700, + "Target should have exact requested mode 0700, got {:o}", + target_perms & 0o777 + ); +} + #[test] fn test_mkdir_concurrent_creation() { // Test concurrent mkdir -p operations: 10 iterations, 8 threads, 40 levels nesting