From 7cc75c9ba38e516067e5a4ab84311c62ddddced7 Mon Sep 17 00:00:00 2001 From: practicalswift Date: Mon, 16 Nov 2020 15:42:36 +0000 Subject: [PATCH 1/2] util: Avoid invalid integer negation in FormatMoney: make FormatMoney(const CAmount& n) well-defined also when n is std::numeric_limits::min() --- src/test/fuzz/integer.cpp | 3 +-- src/test/util_tests.cpp | 10 ++++++++++ src/util/moneystr.cpp | 12 ++++++++---- src/util/moneystr.h | 2 +- 4 files changed, 20 insertions(+), 7 deletions(-) diff --git a/src/test/fuzz/integer.cpp b/src/test/fuzz/integer.cpp index ac83d91ea03..30b0fb5bfe5 100644 --- a/src/test/fuzz/integer.cpp +++ b/src/test/fuzz/integer.cpp @@ -84,8 +84,7 @@ FUZZ_TARGET_INIT(integer, initialize_integer) (void)DecompressAmount(u64); (void)FormatISO8601Date(i64); (void)FormatISO8601DateTime(i64); - // FormatMoney(i) not defined when i == std::numeric_limits::min() - if (i64 != std::numeric_limits::min()) { + { int64_t parsed_money; if (ParseMoney(FormatMoney(i64), parsed_money)) { assert(parsed_money == i64); diff --git a/src/test/util_tests.cpp b/src/test/util_tests.cpp index 845854bd4b8..5a46002a79d 100644 --- a/src/test/util_tests.cpp +++ b/src/test/util_tests.cpp @@ -1180,6 +1180,16 @@ BOOST_AUTO_TEST_CASE(util_FormatMoney) BOOST_CHECK_EQUAL(FormatMoney(COIN/1000000), "0.000001"); BOOST_CHECK_EQUAL(FormatMoney(COIN/10000000), "0.0000001"); BOOST_CHECK_EQUAL(FormatMoney(COIN/100000000), "0.00000001"); + + BOOST_CHECK_EQUAL(FormatMoney(std::numeric_limits::max()), "92233720368.54775807"); + BOOST_CHECK_EQUAL(FormatMoney(std::numeric_limits::max() - 1), "92233720368.54775806"); + BOOST_CHECK_EQUAL(FormatMoney(std::numeric_limits::max() - 2), "92233720368.54775805"); + BOOST_CHECK_EQUAL(FormatMoney(std::numeric_limits::max() - 3), "92233720368.54775804"); + // ... + BOOST_CHECK_EQUAL(FormatMoney(std::numeric_limits::min() + 3), "-92233720368.54775805"); + BOOST_CHECK_EQUAL(FormatMoney(std::numeric_limits::min() + 2), "-92233720368.54775806"); + BOOST_CHECK_EQUAL(FormatMoney(std::numeric_limits::min() + 1), "-92233720368.54775807"); + BOOST_CHECK_EQUAL(FormatMoney(std::numeric_limits::min()), "-92233720368.54775808"); } BOOST_AUTO_TEST_CASE(util_ParseMoney) diff --git a/src/util/moneystr.cpp b/src/util/moneystr.cpp index 1bc8d02eab1..3f9ce7dce4c 100644 --- a/src/util/moneystr.cpp +++ b/src/util/moneystr.cpp @@ -9,13 +9,17 @@ #include #include -std::string FormatMoney(const CAmount& n) +std::string FormatMoney(const CAmount n) { // Note: not using straight sprintf here because we do NOT want // localized number formatting. - int64_t n_abs = (n > 0 ? n : -n); - int64_t quotient = n_abs/COIN; - int64_t remainder = n_abs%COIN; + static_assert(COIN > 1); + int64_t quotient = n / COIN; + int64_t remainder = n % COIN; + if (n < 0) { + quotient = -quotient; + remainder = -remainder; + } std::string str = strprintf("%d.%08d", quotient, remainder); // Right-trim excess zeros before the decimal point: diff --git a/src/util/moneystr.h b/src/util/moneystr.h index da7f673cda1..2aedbee358c 100644 --- a/src/util/moneystr.h +++ b/src/util/moneystr.h @@ -17,7 +17,7 @@ /* Do not use these functions to represent or parse monetary amounts to or from * JSON but use AmountFromValue and ValueFromAmount for that. */ -std::string FormatMoney(const CAmount& n); +std::string FormatMoney(const CAmount n); /** Parse an amount denoted in full coins. E.g. "0.0034" supplied on the command line. **/ [[nodiscard]] bool ParseMoney(const std::string& str, CAmount& nRet); From 1f05dbd06d896849d16b026bfc3315ee8b73a89f Mon Sep 17 00:00:00 2001 From: practicalswift Date: Mon, 16 Nov 2020 16:44:50 +0000 Subject: [PATCH 2/2] util: Avoid invalid integer negation in ValueFromAmount: make ValueFromAmount(const CAmount& n) well-defined also when n is std::numeric_limits::min() --- src/core_io.h | 2 +- src/core_write.cpp | 17 ++++++++++------- src/test/fuzz/integer.cpp | 3 +-- src/test/fuzz/transaction.cpp | 15 +++------------ src/test/rpc_tests.cpp | 10 ++++++++++ 5 files changed, 25 insertions(+), 22 deletions(-) diff --git a/src/core_io.h b/src/core_io.h index 5469a760eec..01340ae2ee0 100644 --- a/src/core_io.h +++ b/src/core_io.h @@ -40,7 +40,7 @@ std::vector ParseHexUV(const UniValue& v, const std::string& strN int ParseSighashString(const UniValue& sighash); // core_write.cpp -UniValue ValueFromAmount(const CAmount& amount); +UniValue ValueFromAmount(const CAmount amount); std::string FormatScript(const CScript& script); std::string EncodeHexTx(const CTransaction& tx, const int serializeFlags = 0); std::string SighashToStr(unsigned char sighash_type); diff --git a/src/core_write.cpp b/src/core_write.cpp index a3902863d6e..d3034ae25d5 100644 --- a/src/core_write.cpp +++ b/src/core_write.cpp @@ -14,17 +14,20 @@ #include #include #include -#include #include +#include -UniValue ValueFromAmount(const CAmount& amount) +UniValue ValueFromAmount(const CAmount amount) { - bool sign = amount < 0; - int64_t n_abs = (sign ? -amount : amount); - int64_t quotient = n_abs / COIN; - int64_t remainder = n_abs % COIN; + static_assert(COIN > 1); + int64_t quotient = amount / COIN; + int64_t remainder = amount % COIN; + if (amount < 0) { + quotient = -quotient; + remainder = -remainder; + } return UniValue(UniValue::VNUM, - strprintf("%s%d.%08d", sign ? "-" : "", quotient, remainder)); + strprintf("%s%d.%08d", amount < 0 ? "-" : "", quotient, remainder)); } std::string FormatScript(const CScript& script) diff --git a/src/test/fuzz/integer.cpp b/src/test/fuzz/integer.cpp index 30b0fb5bfe5..5bc99ddcb9b 100644 --- a/src/test/fuzz/integer.cpp +++ b/src/test/fuzz/integer.cpp @@ -131,8 +131,7 @@ FUZZ_TARGET_INIT(integer, initialize_integer) (void)SipHashUint256Extra(u64, u64, u256, u32); (void)ToLower(ch); (void)ToUpper(ch); - // ValueFromAmount(i) not defined when i == std::numeric_limits::min() - if (i64 != std::numeric_limits::min()) { + { int64_t parsed_money; if (ParseMoney(ValueFromAmount(i64).getValStr(), parsed_money)) { assert(parsed_money == i64); diff --git a/src/test/fuzz/transaction.cpp b/src/test/fuzz/transaction.cpp index 13ae4507568..41e16874055 100644 --- a/src/test/fuzz/transaction.cpp +++ b/src/test/fuzz/transaction.cpp @@ -100,16 +100,7 @@ FUZZ_TARGET_INIT(transaction, initialize_transaction) (void)IsWitnessStandard(tx, coins_view_cache); UniValue u(UniValue::VOBJ); - // ValueFromAmount(i) not defined when i == std::numeric_limits::min() - bool skip_tx_to_univ = false; - for (const CTxOut& txout : tx.vout) { - if (txout.nValue == std::numeric_limits::min()) { - skip_tx_to_univ = true; - } - } - if (!skip_tx_to_univ) { - TxToUniv(tx, /* hashBlock */ {}, u); - static const uint256 u256_max(uint256S("ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff")); - TxToUniv(tx, u256_max, u); - } + TxToUniv(tx, /* hashBlock */ {}, u); + static const uint256 u256_max(uint256S("ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff")); + TxToUniv(tx, u256_max, u); } diff --git a/src/test/rpc_tests.cpp b/src/test/rpc_tests.cpp index b54cbb3f002..810665877d2 100644 --- a/src/test/rpc_tests.cpp +++ b/src/test/rpc_tests.cpp @@ -174,6 +174,16 @@ BOOST_AUTO_TEST_CASE(rpc_format_monetary_values) BOOST_CHECK_EQUAL(ValueFromAmount(COIN/1000000).write(), "0.00000100"); BOOST_CHECK_EQUAL(ValueFromAmount(COIN/10000000).write(), "0.00000010"); BOOST_CHECK_EQUAL(ValueFromAmount(COIN/100000000).write(), "0.00000001"); + + BOOST_CHECK_EQUAL(ValueFromAmount(std::numeric_limits::max()).write(), "92233720368.54775807"); + BOOST_CHECK_EQUAL(ValueFromAmount(std::numeric_limits::max() - 1).write(), "92233720368.54775806"); + BOOST_CHECK_EQUAL(ValueFromAmount(std::numeric_limits::max() - 2).write(), "92233720368.54775805"); + BOOST_CHECK_EQUAL(ValueFromAmount(std::numeric_limits::max() - 3).write(), "92233720368.54775804"); + // ... + BOOST_CHECK_EQUAL(ValueFromAmount(std::numeric_limits::min() + 3).write(), "-92233720368.54775805"); + BOOST_CHECK_EQUAL(ValueFromAmount(std::numeric_limits::min() + 2).write(), "-92233720368.54775806"); + BOOST_CHECK_EQUAL(ValueFromAmount(std::numeric_limits::min() + 1).write(), "-92233720368.54775807"); + BOOST_CHECK_EQUAL(ValueFromAmount(std::numeric_limits::min()).write(), "-92233720368.54775808"); } static UniValue ValueFromString(const std::string &str)