Skip to content

Commit

Permalink
Fix data races triggered by functional tests.
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@UdjinM6 @gabriel-bjg
UdjinM6 authored and gabriel-bjg committed Jul 17, 2021
1 parent 886024b commit 3778e3f
Show file tree
Hide file tree
Showing 21 changed files with 274 additions and 192 deletions.
10 changes: 5 additions & 5 deletions src/coinjoin/coinjoin-server.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ void CCoinJoinServer::ProcessMessage(CNode* pfrom, const std::string& strCommand
LogPrint(BCLog::COINJOIN, "DSACCEPT -- nDenom %d (%s) txCollateral %s", dsa.nDenom, CCoinJoin::DenominationToString(dsa.nDenom), dsa.txCollateral.ToString()); /* Continued */

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

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

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

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

if (!fUnitTest) {
//broadcast that I'm accepting entries, only if it's the first entry through
CCoinJoinQueue dsq(nSessionDenom, activeMasternodeInfo.outpoint, GetAdjustedTime(), false);
CCoinJoinQueue dsq(nSessionDenom, WITH_LOCK(activeMasternodeInfoCs, return activeMasternodeInfo.outpoint), GetAdjustedTime(), false);
LogPrint(BCLog::COINJOIN, "CCoinJoinServer::CreateNewSession -- signing and relaying new queue: %s\n", dsq.ToString());
dsq.Sign();
dsq.Relay(connman);
Expand Down
4 changes: 2 additions & 2 deletions src/coinjoin/coinjoin.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ bool CCoinJoinQueue::Sign()


uint256 hash = GetSignatureHash();
CBLSSignature sig = activeMasternodeInfo.blsKeyOperator->Sign(hash);
CBLSSignature sig = WITH_LOCK(activeMasternodeInfoCs, return activeMasternodeInfo.blsKeyOperator->Sign(hash));
if (!sig.IsValid()) {
return false;
}
Expand Down Expand Up @@ -96,7 +96,7 @@ bool CCoinJoinBroadcastTx::Sign()

uint256 hash = GetSignatureHash();

CBLSSignature sig = activeMasternodeInfo.blsKeyOperator->Sign(hash);
CBLSSignature sig = WITH_LOCK(activeMasternodeInfoCs, return activeMasternodeInfo.blsKeyOperator->Sign(hash));
if (!sig.IsValid()) {
return false;
}
Expand Down
93 changes: 41 additions & 52 deletions src/evo/mnauth.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,31 +18,30 @@

void CMNAuth::PushMNAUTH(CNode* pnode, CConnman& connman)
{
LOCK(activeMasternodeInfoCs);
if (!fMasternodeMode || activeMasternodeInfo.proTxHash.IsNull()) {
return;
}

uint256 signHash;
{
LOCK(pnode->cs_mnauth);
if (pnode->receivedMNAuthChallenge.IsNull()) {
return;
}
// We include fInbound in signHash to forbid interchanging of challenges by a man in the middle (MITM). This way
// we protect ourselves against MITM in this form:
// node1 <- Eve -> node2
// It does not protect against:
// node1 -> Eve -> node2
// This is ok as we only use MNAUTH as a DoS protection and not for sensitive stuff
int nOurNodeVersion{PROTOCOL_VERSION};
if (Params().NetworkIDString() != CBaseChainParams::MAIN && gArgs.IsArgSet("-pushversion")) {
nOurNodeVersion = gArgs.GetArg("-pushversion", PROTOCOL_VERSION);
}
if (pnode->nVersion < MNAUTH_NODE_VER_VERSION || nOurNodeVersion < MNAUTH_NODE_VER_VERSION) {
signHash = ::SerializeHash(std::make_tuple(*activeMasternodeInfo.blsPubKeyOperator, pnode->receivedMNAuthChallenge, pnode->fInbound));
} else {
signHash = ::SerializeHash(std::make_tuple(*activeMasternodeInfo.blsPubKeyOperator, pnode->receivedMNAuthChallenge, pnode->fInbound, nOurNodeVersion));
}
auto receivedMNAuthChallenge = pnode->GetReceivedMNAuthChallenge();
if (receivedMNAuthChallenge.IsNull()) {
return;
}
// We include fInbound in signHash to forbid interchanging of challenges by a man in the middle (MITM). This way
// we protect ourselves against MITM in this form:
// node1 <- Eve -> node2
// It does not protect against:
// node1 -> Eve -> node2
// This is ok as we only use MNAUTH as a DoS protection and not for sensitive stuff
int nOurNodeVersion{PROTOCOL_VERSION};
if (Params().NetworkIDString() != CBaseChainParams::MAIN && gArgs.IsArgSet("-pushversion")) {
nOurNodeVersion = gArgs.GetArg("-pushversion", PROTOCOL_VERSION);
}
if (pnode->nVersion < MNAUTH_NODE_VER_VERSION || nOurNodeVersion < MNAUTH_NODE_VER_VERSION) {
signHash = ::SerializeHash(std::make_tuple(*activeMasternodeInfo.blsPubKeyOperator, receivedMNAuthChallenge, pnode->fInbound));
} else {
signHash = ::SerializeHash(std::make_tuple(*activeMasternodeInfo.blsPubKeyOperator, receivedMNAuthChallenge, pnode->fInbound, nOurNodeVersion));
}

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

// only one MNAUTH allowed
bool fAlreadyHaveMNAUTH = false;
{
LOCK(pnode->cs_mnauth);
fAlreadyHaveMNAUTH = !pnode->verifiedProRegTxHash.IsNull();
}
bool fAlreadyHaveMNAUTH = !pnode->GetVerifiedProRegTxHash().IsNull();
if (fAlreadyHaveMNAUTH) {
LOCK(cs_main);
Misbehaving(pnode->GetId(), 100, "duplicate mnauth");
Expand Down Expand Up @@ -108,20 +103,17 @@ void CMNAuth::ProcessMessage(CNode* pnode, const std::string& strCommand, CDataS
}

uint256 signHash;
{
LOCK(pnode->cs_mnauth);
int nOurNodeVersion{PROTOCOL_VERSION};
if (Params().NetworkIDString() != CBaseChainParams::MAIN && gArgs.IsArgSet("-pushversion")) {
nOurNodeVersion = gArgs.GetArg("-pushversion", PROTOCOL_VERSION);
}
// See comment in PushMNAUTH (fInbound is negated here as we're on the other side of the connection)
if (pnode->nVersion < MNAUTH_NODE_VER_VERSION || nOurNodeVersion < MNAUTH_NODE_VER_VERSION) {
signHash = ::SerializeHash(std::make_tuple(dmn->pdmnState->pubKeyOperator, pnode->sentMNAuthChallenge, !pnode->fInbound));
} else {
signHash = ::SerializeHash(std::make_tuple(dmn->pdmnState->pubKeyOperator, pnode->sentMNAuthChallenge, !pnode->fInbound, pnode->nVersion.load()));
}
LogPrint(BCLog::NET_NETCONN, "CMNAuth::%s -- constructed signHash for nVersion %d, peer=%d\n", __func__, pnode->nVersion, pnode->GetId());
int nOurNodeVersion{PROTOCOL_VERSION};
if (Params().NetworkIDString() != CBaseChainParams::MAIN && gArgs.IsArgSet("-pushversion")) {
nOurNodeVersion = gArgs.GetArg("-pushversion", PROTOCOL_VERSION);
}
// See comment in PushMNAUTH (fInbound is negated here as we're on the other side of the connection)
if (pnode->nVersion < MNAUTH_NODE_VER_VERSION || nOurNodeVersion < MNAUTH_NODE_VER_VERSION) {
signHash = ::SerializeHash(std::make_tuple(dmn->pdmnState->pubKeyOperator, pnode->GetSentMNAuthChallenge(), !pnode->fInbound));
} else {
signHash = ::SerializeHash(std::make_tuple(dmn->pdmnState->pubKeyOperator, pnode->GetSentMNAuthChallenge(), !pnode->fInbound, pnode->nVersion.load()));
}
LogPrint(BCLog::NET_NETCONN, "CMNAuth::%s -- constructed signHash for nVersion %d, peer=%d\n", __func__, pnode->nVersion, pnode->GetId());

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

if (pnode2->verifiedProRegTxHash == mnauth.proRegTxHash) {
if (pnode2->GetVerifiedProRegTxHash() == mnauth.proRegTxHash) {
if (fMasternodeMode) {
auto deterministicOutbound = llmq::CLLMQUtils::DeterministicOutboundConnection(activeMasternodeInfo.proTxHash, mnauth.proRegTxHash);
auto deterministicOutbound = WITH_LOCK(activeMasternodeInfoCs, return llmq::CLLMQUtils::DeterministicOutboundConnection(activeMasternodeInfo.proTxHash, mnauth.proRegTxHash));
LogPrint(BCLog::NET_NETCONN, "CMNAuth::ProcessMessage -- Masternode %s has already verified as peer %d, deterministicOutbound=%s. peer=%d\n",
mnauth.proRegTxHash.ToString(), pnode2->GetId(), deterministicOutbound.ToString(), pnode->GetId());
if (deterministicOutbound == activeMasternodeInfo.proTxHash) {
if (WITH_LOCK(activeMasternodeInfoCs, return deterministicOutbound == activeMasternodeInfo.proTxHash)) {
if (pnode2->fInbound) {
LogPrint(BCLog::NET_NETCONN, "CMNAuth::ProcessMessage -- dropping old inbound, peer=%d\n", pnode2->GetId());
pnode2->fDisconnect = true;
Expand Down Expand Up @@ -181,13 +173,10 @@ void CMNAuth::ProcessMessage(CNode* pnode, const std::string& strCommand, CDataS
return;
}

{
LOCK(pnode->cs_mnauth);
pnode->verifiedProRegTxHash = mnauth.proRegTxHash;
pnode->verifiedPubKeyHash = dmn->pdmnState->pubKeyOperator.GetHash();
}
pnode->SetVerifiedProRegTxHash(mnauth.proRegTxHash);
pnode->SetVerifiedPubKeyHash(dmn->pdmnState->pubKeyOperator.GetHash());

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

g_connman->ForEachNode([&](CNode* pnode) {
LOCK(pnode->cs_mnauth);
if (pnode->verifiedProRegTxHash.IsNull()) {
auto verifiedProRegTxHash = pnode->GetVerifiedProRegTxHash();
if (verifiedProRegTxHash.IsNull()) {
return;
}
auto verifiedDmn = oldMNList.GetMN(pnode->verifiedProRegTxHash);
auto verifiedDmn = oldMNList.GetMN(verifiedProRegTxHash);
if (!verifiedDmn) {
return;
}
Expand All @@ -223,15 +212,15 @@ void CMNAuth::NotifyMasternodeListChanged(bool undo, const CDeterministicMNList&
} else {
auto it = diff.updatedMNs.find(verifiedDmn->GetInternalId());
if (it != diff.updatedMNs.end()) {
if ((it->second.fields & CDeterministicMNStateDiff::Field_pubKeyOperator) && it->second.state.pubKeyOperator.GetHash() != pnode->verifiedPubKeyHash) {
if ((it->second.fields & CDeterministicMNStateDiff::Field_pubKeyOperator) && it->second.state.pubKeyOperator.GetHash() != pnode->GetVerifiedPubKeyHash()) {
doRemove = true;
}
}
}

if (doRemove) {
LogPrint(BCLog::NET_NETCONN, "CMNAuth::NotifyMasternodeListChanged -- Disconnecting MN %s due to key changed/removed, peer=%d\n",
pnode->verifiedProRegTxHash.ToString(), pnode->GetId());
pnode->GetVerifiedProRegTxHash().ToString(), pnode->GetId());
pnode->fDisconnect = true;
}
});
Expand Down
30 changes: 20 additions & 10 deletions src/init.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -363,9 +363,12 @@ void PrepareShutdown()
UnregisterValidationInterface(activeMasternodeManager);
}

// make sure to clean up BLS keys before global destructors are called (they have allocated from the secure memory pool)
activeMasternodeInfo.blsKeyOperator.reset();
activeMasternodeInfo.blsPubKeyOperator.reset();
{
LOCK(activeMasternodeInfoCs);
// make sure to clean up BLS keys before global destructors are called (they have allocated from the secure memory pool)
activeMasternodeInfo.blsKeyOperator.reset();
activeMasternodeInfo.blsPubKeyOperator.reset();
}

#ifndef WIN32
try {
Expand Down Expand Up @@ -2323,8 +2326,12 @@ bool AppInitMain()
return InitError(_("Invalid masternodeblsprivkey. Please see documentation."));
}
fMasternodeMode = true;
activeMasternodeInfo.blsKeyOperator = std::make_unique<CBLSSecretKey>(keyOperator);
activeMasternodeInfo.blsPubKeyOperator = std::make_unique<CBLSPublicKey>(activeMasternodeInfo.blsKeyOperator->GetPublicKey());
{
LOCK(activeMasternodeInfoCs);
activeMasternodeInfo.blsKeyOperator = std::make_unique<CBLSSecretKey>(keyOperator);
activeMasternodeInfo.blsPubKeyOperator = std::make_unique<CBLSPublicKey>(
activeMasternodeInfo.blsKeyOperator->GetPublicKey());
}
LogPrintf("MASTERNODE:\n");
LogPrintf(" blsPubKeyOperator: %s\n", keyOperator.GetPublicKey().ToString());
}
Expand All @@ -2335,11 +2342,14 @@ bool AppInitMain()
RegisterValidationInterface(activeMasternodeManager);
}

if (activeMasternodeInfo.blsKeyOperator == nullptr) {
activeMasternodeInfo.blsKeyOperator = std::make_unique<CBLSSecretKey>();
}
if (activeMasternodeInfo.blsPubKeyOperator == nullptr) {
activeMasternodeInfo.blsPubKeyOperator = std::make_unique<CBLSPublicKey>();
{
LOCK(activeMasternodeInfoCs);
if (activeMasternodeInfo.blsKeyOperator == nullptr) {
activeMasternodeInfo.blsKeyOperator = std::make_unique<CBLSSecretKey>();
}
if (activeMasternodeInfo.blsPubKeyOperator == nullptr) {
activeMasternodeInfo.blsPubKeyOperator = std::make_unique<CBLSPublicKey>();
}
}

// ********************************************************* Step 10b: setup CoinJoin
Expand Down
Loading

0 comments on commit 3778e3f

Please sign in to comment.