Fix silent overflow in from_string() decimal parsing - #370
Open
cavdarahmet wants to merge 1 commit into
Open
Conversation
The overflow check only validated the final addition (`x < d`), missing overflow in the preceding multiplication by 10. As a result, most out-of-range decimal inputs of the maximum digit length wrapped around silently instead of throwing, for every uint<N> width (128/192/256/384/512). Replace it with a pre-multiplication bound check.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #343.
The decimal-parsing loop in
from_string()only checked for overflowafter the fact, via
if (x < d)— which only catches overflow in thefinal addition, not in the preceding
x * Int{10}multiplication. Foran input at the maximum valid digit count (
digits10), most valuesabove
max()wrap around silently instead of throwingstd::out_of_range.I verified this isn't limited to the one input in the issue:
cross-checking ~2200 randomized boundary-length decimal strings against
Python's arbitrary-precision arithmetic as an oracle, 89% of the
out-of-range cases silently wrapped with the current code, for
uint256. The same bug reproduces identically foruint128.Fix: check the bound before multiplying (
x > (max() - d) / 10)instead of inspecting the result afterward — this can't be fooled by
wraparound since it never lets the multiplication overflow in the
first place.
Added a regression test parameterized over all five
uint<N>widths(128/192/256/384/512): for each type, a decimal string of
max()'sdigit length filled with 9s must be rejected. Confirmed this test
fails (silently accepts invalid input) on all five widths without the
fix, and passes with it.