diff: don't panic on an operand ending in --width=N - #279
Conversation
Sorry, something went wrong.
The `--width` regex was missing the `^` anchor that its `--tabsize` sibling
one line above already has, so it matched any argument whose lossy form ended
in `--width=<digits>` rather than the option itself. Two symptoms follow, and
the quieter one is worse:
diff $'\xff--width=5' A B # aborts, exit 134
diff xx--width=5 A B # taken as a width, the operand is discarded
The first reaches `into_string().unwrap()` with a non-UTF-8 argument and, under
`panic = "abort"`, takes the process down. The second does not crash at all: the
operand is swallowed as a width and diff compares the remaining two files,
exiting 1. GNU treats both as file operands, reports the extra operand, exits 2.
Anchoring restores the invariant the `--tabsize` block documents, that a match
implies valid UTF-8, so the existing `unwrap` is sound rather than merely
unlikely to fire. I mirrored that block rather than reworking the `unwrap`
separately, to keep the two option paths reading the same way.
`--width` had no test coverage, which is how this survived. Added a `width` test
alongside `tabsize` covering valid values and the invalid forms, plus both cases
above. The non-UTF-8 one is `cfg(unix)`, since it needs bytes an `OsString`
cannot hold on Windows.
Exit codes now match GNU diffutils 3.12 for `--width=5`, `--width=5x`,
`xx--width=5` and the non-UTF-8 form. The error text still differs, because uu
prints its usage line where GNU names the extra operand, but that gap predates
this change and affects any three-operand invocation.
Closes uutils#247
Merging this PR will degrade performance by 3.47%⚡ 1 improved benchmark Warning Please fix the performance issues or acknowledge them on CodSpeed. Performance Changes
Tip Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent. Comparing ARMeeru:fix/anchor-width-option-regex (5377ac8) with main (cbc5298) |
Sorry, something went wrong.
|
I don't think the CodSpeed report is attributable to this change, and part of it may be pointing at the benchmark suite rather than the PR. Two of the five regressions, cmd_cmp_gnu_equal[1000] and [100], call Command::new("cmp"), so they spawn the system GNU cmp rather than anything built here. Nothing in this repo can move those. The other three spawn our binary's cmp subcommand, which parses its own arguments in cmp.rs and never reaches src/params.rs. All six benchmarks that moved are subprocess spawns, and none of the in-process ones moved. That includes diff_parser, which calls params::parse_params directly with --width=100 in its input, so it exercises exactly the line this PR changes. Locally it goes 394.1 µs to 387.1 µs, while cmp_parser goes 249.7 ns to 228.9 ns with identical code on both sides, which is about the noise floor at that scale. The one benchmark on the diff path that did move got faster. The base run was also on a different runner image, 20260729.566 against 20260819.586 for this branch, and spawn cost is mostly exec and dynamic linking. Happy to look again if you read it differently. |
Sorry, something went wrong.
--width is parsed with a regex that is missing the ^ anchor its --tabsize neighbour has one line above it, so it matches any argument whose lossy form ends in --width=<digits> rather than the option itself.
That produces two symptoms, and the quieter one is worse than the reported crash:
The first is the abort from the issue: a non-UTF-8 argument reaches into_string().unwrap() and, under panic = "abort", takes the process down. The second does not crash at all. xx--width=5 is accepted as a width, the operand is quietly discarded, and diff compares the two files that are left. GNU treats both as file operands and exits 2 with extra operand 'B'.
The fix
Anchoring the regex restores the invariant the --tabsize block documents a little further down, that a match implies valid UTF-8. I added that same comment to the --width block and left the existing unwrap in place rather than reworking it separately, so the two option paths keep reading the same way. To be precise about what this does: the anchor makes that unwrap unreachable, it does not remove it.
Tests
--width had no test coverage, which is how this survived. There is now a width test next to tabsize, covering valid values and the same invalid forms tabsize already checks, plus both cases above. The non-UTF-8 case is cfg(unix), since it needs bytes an OsString cannot hold on Windows.
Both new assertions fail without the anchor: width_non_utf8_operand aborts at into_string().unwrap(), and width fails on xx--width=5.
Verification
Exit codes compared against GNU diffutils 3.12:
cargo test goes from 272 to 274 passing, and cargo fmt --all -- --check, cargo clippy -- -D warnings and cargo test --all-features --no-fail-fast are clean. The GNU upstream test suite gives identical per-test results before and after. I ran it on macOS, where much of it fails for unrelated environment reasons, so I compared per test against a clean main build rather than reading the totals.
Out of scope
The error text still differs from GNU here: this prints Usage: diff <from> <to> where GNU prints extra operand 'B' followed by a Try --help line. That is not specific to --width, it happens for any three-operand invocation, so I left it alone.
Closes #247