Page MenuHomePhabricator

util: Avoid invalid integer negation in FormatMoney and its opposite
ClosedPublic

Authored by Fabien on Wed, Aug 5, 14:09.

Details

Summary

This fixes a UB when amount is min int64_t.

Backport of core#20406.

Unlike core we don't have a dedicated ValueFromAmount function, it's using the implicit cast operator.
The static_assert are moved to Currency where they belong.

Before this fix the displayed result would be messed up (like --92233720368547758.-8).

Depends on D20443.

Test Plan
ninja all check-all

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Fabien requested review of this revision.Wed, Aug 5, 14:09

Tail of the build log:

[452/594] Building CXX object src/seeder/CMakeFiles/bitcoin-seeder.dir/main.cpp.o
[453/594] Building CXX object src/wallet/CMakeFiles/wallet.dir/wallet.cpp.o
[454/594] Building CXX object src/seeder/CMakeFiles/seeder-base.dir/bitcoin.cpp.o
[455/594] Building CXX object src/iguana/CMakeFiles/iguana.dir/iguana.cpp.o
[456/594] Building CXX object src/CMakeFiles/bitcoinconsensus.dir/consensus/amount.cpp.o
FAILED: src/CMakeFiles/bitcoinconsensus.dir/consensus/amount.cpp.o 
/usr/bin/ccache /usr/bin/clang++ -DBOOST_ALL_NO_LIB -DBOOST_NO_CXX98_FUNCTION_BASE -DBUILD_BITCOIN_INTERNAL -DENABLE_AVX2 -DENABLE_SHANI -DENABLE_SSE41 -DHAVE_BUILD_INFO -DHAVE_CONFIG_H -DHAVE_CONSENSUS_LIB -I/work/src/. -I/work/abc-ci-builds/build-clang/src -I/work/src/univalue/include -I/work/abc-ci-builds/build-clang/src/crypto/.. -I/work/src/secp256k1/include -isystem /usr/include/jemalloc -Werror -g -O2 -fPIC -fvisibility=hidden -fstack-protector-all -Wstack-protector -fcf-protection=full -fstack-clash-protection -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -Wall -Wextra -Wformat -Wgnu -Wvla -Wcast-align -Wunused-parameter -Wmissing-braces -Wno-implicit-fallthrough -Wno-psabi -Wno-unused-parameter -Wthread-safety -Wrange-loop-analysis -Wredundant-decls -Wunreachable-code-loop-increment -Wsign-compare -Wconditional-uninitialized -Wdocumentation -Wno-error=deprecated-declarations -Wno-error=enum-constexpr-conversion -Wformat-security -Wredundant-move -Wsuggest-override -Woverloaded-virtual -Wshadow -Wshadow-field -std=gnu++20 -MD -MT src/CMakeFiles/bitcoinconsensus.dir/consensus/amount.cpp.o -MF src/CMakeFiles/bitcoinconsensus.dir/consensus/amount.cpp.o.d -o src/CMakeFiles/bitcoinconsensus.dir/consensus/amount.cpp.o -c /work/src/consensus/amount.cpp
/work/src/consensus/amount.cpp:15:27: error: constexpr variable 'BCHA' must be initialized by a constant expression
   15 | static constexpr Currency BCHA{COIN, SATOSHI, 8, "BCHA"};
      |                           ^   ~~~~~~~~~~~~~~~~~~~~~~~~~~
/usr/lib/gcc/x86_64-linux-gnu/12/../../../../include/c++/12/bits/basic_string.h:356:10: note: assignment to member '_M_local_buf' of union with no active member is not allowed in a constant expression
  356 |             __c = _CharT();
      |                 ^
/usr/lib/gcc/x86_64-linux-gnu/12/../../../../include/c++/12/bits/basic_string.tcc:229:4: note: in call to 'this->_M_use_local_data()'
  229 |           _M_use_local_data();
      |           ^~~~~~~~~~~~~~~~~~~
