From 2895db95c1a330f6eb55cffa5008aed17da830d5 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 8 Feb 2026 07:27:08 +0000 Subject: [PATCH 1/4] Initial plan From a4ee32056aa57d3b76152cdd47e20a78c0bde974 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 8 Feb 2026 07:40:07 +0000 Subject: [PATCH 2/4] Fix mkfifo symbolic mode handling for POSIX compliance - Fix clap to allow mode values starting with - (e.g., `-m -w`) - Fix modestr::mutate to correctly handle Remove operations with umask - Bypass umask when creating FIFO with explicit -m mode per POSIX - When no who is specified in symbolic mode remove operations, umask protects bits from being removed Co-authored-by: jgarzik <494411+jgarzik@users.noreply.github.com> --- plib/src/modestr.rs | 8 +++++--- tree/mkfifo.rs | 21 ++++++++++++++++++--- 2 files changed, 23 insertions(+), 6 deletions(-) diff --git a/plib/src/modestr.rs b/plib/src/modestr.rs index f612dc114..ecd6fefcc 100644 --- a/plib/src/modestr.rs +++ b/plib/src/modestr.rs @@ -289,9 +289,11 @@ pub fn mutate(init_mode: u32, is_dir: bool, symbolic: &ChmodSymbolic) -> u32 { if who_is_not_specified { let umask = get_umask(); - user &= !(rwx << 6) & !umask; - group &= !(rwx << 3) & !umask; - others &= !rwx & !umask; + // When who is not specified, umask protects bits from being removed + // We remove bits in rwx, except those protected by umask + user &= !(rwx << 6) | (umask & (S_IRWXU as u32)); + group &= !(rwx << 3) | (umask & (S_IRWXG as u32)); + others &= !rwx | (umask & (S_IRWXO as u32)); } } diff --git a/tree/mkfifo.rs b/tree/mkfifo.rs index f76e961f9..e616b2c82 100644 --- a/tree/mkfifo.rs +++ b/tree/mkfifo.rs @@ -17,25 +17,39 @@ use std::io; #[derive(Parser)] #[command(version, about = gettext("mkfifo - make FIFO special files"))] struct Args { - #[arg(short, long, help = gettext("Set the file permission bits of the newly-created FIFO to the specified mode value"))] + #[arg(short, long, allow_hyphen_values = true, help = gettext("Set the file permission bits of the newly-created FIFO to the specified mode value"))] mode: Option, #[arg(help = gettext("A pathname of the FIFO special file to be created"))] files: Vec, } -fn do_mkfifo(filename: &str, mode: &ChmodMode) -> io::Result<()> { +fn do_mkfifo(filename: &str, mode: &ChmodMode, explicit_mode: bool) -> io::Result<()> { let mode_val = match mode { ChmodMode::Absolute(mode, _) => *mode, ChmodMode::Symbolic(sym) => modestr::mutate(0o666, false, sym), }; + // When mode is explicitly specified with -m, bypass umask per POSIX spec + // When no mode is specified, let umask apply normally + let old_umask = if explicit_mode { + unsafe { libc::umask(0) } + } else { + 0 // Dummy value, won't be used + }; + let res = unsafe { libc::mkfifo( filename.as_ptr() as *const libc::c_char, mode_val as libc::mode_t, ) }; + + // Restore the original umask if we changed it + if explicit_mode { + unsafe { libc::umask(old_umask) }; + } + if res < 0 { return Err(io::Error::last_os_error()); } @@ -53,6 +67,7 @@ fn main() -> Result<(), Box> { let mut exit_code = 0; // parse the mode string + let explicit_mode = args.mode.is_some(); let mode = match args.mode { Some(mode) => modestr::parse(&mode)?, None => ChmodMode::Absolute(0o666, 3), @@ -60,7 +75,7 @@ fn main() -> Result<(), Box> { // apply the mode to each file for filename in &args.files { - if let Err(e) = do_mkfifo(filename, &mode) { + if let Err(e) = do_mkfifo(filename, &mode, explicit_mode) { exit_code = 1; eprintln!("{}: {}", filename, e); } From 1e91d8011908d610a9a3082e7b2079065a66cc11 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 8 Feb 2026 07:44:19 +0000 Subject: [PATCH 3/4] Add comprehensive tests for mkfifo - Test basic FIFO creation - Test error cases (already exists, invalid mode) - Test absolute mode setting - Test symbolic modes (+x, -w, a-w) - Test multiple FIFO creation - All tests verify POSIX-compliant behavior Co-authored-by: jgarzik <494411+jgarzik@users.noreply.github.com> --- tree/tests/mkfifo/mod.rs | 170 +++++++++++++++++++++++++++++++++++++++ tree/tests/tree-tests.rs | 1 + 2 files changed, 171 insertions(+) create mode 100644 tree/tests/mkfifo/mod.rs diff --git a/tree/tests/mkfifo/mod.rs b/tree/tests/mkfifo/mod.rs new file mode 100644 index 000000000..c70ab7057 --- /dev/null +++ b/tree/tests/mkfifo/mod.rs @@ -0,0 +1,170 @@ +// +// Copyright (c) 2024 Jeff Garzik +// Copyright (c) 2024 Hemi Labs, Inc. +// +// This file is part of the posixutils-rs project covered under +// the MIT License. For the full license text, please see the LICENSE +// file in the root directory of this project. +// SPDX-License-Identifier: MIT +// + +use plib::testing::{run_test_with_checker, TestPlan}; +use std::fs; +use std::path::Path; +use std::process::Output; + +fn run_mkfifo_test(args: Vec<&str>, expected_exit_code: i32) { + let plan = TestPlan { + cmd: String::from("mkfifo"), + args: args.iter().map(|&s| s.into()).collect(), + stdin_data: String::new(), + expected_out: String::new(), + expected_err: String::new(), + expected_exit_code, + }; + + run_test_with_checker(plan, |_, output: &Output| { + assert_eq!(output.status.code(), Some(expected_exit_code)); + }); +} + +#[test] +fn test_create_single_fifo() { + let fifo_path = "/tmp/posixutils_mkfifo_test_1"; + let _ = fs::remove_file(fifo_path); + + run_mkfifo_test(vec![fifo_path], 0); + + assert!(Path::new(fifo_path).exists()); + + #[cfg(unix)] + { + use std::os::unix::fs::FileTypeExt; + let metadata = fs::metadata(fifo_path).expect("Unable to get FIFO metadata"); + assert!(metadata.file_type().is_fifo()); + } + + fs::remove_file(fifo_path).expect("Unable to remove test FIFO"); +} + +#[test] +fn test_fifo_already_exists() { + let fifo_path = "/tmp/posixutils_mkfifo_test_2"; + let _ = fs::remove_file(fifo_path); + + run_mkfifo_test(vec![fifo_path], 0); + assert!(Path::new(fifo_path).exists()); + + run_mkfifo_test(vec![fifo_path], 1); + + fs::remove_file(fifo_path).expect("Unable to remove test FIFO"); +} + +#[test] +fn test_invalid_mode() { + let fifo_path = "/tmp/posixutils_mkfifo_test_3"; + let _ = fs::remove_file(fifo_path); + + run_mkfifo_test(vec!["-m", "invalid", fifo_path], 1); + + assert!(!Path::new(fifo_path).exists()); +} + +#[test] +fn test_set_fifo_mode_absolute() { + let fifo_path = "/tmp/posixutils_mkfifo_test_4"; + let _ = fs::remove_file(fifo_path); + + run_mkfifo_test(vec!["-m", "644", fifo_path], 0); + + assert!(Path::new(fifo_path).exists()); + + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + let metadata = fs::metadata(fifo_path).expect("Unable to get FIFO metadata"); + let permissions = metadata.permissions(); + assert_eq!(permissions.mode() & 0o777, 0o644); + } + + fs::remove_file(fifo_path).expect("Unable to remove test FIFO"); +} + +#[test] +fn test_set_fifo_mode_symbolic_plus() { + let fifo_path = "/tmp/posixutils_mkfifo_test_5"; + let _ = fs::remove_file(fifo_path); + + run_mkfifo_test(vec!["-m", "+x", fifo_path], 0); + + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + let metadata = fs::metadata(fifo_path).expect("Unable to get FIFO metadata"); + let permissions = metadata.permissions(); + assert_eq!( + permissions.mode() & 0o777, + 0o777, + "+x should produce rwxrwxrwx" + ); + } + + fs::remove_file(fifo_path).expect("Unable to remove test FIFO"); +} + +#[test] +fn test_set_fifo_mode_symbolic_minus() { + let fifo_path = "/tmp/posixutils_mkfifo_test_6"; + let _ = fs::remove_file(fifo_path); + + run_mkfifo_test(vec!["-m", "-w", fifo_path], 0); + + assert!(Path::new(fifo_path).exists()); + + #[cfg(unix)] + { + use std::os::unix::fs::FileTypeExt; + let metadata = fs::metadata(fifo_path).expect("Unable to get FIFO metadata"); + assert!(metadata.file_type().is_fifo()); + } + + fs::remove_file(fifo_path).expect("Unable to remove test FIFO"); +} + +#[test] +fn test_set_fifo_mode_symbolic_who_specified() { + let fifo_path = "/tmp/posixutils_mkfifo_test_7"; + let _ = fs::remove_file(fifo_path); + + run_mkfifo_test(vec!["-m", "a-w", fifo_path], 0); + + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + let metadata = fs::metadata(fifo_path).expect("Unable to get FIFO metadata"); + let permissions = metadata.permissions(); + assert_eq!( + permissions.mode() & 0o777, + 0o444, + "a-w should produce r--r--r--" + ); + } + + fs::remove_file(fifo_path).expect("Unable to remove test FIFO"); +} + +#[test] +fn test_create_multiple_fifos() { + let fifo1 = "/tmp/posixutils_mkfifo_test_8a"; + let fifo2 = "/tmp/posixutils_mkfifo_test_8b"; + let _ = fs::remove_file(fifo1); + let _ = fs::remove_file(fifo2); + + run_mkfifo_test(vec![fifo1, fifo2], 0); + + assert!(Path::new(fifo1).exists()); + assert!(Path::new(fifo2).exists()); + + fs::remove_file(fifo1).expect("Unable to remove test FIFO"); + fs::remove_file(fifo2).expect("Unable to remove test FIFO"); +} diff --git a/tree/tests/tree-tests.rs b/tree/tests/tree-tests.rs index 4f6282853..75738d3d7 100644 --- a/tree/tests/tree-tests.rs +++ b/tree/tests/tree-tests.rs @@ -15,6 +15,7 @@ mod du; mod link; mod ls; mod mkdir; +mod mkfifo; mod mv; mod readlink; mod rm; From 3ed469bd4183115ef7e0d5b0d8f61d888d4d2e2d Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 8 Feb 2026 07:45:16 +0000 Subject: [PATCH 4/4] Address code review feedback: use Option instead of dummy value - Replace dummy umask value with Option - Improves code clarity and safety Co-authored-by: jgarzik <494411+jgarzik@users.noreply.github.com> --- tree/mkfifo.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tree/mkfifo.rs b/tree/mkfifo.rs index e616b2c82..4f79a2108 100644 --- a/tree/mkfifo.rs +++ b/tree/mkfifo.rs @@ -33,9 +33,9 @@ fn do_mkfifo(filename: &str, mode: &ChmodMode, explicit_mode: bool) -> io::Resul // When mode is explicitly specified with -m, bypass umask per POSIX spec // When no mode is specified, let umask apply normally let old_umask = if explicit_mode { - unsafe { libc::umask(0) } + Some(unsafe { libc::umask(0) }) } else { - 0 // Dummy value, won't be used + None }; let res = unsafe { @@ -46,8 +46,8 @@ fn do_mkfifo(filename: &str, mode: &ChmodMode, explicit_mode: bool) -> io::Resul }; // Restore the original umask if we changed it - if explicit_mode { - unsafe { libc::umask(old_umask) }; + if let Some(umask) = old_umask { + unsafe { libc::umask(umask) }; } if res < 0 {