Skip to content

fix(primitives): return Err on parse_units multiply overflow - #1156

Merged
DaniPopes merged 1 commit into
alloy-rs:mainfrom
Kropiunig:fix/parse-units-mul-overflow
Jul 20, 2026
Merged

fix(primitives): return Err on parse_units multiply overflow#1156
DaniPopes merged 1 commit into
alloy-rs:mainfrom
Kropiunig:fix/parse-units-mul-overflow

Conversation

@Kropiunig

Copy link
Copy Markdown
Contributor

Motivation

ParseUnits::parse_units returns a Result and is documented/tested to reject out-of-range inputs with an Err — see the existing assert!(parse_units("1", 80).is_err(), "overflow") test. It correctly guards the 10^exponent factor with checked_pow, but then folds that factor into the parsed mantissa with an unchecked *=:

let mut a_uint = U256::from_str_radix(amount, 10)?;
a_uint *= U256::from(10)
    .checked_pow(U256::from(exponent - dec_len))
    .ok_or(UnitsError::ParseSigned(ParseSignedError::IntegerOverflow))?;

When 10^exponent fits in the target integer (so checked_pow succeeds) but mantissa * 10^exponent does not, the overflow is silently swallowed:

  • Unsigned U256: ruint implements Mul/MulAssign via wrapping_mul, so the multiply wraps to a wrong value in every build profile. For example parse_units("2", 77)10^77 < U256::MAX < 2·10^77 — returns Ok(<wrapped garbage>) instead of Err.
  • Signed I256: Signed::mul uses debug_assert!(!overflow), so the equivalent input panics in debug builds and silently wraps in release. For example parse_units("-6", 76) (10^76 < I256::MAX < 6·10^76).

Both outcomes contradict the intended contract (graceful Err on overflow).

Solution

Replace the unchecked *= with checked_mul on both the unsigned and signed paths, propagating the same UnitsError::ParseSigned(ParseSignedError::IntegerOverflow) already used for the checked_pow guard. Minimal change, no new error variants.

Adds a regression test (test_parse_units_mul_overflow) covering the unsigned wrap and signed overflow paths. It fails before this change (unsigned returns Ok; signed panics in debug) and passes after.

Testing

cargo test -p alloy-primitives --lib utils::units — all pass. cargo +nightly fmt --check clean.

`ParseUnits::parse_units` guards `10^exponent` with `checked_pow` but then
multiplies it into the parsed mantissa with an unchecked `*=`. When
`10^exponent` fits in the target integer but `mantissa * 10^exponent` does
not, this overflow is not caught:

- unsigned `U256`: ruint's `Mul`/`MulAssign` is `wrapping_mul`, so the result
  silently wraps to a wrong value in every profile (e.g. `parse_units("2", 77)`
  returns `Ok` with a garbage value instead of an error);
- signed `I256`: `Signed::mul` uses `debug_assert!(!overflow)`, so it panics in
  debug builds and silently wraps in release.

Both contradict the documented behavior for out-of-range inputs, which is a
graceful `Err` (as asserted by the existing `parse_units("1", 80).is_err()`
test). Replace the unchecked `*=` with `checked_mul`, propagating the same
`IntegerOverflow` error already used for the `checked_pow` guard.

Adds a regression test covering the unsigned wrap and signed overflow paths.

Signed-off-by: Kropiunig <48442031+Kropiunig@users.noreply.github.com>
@github-project-automation github-project-automation Bot moved this to Reviewed in Alloy Jul 20, 2026
@DaniPopes
DaniPopes enabled auto-merge (squash) July 20, 2026 20:49
@DaniPopes
DaniPopes merged commit a5df2f9 into alloy-rs:main Jul 20, 2026
31 checks passed
@github-project-automation github-project-automation Bot moved this from Reviewed to Done in Alloy Jul 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

2 participants