rpc: Fix rpcRunLater race in walletpassphrase

promag commented at 11:30 pm on March 31, 2020: member

Release locks before calling rpcRunLater.

Quick explanation: rpcRunLater leads to event_free which calls event_del which can wait for the event callback to finish if it’s already running and that callback will try to lock wallet mutex - which is already locked in http thread.

Fixes #14995 , fixes #18482. Best reviewed with whitespace changes hidden.

DrahtBot added the label RPC/REST/ZMQ on Mar 31, 2020

DrahtBot added the label Wallet on Mar 31, 2020

promag force-pushed on Apr 1, 2020

promag closed this on Apr 1, 2020

promag reopened this on Apr 1, 2020

promag renamed this:
~~wip: rpc: Fix rpcRunLater race in walletpassphrase~~
rpc: Fix rpcRunLater race in walletpassphrase
on Apr 1, 2020

laanwj added this to the milestone 0.20.0 on Apr 1, 2020

DrahtBot commented at 9:56 am on April 1, 2020: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

in src/wallet/rpcwallet.cpp:1960 in 0944cafe29 outdated

1986-    pwallet->TopUpKeyPool();
1987+        pwallet->TopUpKeyPool();
1988 
1989-    pwallet->nRelockTime = GetTime() + nSleepTime;
1990+        pwallet->nRelockTime = GetTime() + nSleepTime;
1991+    }

MarcoFalke commented at 3:23 pm on April 2, 2020:

Would be nice to add a comment why this scope is needed for cs_wallet

promag commented at 3:25 pm on April 2, 2020:

Ok.

MarcoFalke commented at 3:27 pm on April 2, 2020: member

Can be tested with #18507

promag force-pushed on Apr 2, 2020

in src/wallet/rpcwallet.cpp:1969 in 47dd37cc83 outdated

1990+        pwallet->nRelockTime = GetTime() + nSleepTime;
1991+    }
1992 
1993     // Keep a weak pointer to the wallet so that it is possible to unload the
1994     // wallet before the following callback is called. If a valid shared pointer
1995     // is acquired in the callback then the wallet is still loaded.

MarcoFalke commented at 3:33 pm on April 2, 2020:

Should this say “locked” instead of “loaded”?

promag commented at 4:19 pm on April 2, 2020:

No, loaded. This is to handle the case a wallet is unloaded before the timer is triggered.

in src/wallet/rpcwallet.cpp:1965 in 47dd37cc83 outdated

1991+    }
1992 
1993     // Keep a weak pointer to the wallet so that it is possible to unload the
1994     // wallet before the following callback is called. If a valid shared pointer
1995     // is acquired in the callback then the wallet is still loaded.
1996+    // Currently this must be called without locks held otherwise a deadlock

MarcoFalke commented at 3:34 pm on April 2, 2020:

Why “currently”? Is there a plan to change this in the future?

Also, could add a AssertLockNotHeld(cs_wallet); to accommodate the comment?

promag commented at 4:20 pm on April 2, 2020:

Why “currently”? Is there a plan to change this in the future?

No plans. However I think it could be improved because after the timer stays in deadlineTimers after being triggered.

promag commented at 4:21 pm on April 2, 2020:

Also, could add a AssertLockNotHeld(cs_wallet); to accommodate the comment?

Done.

in src/wallet/rpcwallet.cpp:1966 in 47dd37cc83 outdated

1992 
1993     // Keep a weak pointer to the wallet so that it is possible to unload the
1994     // wallet before the following callback is called. If a valid shared pointer
1995     // is acquired in the callback then the wallet is still loaded.
1996+    // Currently this must be called without locks held otherwise a deadlock
1997+    // can occur with cs_wallet. For more details see #18487.

MarcoFalke commented at 3:37 pm on April 2, 2020:

Instead of plain hashtag and number, which is hard to quickly look up (especially without internet), what about describing the issue here briefly?

MarcoFalke commented at 4:08 pm on April 2, 2020:

The comment from the pull request description seems suitable

promag commented at 4:21 pm on April 2, 2020:

Will improve&mix.

MarcoFalke commented at 4:09 pm on April 2, 2020: member

ACK 47dd37cc83bdcaf19a3abe95950b2409baae6249 only tested that this avoids the node freezing. Didn’t look at how libevent works or how the deadlock happens or if this breaks other stuff.

ryanofsky approved

ryanofsky commented at 4:24 pm on April 2, 2020: member

