diff options
author | Vasil Dimov <vd@FreeBSD.org> | 2020-06-01 14:04:36 +0200 |
---|---|---|
committer | Vasil Dimov <vd@FreeBSD.org> | 2020-06-01 17:14:43 +0200 |
commit | f46b678acff0b2e75e26aa50b14d935b3d251a2a (patch) | |
tree | c44064bba46e0dc3e0ae07822a9a244645299a4e /src | |
parent | 9bc7751cadbd038faf8ac1d62cda23fcf00d4cc2 (diff) |
qt: lock cs_main, m_cached_tip_mutex in that order
Always lock the mutexes `cs_main` and `m_cached_tip_mutex` in
the same order: `cs_main`, `m_cached_tip_mutex`. Otherwise we may end up
in a deadlock.
`ClientModel::m_cached_tip_blocks` is protected by
`ClientModel::m_cached_tip_mutex`. There are two access paths that
lock the two mutexes in opposite order:
```
validation.cpp:2868 CChainState::ActivateBestChain(): lock cs_main
validation.cpp:2916 CChainState::ActivateBestChain(): call uiInterface.NotifyBlockTip()
ui_interface.cpp:52 CClientUIInterface::NotifyBlockTip(): go deep in boost
...
qt/clientmodel.cpp:255 BlockTipChanged(): lock m_cached_tip_mutex
```
and
```
qt/clientmodel.cpp:119 ClientModel::getBestBlockHash(): lock m_cached_tip_mutex
qt/clientmodel.cpp:121 ClientModel::getBestBlockHash(): call m_node.getBestBlockHash()
interfaces/node.cpp:200 NodeImpl::getBestBlockHash(): lock cs_main
```
From `debug.log`:
```
POTENTIAL DEADLOCK DETECTED
Previous lock order was:
m_cs_chainstate validation.cpp:2851
(1) cs_main validation.cpp:2868
::mempool.cs validation.cpp:2868
(2) clientmodel->m_cached_tip_mutex qt/clientmodel.cpp:255
Current lock order is:
(2) m_cached_tip_mutex qt/clientmodel.cpp:119
(1) ::cs_main interfaces/node.cpp:200
```
The possible deadlock was introduced in
https://github.com/bitcoin/bitcoin/pull/17993
Diffstat (limited to 'src')
-rw-r--r-- | src/qt/clientmodel.cpp | 16 |
1 files changed, 15 insertions, 1 deletions
diff --git a/src/qt/clientmodel.cpp b/src/qt/clientmodel.cpp index f15921c5bc..c1d358f7f6 100644 --- a/src/qt/clientmodel.cpp +++ b/src/qt/clientmodel.cpp @@ -116,9 +116,23 @@ int ClientModel::getNumBlocks() const uint256 ClientModel::getBestBlockHash() { + uint256 tip{WITH_LOCK(m_cached_tip_mutex, return m_cached_tip_blocks)}; + + if (!tip.IsNull()) { + return tip; + } + + // Lock order must be: first `cs_main`, then `m_cached_tip_mutex`. + // The following will lock `cs_main` (and release it), so we must not + // own `m_cached_tip_mutex` here. + tip = m_node.getBestBlockHash(); + LOCK(m_cached_tip_mutex); + // We checked that `m_cached_tip_blocks` is not null above, but then we + // released the mutex `m_cached_tip_mutex`, so it could have changed in the + // meantime. Thus, check again. if (m_cached_tip_blocks.IsNull()) { - m_cached_tip_blocks = m_node.getBestBlockHash(); + m_cached_tip_blocks = tip; } return m_cached_tip_blocks; } |