Page MenuHomePhabricator
Differential D9105

miner: Avoid stack-use-after-return in validationinterface
ClosedPublic
Actions

Authored by PiRK on Jan 28 2021, 14:32.

Details

Reviewers
Fabien
Group Reviewers
Restricted Project
Commits
rABCb3681986e349: miner: Avoid stack-use-after-return in validationinterface
Summary

This is a squash of 2 commits, to add the unbroken test and the solution
at the same time, and a revert of D5140 (different solution to the same problem)

test: Add unregister_validation_interface_race test

https://github.com/bitcoin/bitcoin/pull/18742/commits/fab6d060ce5f580db538070beec1c5518c8c777c

miner: Avoid stack-use-after-return in validationinterface

This is achieved by switching to a shared_ptr.

Also, switch the validationinterfaces in the tests to use shared_ptrs
for the same reason.

https://github.com/bitcoin/bitcoin/pull/18742/commits/7777f2a4bb1f9d843bc50a4e35085cfbb2808780

This is a backport of Core PR18742 [3/3]

This replaces the solution in D5140
See comment https://reviews.bitcoinabc.org/D5140#124338

Depends on D9104

Test Plan

Test suggested by this PR:

export ASAN_OPTIONS=detect_leaks=0
ninja && ninja check
while ./src/test/test_bitcoin -t validationinterface_tests/unregister_validation_interface_race --catch_system_errors=no  ; do true; done

Test from D5140:

CC=clang CXX=clang++ cmake -GNinja .. -DCMAKE_BUILD_TYPE=Debug
ninja check-extended

Diff Detail

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

Event Timeline

PiRK created this revision.Jan 28 2021, 14:32
Herald added a reviewer: Restricted Project. · View Herald TranscriptJan 28 2021, 14:32
PiRK requested review of this revision.Jan 28 2021, 14:32
teamcity edited the summary of this revision. (Show Details)Jan 28 2021, 14:32
PiRK edited the summary of this revision. (Show Details)Jan 28 2021, 14:33
PiRK added a parent revision: D9104: test: Remove UninterruptibleSleep from test and replace it by SyncWithValidationInterfaceQueue.
PiRK edited the summary of this revision. (Show Details)
Harbormaster completed remote builds in B15044: Diff 27411.Jan 28 2021, 14:57
teamcity added a comment.Jan 28 2021, 14:57
Build Bitcoin ABC Diffs / Diff Testing (build-debug) failed.

Tail of the build log:

wallet_create_tx.py                              | ✓ Passed  | 6 s
wallet_createwallet.py                           | ✓ Passed  | 3 s
wallet_createwallet.py --usecli                  | ✓ Passed  | 3 s
wallet_descriptor.py                             | ✓ Passed  | 9 s
wallet_disable.py                                | ✓ Passed  | 1 s
wallet_dump.py                                   | ✓ Passed  | 5 s
wallet_encryption.py                             | ✓ Passed  | 5 s
wallet_groups.py                                 | ✓ Passed  | 46 s
wallet_hd.py                                     | ✓ Passed  | 5 s
wallet_import_rescan.py                          | ✓ Passed  | 10 s
wallet_import_with_label.py                      | ✓ Passed  | 1 s
wallet_importdescriptors.py                      | ✓ Passed  | 5 s
wallet_importmulti.py                            | ✓ Passed  | 5 s
wallet_importprunedfunds.py                      | ✓ Passed  | 1 s
wallet_keypool.py                                | ✓ Passed  | 3 s
wallet_keypool_topup.py                          | ✓ Passed  | 3 s
wallet_labels.py                                 | ✓ Passed  | 2 s
wallet_listreceivedby.py                         | ✓ Passed  | 14 s
wallet_listsinceblock.py                         | ✓ Passed  | 6 s
wallet_listtransactions.py                       | ✓ Passed  | 14 s
wallet_multiwallet.py                            | ✓ Passed  | 12 s
wallet_multiwallet.py --usecli                   | ✓ Passed  | 13 s
wallet_reorgsrestore.py                          | ✓ Passed  | 3 s
wallet_resendwallettransactions.py               | ✓ Passed  | 7 s
wallet_txn_clone.py                              | ✓ Passed  | 3 s
wallet_txn_clone.py --mineblock                  | ✓ Passed  | 3 s
wallet_txn_doublespend.py                        | ✓ Passed  | 3 s
wallet_txn_doublespend.py --mineblock            | ✓ Passed  | 3 s
wallet_watchonly.py                              | ✓ Passed  | 1 s
wallet_watchonly.py --usecli                     | ✓ Passed  | 1 s
wallet_zapwallettxes.py                          | ✓ Passed  | 5 s

