UB in `SizeValue::mul` when multiplying by zero

[varies-rust]

Hi maintainers, and thanks for your work on encase.

Our tool detects a minor Undefined Behavior case in SizeValue::mul when the multiplier is 0.

Minimal reproducer

fn test_function() {
    let _local0 = encase::internal::SizeValue::new(1);
    let _ = encase::internal::SizeValue::mul(_local0, 0);
}

Miri output

Undefined Behavior: entering unreachable code
  --> encase-0.12.0/src/core/size_value.rs:35:31
   |
35 |                 Self(unsafe { NonZeroU64::new_unchecked(val) })
   |                               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Undefined Behavior occurred here

Location

src/core/size_value.rs line 35 (NonZeroU64::new_unchecked(val)).

Expected behavior

SizeValue::mul should handle zero safely (e.g. return an error / None / panic explicitly), but should not invoke UB.

Actual behavior

When multiplication results in 0, NonZeroU64::new_unchecked(0) is reached, which is UB.

Suggested fix

Add an explicit zero check before constructing NonZeroU64, or replace new_unchecked with a checked path (NonZeroU64::new(...)) and handle None accordingly.

If helpful, I can also open a PR with a patch and regression test.

[teoxoy]

Thanks for the report! We should call Self::new() to avoid this. A patch and test would be great!

It's worth noting that while this is an issue, all the callers of this function make sure to not pass 0. SizeValue is pub and people might use it but I hope the internal module that it's exposed in discourages people to use it (it's only pub because the derive macro needs it).

[varies-rust]

Thanks for the quick response and we fully understand your visibility handling of SizeValue.

Given that zero is not allowed to be passed in, a simple debug_assert can be added to the beginning of the function, which would not affect the release performance.

[teoxoy]

Since it's UB and the code in question is not in a hot path it should be fine to panic via Self::new().

[varies-rust]

The panic in Self::new() cannot cover case of rhs = 0 for SizeValue::mul. Thus, a simple debug_assert(rhs > 0) can be added to mul to handle the UB.

If you think there is no need for the modification, feel free to close this issue :)

[teoxoy]

It can though, multiplying by 0 will yield 0. Replacing Self(unsafe { NonZeroU64::new_unchecked(val) }) with Self::new(val) would fix the issue.

[varies-rust]

Sorry for my misunderstanding. The replacement strategy is effective :) 👍