Skip to content

Commit ef2fb7b

Browse files
kwvgkwvg
committed
evo: drop std::reference_wrapper usage, make copies instead
With extended addresses, the risk of dangling references is present as Clear()'ing it while another thread is iterating through GetEntries() could result in undefined behavior. Let's avoid that by making copies, the data structures aren't expensive enough to risk safety.
1 parent 50cdc84 commit ef2fb7b

File tree

7 files changed

+52
-60
lines changed

7 files changed

+52
-60
lines changed

‎src/evo/deterministicmns.cpp‎

Lines changed: 24 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -432,13 +432,12 @@ void CDeterministicMNList::AddMN(const CDeterministicMNCPtr& dmn, bool fBumpTota
432432
throw(std::runtime_error(strprintf("%s: Can't add a masternode %s with a duplicate collateralOutpoint=%s", __func__,
433433
dmn->proTxHash.ToString(), dmn->collateralOutpoint.ToStringShort())));
434434
}
435-
for (const NetInfoEntry& entry : dmn->pdmnState->netInfo->GetEntries()) {
436-
if (const auto& service_opt{entry.GetAddrPort()}; service_opt.has_value()) {
437-
const CService& service{service_opt.value()};
438-
if (!AddUniqueProperty(*dmn, service)) {
435+
for (const auto& entry : dmn->pdmnState->netInfo->GetEntries()) {
436+
if (const auto service_opt{entry.GetAddrPort()}) {
437+
if (!AddUniqueProperty(*dmn, *service_opt)) {
439438
mnUniquePropertyMap = mnUniquePropertyMapSaved;
440-
throw std::runtime_error(strprintf("%s: Can't add a masternode %s with a duplicate address=%s",
441-
__func__, dmn->proTxHash.ToString(), service.ToStringAddrPort()));
439+
throw std::runtime_error(strprintf("%s: Can't add a masternode %s with a duplicate address=%s", __func__,
440+
dmn->proTxHash.ToString(), service_opt->ToStringAddrPort()));
442441
}
443442
} else {
444443
mnUniquePropertyMap = mnUniquePropertyMapSaved;
@@ -489,21 +488,19 @@ void CDeterministicMNList::UpdateMN(const CDeterministicMN& oldDmn, const std::s
489488
// We track each individual entry in netInfo as opposed to netInfo itself (preventing us from
490489
// using UpdateUniqueProperty()), so we need to successfully purge all old entries and insert
491490
// new entries to successfully update.
492-
for (const NetInfoEntry& old_entry : oldInfo->GetEntries()) {
493-
if (const auto& service_opt{old_entry.GetAddrPort()}) {
494-
const CService& service{service_opt.value()};
495-
if (!DeleteUniqueProperty(dmn, service)) {
491+
for (const auto& old_entry : oldInfo->GetEntries()) {
492+
if (const auto service_opt{old_entry.GetAddrPort()}) {
493+
if (!DeleteUniqueProperty(dmn, *service_opt)) {
496494
return "internal error"; // This shouldn't be possible
497495
}
498496
} else {
499497
return "invalid address";
500498
}
501499
}
502-
for (const NetInfoEntry& new_entry : newInfo->GetEntries()) {
503-
if (const auto& service_opt{new_entry.GetAddrPort()}) {
504-
const CService& service{service_opt.value()};
505-
if (!AddUniqueProperty(dmn, service)) {
506-
return strprintf("duplicate (%s)", service.ToStringAddrPort());
500+
for (const auto& new_entry : newInfo->GetEntries()) {
501+
if (const auto service_opt{new_entry.GetAddrPort()}) {
502+
if (!AddUniqueProperty(dmn, *service_opt)) {
503+
return strprintf("duplicate (%s)", service_opt->ToStringAddrPort());
507504
}
508505
} else {
509506
return "invalid address";
@@ -578,13 +575,12 @@ void CDeterministicMNList::RemoveMN(const uint256& proTxHash)
578575
throw(std::runtime_error(strprintf("%s: Can't delete a masternode %s with a collateralOutpoint=%s", __func__,
579576
proTxHash.ToString(), dmn->collateralOutpoint.ToStringShort())));
580577
}
581-
for (const NetInfoEntry& entry : dmn->pdmnState->netInfo->GetEntries()) {
582-
if (const auto& service_opt{entry.GetAddrPort()}; service_opt.has_value()) {
583-
const CService& service{service_opt.value()};
584-
if (!DeleteUniqueProperty(*dmn, service)) {
578+
for (const auto& entry : dmn->pdmnState->netInfo->GetEntries()) {
579+
if (const auto service_opt{entry.GetAddrPort()}) {
580+
if (!DeleteUniqueProperty(*dmn, *service_opt)) {
585581
mnUniquePropertyMap = mnUniquePropertyMapSaved;
586582
throw std::runtime_error(strprintf("%s: Can't delete a masternode %s with an address=%s", __func__,
587-
proTxHash.ToString(), service.ToStringAddrPort()));
583+
proTxHash.ToString(), service_opt->ToStringAddrPort()));
588584
}
589585
} else {
590586
mnUniquePropertyMap = mnUniquePropertyMapSaved;
@@ -1119,11 +1115,10 @@ bool CheckProRegTx(CDeterministicMNManager& dmnman, const CTransaction& tx, gsl:
11191115
auto mnList = dmnman.GetListForBlock(pindexPrev);
11201116

11211117
// only allow reusing of addresses when it's for the same collateral (which replaces the old MN)
1122-
for (const NetInfoEntry& entry : opt_ptx->netInfo->GetEntries()) {
1123-
if (const auto& service_opt{entry.GetAddrPort()}; service_opt.has_value()) {
1124-
const CService& service{service_opt.value()};
1125-
if (mnList.HasUniqueProperty(service) &&
1126-
mnList.GetUniquePropertyMN(service)->collateralOutpoint != collateralOutpoint) {
1118+
for (const auto& entry : opt_ptx->netInfo->GetEntries()) {
1119+
if (const auto service_opt{entry.GetAddrPort()}) {
1120+
if (mnList.HasUniqueProperty(*service_opt) &&
1121+
mnList.GetUniquePropertyMN(*service_opt)->collateralOutpoint != collateralOutpoint) {
11271122
return state.Invalid(TxValidationResult::TX_BAD_SPECIAL, "bad-protx-dup-netinfo-entry");
11281123
}
11291124
} else {
@@ -1202,10 +1197,10 @@ bool CheckProUpServTx(CDeterministicMNManager& dmnman, const CTransaction& tx, g
12021197
}
12031198

12041199
// don't allow updating to addresses already used by other MNs
1205-
for (const NetInfoEntry& entry : opt_ptx->netInfo->GetEntries()) {
1206-
if (const auto& service_opt{entry.GetAddrPort()}; service_opt.has_value()) {
1207-
const CService& service{service_opt.value()};
1208-
if (mnList.HasUniqueProperty(service) && mnList.GetUniquePropertyMN(service)->proTxHash != opt_ptx->proTxHash) {
1200+
for (const auto& entry : opt_ptx->netInfo->GetEntries()) {
1201+
if (const auto service_opt{entry.GetAddrPort()}) {
1202+
if (mnList.HasUniqueProperty(*service_opt) &&
1203+
mnList.GetUniquePropertyMN(*service_opt)->proTxHash != opt_ptx->proTxHash) {
12091204
return state.Invalid(TxValidationResult::TX_BAD_SPECIAL, "bad-protx-dup-netinfo-entry");
12101205
}
12111206
} else {

‎src/evo/netinfo.cpp‎

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,6 @@
1616
namespace {
1717
static std::unique_ptr<const CChainParams> g_main_params{nullptr};
1818
static std::once_flag g_main_params_flag;
19-
static const CService empty_service{};
2019

2120
static constexpr std::string_view SAFE_CHARS_IPV4{"1234567890."};
2221

@@ -78,7 +77,7 @@ bool NetInfoEntry::operator<(const NetInfoEntry& rhs) const
7877
m_data, rhs.m_data);
7978
}
8079

81-
std::optional<std::reference_wrapper<const CService>> NetInfoEntry::GetAddrPort() const
80+
std::optional<CService> NetInfoEntry::GetAddrPort() const
8281
{
8382
if (const auto* data_ptr{std::get_if<CService>(&m_data)}; m_type == NetInfoType::Service && data_ptr) {
8483
ASSERT_IF_DEBUG(data_ptr->IsValid());
@@ -226,12 +225,12 @@ NetInfoList MnNetInfo::GetEntries() const
226225
return {};
227226
}
228227

229-
const CService& MnNetInfo::GetPrimary() const
228+
CService MnNetInfo::GetPrimary() const
230229
{
231-
if (const auto& service_opt{m_addr.GetAddrPort()}) {
230+
if (const auto service_opt{m_addr.GetAddrPort()}) {
232231
return *service_opt;
233232
}
234-
return empty_service;
233+
return CService{};
235234
}
236235

237236
NetInfoStatus MnNetInfo::Validate() const

‎src/evo/netinfo.h‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -118,7 +118,7 @@ class NetInfoEntry
118118
m_data = std::monostate{};
119119
}
120120

121-
std::optional<std::reference_wrapper<const CService>> GetAddrPort() const;
121+
std::optional<CService> GetAddrPort() const;
122122
uint16_t GetPort() const;
123123
bool IsEmpty() const { return *this == NetInfoEntry{}; }
124124
bool IsTriviallyValid() const;
@@ -129,7 +129,7 @@ class NetInfoEntry
129129

130130
template<> struct is_serializable_enum<NetInfoEntry::NetInfoType> : std::true_type {};
131131

132-
using NetInfoList = std::vector<std::reference_wrapper<const NetInfoEntry>>;
132+
using NetInfoList = std::vector<NetInfoEntry>;
133133

134134
class NetInfoInterface
135135
{
@@ -142,7 +142,7 @@ class NetInfoInterface
142142
virtual NetInfoStatus AddEntry(const std::string& service) = 0;
143143
virtual NetInfoList GetEntries() const = 0;
144144

145-
virtual const CService& GetPrimary() const = 0;
145+
virtual CService GetPrimary() const = 0;
146146
virtual bool CanStorePlatform() const = 0;
147147
virtual bool IsEmpty() const = 0;
148148
virtual NetInfoStatus Validate() const = 0;
@@ -176,8 +176,8 @@ class MnNetInfo final : public NetInfoInterface
176176
template <typename Stream>
177177
void Serialize(Stream& s) const
178178
{
179-
if (const auto& service{m_addr.GetAddrPort()}; service.has_value()) {
180-
s << service->get();
179+
if (const auto service_opt{m_addr.GetAddrPort()}) {
180+
s << *service_opt;
181181
} else {
182182
s << CService{};
183183
}
@@ -199,7 +199,7 @@ class MnNetInfo final : public NetInfoInterface
199199
NetInfoStatus AddEntry(const std::string& service) override;
200200
NetInfoList GetEntries() const override;
201201

202-
const CService& GetPrimary() const override;
202+
CService GetPrimary() const override;
203203
bool IsEmpty() const override { return m_addr.IsEmpty(); }
204204
bool CanStorePlatform() const override { return false; }
205205
NetInfoStatus Validate() const override;

‎src/evo/providertx.cpp‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,7 @@ bool CProRegTx::IsTriviallyValid(gsl::not_null<const CBlockIndex*> pindexPrev, T
5959
if (netInfo->CanStorePlatform() != (nVersion == ProTxVersion::ExtAddr)) {
6060
return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-protx-netinfo-version");
6161
}
62-
for (const NetInfoEntry& entry : netInfo->GetEntries()) {
62+
for (const auto& entry : netInfo->GetEntries()) {
6363
if (!entry.IsTriviallyValid()) {
6464
return state.Invalid(TxValidationResult::TX_BAD_SPECIAL, "bad-protx-netinfo-bad");
6565
}
@@ -139,7 +139,7 @@ bool CProUpServTx::IsTriviallyValid(gsl::not_null<const CBlockIndex*> pindexPrev
139139
if (netInfo->IsEmpty()) {
140140
return state.Invalid(TxValidationResult::TX_BAD_SPECIAL, "bad-protx-netinfo-empty");
141141
}
142-
for (const NetInfoEntry& entry : netInfo->GetEntries()) {
142+
for (const auto& entry : netInfo->GetEntries()) {
143143
if (!entry.IsTriviallyValid()) {
144144
return state.Invalid(TxValidationResult::TX_BAD_SPECIAL, "bad-protx-netinfo-bad");
145145
}

‎src/evo/specialtxman.cpp‎

Lines changed: 7 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -257,10 +257,9 @@ bool CSpecialTxProcessor::BuildNewListFromBlock(const CBlock& block, gsl::not_nu
257257
}
258258
}
259259

260-
for (const NetInfoEntry& entry : proTx.netInfo->GetEntries()) {
261-
if (const auto& service_opt{entry.GetAddrPort()}; service_opt.has_value()) {
262-
const CService& service{service_opt.value()};
263-
if (newList.HasUniqueProperty(service)) {
260+
for (const auto& entry : proTx.netInfo->GetEntries()) {
261+
if (const auto service_opt{entry.GetAddrPort()}) {
262+
if (newList.HasUniqueProperty(*service_opt)) {
264263
return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-protx-dup-netinfo-entry");
265264
}
266265
} else {
@@ -293,11 +292,10 @@ bool CSpecialTxProcessor::BuildNewListFromBlock(const CBlock& block, gsl::not_nu
293292
return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-protx-payload");
294293
}
295294

296-
for (const NetInfoEntry& entry : opt_proTx->netInfo->GetEntries()) {
297-
if (const auto& service_opt{entry.GetAddrPort()}; service_opt.has_value()) {
298-
const CService& service{service_opt.value()};
299-
if (newList.HasUniqueProperty(service) &&
300-
newList.GetUniquePropertyMN(service)->proTxHash != opt_proTx->proTxHash) {
295+
for (const auto& entry : opt_proTx->netInfo->GetEntries()) {
296+
if (const auto service_opt{entry.GetAddrPort()}) {
297+
if (newList.HasUniqueProperty(*service_opt) &&
298+
newList.GetUniquePropertyMN(*service_opt)->proTxHash != opt_proTx->proTxHash) {
301299
return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-protx-dup-netinfo-entry");
302300
}
303301
} else {

‎src/rpc/masternode.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -566,7 +566,7 @@ static RPCHelpMan masternodelist_helper(bool is_composite)
566566
std::string strAddress{};
567567
if (strMode == "addr" || strMode == "full" || strMode == "info" || strMode == "json" || strMode == "recent" ||
568568
strMode == "evo") {
569-
for (const NetInfoEntry& entry : dmn.pdmnState->netInfo->GetEntries()) {
569+
for (const auto& entry : dmn.pdmnState->netInfo->GetEntries()) {
570570
strAddress += entry.ToStringAddrPort() + " ";
571571
}
572572
if (!strAddress.empty()) strAddress.pop_back(); // Remove trailing space

‎src/txmempool.cpp‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -651,7 +651,7 @@ void CTxMemPool::addUncheckedProTx(indexed_transaction_set::iterator& newit, con
651651
if (!proTx.collateralOutpoint.hash.IsNull()) {
652652
mapProTxRefs.emplace(tx_hash, proTx.collateralOutpoint.hash);
653653
}
654-
for (const NetInfoEntry& entry : proTx.netInfo->GetEntries()) {
654+
for (const auto& entry : proTx.netInfo->GetEntries()) {
655655
mapProTxAddresses.emplace(entry, tx_hash);
656656
}
657657
mapProTxPubKeyIDs.emplace(proTx.keyIDOwner, tx_hash);
@@ -664,7 +664,7 @@ void CTxMemPool::addUncheckedProTx(indexed_transaction_set::iterator& newit, con
664664
} else if (tx.nType == TRANSACTION_PROVIDER_UPDATE_SERVICE) {
665665
auto proTx = *Assert(GetTxPayload<CProUpServTx>(tx));
666666
mapProTxRefs.emplace(proTx.proTxHash, tx_hash);
667-
for (const NetInfoEntry& entry : proTx.netInfo->GetEntries()) {
667+
for (const auto& entry : proTx.netInfo->GetEntries()) {
668668
mapProTxAddresses.emplace(entry, tx_hash);
669669
}
670670
} else if (tx.nType == TRANSACTION_PROVIDER_UPDATE_REGISTRAR) {
@@ -755,7 +755,7 @@ void CTxMemPool::removeUncheckedProTx(const CTransaction& tx)
755755
if (!proTx.collateralOutpoint.IsNull()) {
756756
eraseProTxRef(tx_hash, proTx.collateralOutpoint.hash);
757757
}
758-
for (const NetInfoEntry& entry : proTx.netInfo->GetEntries()) {
758+
for (const auto& entry : proTx.netInfo->GetEntries()) {
759759
mapProTxAddresses.erase(entry);
760760
}
761761
mapProTxPubKeyIDs.erase(proTx.keyIDOwner);
@@ -765,7 +765,7 @@ void CTxMemPool::removeUncheckedProTx(const CTransaction& tx)
765765
} else if (tx.nType == TRANSACTION_PROVIDER_UPDATE_SERVICE) {
766766
auto proTx = *Assert(GetTxPayload<CProUpServTx>(tx));
767767
eraseProTxRef(proTx.proTxHash, tx_hash);
768-
for (const NetInfoEntry& entry : proTx.netInfo->GetEntries()) {
768+
for (const auto& entry : proTx.netInfo->GetEntries()) {
769769
mapProTxAddresses.erase(entry);
770770
}
771771
} else if (tx.nType == TRANSACTION_PROVIDER_UPDATE_REGISTRAR) {
@@ -994,7 +994,7 @@ void CTxMemPool::removeProTxConflicts(const CTransaction &tx)
994994
}
995995
auto& proTx = *opt_proTx;
996996

997-
for (const NetInfoEntry& entry : proTx.netInfo->GetEntries()) {
997+
for (const auto& entry : proTx.netInfo->GetEntries()) {
998998
if (mapProTxAddresses.count(entry)) {
999999
uint256 conflictHash = mapProTxAddresses[entry];
10001000
if (conflictHash != tx_hash && mapTx.count(conflictHash)) {
@@ -1016,7 +1016,7 @@ void CTxMemPool::removeProTxConflicts(const CTransaction &tx)
10161016
return;
10171017
}
10181018

1019-
for (const NetInfoEntry& entry : opt_proTx->netInfo->GetEntries()) {
1019+
for (const auto& entry : opt_proTx->netInfo->GetEntries()) {
10201020
if (mapProTxAddresses.count(entry)) {
10211021
uint256 conflictHash = mapProTxAddresses[entry];
10221022
if (conflictHash != tx_hash && mapTx.count(conflictHash)) {
@@ -1354,7 +1354,7 @@ bool CTxMemPool::existsProviderTxConflict(const CTransaction &tx) const {
13541354
return true; // i.e. can't decode payload == conflict
13551355
}
13561356
auto& proTx = *opt_proTx;
1357-
for (const NetInfoEntry& entry : proTx.netInfo->GetEntries()) {
1357+
for (const auto& entry : proTx.netInfo->GetEntries()) {
13581358
if (mapProTxAddresses.count(entry)) {
13591359
return true;
13601360
}
@@ -1379,7 +1379,7 @@ bool CTxMemPool::existsProviderTxConflict(const CTransaction &tx) const {
13791379
LogPrint(BCLog::MEMPOOL, "%s: ERROR: Invalid transaction payload, tx: %s\n", __func__, tx_hash.ToString());
13801380
return true; // i.e. can't decode payload == conflict
13811381
}
1382-
for (const NetInfoEntry& entry : opt_proTx->netInfo->GetEntries()) {
1382+
for (const auto& entry : opt_proTx->netInfo->GetEntries()) {
13831383
auto it = mapProTxAddresses.find(entry);
13841384
if (it != mapProTxAddresses.end() && it->second != opt_proTx->proTxHash) {
13851385
return true;

0 commit comments

Comments
 (0)