getTipLocator() reads chainActive without holding cs_main despite LockAnnotation lock(::cs_main) guarantee #16028

issue practicalswift opened this issue on May 15, 2019

practicalswift commented at 1:15 PM on May 15, 2019: contributor

LockAnnotation lock(::cs_main) is a guarantee to the compiler thread analysis that ::cs_main is locked (when it couldn't be determined otherwise).

Despite being annotated with the locking guarantee ...

https://github.com/bitcoin/bitcoin/blob/65526fc8666fef35ef908dbc225f706bef642c7e/src/interfaces/chain.cpp#L134-L138

... getTipLocator() reads chainActive (via ::ChainActive()) without holding cs_main.

This can be verified by adding the following AssertLockHeld(cs_main):

$ git diff
diff --git a/src/interfaces/chain.cpp b/src/interfaces/chain.cpp
index 59623284d..9fc693a0f 100644
--- a/src/interfaces/chain.cpp
+++ b/src/interfaces/chain.cpp
@@ -134,6 +134,7 @@ class LockImpl : public Chain::Lock
     CBlockLocator getTipLocator() override
     {
         LockAnnotation lock(::cs_main);
+        AssertLockHeld(::cs_main);
         return ::ChainActive().GetLocator();
     }
     Optional<int> findLocatorFork(const CBlockLocator& locator) override
$ make check
../build-aux/test-driver: line 107: 12881 Aborted                 "$@" > $log_file 2>&1
FAIL: qt/test/test_bitcoin-qt

ryanofsky commented at 2:25 PM on May 15, 2019: member

It'd be useful to see a stack trace where the assert fails. Probably this is due to an assumeLocked() usage, and at this point I think we can pretty easily remove the assumeLocked method, which was supposed to be temporary. The last hard to replace assumeLocked usage was removed in 52b760fc6a9b26e405a0553ee8285b0f03ca1604 in #15632.
MarcoFalke added this to the milestone 0.19.0 on May 15, 2019

practicalswift commented at 2:52 PM on May 15, 2019: contributor

@ryanofsky Here goes:

Assertion failed: lock ::cs_main not held in interfaces/chain.cpp:137; locks held:

…
(gdb) bt
[#0](/bitcoin-bitcoin/0/)  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:51
[#1](/bitcoin-bitcoin/1/)  0x00007ffff2d23801 in __GI_abort () at abort.c:79
[#2](/bitcoin-bitcoin/2/)  0x0000555555aa7cfd in AssertLockHeldInternal (pszName=0x555556106524 "::cs_main", pszFile=0x5555561063b2 "interfaces/chain.cpp", nLine=137, cs=<optimized out>) at sync.cpp:175
[#3](/bitcoin-bitcoin/3/)  0x0000555555774537 in interfaces::(anonymous namespace)::LockImpl::getTipLocator (this=<optimized out>) at interfaces/chain.cpp:137
[#4](/bitcoin-bitcoin/4/)  0x0000555555b53630 in CWallet::CreateWalletFromFile (chain=..., location=..., wallet_creation_flags=0) at wallet/wallet.cpp:4078
[#5](/bitcoin-bitcoin/5/)  0x0000555555b07523 in LoadWallets (chain=..., wallet_files=...) at wallet/load.cpp:68
[#6](/bitcoin-bitcoin/6/)  0x0000555555ad383e in interfaces::(anonymous namespace)::WalletClientImpl::load (this=<optimized out>) at interfaces/wallet.cpp:521
[#7](/bitcoin-bitcoin/7/)  0x000055555579282b in AppInitMain (interfaces=...) at init.cpp:1681
[#8](/bitcoin-bitcoin/8/)  0x000055555577c8d7 in interfaces::(anonymous namespace)::NodeImpl::appInitMain (this=<optimized out>) at interfaces/node.cpp:78
[#9](/bitcoin-bitcoin/9/)  0x000055555564b0ec in BitcoinCore::initialize (this=0x555556f2cc60) at qt/bitcoin.cpp:154

MarcoFalke added the label GUI on May 15, 2019
MarcoFalke renamed this:
~~getTipLocator() reads chainActive without holding cs_main despite LockAnnotation lock(::cs_main) guarantee~~
gui: getTipLocator() reads chainActive without holding cs_main despite LockAnnotation lock(::cs_main) guarantee
on May 15, 2019

practicalswift commented at 3:11 PM on May 15, 2019: contributor

@ryanofsky Like this?

diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp
index 69a78f1fc..44db2f29b 100644
--- a/src/wallet/test/wallet_tests.cpp
+++ b/src/wallet/test/wallet_tests.cpp
@@ -387,7 +387,7 @@ public:
     }

     std::unique_ptr<interfaces::Chain> m_chain = interfaces::MakeChain();
-    std::unique_ptr<interfaces::Chain::Lock> m_locked_chain = m_chain->assumeLocked();  // Temporary. Removed in upcoming lock cleanup
+    std::unique_ptr<interfaces::Chain::Lock> m_locked_chain = m_chain->lock();
     std::unique_ptr<CWallet> wallet;
 };

diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index 054329fbd..0822c7c30 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -4074,7 +4074,7 @@ std::shared_ptr<CWallet> CWallet::CreateWalletFromFile(interfaces::Chain& chain,
             return nullptr;
         }

-        auto locked_chain = chain.assumeLocked();  // Temporary. Removed in upcoming lock cleanup
+        auto locked_chain = chain.lock();
         walletInstance->ChainStateFlushed(locked_chain->getTipLocator());
     } else if (wallet_creation_flags & WALLET_FLAG_DISABLE_PRIVATE_KEYS) {
         // Make it impossible to disable private keys after creation

ryanofsky commented at 3:21 PM on May 15, 2019: member

@ryanofsky Like this?

Yes, I think that should work. As followup, the assumeLocked method declarations and definitions could also be removed, the LockingStateImpl class could be deleted, and LockImpl could inherit directly from UniqueLock.
MarcoFalke removed the label GUI on May 15, 2019
MarcoFalke renamed this:
~~gui: getTipLocator() reads chainActive without holding cs_main despite LockAnnotation lock(::cs_main) guarantee~~
getTipLocator() reads chainActive without holding cs_main despite LockAnnotation lock(::cs_main) guarantee
on May 15, 2019
practicalswift commented at 8:50 AM on May 16, 2019: contributor

Fix in #16033.
MarcoFalke closed this on May 17, 2019
MarcoFalke referenced this in commit f3d27d126b on May 17, 2019
MarcoFalke referenced this in commit 65c4bbe629 on May 23, 2019
sidhujag referenced this in commit 7fb5ee1cab on May 24, 2019
jasonbcox referenced this in commit e942ee183d on Jul 2, 2020
DrahtBot locked this on Dec 16, 2021

Contributors

practicalswift

ryanofsky

Milestone
0.19.0

Linked (view graph)

#16033 Hold cs_main when reading chainActive via getTipLocator(). Remove assumeLocked().#16034 refactoring: Rename LockAnnotation to LockAssertion and add run-time check to it