Repository navigation
trap out-of-range exponents in the Dfp string constructor - #341
Merged
Merged
Conversation
Dfp(DfpField, String) discarded the flag returned by round(), so an exponent above MAX_EXP or below the gradual underflow range produced a finite value that every arithmetic operation would have trapped to infinity or zero. The exponent digits were also accumulated in an int without a bound, so "1e2147483648" wrapped negative and toString() on the result looped forever in dfp2sci. Stop accumulating exponent digits once they are past any representable magnitude, store a zero mantissa with a zero exponent, and route the round() flag through dotrap as the arithmetic operations do. The "Next After apache#14" literal 9.99e131078 in DfpTest and DfpDecTest is beyond the largest finite value and only parsed as finite because of this bug; it is corrected to e131071.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #341 +/- ##
============================================
+ Coverage 86.54% 87.22% +0.68%
+ Complexity 9787 91 -9696
============================================
Files 532 499 -33
Lines 35516 33527 -1989
Branches 6194 5850 -344
============================================
- Hits 30738 29245 -1493
+ Misses 3518 3163 -355
+ Partials 1260 1119 -141 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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
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.
Observed on master with a 20 digit DfpField:
The string constructor discards the flag returned by
round(). Every arithmetic operation passes that flag throughdotrap, which turns an exponent above MAX_EXP into infinity and one below the gradual underflow range into zero, so a parsed value can break an invariant the rest of the class relies on and disagree with any operation applied to it. The exponent digits are also accumulated in an int with no bound, so "1e2147483648" wraps negative;dfp2scithen computese == Integer.MIN_VALUE,ae = -estays negative andwhile (p > ae)spins forever oncepreaches 0.The fix stays in the constructor: stop accumulating exponent digits once the value is past anything representable, store a zero mantissa with a zero exponent as
adddoes, and hand theround()flag todotrap, copying the trapped result back the way the double constructor already does. Values inside the representable range, including gradual underflow, are unchanged.Two existing tests ("Next After #14" in DfpTest and DfpDecTest) parsed
9.99...e131078, which is beyond the largest finite value9.99...e131071and only came out finite because of this bug; the literal is corrected to e131071 and the test still exercises the overflow onnextAfter. The new DfpTest case fails on master at its first assertion.mvn -pl commons-math-legacy-core installpasses on JDK 21 (checkstyle, SpotBugs, PMD, 291 tests), and the Dfp based tests in commons-math-legacy pass against the patched module.