Skip to content

Commit

Permalink
fix: protect m_wallet_manager_map with cs_wallet_manager_map
Browse files Browse the repository at this point in the history
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 [...]
```
  • Loading branch information
kwvg committed Aug 10, 2024
1 parent b136742 commit b85ac23
Show file tree
Hide file tree
Showing 3 changed files with 17 additions and 8 deletions.
7 changes: 7 additions & 0 deletions src/coinjoin/client.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1895,6 +1895,8 @@ void CCoinJoinClientManager::GetJsonInfo(UniValue& obj) const
}

void CoinJoinWalletManager::Add(CWallet& wallet) {
AssertLockHeld(cs_wallet_manager_map);

m_wallet_manager_map.try_emplace(
wallet.GetName(),
std::make_unique<CCoinJoinClientManager>(wallet, *this, m_dmnman, m_mn_metaman, m_mn_sync, m_queueman, m_is_masternode)
Expand All @@ -1903,12 +1905,17 @@ void CoinJoinWalletManager::Add(CWallet& wallet) {
}

void CoinJoinWalletManager::DoMaintenance() {
AssertLockNotHeld(cs_wallet_manager_map);
LOCK(cs_wallet_manager_map);

for (auto& [wallet_str, walletman] : m_wallet_manager_map) {
walletman->DoMaintenance(m_chainstate, m_connman, m_mempool);
}
}

void CoinJoinWalletManager::Remove(const std::string& name) {
AssertLockHeld(cs_wallet_manager_map);

m_wallet_manager_map.erase(name);
g_wallet_init_interface.InitCoinJoinSettings(*this);
}
Expand Down
12 changes: 7 additions & 5 deletions src/coinjoin/client.h
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,8 @@ class CoinJoinWalletManager {
public:
using wallet_name_cjman_map = std::map<const std::string, std::unique_ptr<CCoinJoinClientManager>>;

Mutex cs_wallet_manager_map;

public:
CoinJoinWalletManager(CChainState& chainstate, CConnman& connman, CDeterministicMNManager& dmnman, CMasternodeMetaMan& mn_metaman, CTxMemPool& mempool,
const CMasternodeSync& mn_sync, const std::unique_ptr<CCoinJoinClientQueueManager>& queueman, bool is_masternode)
Expand All @@ -85,11 +87,11 @@ class CoinJoinWalletManager {
}
}

void Add(CWallet& wallet);
void DoMaintenance();
void Add(CWallet& wallet) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet_manager_map);
void DoMaintenance() EXCLUSIVE_LOCKS_REQUIRED(!cs_wallet_manager_map);

void Remove(const std::string& name);
void Flush(const std::string& name);
void Remove(const std::string& name) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet_manager_map);
void Flush(const std::string& name) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet_manager_map);

CCoinJoinClientManager* Get(const std::string& name) const;

Expand All @@ -105,7 +107,7 @@ class CoinJoinWalletManager {
const std::unique_ptr<CCoinJoinClientQueueManager>& m_queueman;

const bool m_is_masternode;
wallet_name_cjman_map m_wallet_manager_map;
wallet_name_cjman_map m_wallet_manager_map GUARDED_BY(cs_wallet_manager_map);
};

class CCoinJoinClientSession : public CCoinJoinBaseSession
Expand Down
6 changes: 3 additions & 3 deletions src/coinjoin/interfaces.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -69,15 +69,15 @@ class CoinJoinLoaderImpl : public interfaces::CoinJoin::Loader

void AddWallet(CWallet& wallet) override
{
m_walletman.Add(wallet);
WITH_LOCK(m_walletman.cs_wallet_manager_map, m_walletman.Add(wallet));
}
void RemoveWallet(const std::string& name) override
{
m_walletman.Remove(name);
WITH_LOCK(m_walletman.cs_wallet_manager_map, m_walletman.Remove(name));
}
void FlushWallet(const std::string& name) override
{
m_walletman.Flush(name);
WITH_LOCK(m_walletman.cs_wallet_manager_map, m_walletman.Flush(name));
}
std::unique_ptr<interfaces::CoinJoin::Client> GetClient(const std::string& name) override
{
Expand Down

0 comments on commit b85ac23

Please sign in to comment.