Skip to content

Move UmaskGuard in uucore and use it for mkdir, mkfifo & mknod #10310

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
sylvestre wants to merge 6 commits into
base: main
Choose a base branch
from
+164 −81
Open

Move UmaskGuard in uucore and use it for mkdir, mkfifo & mknod #10310

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
90f7756
uucore: add UmaskGuard RAII helper for safe umask manipulation
sylvestre Jan 18, 2026
39fa9de
mkdir: use uucore::mode::UmaskGuard instead of local implementation
sylvestre Jan 18, 2026
de567c4
mkfifo: fix race condition by using UmaskGuard for atomic permissions
sylvestre Jan 18, 2026
c20327c
mknod: make umask manipulation panic-safe with UmaskGuard
sylvestre Jan 18, 2026
29061b6
Update spell-checker ignore list in mode.rs
sylvestre Jan 19, 2026
64c9dda
Merge branch 'main' into followup-10036
sylvestre Jan 24, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 2 additions & 24 deletions src/uu/mkdir/src/mkdir.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -245,28 +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(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
Expand All @@ -278,7 +256,7 @@ fn create_dir_with_mode(path: &Path, mode: u32) -> std::io::Result<()> {

// 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);
let _guard = mode::UmaskGuard::set(0);

std::fs::DirBuilder::new().mode(mode).create(path)
}
Expand Down
22 changes: 10 additions & 12 deletions src/uu/mkfifo/src/mkfifo.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,6 @@
use clap::{Arg, ArgAction, Command, value_parser};
use nix::sys::stat::Mode;
use nix::unistd::mkfifo;
use std::fs;
use std::os::unix::fs::PermissionsExt;
use uucore::display::Quotable;
use uucore::error::{UResult, USimpleError};
use uucore::translate;
Expand Down Expand Up @@ -48,22 +46,22 @@
};

