fmt: reject --tab-width values above MAX_TAB_WIDTH (2500) - #14666
koopatroopa787 wants to merge 2 commits into
Conversation
`--tab-width`/`-T` had no upper bound: any usize value was accepted and flowed directly into arithmetic in linebreak.rs and parasplit.rs. Values near usize::MAX caused panics (add/subtract/multiply overflow) or an `attempt to subtract with overflow` at multiple call sites. Add `MAX_TAB_WIDTH = 2500` (matching `MAX_WIDTH`) and validate the parsed value before constructing `FmtOptions`. A new `TabWidthOutOfRange` error variant mirrors the existing `WidthOutOfRange` path and produces the expected "Numerical result out of range" diagnostic. Fixes uutils#14460
| } | ||
|
|
||
| const MAX_WIDTH: usize = 2500; | ||
| const MAX_TAB_WIDTH: usize = 2500; |
There was a problem hiding this comment.
We don't need two names for the same value: 2500.
|
As I noted at #14460 (comment) , this is not part of GNU. |
| fmt-error-invalid-width = invalid width: {$width} | ||
| fmt-error-width-out-of-range = invalid width: '{$width}': Numerical result out of range | ||
| fmt-error-invalid-tabwidth = Invalid TABWIDTH specification: {$tabwidth} | ||
| fmt-error-tabwidth-out-of-range = invalid tab width: '{$tabwidth}': Numerical result out of range |
There was a problem hiding this comment.
We don't need two aliases for the same output: invalid width: '{$key}': Numerical result out of range
There was a problem hiding this comment.
Sorry. It is slightly different. But I think adding different message is overkill.
| #[error("{}", translate!("fmt-error-invalid-tabwidth", "tabwidth" => .0.quote()))] | ||
| InvalidTabWidth(String), | ||
| #[error("{}", translate!("fmt-error-tabwidth-out-of-range", "tabwidth" => .0))] | ||
| TabWidthOutOfRange(usize), |
| .max(1); | ||
| if tabwidth > MAX_TAB_WIDTH { | ||
| return Err(FmtError::TabWidthOutOfRange(tabwidth).into()); | ||
| } |
There was a problem hiding this comment.
I would use .filter() and .ok_or_else() at 1st chain.
There was a problem hiding this comment.
Can we use let tabwidth @ .. MAX_WIDTH = ... else {?
I am fine keeping it for now |
|
GNU testsuite comparison: |
Problem
fmt --tab-width/-Taccepted anyusizevalue with no upper bound. Large values flowed directly into arithmetic inlinebreak.rsandparasplit.rs, causing panics at 11 distinct sites under overflow-checks builds and undefined behaviour (wrapping arithmetic) in release builds.Examples from #14460:
Fix
const MAX_TAB_WIDTH: usize = 2500(mirrors the existingMAX_WIDTHlimit).MAX_TAB_WIDTHand return a newTabWidthOutOfRangeerror if exceeded.WidthOutOfRangepattern:"invalid tab width: '<N>': Numerical result out of range".en-US.ftlandfr-FR.ftl.Tests
Two new integration tests:
test_fmt_tab_width_too_big— verifies 2501 is rejected with exit code 1 and the expected message.test_fmt_tab_width_at_max— verifies 2500 is accepted.Fixes #14460
🤖 Generated with Claude Code