uucore: fix parse_size overflow on a large % size - #13742
Conversation
The `%` (percent-of-physical-memory) branch of `Parser::parse` computed
`(number / 100) * total` as an unchecked `u128` multiply. A percentage
that fits in a `u128` but whose product with the total physical memory
does not would overflow, panicking with "attempt to multiply with
overflow" under `-C overflow-checks` (exit 134) instead of reporting a
parse error:
$ du --block-size=9223372036854775808000000000000% /tmp
thread 'main' panicked at parse_size.rs:258:33:
attempt to multiply with overflow
Because `parse_size` is shared code, the panic was reachable from every
util that accepts a size: du, sort, df, ls, split, stdbuf, od, shred,
dd, head and tail.
Use `checked_mul` and return `ParseSizeError::SizeTooBig` on overflow,
matching how the non-percent path already handles the same condition a
few lines below. Callers therefore get the behaviour they already have
for other too-large sizes such as `1Y`: utils that report an error keep
reporting one, and utils using `parse_u64_max`/`parse_u128_max` clamp to
the maximum, as GNU does.
Fixes uutils#13736
Merging this PR will degrade performance by 18.46%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | Simulation | df_with_path |
571 µs | 700.2 µs | -18.46% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing MsfPablo:fix/parse-size-percent-overflow (44f2b0b) with main (0b214cc)
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. ↩
|
please add tests in the programs to make sure we don't regress, thanks |
|
GNU testsuite comparison: |
Summary
Fixes #13736. The
%(percent-of-physical-memory) branch ofParser::parseinsrc/uucore/src/lib/features/parser/parse_size.rscomputed(number / 100) * totalas an uncheckedu128multiply. A percentage that fits in au128but whose product with the total physical memory does not overflows, panicking with "attempt to multiply with overflow" (exit 134) under-C overflow-checksinstead of reporting a parse error:Since
parse_sizeis shared code, this is reachable from every util that accepts a size:du,sort,df,ls,split,stdbuf,od,shred,dd,head, andtail.Fix
Use
checked_muland returnParseSizeError::size_too_bigon overflow, matching how the non-percent path (a few lines below) already handles the same too-large-size condition — per @RenjiSann's comment on the issue. Callers get the same behavior they already have for other too-large sizes like1Y: utils that report an error keep reporting one, and utils usingparse_u64_max/parse_u128_maxclamp to the maximum, matching GNU.Test plan
Added a regression test (
parse_percent_overflow) covering the exact overflow case, gated#[cfg(target_os = "linux")]to match the existing convention for physical-memory-dependent tests in this file (there are several already gated the same way, sincetotal_physical_memory()is platform-specific).cargo test -p uucore --lib --features parser-size— 16/16 existingparse_sizetests pass (the new Linux-gated test doesn't run on my macOS sandbox, but follows the exact same gating as pre-existing tests in this file, so it'll run in CI)cargo clippy -p uucore --features parser-size --lib -- -D warnings— clean