wallet: Fix possible memory leak in CreateWalletFromFile.

jimpo commented at 3:41 pm on March 8, 2018: contributor

If there was an error loading the wallet after creating the instance, the instance would leak. This uses RAII to ensure the instance is cleaned up in that case. Issue found during review of #11687.

meshcollider added the label Wallet on Mar 9, 2018

eklitzke commented at 2:26 am on March 11, 2018: contributor

This looks fine to me. It looks like the existing leak scenarios are for fatal errors, but more RAII pointer types is good in my opinion.

It’s kind of weird that you’re putting this in a std::unique_ptr and then later on giving the raw pointer to a WalletRescanReserver(); for that pattern I would have expected to see a std::shared_ptr. That being said WalletRescanReserver() looks like kind of a mess in general (e.g. it uses a mutex to access fScanningWallet in reserve(), but not in isReserved()).

utACK 9378d89

in src/wallet/wallet.cpp:3932 in 9378d89cbb outdated

3928@@ -3929,7 +3929,7 @@ CWallet* CWallet::CreateWalletFromFile(const std::string& name, const fs::path&
3929 
3930     int64_t nStart = GetTimeMillis();
3931     bool fFirstRun = true;
3932-    CWallet *walletInstance = new CWallet(name, CWalletDBWrapper::Create(path));
3933+    std::unique_ptr<CWallet> walletInstance(new CWallet(name, CWalletDBWrapper::Create(path)));

promag commented at 6:57 pm on March 11, 2018:

Use MakeUnique?

jimpo commented at 0:51 am on March 12, 2018:

Why? This is exactly what MakeUnique does, except that this constructs one unique_ptr, whereas std::unique_ptr<CWallet> walletInstance = MakeUnique<...>(...) would construct two.

promag commented at 1:19 am on March 12, 2018:

Why do you say it would construct two?

jimpo commented at 5:20 am on March 12, 2018:

MakeUnique constructs one (the right-hand side), then the left hand side is created using the move constructor.

promag commented at 9:52 am on March 12, 2018:

I think the compiler will optimize the copy initialization. Anyway, there are several advantages and we are using MakeUnique so that we can replace with std::make_unique when we switch to c++14.

jimpo commented at 3:45 pm on March 12, 2018:

Ah, you’re right about the compiler optimization. Didn’t realize that. http://en.cppreference.com/w/cpp/language/copy_elision

in src/wallet/wallet.cpp:4105 in 9378d89cbb outdated

4097@@ -4101,7 +4098,11 @@ CWallet* CWallet::CreateWalletFromFile(const std::string& name, const fs::path&
4098         LogPrintf("mapAddressBook.size() = %u\n",  walletInstance->mapAddressBook.size());
4099     }
4100 
4101-    return walletInstance;
4102+    walletInstance->m_last_block_processed = chainActive.Tip();
4103+
4104+    CWallet* wallet_ptr = walletInstance.release();
4105+    RegisterValidationInterface(wallet_ptr);
4106+    return wallet_ptr;

promag commented at 6:58 pm on March 11, 2018:

Release on return?

jimpo commented at 9:51 pm on March 11, 2018:

Any particular reason? I did it this way so that it would be released before RegisterValidationInterface creates a global reference. Basically, if RegisterValidationInterface failed for some reason, a memory leak is probably better than a segfault. I don’t feel too strongly though.

promag commented at 9:56 am on March 12, 2018:

RegisterValidationInterface failed for some reason

We would want to shutdown no? Anyway leave as it is then.

promag commented at 7:00 pm on March 11, 2018: member

I think it’s fine moving down the registration.

jimpo commented at 11:21 pm on March 11, 2018: contributor

@eklitzke Thanks for the review. I think it is OK to use a unique_ptr there because reserver only exists within the lifetime of walletInstance so it will go out of scope before walletInstance releases or frees the allocated CWallet.

jimpo force-pushed on Mar 12, 2018

promag commented at 2:18 pm on March 13, 2018: member

utACK 7dc50d4. In the future we could move CreateWalletFromFile to WalletManager (introduced in ~~#12587~~ #13034).

in src/wallet/wallet.cpp:4062 in 7dc50d40c8 outdated

4024@@ -4025,9 +4025,6 @@ CWallet* CWallet::CreateWalletFromFile(const std::string& name, const fs::path&
4025             pindexRescan = FindForkInGlobalIndex(chainActive, locator);
4026     }
4027 
4028-    walletInstance->m_last_block_processed = chainActive.Tip();
4029-    RegisterValidationInterface(walletInstance);

ryanofsky commented at 5:56 pm on March 15, 2018:

I don’t think you want to move the m_last_block_processed line above, because rescan code below will access it.

I’m also not sure it’s a great idea to be moving the RegisterValidationInterface call. It may work for now, but create a race condition if we allow loading wallets after node startup (see #10740).

I think I’d suggest a more limited change than what’s implemented here:

Creating the wallet above with:

0auto wallet_deleter = MakeUnique<CWallet>(...);
1CWallet* walletInstance = wallet_deleter.get();

And changing this line to:

0RegisterValidationInterface(wallet_deleter.release());

jimpo commented at 11:18 pm on March 16, 2018:

I don’t see where m_last_block_processed is accessed in the rescan code. It seems to me it’s just used to implement BlockUntilSyncedToCurrentChain.

The RegisterValidationInterface call is tricky. I agree moving it down only works if the wallets are all loaded before the Connman starts, as is the case right now, but it would break if wallets are loaded/unloaded via RPC. Once way to handle it could be to call UnregisterValidationInterface in the CWallet destructor (even without calling RegisterValidationInterface in the constructor). In that case, we could leave the register call where it is. What do you think of that?

ryanofsky commented at 3:12 pm on March 20, 2018:

#12647 (review)

I don’t see where m_last_block_processed is accessed in the rescan code. It seems to me it’s just used to implement BlockUntilSyncedToCurrentChain.

You’re right. I misremembered and it should be fine to move this, if that’s desirable.

Once way to handle it could be to call UnregisterValidationInterface in the CWallet destructor (even without calling RegisterValidationInterface in the constructor). In that case, we could leave the register call where it is. What do you think of that?

Seems good. Would fix the immediate problem and also help with dynamic loading.

ryanofsky commented at 3:16 pm on March 20, 2018: member

WalletRescanReserver() looks like kind of a mess in general (e.g. it uses a mutex to access fScanningWallet in reserve(), but not in isReserved()).

This should be fine because it’s an atomic bool.

jimpo force-pushed on Mar 23, 2018

jimpo force-pushed on Apr 3, 2018

in src/wallet/wallet.h:794 in effd82a08f outdated

788@@ -789,6 +789,10 @@ class CWallet final : public CCryptoKeyStore, public CValidationInterface
789 
790     ~CWallet()
791     {
792+        // Even if RegisterValidationInstance was never called on this instance,
793+        // the unregister call is safe to make and is a no-op.
794+        UnregisterValidationInterface(this);

ryanofsky commented at 3:17 pm on April 6, 2018:

I think this doesn’t work because UnregisterValidationInterface tries to call virtual methods, which isn’t something you can do in c++ in the middle of object destruction. I ran into this problem in #10973

https://github.com/bitcoin/bitcoin/blob/0bdf0e330d5d16e8e6a93d7d4ad663db812c4c91/src/interface/chain.cpp#L163-L169

jimpo commented at 9:54 pm on April 6, 2018:

Hmm, so then should I revert to the version where the Register call was moved to the bottom of the method?

ryanofsky commented at 11:36 am on April 10, 2018:

Hmm, so then should I revert to the version where the Register call was moved to the bottom of the method?

IIRC the problem with moving the registration is that it creates potential for race conditions if the code is called at any time other than startup, which is something that’s been attempted in prs like #10740 (and I also want to support in #10102).

I think I’d recommend just fixing the memory leak with the minimal suggestion from #12647 (review). But if you prefer moving the registration, that also seems fine with a comment mentioning the potential for race conditions.

I think maybe the best way to clean up error handling and leaks here going forward would be to move initialization logic out of the static CWallet::CreateWalletFromFile method into a non-static bool CWallet::Initialize(...) method. This way errors could just be handled by returning false from Initialize and CreateWalletFromFile would only be responsible for constructing the wallet, calling Initialize, and deleting/unregistering the wallet object in the case of errors.

jimpo commented at 5:57 am on April 16, 2018:

Is there actually a potential for race conditions? The function locks cs_main before the rescan logic and before the RegisterValidationInterface call even before it was moved.

MarcoFalke commented at 6:25 pm on April 9, 2018: member

Needs rebase

jimpo force-pushed on Apr 16, 2018

jimpo commented at 5:57 am on April 16, 2018: contributor

@MarcoFalke Rebased

jimpo force-pushed on Apr 16, 2018

jimpo force-pushed on Apr 18, 2018

wallet: Fix possible memory leak in CreateWalletFromFile.

If there was an error loading the wallet after creating the instance,
the instance would leak. This uses RAII to ensure the instance is
cleaned up in that case.

5799e57b12

promag commented at 1:59 pm on April 24, 2018: member

This conflicts with #13063. See here.

jimpo commented at 5:53 pm on April 24, 2018: contributor

No one seems to care about this (myself included), so I’ll just close in favor of #13063.

jimpo closed this on Apr 24, 2018

jimpo deleted the branch on Apr 24, 2018

jnewbery referenced this in commit 943ca84e42 on May 7, 2018

jnewbery referenced this in commit f8c81def4f on May 9, 2018

jnewbery referenced this in commit 59b87a27ef on May 15, 2018

uhliksk referenced this in commit 8ea04e9de3 on May 17, 2018

Bushstar referenced this in commit adc9eca9a4 on May 18, 2018

stamhe referenced this in commit 3d8ff8e377 on Jun 27, 2018

HashUnlimited referenced this in commit 7af4cfeed7 on Sep 7, 2018

joemphilips referenced this in commit 87a40f5222 on Nov 9, 2018

schancel referenced this in commit 4c69be0a19 on Jul 18, 2019

PastaPastaPasta referenced this in commit 8757b7c19e on Apr 14, 2020

PastaPastaPasta referenced this in commit c84df3002e on Apr 14, 2020

PastaPastaPasta referenced this in commit 3da609a80c on Apr 15, 2020

PastaPastaPasta referenced this in commit a5c16e9411 on Apr 15, 2020

PastaPastaPasta referenced this in commit 8cc3f0c93c on Apr 15, 2020

PastaPastaPasta referenced this in commit 612ae6fa69 on Apr 15, 2020

PastaPastaPasta referenced this in commit 9dfe59649c on Apr 15, 2020

PastaPastaPasta referenced this in commit b7bf1f9b06 on Apr 15, 2020

PastaPastaPasta referenced this in commit a9f5f61c0e on Apr 16, 2020

PastaPastaPasta referenced this in commit 345859caea on Apr 16, 2020

PastaPastaPasta referenced this in commit 57f75171bc on Apr 26, 2020

PastaPastaPasta referenced this in commit 7649c32ed5 on May 10, 2020

MarcoFalke locked this on Sep 8, 2021

wallet: Fix possible memory leak in CreateWalletFromFile. #12647