Skip to content

Commit 9047608

Browse files
UdjinM6gabriel-bjg
UdjinM6
authored and
gabriel-bjg
committed
Fix data races triggered by functional tests.
Function CWallet::KeepKey requires locking as it has concurrent access to database and member nKeysLeftSinceAutoBackup. Avoid data race when reading setInventoryTxToSend size by locking the read. If locking happens after the read, the size may change. Lock cs_mnauth when reading verifiedProRegTxHash. Make fRPCRunning atomic as it can be read/written from different threads simultaneously. Make m_masternode_iqr_connection atomic as it can be read/written from different threads simultaneously. Use a recursive mutex to synchronize concurrent access to quorumVvec. Make m_masternode_connection atomic as it can be read/written from different threads simultaneously. Make m_masternode_probe_connection atomic as it can be read/written from different threads simultaneously. Use a recursive mutex in order to lock access to activeMasterNode. Use a recursive mutex to synchronize concurrent access to skShare. Guarded all mnauth fields of a CNode.
1 parent 886024b commit 9047608

21 files changed

+274
-192
lines changed

‎src/coinjoin/coinjoin-server.cpp

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,7 @@ void CCoinJoinServer::ProcessMessage(CNode* pfrom, const std::string& strCommand
5656
LogPrint(BCLog::COINJOIN, "DSACCEPT -- nDenom %d (%s) txCollateral %s", dsa.nDenom, CCoinJoin::DenominationToString(dsa.nDenom), dsa.txCollateral.ToString()); /* Continued */
5757

5858
auto mnList = deterministicMNManager->GetListAtChainTip();
59-
auto dmn = mnList.GetValidMNByCollateral(activeMasternodeInfo.outpoint);
59+
auto dmn = WITH_LOCK(activeMasternodeInfoCs, return mnList.GetValidMNByCollateral(activeMasternodeInfo.outpoint));
6060
if (!dmn) {
6161
PushStatus(pfrom, STATUS_REJECTED, ERR_MN_LIST, connman);
6262
return;
@@ -68,7 +68,7 @@ void CCoinJoinServer::ProcessMessage(CNode* pfrom, const std::string& strCommand
6868
if (!lockRecv) return;
6969

7070
for (const auto& q : vecCoinJoinQueue) {
71-
if (q.masternodeOutpoint == activeMasternodeInfo.outpoint) {
71+
if (WITH_LOCK(activeMasternodeInfoCs, return q.masternodeOutpoint == activeMasternodeInfo.outpoint)) {
7272
// refuse to create another queue this often
7373
LogPrint(BCLog::COINJOIN, "DSACCEPT -- last dsq is still in queue, refuse to mix\n");
7474
PushStatus(pfrom, STATUS_REJECTED, ERR_RECENT, connman);
@@ -334,7 +334,7 @@ void CCoinJoinServer::CommitFinalTransaction(CConnman& connman)
334334

335335
// create and sign masternode dstx transaction
336336
if (!CCoinJoin::GetDSTX(hashTx)) {
337-
CCoinJoinBroadcastTx dstxNew(finalTransaction, activeMasternodeInfo.outpoint, GetAdjustedTime());
337+
CCoinJoinBroadcastTx dstxNew(finalTransaction, WITH_LOCK(activeMasternodeInfoCs, return activeMasternodeInfo.outpoint), GetAdjustedTime());
338338
dstxNew.Sign();
339339
CCoinJoin::AddDSTX(dstxNew);
340340
}
@@ -501,7 +501,7 @@ void CCoinJoinServer::CheckForCompleteQueue(CConnman& connman)
501501
if (nState == POOL_STATE_QUEUE && IsSessionReady()) {
502502
SetState(POOL_STATE_ACCEPTING_ENTRIES);
503503

504-
CCoinJoinQueue dsq(nSessionDenom, activeMasternodeInfo.outpoint, GetAdjustedTime(), true);
504+
CCoinJoinQueue dsq(nSessionDenom, WITH_LOCK(activeMasternodeInfoCs, return activeMasternodeInfo.outpoint), GetAdjustedTime(), true);
505505
LogPrint(BCLog::COINJOIN, "CCoinJoinServer::CheckForCompleteQueue -- queue is ready, signing and relaying (%s) " /* Continued */
506506
"with %d participants\n", dsq.ToString(), vecSessionCollaterals.size());
507507
dsq.Sign();
@@ -708,7 +708,7 @@ bool CCoinJoinServer::CreateNewSession(const CCoinJoinAccept& dsa, PoolMessage&
708708

709709
if (!fUnitTest) {
710710
//broadcast that I'm accepting entries, only if it's the first entry through
711-
CCoinJoinQueue dsq(nSessionDenom, activeMasternodeInfo.outpoint, GetAdjustedTime(), false);
711+
CCoinJoinQueue dsq(nSessionDenom, WITH_LOCK(activeMasternodeInfoCs, return activeMasternodeInfo.outpoint), GetAdjustedTime(), false);
712712
LogPrint(BCLog::COINJOIN, "CCoinJoinServer::CreateNewSession -- signing and relaying new queue: %s\n", dsq.ToString());
713713
dsq.Sign();
714714
dsq.Relay(connman);

‎src/coinjoin/coinjoin.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,7 @@ bool CCoinJoinQueue::Sign()
5050

5151

5252
uint256 hash = GetSignatureHash();
53-
CBLSSignature sig = activeMasternodeInfo.blsKeyOperator->Sign(hash);
53+
CBLSSignature sig = WITH_LOCK(activeMasternodeInfoCs, return activeMasternodeInfo.blsKeyOperator->Sign(hash));
5454
if (!sig.IsValid()) {
5555
return false;
5656
}
@@ -96,7 +96,7 @@ bool CCoinJoinBroadcastTx::Sign()
9696

9797
uint256 hash = GetSignatureHash();
9898

99-
CBLSSignature sig = activeMasternodeInfo.blsKeyOperator->Sign(hash);
99+
CBLSSignature sig = WITH_LOCK(activeMasternodeInfoCs, return activeMasternodeInfo.blsKeyOperator->Sign(hash));
100100
if (!sig.IsValid()) {
101101
return false;
102102
}

‎src/evo/mnauth.cpp

Lines changed: 41 additions & 52 deletions
Original file line numberDiff line numberDiff line change
@@ -18,31 +18,30 @@
1818

1919
void CMNAuth::PushMNAUTH(CNode* pnode, CConnman& connman)
2020
{
21+
LOCK(activeMasternodeInfoCs);
2122
if (!fMasternodeMode || activeMasternodeInfo.proTxHash.IsNull()) {
2223
return;
2324
}
2425

2526
uint256 signHash;
26-
{
27-
LOCK(pnode->cs_mnauth);
28-
if (pnode->receivedMNAuthChallenge.IsNull()) {
29-
return;
30-
}
31-
// We include fInbound in signHash to forbid interchanging of challenges by a man in the middle (MITM). This way
32-
// we protect ourselves against MITM in this form:
33-
// node1 <- Eve -> node2
34-
// It does not protect against:
35-
// node1 -> Eve -> node2
36-
// This is ok as we only use MNAUTH as a DoS protection and not for sensitive stuff
37-
int nOurNodeVersion{PROTOCOL_VERSION};
38-
if (Params().NetworkIDString() != CBaseChainParams::MAIN && gArgs.IsArgSet("-pushversion")) {
39-
nOurNodeVersion = gArgs.GetArg("-pushversion", PROTOCOL_VERSION);
40-
}
41-
if (pnode->nVersion < MNAUTH_NODE_VER_VERSION || nOurNodeVersion < MNAUTH_NODE_VER_VERSION) {
42-
signHash = ::SerializeHash(std::make_tuple(*activeMasternodeInfo.blsPubKeyOperator, pnode->receivedMNAuthChallenge, pnode->fInbound));
43-
} else {
44-
signHash = ::SerializeHash(std::make_tuple(*activeMasternodeInfo.blsPubKeyOperator, pnode->receivedMNAuthChallenge, pnode->fInbound, nOurNodeVersion));
45-
}
27+
auto receivedMNAuthChallenge = pnode->GetReceivedMNAuthChallenge();
28+
if (receivedMNAuthChallenge.IsNull()) {
29+
return;
30+
}
31+
// We include fInbound in signHash to forbid interchanging of challenges by a man in the middle (MITM). This way
32+
// we protect ourselves against MITM in this form:
33+
// node1 <- Eve -> node2
34+
// It does not protect against:
35+
// node1 -> Eve -> node2
36+
// This is ok as we only use MNAUTH as a DoS protection and not for sensitive stuff
37+
int nOurNodeVersion{PROTOCOL_VERSION};
38+
if (Params().NetworkIDString() != CBaseChainParams::MAIN && gArgs.IsArgSet("-pushversion")) {
39+
nOurNodeVersion = gArgs.GetArg("-pushversion", PROTOCOL_VERSION);
40+
}
41+
if (pnode->nVersion < MNAUTH_NODE_VER_VERSION || nOurNodeVersion < MNAUTH_NODE_VER_VERSION) {
42+
signHash = ::SerializeHash(std::make_tuple(*activeMasternodeInfo.blsPubKeyOperator, receivedMNAuthChallenge, pnode->fInbound));
43+
} else {
44+
signHash = ::SerializeHash(std::make_tuple(*activeMasternodeInfo.blsPubKeyOperator, receivedMNAuthChallenge, pnode->fInbound, nOurNodeVersion));
4645
}
4746

4847
CMNAuth mnauth;
@@ -66,11 +65,7 @@ void CMNAuth::ProcessMessage(CNode* pnode, const std::string& strCommand, CDataS
6665
vRecv >> mnauth;
6766

6867
// only one MNAUTH allowed
69-
bool fAlreadyHaveMNAUTH = false;
70-
{
71-
LOCK(pnode->cs_mnauth);
72-
fAlreadyHaveMNAUTH = !pnode->verifiedProRegTxHash.IsNull();
73-
}
68+
bool fAlreadyHaveMNAUTH = !pnode->GetVerifiedProRegTxHash().IsNull();
7469
if (fAlreadyHaveMNAUTH) {
7570
LOCK(cs_main);
7671
Misbehaving(pnode->GetId(), 100, "duplicate mnauth");
@@ -108,20 +103,17 @@ void CMNAuth::ProcessMessage(CNode* pnode, const std::string& strCommand, CDataS
108103
}
109104

110105
uint256 signHash;
111-
{
112-
LOCK(pnode->cs_mnauth);
113-
int nOurNodeVersion{PROTOCOL_VERSION};
114-
if (Params().NetworkIDString() != CBaseChainParams::MAIN && gArgs.IsArgSet("-pushversion")) {
115-
nOurNodeVersion = gArgs.GetArg("-pushversion", PROTOCOL_VERSION);
116-
}
117-
// See comment in PushMNAUTH (fInbound is negated here as we're on the other side of the connection)
118-
if (pnode->nVersion < MNAUTH_NODE_VER_VERSION || nOurNodeVersion < MNAUTH_NODE_VER_VERSION) {
119-
signHash = ::SerializeHash(std::make_tuple(dmn->pdmnState->pubKeyOperator, pnode->sentMNAuthChallenge, !pnode->fInbound));
120-
} else {
121-
signHash = ::SerializeHash(std::make_tuple(dmn->pdmnState->pubKeyOperator, pnode->sentMNAuthChallenge, !pnode->fInbound, pnode->nVersion.load()));
122-
}
123-
LogPrint(BCLog::NET_NETCONN, "CMNAuth::%s -- constructed signHash for nVersion %d, peer=%d\n", __func__, pnode->nVersion, pnode->GetId());
106+
int nOurNodeVersion{PROTOCOL_VERSION};
107+
if (Params().NetworkIDString() != CBaseChainParams::MAIN && gArgs.IsArgSet("-pushversion")) {
108+
nOurNodeVersion = gArgs.GetArg("-pushversion", PROTOCOL_VERSION);
124109
}
110+
// See comment in PushMNAUTH (fInbound is negated here as we're on the other side of the connection)
111+
if (pnode->nVersion < MNAUTH_NODE_VER_VERSION || nOurNodeVersion < MNAUTH_NODE_VER_VERSION) {
112+
signHash = ::SerializeHash(std::make_tuple(dmn->pdmnState->pubKeyOperator, pnode->GetSentMNAuthChallenge(), !pnode->fInbound));
113+
} else {
114+
signHash = ::SerializeHash(std::make_tuple(dmn->pdmnState->pubKeyOperator, pnode->GetSentMNAuthChallenge(), !pnode->fInbound, pnode->nVersion.load()));
115+
}
116+
LogPrint(BCLog::NET_NETCONN, "CMNAuth::%s -- constructed signHash for nVersion %d, peer=%d\n", __func__, pnode->nVersion, pnode->GetId());
125117

126118
if (!mnauth.sig.VerifyInsecure(dmn->pdmnState->pubKeyOperator.Get(), signHash)) {
127119
LOCK(cs_main);
@@ -147,12 +139,12 @@ void CMNAuth::ProcessMessage(CNode* pnode, const std::string& strCommand, CDataS
147139
return;
148140
}
149141

150-
if (pnode2->verifiedProRegTxHash == mnauth.proRegTxHash) {
142+
if (pnode2->GetVerifiedProRegTxHash() == mnauth.proRegTxHash) {
151143
if (fMasternodeMode) {
152-
auto deterministicOutbound = llmq::CLLMQUtils::DeterministicOutboundConnection(activeMasternodeInfo.proTxHash, mnauth.proRegTxHash);
144+
auto deterministicOutbound = WITH_LOCK(activeMasternodeInfoCs, return llmq::CLLMQUtils::DeterministicOutboundConnection(activeMasternodeInfo.proTxHash, mnauth.proRegTxHash));
153145
LogPrint(BCLog::NET_NETCONN, "CMNAuth::ProcessMessage -- Masternode %s has already verified as peer %d, deterministicOutbound=%s. peer=%d\n",
154146
mnauth.proRegTxHash.ToString(), pnode2->GetId(), deterministicOutbound.ToString(), pnode->GetId());
155-
if (deterministicOutbound == activeMasternodeInfo.proTxHash) {
147+
if (WITH_LOCK(activeMasternodeInfoCs, return deterministicOutbound == activeMasternodeInfo.proTxHash)) {
156148
if (pnode2->fInbound) {
157149
LogPrint(BCLog::NET_NETCONN, "CMNAuth::ProcessMessage -- dropping old inbound, peer=%d\n", pnode2->GetId());
158150
pnode2->fDisconnect = true;
@@ -181,13 +173,10 @@ void CMNAuth::ProcessMessage(CNode* pnode, const std::string& strCommand, CDataS
181173
return;
182174
}
183175

184-
{
185-
LOCK(pnode->cs_mnauth);
186-
pnode->verifiedProRegTxHash = mnauth.proRegTxHash;
187-
pnode->verifiedPubKeyHash = dmn->pdmnState->pubKeyOperator.GetHash();
188-
}
176+
pnode->SetVerifiedProRegTxHash(mnauth.proRegTxHash);
177+
pnode->SetVerifiedPubKeyHash(dmn->pdmnState->pubKeyOperator.GetHash());
189178

190-
if (!pnode->m_masternode_iqr_connection && connman.IsMasternodeQuorumRelayMember(pnode->verifiedProRegTxHash)) {
179+
if (!pnode->m_masternode_iqr_connection && connman.IsMasternodeQuorumRelayMember(pnode->GetVerifiedProRegTxHash())) {
191180
// Tell our peer that we're interested in plain LLMQ recovered signatures.
192181
// Otherwise the peer would only announce/send messages resulting from QRECSIG,
193182
// e.g. InstantSend locks or ChainLocks. SPV and regular full nodes should not send
@@ -209,11 +198,11 @@ void CMNAuth::NotifyMasternodeListChanged(bool undo, const CDeterministicMNList&
209198
}
210199

211200
g_connman->ForEachNode([&](CNode* pnode) {
212-
LOCK(pnode->cs_mnauth);
213-
if (pnode->verifiedProRegTxHash.IsNull()) {
201+
auto verifiedProRegTxHash = pnode->GetVerifiedProRegTxHash();
202+
if (verifiedProRegTxHash.IsNull()) {
214203
return;
215204
}
216-
auto verifiedDmn = oldMNList.GetMN(pnode->verifiedProRegTxHash);
205+
auto verifiedDmn = oldMNList.GetMN(verifiedProRegTxHash);
217206
if (!verifiedDmn) {
218207
return;
219208
}
@@ -223,15 +212,15 @@ void CMNAuth::NotifyMasternodeListChanged(bool undo, const CDeterministicMNList&
223212
} else {
224213
auto it = diff.updatedMNs.find(verifiedDmn->GetInternalId());
225214
if (it != diff.updatedMNs.end()) {
226-
if ((it->second.fields & CDeterministicMNStateDiff::Field_pubKeyOperator) && it->second.state.pubKeyOperator.GetHash() != pnode->verifiedPubKeyHash) {
215+
if ((it->second.fields & CDeterministicMNStateDiff::Field_pubKeyOperator) && it->second.state.pubKeyOperator.GetHash() != pnode->GetVerifiedPubKeyHash()) {
227216
doRemove = true;
228217
}
229218
}
230219
}
231220

232221
if (doRemove) {
233222
LogPrint(BCLog::NET_NETCONN, "CMNAuth::NotifyMasternodeListChanged -- Disconnecting MN %s due to key changed/removed, peer=%d\n",
234-
pnode->verifiedProRegTxHash.ToString(), pnode->GetId());
223+
pnode->GetVerifiedProRegTxHash().ToString(), pnode->GetId());
235224
pnode->fDisconnect = true;
236225
}
237226
});

‎src/init.cpp

Lines changed: 20 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -363,9 +363,12 @@ void PrepareShutdown()
363363
UnregisterValidationInterface(activeMasternodeManager);
364364
}
365365

366-
// make sure to clean up BLS keys before global destructors are called (they have allocated from the secure memory pool)
367-
activeMasternodeInfo.blsKeyOperator.reset();
368-
activeMasternodeInfo.blsPubKeyOperator.reset();
366+
{
367+
LOCK(activeMasternodeInfoCs);
368+
// make sure to clean up BLS keys before global destructors are called (they have allocated from the secure memory pool)
369+
activeMasternodeInfo.blsKeyOperator.reset();
370+
activeMasternodeInfo.blsPubKeyOperator.reset();
371+
}
369372

370373
#ifndef WIN32
371374
try {
@@ -2323,8 +2326,12 @@ bool AppInitMain()
23232326
return InitError(_("Invalid masternodeblsprivkey. Please see documentation."));
23242327
}
23252328
fMasternodeMode = true;
2326-
activeMasternodeInfo.blsKeyOperator = std::make_unique<CBLSSecretKey>(keyOperator);
2327-
activeMasternodeInfo.blsPubKeyOperator = std::make_unique<CBLSPublicKey>(activeMasternodeInfo.blsKeyOperator->GetPublicKey());
2329+
{
2330+
LOCK(activeMasternodeInfoCs);
2331+
activeMasternodeInfo.blsKeyOperator = std::make_unique<CBLSSecretKey>(keyOperator);
2332+
activeMasternodeInfo.blsPubKeyOperator = std::make_unique<CBLSPublicKey>(
2333+
activeMasternodeInfo.blsKeyOperator->GetPublicKey());
2334+
}
23282335
LogPrintf("MASTERNODE:\n");
23292336
LogPrintf(" blsPubKeyOperator: %s\n", keyOperator.GetPublicKey().ToString());
23302337
}
@@ -2335,11 +2342,14 @@ bool AppInitMain()
23352342
RegisterValidationInterface(activeMasternodeManager);
23362343
}
23372344

2338-
if (activeMasternodeInfo.blsKeyOperator == nullptr) {
2339-
activeMasternodeInfo.blsKeyOperator = std::make_unique<CBLSSecretKey>();
2340-
}
2341-
if (activeMasternodeInfo.blsPubKeyOperator == nullptr) {
2342-
activeMasternodeInfo.blsPubKeyOperator = std::make_unique<CBLSPublicKey>();
2345+
{
2346+
LOCK(activeMasternodeInfoCs);
2347+
if (activeMasternodeInfo.blsKeyOperator == nullptr) {
2348+
activeMasternodeInfo.blsKeyOperator = std::make_unique<CBLSSecretKey>();
2349+
}
2350+
if (activeMasternodeInfo.blsPubKeyOperator == nullptr) {
2351+
activeMasternodeInfo.blsPubKeyOperator = std::make_unique<CBLSPublicKey>();
2352+
}
23432353
}
23442354

23452355
// ********************************************************* Step 10b: setup CoinJoin

0 commit comments

Comments
 (0)