Skip to content

Commit 972c516

Browse files
committed
qt/test: Reset chainman in ~ChainstateManager instead
There are some mutable, global state variables that are currently reset by UnloadBlockIndex such as pindexBestHeader which should be cleaned up whenever the ChainstateManager is unloaded/reset/destructed/etc. Not cleaning them up leads to bugs like a use-after-free that happens like so: 1. At the end of a test, ChainstateManager is destructed, which also destructs BlockManager, which calls BlockManager::Unload to free all CBlockIndexes in its BlockMap 2. Since pindexBestHeader is not cleaned up, it now points to an invalid location 3. Another test starts to init, and calls LoadGenesisBlock, which calls AddToBlockIndex, which compares the genesis block with an invalid location 4. Cute puppies perish by the hundreds Previously, for normal codepaths (e.g. bitcoind), we relied on the fact that our program will be unloaded by the operating system which effectively resets these variables. The one exception is in QT tests, where these variables had to be manually reset. Since now ChainstateManager is no longer a global, we can just put this logic in its destructor to make sure that callers are always correct. Over time, we should probably move these mutable global state variables into ChainstateManager or CChainState so it's easier to reason about their lifecycles.
1 parent 6c3b5dc commit 972c516

File tree

2 files changed

+6
-5
lines changed

2 files changed

+6
-5
lines changed

src/qt/test/apptests.cpp

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -85,11 +85,6 @@ void AppTests::appTests()
8585
// Reset global state to avoid interfering with later tests.
8686
LogInstance().DisconnectTestLogger();
8787
AbortShutdown();
88-
{
89-
LOCK(cs_main);
90-
UnloadBlockIndex(/* mempool */ nullptr, g_chainman);
91-
g_chainman.Reset();
92-
}
9388
}
9489

9590
//! Entry point for BitcoinGUI tests.

src/validation.h

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1017,6 +1017,12 @@ class ChainstateManager
10171017
//! Check to see if caches are out of balance and if so, call
10181018
//! ResizeCoinsCaches() as needed.
10191019
void MaybeRebalanceCaches() EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
1020+
1021+
~ChainstateManager() {
1022+
LOCK(::cs_main);
1023+
UnloadBlockIndex(/* mempool */ nullptr, *this);
1024+
Reset();
1025+
}
10201026
};
10211027

10221028
/** DEPRECATED! Please use node.chainman instead. May only be used in validation.cpp internally */

0 commit comments

Comments
 (0)