[wallet] Fix leak in CDB constructor

promag commented at 10:22 pm on October 12, 2017: member

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

ryanofsky commented at 10:30 pm on October 12, 2017: member

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 commented at 10:35 pm on October 12, 2017: member

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.

in src/wallet/db.cpp:394 in a5386db6d1 outdated

389@@ -390,8 +390,11 @@ CDB::CDB(CWalletDBWrapper& dbw, const char* pszMode, bool fFlushOnCloseIn) : pdb
390             if (fMockDb) {
391                 DbMpoolFile* mpf = pdb->get_mpf();
392                 ret = mpf->set_flags(DB_MPOOL_NOFILE, 1);
393-                if (ret != 0)
394+                if (ret != 0) {
395+                    delete pdb;

ryanofsky commented at 10:46 pm on October 12, 2017:

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.

promag commented at 11:18 pm on October 12, 2017:

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

ryanofsky commented at 2:15 pm on October 13, 2017:

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.

promag commented at 2:44 pm on October 13, 2017:

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

ryanofsky commented at 3:15 pm on October 13, 2017:

Ok, 0d3ec0c3a15e7afde42b3b98e369d989a6d9e7f7 is the minimal fix for reference.

promag commented at 3:16 pm on October 13, 2017:

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

promag commented at 3:18 pm on October 13, 2017:

Ok, 0d3ec0c is the minimal fix for reference.

Had the same, but see my previous comment.

promag commented at 3:20 pm on October 13, 2017:

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

promag commented at 3:24 pm on October 13, 2017:

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

fanquake added the label Wallet on Oct 13, 2017

promag force-pushed on Oct 13, 2017

promag commented at 3:30 pm on October 13, 2017: member

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

promag force-pushed on Oct 13, 2017

ryanofsky commented at 3:40 pm on October 13, 2017: member

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

promag force-pushed on Oct 14, 2017

promag commented at 10:59 pm on October 14, 2017: member

Squashed and added a detailed description in the commit message.

[wallet] Fix leak in CDB constructor

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.

7104de8b1f

promag force-pushed on Oct 14, 2017

promag renamed this:
~~Fix leak in CDB constructor~~
[wallet] Fix leak in CDB constructor
on Oct 14, 2017

ryanofsky commented at 2:44 pm on October 16, 2017: member

utACK 7104de8b1f3a31d3a60009b5dc376adbedac6a9c (squashed commit, no changes)

laanwj commented at 1:39 pm on October 18, 2017: member

utACK 7104de8

laanwj merged this on Oct 18, 2017

laanwj closed this on Oct 18, 2017

laanwj referenced this in commit b645f368f2 on Oct 18, 2017

promag deleted the branch on Oct 19, 2017

MarcoFalke referenced this in commit abe313471f on Nov 1, 2017

MarcoFalke referenced this in commit de7053f114 on Nov 1, 2017

codablock referenced this in commit ae3118231e on Sep 26, 2019

codablock referenced this in commit d05801c8a2 on Sep 29, 2019

barrystyle referenced this in commit b8f7ced15f on Jan 22, 2020

random-zebra referenced this in commit 2d50b6e3b9 on Jun 9, 2021

DrahtBot locked this on Sep 8, 2021

[wallet] Fix leak in CDB constructor #11492