for f in fifos {
if mkfifo(f.as_str(), Mode::from_bits_truncate(0o666)).is_err() {
// Create FIFO with exact mode by temporarily setting umask to 0.
// This avoids a race condition where the FIFO briefly exists with
// umask-based permissions before chmod is called.
let result = {
let _guard = uucore::mode::UmaskGuard::set(0);
mkfifo(f.as_str(), Mode::from_bits_truncate(mode as libc::mode_t))
};

if result.is_err() {
show!(USimpleError::new(
1,
translate!("mkfifo-error-cannot-create-fifo", "path" => f.quote()),
));
continue;

Check failure on line 62 in src/uu/mkfifo/src/mkfifo.rs

View workflow job for this annotation

GitHub Actions / Style and Lint (unix) 

ERROR: `cargo clippy`: this `continue` expression is redundant (file:'src/uu/mkfifo/src/mkfifo.rs', line:62)
}

// Explicitly set the permissions to ignore umask
if let Err(e) = fs::set_permissions(&f, fs::Permissions::from_mode(mode)) {
return Err(USimpleError::new(
1,
translate!("mkfifo-error-cannot-set-permissions", "path" => f.quote(), "error" => e),
));
}

// Apply SELinux context if requested
#[cfg(all(feature = "selinux", any(target_os = "linux", target_os = "android")))]
{
Expand All @@ -76,7 +74,7 @@
if let Err(e) =
uucore::selinux::set_selinux_security_context(Path::new(&f), context)
{
let _ = fs::remove_file(f);
let _ = std::fs::remove_file(f);
return Err(USimpleError::new(1, e.to_string()));
}
}
Expand Down
76 changes: 37 additions & 39 deletions src/uu/mknod/src/mknod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,57 +64,55 @@ pub struct Config<'a> {
fn mknod(file_name: &str, config: Config) -> i32 {
let c_str = CString::new(file_name).expect("Failed to convert to CString");

unsafe {
// set umask to 0 and store previous umask
let have_prev_umask = if config.use_umask {
let errno = {
// Use UmaskGuard to ensure umask is restored even on panic
let _guard = if config.use_umask {
None
} else {
Some(libc::umask(0))
Some(uucore::mode::UmaskGuard::set(0))
};

let errno = libc::mknod(c_str.as_ptr(), config.mode, config.dev);
// SAFETY: mknod is a standard POSIX syscall. The c_str pointer is valid
// for the duration of the call.
unsafe { libc::mknod(c_str.as_ptr(), config.mode, config.dev) }
}; // Guard dropped here, restoring umask

// set umask back to original value
if let Some(prev_umask) = have_prev_umask {
libc::umask(prev_umask);
}

if errno == -1 {
let c_str = CString::new(uucore::execution_phrase().as_bytes())
.expect("Failed to convert to CString");
// shows the error from the mknod syscall
if errno == -1 {
let c_str = CString::new(uucore::execution_phrase().as_bytes())
.expect("Failed to convert to CString");
// shows the error from the mknod syscall
// SAFETY: perror is a standard C function that doesn't store the pointer
unsafe {
libc::perror(c_str.as_ptr());
}
}

// Apply SELinux context if requested
#[cfg(feature = "selinux")]
if config.set_security_context {
if let Err(e) = uucore::selinux::set_selinux_security_context(
std::path::Path::new(file_name),
config.context,
) {
// if it fails, delete the file
let _ = std::fs::remove_dir(file_name);
eprintln!("{}: {}", uucore::util_name(), e);
return 1;
}
// Apply SELinux context if requested
#[cfg(feature = "selinux")]
if config.set_security_context {
if let Err(e) = uucore::selinux::set_selinux_security_context(
std::path::Path::new(file_name),
config.context,
) {
// if it fails, delete the file
let _ = std::fs::remove_dir(file_name);
eprintln!("{}: {}", uucore::util_name(), e);
return 1;
}
}

// Apply SMACK context if requested
#[cfg(feature = "smack")]
if config.set_security_context {
if let Err(e) =
uucore::smack::set_smack_label_and_cleanup(file_name, config.context, |p| {
std::fs::remove_file(p)
})
{
eprintln!("{}: {}", uucore::util_name(), e);
return 1;
}
// Apply SMACK context if requested
#[cfg(feature = "smack")]
if config.set_security_context {
if let Err(e) = uucore::smack::set_smack_label_and_cleanup(file_name, config.context, |p| {
std::fs::remove_file(p)
}) {
eprintln!("{}: {}", uucore::util_name(), e);
return 1;
}

errno
}

errno
}

#[uucore::main]
Expand Down
112 changes: 111 additions & 1 deletion src/uucore/src/lib/features/mode.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@

//! Set of functions to parse modes

// spell-checker:ignore (vars) fperm srwx
// spell-checker:ignore (vars) fperm srwx RAII

use libc::umask;

Expand Down Expand Up @@ -195,6 +195,53 @@ pub fn get_umask() -> u32 {
return mask as u32;
}

/// RAII guard to restore umask on drop, ensuring cleanup even on panic.
///
/// This is useful when temporarily setting umask to 0 to create files or
/// directories with exact permissions, bypassing umask. The guard ensures
/// the original umask is restored even if a panic occurs.
///
/// # Example
///
/// ```no_run
/// use uucore::mode::UmaskGuard;
/// use std::fs;
///
/// // Temporarily set umask to 0, then restore original on drop
/// let _guard = UmaskGuard::set(0);
/// fs::create_dir("dir_with_exact_mode").unwrap();
/// // Original umask is restored here when _guard is dropped
/// ```
#[cfg(unix)]
pub struct UmaskGuard(libc::mode_t);

#[cfg(unix)]
impl UmaskGuard {
/// Set umask to the given value and return a guard that restores the original on drop.
///
/// # Safety
///
/// This function manipulates the process-wide umask. While this is safe from a
/// memory safety perspective, it can affect other threads in multi-threaded programs.
/// The guard pattern ensures restoration even on panic.
pub fn set(new_mask: libc::mode_t) -> Self {
// SAFETY: umask always succeeds and doesn't operate on memory.
// The returned value is the previous umask which we store for restoration.
let old_mask = unsafe { libc::umask(new_mask) };
Self(old_mask)
}
}

#[cfg(unix)]
impl Drop for UmaskGuard {
fn drop(&mut self) {
// SAFETY: umask always succeeds. We're restoring the original value.
unsafe {
libc::umask(self.0);
}
}
}

#[cfg(test)]
mod tests {

Expand Down Expand Up @@ -334,4 +381,67 @@ mod tests {
// First add user write, then set to 755 (should override)
assert_eq!(parse("u+w,755", false, 0).unwrap(), 0o755);
}

#[test]
#[cfg(unix)]
fn test_umask_guard_basic() {
use super::{UmaskGuard, get_umask};

// Save original umask
let original = get_umask();

// Set umask to 0 and verify it's restored
{
let _guard = UmaskGuard::set(0);
assert_eq!(get_umask(), 0);
} // Guard dropped here

// Verify original umask is restored
assert_eq!(get_umask(), original);
}

#[test]
#[cfg(unix)]
fn test_umask_guard_nested() {
use super::{UmaskGuard, get_umask};

let original = get_umask();

// Test nested guards work correctly
{
let _guard1 = UmaskGuard::set(0o077);
assert_eq!(get_umask(), 0o077);

{
let _guard2 = UmaskGuard::set(0);
assert_eq!(get_umask(), 0);
} // guard2 dropped, should restore to 0o077

assert_eq!(get_umask(), 0o077);
} // guard1 dropped, should restore to original

assert_eq!(get_umask(), original);
}

#[test]
#[cfg(unix)]
Copy link
Collaborator

@ChrisDryden ChrisDryden Jan 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test is failing because the coverage tests use -Cpanic=abort so they don't have catch_unwind

You can add:

#[cfg(panic = "unwind")]

fn test_umask_guard_panic_safety() {
use super::{UmaskGuard, get_umask};
use std::panic::{AssertUnwindSafe, catch_unwind};

let original = get_umask();

// Test that umask is restored even if code panics
let result = catch_unwind(AssertUnwindSafe(|| {
let _guard = UmaskGuard::set(0o777);
assert_eq!(get_umask(), 0o777);
panic!("Test panic");
}));

// Panic should have been caught
assert!(result.is_err());

// Umask should still be restored to original
assert_eq!(get_umask(), original);
}
}
9 changes: 4 additions & 5 deletions tests/by-util/test_mkfifo.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,17 +153,16 @@ fn test_create_fifo_permission_denied() {
at.mkdir(no_exec_dir);
at.set_mode(no_exec_dir, 0o644);

// We no longer attempt to modify file permission if the file was failed to be created.
// Therefore the error message should only contain "cannot create".
let err_msg = format!("mkfifo: cannot create fifo '{named_pipe}': File exists\n");

// With atomic permission setting, mkfifo fails with a single error.
// The exact error depends on whether the FIFO already exists or the
// directory lacks execute permission.
scene
.ucmd()
.arg(named_pipe)
.arg("-m")
.arg("666")
.fails()
.stderr_is(err_msg.as_str());
.stderr_contains("mkfifo: cannot create fifo '{named_pipe}': File exists\n");
}

#[test]
Expand Down
Loading