From 2dd10215d95c130ace55313a8e3570d352e57020 Mon Sep 17 00:00:00 2001 From: arbelonson-source <269032023+arbelonson-source@users.noreply.github.com> Date: Mon, 31 Aug 2026 05:21:36 +0300 Subject: [PATCH 1/2] mkdir, install, mkfifo, mknod: fix -m's invalid-mode message and an empty-clause bug Follow-up to the chmod fix: `mkdir`/`install`/`mkfifo`/`mknod` share the same mode parser chmod uses, and had the identical class of bug in their own `-m`/`--mode` handling -- each leaking a message shaped like whatever failed to parse internally, rather than GNU's own wording: $ mkdir -m 999 d # ours, before: a raw ParseIntError mkdir: invalid digit found in string $ mkdir -m 999 d # GNU mkdir: invalid mode '999' GNU's wording is not identical across these, so each got its own fix rather than reusing chmod's message: `mkdir`/`install` name the operand but, unlike `chmod`, use no colon and add no "Try --help" hint; `mkfifo`/ `mknod` don't name the operand at all, just a bare "invalid mode". A second, more serious bug turned up in the process, in the parser these four share (`chmod`'s own inline loop does not use it, which is why this one didn't affect chmod): an empty clause in a comma-separated mode -- the whole mode being empty, or a leading, trailing, or doubled comma -- was silently treated as a no-op instead of an error, so `mkdir -m ',' d` and `mkdir -m 'u+rwx,' d` both succeeded where GNU rejects both. Verified against GNU that no comma-separated form tolerates an empty clause. `describe()`'s caret label for `InvalidOperator` picks up the same fix `mode.rs` needed for chmod's PR: now that the headline never explains the operator, the label does, reusing the message that used to be the headline. (Independent of, but touching the same function as, the chmod fix in #14301 -- expect a trivial rebase against whichever lands second.) --- src/uu/install/locales/en-US.ftl | 2 +- src/uu/install/locales/fr-FR.ftl | 2 +- src/uu/install/src/install.rs | 4 +++- src/uu/mkdir/locales/en-US.ftl | 1 + src/uu/mkdir/locales/fr-FR.ftl | 1 + src/uu/mkdir/src/mkdir.rs | 8 ++++++-- src/uu/mkfifo/locales/en-US.ftl | 2 +- src/uu/mkfifo/locales/fr-FR.ftl | 2 +- src/uu/mkfifo/src/mkfifo.rs | 4 +++- src/uu/mknod/locales/en-US.ftl | 2 +- src/uu/mknod/locales/fr-FR.ftl | 2 +- src/uu/mknod/src/mknod.rs | 6 ++++-- src/uucore/src/lib/features/mode.rs | 28 +++++++++++++++------------- tests/by-util/test_install.rs | 17 ++++++++++++++++- tests/by-util/test_mkdir.rs | 26 ++++++++++++++++++++++++++ tests/by-util/test_mkfifo.rs | 12 ++++++++++++ tests/by-util/test_mknod.rs | 14 +++++++++++++- 17 files changed, 106 insertions(+), 27 deletions(-) diff --git a/src/uu/install/locales/en-US.ftl b/src/uu/install/locales/en-US.ftl index f7104d5e415..e71c5799cf8 100644 --- a/src/uu/install/locales/en-US.ftl +++ b/src/uu/install/locales/en-US.ftl @@ -45,7 +45,7 @@ install-error-same-file = { $file1 } and { $file2 } are the same file install-error-extra-operand = extra operand { $operand } { $usage } install-error-not-permitted = cannot remove { $path }: Operation not permitted -install-error-invalid-mode = Invalid mode string: { $error } +install-error-invalid-mode = invalid mode '{ $mode }' install-error-mutually-exclusive-target = Options --target-directory and --no-target-directory are mutually exclusive install-error-mutually-exclusive-compare-strip = Options --compare and --strip are mutually exclusive install-error-missing-file-operand = missing file operand diff --git a/src/uu/install/locales/fr-FR.ftl b/src/uu/install/locales/fr-FR.ftl index 1503c0c7c7d..23c7ce0e60d 100644 --- a/src/uu/install/locales/fr-FR.ftl +++ b/src/uu/install/locales/fr-FR.ftl @@ -44,7 +44,7 @@ install-error-override-directory-failed = impossible d'écraser le répertoire { install-error-same-file = { $file1 } et { $file2 } sont le même fichier install-error-extra-operand = opérande supplémentaire { $operand } { $usage } -install-error-invalid-mode = Chaîne de mode invalide : { $error } +install-error-invalid-mode = mode invalide '{ $mode }' install-error-mutually-exclusive-target = Les options --target-directory et --no-target-directory sont mutuellement exclusives install-error-mutually-exclusive-compare-strip = Les options --compare et --strip sont mutuellement exclusives install-error-missing-file-operand = opérande de fichier manquant diff --git a/src/uu/install/src/install.rs b/src/uu/install/src/install.rs index b8ff492039c..ac3eb6ddab0 100644 --- a/src/uu/install/src/install.rs +++ b/src/uu/install/src/install.rs @@ -374,7 +374,9 @@ fn behavior(matches: &ArgMatches, diag_args: Option<&[OsString]>) -> UResult = if matches.contains_id(OPT_MODE) { let x = matches.get_one::(OPT_MODE).ok_or(1)?; Some(uucore::mode::parse(x, considering_dir, 0).map_err(|err| { - let message = translate!("install-error-invalid-mode", "error" => err.to_string()); + // GNU always says the same thing regardless of what specifically + // went wrong, and does not add a "Try --help" hint here. + let message = translate!("install-error-invalid-mode", "mode" => x.clone()); // When the diagnostic is rendered it is already on stderr; exit quietly. if !diag_args.is_some_and(|args| err.render_mode_value(args, x, 0, &message)) { show_error!("{message}"); diff --git a/src/uu/mkdir/locales/en-US.ftl b/src/uu/mkdir/locales/en-US.ftl index fa2ef528d45..5ce490d4526 100644 --- a/src/uu/mkdir/locales/en-US.ftl +++ b/src/uu/mkdir/locales/en-US.ftl @@ -15,6 +15,7 @@ mkdir-error-file-exists = { $path }: File exists mkdir-error-failed-to-create-tree = failed to create whole tree mkdir-error-cannot-set-permissions = cannot set permissions { $path } mkdir-error-cannot-create-directory = cannot create directory '{ $path }': { $error } +mkdir-error-invalid-mode = invalid mode '{ $mode }' # Verbose output mkdir-verbose-created-directory = { $util_name }: created directory { $path } diff --git a/src/uu/mkdir/locales/fr-FR.ftl b/src/uu/mkdir/locales/fr-FR.ftl index 988f7c21f44..a60fbc2773a 100644 --- a/src/uu/mkdir/locales/fr-FR.ftl +++ b/src/uu/mkdir/locales/fr-FR.ftl @@ -15,6 +15,7 @@ mkdir-error-file-exists = { $path } : Le fichier existe mkdir-error-failed-to-create-tree = échec de la création de l'arborescence complète mkdir-error-cannot-set-permissions = impossible de définir les permissions { $path } mkdir-error-cannot-create-directory = impossible de créer le répertoire '{ $path }': { $error } +mkdir-error-invalid-mode = mode invalide '{ $mode }' # Sortie détaillée mkdir-verbose-created-directory = { $util_name } : répertoire créé { $path } diff --git a/src/uu/mkdir/src/mkdir.rs b/src/uu/mkdir/src/mkdir.rs index 0cb71395ae0..9c26ef5b3ea 100644 --- a/src/uu/mkdir/src/mkdir.rs +++ b/src/uu/mkdir/src/mkdir.rs @@ -70,11 +70,15 @@ fn get_mode(matches: &ArgMatches, diag_args: Option<&[OsString]>) -> UResult m.clone()); + if diag_args.is_some_and(|args| err.render_mode_value(args, m, 0, &message)) { // The diagnostic is already on stderr; exit quietly. ExitCode::new(1) } else { - USimpleError::new(1, err.to_string()) + USimpleError::new(1, message) } }) } diff --git a/src/uu/mkfifo/locales/en-US.ftl b/src/uu/mkfifo/locales/en-US.ftl index f66a6b0459a..1633c4ed1df 100644 --- a/src/uu/mkfifo/locales/en-US.ftl +++ b/src/uu/mkfifo/locales/en-US.ftl @@ -7,7 +7,7 @@ mkfifo-help-selinux = set the SELinux security context to default type mkfifo-help-context = like -Z, or if CTX is specified then set the SELinux or SMACK security context to CTX # Error messages -mkfifo-error-invalid-mode = invalid mode: { $error } +mkfifo-error-invalid-mode = invalid mode mkfifo-error-cannot-create-fifo = cannot create fifo { $path }: { $error } mkfifo-error-cannot-set-permissions = cannot set permissions on { $path }: { $error } mkfifo-error-non-file-permission = mode must specify only file permission bits diff --git a/src/uu/mkfifo/locales/fr-FR.ftl b/src/uu/mkfifo/locales/fr-FR.ftl index 038d8284927..4dc349a2374 100644 --- a/src/uu/mkfifo/locales/fr-FR.ftl +++ b/src/uu/mkfifo/locales/fr-FR.ftl @@ -7,7 +7,7 @@ mkfifo-help-selinux = définir le contexte de sécurité SELinux au type par dé mkfifo-help-context = comme -Z, ou si CTX est spécifié, définir le contexte de sécurité SELinux ou SMACK à CTX # Messages d'erreur -mkfifo-error-invalid-mode = mode invalide : { $error } +mkfifo-error-invalid-mode = mode invalide mkfifo-error-cannot-create-fifo = impossible de créer le fifo { $path } : { $error } mkfifo-error-cannot-set-permissions = impossible de définir les permissions sur { $path } : { $error } mkfifo-error-non-file-permission = le mode ne doit spécifier que des bits de permission de fichier diff --git a/src/uu/mkfifo/src/mkfifo.rs b/src/uu/mkfifo/src/mkfifo.rs index a28151cb443..30794ad6791 100644 --- a/src/uu/mkfifo/src/mkfifo.rs +++ b/src/uu/mkfifo/src/mkfifo.rs @@ -28,7 +28,9 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { let matches = uucore::clap_localization::handle_clap_result(uu_app(), args)?; let mode = calculate_mode(matches.get_one::(options::MODE)).map_err(|err| { - let message = translate!("mkfifo-error-invalid-mode", "error" => err.to_string()); + // Unlike `chmod`/`mkdir`/`install`, GNU does not name the mode + // operand here at all, regardless of what went wrong with it. + let message = translate!("mkfifo-error-invalid-mode"); if let Some(args) = &diag_args && let Some(mode) = matches.get_one::(options::MODE) && err.render_mode_value(args, mode, 0, &message) diff --git a/src/uu/mknod/locales/en-US.ftl b/src/uu/mknod/locales/en-US.ftl index 2198bcf382c..30f33a97cf8 100644 --- a/src/uu/mknod/locales/en-US.ftl +++ b/src/uu/mknod/locales/en-US.ftl @@ -28,7 +28,7 @@ mknod-help-context = like -Z, or if CTX is specified then set the SELinux or SMA # Error messages mknod-error-fifo-no-major-minor = Fifos do not have major and minor device numbers. mknod-error-special-require-major-minor = Special files require major and minor device numbers. -mknod-error-invalid-mode = invalid mode ({ $error }) +mknod-error-invalid-mode = invalid mode mknod-error-mode-permission-bits-only = mode must specify only file permission bits mknod-error-missing-device-type = missing device type mknod-error-invalid-device-type = invalid device type { $type } diff --git a/src/uu/mknod/locales/fr-FR.ftl b/src/uu/mknod/locales/fr-FR.ftl index c3917aac52c..6cc1f2b1938 100644 --- a/src/uu/mknod/locales/fr-FR.ftl +++ b/src/uu/mknod/locales/fr-FR.ftl @@ -28,7 +28,7 @@ mknod-help-context = comme -Z, ou si CTX est spécifié, définir le contexte de # Messages d'erreur mknod-error-fifo-no-major-minor = Les fifos n'ont pas de numéros de périphérique majeur et mineur. mknod-error-special-require-major-minor = Les fichiers spéciaux nécessitent des numéros de périphérique majeur et mineur. -mknod-error-invalid-mode = mode invalide ({ $error }) +mknod-error-invalid-mode = mode invalide mknod-error-mode-permission-bits-only = le mode ne doit spécifier que les bits de permission de fichier mknod-error-missing-device-type = type de périphérique manquant mknod-error-invalid-device-type = type de périphérique invalide { $type } diff --git a/src/uu/mknod/src/mknod.rs b/src/uu/mknod/src/mknod.rs index a4398a03323..201fc32231f 100644 --- a/src/uu/mknod/src/mknod.rs +++ b/src/uu/mknod/src/mknod.rs @@ -155,8 +155,10 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { let mode = uucore::mode::parse_chmod(MODE_RW_UGO, str_mode, true, uucore::mode::get_umask()) .map_err(|err| { - let message = - translate!("mknod-error-invalid-mode", "error" => err.to_string()); + // Unlike `chmod`/`mkdir`/`install`, GNU does not name the + // mode operand here at all, regardless of what went wrong + // with it. + let message = translate!("mknod-error-invalid-mode"); if let Some(args) = &diag_args && err.render_mode_value(args, str_mode, 0, &message) { diff --git a/src/uucore/src/lib/features/mode.rs b/src/uucore/src/lib/features/mode.rs index 8dfeccb4c94..955f0ebfbf7 100644 --- a/src/uucore/src/lib/features/mode.rs +++ b/src/uucore/src/lib/features/mode.rs @@ -131,19 +131,19 @@ impl ModeError { /// The caret label for this error, translated, and the advice that goes /// under it. /// - /// Labelled only where a label would add to the message, per the - /// convention in [`crate::diagnostics`]. + /// Every caller shows the same headline for every `ModeError` -- GNU + /// itself only ever says "invalid mode" (with or without the operand, + /// depending on the utility) -- so the detail that used to live there + /// lives in the label instead. fn describe(&self) -> (Option, Option) { let label = match self.kind { - // The message already names the expected operators. - ModeErrorKind::InvalidOperator => None, - ModeErrorKind::MissingOperator => Some("mode-diag-label-missing-operator"), - ModeErrorKind::InvalidNumber => Some("mode-diag-label-invalid-number"), + // `self.message` already names the operator that was expected + // and the one that was found instead. + ModeErrorKind::InvalidOperator => Some(self.message.clone()), + ModeErrorKind::MissingOperator => Some(translate!("mode-diag-label-missing-operator")), + ModeErrorKind::InvalidNumber => Some(translate!("mode-diag-label-invalid-number")), }; - ( - label.map(|label| translate!(label)), - Some(translate!("mode-diag-help-syntax")), - ) + (label, Some(translate!("mode-diag-help-syntax"))) } /// Where this error sits inside the whole mode, given where the clause it @@ -335,10 +335,12 @@ pub fn parse_chmod( offset += raw_part.len() + 1; let mode_part = raw_part.trim(); - if mode_part.is_empty() { - continue; - } + // An empty clause -- the whole mode, a leading, trailing or doubled + // comma -- is not tolerated: GNU rejects `chmod u+rwx, f` the same + // way it rejects `chmod , f`. `parse_symbolic` already reports that + // correctly for an empty string, so it is not special-cased away + // here. new_mode = if mode_part.chars().any(|c| c.is_ascii_digit()) { parse_numeric(new_mode, mode_part, considering_dir) } else { diff --git a/tests/by-util/test_install.rs b/tests/by-util/test_install.rs index dabd76f01cc..21b4181fbbe 100644 --- a/tests/by-util/test_install.rs +++ b/tests/by-util/test_install.rs @@ -339,13 +339,28 @@ fn test_install_mode_failing() { .arg(dir) .arg(mode_arg) .fails() - .stderr_contains("Invalid mode string: invalid digit found in string"); + .stderr_contains("invalid mode '999'"); let dest_file = &format!("{dir}/{file}"); assert!(at.file_exists(file)); assert!(!at.file_exists(dest_file)); } +/// GNU says the same thing for any way `-m`'s mode can be malformed, and it +/// is not the message chmod uses for the same failure: install's has no +/// colon before the quoted mode, and no "Try --help" hint. +#[test] +fn test_install_invalid_mode_names_the_operand() { + // The mode is validated before install so much as looks for the source, + // so a nonexistent one does not confuse this with a different error. + for mode in ["999", "", "a", "u?rwx"] { + new_ucmd!() + .args(&["source_file", "dest", "--mode", mode]) + .fails_with_code(1) + .stderr_only(format!("install: invalid mode '{mode}'\n")); + } +} + #[test] fn test_install_mode_directories() { let (at, mut ucmd) = at_and_ucmd!(); diff --git a/tests/by-util/test_mkdir.rs b/tests/by-util/test_mkdir.rs index c1796e332e1..053bfa9baf1 100644 --- a/tests/by-util/test_mkdir.rs +++ b/tests/by-util/test_mkdir.rs @@ -108,6 +108,32 @@ fn test_mkdir_mode() { new_ucmd!().arg("-m").arg("755").arg("test_dir").succeeds(); } +/// GNU says the same thing for any way `-m`'s mode can be malformed, and it +/// is not the message chmod uses for the same failure: mkdir's has no +/// colon before the quoted mode, and no "Try --help" hint. +#[test] +fn test_mkdir_invalid_mode_names_the_operand() { + for mode in ["999", "", "a", "u?rwx"] { + new_ucmd!() + .args(&["-m", mode, "test_dir"]) + .fails_with_code(1) + .stderr_only(format!("mkdir: invalid mode '{mode}'\n")); + } +} + +/// A comma-separated mode with an empty clause -- leading, trailing, or +/// doubled -- is not tolerated as a no-op clause: GNU rejects the whole +/// thing, the same as it rejects an empty mode outright. +#[test] +fn test_mkdir_rejects_an_empty_clause() { + for mode in ["u+rwx,", ",u+rwx", "u+rwx,,g+r"] { + new_ucmd!() + .args(&["-m", mode, "test_dir"]) + .fails_with_code(1) + .stderr_only(format!("mkdir: invalid mode '{mode}'\n")); + } +} + #[test] fn test_mkdir_parent() { let scene = TestScenario::new(util_name!()); diff --git a/tests/by-util/test_mkfifo.rs b/tests/by-util/test_mkfifo.rs index 13d43f5a59a..73e7200c24f 100644 --- a/tests/by-util/test_mkfifo.rs +++ b/tests/by-util/test_mkfifo.rs @@ -83,6 +83,18 @@ fn test_create_one_fifo_already_exists() { .stderr_is("mkfifo: cannot create fifo 'abcdef': File exists\n"); } +/// Unlike `chmod`/`mkdir`/`install`, GNU does not name the mode operand at +/// all here, regardless of what specifically was wrong with it. +#[test] +fn test_invalid_mode_is_not_named() { + for mode in ["999", "", "a", "u?rwx"] { + new_ucmd!() + .args(&["-m", mode, "probe"]) + .fails_with_code(1) + .stderr_only("mkfifo: invalid mode\n"); + } +} + #[test] fn test_create_fifo_with_mode_and_umask() { use uucore::fs::display_permissions; diff --git a/tests/by-util/test_mknod.rs b/tests/by-util/test_mknod.rs index 49fdbc243f9..885356d4c7c 100644 --- a/tests/by-util/test_mknod.rs +++ b/tests/by-util/test_mknod.rs @@ -135,7 +135,19 @@ fn test_mknod_invalid_mode() { .fails() .no_stdout() .code_is(1) - .stderr_contains("invalid mode"); + .stderr_only("mknod: invalid mode\n"); +} + +/// Unlike `chmod`/`mkdir`/`install`, GNU does not name the mode operand at +/// all here, regardless of what specifically was wrong with it. +#[test] +fn test_invalid_mode_is_not_named() { + for mode in ["999", "", "a", "u?rwx"] { + new_ucmd!() + .args(&["--mode", mode, "probe", "p"]) + .fails_with_code(1) + .stderr_only("mknod: invalid mode\n"); + } } #[test] From 77347bc52c8577c8a66a49c0fd5fd43b3d711a0f Mon Sep 17 00:00:00 2001 From: arbelonson-source <269032023+arbelonson-source@users.noreply.github.com> Date: Mon, 31 Aug 2026 10:34:36 +0300 Subject: [PATCH 2/2] fix CI: correct stale empty-mode unit test, skip mkdir mode tests on Windows Two real CI failures surfaced after this branch's own change, both pre-existing gaps this PR's fix exposed rather than caused: - uucore::features::mode::tests::test_parse_empty_string asserted that an empty/whitespace/comma-only mode string parses to 0. That was never true of GNU: verified against chmod/install/mkdir 9.11 that all three reject an empty, whitespace-only, or comma-only mode operand outright. The test encoded the old (wrong) silently-skip-empty-clause behavior this PR intentionally removed; updated it to assert the correct rejection instead. - test_mkdir_invalid_mode_names_the_operand and test_mkdir_rejects_an_empty_clause failed on Windows CI because mkdir's own get_mode() is a no-op there (#[cfg(windows)] always returns Ok(None) -- there is no POSIX mode to validate or apply on that platform), so there is nothing for these tests to fail on. Gated both #[cfg(not(windows))], matching the existing convention in this same test file and the split in get_mode() itself. install, mkfifo, and mknod are all unix-only builds (feat_require_unix_core), so their own new tests were never at risk. AI-assisted-by: Claude Opus 5, via Claude Code --- src/uucore/src/lib/features/mode.rs | 13 ++++++++----- tests/by-util/test_mkdir.rs | 8 ++++++++ 2 files changed, 16 insertions(+), 5 deletions(-) diff --git a/src/uucore/src/lib/features/mode.rs b/src/uucore/src/lib/features/mode.rs index 955f0ebfbf7..eab461c3aea 100644 --- a/src/uucore/src/lib/features/mode.rs +++ b/src/uucore/src/lib/features/mode.rs @@ -471,11 +471,14 @@ mod tests { } #[test] - fn test_parse_empty_string() { - // Empty string should return 0 - assert_eq!(parse("", false, 0).unwrap(), 0); - assert_eq!(parse(" ", false, 0).unwrap(), 0); - assert_eq!(parse(",,", false, 0).unwrap(), 0); + fn test_parse_empty_string_is_invalid() { + // GNU rejects an empty, all-whitespace, or comma-only mode string + // (verified against chmod/install/mkdir 9.11: `chmod '' f`, + // `chmod ' ' f`, `chmod ',' f`, `install -m '' ...`, + // `mkdir -m '' ...` all fail with "invalid mode"). + assert!(parse("", false, 0).is_err()); + assert!(parse(" ", false, 0).is_err()); + assert!(parse(",,", false, 0).is_err()); } #[test] diff --git a/tests/by-util/test_mkdir.rs b/tests/by-util/test_mkdir.rs index 053bfa9baf1..11a083878e6 100644 --- a/tests/by-util/test_mkdir.rs +++ b/tests/by-util/test_mkdir.rs @@ -111,7 +111,11 @@ fn test_mkdir_mode() { /// GNU says the same thing for any way `-m`'s mode can be malformed, and it /// is not the message chmod uses for the same failure: mkdir's has no /// colon before the quoted mode, and no "Try --help" hint. +/// +/// `-m` is a no-op on Windows (`get_mode` always returns `None` there), so +/// there is nothing to validate and nothing to fail on that platform. #[test] +#[cfg(not(windows))] fn test_mkdir_invalid_mode_names_the_operand() { for mode in ["999", "", "a", "u?rwx"] { new_ucmd!() @@ -124,7 +128,11 @@ fn test_mkdir_invalid_mode_names_the_operand() { /// A comma-separated mode with an empty clause -- leading, trailing, or /// doubled -- is not tolerated as a no-op clause: GNU rejects the whole /// thing, the same as it rejects an empty mode outright. +/// +/// `-m` is a no-op on Windows (`get_mode` always returns `None` there), so +/// there is nothing to validate and nothing to fail on that platform. #[test] +#[cfg(not(windows))] fn test_mkdir_rejects_an_empty_clause() { for mode in ["u+rwx,", ",u+rwx", "u+rwx,,g+r"] { new_ucmd!()