/usr/lib/gcc/x86_64-linux-gnu/12/../../../../include/c++/12/bits/basic_string.h:642:2: note: in call to 'this->_M_construct<const char *>(&"BCHA"[0], &"BCHA"[4], {{}})'
  642 |         _M_construct(__s, __end, forward_iterator_tag());
      |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/work/src/consensus/amount.cpp:15:50: note: in call to 'basic_string<std::allocator<char>>(&"BCHA"[0], std::allocator<char>())'
   15 | static constexpr Currency BCHA{COIN, SATOSHI, 8, "BCHA"};
      |                                                  ^~~~~~
/work/src/consensus/amount.cpp:16:27: error: constexpr variable 'XEC' must be initialized by a constant expression
   16 | static constexpr Currency XEC{100 * SATOSHI, SATOSHI, 2, "XEC"};
      |                           ^  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/usr/lib/gcc/x86_64-linux-gnu/12/../../../../include/c++/12/bits/basic_string.h:356:10: note: assignment to member '_M_local_buf' of union with no active member is not allowed in a constant expression
  356 |             __c = _CharT();
      |                 ^
/usr/lib/gcc/x86_64-linux-gnu/12/../../../../include/c++/12/bits/basic_string.tcc:229:4: note: in call to 'this->_M_use_local_data()'
  229 |           _M_use_local_data();
      |           ^~~~~~~~~~~~~~~~~~~
/usr/lib/gcc/x86_64-linux-gnu/12/../../../../include/c++/12/bits/basic_string.h:642:2: note: in call to 'this->_M_construct<const char *>(&"XEC"[0], &"XEC"[3], {{}})'
  642 |         _M_construct(__s, __end, forward_iterator_tag());
      |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/work/src/consensus/amount.cpp:16:58: note: in call to 'basic_string<std::allocator<char>>(&"XEC"[0], std::allocator<char>())'
   16 | static constexpr Currency XEC{100 * SATOSHI, SATOSHI, 2, "XEC"};
      |                                                          ^~~~~
