Fix mkfifo symbolic mode handling for POSIX compliance

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));
                     }
                 }
 

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<String>,
 
     #[arg(help = gettext("A pathname of the FIFO special file to be created"))]
     files: Vec<String>,
 }
 
-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 {
+        Some(unsafe { libc::umask(0) })
+    } else {
+        None
+    };
+
     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 let Some(umask) = old_umask {
+        unsafe { libc::umask(umask) };
+    }
+
     if res < 0 {
         return Err(io::Error::last_os_error());
     }
@@ -53,14 +67,15 @@ fn main() -> Result<(), Box<dyn std::error::Error>> {
     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),
     };
 
     // 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);
         }

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");
+}

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;