diff --git a/include/xrpl/basics/Number.h b/include/xrpl/basics/Number.h index 950e8b8c6d..87a99683ab 100644 --- a/include/xrpl/basics/Number.h +++ b/include/xrpl/basics/Number.h @@ -871,12 +871,26 @@ Number::normalizeToRange(T minMantissa, T maxMantissa) const int exponent = exponent_; if constexpr (std::is_unsigned_v) + { XRPL_ASSERT_PARTS( !negative, "xrpl::Number::normalizeToRange", "Number is non-negative for unsigned range."); + // To avoid logical errors in release builds, throw if the Number is + // negative for an unsigned range. + if (negative) + throw std::runtime_error( + "Number::normalizeToRange: Number is negative for " + "unsigned range."); + } Number::normalize(negative, mantissa, exponent, minMantissa, maxMantissa); + // Cast mantissa to signed type first (if T is a signed type) to avoid + // unsigned integer overflow when multiplying by negative sign + T signedMantissa = static_cast(mantissa); + if (negative) + signedMantissa = -signedMantissa; + return std::make_pair(signedMantissa, exponent); return std::make_pair(sign * static_cast(mantissa), exponent); } diff --git a/include/xrpl/protocol/STAmount.h b/include/xrpl/protocol/STAmount.h index 4d86aed2ec..0c86bc5747 100644 --- a/include/xrpl/protocol/STAmount.h +++ b/include/xrpl/protocol/STAmount.h @@ -568,6 +568,10 @@ STAmount::fromNumber(A const& a, Number const& number) return STAmount{asset, intValue, 0, negative}; } + XRPL_ASSERT_PARTS( + working.signum() >= 0, + "ripple::STAmount::fromNumber", + "non-negative Number to normalize"); auto const [mantissa, exponent] = working.normalizeToRange(cMinValue, cMaxValue); diff --git a/src/libxrpl/basics/Number.cpp b/src/libxrpl/basics/Number.cpp index f570bc2bb6..5a1d84c8d3 100644 --- a/src/libxrpl/basics/Number.cpp +++ b/src/libxrpl/basics/Number.cpp @@ -11,6 +11,7 @@ #include #include #include +#include #include #include @@ -102,7 +103,7 @@ public: int& exponent, internalrep const& minMantissa, internalrep const& maxMantissa, - std::string location); + std::string_view location); // Modify the result to the correctly rounded value template @@ -115,7 +116,7 @@ public: // Modify the result to the correctly rounded value void - doRound(rep& drops, std::string location); + doRound(rep& drops, std::string_view location); private: void @@ -245,7 +246,7 @@ Number::Guard::doRoundUp( int& exponent, internalrep const& minMantissa, internalrep const& maxMantissa, - std::string location) + std::string_view location) { auto r = round(); if (r == 1 || (r == 0 && (mantissa & 1) == 1)) @@ -261,7 +262,7 @@ Number::Guard::doRoundUp( } bringIntoRange(negative, mantissa, exponent, minMantissa); if (exponent > maxExponent) - throw std::overflow_error(location); + throw std::overflow_error(std::string{location}); } template @@ -287,7 +288,7 @@ Number::Guard::doRoundDown( // Modify the result to the correctly rounded value void -Number::Guard::doRound(rep& drops, std::string location) +Number::Guard::doRound(rep& drops, std::string_view location) { auto r = round(); if (r == 1 || (r == 0 && (drops & 1) == 1)) @@ -302,7 +303,7 @@ Number::Guard::doRound(rep& drops, std::string location) // or "(maxRep + 1) / 10", neither of which will round up when // converting to rep, though the latter might overflow _before_ // rounding. - throw std::overflow_error(location); // LCOV_EXCL_LINE + throw std::overflow_error(std::string{location}); // LCOV_EXCL_LINE } ++drops; } @@ -322,17 +323,10 @@ Number::externalToInternal(rep mantissa) // If the mantissa is already positive, just return it if (mantissa >= 0) return mantissa; - // If the mantissa is negative, but fits within the positive range of rep, - // return it negated - if (mantissa >= -std::numeric_limits::max()) - return -mantissa; - // If the mantissa doesn't fit within the positive range, convert to - // int128_t, negate that, and cast it back down to the internalrep - // In practice, this is only going to cover the case of - // std::numeric_limits::min(). - int128_t temp = mantissa; - return static_cast(-temp); + // Cast to unsigned before negating to avoid undefined behavior + // when v == INT64_MIN (negating INT64_MIN in signed is UB) + return -static_cast(mantissa); } /** Breaks down the number into components, potentially de-normalizing it. diff --git a/src/test/basics/Number_test.cpp b/src/test/basics/Number_test.cpp index 314e0f58e6..4da0621439 100644 --- a/src/test/basics/Number_test.cpp +++ b/src/test/basics/Number_test.cpp @@ -1884,6 +1884,206 @@ public: } } + void + testNormalizeToRange() + { + // Test edge-cases of normalizeToRange + auto const scale = Number::getMantissaScale(); + testcase << "normalizeToRange " << to_string(scale); + + auto test = [this]( + Number const& n, + auto const rangeMin, + auto const rangeMax, + auto const expectedMantissa, + auto const expectedExponent, + auto const line) { + auto const normalized = n.normalizeToRange(rangeMin, rangeMax); + BEAST_EXPECTS( + normalized.first == expectedMantissa, + "Number " + to_string(n) + " scaled to " + + std::to_string(rangeMax) + + ". Expected mantissa:" + std::to_string(expectedMantissa) + + ", got: " + std::to_string(normalized.first) + " @ " + + std::to_string(line)); + BEAST_EXPECTS( + normalized.second == expectedExponent, + "Number " + to_string(n) + " scaled to " + + std::to_string(rangeMax) + + ". Expected exponent:" + std::to_string(expectedExponent) + + ", got: " + std::to_string(normalized.second) + " @ " + + std::to_string(line)); + }; + + std::int64_t constexpr iRangeMin = 100; + std::int64_t constexpr iRangeMax = 999; + + std::uint64_t constexpr uRangeMin = 100; + std::uint64_t constexpr uRangeMax = 999; + + constexpr static MantissaRange largeRange{MantissaRange::large}; + + std::int64_t constexpr iBigMin = largeRange.min; + std::int64_t constexpr iBigMax = largeRange.max; + + auto const testSuite = [&](Number const& n, + auto const expectedSmallMantissa, + auto const expectedSmallExponent, + auto const expectedLargeMantissa, + auto const expectedLargeExponent, + auto const line) { + test( + n, + iRangeMin, + iRangeMax, + expectedSmallMantissa, + expectedSmallExponent, + line); + test( + n, + iBigMin, + iBigMax, + expectedLargeMantissa, + expectedLargeExponent, + line); + + // Only test non-negative. testing a negative number with an + // unsigned range will assert, and asserts can't be tested. + if (n.signum() >= 0) + { + test( + n, + uRangeMin, + uRangeMax, + expectedSmallMantissa, + expectedSmallExponent, + line); + test( + n, + largeRange.min, + largeRange.max, + expectedLargeMantissa, + expectedLargeExponent, + line); + } + }; + + { + // zero + Number const n{0}; + + testSuite( + n, + 0, + std::numeric_limits::lowest(), + 0, + std::numeric_limits::lowest(), + __LINE__); + } + { + // Small positive number + Number const n{2}; + + testSuite(n, 200, -2, 2'000'000'000'000'000'000, -18, __LINE__); + } + { + // Negative number + Number const n{-2}; + + testSuite(n, -200, -2, -2'000'000'000'000'000'000, -18, __LINE__); + } + { + // Biggest valid mantissa + Number const n{Number::largestMantissa, 0, Number::normalized{}}; + + if (scale == MantissaRange::small) + // With the small mantissa range, the value rounds up. Because + // it rounds up, when scaling up to the full int64 range, it + // can't go over the max, so it is one digit smaller than the + // full value. + testSuite(n, 922, 16, 922'337'203'685'477'600, 1, __LINE__); + else + testSuite(n, 922, 16, Number::largestMantissa, 0, __LINE__); + } + { + // Biggest valid mantissa + 1 + Number const n{ + Number::largestMantissa + 1, 0, Number::normalized{}}; + + if (scale == MantissaRange::small) + // With the small mantissa range, the value rounds up. Because + // it rounds up, when scaling up to the full int64 range, it + // can't go over the max, so it is one digit smaller than the + // full value. + testSuite(n, 922, 16, 922'337'203'685'477'600, 1, __LINE__); + else + testSuite(n, 922, 16, 922'337'203'685'477'581, 1, __LINE__); + } + { + // Biggest valid mantissa + 2 + Number const n{ + Number::largestMantissa + 2, 0, Number::normalized{}}; + + if (scale == MantissaRange::small) + // With the small mantissa range, the value rounds up. Because + // it rounds up, when scaling up to the full int64 range, it + // can't go over the max, so it is one digit smaller than the + // full value. + testSuite(n, 922, 16, 922'337'203'685'477'600, 1, __LINE__); + else + testSuite(n, 922, 16, 922'337'203'685'477'581, 1, __LINE__); + } + { + // Biggest valid mantissa + 3 + Number const n{ + Number::largestMantissa + 3, 0, Number::normalized{}}; + + if (scale == MantissaRange::small) + // With the small mantissa range, the value rounds up. Because + // it rounds up, when scaling up to the full int64 range, it + // can't go over the max, so it is one digit smaller than the + // full value. + testSuite(n, 922, 16, 922'337'203'685'477'600, 1, __LINE__); + else + testSuite(n, 922, 16, 922'337'203'685'477'581, 1, __LINE__); + } + { + // int64 min + Number const n{std::numeric_limits::min(), 0}; + + if (scale == MantissaRange::small) + testSuite(n, -922, 16, -922'337'203'685'477'600, 1, __LINE__); + else + testSuite(n, -922, 16, -922'337'203'685'477'581, 1, __LINE__); + } + { + // int64 min + 1 + Number const n{std::numeric_limits::min() + 1, 0}; + + if (scale == MantissaRange::small) + testSuite(n, -922, 16, -922'337'203'685'477'600, 1, __LINE__); + else + testSuite(n, -922, 16, -9'223'372'036'854'775'807, 0, __LINE__); + } + { + // int64 min - 1 + // Need to cast to uint, even though we're dealing with a negative + // number to avoid overflow and UB + Number const n{ + true, + -static_cast( + std::numeric_limits::min()) + + 1, + 0, + Number::normalized{}}; + + if (scale == MantissaRange::small) + testSuite(n, -922, 16, -922'337'203'685'477'600, 1, __LINE__); + else + testSuite(n, -922, 16, -922'337'203'685'477'581, 1, __LINE__); + } + } + void run() override { @@ -1911,6 +2111,7 @@ public: test_truncate(); testRounding(); testInt64(); + testNormalizeToRange(); } } };