Merge #20406: util: Avoid invalid integer negation in FormatMoney and ValueFromAmount

1f05dbd06d896849d16b026bfc3315ee8b73a89f util: Avoid invalid integer negation in ValueFromAmount: make ValueFromAmount(const CAmount& n) well-defined also when n is std::numeric_limits<CAmount>::min() (practicalswift)
7cc75c9ba38e516067e5a4ab84311c62ddddced7 util: Avoid invalid integer negation in FormatMoney: make FormatMoney(const CAmount& n) well-defined also when n is std::numeric_limits<CAmount>::min() (practicalswift)

Pull request description:

  Avoid invalid integer negation in `FormatMoney` and `ValueFromAmount`.

  Fixes #20402.

  Before this patch:

  ```
  $ CC=clang CXX=clang++ ./configure --with-sanitizers=undefined
  $ make -C src/ test/test_bitcoin
  $ src/test/test_bitcoin -t rpc_tests/rpc_format_monetary_values -t util_tests/util_FormatMoney
  core_write.cpp:21:29: runtime error: negation of -9223372036854775808 cannot be represented in type 'CAmount'
    (aka 'long'); cast to an unsigned type to negate this value to itself
  SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior core_write.cpp:21:29 in
  test/rpc_tests.cpp(186): error: in "rpc_tests/rpc_format_monetary_values":
    check ValueFromAmount(std::numeric_limits<CAmount>::min()).write() == "-92233720368.54775808" has failed
    [--92233720368.-54775808 != -92233720368.54775808]
  util/moneystr.cpp:16:34: runtime error: negation of -9223372036854775808 cannot be represented in type 'CAmount'
    (aka 'long'); cast to an unsigned type to negate this value to itself
  SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior util/moneystr.cpp:16:34 in
  test/util_tests.cpp(1188): error: in "util_tests/util_FormatMoney":
    check FormatMoney(std::numeric_limits<CAmount>::min()) == "-92233720368.54775808" has failed
    [--92233720368.-54775808 != -92233720368.54775808]
  ```

  After this patch:

  ```
  $ CC=clang CXX=clang++ ./configure --with-sanitizers=undefined
  $ make -C src/ test/test_bitcoin
  $ src/test/test_bitcoin -t rpc_tests/rpc_format_monetary_values -t util_tests/util_FormatMoney
  ```

ACKs for top commit:
  laanwj:
    re-ACK 1f05dbd06d896849d16b026bfc3315ee8b73a89f

Tree-SHA512: 5aaeb8e2178f1597921f53c12bdfc2f3d5993d10c41658dcd25943e54e8cc2116a411bc71d928f890b33bc0b3761a8ee4449b0532bce41125b6c60692808c8c3
This commit is contained in:
Wladimir J. van der Laan 2021-03-03 19:04:21 +01:00 committed by pasta
parent 1a07e04a0e
commit 666e6e2015
No known key found for this signature in database
GPG Key ID: 52527BEDABE87984
9 changed files with 48 additions and 29 deletions

View File

@ -41,7 +41,7 @@ std::vector<unsigned char> 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);
std::string SighashToStr(unsigned char sighash_type);

View File

@ -15,6 +15,7 @@
#include <univalue.h>
#include <util/check.h>
#include <util/strencodings.h>
#include <util/system.h>
#include <addressindex.h>
#include <spentindex.h>
@ -26,14 +27,17 @@
#include <evo/specialtx.h>
#include <llmq/commitment.h>
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)

View File

@ -24,7 +24,7 @@ class CChainLocksHandler;
}// namespace llmq
// Forward declaration from core_io to get rid of circular dependency
UniValue ValueFromAmount(const CAmount& amount);
UniValue ValueFromAmount(const CAmount amount);
// coinbase transaction
class CCbTx

View File

@ -79,8 +79,8 @@ FUZZ_TARGET_INIT(integer, initialize_integer)
(void)DecompressAmount(u64);
(void)FormatISO8601Date(i64);
(void)FormatISO8601DateTime(i64);
// FormatMoney(i) not defined when i == std::numeric_limits<int64_t>::min()
if (i64 != std::numeric_limits<int64_t>::min()) {
{
if (std::optional<CAmount> parsed = ParseMoney(FormatMoney(i64))) {
assert(parsed.value() == i64);
}
@ -122,8 +122,8 @@ 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<int64_t>::min()
if (i64 != std::numeric_limits<int64_t>::min()) {
{
if (std::optional<CAmount> parsed = ParseMoney(ValueFromAmount(i64).getValStr())) {
assert(parsed.value() == i64);
}

View File

@ -94,16 +94,7 @@ FUZZ_TARGET_INIT(transaction, initialize_transaction)
(void)AreInputsStandard(tx, coins_view_cache);
UniValue u(UniValue::VOBJ);
// ValueFromAmount(i) not defined when i == std::numeric_limits<int64_t>::min()
bool skip_tx_to_univ = false;
for (const CTxOut& txout : tx.vout) {
if (txout.nValue == std::numeric_limits<int64_t>::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);
}

View File

@ -232,6 +232,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<CAmount>::max()).write(), "92233720368.54775807");
BOOST_CHECK_EQUAL(ValueFromAmount(std::numeric_limits<CAmount>::max() - 1).write(), "92233720368.54775806");
BOOST_CHECK_EQUAL(ValueFromAmount(std::numeric_limits<CAmount>::max() - 2).write(), "92233720368.54775805");
BOOST_CHECK_EQUAL(ValueFromAmount(std::numeric_limits<CAmount>::max() - 3).write(), "92233720368.54775804");
// ...
BOOST_CHECK_EQUAL(ValueFromAmount(std::numeric_limits<CAmount>::min() + 3).write(), "-92233720368.54775805");
BOOST_CHECK_EQUAL(ValueFromAmount(std::numeric_limits<CAmount>::min() + 2).write(), "-92233720368.54775806");
BOOST_CHECK_EQUAL(ValueFromAmount(std::numeric_limits<CAmount>::min() + 1).write(), "-92233720368.54775807");
BOOST_CHECK_EQUAL(ValueFromAmount(std::numeric_limits<CAmount>::min()).write(), "-92233720368.54775808");
}
static UniValue ValueFromString(const std::string &str)

View File

@ -1302,6 +1302,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<CAmount>::max()), "92233720368.54775807");
BOOST_CHECK_EQUAL(FormatMoney(std::numeric_limits<CAmount>::max() - 1), "92233720368.54775806");
BOOST_CHECK_EQUAL(FormatMoney(std::numeric_limits<CAmount>::max() - 2), "92233720368.54775805");
BOOST_CHECK_EQUAL(FormatMoney(std::numeric_limits<CAmount>::max() - 3), "92233720368.54775804");
// ...
BOOST_CHECK_EQUAL(FormatMoney(std::numeric_limits<CAmount>::min() + 3), "-92233720368.54775805");
BOOST_CHECK_EQUAL(FormatMoney(std::numeric_limits<CAmount>::min() + 2), "-92233720368.54775806");
BOOST_CHECK_EQUAL(FormatMoney(std::numeric_limits<CAmount>::min() + 1), "-92233720368.54775807");
BOOST_CHECK_EQUAL(FormatMoney(std::numeric_limits<CAmount>::min()), "-92233720368.54775808");
}
BOOST_AUTO_TEST_CASE(util_ParseMoney)

View File

@ -12,13 +12,17 @@
#include <optional>
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:

View File

@ -18,7 +18,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. **/
std::optional<CAmount> ParseMoney(const std::string& str);