ALL                                              | ✓ Passed  | 1241 s (accumulated) 
Runtime: 250 s

----------------------------------------------------------------------
Ran 1 test in 0.000s

OK

[179/438] Test Bitcoin RPC authentication...
...
----------------------------------------------------------------------
Ran 3 tests in 0.014s

OK
[180/438] cd /work/contrib/devtools/chainparams && /usr/bin/python3.7 ./test_make_chainparams.py
.....
----------------------------------------------------------------------
Ran 5 tests in 0.001s

OK
[190/438] Running seeder test suite
PASSED: seeder test suite
[199/438] Running pow test suite
PASSED: pow test suite
[205/438] Running bitcoin-qt test suite
PASSED: bitcoin-qt test suite
ninja: build stopped: cannot make progress due to previous errors.
Build build-debug failed with exit code 1
teamcity added a comment.Jan 28 2021, 14:57
Build Bitcoin ABC Diffs / Diff Testing (build-without-wallet) failed.

Tail of the build log:

wallet_import_rescan.py                          | ○ Skipped | 0 s
wallet_import_with_label.py                      | ○ Skipped | 0 s
wallet_importdescriptors.py                      | ○ Skipped | 0 s
wallet_importmulti.py                            | ○ Skipped | 0 s
wallet_importprunedfunds.py                      | ○ Skipped | 0 s
wallet_keypool.py                                | ○ Skipped | 0 s
wallet_keypool_topup.py                          | ○ Skipped | 0 s
wallet_labels.py                                 | ○ Skipped | 0 s
wallet_listreceivedby.py                         | ○ Skipped | 0 s
wallet_listsinceblock.py                         | ○ Skipped | 0 s
wallet_listtransactions.py                       | ○ Skipped | 0 s
wallet_multiwallet.py                            | ○ Skipped | 0 s
wallet_multiwallet.py --usecli                   | ○ Skipped | 0 s
wallet_reorgsrestore.py                          | ○ Skipped | 0 s
wallet_resendwallettransactions.py               | ○ Skipped | 0 s
wallet_txn_clone.py                              | ○ Skipped | 0 s
wallet_txn_clone.py --mineblock                  | ○ Skipped | 0 s
wallet_txn_doublespend.py                        | ○ Skipped | 0 s
wallet_txn_doublespend.py --mineblock            | ○ Skipped | 0 s
wallet_watchonly.py                              | ○ Skipped | 0 s
wallet_watchonly.py --usecli                     | ○ Skipped | 0 s
wallet_zapwallettxes.py                          | ○ Skipped | 0 s

ALL                                              | ✓ Passed  | 445 s (accumulated) 
Runtime: 103 s

----------------------------------------------------------------------
Ran 1 test in 0.000s

OK

[145/401] Running avalanche test suite
PASSED: avalanche test suite
[147/401] Test Bitcoin RPC authentication...
...
----------------------------------------------------------------------
Ran 3 tests in 0.005s

OK
[149/401] cd /work/contrib/devtools/chainparams && /usr/bin/python3.7 ./test_make_chainparams.py
.....
----------------------------------------------------------------------
Ran 5 tests in 0.001s

OK
[164/401] Running pow test suite
PASSED: pow test suite
[174/401] Running seeder test suite
PASSED: seeder test suite
[182/401] Running bitcoin-qt test suite
PASSED: bitcoin-qt test suite
[184/401] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/script_tests.cpp.o
In file included from /usr/include/boost/test/unit_test.hpp:19,
                 from ../../src/test/script_tests.cpp:30:
