Page MenuHomePhabricator
Differential D17001

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

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

Details

Reviewers
Fabien
Group Reviewers
Restricted Project
Commits
rABCf4c5b773c957: [64-bit ints] Add overflow checks to CScriptNum for +, -, +=, -=
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
Lint
Lint Not Applicable
Unit
Tests Not Applicable

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
teamcity added a comment.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
tobias_ruck updated this revision to Diff 51434.Dec 6 2024, 10:51

rebase, fix build

tobias_ruck edited the summary of this revision. (Show Details)Dec 6 2024, 10:52
Harbormaster completed remote builds in B31546: Diff 51434.Dec 6 2024, 11:20
tobias_ruck published this revision for review.Dec 6 2024, 11:55
Fabien requested changes to this revision.Dec 6 2024, 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.Dec 6 2024, 13:52
tobias_ruck added inline comments.Dec 6 2024, 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

tobias_ruck updated this revision to Diff 51455.Dec 6 2024, 21:13

address review

Harbormaster completed remote builds in B31562: Diff 51455.Dec 6 2024, 21:46
Fabien requested changes to this revision.Dec 9 2024, 13:04
Fabien added inline comments.
src/script/script.h
338 ↗(On Diff #51455)

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 ↗(On Diff #51455)

You should use HasReason from setup_common.h

This revision now requires changes to proceed.Dec 9 2024, 13:04
tobias_ruck updated this revision to Diff 55083.Aug 1 2025, 14:10

use HasReason

note: 0 is already tested, as "0" is part of INTERESTING_63_BIT_NUMBERS

Harbormaster completed remote builds in B34031: Diff 55083.Aug 1 2025, 14:24
tobias_ruck mentioned this in D17300: [64-bit ints] Add `SCRIPT_ENABLE_63_BIT_INTS` flag.Aug 1 2025, 14:47
tobias_ruck added a child revision: D17300: [64-bit ints] Add `SCRIPT_ENABLE_63_BIT_INTS` flag.Aug 1 2025, 15:22
tobias_ruck updated this revision to Diff 55087.Aug 1 2025, 18:46

add back asserts that we actually still need

Fabien accepted this revision.Aug 1 2025, 18:57
This revision is now accepted and ready to land.Aug 1 2025, 18:57
This revision was landed with ongoing or failed builds.Aug 1 2025, 19:11
Closed by commit rABCf4c5b773c957: [64-bit ints] Add overflow checks to CScriptNum for +, -, +=, -= (authored by tobias_ruck). · Explain Why
This revision was automatically updated to reflect the committed changes.
tobias_ruck added a commit: rABCf4c5b773c957: [64-bit ints] Add overflow checks to CScriptNum for +, -, +=, -=.
Harbormaster completed remote builds in B34034: Diff 55087.Aug 1 2025, 19:18

Revision Contents
Changeset List

PathSize
src/
1 line
script/
27 lines
test/
31 lines

Diff 55088

View Options

src/CMakeLists.txt

Loading...
View Options

src/script/script.h

Loading...
View Options

src/test/intmath_tests.cpp

Loading...