Skip to content

Commit 5e9be16

Browse files
laanwjlaanwj
committed
Merge #11043: Use std::unique_ptr (C++11) where possible
a357293 Use MakeUnique<Db>(...) (practicalswift) 3e09b39 Use MakeUnique<T>(...) instead of std::unique_ptr<T>(new T(...)) (practicalswift) 8617989 Add MakeUnique (substitute for C++14 std::make_unique) (practicalswift) d223bc9 Use unique_ptr for pcoinscatcher/pcoinsdbview/pcoinsTip/pblocktree (practicalswift) b45c597 Use unique_ptr for pdbCopy (Db) and fix potential memory leak (practicalswift) 29ab96d Use unique_ptr for dbenv (DbEnv) (practicalswift) f72cbf9 Use unique_ptr for pfilter (CBloomFilter) (practicalswift) 8ccf1bb Use unique_ptr for sem{Addnode,Outbound} (CSemaphore) (practicalswift) 73db063 Use unique_ptr for upnp_thread (boost::thread) (practicalswift) 0024531 Use unique_ptr for dbw (CDBWrapper) (practicalswift) fa6d122 Use unique_ptr:s for {fee,short,long}Stats (TxConfirmStats) (practicalswift) 5a6f768 Use unique_ptr for httpRPCTimerInterface (HTTPRPCTimerInterface) (practicalswift) 860e912 Use unique_ptr for pwalletMain (CWallet) (practicalswift) Pull request description: Use `std::unique_ptr` (C++11) where possible. Rationale: 1. Avoid resource leaks (specifically: forgetting to `delete` an object created using `new`) 2. Avoid undefined behaviour (specifically: double `delete`:s) **Note to reviewers:** Please let me know if I've missed any obvious `std::unique_ptr` candidates. Hopefully this PR should cover all the trivial cases. Tree-SHA512: 9fbeb47b800ab8ff4e0be9f2a22ab63c23d5c613a0c6716d9183db8d22ddbbce592fb8384a8b7874bf7375c8161efb13ca2197ad6f24b75967148037f0f7b20c
2 parents 23e9074 + a357293 commit 5e9be16

20 files changed

+104
-131
lines changed

‎src/httprpc.cpp

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -62,7 +62,7 @@ class HTTPRPCTimerInterface : public RPCTimerInterface
6262
/* Pre-base64-encoded authentication token */
6363
static std::string strRPCUserColonPass;
6464
/* Stored RPC timer interface (for unregistration) */
65-
static HTTPRPCTimerInterface* httpRPCTimerInterface = nullptr;
65+
static std::unique_ptr<HTTPRPCTimerInterface> httpRPCTimerInterface;
6666

6767
static void JSONErrorReply(HTTPRequest* req, const UniValue& objError, const UniValue& id)
6868
{
@@ -238,8 +238,8 @@ bool StartHTTPRPC()
238238
RegisterHTTPHandler("/wallet/", false, HTTPReq_JSONRPC);
239239
#endif
240240
assert(EventBase());
241-
httpRPCTimerInterface = new HTTPRPCTimerInterface(EventBase());
242-
RPCSetTimerInterface(httpRPCTimerInterface);
241+
httpRPCTimerInterface = MakeUnique<HTTPRPCTimerInterface>(EventBase());
242+
RPCSetTimerInterface(httpRPCTimerInterface.get());
243243
return true;
244244
}
245245

@@ -253,8 +253,7 @@ void StopHTTPRPC()
253253
LogPrint(BCLog::RPC, "Stopping HTTP RPC server\n");
254254
UnregisterHTTPHandler("/", true);
255255
if (httpRPCTimerInterface) {
256-
RPCUnsetTimerInterface(httpRPCTimerInterface);
257-
delete httpRPCTimerInterface;
258-
httpRPCTimerInterface = nullptr;
256+
RPCUnsetTimerInterface(httpRPCTimerInterface.get());
257+
httpRPCTimerInterface.reset();
259258
}
260259
}