../../src/test/script_tests.cpp: In member function ‘void script_tests::script_build::test_method()’:
../../src/test/script_tests.cpp:541:22: note: variable tracking size limit exceeded with -fvar-tracking-assignments, retrying without
 BOOST_AUTO_TEST_CASE(script_build) {
                      ^~~~~~~~~~~~
ninja: build stopped: cannot make progress due to previous errors.
Build build-without-wallet failed with exit code 1
teamcity added a comment.Jan 28 2021, 15:02
Build Bitcoin ABC Diffs / Diff Testing (build-diff) failed.

Tail of the build log:

rpc_psbt.py                                      | ✓ Passed  | 28 s
rpc_rawtransaction.py                            | ✓ Passed  | 37 s
rpc_scantxoutset.py                              | ✓ Passed  | 3 s
rpc_setban.py                                    | ✓ Passed  | 2 s
rpc_signmessage.py                               | ✓ Passed  | 1 s
rpc_signrawtransaction.py                        | ✓ Passed  | 1 s
rpc_txoutproof.py                                | ✓ Passed  | 2 s
rpc_uptime.py                                    | ✓ Passed  | 1 s
rpc_users.py                                     | ✓ Passed  | 5 s
rpc_whitelist.py                                 | ✓ Passed  | 1 s
tool_wallet.py                                   | ✓ Passed  | 4 s
wallet_abandonconflict.py                        | ✓ Passed  | 16 s
wallet_address_types.py                          | ✓ Passed  | 12 s
wallet_avoidreuse.py                             | ✓ Passed  | 4 s
wallet_backup.py                                 | ✓ Passed  | 23 s
wallet_balance.py                                | ✓ Passed  | 16 s
wallet_basic.py                                  | ✓ Passed  | 25 s
wallet_coinbase_category.py                      | ✓ Passed  | 1 s
wallet_create_tx.py                              | ✓ Passed  | 5 s
wallet_createwallet.py                           | ✓ Passed  | 2 s
wallet_createwallet.py --usecli                  | ✓ Passed  | 3 s
wallet_descriptor.py                             | ✓ Passed  | 6 s
wallet_disable.py                                | ✓ Passed  | 0 s
wallet_dump.py                                   | ✓ Passed  | 5 s
wallet_encryption.py                             | ✓ Passed  | 5 s
wallet_groups.py                                 | ✓ Passed  | 31 s
wallet_hd.py                                     | ✓ Passed  | 5 s
wallet_import_rescan.py                          | ✓ Passed  | 5 s
wallet_import_with_label.py                      | ✓ Passed  | 1 s
wallet_importdescriptors.py                      | ✓ Passed  | 4 s
wallet_importmulti.py                            | ✓ Passed  | 3 s
wallet_importprunedfunds.py                      | ✓ Passed  | 1 s
wallet_keypool.py                                | ✓ Passed  | 3 s
wallet_keypool_topup.py                          | ✓ Passed  | 2 s
wallet_labels.py                                 | ✓ Passed  | 1 s
wallet_listreceivedby.py                         | ✓ Passed  | 21 s
wallet_listsinceblock.py                         | ✓ Passed  | 3 s
wallet_listtransactions.py                       | ✓ Passed  | 23 s
wallet_multiwallet.py                            | ✓ Passed  | 12 s
wallet_multiwallet.py --usecli                   | ✓ Passed  | 12 s
wallet_reorgsrestore.py                          | ✓ Passed  | 3 s
wallet_resendwallettransactions.py               | ✓ Passed  | 10 s
wallet_txn_clone.py                              | ✓ Passed  | 1 s
wallet_txn_clone.py --mineblock                  | ✓ Passed  | 3 s
wallet_txn_doublespend.py                        | ✓ Passed  | 1 s
wallet_txn_doublespend.py --mineblock            | ✓ Passed  | 3 s
wallet_watchonly.py                              | ✓ Passed  | 1 s
wallet_watchonly.py --usecli                     | ✓ Passed  | 1 s
wallet_zapwallettxes.py                          | ✓ Passed  | 4 s

ALL                                              | ✓ Passed  | 973 s (accumulated) 
Runtime: 195 s

----------------------------------------------------------------------
Ran 1 test in 0.000s

OK

ninja: build stopped: cannot make progress due to previous errors.
Build build-diff failed with exit code 1
Fabien requested changes to this revision.Jan 28 2021, 15:33
Fabien added a subscriber: Fabien.

Please fix the warning

This revision now requires changes to proceed.Jan 28 2021, 15:33
PiRK updated this revision to Diff 27416.Jan 28 2021, 15:53

rename inner sub to subscriber to avoid a shadow warning

error: declaration of ‘sub’ shadows a previous local

Harbormaster completed remote builds in B15048: Diff 27416.Jan 28 2021, 16:12
Fabien accepted this revision.Jan 28 2021, 16:42
This revision is now accepted and ready to land.Jan 28 2021, 16:42
Closed by commit rABCb3681986e349: miner: Avoid stack-use-after-return in validationinterface (authored by MarcoFalke <falke.marco@gmail.com>, committed by PiRK). · Explain WhyJan 28 2021, 17:04
This revision was automatically updated to reflect the committed changes.
PiRK added a commit: rABCb3681986e349: miner: Avoid stack-use-after-return in validationinterface.

Revision Contents
Changeset List

PathSize
src/
rpc/
17 lines
test/
10 lines
33 lines

Diff 27422

View Options

src/rpc/mining.cpp

Loading...
View Options

src/test/validation_block_tests.cpp

Loading...
View Options

src/test/validationinterface_tests.cpp

Loading...