diff --git a/src/libxrpl/protocol/STNumber.cpp b/src/libxrpl/protocol/STNumber.cpp index 5944d4cd6b..c37e6a9c19 100644 --- a/src/libxrpl/protocol/STNumber.cpp +++ b/src/libxrpl/protocol/STNumber.cpp @@ -257,49 +257,42 @@ numberFromJson(SField const& field, json::Value const& value) Number const num{parts.negative, parts.mantissa, parts.exponent, Number::Normalized{}}; - // Bring the ranges of parts and num into the same range. Then check that they match. - // If they do not, then the value has been rounded one way or another, and should not be - // used, because it may lead to an unexpected result. - auto const negative = num.mantissa() < 0; - auto mantissa = Number::externalToInternal(num.mantissa()); - auto exponent = num.exponent(); + // Canonicalize "parts" and "num" with each other by getting rid of trailing 0s until either the + // exponents match, or there are no more 0s. If the two results don't match exactly, then the + // value has been rounded one way or another, and should not be used, because it may lead to an + // unexpected result. canonicalizeParts is not to be confused with Number::canonicalize, because + // they're have completely different goals. + auto canonicalizeParts = [](NumberParts p, int otherExponent) { + if (p.mantissa == 0) + return NumberParts{}; - // Number zero has a special representation, so if the result is 0, nothing else needs to match. - // i.e. the Json string could be negative 0 ("-0") or a very "large" 0 ("0e99"), but they all - // represent 0. - bool const isZero = num == beast::kZero && parts.mantissa == 0; - XRPL_ASSERT(negative == parts.negative || isZero, "numberFromJson : signs agree"); - static_assert( - // NOLINTNEXTLINE(misc-redundant-expression) - std::is_same_v); - static_assert( - // NOLINTNEXTLINE(misc-redundant-expression) - std::is_same_v); - if (!isZero) + while (p.exponent < otherExponent && p.mantissa % 10 == 0) + { + p.mantissa /= 10; + ++p.exponent; + } + + return p; + }; + + auto const numberMantissa = num.mantissa(); + auto const numberExponent = num.exponent(); + + auto const canonicalParts = canonicalizeParts(parts, numberExponent); + + auto const canonicalNum = canonicalizeParts( + NumberParts{ + .mantissa = Number::externalToInternal(numberMantissa), + .exponent = numberExponent, + .negative = numberMantissa < 0, + }, + canonicalParts.exponent); + + if (canonicalParts.mantissa != canonicalNum.mantissa || + canonicalParts.exponent != canonicalNum.exponent || + canonicalParts.negative != canonicalNum.negative) { - auto constexpr minMantissa = Number::kMaxRep / 10; - while (parts.exponent > exponent && parts.mantissa < minMantissa) - { - parts.mantissa *= 10; - --parts.exponent; - } - while (parts.exponent < exponent && mantissa < minMantissa) - { - // This should only happen in the edge case where the internal mantissa > kMaxRep - mantissa *= 10; - --exponent; - } - while (parts.exponent < exponent && parts.mantissa % 10 == 0) - { - // LCOV_EXCL_START - // This will probably never be needed - parts.mantissa /= 10; - ++parts.exponent; - // LCOV_EXCL_STOP - } - - if (negative != parts.negative || mantissa != parts.mantissa || exponent != parts.exponent) - Throw("number can not be represented"); + Throw("number cannot be represented"); } return STNumber{field, num}; diff --git a/src/test/app/LoanBroker_test.cpp b/src/test/app/LoanBroker_test.cpp index 73fe8dc740..2ec3e5573c 100644 --- a/src/test/app/LoanBroker_test.cpp +++ b/src/test/app/LoanBroker_test.cpp @@ -1466,6 +1466,7 @@ class LoanBroker_test : public beast::unit_test::Suite std::string(e.what()) == "invalidParamsField 'tx_json.DebtMaximum' has invalid data."); } + env.setParseFailureExpected(false); if (Number::getMantissaScale() >= MantissaRange::MantissaScale::Large330) { diff --git a/src/test/app/Vault_test.cpp b/src/test/app/Vault_test.cpp index 1870a30e8a..57d5646886 100644 --- a/src/test/app/Vault_test.cpp +++ b/src/test/app/Vault_test.cpp @@ -5641,6 +5641,7 @@ class Vault_test : public beast::unit_test::Suite env(tx); env.close(); + // There are several parse failures expected in this function, so just disable it once. env.setParseFailureExpected(true); try { @@ -5786,6 +5787,9 @@ class Vault_test : public beast::unit_test::Suite env(tx); env.close(); + // Since several tests are expected to have parser failures, leave this flag set for the + // remainder of this function. + env.setParseFailureExpected(true); try { tx[sfAssetsMaximum] = maxInt64Plus2; diff --git a/src/test/protocol/STNumber_test.cpp b/src/test/protocol/STNumber_test.cpp index 7339df7c04..08b4526251 100644 --- a/src/test/protocol/STNumber_test.cpp +++ b/src/test/protocol/STNumber_test.cpp @@ -101,6 +101,39 @@ struct STNumber_test : public beast::unit_test::Suite BEAST_EXPECT(numberFromJson(sfNumber, "-0.000e6") == STNumber(sfNumber, 0)); { + auto const parseNumber = [](std::string const& boundary) { + return numberFromJson(sfNumber, boundary); + }; + auto const expectParseThrows = [this, &parseNumber](std::string const& boundary) { + try + { + auto const bad = parseNumber(boundary); + fail(); + } + catch (std::exception const& e) + { + BEAST_EXPECT(std::string(e.what()) == "number cannot be represented"); + } + }; + + // Small rejects this; large scales parse it as 9223372036854775800e-1. + auto constexpr positiveBoundary = "922337203685477580"; + auto constexpr negativeBoundary = "-922337203685477580"; + if (Number::getMantissaScale() == MantissaRange::MantissaScale::Small) + { + expectParseThrows(positiveBoundary); + expectParseThrows(negativeBoundary); + } + else + { + BEAST_EXPECT( + parseNumber(positiveBoundary) == + STNumber(sfNumber, Number{922'337'203'685'477'580, 0})); + BEAST_EXPECT( + parseNumber(negativeBoundary) == + STNumber(sfNumber, Number{-922'337'203'685'477'580, 0})); + } + NumberRoundModeGuard const mg(Number::RoundingMode::TowardsZero); // maxint64 9,223,372,036,854,775,807 auto const maxInt = std::to_string(std::numeric_limits::max()); @@ -110,41 +143,17 @@ struct STNumber_test : public beast::unit_test::Suite { // min/maxInt can't be exactly represented with the small mantissa, so they // don't parse, and are expected to throw. - try - { - auto const bad = numberFromJson(sfNumber, maxInt); - fail(); - } - catch (std::exception const& e) - { - BEAST_EXPECT(std::string(e.what()) == "number can not be represented"); - } - try - { - auto const bad = numberFromJson(sfNumber, minInt); - fail(); - } - catch (std::exception const& e) - { - BEAST_EXPECT(std::string(e.what()) == "number can not be represented"); - } + expectParseThrows(maxInt); + expectParseThrows(minInt); } else { // with large mantissas, maxint is fine BEAST_EXPECT( - numberFromJson(sfNumber, maxInt) == + parseNumber(maxInt) == STNumber(sfNumber, Number{9'223'372'036'854'775'807, 0})); // but minint's mantissa is > kMaxRep, and so rounds, and thus can't be parsed - try - { - auto const bad = numberFromJson(sfNumber, minInt); - fail(); - } - catch (std::exception const& e) - { - BEAST_EXPECT(std::string(e.what()) == "number can not be represented"); - } + expectParseThrows(minInt); } }