[wallet] Close DB on error. #11017

pull kallewoof wants to merge 1 commits into bitcoin:master from kallewoof:close-dbenv changing 1 files +8 −4

kallewoof commented at 8:44 AM on August 9, 2017: member

This PR intends to plug some leaks. It specifically implements adherence to the requirement in BDB to close a handle which failed to open (https://docs.oracle.com/cd/E17276_01/html/api_reference/C/dbopen.html):

The DB->open() method returns a non-zero error value on failure and 0 on success. If DB->open() fails, the DB->close() method must be called to discard the DB handle.
[wallet] Close DB on error. 03bc719a85
practicalswift commented at 8:49 AM on August 9, 2017: contributor

Concept ACK! Nice find 👍
fanquake added the label Wallet on Aug 9, 2017
gmaxwell approved
gmaxwell commented at 4:30 AM on August 10, 2017: contributor

utACK
MarcoFalke commented at 8:46 AM on August 10, 2017: member

utACK 03bc719a85cb4928cb4b43d0bc4142f72cb01b23
paveljanik commented at 11:14 AM on August 10, 2017: contributor

utACK 03bc719

in src/wallet/db.cpp:544 in 03bc719a85

 537 | @@ -536,8 +538,10 @@ bool CDB::Rewrite(CWalletDBWrapper& dbw, const char* pszSkip)
 538 |                          env->CloseDb(strFile);
 539 |                          if (pdbCopy->close(0))
 540 |                              fSuccess = false;
 541 | -                        delete pdbCopy;
 542 | +                    } else {
 543 | +                        pdbCopy->close(0);
 544 |                      }
 545 | +                    delete pdbCopy;

laanwj commented at 6:55 PM on August 10, 2017:

Shouldn't it always close pdbCopy before deleting it, not just on failure?

kallewoof commented at 9:29 PM on August 10, 2017:

Line #537 closes for fSuccess being false and #542 for true. Am I missing a case?

promag commented at 1:28 AM on August 12, 2017: member

utACK 03bc719.

BTW, how did you catch this?
kallewoof commented at 8:48 AM on August 14, 2017: member

@promag I ran bitcoind through Instruments on a mac, and noticed I think 40k leaks (not 40 kb, 40 thousand occurrences..) on startup. One of the traces for leaked memory was the DB open call.
jonasschnelli commented at 10:23 AM on August 14, 2017: contributor

utACK 03bc719a85cb4928cb4b43d0bc4142f72cb01b23
jonasschnelli merged this on Aug 15, 2017
jonasschnelli closed this on Aug 15, 2017
jonasschnelli referenced this in commit ae47724687 on Aug 15, 2017
MarcoFalke referenced this in commit 9e8aae31c1 on Oct 3, 2017
schancel referenced this in commit bcaab422dc on Apr 19, 2019
sickpig cross-referenced this on Apr 25, 2019 from issue [PORT] [wallet] Close DB on error by sickpig
proteanx referenced this in commit 65214c6dc5 on Apr 26, 2019
Greg-Griffith referenced this in commit f2e99833f9 on May 1, 2019
Greg-Griffith cross-referenced this on May 1, 2019 from issue [PORT] [wallet] Close DB on error by Greg-Griffith
PastaPastaPasta referenced this in commit 411b0aae09 on Sep 19, 2019
PastaPastaPasta referenced this in commit 8abbae16ed on Sep 23, 2019
PastaPastaPasta referenced this in commit fbae5aa260 on Sep 24, 2019
codablock referenced this in commit 76776fb62e on Sep 25, 2019
kallewoof deleted the branch on Oct 17, 2019
barrystyle referenced this in commit 299a3a10ca on Jan 22, 2020
Fuzzbawls cross-referenced this on Feb 7, 2021 from issue [Wallet] Close DB on error by Fuzzbawls
furszy referenced this in commit d3afb339b8 on Feb 12, 2021
barton2526 referenced this in commit 686bdb383c on Jul 12, 2021
barton2526 referenced this in commit bca2f93fdf on Jul 12, 2021
barton2526 cross-referenced this on Jul 12, 2021 from issue wallet: Close DB on error, use memory_cleanse by barton2526
barton2526 referenced this in commit 1e5c60e9c7 on Jul 12, 2021
bitcoin locked this on Dec 16, 2021

Contributors

Labels

Linked (view graph)

#537 remove deprecated RPCs #542 qmake build system improvements