diff options
author | Carl Dong <contact@carldong.me> | 2020-10-06 17:35:53 -0400 |
---|---|---|
committer | Carl Dong <contact@carldong.me> | 2021-06-10 15:05:25 -0400 |
commit | 972c5166ee685447a6d4bf5e501b07a0871fba85 (patch) | |
tree | 184980a731a42e4bee913efd40958ae6d114989e /src | |
parent | 6c3b5dc0c13c3ac8c6e86298f924abe99d8d6bd1 (diff) | |
download | bitcoin-972c5166ee685447a6d4bf5e501b07a0871fba85.tar.xz |
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.
Diffstat (limited to 'src')
-rw-r--r-- | src/qt/test/apptests.cpp | 5 | ||||
-rw-r--r-- | src/validation.h | 6 |
2 files changed, 6 insertions, 5 deletions
diff --git a/src/qt/test/apptests.cpp b/src/qt/test/apptests.cpp index cb3dbd2267..9c31cd50df 100644 --- a/src/qt/test/apptests.cpp +++ b/src/qt/test/apptests.cpp @@ -85,11 +85,6 @@ void AppTests::appTests() // Reset global state to avoid interfering with later tests. LogInstance().DisconnectTestLogger(); AbortShutdown(); - { - LOCK(cs_main); - UnloadBlockIndex(/* mempool */ nullptr, g_chainman); - g_chainman.Reset(); - } } //! Entry point for BitcoinGUI tests. diff --git a/src/validation.h b/src/validation.h index 0c396c92ee..a3ead9443e 100644 --- a/src/validation.h +++ b/src/validation.h @@ -1017,6 +1017,12 @@ public: //! Check to see if caches are out of balance and if so, call //! ResizeCoinsCaches() as needed. void MaybeRebalanceCaches() EXCLUSIVE_LOCKS_REQUIRED(::cs_main); + + ~ChainstateManager() { + LOCK(::cs_main); + UnloadBlockIndex(/* mempool */ nullptr, *this); + Reset(); + } }; /** DEPRECATED! Please use node.chainman instead. May only be used in validation.cpp internally */ |