wallet: Fix possible memory leak in CreateWalletFromFile. #12647

jimpo · 2018-03-08T15:41:33Z

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.

eklitzke · 2018-03-11T02:26:48Z

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

promag

I think it's fine moving down the registration.

promag · 2018-03-11T18:57:58Z

src/wallet/wallet.cpp

@@ -3929,7 +3929,7 @@ CWallet* CWallet::CreateWalletFromFile(const std::string& name, const fs::path&

    int64_t nStart = GetTimeMillis();
    bool fFirstRun = true;
-    CWallet *walletInstance = new CWallet(name, CWalletDBWrapper::Create(path));
+    std::unique_ptr<CWallet> walletInstance(new CWallet(name, CWalletDBWrapper::Create(path)));


Use MakeUnique?

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

Why do you say it would construct two?

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

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.

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

promag · 2018-03-11T18:58:27Z

src/wallet/wallet.cpp

+
+    CWallet* wallet_ptr = walletInstance.release();
+    RegisterValidationInterface(wallet_ptr);
+    return wallet_ptr;


Release on return?

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.

RegisterValidationInterface failed for some reason

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

jimpo · 2018-03-11T23:21:19Z

@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.

promag · 2018-03-13T14:18:51Z

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

ryanofsky · 2018-03-15T17:56:46Z

src/wallet/wallet.cpp

@@ -4025,9 +4025,6 @@ CWallet* CWallet::CreateWalletFromFile(const std::string& name, const fs::path&
            pindexRescan = FindForkInGlobalIndex(chainActive, locator);
    }

-    walletInstance->m_last_block_processed = chainActive.Tip();
-    RegisterValidationInterface(walletInstance);


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:

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

And changing this line to:

RegisterValidationInterface(wallet_deleter.release());

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?

#12647 (comment)

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 · 2018-03-20T15:16:04Z

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.

ryanofsky · 2018-04-06T15:17:02Z

src/wallet/wallet.h

@@ -789,6 +789,10 @@ class CWallet final : public CCryptoKeyStore, public CValidationInterface

    ~CWallet()
    {
+        // Even if RegisterValidationInstance was never called on this instance,
+        // the unregister call is safe to make and is a no-op.
+        UnregisterValidationInterface(this);


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

bitcoin/src/interface/chain.cpp

Lines 163 to 169 in 0bdf0e3

~HandlerImpl() override

{

// Don't call UnregisterValidationInterface here because it would try to

// access virtual methods on this object which can't be accessed during

// destruction. Also UnregisterAllValidationInterfaces is already called

// at this point, so unregistering this object would be redundant.

}

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

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 (comment). 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.

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.

maflcko · 2018-04-09T18:25:50Z

Needs rebase

jimpo · 2018-04-16T05:57:34Z

@MarcoFalke Rebased

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.

promag · 2018-04-24T13:59:49Z

This conflicts with #13063. See here.

jimpo · 2018-04-24T17:53:06Z

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

Fix proposed by ryanofsky in bitcoin#12647 (comment)

Fix proposed by ryanofsky in bitcoin/bitcoin#12647 (comment)

Fix proposed by ryanofsky in bitcoin#12647 (comment)

Summary: ``` Fix proposed by ryanofsky in bitcoin/bitcoin#12647 (comment) ``` This cleans the `wallet_hd.py` with asan. Partial backport of core PR10740 (commit 59b87a2) bitcoin/bitcoin@59b87a2 Progress towards T459 Depends on D3618 Test Plan: make check With ASAN enabled: ./test/functional/test_runner.py wallet_hd Reviewers: #bitcoin_abc, deadalnix, markblundeberg Reviewed By: #bitcoin_abc, markblundeberg Differential Revision: https://reviews.bitcoinabc.org/D3629

Fix proposed by ryanofsky in bitcoin#12647 (comment)

meshcollider added the Wallet label Mar 9, 2018

promag reviewed Mar 11, 2018

View reviewed changes

jimpo force-pushed the wallet-pointer branch from 9378d89 to 7dc50d4 Compare March 12, 2018 16:38

ryanofsky reviewed Mar 15, 2018

View reviewed changes

jimpo force-pushed the wallet-pointer branch from 7dc50d4 to ea2f3d4 Compare March 23, 2018 22:58

jimpo force-pushed the wallet-pointer branch from ea2f3d4 to effd82a Compare April 3, 2018 03:13

ryanofsky reviewed Apr 6, 2018

View reviewed changes

ryanofsky mentioned this pull request Apr 11, 2018

Build tx index in parallel with validation #11857

Closed

jimpo force-pushed the wallet-pointer branch from effd82a to 5f77db7 Compare April 16, 2018 05:55

jimpo force-pushed the wallet-pointer branch 2 times, most recently from 1fd6823 to 5799e57 Compare April 18, 2018 17:20

wallet: Fix possible memory leak in CreateWalletFromFile.

5799e57

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.

jimpo closed this Apr 24, 2018

jimpo deleted the wallet-pointer branch April 24, 2018 17:53

promag mentioned this pull request May 7, 2018

[wallet] loadwallet RPC - load wallet at runtime #10740

Merged

jnewbery added a commit to jnewbery/bitcoin that referenced this pull request May 15, 2018

[wallet] Fix potential memory leak in CreateWalletFromFile

59b87a2

Fix proposed by ryanofsky in bitcoin#12647 (comment)

uhliksk pushed a commit to uhliksk/fixed-trade-coin that referenced this pull request May 17, 2018

[wallet] Fix potential memory leak in CreateWalletFromFile

8ea04e9

Fix proposed by ryanofsky in bitcoin#12647 (comment)

Bushstar pushed a commit to Bushstar/Feathercoin that referenced this pull request May 18, 2018

[wallet] Fix potential memory leak in CreateWalletFromFile

adc9eca

Fix proposed by ryanofsky in bitcoin/bitcoin#12647 (comment)

stamhe pushed a commit to stamhe/bitcoin that referenced this pull request Jun 27, 2018

[wallet] Fix potential memory leak in CreateWalletFromFile

3d8ff8e

Fix proposed by ryanofsky in bitcoin#12647 (comment)

HashUnlimited pushed a commit to HashUnlimited/chaincoin that referenced this pull request Sep 7, 2018

[wallet] Fix potential memory leak in CreateWalletFromFile

7af4cfe

Fix proposed by ryanofsky in bitcoin#12647 (comment)

PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Apr 14, 2020

[wallet] Fix potential memory leak in CreateWalletFromFile

8757b7c

Fix proposed by ryanofsky in bitcoin#12647 (comment)

PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Apr 14, 2020

[wallet] Fix potential memory leak in CreateWalletFromFile

c84df30

Fix proposed by ryanofsky in bitcoin#12647 (comment)

PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Apr 15, 2020

[wallet] Fix potential memory leak in CreateWalletFromFile

3da609a

Fix proposed by ryanofsky in bitcoin#12647 (comment)

PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Apr 15, 2020

[wallet] Fix potential memory leak in CreateWalletFromFile

a5c16e9

Fix proposed by ryanofsky in bitcoin#12647 (comment)

PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Apr 15, 2020

[wallet] Fix potential memory leak in CreateWalletFromFile

8cc3f0c

Fix proposed by ryanofsky in bitcoin#12647 (comment)

PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Apr 15, 2020

[wallet] Fix potential memory leak in CreateWalletFromFile

612ae6f

Fix proposed by ryanofsky in bitcoin#12647 (comment)

PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Apr 15, 2020

[wallet] Fix potential memory leak in CreateWalletFromFile

9dfe596

Fix proposed by ryanofsky in bitcoin#12647 (comment)

PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Apr 15, 2020

[wallet] Fix potential memory leak in CreateWalletFromFile

b7bf1f9

Fix proposed by ryanofsky in bitcoin#12647 (comment)

PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Apr 16, 2020

[wallet] Fix potential memory leak in CreateWalletFromFile

a9f5f61

Fix proposed by ryanofsky in bitcoin#12647 (comment)

PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Apr 16, 2020

[wallet] Fix potential memory leak in CreateWalletFromFile

345859c

Fix proposed by ryanofsky in bitcoin#12647 (comment)

PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Apr 26, 2020

[wallet] Fix potential memory leak in CreateWalletFromFile

57f7517

Fix proposed by ryanofsky in bitcoin#12647 (comment)

PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request May 10, 2020

[wallet] Fix potential memory leak in CreateWalletFromFile

7649c32

Fix proposed by ryanofsky in bitcoin#12647 (comment)

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

wallet: Fix possible memory leak in CreateWalletFromFile. #12647

wallet: Fix possible memory leak in CreateWalletFromFile. #12647

jimpo commented Mar 8, 2018 •

edited

eklitzke commented Mar 11, 2018 •

edited

promag left a comment

promag Mar 11, 2018

jimpo Mar 12, 2018

promag Mar 12, 2018

jimpo Mar 12, 2018

promag Mar 12, 2018

jimpo Mar 12, 2018

promag Mar 11, 2018

jimpo Mar 11, 2018

promag Mar 12, 2018

jimpo commented Mar 11, 2018

promag commented Mar 13, 2018 •

edited

ryanofsky Mar 15, 2018

jimpo Mar 16, 2018

ryanofsky Mar 20, 2018

ryanofsky commented Mar 20, 2018

ryanofsky Apr 6, 2018 •

edited

jimpo Apr 6, 2018

ryanofsky Apr 10, 2018

jimpo Apr 16, 2018

maflcko commented Apr 9, 2018

jimpo commented Apr 16, 2018

promag commented Apr 24, 2018

jimpo commented Apr 24, 2018

	~HandlerImpl() override
	{
	// Don't call UnregisterValidationInterface here because it would try to
	// access virtual methods on this object which can't be accessed during
	// destruction. Also UnregisterAllValidationInterfaces is already called
	// at this point, so unregistering this object would be redundant.
	}

wallet: Fix possible memory leak in CreateWalletFromFile. #12647

wallet: Fix possible memory leak in CreateWalletFromFile. #12647

Conversation

jimpo commented Mar 8, 2018 • edited

eklitzke commented Mar 11, 2018 • edited

promag left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimpo commented Mar 11, 2018

promag commented Mar 13, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanofsky commented Mar 20, 2018

ryanofsky Apr 6, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maflcko commented Apr 9, 2018

jimpo commented Apr 16, 2018

promag commented Apr 24, 2018

jimpo commented Apr 24, 2018

jimpo commented Mar 8, 2018 •

edited

eklitzke commented Mar 11, 2018 •

edited

promag commented Mar 13, 2018 •

edited

ryanofsky Apr 6, 2018 •

edited