From da7ca00f036ca5963ac797e63145de903bf9e3c1 Mon Sep 17 00:00:00 2001 From: fanquake Date: Wed, 13 May 2020 17:29:10 +0800 Subject: [PATCH 1/2] Merge #18814: rpc: Relock wallet only if most recent callback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 9f59dde9740d065118bdddde75ef9f4e4603a7b1 rpc: Relock wallet only if most recent callback (João Barbosa) a2e6db5c4f1bb52a8814102b628e51652493d06a rpc: Add mutex to guard deadlineTimers (João Barbosa) Pull request description: This PR fixes an early relocking race condition from #18811 where old relock callback runs after new wallet unlock code and nRelockTime update but before rpcRunLater call, causing early relock and incorrect nRelockTime time Issue introduced in #18487. Fixes #18811. ACKs for top commit: MarcoFalke: ACK 9f59dde9740d065118bdddde75ef9f4e4603a7b1 ryanofsky: Code review ACK 9f59dde9740d065118bdddde75ef9f4e4603a7b1. No changes since last review except squashing commits. jonatack: ACK 9f59dde9740d065118bdddde75ef Tree-SHA512: 2f7fc03e5ab6037337f2d82dfad432495cc337c77d07c968ee2355105db6292f24543c03456f5402e0e759577a4327758f9372f7ea29de6d56dc3695fda9b379 --- src/rpc/server.cpp | 6 ++++-- src/wallet/rpcwallet.cpp | 9 +++++++-- src/wallet/wallet.h | 4 +++- 3 files changed, 14 insertions(+), 5 deletions(-) diff --git a/src/rpc/server.cpp b/src/rpc/server.cpp index a87c7e2263..6ccd7c278f 100644 --- a/src/rpc/server.cpp +++ b/src/rpc/server.cpp @@ -28,7 +28,8 @@ static std::string rpcWarmupStatus GUARDED_BY(g_rpc_warmup_mutex) = "RPC server /* Timer-creating functions */ static RPCTimerInterface* timerInterface = nullptr; /* Map of name to timer. */ -static std::map > deadlineTimers; +static Mutex g_deadline_timers_mutex; +static std::map > deadlineTimers GUARDED_BY(g_deadline_timers_mutex); static bool ExecuteCommand(const CRPCCommand& command, const JSONRPCRequest& request, UniValue& result, bool last_handler, std::multimap> mapPlatformRestrictions); // Any commands submitted by this user will have their commands filtered based on the mapPlatformRestrictions @@ -330,7 +331,7 @@ void InterruptRPC() void StopRPC() { LogPrint(BCLog::RPC, "Stopping RPC\n"); - deadlineTimers.clear(); + WITH_LOCK(g_deadline_timers_mutex, deadlineTimers.clear()); DeleteAuthCookie(); g_rpcSignals.Stopped(); } @@ -609,6 +610,7 @@ void RPCRunLater(const std::string& name, std::function func, int64_t nS { if (!timerInterface) throw JSONRPCError(RPC_INTERNAL_ERROR, "No timer handler registered for RPC"); + LOCK(g_deadline_timers_mutex); deadlineTimers.erase(name); LogPrint(BCLog::RPC, "queue run of timer %s in %i seconds (using %s)\n", name, nSeconds, timerInterface->Name()); deadlineTimers.emplace(name, std::unique_ptr(timerInterface->NewTimer(func, nSeconds*1000))); diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 703da04873..752098cbf5 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -1927,6 +1927,9 @@ static UniValue walletpassphrase(const JSONRPCRequest& request) CWallet* const pwallet = wallet.get(); int64_t nSleepTime; + int64_t relock_time; + // Prevent concurrent calls to walletpassphrase with the same wallet. + LOCK(pwallet->m_unlock_mutex); { LOCK(pwallet->cs_wallet); @@ -1975,7 +1978,7 @@ static UniValue walletpassphrase(const JSONRPCRequest& request) pwallet->TopUpKeyPool(); pwallet->nRelockTime = GetTime() + nSleepTime; - + relock_time = pwallet->nRelockTime; } // rpcRunLater must be called without cs_wallet held otherwise a deadlock // can occur. The deadlock would happen when RPCRunLater removes the @@ -1986,9 +1989,11 @@ static UniValue walletpassphrase(const JSONRPCRequest& request) // wallet before the following callback is called. If a valid shared pointer // is acquired in the callback then the wallet is still loaded. std::weak_ptr weak_wallet = wallet; - pwallet->chain().rpcRunLater(strprintf("lockwallet(%s)", pwallet->GetName()), [weak_wallet] { + pwallet->chain().rpcRunLater(strprintf("lockwallet(%s)", pwallet->GetName()), [weak_wallet, relock_time] { if (auto shared_wallet = weak_wallet.lock()) { LOCK(shared_wallet->cs_wallet); + // Skip if this is not the most recent rpcRunLater callback. + if (shared_wallet->nRelockTime != relock_time) return; shared_wallet->Lock(); shared_wallet->nRelockTime = 0; } diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index b816a2d183..e70c4b6050 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -965,8 +965,10 @@ public: std::vector GetDestValues(const std::string& prefix) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); //! Holds a timestamp at which point the wallet is scheduled (externally) to be relocked. Caller must arrange for actual relocking to occur via Lock(). - int64_t nRelockTime = 0; + int64_t nRelockTime GUARDED_BY(cs_wallet){0}; + // Used to prevent concurrent calls to walletpassphrase RPC. + Mutex m_unlock_mutex; bool Unlock(const SecureString& strWalletPassphrase, bool fForMixingOnly = false, bool accept_no_keys = false); bool ChangeWalletPassphrase(const SecureString& strOldWalletPassphrase, const SecureString& strNewWalletPassphrase); bool EncryptWallet(const SecureString& strWalletPassphrase); From 5ebb66db188f6309fa7bcb2c1f5319b3696e3a7f Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Fri, 15 May 2020 08:58:42 -0400 Subject: [PATCH 2/2] Merge #18781: Add templated GetRandDuration<> 0000ea32656833efa3d2ffd9bab66c88c83334f0 test: Add test for GetRandMillis and GetRandMicros (MarcoFalke) fa0e5b89cf742df56c6c8f49fe9b3c54d2970a66 Add templated GetRandomDuration<> (MarcoFalke) Pull request description: A naive implementation of this template is dangerous, because the call site might accidentally omit the template parameter: ```cpp template D GetRandDur(const D& duration_max) { return D{GetRand(duration_max.count())}; } BOOST_AUTO_TEST_CASE(util_time_GetRandTime) { std::chrono::seconds rand_hour = GetRandDur(std::chrono::hours{1}); // Want seconds to be in range [0..1hour), but always get zero :(((( BOOST_CHECK_EQUAL(rand_hour.count(), 0); } ``` Luckily `std::common_type` is already specialised in the standard lib for `std::chrono::duration` (https://en.cppreference.com/w/cpp/chrono/duration/common_type). And its effect seem to be that the call site must always specify the template argument explicitly. So instead of implementing the function for each duration type by hand, replace it with a templated version that is safe to use. ACKs for top commit: laanwj: Code review ACK 0000ea32656833efa3d2ffd9bab66c88c83334f0 promag: Code review ACK 0000ea32656833efa3d2ffd9bab66c88c83334f0. jonatack: ACK 0000ea3 thanks for the improved documentation. Code review, built, ran `src/test/test_bitcoin -t random_tests -l test_suite` for the new unit tests, `git diff fa05a4c 0000ea3` since previous review: hebasto: ACK 0000ea32656833efa3d2ffd9bab66c88c83334f0 with non-blocking [nit](https://github.com/bitcoin/bitcoin/pull/18781#discussion_r424924671). Tree-SHA512: e89d46e31452be6ea14269ecbbb2cdd9ae83b4412cd14dff7d1084283092722a2f847cb501e8054394e4a3eff852f9c87f6d694fd008b3f7e8458cb5a3068af7 --- src/random.cpp | 16 ++-------------- src/random.h | 16 ++++++++++++++-- src/test/random_tests.cpp | 7 +++++-- 3 files changed, 21 insertions(+), 18 deletions(-) diff --git a/src/random.cpp b/src/random.cpp index 5674b56865..6f57e582d2 100644 --- a/src/random.cpp +++ b/src/random.cpp @@ -14,16 +14,14 @@ #include #endif #include // for LogPrintf() +#include +#include #include // for Mutex #include // for GetTimeMicros() #include #include -#include - -#include - #ifndef WIN32 #include #endif @@ -582,16 +580,6 @@ static void ProcRand(unsigned char* out, int num, RNGLevel level) noexcept } } -std::chrono::microseconds GetRandMicros(std::chrono::microseconds duration_max) noexcept -{ - return std::chrono::microseconds{GetRand(duration_max.count())}; -} - -std::chrono::milliseconds GetRandMillis(std::chrono::milliseconds duration_max) noexcept -{ - return std::chrono::milliseconds{GetRand(duration_max.count())}; -} - void GetRandBytes(unsigned char* buf, int num) noexcept { ProcRand(buf, num, RNGLevel::FAST); } void GetStrongRandBytes(unsigned char* buf, int num) noexcept { ProcRand(buf, num, RNGLevel::SLOW); } void RandAddPeriodic() noexcept { ProcRand(nullptr, 0, RNGLevel::PERIODIC); } diff --git a/src/random.h b/src/random.h index b957b52c12..f1b31aae66 100644 --- a/src/random.h +++ b/src/random.h @@ -70,9 +70,21 @@ * Thread-safe. */ void GetRandBytes(unsigned char* buf, int num) noexcept; +/** Generate a uniform random integer in the range [0..range). Precondition: range > 0 */ uint64_t GetRand(uint64_t nMax) noexcept; -std::chrono::microseconds GetRandMicros(std::chrono::microseconds duration_max) noexcept; -std::chrono::milliseconds GetRandMillis(std::chrono::milliseconds duration_max) noexcept; +/** Generate a uniform random duration in the range [0..max). Precondition: max.count() > 0 */ +template +D GetRandomDuration(typename std::common_type::type max) noexcept +// Having the compiler infer the template argument from the function argument +// is dangerous, because the desired return value generally has a different +// type than the function argument. So std::common_type is used to force the +// call site to specify the type of the return value. +{ + assert(max.count() > 0); + return D{GetRand(max.count())}; +}; +constexpr auto GetRandMicros = GetRandomDuration; +constexpr auto GetRandMillis = GetRandomDuration; int GetRandInt(int nMax) noexcept; uint256 GetRandHash() noexcept; diff --git a/src/test/random_tests.cpp b/src/test/random_tests.cpp index e65603cd76..40a2378fe9 100644 --- a/src/test/random_tests.cpp +++ b/src/test/random_tests.cpp @@ -28,6 +28,8 @@ BOOST_AUTO_TEST_CASE(fastrandom_tests) for (int i = 10; i > 0; --i) { BOOST_CHECK_EQUAL(GetRand(std::numeric_limits::max()), uint64_t{10393729187455219830U}); BOOST_CHECK_EQUAL(GetRandInt(std::numeric_limits::max()), int{769702006}); + BOOST_CHECK_EQUAL(GetRandMicros(std::chrono::hours{1}).count(), 2917185654); + BOOST_CHECK_EQUAL(GetRandMillis(std::chrono::hours{1}).count(), 2144374); } { constexpr SteadySeconds time_point{1s}; @@ -67,6 +69,8 @@ BOOST_AUTO_TEST_CASE(fastrandom_tests) for (int i = 10; i > 0; --i) { BOOST_CHECK(GetRand(std::numeric_limits::max()) != uint64_t{10393729187455219830U}); BOOST_CHECK(GetRandInt(std::numeric_limits::max()) != int{769702006}); + BOOST_CHECK(GetRandMicros(std::chrono::hours{1}) != std::chrono::microseconds{2917185654}); + BOOST_CHECK(GetRandMillis(std::chrono::hours{1}) != std::chrono::milliseconds{2144374}); } { @@ -108,7 +112,7 @@ BOOST_AUTO_TEST_CASE(stdrandom_test) BOOST_CHECK(x >= 3); BOOST_CHECK(x <= 9); - std::vector test{1,2,3,4,5,6,7,8,9,10}; + std::vector test{1, 2, 3, 4, 5, 6, 7, 8, 9, 10}; std::shuffle(test.begin(), test.end(), ctx); for (int j = 1; j <= 10; ++j) { BOOST_CHECK(std::find(test.begin(), test.end(), j) != test.end()); @@ -118,7 +122,6 @@ BOOST_AUTO_TEST_CASE(stdrandom_test) BOOST_CHECK(std::find(test.begin(), test.end(), j) != test.end()); } } - } /** Test that Shuffle reaches every permutation with equal probability. */