Skip to content

mkdir, install, mkfifo, mknod: fix -m's invalid-mode message and an empty-clause bug - #14302

Open
arbelonson-source wants to merge 2 commits into
uutils:mainfrom
arbelonson-source:fix/mode-parsers-invalid-mode-message
Open

mkdir, install, mkfifo, mknod: fix -m's invalid-mode message and an empty-clause bug#14302
arbelonson-source wants to merge 2 commits into
uutils:mainfrom
arbelonson-source:fix/mode-parsers-invalid-mode-message

Conversation

@arbelonson-source

Copy link
Copy Markdown
Contributor

Follow-up to #14301 (chmod). mkdir/install/mkfifo/mknod share the same mode parser chmod uses, and — as flagged in that PR's body — have the identical class of bug in their own -m/--mode handling: each leaked 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 9.11
mkdir: invalid mode '999'

Three different messages, verified individually — not assumed

GNU's wording is not identical across these four, so I checked each rather than assuming chmod's fix applies as-is:

  • mkdir/install name the operand, but unlike chmod use no colon before the quote and add no "Try --help" hint: mkdir: invalid mode '999', install: invalid mode '999'.
  • mkfifo/mknod don't name the operand at all, regardless of what was wrong with it: just mkfifo: invalid mode / mknod: invalid mode.

A second, more serious bug

Investigating turned up something beyond message wording, in the parser these four share but chmod's own inline loop does not use (which is why it didn't affect chmod): an empty clause in a comma-separated mode was silently skipped as a no-op instead of erroring.

$ mkdir -m ',' d                       # ours, before: silently created d with a default mode
$ mkdir -m 'u+rwx,' d                  # ours, before: same
$ mkdir -m ',' d                       # GNU
mkdir: invalid mode ','

Checked against GNU with a leading, trailing, and doubled comma — none are tolerated, so parse_chmod's if mode_part.is_empty() { continue; } was simply wrong; removing it lets parse_symbolic's existing empty-string handling report it the way it already does for chmod.

Also touches describe()

mode.rs's describe() needed the same fix chmod's PR made: now that the headline never explains which operator was invalid or expected, that detail lives in the caret-diagram label instead. This function is shared, so both PRs touch the same three lines with the same final text — whichever of the two merges second will need a trivial, content-identical rebase.

Testing

  • Differential sweep against GNU 9.11 for all four utilities across bad digits, too-large octal, missing/invalid operators, empty mode, and the empty-clause cases (leading/trailing/doubled comma): all match.
  • New regression tests in each of the four utilities' test files, each verified to fail without its respective fix (5 failures reproduced by stashing the fix and re-running).
  • cargo test --features mkdir,install,mkfifo,mknod,chmod --test tests -- test_mkdir test_install test_mkfifo test_mknod test_chmod: 257 passed, 0 failed (all pre-existing tests unaffected; chmod's own suite included since it shares mode.rs).
  • moz-fluent-lint on all touched locale directories: no errors.
  • cargo fmt --check and cargo clippy -p uu_mkdir -p uu_install -p uu_mkfifo -p uu_mknod -p uucore --features mode --all-targets -- -D warnings: clean.

Disclosure

Prepared with AI assistance (Claude Opus 5, via Claude Code), per the AI policy in CONTRIBUTING.md. GNU's behaviour was established by running the installed GNU binary as a black box; I did not read GNU coreutils source. All testing was run locally.

…mpty-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 uutils#14301 -- expect a trivial rebase against whichever lands second.)
@github-actions

github-actions Bot commented Aug 31, 2026

Copy link
Copy Markdown

GNU testsuite comparison:

Skip an intermittent issue tests/misc/usage_vs_getopt (fails in this run but passes in the 'main' branch)
Skipping an intermittent issue tests/cut/bounded-memory (passes in this run but fails in the 'main' branch)
Skipping an intermittent issue tests/date/date-locale-hour (passes in this run but fails in the 'main' branch)
Note: The gnu test tests/tail/tail-n0f is now being skipped but was previously passing.
Skip an intermittent issue tests/pr/bounded-memory (was skipped on 'main', now failing)

…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant