[64-bit ints] Add overflow checks to CScriptNum for +, -, +=, -=
Needs RevisionPublic
Actions

Authored by tobias_ruck on Oct 25 2024, 09:55.

Details

Reviewers

Fabien

Group Reviewers

Restricted Project

Summary

Check for overflows in these functions:

operator+
operator-
operator+=
operator-=

Note: operator- (negation), operator/ and operator% don't need overflow checks, because by design these can never overflow, as we exclude std::numeric_limits<int64_t>::min() from the valid script integer range.

Depends on D17000 and D16997.

Test Plan

ninja check

Diff Detail

Repository

rABC Bitcoin ABC

Branch

add-checked-scriptnum-ops

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 31562
Build 62621: Build Diff	lint-circular-dependencies · build-without-wallet · build-diff · build-debug · build-clang-tidy · build-clang
Build 62620: arc lint + arc unit

Event Timeline

tobias_ruck created this revision.Oct 25 2024, 09:55

Herald added a reviewer: Restricted Project. · View Herald TranscriptOct 25 2024, 09:55

Harbormaster completed remote builds in B30837: Diff 50431.Oct 25 2024, 10:10

Build Bitcoin ABC Diffs / Diff Testing (build-without-wallet) failed.

Tail of the build log:

[597/648] Building CXX object src/bench/CMakeFiles/bitcoin-bench.dir/data.cpp.o
[598/648] Building CXX object src/qt/CMakeFiles/bitcoin-qt-base.dir/bitcoin-qt-base_autogen/mocs_compilation.cpp.o
[599/648] Building CXX object src/bench/CMakeFiles/bitcoin-bench.dir/examples.cpp.o
[600/648] Building CXX object src/qt/CMakeFiles/bitcoin-qt-base.dir/clientmodel.cpp.o
[601/648] Building CXX object src/bench/CMakeFiles/bitcoin-bench.dir/hashpadding.cpp.o
[602/648] Building CXX object src/qt/CMakeFiles/bitcoin-qt-base.dir/intro.cpp.o
[603/648] Building CXX object src/bench/CMakeFiles/bitcoin-bench.dir/lockedpool.cpp.o
[604/648] Building CXX object src/qt/CMakeFiles/bitcoin-qt-base.dir/bitcoingui.cpp.o
[605/648] Linking CXX static library src/libbitcoinkernel.a
[606/648] Building CXX object src/bench/CMakeFiles/bitcoin-bench.dir/gcs_filter.cpp.o
[607/648] Building CXX object src/qt/CMakeFiles/bitcoin-qt-base.dir/optionsmodel.cpp.o
[608/648] Building CXX object src/bench/CMakeFiles/bitcoin-bench.dir/nanobench.cpp.o
[609/648] Building CXX object src/bench/CMakeFiles/bitcoin-bench.dir/pool.cpp.o
[610/648] Building CXX object src/qt/CMakeFiles/bitcoin-qt-base.dir/guiutil.cpp.o
[611/648] Building CXX object src/bench/CMakeFiles/bitcoin-bench.dir/poly1305.cpp.o
[612/648] Building CXX object src/qt/CMakeFiles/bitcoin-qt-base.dir/optionsdialog.cpp.o
[613/648] Building CXX object src/qt/CMakeFiles/bitcoin-qt-base.dir/bitcoin.cpp.o
[614/648] Building CXX object src/bench/CMakeFiles/bitcoin-bench.dir/prevector.cpp.o
[615/648] Building CXX object src/bench/CMakeFiles/bitcoin-bench.dir/rollingbloom.cpp.o
[616/648] Building CXX object src/bench/CMakeFiles/bitcoin-bench.dir/streams_findbyte.cpp.o
[617/648] Building CXX object src/bench/CMakeFiles/bitcoin-bench.dir/util_time.cpp.o
[618/648] Linking CXX executable src/bitcoin-chainstate
FAILED: src/bitcoin-chainstate 
: && /usr/bin/c++ -Werror -g -O2 -fuse-ld=gold -Wl,-z,noexecstack -Wl,-z,relro -Wl,-z,now -lstdc++fs -fPIE -pie src/CMakeFiles/bitcoin-chainstate.dir/bitcoin-chainstate.cpp.o -o src/bitcoin-chainstate  /usr/lib/x86_64-linux-gnu/libjemalloc_pic.a  -lstdc++fs  src/libbitcoinkernel.a  src/crypto/libcrypto.a  src/crypto/libcrypto_sse4.1.a  src/crypto/libcrypto_avx2.a  src/crypto/libcrypto_shani.a  src/univalue/libunivalue.a  src/secp256k1/libsecp256k1.a  src/leveldb/libleveldb.a  src/leveldb/libleveldb-sse4.2.a  src/leveldb/libmemenv.a  /usr/lib/x86_64-linux-gnu/libjemalloc_pic.a  -lm  -pthread  -ldl  -lstdc++fs && :
../../src/./script/script.h:276: error: undefined reference to 'AddInt63Overflow(long, long, long&)'
../../src/./script/script.h:283: error: undefined reference to 'SubInt63Overflow(long, long, long&)'
../../src/./script/script.h:276: error: undefined reference to 'AddInt63Overflow(long, long, long&)'
../../src/./script/script.h:283: error: undefined reference to 'SubInt63Overflow(long, long, long&)'
collect2: error: ld returned 1 exit status
[619/648] Building CXX object src/bench/CMakeFiles/bitcoin-bench.dir/checkblock.cpp.o
[620/648] Building CXX object src/qt/CMakeFiles/bitcoin-qt-base.dir/platformstyle.cpp.o
[621/648] Building CXX object src/qt/CMakeFiles/bitcoin-qt-base.dir/qvalidatedlineedit.cpp.o
[622/648] Building CXX object src/qt/CMakeFiles/bitcoin-qt-base.dir/qvaluecombobox.cpp.o
[623/648] Building CXX object src/bench/CMakeFiles/bitcoin-bench.dir/merkle_root.cpp.o
[624/648] Building CXX object src/bench/CMakeFiles/bitcoin-bench.dir/bench.cpp.o
[625/648] Building CXX object src/bench/CMakeFiles/bitcoin-bench.dir/chained_tx.cpp.o
[626/648] Building CXX object src/bench/CMakeFiles/bitcoin-bench.dir/crypto_aes.cpp.o
[627/648] Building CXX object src/bench/CMakeFiles/bitcoin-bench.dir/load_external.cpp.o
[628/648] Building CXX object src/bench/CMakeFiles/bitcoin-bench.dir/verify_script.cpp.o
[629/648] Building CXX object src/qt/CMakeFiles/bitcoin-qt-base.dir/qrc_bitcoin_locale.cpp.o
[630/648] Building CXX object src/qt/CMakeFiles/bitcoin-qt-base.dir/qrc_bitcoin.cpp.o
[631/648] Building CXX object src/bench/CMakeFiles/bitcoin-bench.dir/duplicate_inputs.cpp.o
[632/648] Building CXX object src/bench/CMakeFiles/bitcoin-bench.dir/mempool_eviction.cpp.o
[633/648] Building CXX object src/bench/CMakeFiles/bitcoin-bench.dir/rpc_mempool.cpp.o
[634/648] Building CXX object src/bench/CMakeFiles/bitcoin-bench.dir/peer_eviction.cpp.o
[635/648] Building CXX object src/bench/CMakeFiles/bitcoin-bench.dir/mempool_stress.cpp.o
[636/648] Building CXX object src/bench/CMakeFiles/bitcoin-bench.dir/rpc_blockchain.cpp.o
[637/648] Building CXX object src/qt/CMakeFiles/bitcoin-qt-base.dir/utilitydialog.cpp.o
[638/648] Building CXX object src/qt/CMakeFiles/bitcoin-qt-base.dir/trafficgraphwidget.cpp.o
[639/648] Building CXX object src/qt/CMakeFiles/bitcoin-qt-base.dir/splashscreen.cpp.o
[640/648] Linking CXX executable src/bench/bitcoin-bench
[641/648] Building CXX object src/qt/CMakeFiles/bitcoin-qt-base.dir/peertablemodel.cpp.o
[642/648] Building CXX object src/qt/CMakeFiles/bitcoin-qt-base.dir/rpcconsole.cpp.o
[643/648] Linking CXX static library src/qt/libbitcoin-qt-base.a
[644/648] Automatic MOC for target bitcoin-qt
[645/648] Building CXX object src/qt/CMakeFiles/bitcoin-qt.dir/bitcoin-qt_autogen/mocs_compilation.cpp.o
[646/648] Building CXX object src/qt/CMakeFiles/bitcoin-qt.dir/main.cpp.o
[647/648] Linking CXX executable src/qt/bitcoin-qt
ninja: build stopped: cannot make progress due to previous errors.
Build build-without-wallet failed with exit code 1

