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 1 commit 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 1 commit 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

Copy link
Copy Markdown

GNU testsuite comparison:

Skip an intermittent issue tests/date/resolution (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)
Note: The gnu test tests/tail/pipe-f is now being skipped but was previously passing.
Congrats! The gnu test tests/pr/bounded-memory is now passing!

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