diff options
author | Wladimir J. van der Laan <laanwj@protonmail.com> | 2020-04-06 20:29:16 +0200 |
---|---|---|
committer | Wladimir J. van der Laan <laanwj@protonmail.com> | 2020-04-06 20:29:35 +0200 |
commit | 75021e80ee4439dddadbe8c586cee04b85ac110c (patch) | |
tree | b0bcab3fb3dc40a7879dbd8c0f742fda763eb65f | |
parent | 54d5ba3d9cb45d8417ecca0f09c68d865d0c423c (diff) | |
parent | 7b8e15728d1ad058a4b7d7569fd5d5ba6806ca28 (diff) | |
download | bitcoin-75021e80ee4439dddadbe8c586cee04b85ac110c.tar.xz |
Merge #18487: rpc: Fix rpcRunLater race in walletpassphrase
7b8e15728d1ad058a4b7d7569fd5d5ba6806ca28 rpc: Fix rpcRunLater race in walletpassphrase (João Barbosa)
Pull request description:
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.
ACKs for top commit:
MarcoFalke:
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. 📞
ryanofsky:
Code review ACK 7b8e15728d1ad058a4b7d7569fd5d5ba6806ca28. Just updated comment since last review
Tree-SHA512: 17874a2fa7b0e164fb0d7ee4cb7d59650275b8c03476fb291d60af8b758495457660d3912623fb26259fefe84aeba21c0a9e0c6467982ba511f19344ed5413ab
-rw-r--r-- | src/wallet/rpcwallet.cpp | 70 |
1 files changed, 39 insertions, 31 deletions
diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index d8b3b57b61..ea1d937849 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -1912,44 +1912,52 @@ static UniValue walletpassphrase(const JSONRPCRequest& request) }, }.Check(request); - auto locked_chain = pwallet->chain().lock(); - LOCK(pwallet->cs_wallet); - - if (!pwallet->IsCrypted()) { - throw JSONRPCError(RPC_WALLET_WRONG_ENC_STATE, "Error: running with an unencrypted wallet, but walletpassphrase was called."); - } + int64_t nSleepTime; + { + auto locked_chain = pwallet->chain().lock(); + LOCK(pwallet->cs_wallet); - // Note that the walletpassphrase is stored in request.params[0] which is not mlock()ed - SecureString strWalletPass; - strWalletPass.reserve(100); - // TODO: get rid of this .c_str() by implementing SecureString::operator=(std::string) - // Alternately, find a way to make request.params[0] mlock()'d to begin with. - strWalletPass = request.params[0].get_str().c_str(); + if (!pwallet->IsCrypted()) { + throw JSONRPCError(RPC_WALLET_WRONG_ENC_STATE, "Error: running with an unencrypted wallet, but walletpassphrase was called."); + } - // Get the timeout - int64_t nSleepTime = request.params[1].get_int64(); - // Timeout cannot be negative, otherwise it will relock immediately - if (nSleepTime < 0) { - throw JSONRPCError(RPC_INVALID_PARAMETER, "Timeout cannot be negative."); - } - // Clamp timeout - constexpr int64_t MAX_SLEEP_TIME = 100000000; // larger values trigger a macos/libevent bug? - if (nSleepTime > MAX_SLEEP_TIME) { - nSleepTime = MAX_SLEEP_TIME; - } + // Note that the walletpassphrase is stored in request.params[0] which is not mlock()ed + SecureString strWalletPass; + strWalletPass.reserve(100); + // TODO: get rid of this .c_str() by implementing SecureString::operator=(std::string) + // Alternately, find a way to make request.params[0] mlock()'d to begin with. + strWalletPass = request.params[0].get_str().c_str(); + + // Get the timeout + nSleepTime = request.params[1].get_int64(); + // Timeout cannot be negative, otherwise it will relock immediately + if (nSleepTime < 0) { + throw JSONRPCError(RPC_INVALID_PARAMETER, "Timeout cannot be negative."); + } + // Clamp timeout + constexpr int64_t MAX_SLEEP_TIME = 100000000; // larger values trigger a macos/libevent bug? + if (nSleepTime > MAX_SLEEP_TIME) { + nSleepTime = MAX_SLEEP_TIME; + } - if (strWalletPass.empty()) { - throw JSONRPCError(RPC_INVALID_PARAMETER, "passphrase can not be empty"); - } + if (strWalletPass.empty()) { + throw JSONRPCError(RPC_INVALID_PARAMETER, "passphrase can not be empty"); + } - if (!pwallet->Unlock(strWalletPass)) { - throw JSONRPCError(RPC_WALLET_PASSPHRASE_INCORRECT, "Error: The wallet passphrase entered was incorrect."); - } + if (!pwallet->Unlock(strWalletPass)) { + throw JSONRPCError(RPC_WALLET_PASSPHRASE_INCORRECT, "Error: The wallet passphrase entered was incorrect."); + } - pwallet->TopUpKeyPool(); + pwallet->TopUpKeyPool(); - pwallet->nRelockTime = GetTime() + nSleepTime; + pwallet->nRelockTime = GetTime() + nSleepTime; + } + // rpcRunLater must be called without cs_wallet held otherwise a deadlock + // can occur. The deadlock would happen when RPCRunLater removes the + // previous timer (and waits for the callback to finish if already running) + // and the callback locks cs_wallet. + AssertLockNotHeld(wallet->cs_wallet); // Keep a weak pointer to the wallet so that it is possible to unload the // wallet before the following callback is called. If a valid shared pointer // is acquired in the callback then the wallet is still loaded. |