rebase, fix build

tobias_ruck edited the summary of this revision. (Show Details)Fri, Dec 6, 10:52

Harbormaster completed remote builds in B31546: Diff 51434.Fri, Dec 6, 11:20

tobias_ruck published this revision for review.Fri, Dec 6, 11:55

Fabien requested changes to this revision.Fri, Dec 6, 13:52

Fabien added inline comments.

src/script/script.h
338 ↗	(On Diff #51434)	I don't understand this assertion, can you please explain ? You already benefit from the overflow check. Also the case where rhs is 0 should be dealt with like it was in the previous version.
344 ↗	(On Diff #51434)	Same question
src/test/intmath_tests.cpp
118 ↗	(On Diff #51434)	Please use BOOST_CHECK_EXCEPTION and make sure you're getting the exception you're looking for. This will pass for any kind of exception so if your code has a bug it might not be caught

This revision now requires changes to proceed.Fri, Dec 6, 13:52

tobias_ruck added inline comments.Fri, Dec 6, 20:41

src/script/script.h
338 ↗	(On Diff #51434)	hmm, the meaning here changed because `std::numeric_limits<int64_t>::min()` is basically an illegal value now but maybe just removing the assert is better because it's indeed handled in operator+ now

address review

Harbormaster completed remote builds in B31562: Diff 51455.Fri, Dec 6, 21:46

Fabien requested changes to this revision.Mon, Dec 9, 13:04

Fabien added inline comments.

src/script/script.h
338	Is the +0 (and -0) case tested ? Would be good to make sure we don't introduce a regression here
src/test/intmath_tests.cpp
120	You should use HasReason from setup_common.h