Code review ACK 0944cafe294d4ee009b615fc8d8caca77c117481. This is a nice, clean bug fix, but agree with Marco’s suggestions to improve the comment. Adding the explanation of why the lock is released from the PR description would be very helpful. And it doesn’t seem right to say “Currently this must be called without locks held”, when I don’t think it was or will be safe to safe to add a timer with locks held.

rpc: Fix rpcRunLater race in walletpassphrase 7b8e15728d

promag force-pushed on Apr 2, 2020

ryanofsky approved

ryanofsky commented at 4:27 pm on April 2, 2020: member

Code review ACK 7b8e15728d1ad058a4b7d7569fd5d5ba6806ca28. Just updated comment since last review

MarcoFalke commented at 4:31 pm on April 2, 2020: member

ACK 7b8e15728d, only tested that this avoids the node freezing. Did not look at how libevent works or how the deadlock happens or if this breaks other stuff. 📞

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3ACK 7b8e15728d, only tested that this avoids the node freezing. Did not look at how libevent works or how the deadlock happens or if this breaks other stuff. 📞
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUjKCgwAjQesMVDRk/0HJqT63QzYXgsky/+kGB7rdd3glG/ytl06M6dgM3bgepAv
 8t2Jfe/uGUugB9v0ettQ5/MkXewKi/r9dllozXIf6ZYyW0tzQKN22kSO6ybRcn0bL
 9GJDJeK35QG5CEoRB8kJiwohGhxPBeI0HF9LkkfEb7eLsXJGH4hliuQ9xpc0KGr32
100Sj3SUttKlK4xUbyDVyC7VOGeD53ZphJFiq+HPMY3lm6FmXn5Z2wXv6/FeV2jFQb
11oxCCJ52gBzijv5YImikXjeOBEElUhI2IuIKCmTvNgXFv3T/c/ToSPShFpH2j2btP
12cCzsIK3pex8fJSZI1ZE+PszAeh2FejdDN144fP7Ds/9fOwXIARU/vPVCgXeDsHZe
130yl17Qtl6p1YAGK2NGDXNM8UOA9MtzqAUyDBl4jRblIux3qEwEsxskjsDUv5d3JX
14Im6mmvKOk/5loNPGXNhIE150YdbYZ4PNKjy4ke6rLvMW60tX0PnhfwOKWGdVM5iw
15Lxj6BnQy
16=jUhE
17-----END PGP SIGNATURE-----

Timestamp of file with hash 72e45144d2300c2bfb37bf5663a6b1678638fd7ac770b9ac957658a6a77b4224 -

promag commented at 4:33 pm on April 2, 2020: member

I didn’t want to write libevent details in rpcwallet and also how RPCRunLater is implemented.

promag commented at 4:45 pm on April 2, 2020: member

@MarcoFalke the deadlock happens in this case:

(we are in one of the http thread)
walletpassphrase
RPCRunLater
~HTTPEvent()
event_free()
event_del()
event_del_(, EVENT_DEL_AUTOBLOCK);

And from libevent header

0/** Argument for event_del_nolock_. Tells event_del to block on the event
1 * if it is running in another thread and it doesn't have EV_FINALIZE set.
2 */
3#define EVENT_DEL_AUTOBLOCK 2

And the event callback (triggered from the event base thread) will lock cs_wallet and deadlock: https://github.com/bitcoin/bitcoin/blob/b83565625e32b22395e28c1965b2e42fc17f04d7/src/wallet/rpcwallet.cpp#L1963-L1969

Also, I think its reasonable to reduce lock scope (which we already did in other cases for other reasons) than to change the EV_FINALIZE flag.

or if this breaks other stuff.

I can’t see how that could happen. but..

laanwj merged this on Apr 6, 2020

laanwj closed this on Apr 6, 2020

promag deleted the branch on Apr 6, 2020

sidhujag referenced this in commit 82255a05ec on Apr 8, 2020

promag commented at 2:57 am on April 29, 2020: member

or if this breaks other stuff.

I can’t see how that could happen. but.. @MarcoFalke but #18811 happened.

fanquake referenced this in commit a33901cb6d on May 13, 2020

sidhujag referenced this in commit c7204ec263 on May 14, 2020

jasonbcox referenced this in commit 8c361009af on Oct 13, 2020

DrahtBot locked this on Feb 15, 2022

rpc: Fix rpcRunLater race in walletpassphrase #18487

Conflicts