nl: reject line number width above INT_MAX instead of aborting - #13880
nl: reject line number width above INT_MAX instead of aborting#13880AlejandroCoronadoN wants to merge 1 commit into
Conversation
Merging this PR will degrade performance by 3.37%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | Simulation | df_with_path |
573.7 µs | 704.6 µs | -18.58% |
| ⚡ | Simulation | du_max_depth_balanced_tree[(6, 4, 10)] |
65.2 ms | 61.8 ms | +5.43% |
| ⚡ | Simulation | du_summarize_balanced_tree[(5, 4, 10)] |
16.8 ms | 16 ms | +5.09% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing AlejandroCoronadoN:fix-nl-large-width-overflow (577daa5) with main (822aa83)
Footnotes
-
46 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
|
GNU testsuite comparison: |
| // `" ".repeat(number_width + 1)` abort with a capacity overflow. | ||
| Some(num) if *num > i32::MAX as usize => { | ||
| errs.push(translate!("nl-error-invalid-line-width", "value" => num.to_string())); | ||
| } |
There was a problem hiding this comment.
Please reject it by clap's value_parser instead.
577daa5 to
822aa83
Compare
|
Thanks for the review! Done: I moved the bound to clap's |
Per review feedback, bound -w with clap value_parser to match GNU's C int limit instead of a manual check, so a huge width is rejected at parse time rather than aborting later with a capacity overflow. Adds a regression test. Fixes uutils#13347.
| // Bound the width to a C int like GNU. This rejects a huge width | ||
| // up front instead of letting a later `" ".repeat(width + 1)` | ||
| // abort with a capacity overflow (#13347). | ||
| .value_parser(clap::value_parser!(u64).range(..=i32::MAX as u64)), |
There was a problem hiding this comment.
| .value_parser(clap::value_parser!(u64).range(..=i32::MAX as u64)), | |
| .value_parser(clap::value_parser!(u64).range(1..=i32::MAX as u64)), |
There was a problem hiding this comment.
We don't make stderr 100% same with GNU too.
| .value_parser(clap::value_parser!(usize)), | ||
| // Bound the width to a C int like GNU. This rejects a huge width | ||
| // up front instead of letting a later `" ".repeat(width + 1)` | ||
| // abort with a capacity overflow (#13347). |
There was a problem hiding this comment.
If test is added already, the comment is unnecessary.
| .args(&["-w", "2147483648"]) | ||
| .pipe_in("x\n") | ||
| .fails() | ||
| .stderr_contains("is not in 0..=2147483647"); |
There was a problem hiding this comment.
I would drop .stderr_contains and kep fails only since it is clap's message instead of GNU's one.
2 participants
Summary
nl -w Naccepts an arbitrarily large line-number field width and aborts with acapacity overflow for very large values:
Root cause
The width is only checked for
> 0. A large width then reaches" ".repeat(number_width + 1), which aborts when the requested capacity exceedsisize::MAX.Fix
Match GNU, which bounds
-wto a Cint([1, INT_MAX]): reject a width abovei32::MAXup front with the existing "invalid line number field width" error(exit 1) instead of aborting. Widths up to
i32::MAXare still accepted, as inGNU. Adds a regression test.
Fixes #13347.