Skip to content

Commit a43ab1c

Browse files
kwvgkwvg
committed
fix: protect m_wallet_manager_map with cs_wallet_manager_map
Avoid TSan-reported data race ``` WARNING: ThreadSanitizer: data race (pid=374820) Read of size 8 at 0x7b140002ce10 by thread T12: #0 _M_ptr /usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/bits/unique_ptr.h:154:42 (dashd+0xb58e08) (BuildId: c3fdce9f7e778985a4fb0968ff4506d9ad24d408) dashpay#1 get /usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/bits/unique_ptr.h:361:21 (dashd+0xb58e08) dashpay#2 operator-> /usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/bits/unique_ptr.h:355:9 (dashd+0xb58e08) dashpay#3 CoinJoinWalletManager::DoMaintenance() /src/dash/src/coinjoin/client.cpp:1907:9 (dashd+0xb58e08) [...] Previous write of size 8 at 0x7b140002ce10 by thread T17 (mutexes: write M0): #0 operator new(unsigned long) <null> (dashd+0x162657) (BuildId: c3fdce9f7e778985a4fb0968ff4506d9ad24d408) dashpay#1 allocate /usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/ext/new_allocator.h:114:27 (dashd+0xb772b4) (BuildId: c3fdce9f7e778985a4fb0968ff4506d9ad24d408) dashpay#2 allocate /usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/bits/alloc_traits.h:443:20 (dashd+0xb772b4) dashpay#3 _M_get_node /usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/bits/stl_tree.h:580:16 (dashd+0xb772b4) [...] dashpay#8 CoinJoinWalletManager::Add(CWallet&) /src/dash/src/coinjoin/client.cpp:1898:26 (dashd+0xb58c73) (BuildId: c3fdce9f7e778985a4fb0968ff4506d9ad24d408) [...] SUMMARY: ThreadSanitizer: data race [...] ```
1 parent 9e7c685 commit a43ab1c

10 files changed

+68
-27
lines changed

‎src/coinjoin/client.cpp

+12-3
Original file line numberDiff line numberDiff line change
@@ -96,7 +96,7 @@ PeerMsgRet CCoinJoinClientQueueManager::ProcessDSQueue(const CNode& peer, CDataS
9696
}
9797