‎src/init.cpp

Lines changed: 14 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -152,7 +152,7 @@ class CCoinsViewErrorCatcher final : public CCoinsViewBacked
152152
// Writes do not need similar protection, as failure to write is handled by the caller.
153153
};
154154

155-
static CCoinsViewErrorCatcher *pcoinscatcher = nullptr;
155+
static std::unique_ptr<CCoinsViewErrorCatcher> pcoinscatcher;
156156
static std::unique_ptr<ECCVerifyHandle> globalVerifyHandle;
157157

158158
void Interrupt(boost::thread_group& threadGroup)
@@ -235,14 +235,10 @@ void Shutdown()
235235
if (pcoinsTip != nullptr) {
236236
FlushStateToDisk();
237237
}
238-
delete pcoinsTip;
239-
pcoinsTip = nullptr;
240-
delete pcoinscatcher;
241-
pcoinscatcher = nullptr;
242-
delete pcoinsdbview;
243-
pcoinsdbview = nullptr;
244-
delete pblocktree;
245-
pblocktree = nullptr;
238+
pcoinsTip.reset();
239+
pcoinscatcher.reset();
240+
pcoinsdbview.reset();
241+
pblocktree.reset();
246242
}
247243
#ifdef ENABLE_WALLET
248244
StopWallets();
@@ -1406,12 +1402,10 @@ bool AppInitMain(boost::thread_group& threadGroup, CScheduler& scheduler)
14061402
do {
14071403
try {
14081404
UnloadBlockIndex();
1409-
delete pcoinsTip;
1410-
delete pcoinsdbview;
1411-
delete pcoinscatcher;
1412-
delete pblocktree;
1413-
1414-
pblocktree = new CBlockTreeDB(nBlockTreeDBCache, false, fReset);
1405+
pcoinsTip.reset();
1406+
pcoinsdbview.reset();
1407+
pcoinscatcher.reset();
1408+
pblocktree.reset(new CBlockTreeDB(nBlockTreeDBCache, false, fReset));
14151409

14161410
if (fReset) {
14171411
pblocktree->WriteReindexing(true);
@@ -1462,8 +1456,8 @@ bool AppInitMain(boost::thread_group& threadGroup, CScheduler& scheduler)
14621456
// At this point we're either in reindex or we've loaded a useful
14631457
// block tree into mapBlockIndex!
14641458

1465-
pcoinsdbview = new CCoinsViewDB(nCoinDBCache, false, fReset || fReindexChainState);
1466-
pcoinscatcher = new CCoinsViewErrorCatcher(pcoinsdbview);
1459+
pcoinsdbview.reset(new CCoinsViewDB(nCoinDBCache, false, fReset || fReindexChainState));
1460+
pcoinscatcher.reset(new CCoinsViewErrorCatcher(pcoinsdbview.get()));
14671461

14681462
// If necessary, upgrade from older database format.
14691463
// This is a no-op if we cleared the coinsviewdb with -reindex or -reindex-chainstate
@@ -1473,13 +1467,13 @@ bool AppInitMain(boost::thread_group& threadGroup, CScheduler& scheduler)
14731467
}
14741468

14751469
// ReplayBlocks is a no-op if we cleared the coinsviewdb with -reindex or -reindex-chainstate
1476-
if (!ReplayBlocks(chainparams, pcoinsdbview)) {
1470+
if (!ReplayBlocks(chainparams, pcoinsdbview.get())) {
14771471
strLoadError = _("Unable to replay blocks. You will need to rebuild the database using -reindex-chainstate.");
14781472
break;
14791473
}
14801474

14811475
// The on-disk coinsdb is now in a good state, create the cache
1482-
pcoinsTip = new CCoinsViewCache(pcoinscatcher);
1476+
pcoinsTip.reset(new CCoinsViewCache(pcoinscatcher.get()));
14831477

14841478
bool is_coinsview_empty = fReset || fReindexChainState || pcoinsTip->GetBestBlock().IsNull();
14851479
if (!is_coinsview_empty) {
@@ -1521,7 +1515,7 @@ bool AppInitMain(boost::thread_group& threadGroup, CScheduler& scheduler)
15211515
}
15221516
}
15231517

1524-
if (!CVerifyDB().VerifyDB(chainparams, pcoinsdbview, gArgs.GetArg("-checklevel", DEFAULT_CHECKLEVEL),
1518+
if (!CVerifyDB().VerifyDB(chainparams, pcoinsdbview.get(), gArgs.GetArg("-checklevel", DEFAULT_CHECKLEVEL),
15251519
gArgs.GetArg("-checkblocks", DEFAULT_CHECKBLOCKS))) {
15261520
strLoadError = _("Corrupted block database detected");
15271521
break;

‎src/net.cpp

Lines changed: 8 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1534,22 +1534,20 @@ void ThreadMapPort()
15341534

15351535
void MapPort(bool fUseUPnP)
15361536
{
1537-
static boost::thread* upnp_thread = nullptr;
1537+
static std::unique_ptr<boost::thread> upnp_thread;
15381538

15391539
if (fUseUPnP)
15401540
{
15411541
if (upnp_thread) {
15421542
upnp_thread->interrupt();
15431543
upnp_thread->join();
1544-
delete upnp_thread;
15451544
}
1546-
upnp_thread = new boost::thread(boost::bind(&TraceThread<void (*)()>, "upnp", &ThreadMapPort));
1545+
upnp_thread.reset(new boost::thread(boost::bind(&TraceThread<void (*)()>, "upnp", &ThreadMapPort)));
15471546
}
15481547
else if (upnp_thread) {
15491548
upnp_thread->interrupt();
15501549
upnp_thread->join();
1551-
delete upnp_thread;
1552-
upnp_thread = nullptr;
1550+
upnp_thread.reset();
15531551
}
15541552
}
15551553

@@ -2224,8 +2222,6 @@ CConnman::CConnman(uint64_t nSeed0In, uint64_t nSeed1In) : nSeed0(nSeed0In), nSe
22242222
nLastNodeId = 0;
22252223
nSendBufferMaxSize = 0;
22262224
nReceiveFloodSize = 0;
2227-
semOutbound = nullptr;
2228-
semAddnode = nullptr;
22292225
flagInterruptMsgProc = false;
22302226
SetTryNewOutboundPeer(false);
22312227

@@ -2331,11 +2327,11 @@ bool CConnman::Start(CScheduler& scheduler, const Options& connOptions)
23312327

23322328
if (semOutbound == nullptr) {
23332329
// initialize semaphore
2334-
semOutbound = new CSemaphore(std::min((nMaxOutbound + nMaxFeeler), nMaxConnections));
2330+
semOutbound = MakeUnique<CSemaphore>(std::min((nMaxOutbound + nMaxFeeler), nMaxConnections));
23352331
}
23362332
if (semAddnode == nullptr) {
23372333
// initialize semaphore
2338-
semAddnode = new CSemaphore(nMaxAddnode);
2334+
semAddnode = MakeUnique<CSemaphore>(nMaxAddnode);
23392335
}
23402336

23412337
//
@@ -2458,10 +2454,8 @@ void CConnman::Stop()
24582454
vNodes.clear();
24592455
vNodesDisconnected.clear();
24602456
vhListenSocket.clear();
2461-
delete semOutbound;
2462-
semOutbound = nullptr;
2463-
delete semAddnode;
2464-
semAddnode = nullptr;
2457+
semOutbound.reset();
2458+
semAddnode.reset();
24652459
}
24662460

24672461
void CConnman::DeleteNode(CNode* pnode)
@@ -2747,7 +2741,7 @@ CNode::CNode(NodeId idIn, ServiceFlags nLocalServicesIn, int nMyStartingHeightIn
27472741
nNextInvSend = 0;
27482742
fRelayTxes = false;
27492743
fSentAddr = false;
2750-
pfilter = new CBloomFilter();
2744+
pfilter = MakeUnique<CBloomFilter>();
27512745
timeLastMempoolReq = 0;
27522746
nLastBlockTime = 0;
27532747
nLastTXTime = 0;
@@ -2777,8 +2771,6 @@ CNode::CNode(NodeId idIn, ServiceFlags nLocalServicesIn, int nMyStartingHeightIn
27772771
CNode::~CNode()
27782772
{
27792773
CloseSocket(hSocket);
2780-
2781-
delete pfilter;
27822774
}
27832775

27842776
void CNode::AskFor(const CInv& inv)

‎src/net.h

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -399,8 +399,8 @@ class CConnman
399399
/** Services this instance offers */
400400
ServiceFlags nLocalServices;
401401

402-
CSemaphore *semOutbound;
403-
CSemaphore *semAddnode;
402+
std::unique_ptr<CSemaphore> semOutbound;
403+
std::unique_ptr<CSemaphore> semAddnode;
404404
int nMaxConnections;
405405
int nMaxOutbound;
406406
int nMaxAddnode;
@@ -648,7 +648,7 @@ class CNode
648648
bool fSentAddr;
649649
CSemaphoreGrant grantOutbound;
650650
CCriticalSection cs_filter;
651-
CBloomFilter* pfilter;
651+
std::unique_ptr<CBloomFilter> pfilter;
652652
std::atomic<int> nRefCount;
653653

654654
const uint64_t nKeyedNetGroup;

‎src/net_processing.cpp

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -2102,7 +2102,7 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
21022102

21032103
if (!AlreadyHave(inv) &&
21042104
AcceptToMemoryPool(mempool, state, ptx, &fMissingInputs, &lRemovedTxn, false /* bypass_limits */, 0 /* nAbsurdFee */)) {
2105-
mempool.check(pcoinsTip);
2105+
mempool.check(pcoinsTip.get());
21062106
RelayTransaction(tx, connman);
21072107
for (unsigned int i = 0; i < tx.vout.size(); i++) {
21082108
vWorkQueue.emplace_back(inv.hash, i);
@@ -2169,7 +2169,7 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
21692169
recentRejects->insert(orphanHash);
21702170
}
21712171
}
2172-
mempool.check(pcoinsTip);
2172+
mempool.check(pcoinsTip.get());
21732173
}
21742174
}
21752175

@@ -2751,8 +2751,7 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
27512751
else
27522752
{
27532753
LOCK(pfrom->cs_filter);
2754-
delete pfrom->pfilter;
2755-
pfrom->pfilter = new CBloomFilter(filter);
2754+
pfrom->pfilter.reset(new CBloomFilter(filter));
27562755
pfrom->pfilter->UpdateEmptyFull();
27572756
pfrom->fRelayTxes = true;
27582757
}
@@ -2788,8 +2787,7 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
27882787
{
27892788
LOCK(pfrom->cs_filter);
27902789
if (pfrom->GetLocalServices() & NODE_BLOOM) {
2791-
delete pfrom->pfilter;
2792-
pfrom->pfilter = new CBloomFilter();
2790+
pfrom->pfilter.reset(new CBloomFilter());
27932791
}
27942792
pfrom->fRelayTxes = true;
27952793
}

‎src/policy/fees.cpp

Lines changed: 9 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -548,16 +548,13 @@ CBlockPolicyEstimator::CBlockPolicyEstimator()
548548
bucketMap[INF_FEERATE] = bucketIndex;
549549
assert(bucketMap.size() == buckets.size());
550550

551-
feeStats = new TxConfirmStats(buckets, bucketMap, MED_BLOCK_PERIODS, MED_DECAY, MED_SCALE);
552-
shortStats = new TxConfirmStats(buckets, bucketMap, SHORT_BLOCK_PERIODS, SHORT_DECAY, SHORT_SCALE);
553-
longStats = new TxConfirmStats(buckets, bucketMap, LONG_BLOCK_PERIODS, LONG_DECAY, LONG_SCALE);
551+
feeStats = std::unique_ptr<TxConfirmStats>(new TxConfirmStats(buckets, bucketMap, MED_BLOCK_PERIODS, MED_DECAY, MED_SCALE));
552+
shortStats = std::unique_ptr<TxConfirmStats>(new TxConfirmStats(buckets, bucketMap, SHORT_BLOCK_PERIODS, SHORT_DECAY, SHORT_SCALE));
553+
longStats = std::unique_ptr<TxConfirmStats>(new TxConfirmStats(buckets, bucketMap, LONG_BLOCK_PERIODS, LONG_DECAY, LONG_SCALE));
554554
}
555555

556556
CBlockPolicyEstimator::~CBlockPolicyEstimator()
557557
{
558-
delete feeStats;
559-
delete shortStats;
560-
delete longStats;
561558
}
562559

563560
void CBlockPolicyEstimator::processTransaction(const CTxMemPoolEntry& entry, bool validFeeEstimate)
@@ -690,16 +687,16 @@ CFeeRate CBlockPolicyEstimator::estimateRawFee(int confTarget, double successThr
690687
double sufficientTxs = SUFFICIENT_FEETXS;
691688
switch (horizon) {
692689
case FeeEstimateHorizon::SHORT_HALFLIFE: {
693-
stats = shortStats;
690+
stats = shortStats.get();
694691
sufficientTxs = SUFFICIENT_TXS_SHORT;
695692
break;
696693
}
697694
case FeeEstimateHorizon::MED_HALFLIFE: {
698-
stats = feeStats;
695+
stats = feeStats.get();
699696
break;
700697
}
701698
case FeeEstimateHorizon::LONG_HALFLIFE: {
702-
stats = longStats;
699+
stats = longStats.get();
703700
break;
704701
}
705702
default: {
@@ -1002,12 +999,9 @@ bool CBlockPolicyEstimator::Read(CAutoFile& filein)
1002999
}
10031000

10041001
// Destroy old TxConfirmStats and point to new ones that already reference buckets and bucketMap
1005-
delete feeStats;
1006-
delete shortStats;
1007-
delete longStats;
1008-
feeStats = fileFeeStats.release();
1009-
shortStats = fileShortStats.release();
1010-
longStats = fileLongStats.release();
1002+
feeStats = std::move(fileFeeStats);
1003+
shortStats = std::move(fileShortStats);
1004+
longStats = std::move(fileLongStats);
10111005

10121006
nBestSeenHeight = nFileBestSeenHeight;
10131007
historicalFirst = nFileHistoricalFirst;

‎src/policy/fees.h

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -245,9 +245,9 @@ class CBlockPolicyEstimator
245245
std::map<uint256, TxStatsInfo> mapMemPoolTxs;
246246

247247
/** Classes to track historical data on transaction confirmations */
248-
TxConfirmStats* feeStats;
249-
TxConfirmStats* shortStats;
250-
TxConfirmStats* longStats;
248+
std::unique_ptr<TxConfirmStats> feeStats;
249+
std::unique_ptr<TxConfirmStats> shortStats;
250+
std::unique_ptr<TxConfirmStats> longStats;
251251

252252
unsigned int trackedTxs;
253253
unsigned int untrackedTxs;

‎src/qt/test/rpcnestedtests.h

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -17,9 +17,6 @@ class RPCNestedTests : public QObject
1717

1818
private Q_SLOTS:
1919
void rpcNestedTests();
20-
21-
private:
22-
CCoinsViewDB *pcoinsdbview;
2320
};
2421

2522
#endif // BITCOIN_QT_TEST_RPC_NESTED_TESTS_H

‎src/rpc/blockchain.cpp

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -928,7 +928,7 @@ UniValue gettxoutsetinfo(const JSONRPCRequest& request)
928928

929929
CCoinsStats stats;
930930
FlushStateToDisk();
931-
if (GetUTXOStats(pcoinsdbview, stats)) {
931+
if (GetUTXOStats(pcoinsdbview.get(), stats)) {
932932
ret.push_back(Pair("height", (int64_t)stats.nHeight));
933933
ret.push_back(Pair("bestblock", stats.hashBlock.GetHex()));
934934
ret.push_back(Pair("transactions", (int64_t)stats.nTransactions));
@@ -996,7 +996,7 @@ UniValue gettxout(const JSONRPCRequest& request)
996996
Coin coin;
997997
if (fMempool) {
998998
LOCK(mempool.cs);
999-
CCoinsViewMemPool view(pcoinsTip, mempool);
999+
CCoinsViewMemPool view(pcoinsTip.get(), mempool);
10001000
if (!view.GetCoin(out, coin) || mempool.isSpent(out)) {
10011001
return NullUniValue;
10021002
}
@@ -1048,7 +1048,7 @@ UniValue verifychain(const JSONRPCRequest& request)
10481048
if (!request.params[1].isNull())
10491049
nCheckDepth = request.params[1].get_int();
10501050

1051-
return CVerifyDB().VerifyDB(Params(), pcoinsTip, nCheckLevel, nCheckDepth);
1051+
return CVerifyDB().VerifyDB(Params(), pcoinsTip.get(), nCheckLevel, nCheckDepth);
10521052
}
10531053

10541054
/** Implementation of IsSuperMajority with better feedback */

‎src/test/dbwrapper_tests.cpp

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -125,7 +125,7 @@ BOOST_AUTO_TEST_CASE(existing_data_no_obfuscate)
125125
create_directories(ph);
126126

127127
// Set up a non-obfuscated wrapper to write some initial data.
128-
CDBWrapper* dbw = new CDBWrapper(ph, (1 << 10), false, false, false);
128+
std::unique_ptr<CDBWrapper> dbw = MakeUnique<CDBWrapper>(ph, (1 << 10), false, false, false);
129129
char key = 'k';
130130
uint256 in = InsecureRand256();
131131
uint256 res;
@@ -135,8 +135,7 @@ BOOST_AUTO_TEST_CASE(existing_data_no_obfuscate)
135135
BOOST_CHECK_EQUAL(res.ToString(), in.ToString());
136136

137137
// Call the destructor to free leveldb LOCK
138-
delete dbw;
139-
dbw = nullptr;
138+
dbw.reset();
140139

141140
// Now, set up another wrapper that wants to obfuscate the same directory
142141
CDBWrapper odbw(ph, (1 << 10), false, false, true);
@@ -167,7 +166,7 @@ BOOST_AUTO_TEST_CASE(existing_data_reindex)
167166
create_directories(ph);
168167

169168
// Set up a non-obfuscated wrapper to write some initial data.
170-
CDBWrapper* dbw = new CDBWrapper(ph, (1 << 10), false, false, false);
169+
std::unique_ptr<CDBWrapper> dbw = MakeUnique<CDBWrapper>(ph, (1 << 10), false, false, false);
171170
char key = 'k';
172171
uint256 in = InsecureRand256();
173172
uint256 res;
@@ -177,8 +176,7 @@ BOOST_AUTO_TEST_CASE(existing_data_reindex)
177176
BOOST_CHECK_EQUAL(res.ToString(), in.ToString());
178177

179178
// Call the destructor to free leveldb LOCK
180-
delete dbw;
181-
dbw = nullptr;
179+
dbw.reset();
182180

183181
// Simulate a -reindex by wiping the existing data store
184182
CDBWrapper odbw(ph, (1 << 10), false, true, true);

0 commit comments

Comments
 (0)