Skip to content

[wallet] Fix leak in CDB constructor #11492

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Oct 18, 2017
Merged

[wallet] Fix leak in CDB constructor #11492

merged 1 commit into from
Oct 18, 2017

Conversation

promag
Copy link
Contributor

@promag promag commented Oct 12, 2017

First commit fixes a minor leak.
Second commit improves the constructor in the failure cases.

ryanofsky
ryanofsky reviewed Oct 12, 2017
View reviewed changes
Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK e25b6c266ed8dad3c98de4e7c0eeb6cbdbb29533

You should note in second commit message that it is not a pure refactoring and has slightly changed behavior if the mockdb set_flags call fails.

@promag
Copy link
Contributor Author

promag commented Oct 12, 2017 •
edited
Loading

has slightly changed behavior if the mockdb set_flags call fails

Actually 2nd commit is also a fix since it doesn't ++env->mapFileUseCount[strFile] if set_flags fails. I'll amend.

ryanofsky
ryanofsky reviewed Oct 12, 2017
View reviewed changes
src/wallet/db.cpp Outdated
@@ -390,8 +390,11 @@ CDB::CDB(CWalletDBWrapper& dbw, const char* pszMode, bool fFlushOnCloseIn) : pdb
if (fMockDb) {
DbMpoolFile* mpf = pdb->get_mpf();
ret = mpf->set_flags(DB_MPOOL_NOFILE, 1);
if (ret != 0)
if (ret != 0) {
delete pdb;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe another way you could fix this without adding a delete call would be to declare pdb as a unique_ptr in the header? This would also let you remove the other deletes below.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And in std::map<std::string, Db*> CDBEnv::mapDb?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And in std::map<std::string, Db*> CDBEnv::mapDb?

Oops, sorry. The pdb member should not be a unique_ptr because the pointer is actually owned (deleted) by mapdb, not the cdb class.

If you wanted to do a minimal fix you could declare a unique_ptr<Db> temp_db here and then do pdb = temp_db.release() at the end of this function.

If you wanted to go further, you could change mapDb to hold unique_ptrs and std::move from temp_db into the map at the end of this function.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't go that further in a bugfix. But I'll probably do that in a different PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, 0d3ec0c is the minimal fix for reference.

Copy link
Contributor Author

@promag promag Oct 13, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anyway, the calls Exists and Write below use pdb internally. I believe this is the minimal fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, 0d3ec0c is the minimal fix for reference.

Had the same, but see my previous comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I thought of not doing pdb = nullptr but if this code is moved to some member function then that can a problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I can .release() before Exists().

@promag promag force-pushed the 2017-10-cdb-constructor-leak branch from e25b6c2 to a5a900c Compare October 13, 2017 15:21
@promag
Copy link
Contributor Author

promag commented Oct 13, 2017

@ryanofsky now with unique_ptr but the release is before. I would squash the 3 commits before merge.

@promag promag force-pushed the 2017-10-cdb-constructor-leak branch 2 times, most recently from dfa34e7 to 4e17b6e Compare October 13, 2017 15:32
ryanofsky
ryanofsky reviewed Oct 13, 2017
View reviewed changes
Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK 4e17b6e1f76fcc98b7b5326c94127099e9e8f382. First two commits are unchanged since last review except for commit message. Third commit adds unique_ptr.

@promag promag force-pushed the 2017-10-cdb-constructor-leak branch from 4e17b6e to 79c3929 Compare October 14, 2017 22:58
@promag
Copy link
Contributor Author

promag commented Oct 14, 2017

Squashed and added a detailed description in the commit message.

@promag
[wallet] Fix leak in CDB constructor
7104de8 
Now using a std::unique_ptr, the Db instance is correctly released
when CDB initialization fails.
The internal CDB state and mapFileUseCount are only mutated when
the CDB initialization succeeds.
@promag promag force-pushed the 2017-10-cdb-constructor-leak branch from 79c3929 to 7104de8 Compare October 14, 2017 23:00
@promag promag changed the title Fix leak in CDB constructor [wallet] Fix leak in CDB constructor Oct 14, 2017
ryanofsky
ryanofsky reviewed Oct 16, 2017
View reviewed changes
Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK 7104de8 (squashed commit, no changes)

@laanwj
Copy link
Member

laanwj commented Oct 18, 2017

utACK 7104de8

@laanwj laanwj merged commit 7104de8 into bitcoin:master Oct 18, 2017
laanwj added a commit that referenced this pull request Oct 18, 2017
@laanwj
Merge #11492: [wallet] Fix leak in CDB constructor
b645f36 
7104de8 [wallet] Fix leak in CDB constructor (João Barbosa)

Pull request description:

  First commit fixes a minor leak.
  Second commit improves the constructor in the failure cases.

Tree-SHA512: 5165413d60ed9fc28203c9fe128adbba03a9ea9e9aa3734d9ea2522dafd815ba0fb8b90fd0809dbc06eb3ad360e7764de01dadf653ade3350fe86f6b8f04bc90
@laanwj laanwj mentioned this pull request Oct 19, 2017
Merged
@promag promag deleted the 2017-10-cdb-constructor-leak branch October 19, 2017 16:17
maflcko pushed a commit to maflcko/bitcoin-core that referenced this pull request Nov 1, 2017
@promag
[wallet] Fix leak in CDB constructor
de7053f 
Now using a std::unique_ptr, the Db instance is correctly released
when CDB initialization fails.
The internal CDB state and mapFileUseCount are only mutated when
the CDB initialization succeeds.

Github-Pull: bitcoin#11492
Rebased-From: 7104de8
@morcos morcos mentioned this pull request Nov 2, 2017
Merged
codablock pushed a commit to codablock/dash that referenced this pull request Sep 26, 2019
@laanwj @codablock
Merge bitcoin#11492: [wallet] Fix leak in CDB constructor
ae31182 
7104de8 [wallet] Fix leak in CDB constructor (João Barbosa)

Pull request description:

  First commit fixes a minor leak.
  Second commit improves the constructor in the failure cases.

Tree-SHA512: 5165413d60ed9fc28203c9fe128adbba03a9ea9e9aa3734d9ea2522dafd815ba0fb8b90fd0809dbc06eb3ad360e7764de01dadf653ade3350fe86f6b8f04bc90
codablock pushed a commit to codablock/dash that referenced this pull request Sep 29, 2019
@laanwj @codablock
Merge bitcoin#11492: [wallet] Fix leak in CDB constructor
d05801c 
7104de8 [wallet] Fix leak in CDB constructor (João Barbosa)

Pull request description:

  First commit fixes a minor leak.
  Second commit improves the constructor in the failure cases.

Tree-SHA512: 5165413d60ed9fc28203c9fe128adbba03a9ea9e9aa3734d9ea2522dafd815ba0fb8b90fd0809dbc06eb3ad360e7764de01dadf653ade3350fe86f6b8f04bc90
barrystyle pushed a commit to PACGlobalOfficial/PAC that referenced this pull request Jan 22, 2020
@laanwj
Merge bitcoin#11492: [wallet] Fix leak in CDB constructor
b8f7ced 
7104de8 [wallet] Fix leak in CDB constructor (João Barbosa)

Pull request description:

  First commit fixes a minor leak.
  Second commit improves the constructor in the failure cases.

Tree-SHA512: 5165413d60ed9fc28203c9fe128adbba03a9ea9e9aa3734d9ea2522dafd815ba0fb8b90fd0809dbc06eb3ad360e7764de01dadf653ade3350fe86f6b8f04bc90
@random-zebra random-zebra mentioned this pull request Apr 28, 2021
Merged
1 task
random-zebra added a commit to PIVX-Project/PIVX that referenced this pull request Jun 9, 2021
@random-zebra
Merge #2351: [Wallet] Laggard wallet-related backports from btc 0.15
2d50b6e 
d5526bd Wrap dumpwallet warning and note scripts aren't dumped (MeshCollider)
3711c6a Add wallet backup text to import*, add* and dumpwallet RPCs (MeshCollider)
dbda874 [Wallet] always show help-line of wallet encryption calls (Jonas Schnelli)
20c269b Avoid opening copied wallet databases simultaneously (Russell Yanofsky)
e411b70 [wallet] Fix leak in CDB constructor (João Barbosa)
f15aeea Change getmininginfo errors field to warnings (Andrew Chow)
c04390b Unify help text for GetWarnings output in get*info RPCs (random-zebra)
1d966ce Add warnings field to getblockchaininfo (Andrew Chow)
ffcd781 [Trivial] Cleanup after MOVE-ONLY commits (random-zebra)
e067235 MOVEONLY: Init functions wallet/wallet.cpp -> wallet/init.cpp (random-zebra)
e947eec MOVEONLY: Fee functions wallet/wallet.cpp -> wallet/fees.cpp (random-zebra)
2188c3e Move some static functions out of wallet.h/cpp (random-zebra)
f49acf7 [wallet] [moveonly] Move CAffectedKeysVisitor (random-zebra)
8bd979f [wallet] Specify wallet name in wallet loading errors (random-zebra)
900bbfa Reject invalid wallet files (João Barbosa)
a1f4e2a Reject duplicate wallet filenames (random-zebra)
ee52c2e Fix misleading "Method not found" multiwallet errors (Russell Yanofsky)
ce35e1e [Qt] Use wallet 0 in rpc console if running with multiple wallets (random-zebra)
37089d1 [tests] Update wallet_multiwallet.py functional test (random-zebra)
3955ee9 [Doc] Update release notes (random-zebra)
4fd5913 [wallet] [rpc] Add listwallets RPC (John Newbery)
1525281 [wallet] [rpc] print wallet name in getwalletinfo (John Newbery)
fdf5da0 [wallet] fix comment for CWallet::Verify() (John Newbery)
cf4a90b Remove factor of 3 from definition of dust. (random-zebra)
a1c56fd [Policy] Introduce -dustrelayfee (random-zebra)
9fb29cc [Doc] Update multiwallet release notes (random-zebra)
379255e [Tests][Trivial] Add wallet_multiwallet.py to test_runner (random-zebra)
808fbc3 [Bugfix] consider boolean value of -zapwallettxes ParameterInteraction (random-zebra)
f9141f8 [QA] Add wallet_multiwallet.py functional test (John Newbery)
2e02006 Rename -usewallet to -rpcwallet (Alex Morcos)
a64440b Select wallet based on the given endpoint (Jonas Schnelli)
5683a9c Complete previous commit by moving mn stuff out of libbitcoin_wallet (random-zebra)
b0fe92f Fix test_pivx circular dependency issue (random-zebra)
6cb2b92 Add wallet endpoint support to bitcoin-cli (-usewallet) (Jonas Schnelli)
7dd3916 Register wallet endpoint (Jonas Schnelli)
5bd1bd7 Properly forbid -salvagewallet and -zapwallettxes for multi wallet. (Alex Morcos)
41a7335 Remove unused variables (practicalswift)
5c3d73f Avoid CWalletTx copies in GetAddressBalances and GetAddressGroupings (Russell Yanofsky)
e7cafab [Refactoring] Mimic ListCoins for sapling notes (random-zebra)
54fa122 [qt] Move some WalletModel functions into CWallet (random-zebra)
494ba64 [test] Add test for getmemoryinfo (random-zebra)
2394083 [Wallet] unset change position when there is no change on exact match (Gregory Sanders)
7d977ac Remove unused C++ code not covered by unit tests (random-zebra)
60bb4da ApproximateBestSubset should take inputs by reference, not value (Ryan Havar)
3633d75 Initialize nRelockTime (Patrick Strateman)
3a599d0 [Refactor] Return safeTx boolean in CheckTXAvailability (random-zebra)
f219be9 Add safe flag to listunspent result (NicolasDorier)
0201065 Add COutput::fSafe member for safe handling of unconfirmed outputs (random-zebra)
75c8c6d Disallow copy of CReserveKeys (Gregory Sanders)
8378322 [Refactor] Replace optional reserveKey in PBF with unique pointer (random-zebra)

Pull request description:

  I think these are all the remaining Bitcoin Core v0.15 PRs in the wallet area that we don't have yet, and are useful to us.
  I've grouped them here since they are all pretty small, simple, and narrow-focused (on the wallet/rpcwallet files).

  This changeset is based on top of
  - [x] #2337

  as it keeps going with the multiwallet implementation, adding:
  - RPC endpoint support
  - `listwallets` RPC
  - wallet name in `getwalletinfo` RPC
  - `wallet_multiwallet.py` functional test

  As in some areas we are much closer to upstream, some of the commits needed adaptations (especially the functional tests). A couple of commits have been extended to include shield-related code.

  List of upstream PRs backported/adapted:

  - bitcoin#9906  : Disallow copy constructor CReserveKeys
  - bitcoin#9830  : Add safe flag to listunspent result
  - bitcoin#9993  : Initialize nRelockTime
  - bitcoin#10108 : ApproximateBestSubset should take inputs by reference, not value
  - bitcoin#10075 : Remove unused C++ code not covered by unit tests
  - bitcoin#10257 : Add test for getmemoryinfo
  - bitcoin#10295 : Move some WalletModel functions into CWallet
  - bitcoin#10500 : Avoid CWalletTx copies in GetAddressBalances and GetAddressGroupings
  - bitcoin#10522 : Remove unused variables
  - bitcoin#10816 : Properly forbid -salvagewallet and -zapwallettxes for multi wallet
  - bitcoin#10849 : Multiwallet: simplest endpoint support
  - bitcoin#9380  : Separate different uses of minimum fees (only dustRelayFee)
  - bitcoin#10817 : Redefine Dust and add a discard_rate (only first commit)
  - bitcoin#10883 : Rename -usewallet to -rpcwallet
  - bitcoin#10604 : Add listwallets RPC, include wallet name in getwalletinfo + tests
  - bitcoin#10893 : Avoid running multiwallet.py twice
  - bitcoin#10870 : Use wallet 0 in rpc console if running with multiple wallets
  - bitcoin#10931 : Fix misleading "method not found" multiwallet errors
  - bitcoin#10885 : Reject invalid wallets
  - bitcoin#11022 : Basic keypool topup
  - bitcoin#10976 : [MOVEONLY] Move some static functions out of wallet.h/cpp
  - bitcoin#11310 : Test listwallets RPC
  - bitcoin#10858 : "errors" to getblockchaininfo + unify "errors" field in get*info RPCs
  - bitcoin#11492 : Fix leak in CDB constructor
  - bitcoin#11476 : Avoid opening copied wallet databases simultaneously
  - bitcoin#11590 : always show help-line of wallet encryption calls
  - bitcoin#11289 : Add wallet backup text to import* and add* RPCs

ACKs for top commit:
  furszy:
    re-re-utACK d5526bd.
  Fuzzbawls:
    ACK d5526bd

Tree-SHA512: 115c4916ee212539b0a1b2cb25783ddf6753f5376de739a084191e874ed8717fff2c7cd9d744e397891f14eaa459ef2f48d675168621ef7316e81758fa6559f2
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ryanofsky ryanofsky ryanofsky left review comments

Assignees
No one assigned
Labels
Wallet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@promag @laanwj @ryanofsky @fanquake