fix 2 (possible) memory leaks #4996

Diapolo commented at 2:18 PM on September 28, 2014: none

No description provided.

Diapolo renamed this:
~~fix some (possible) memory leaks~~
fix 2 (possible) memory leaks
on Sep 29, 2014

in src/walletdb.cpp:None in 6125a20d0c outdated

 935 | @@ -936,6 +936,7 @@ bool CWalletDB::Recover(CDBEnv& dbenv, std::string filename, bool fOnlyKeys)
 936 |      if (ret > 0)
 937 |      {
 938 |          LogPrintf("Cannot create database file %s\n", filename);
 939 | +        delete pdbCopy;

laanwj commented at 11:28 AM on September 29, 2014:

Suggestion: change pdbCopy into a boost::scoped_ptr and delete all the manual deletes

Diapolo commented at 12:27 PM on September 29, 2014:

Done!

in src/wallet.cpp:None in 6125a20d0c outdated

 426 | @@ -427,16 +427,22 @@ bool CWallet::EncryptWallet(const SecureString& strWalletPassphrase)
 427 |          if (fFileBacked)
 428 |          {
 429 |              pwalletdbEncryption = new CWalletDB(strWalletFile);
 430 | -            if (!pwalletdbEncryption->TxnBegin())
 431 | +            if (!pwalletdbEncryption->TxnBegin()) {
 432 | +                delete pwalletdbEncryption;

laanwj commented at 11:31 AM on September 29, 2014:

nit: also set it to zero after deletion

Diapolo commented at 12:04 PM on September 29, 2014:

Is this nit considered best practise after a delete call? If so you mean adding it after all delete pwalletdbEncryption; calls, right?

I guess you mean only here, because this doesn't exit ;).

laanwj commented at 12:20 PM on September 29, 2014:

It's good practice usually to set pointers to 0 after deletion to make sure that no dangling references exist. Especially in this case as the object itself is not going away after it (like in a destructor or when exit is called in the next line :p).

laanwj commented at 12:21 PM on September 29, 2014:

In this case yes just do it after all delete pwalletdbEncryption calls (except where it already happens).

in src/wallet.cpp:None in 6125a20d0c outdated

 426 | @@ -427,16 +427,22 @@ bool CWallet::EncryptWallet(const SecureString& strWalletPassphrase)
 427 |          if (fFileBacked)
 428 |          {

laanwj commented at 11:32 AM on September 29, 2014:

Suggestion: add assert(!pwalletdbEncryption) before this set

in src/wallet.cpp:None in 6125a20d0c outdated

 455 | +            if (!pwalletdbEncryption->TxnCommit()) {
 456 | +                delete pwalletdbEncryption;
 457 | +                // We now have keys encrypted in memory, but no on disk...
 458 | +                // die to avoid confusion and let the user reload their unencrypted wallet.
 459 | +                exit(1);
 460 | +            }

laanwj commented at 11:33 AM on September 29, 2014:

Suggestion: also delete pwalletdbEncryption in CWallet's destructor. This will make sure it will always be deleted when the wallet object is deleted/goes out of scope.

Diapolo commented at 11:33 AM on September 29, 2014: none

@laanwj Do you suggest using boost::scoped_ptr for both pulls or just the one in walletdb?

laanwj commented at 11:38 AM on September 29, 2014: member

@Diapolo just where I commented it; it makes no sense in the other place, as the usage of pwalletdbEncryption is too complex to just use scope-based management.

Diapolo commented at 12:27 PM on September 29, 2014: none

@laanwj Thanks for your feedback, can you re-check?

in src/wallet.cpp:None in 3dd180a977 outdated

 445 | -            exit(1); //We now probably have half of our keys encrypted in memory, and half not...die and let the user reload their unencrypted wallet.
 446 | +                delete pwalletdbEncryption;
 447 | +            }
 448 | +            // We now probably have half of our keys encrypted in memory, and half not...
 449 | +            // die and let the user reload their unencrypted wallet.
 450 | +            exit(1);

TheBlueMatt commented at 1:55 AM on October 1, 2014:

While you're at it, can you change this to an assert(false) so we can at least get a line number printed before exit?

Diapolo commented at 6:43 AM on October 1, 2014:

@TheBlueMatt AFAIK there are 2 exit(1); calls, shall I replace both with an assert?

TheBlueMatt commented at 6:45 AM on October 1, 2014:

I would.

laanwj commented at 6:52 AM on October 1, 2014:

OK it's acceptable to use assert for now, although we should be looking into replacing all asserts with a function that always gets compiled in and not gets stripped with NDEBUG.

TheBlueMatt commented at 2:02 AM on October 1, 2014: member

utACK

fix a possible memory leak in CWalletDB::Recover

- convert pdbCopy into a boost::scoped_ptr to ensure memory gets freed
  in all cases (e.g. after "ret > 0")

f606bb9baf

fix possible memory leaks in CWallet::EncryptWallet

- add missing deletes for pwalletdbEncryption
- add an assert before trying to reserve memory for pwalletdbEncryption
- add a destructor to CWallet, which ensures deletion of
  pwalletdbEncryption on object destruction

870da77da6

change exit(1) to an assert in CWallet::EncryptWallet d0c4197ef6

Diapolo commented at 6:52 AM on October 1, 2014: none

@TheBlueMatt Done, can you re-ACK ;)? @laanwj Maybe also an ACK now?

BitcoinPullTester commented at 7:09 AM on October 1, 2014: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p4996_d0c4197ef6ecfdaff792579810107e2f1b8b319e/ for binaries and test log. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.

sipa commented at 2:10 AM on October 2, 2014: member

utACK

laanwj commented at 9:49 AM on October 2, 2014: member

Untested ACK

sipa merged this on Oct 8, 2014

sipa closed this on Oct 8, 2014

sipa referenced this in commit dec58922d0 on Oct 8, 2014

Diapolo deleted the branch on Oct 9, 2014

MarcoFalke locked this on Sep 8, 2021