Fix wallet unload race condition

Currently it's possible for ReleaseWallet to delete the CWallet pointer while
it is processing BlockConnected, etc chain notifications.

To fix this, unregister from notifications earlier in UnloadWallet instead of
ReleaseWallet, and use a new RegisterSharedValidationInterface function to
prevent the CValidationInterface shared_ptr from being deleted until the last
notification is actually finished.
This commit is contained in:
Russell Yanofsky
2020-03-10 15:46:20 -04:00
committed by João Barbosa
parent 3e50fdbe4e
commit ab31b9d6fe
9 changed files with 62 additions and 42 deletions

View File

@@ -62,8 +62,10 @@ bool AddWallet(const std::shared_ptr<CWallet>& wallet)
bool RemoveWallet(const std::shared_ptr<CWallet>& wallet)
{
LOCK(cs_wallets);
assert(wallet);
// Unregister with the validation interface which also drops shared ponters.
wallet->m_chain_notifications_handler.reset();
LOCK(cs_wallets);
std::vector<std::shared_ptr<CWallet>>::iterator i = std::find(vpwallets.begin(), vpwallets.end(), wallet);
if (i == vpwallets.end()) return false;
vpwallets.erase(i);
@@ -105,13 +107,9 @@ static std::set<std::string> g_unloading_wallet_set;
// Custom deleter for shared_ptr<CWallet>.
static void ReleaseWallet(CWallet* wallet)
{
// Unregister and delete the wallet right after BlockUntilSyncedToCurrentChain
// so that it's in sync with the current chainstate.
const std::string name = wallet->GetName();
wallet->WalletLogPrintf("Releasing wallet\n");
wallet->BlockUntilSyncedToCurrentChain();
wallet->Flush();
wallet->m_chain_notifications_handler.reset();
delete wallet;
// Wallet is now released, notify UnloadWallet, if any.
{
@@ -137,6 +135,7 @@ void UnloadWallet(std::shared_ptr<CWallet>&& wallet)
// Notify the unload intent so that all remaining shared pointers are
// released.
wallet->NotifyUnload();
// Time to ditch our shared_ptr and wait for ReleaseWallet call.
wallet.reset();
{
@@ -4092,7 +4091,7 @@ std::shared_ptr<CWallet> CWallet::CreateWalletFromFile(interfaces::Chain& chain,
}
// Register with the validation interface. It's ok to do this after rescan since we're still holding locked_chain.
walletInstance->handleNotifications();
walletInstance->m_chain_notifications_handler = walletInstance->chain().handleNotifications(walletInstance);
walletInstance->SetBroadcastTransactions(gArgs.GetBoolArg("-walletbroadcast", DEFAULT_WALLETBROADCAST));
@@ -4105,11 +4104,6 @@ std::shared_ptr<CWallet> CWallet::CreateWalletFromFile(interfaces::Chain& chain,
return walletInstance;
}
void CWallet::handleNotifications()
{
m_chain_notifications_handler = m_chain->handleNotifications(*this);
}
void CWallet::postInitProcess()
{
auto locked_chain = chain().lock();