From 0aeeec1711460b626e9d567099e7bf8cc56bccea Mon Sep 17 00:00:00 2001 From: Ruiyang Wang Date: Sat, 3 Jan 2026 12:51:04 -0800 Subject: [PATCH 1/4] mkdir: create directories atomically with correct permissions Fix #10022: mkdir -m MODE was creating directories with umask-based permissions first, then calling chmod afterward. This left a brief window where the directory existed with wrong permissions. Now we match GNU mkdir behavior by temporarily setting umask to 0 before the mkdir syscall, passing the exact requested mode to the kernel, then restoring the original umask. The directory is created atomically with the correct permissions. Before (two syscalls, race condition): mkdir("dir", 0777) -> created with 0755 (umask applied) chmod("dir", 0700) -> fixed afterward After (single syscall, atomic): umask(0) mkdir("dir", 0700) -> created with exact mode umask(original) Also added tests to verify -m MODE bypasses umask correctly. --- src/uu/mkdir/src/mkdir.rs | 66 +++++++++++++------- tests/by-util/test_mkdir.rs | 116 ++++++++++++++++++++++++++++++++++++ 2 files changed, 162 insertions(+), 20 deletions(-) diff --git a/src/uu/mkdir/src/mkdir.rs b/src/uu/mkdir/src/mkdir.rs index 76262f4bdbc..1dbb83e7cfd 100644 --- a/src/uu/mkdir/src/mkdir.rs +++ b/src/uu/mkdir/src/mkdir.rs @@ -250,13 +250,52 @@ fn create_dir(path: &Path, is_parent: bool, config: &Config) -> UResult<()> { create_single_dir(path, is_parent, config) } +/// 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; + use uucore::libc; + + // Save current umask and temporarily set to 0 + let old_umask = unsafe { libc::umask(0) }; + + let result = std::fs::DirBuilder::new().mode(mode).create(path); + + // Restore original umask immediately + unsafe { + libc::umask(old_umask); + } + + result +} + +#[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 +304,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..d7c7d80427b 100644 --- a/tests/by-util/test_mkdir.rs +++ b/tests/by-util/test_mkdir.rs @@ -788,6 +788,122 @@ 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 From 3a3777f029384631e891b65af48d269231f5da72 Mon Sep 17 00:00:00 2001 From: Ruiyang Wang Date: Sun, 4 Jan 2026 10:20:11 -0800 Subject: [PATCH 2/4] mkdir: add UmaskGuard for panic-safe umask restoration Add RAII guard to ensure umask is restored even on panic. Encapsulate unsafe umask calls in UmaskGuard::set() for a safe interface. --- src/uu/mkdir/src/mkdir.rs | 37 ++++++++++++++++++++++++++----------- tests/by-util/test_mkdir.rs | 12 ++++++++---- 2 files changed, 34 insertions(+), 15 deletions(-) diff --git a/src/uu/mkdir/src/mkdir.rs b/src/uu/mkdir/src/mkdir.rs index 1dbb83e7cfd..0d84bbb69a2 100644 --- a/src/uu/mkdir/src/mkdir.rs +++ b/src/uu/mkdir/src/mkdir.rs @@ -250,6 +250,28 @@ 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 @@ -258,19 +280,12 @@ fn create_dir(path: &Path, is_parent: bool, config: &Config) -> UResult<()> { #[cfg(unix)] fn create_dir_with_mode(path: &Path, mode: u32) -> std::io::Result<()> { use std::os::unix::fs::DirBuilderExt; - use uucore::libc; - // Save current umask and temporarily set to 0 - let old_umask = unsafe { libc::umask(0) }; - - let result = std::fs::DirBuilder::new().mode(mode).create(path); - - // Restore original umask immediately - unsafe { - libc::umask(old_umask); - } + // 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); - result + std::fs::DirBuilder::new().mode(mode).create(path) } #[cfg(not(unix))] diff --git a/tests/by-util/test_mkdir.rs b/tests/by-util/test_mkdir.rs index d7c7d80427b..5d68fadfd10 100644 --- a/tests/by-util/test_mkdir.rs +++ b/tests/by-util/test_mkdir.rs @@ -826,7 +826,8 @@ fn test_mkdir_mode_ignores_umask() { let perms = at.metadata("test_777").permissions().mode() as mode_t; assert_eq!( - perms, 0o40777, + perms, + 0o40777, "Expected 0777 (umask should be ignored with -m), got {:o}", perms & 0o777 ); @@ -883,14 +884,16 @@ fn test_mkdir_parent_mode_with_explicit_mode() { 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_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, + child_perms, + expected_parent, "Intermediate dir should have umask-derived mode, got {:o}", child_perms & 0o777 ); @@ -898,7 +901,8 @@ fn test_mkdir_parent_mode_with_explicit_mode() { // 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_perms, + 0o40700, "Target should have exact requested mode 0700, got {:o}", target_perms & 0o777 ); From a7ed54685447b631ab91e6e1abc00428d270f81e Mon Sep 17 00:00:00 2001 From: Ruiyang Wang Date: Sun, 4 Jan 2026 10:29:58 -0800 Subject: [PATCH 3/4] mkdir: fix clippy and spelling warnings - Add RAII to spell-checker ignore list - Narrow chmod cfg to Linux only (only used for ACL bits) --- src/uu/mkdir/src/mkdir.rs | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/src/uu/mkdir/src/mkdir.rs b/src/uu/mkdir/src/mkdir.rs index 0d84bbb69a2..e75b1a05c06 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 +// spell-checker:ignore (ToDO) ugoa cmode RAII use clap::builder::ValueParser; use clap::parser::ValuesRef; @@ -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<()> { From 55724672dca1e3fd06eda392b86459e5e8d8c6b6 Mon Sep 17 00:00:00 2001 From: Ruiyang Wang Date: Sun, 4 Jan 2026 10:59:27 -0800 Subject: [PATCH 4/4] mkdir: fix unused import on non-Linux FromIo is only used in chmod, which is now Linux-only. --- src/uu/mkdir/src/mkdir.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/uu/mkdir/src/mkdir.rs b/src/uu/mkdir/src/mkdir.rs index e75b1a05c06..cad8f5fb64a 100644 --- a/src/uu/mkdir/src/mkdir.rs +++ b/src/uu/mkdir/src/mkdir.rs @@ -10,7 +10,7 @@ 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;