9898
// if the queue is ready, submit if we can
99-
if (dsq.fReady && ranges::any_of(m_walletman.raw(),
99+
if (dsq.fReady && ranges::any_of(WITH_LOCK(m_walletman.cs_wallet_manager_map, return m_walletman.raw()),
100100
[this, &dmn](const auto &pair) {
101101
return pair.second->TrySubmitDenominate(dmn->pdmnState->addr,
102102
this->connman);
@@ -121,7 +121,7 @@ PeerMsgRet CCoinJoinClientQueueManager::ProcessDSQueue(const CNode& peer, CDataS
121121
LogPrint(BCLog::COINJOIN, "DSQUEUE -- new CoinJoin queue (%s) from masternode %s\n", dsq.ToString(),
122122
dmn->pdmnState->addr.ToString());
123123

124-
ranges::any_of(m_walletman.raw(),
124+
ranges::any_of(WITH_LOCK(m_walletman.cs_wallet_manager_map, return m_walletman.raw()),
125125
[&dsq](const auto &pair) { return pair.second->MarkAlreadyJoinedQueueAsTried(dsq); });
126126

127127
WITH_LOCK(cs_vecqueue, vecCoinJoinQueue.push_back(dsq));
@@ -159,7 +159,7 @@ CCoinJoinClientSession::CCoinJoinClientSession(CWallet& wallet, CoinJoinWalletMa
159159
const CMasternodeSync& mn_sync, const std::unique_ptr<CCoinJoinClientQueueManager>& queueman, bool is_masternode) :
160160
m_wallet(wallet),
161161
m_walletman(walletman),
162-
m_manager(*Assert(walletman.Get(wallet.GetName()))),
162+
m_manager(*Assert(WITH_LOCK(walletman.cs_wallet_manager_map, return walletman.Get(wallet.GetName())))),
163163
m_dmnman(dmnman),
164164
m_mn_metaman(mn_metaman),
165165
m_mn_sync(mn_sync),
@@ -1895,6 +1895,8 @@ void CCoinJoinClientManager::GetJsonInfo(UniValue& obj) const
18951895
}
18961896

18971897
void CoinJoinWalletManager::Add(CWallet& wallet) {
1898+
AssertLockHeld(cs_wallet_manager_map);
1899+
18981900
m_wallet_manager_map.try_emplace(
18991901
wallet.GetName(),
19001902
std::make_unique<CCoinJoinClientManager>(wallet, *this, m_dmnman, m_mn_metaman, m_mn_sync, m_queueman, m_is_masternode)
@@ -1903,18 +1905,25 @@ void CoinJoinWalletManager::Add(CWallet& wallet) {
19031905
}
19041906

19051907
void CoinJoinWalletManager::DoMaintenance() {
1908+
AssertLockNotHeld(cs_wallet_manager_map);
1909+
LOCK(cs_wallet_manager_map);
1910+
19061911
for (auto& [wallet_str, walletman] : m_wallet_manager_map) {
19071912
walletman->DoMaintenance(m_chainstate, m_connman, m_mempool);
19081913
}
19091914
}
19101915

19111916
void CoinJoinWalletManager::Remove(const std::string& name) {
1917+
AssertLockHeld(cs_wallet_manager_map);
1918+
19121919
m_wallet_manager_map.erase(name);
19131920
g_wallet_init_interface.InitCoinJoinSettings(*this);
19141921
}
19151922

19161923
void CoinJoinWalletManager::Flush(const std::string& name)
19171924
{
1925+
AssertLockHeld(cs_wallet_manager_map);
1926+
19181927
auto clientman = Get(name);
19191928
assert(clientman != nullptr);
19201929
clientman->ResetPool();

‎src/coinjoin/client.h

+12-7
Original file line numberDiff line numberDiff line change
@@ -72,6 +72,8 @@ class CoinJoinWalletManager {
7272
public:
7373
using wallet_name_cjman_map = std::map<const std::string, std::unique_ptr<CCoinJoinClientManager>>;
7474

75+
Mutex cs_wallet_manager_map;
76+
7577
public:
7678
CoinJoinWalletManager(CChainState& chainstate, CConnman& connman, CDeterministicMNManager& dmnman, CMasternodeMetaMan& mn_metaman, CTxMemPool& mempool,
7779
const CMasternodeSync& mn_sync, const std::unique_ptr<CCoinJoinClientQueueManager>& queueman, bool is_masternode)
@@ -85,15 +87,18 @@ class CoinJoinWalletManager {
8587
}
8688
}
8789

88-
void Add(CWallet& wallet);
89-
void DoMaintenance();
90+
void Add(CWallet& wallet) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet_manager_map);
91+
void DoMaintenance() EXCLUSIVE_LOCKS_REQUIRED(!cs_wallet_manager_map);
9092

91-
void Remove(const std::string& name);
92-
void Flush(const std::string& name);
93+
void Remove(const std::string& name) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet_manager_map);
94+
void Flush(const std::string& name) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet_manager_map);
9395

94-
CCoinJoinClientManager* Get(const std::string& name) const;
96+
CCoinJoinClientManager* Get(const std::string& name) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet_manager_map);
9597

96-
const wallet_name_cjman_map& raw() const { return m_wallet_manager_map; }
98+
const wallet_name_cjman_map& raw() const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet_manager_map)
99+
{
100+
return m_wallet_manager_map;
101+
}
97102

98103
private:
99104
CChainState& m_chainstate;
@@ -105,7 +110,7 @@ class CoinJoinWalletManager {
105110
const std::unique_ptr<CCoinJoinClientQueueManager>& m_queueman;
106111

107112
const bool m_is_masternode;
108-
wallet_name_cjman_map m_wallet_manager_map;
113+
wallet_name_cjman_map m_wallet_manager_map GUARDED_BY(cs_wallet_manager_map);
109114
};
110115

111116
class CCoinJoinClientSession : public CCoinJoinBaseSession

‎src/coinjoin/interfaces.cpp

+4-4
Original file line numberDiff line numberDiff line change
@@ -69,19 +69,19 @@ class CoinJoinLoaderImpl : public interfaces::CoinJoin::Loader
6969

7070
void AddWallet(CWallet& wallet) override
7171
{
72-
m_walletman.Add(wallet);
72+
WITH_LOCK(m_walletman.cs_wallet_manager_map, m_walletman.Add(wallet));
7373
}
7474
void RemoveWallet(const std::string& name) override
7575
{
76-
m_walletman.Remove(name);
76+
WITH_LOCK(m_walletman.cs_wallet_manager_map, m_walletman.Remove(name));
7777
}
7878
void FlushWallet(const std::string& name) override
7979
{
80-
m_walletman.Flush(name);
80+
WITH_LOCK(m_walletman.cs_wallet_manager_map, m_walletman.Flush(name));
8181
}
8282
std::unique_ptr<interfaces::CoinJoin::Client> GetClient(const std::string& name) override
8383
{
84-
auto clientman = m_walletman.Get(name);
84+
auto clientman = WITH_LOCK(m_walletman.cs_wallet_manager_map, return m_walletman.Get(name));
8585
return clientman ? std::make_unique<CoinJoinClientImpl>(*clientman) : nullptr;
8686
}
8787
CoinJoinWalletManager& walletman() override

‎src/dsnotificationinterface.cpp

+1-1
Original file line numberDiff line numberDiff line change
@@ -89,7 +89,7 @@ void CDSNotificationInterface::UpdatedBlockTip(const CBlockIndex *pindexNew, con
8989

9090
m_cj_ctx->dstxman->UpdatedBlockTip(pindexNew, *m_llmq_ctx->clhandler, m_mn_sync);
9191
#ifdef ENABLE_WALLET
92-
for (auto& pair : m_cj_ctx->walletman->raw()) {
92+
for (auto& pair : WITH_LOCK(m_cj_ctx->walletman->cs_wallet_manager_map, return m_cj_ctx->walletman->raw())) {
9393
pair.second->UpdatedBlockTip(pindexNew);
9494
}
9595
#endif // ENABLE_WALLET

‎src/init.cpp

+4-1
Original file line numberDiff line numberDiff line change
@@ -2123,7 +2123,10 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
21232123

21242124
#ifdef ENABLE_WALLET
21252125
node.coinjoin_loader = interfaces::MakeCoinJoinLoader(*node.cj_ctx->walletman);
2126-
g_wallet_init_interface.InitCoinJoinSettings(*node.cj_ctx->walletman);
2126+
{
2127+
LOCK(node.cj_ctx->walletman->cs_wallet_manager_map);
2128+
g_wallet_init_interface.InitCoinJoinSettings(*node.cj_ctx->walletman);
2129+
}
21272130
#endif // ENABLE_WALLET
21282131

21292132
// ********************************************************* Step 7d: Setup other Dash services

‎src/masternode/utils.cpp

+1-1
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@ void CMasternodeUtils::DoMaintenance(CConnman& connman, CDeterministicMNManager&
2323

2424
std::vector<CDeterministicMNCPtr> vecDmns; // will be empty when no wallet
2525
#ifdef ENABLE_WALLET
26-
for (auto& pair : cj_ctx.walletman->raw()) {
26+
for (auto& pair : WITH_LOCK(cj_ctx.walletman->cs_wallet_manager_map, return cj_ctx.walletman->raw())) {
2727
pair.second->GetMixingMasternodesInfo(vecDmns);
2828
}
2929
#endif // ENABLE_WALLET

‎src/net_processing.cpp

+1-1
Original file line numberDiff line numberDiff line change
@@ -5095,7 +5095,7 @@ void PeerManagerImpl::ProcessMessage(
50955095
//probably one the extensions
50965096
#ifdef ENABLE_WALLET
50975097
ProcessPeerMsgRet(m_cj_ctx->queueman->ProcessMessage(pfrom, msg_type, vRecv), pfrom);
5098-
for (auto& pair : m_cj_ctx->walletman->raw()) {
5098+
for (auto& pair : WITH_LOCK(m_cj_ctx->walletman->cs_wallet_manager_map, return m_cj_ctx->walletman->raw())) {
50995099
pair.second->ProcessMessage(pfrom, m_chainman.ActiveChainstate(), m_connman, m_mempool, msg_type, vRecv);
51005100
}
51015101
#endif // ENABLE_WALLET

‎src/rpc/coinjoin.cpp

+24-6
Original file line numberDiff line numberDiff line change
@@ -84,7 +84,10 @@ static RPCHelpMan coinjoin_reset()
8484
ValidateCoinJoinArguments();
8585

8686
CHECK_NONFATAL(node.coinjoin_loader);
87-
auto cj_clientman = node.coinjoin_loader->walletman().Get(wallet->GetName());
87+
auto cj_clientman = [&](){
88+
LOCK(node.coinjoin_loader->walletman().cs_wallet_manager_map);
89+
return node.coinjoin_loader->walletman().Get(wallet->GetName());
90+
}();
8891

8992
CHECK_NONFATAL(cj_clientman);
9093
cj_clientman->ResetPool();
@@ -127,7 +130,10 @@ static RPCHelpMan coinjoin_start()
127130
}
128131

129132
CHECK_NONFATAL(node.coinjoin_loader);
130-
auto cj_clientman = node.coinjoin_loader->walletman().Get(wallet->GetName());
133+
auto cj_clientman = [&](){
134+
LOCK(node.coinjoin_loader->walletman().cs_wallet_manager_map);
135+
return node.coinjoin_loader->walletman().Get(wallet->GetName());
136+
}();
131137

132138
CHECK_NONFATAL(cj_clientman);
133139
if (!cj_clientman->StartMixing()) {
@@ -169,7 +175,10 @@ static RPCHelpMan coinjoin_stop()
169175
ValidateCoinJoinArguments();
170176

171177
CHECK_NONFATAL(node.coinjoin_loader);
172-
auto cj_clientman = node.coinjoin_loader->walletman().Get(wallet->GetName());
178+
auto cj_clientman = [&](){
179+
LOCK(node.coinjoin_loader->walletman().cs_wallet_manager_map);
180+
return node.coinjoin_loader->walletman().Get(wallet->GetName());
181+
}();
173182

174183
CHECK_NONFATAL(cj_clientman);
175184
if (!cj_clientman->IsMixing()) {
@@ -239,7 +248,10 @@ static RPCHelpMan coinjoinsalt_generate()
239248

240249
const NodeContext& node = EnsureAnyNodeContext(request.context);
241250
if (node.coinjoin_loader != nullptr) {
242-
auto cj_clientman = node.coinjoin_loader->walletman().Get(wallet->GetName());
251+
auto cj_clientman = [&](){
252+
LOCK(node.coinjoin_loader->walletman().cs_wallet_manager_map);
253+
return node.coinjoin_loader->walletman().Get(wallet->GetName());
254+
}();
243255
if (cj_clientman != nullptr && cj_clientman->IsMixing()) {
244256
throw JSONRPCError(RPC_WALLET_ERROR,
245257
strprintf("Wallet \"%s\" is currently mixing, cannot change salt!", str_wallet));
@@ -341,7 +353,10 @@ static RPCHelpMan coinjoinsalt_set()
341353

342354
const NodeContext& node = EnsureAnyNodeContext(request.context);
343355
if (node.coinjoin_loader != nullptr) {
344-
auto cj_clientman = node.coinjoin_loader->walletman().Get(wallet->GetName());
356+
auto cj_clientman = [&](){
357+
LOCK(node.coinjoin_loader->walletman().cs_wallet_manager_map);
358+
return node.coinjoin_loader->walletman().Get(wallet->GetName());
359+
}();
345360
if (cj_clientman != nullptr && cj_clientman->IsMixing()) {
346361
throw JSONRPCError(RPC_WALLET_ERROR,
347362
strprintf("Wallet \"%s\" is currently mixing, cannot change salt!", str_wallet));
@@ -450,7 +465,10 @@ static RPCHelpMan getcoinjoininfo()
450465
return obj;
451466
}
452467

453-
auto manager = node.coinjoin_loader->walletman().Get(wallet->GetName());
468+
auto manager = [&](){
469+
LOCK(node.coinjoin_loader->walletman().cs_wallet_manager_map);
470+
return node.coinjoin_loader->walletman().Get(wallet->GetName());
471+
}();
454472
CHECK_NONFATAL(manager != nullptr);
455473
manager->GetJsonInfo(obj);
456474

‎src/wallet/init.cpp

+2-1
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,8 @@ class WalletInit : public WalletInitInterface
4646

4747
// Dash Specific Wallet Init
4848
void AutoLockMasternodeCollaterals() const override;
49-
void InitCoinJoinSettings(const CoinJoinWalletManager& cjwalletman) const override;
49+
void InitCoinJoinSettings(const CoinJoinWalletManager& cjwalletman) const override
50+
EXCLUSIVE_LOCKS_REQUIRED(cjwalletman.cs_wallet_manager_map);
5051
bool InitAutoBackup() const override;
5152
};
5253

‎src/wallet/test/coinjoin_tests.cpp

+7-2
Original file line numberDiff line numberDiff line change
@@ -209,8 +209,13 @@ class CTransactionBuilderTestSetup : public TestChain100Setup
209209

210210
BOOST_FIXTURE_TEST_CASE(coinjoin_manager_start_stop_tests, CTransactionBuilderTestSetup)
211211
{
212-
BOOST_CHECK_EQUAL(m_node.cj_ctx->walletman->raw().size(), 1);
213-
auto& cj_man = m_node.cj_ctx->walletman->raw().begin()->second;
212+
CCoinJoinClientManager* cj_man{nullptr};
213+
{
214+
LOCK(m_node.cj_ctx->walletman->cs_wallet_manager_map);
215+
BOOST_CHECK_EQUAL(m_node.cj_ctx->walletman->raw().size(), 1);
216+
cj_man = m_node.cj_ctx->walletman->raw().begin()->second.get();
217+
}
218+
BOOST_ASSERT(cj_man != nullptr);
214219
BOOST_CHECK_EQUAL(cj_man->IsMixing(), false);
215220
BOOST_CHECK_EQUAL(cj_man->StartMixing(), true);
216221
BOOST_CHECK_EQUAL(cj_man->IsMixing(), true);

0 commit comments

Comments
 (0)