[wallet] Fix potential memory leak in CreateWalletFromFile

John Newbery · John Newbery · commit 59b87a27efea · 2018-05-15T13:28:29.000-04:00
Fix proposed by ryanofsky in bitcoin#12647 (comment)
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
@@ -4012,7 +4012,10 @@ CWallet* CWallet::CreateWalletFromFile(const std::string& name, const fs::path&
 
     int64_t nStart = GetTimeMillis();
     bool fFirstRun = true;
-    CWallet *walletInstance = new CWallet(name, WalletDatabase::Create(path));
+    // Make a temporary wallet unique pointer so memory doesn't get leaked if
+    // wallet creation fails.
+    auto temp_wallet = MakeUnique<CWallet>(name, WalletDatabase::Create(path));
+    CWallet* walletInstance = temp_wallet.get();
     DBErrors nLoadWalletRet = walletInstance->LoadWallet(fFirstRun);
     if (nLoadWalletRet != DBErrors::LOAD_OK)
     {
@@ -4224,7 +4227,6 @@ CWallet* CWallet::CreateWalletFromFile(const std::string& name, const fs::path&
     }
 
     walletInstance->m_last_block_processed = chainActive.Tip();
-    RegisterValidationInterface(walletInstance);
 
     if (chainActive.Tip() && chainActive.Tip() != pindexRescan)
     {
@@ -4290,6 +4292,10 @@ CWallet* CWallet::CreateWalletFromFile(const std::string& name, const fs::path&
             }
         }
     }
+
+    // Register with the validation interface. It's ok to do this after rescan since we're still holding cs_main.
+    RegisterValidationInterface(temp_wallet.release());
+
     walletInstance->SetBroadcastTransactions(gArgs.GetBoolArg("-walletbroadcast", DEFAULT_WALLETBROADCAST));
 
     {

Original file line number	Diff line number	Diff line change
`@@ -4012,7 +4012,10 @@ CWallet* CWallet::CreateWalletFromFile(const std::string& name, const fs::path&`
`4012`	`4012`
`4013`	`4013`	`int64_t nStart = GetTimeMillis();`
`4014`	`4014`	`bool fFirstRun = true;`
`4015`		`- CWallet *walletInstance = new CWallet(name, WalletDatabase::Create(path));`
	`4015`	`+ // Make a temporary wallet unique pointer so memory doesn't get leaked if`
	`4016`	`+ // wallet creation fails.`
	`4017`	`+ auto temp_wallet = MakeUnique<CWallet>(name, WalletDatabase::Create(path));`
	`4018`	`+ CWallet* walletInstance = temp_wallet.get();`
`4016`	`4019`	`DBErrors nLoadWalletRet = walletInstance->LoadWallet(fFirstRun);`
`4017`	`4020`	`if (nLoadWalletRet != DBErrors::LOAD_OK)`
`4018`	`4021`	`{`
`@@ -4224,7 +4227,6 @@ CWallet* CWallet::CreateWalletFromFile(const std::string& name, const fs::path&`
`4224`	`4227`	`}`
`4225`	`4228`
`4226`	`4229`	`walletInstance->m_last_block_processed = chainActive.Tip();`
`4227`		`- RegisterValidationInterface(walletInstance);`
`4228`	`4230`
`4229`	`4231`	`if (chainActive.Tip() && chainActive.Tip() != pindexRescan)`
`4230`	`4232`	`{`
`@@ -4290,6 +4292,10 @@ CWallet* CWallet::CreateWalletFromFile(const std::string& name, const fs::path&`
`4290`	`4292`	`}`
`4291`	`4293`	`}`
`4292`	`4294`	`}`
	`4295`	`+`
	`4296`	`+ // Register with the validation interface. It's ok to do this after rescan since we're still holding cs_main.`
	`4297`	`+ RegisterValidationInterface(temp_wallet.release());`
	`4298`	`+`
`4293`	`4299`	`walletInstance->SetBroadcastTransactions(gArgs.GetBoolArg("-walletbroadcast", DEFAULT_WALLETBROADCAST));`
`4294`	`4300`
`4295`	`4301`	`{`