/work/src/consensus/amount.cpp:18:15: error: static assertion expression is not an integral constant expression
   18 | static_assert(BCHA.baseunit > 1 * SATOSHI,
      |               ^~~~~~~~~~~~~~~~~~~~~~~~~~~
/work/src/consensus/amount.cpp:18:15: note: initializer of 'BCHA' is not a constant expression
/work/src/consensus/amount.cpp:18:15: note: in call to 'Amount(BCHA.baseunit)'
   18 | static_assert(BCHA.baseunit > 1 * SATOSHI,
      |               ^~~~~~~~~~~~~
/work/src/consensus/amount.cpp:15:27: note: declared here
   15 | static constexpr Currency BCHA{COIN, SATOSHI, 8, "BCHA"};
      |                           ^
/work/src/consensus/amount.cpp:20:15: error: static assertion expression is not an integral constant expression
   20 | static_assert(XEC.baseunit > 1 * SATOSHI,
      |               ^~~~~~~~~~~~~~~~~~~~~~~~~~
/work/src/consensus/amount.cpp:20:15: note: initializer of 'XEC' is not a constant expression
/work/src/consensus/amount.cpp:20:15: note: in call to 'Amount(XEC.baseunit)'
   20 | static_assert(XEC.baseunit > 1 * SATOSHI,
      |               ^~~~~~~~~~~~
/work/src/consensus/amount.cpp:16:27: note: declared here
   16 | static constexpr Currency XEC{100 * SATOSHI, SATOSHI, 2, "XEC"};
      |                           ^
4 errors generated.
ninja: build stopped: cannot make progress due to previous errors.
Build build-clang failed with exit code 1

Build Bitcoin ABC Diffs / Diff Testing (ai-review) passed.
CodeRabbit Review

Diff : committed changes only
Compare : HEAD → master
Directory : work
────────────────────────────────────────

(\(\
(• .•) What, what, what, what, what, what, what, what, what, what?

────────────────────────────────────────
Review complete
No findings ✔

6 files reviewed:

  • src/consensus/amount.cpp
  • src/test/fuzz/integer.cpp
  • src/test/fuzz/transaction.cpp
  • src/test/rpc_tests.cpp
  • src/test/util_tests.cpp
  • src/util/moneystr.cpp

────────────────────────────────────────

Tail of the build log:

[372/437] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/parse_univalue.cpp.o
[373/437] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/deserialize.cpp.o
[374/437] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/pow.cpp.o
[375/437] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/primitives_transaction.cpp.o
[376/437] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/psbt.cpp.o
[377/437] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/protocol.cpp.o
[378/437] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/random.cpp.o
[379/437] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/script_parsing.cpp.o
[380/437] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/script_flags.cpp.o
[381/437] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/rolling_bloom_filter.cpp.o
[382/437] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/script_bitcoin_consensus.cpp.o
[383/437] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/script_descriptor_cache.cpp.o
[384/437] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/script.cpp.o
[385/437] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/process_messages.cpp.o
[386/437] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/script_interpreter.cpp.o
[387/437] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/process_message.cpp.o
[388/437] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/signature_checker.cpp.o
[389/437] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/script_ops.cpp.o
[390/437] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/scriptnum_ops.cpp.o
[391/437] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/secp256k1_ecdsa_signature_parse_der_lax.cpp.o
[392/437] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/secp256k1_ec_seckey_import_export_der.cpp.o
[393/437] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/span.cpp.o
[394/437] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/tx_in.cpp.o
[395/437] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/tx_out.cpp.o
[396/437] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/socks5.cpp.o
[397/437] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/timedata.cpp.o
[398/437] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/script_sigcache.cpp.o
[399/437] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/system.cpp.o
[400/437] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/script_sign.cpp.o
[401/437] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/string.cpp.o
[402/437] Building CXX object src/wallet/CMakeFiles/wallet.dir/context.cpp.o
[403/437] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/strprintf.cpp.o
[404/437] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/transaction.cpp.o
[405/437] Building CXX object src/wallet/CMakeFiles/wallet.dir/coincontrol.cpp.o
[406/437] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/util.cpp.o
[407/437] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/txrequest.cpp.o
[408/437] Building CXX object src/wallet/CMakeFiles/wallet.dir/db.cpp.o
[409/437] Building CXX object src/wallet/CMakeFiles/wallet.dir/crypter.cpp.o
[410/437] Building CXX object src/wallet/CMakeFiles/wallet.dir/coinselection.cpp.o
[411/437] Building CXX object src/wallet/CMakeFiles/wallet.dir/bdb.cpp.o
[412/437] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/validation_load_mempool.cpp.o
[413/437] Building CXX object src/wallet/CMakeFiles/wallet.dir/fees.cpp.o
[414/437] Building CXX object src/wallet/CMakeFiles/wallet.dir/transaction.cpp.o
[415/437] Building CXX object src/wallet/CMakeFiles/wallet.dir/sqlite.cpp.o
[416/437] Building CXX object src/wallet/CMakeFiles/wallet.dir/load.cpp.o
[417/437] Building CXX object src/wallet/CMakeFiles/wallet.dir/receive.cpp.o
[418/437] Building CXX object src/wallet/CMakeFiles/wallet.dir/rpc/util.cpp.o
[419/437] Building CXX object src/wallet/CMakeFiles/wallet.dir/rpc/signmessage.cpp.o
[420/437] Building CXX object src/wallet/CMakeFiles/wallet.dir/rpc/encrypt.cpp.o
[421/437] Building CXX object src/wallet/CMakeFiles/wallet.dir/salvage.cpp.o
[422/437] Building CXX object src/wallet/CMakeFiles/wallet.dir/walletutil.cpp.o
[423/437] Building CXX object src/wallet/CMakeFiles/wallet.dir/spend.cpp.o
[424/437] Building CXX object src/wallet/CMakeFiles/wallet.dir/interfaces.cpp.o
[425/437] Building CXX object src/wallet/CMakeFiles/wallet.dir/scriptpubkeyman.cpp.o
[426/437] Building CXX object src/wallet/CMakeFiles/wallet.dir/rpc/backup.cpp.o
[427/437] Building CXX object src/wallet/CMakeFiles/wallet.dir/walletdb.cpp.o
[428/437] Building CXX object src/wallet/CMakeFiles/wallet.dir/wallet.cpp.o
[429/437] Building CXX object src/wallet/CMakeFiles/wallet.dir/rpcwallet.cpp.o
ninja: build stopped: cannot make progress due to previous errors.
Build build-fuzzer failed with exit code 1

Tail of the build log:

[410/604] Linking C static library src/secp256k1/libsecp256k1.a
[411/604] Linking C executable src/secp256k1/bench
[412/604] Linking C executable src/secp256k1/examples/ecdsa_example
[413/604] Linking C executable src/secp256k1/examples/ecdh_example
[414/604] Building CXX object src/test/CMakeFiles/testutil.dir/util/coins.cpp.o
[415/604] Linking C executable src/secp256k1/examples/schnorr_example
[416/604] Building CXX object src/CMakeFiles/bitcoin-wallet.dir/bitcoin-wallet.cpp.o
[417/604] Building CXX object src/test/CMakeFiles/testutil.dir/util/logging.cpp.o
[418/604] Building CXX object src/test/CMakeFiles/testutil.dir/util/str.cpp.o
[419/604] Building C object src/secp256k1/CMakeFiles/internal-bench.dir/src/bench_internal.c.o
[420/604] Linking C executable src/secp256k1/internal-bench
[421/604] Building C object src/secp256k1/CMakeFiles/ecmult-bench.dir/src/bench_ecmult.c.o
[422/604] Linking C executable src/secp256k1/ecmult-bench
[423/604] Building CXX object src/CMakeFiles/server.dir/wallet/init.cpp.o
[424/604] Building CXX object src/test/CMakeFiles/testutil.dir/util/random.cpp.o
[425/604] Building CXX object src/test/CMakeFiles/testutil.dir/util/blockfilter.cpp.o
[426/604] Building CXX object src/wallet/CMakeFiles/wallet.dir/context.cpp.o
[427/604] Building CXX object src/test/CMakeFiles/testutil.dir/util/transaction_utils.cpp.o
[428/604] Building CXX object src/test/CMakeFiles/testutil.dir/util/net.cpp.o
[429/604] Building CXX object src/wallet/CMakeFiles/wallet.dir/coincontrol.cpp.o
[430/604] Building CXX object src/wallet/CMakeFiles/wallet.dir/db.cpp.o
[431/604] Building CXX object src/wallet/CMakeFiles/wallet.dir/coinselection.cpp.o
[432/604] Building CXX object src/wallet/CMakeFiles/wallet.dir/crypter.cpp.o
[433/604] Building CXX object src/wallet/CMakeFiles/wallet.dir/bdb.cpp.o
[434/604] Building CXX object src/test/CMakeFiles/testutil.dir/util/mining.cpp.o
[435/604] Building CXX object src/test/CMakeFiles/testutil.dir/util/validation.cpp.o
[436/604] Building CXX object src/test/CMakeFiles/testutil.dir/util/txmempool.cpp.o
[437/604] Building CXX object src/test/CMakeFiles/testutil.dir/util/wallet.cpp.o
[438/604] Building CXX object src/CMakeFiles/server.dir/validation.cpp.o
[439/604] Building CXX object src/test/CMakeFiles/testutil.dir/util/setup_common.cpp.o
[440/604] Building CXX object src/wallet/CMakeFiles/wallet.dir/sqlite.cpp.o
[441/604] Building CXX object src/wallet/CMakeFiles/wallet.dir/fees.cpp.o
[442/604] Building CXX object src/wallet/CMakeFiles/wallet.dir/load.cpp.o
[443/604] Building CXX object src/wallet/CMakeFiles/wallet.dir/rpc/util.cpp.o
[444/604] Building CXX object src/wallet/CMakeFiles/wallet.dir/receive.cpp.o
[445/604] Building CXX object src/wallet/CMakeFiles/wallet.dir/rpc/signmessage.cpp.o
[446/604] Building CXX object src/wallet/CMakeFiles/wallet.dir/rpc/encrypt.cpp.o
[447/604] Building CXX object src/wallet/CMakeFiles/wallet.dir/transaction.cpp.o
[448/604] Building CXX object src/wallet/CMakeFiles/wallet.dir/salvage.cpp.o
[449/604] Building CXX object src/seeder/CMakeFiles/seeder-base.dir/dns.cpp.o
[450/604] Building CXX object src/wallet/CMakeFiles/wallet.dir/interfaces.cpp.o
[451/604] Building CXX object src/seeder/CMakeFiles/seeder-base.dir/options.cpp.o
[452/604] Building CXX object src/wallet/CMakeFiles/wallet.dir/walletutil.cpp.o
[453/604] Linking CXX static library chronik/libchronik-bridge.a
[454/604] Linking CXX static library chronik/libchronik.a
[455/604] Building CXX object src/seeder/CMakeFiles/seeder-base.dir/db.cpp.o
[456/604] Building CXX object src/wallet/CMakeFiles/wallet.dir/spend.cpp.o
[457/604] Building CXX object src/iguana/CMakeFiles/iguana.dir/iguana_formatter.cpp.o
[458/604] Building CXX object src/wallet/CMakeFiles/wallet.dir/scriptpubkeyman.cpp.o
[459/604] Building CXX object src/iguana/CMakeFiles/iguana.dir/iguana.cpp.o
[460/604] Building CXX object src/wallet/CMakeFiles/wallet.dir/rpc/backup.cpp.o
[461/604] Building CXX object src/iguana/CMakeFiles/iguana.dir/iguana_interpreter.cpp.o
[462/604] Building CXX object src/seeder/CMakeFiles/bitcoin-seeder.dir/main.cpp.o
[463/604] Building CXX object src/seeder/CMakeFiles/seeder-base.dir/bitcoin.cpp.o
[464/604] Building CXX object src/wallet/CMakeFiles/wallet-tool.dir/wallettool.cpp.o
[465/604] Building CXX object src/wallet/CMakeFiles/wallet.dir/walletdb.cpp.o
[466/604] Building CXX object src/wallet/CMakeFiles/wallet.dir/rpcwallet.cpp.o
[467/604] Building CXX object src/wallet/CMakeFiles/wallet.dir/wallet.cpp.o
ninja: build stopped: cannot make progress due to previous errors.
Build build-clang-tidy failed with exit code 1
Fabien planned changes to this revision.Wed, Aug 5, 16:21

I need to rework the static assert for this version of clang

Rebase to fix the constexpr

Build Bitcoin ABC Diffs / Diff Testing (ai-review) passed.
CodeRabbit Review

Diff : committed changes only
Compare : HEAD → master
Directory : work
────────────────────────────────────────

(\(\
(• .•) Buckle up! It's going to be a buggy ride.

────────────────────────────────────────────────────────────────────────

minor [Maintainability & Code Quality]
→ ]8;;vscode://file//work/src/test/fuzz/integer.cpp:107src/test/fuzz/integer.cpp:107]8;;

Remove the obsolete FormatMoney precondition comment.

Line 106 states that FormatMoney is not defined for
std::numeric_limits::min(). This block now executes that case. The
revised FormatMoney implementation supports it.


Proposed fix

-    // FormatMoney(i) not defined when i == std::numeric_limits<int64_t>::min()
     {

────────────────────────────────────────
Review complete
1 finding ✔

Minor 1

6 files reviewed:

  • src/consensus/amount.cpp
  • src/test/fuzz/integer.cpp
  • src/test/fuzz/transaction.cpp
  • src/test/rpc_tests.cpp
  • src/test/util_tests.cpp
  • src/util/moneystr.cpp

────────────────────────────────────────

Print all AI prompts: coderabbit review --show-prompts

Remove obsolete comment

Build Bitcoin ABC Diffs / Diff Testing (ai-review) passed.
CodeRabbit Review

Diff : committed changes only
Compare : HEAD → master
Directory : work
────────────────────────────────────────

(\(\
(• .•) What happens in code review stays in code review.

────────────────────────────────────────
Review complete
No findings ✔

6 files reviewed:

  • src/consensus/amount.cpp
  • src/test/fuzz/integer.cpp
  • src/test/fuzz/transaction.cpp
  • src/test/rpc_tests.cpp
  • src/test/util_tests.cpp
  • src/util/moneystr.cpp

────────────────────────────────────────

This revision is now accepted and ready to land.Thu, Aug 6, 11:21