From 5eaaa83ac1f5eb525f93e2808fafd73f5ed97013 Mon Sep 17 00:00:00 2001 From: "Wladimir J. van der Laan" Date: Thu, 13 Oct 2016 16:19:20 +0200 Subject: [PATCH] Kill insecure_random and associated global state There are only a few uses of `insecure_random` outside the tests. This PR replaces uses of insecure_random (and its accompanying global state) in the core code with an FastRandomContext that is automatically seeded on creation. This is meant to be used for inner loops. The FastRandomContext can be in the outer scope, or the class itself, then rand32() is used inside the loop. Useful e.g. for pushing addresses in CNode or the fee rounding, or randomization for coin selection. As a context is created per purpose, thus it gets rid of cross-thread unprotected shared usage of a single set of globals, this should also get rid of the potential race conditions. - I'd say TxMempool::check is not called enough to warrant using a special fast random context, this is switched to GetRand() (open for discussion...) - The use of `insecure_rand` in ConnectThroughProxy has been replaced by an atomic integer counter. The only goal here is to have a different credentials pair for each connection to go on a different Tor circuit, it does not need to be random nor unpredictable. - To avoid having a FastRandomContext on every CNode, the context is passed into PushAddress as appropriate. There remains an insecure_random for test usage in `test_random.h`. --- src/addrman.cpp | 8 ++++---- src/addrman.h | 3 +++ src/main.cpp | 15 +++++++++------ src/net.cpp | 3 ++- src/net.h | 4 ++-- src/netbase.cpp | 4 ++-- src/policy/fees.cpp | 2 +- src/policy/fees.h | 2 ++ src/random.cpp | 11 +++++------ src/random.h | 33 +++++++++++++++----------------- src/test/addrman_tests.cpp | 2 +- src/test/coins_tests.cpp | 2 +- src/test/crypto_tests.cpp | 2 +- src/test/merkle_tests.cpp | 2 +- src/test/net_tests.cpp | 2 +- src/test/pmt_tests.cpp | 2 +- src/test/prevector_tests.cpp | 14 ++++++-------- src/test/scheduler_tests.cpp | 4 +--- src/test/sighash_tests.cpp | 2 +- src/test/skiplist_tests.cpp | 2 +- src/test/test_bitcoin.cpp | 1 + src/test/test_random.h | 23 ++++++++++++++++++++++ src/test/util_tests.cpp | 2 +- src/test/versionbits_tests.cpp | 2 +- src/txmempool.cpp | 2 +- src/txmempool.h | 1 + src/wallet/test/crypto_tests.cpp | 2 +- src/wallet/wallet.cpp | 4 ++-- 28 files changed, 91 insertions(+), 65 deletions(-) create mode 100644 src/test/test_random.h diff --git a/src/addrman.cpp b/src/addrman.cpp index bfb8e9457..201652321 100644 --- a/src/addrman.cpp +++ b/src/addrman.cpp @@ -358,8 +358,8 @@ CAddrInfo CAddrMan::Select_(bool newOnly) int nKBucket = RandomInt(ADDRMAN_TRIED_BUCKET_COUNT); int nKBucketPos = RandomInt(ADDRMAN_BUCKET_SIZE); while (vvTried[nKBucket][nKBucketPos] == -1) { - nKBucket = (nKBucket + insecure_rand()) % ADDRMAN_TRIED_BUCKET_COUNT; - nKBucketPos = (nKBucketPos + insecure_rand()) % ADDRMAN_BUCKET_SIZE; + nKBucket = (nKBucket + insecure_rand.rand32()) % ADDRMAN_TRIED_BUCKET_COUNT; + nKBucketPos = (nKBucketPos + insecure_rand.rand32()) % ADDRMAN_BUCKET_SIZE; } int nId = vvTried[nKBucket][nKBucketPos]; assert(mapInfo.count(nId) == 1); @@ -375,8 +375,8 @@ CAddrInfo CAddrMan::Select_(bool newOnly) int nUBucket = RandomInt(ADDRMAN_NEW_BUCKET_COUNT); int nUBucketPos = RandomInt(ADDRMAN_BUCKET_SIZE); while (vvNew[nUBucket][nUBucketPos] == -1) { - nUBucket = (nUBucket + insecure_rand()) % ADDRMAN_NEW_BUCKET_COUNT; - nUBucketPos = (nUBucketPos + insecure_rand()) % ADDRMAN_BUCKET_SIZE; + nUBucket = (nUBucket + insecure_rand.rand32()) % ADDRMAN_NEW_BUCKET_COUNT; + nUBucketPos = (nUBucketPos + insecure_rand.rand32()) % ADDRMAN_BUCKET_SIZE; } int nId = vvNew[nUBucket][nUBucketPos]; assert(mapInfo.count(nId) == 1); diff --git a/src/addrman.h b/src/addrman.h index 9bab39049..e9e137c97 100644 --- a/src/addrman.h +++ b/src/addrman.h @@ -211,6 +211,9 @@ protected: //! secret key to randomize bucket select with uint256 nKey; + //! Source of random numbers for randomization in inner loops + FastRandomContext insecure_rand; + //! Find an entry. CAddrInfo* Find(const CNetAddr& addr, int *pnId = NULL); diff --git a/src/main.cpp b/src/main.cpp index c92a38be9..ab0dc0752 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -4758,6 +4758,7 @@ static void RelayAddress(const CAddress& addr, bool fReachable, CConnman& connma uint64_t hashAddr = addr.GetHash(); std::multimap mapMix; const CSipHasher hasher = connman.GetDeterministicRandomizer(RANDOMIZER_ID_ADDRESS_RELAY).Write(hashAddr << 32).Write((GetTime() + hashAddr) / (24*60*60)); + FastRandomContext insecure_rand; auto sortfunc = [&mapMix, &hasher](CNode* pnode) { if (pnode->nVersion >= CADDR_TIME_VERSION) { @@ -4766,9 +4767,9 @@ static void RelayAddress(const CAddress& addr, bool fReachable, CConnman& connma } }; - auto pushfunc = [&addr, &mapMix, &nRelayNodes] { + auto pushfunc = [&addr, &mapMix, &nRelayNodes, &insecure_rand] { for (auto mi = mapMix.begin(); mi != mapMix.end() && nRelayNodes-- > 0; ++mi) - mi->second->PushAddress(addr); + mi->second->PushAddress(addr, insecure_rand); }; connman.ForEachNodeThen(std::move(sortfunc), std::move(pushfunc)); @@ -5078,14 +5079,15 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv, if (fListen && !IsInitialBlockDownload()) { CAddress addr = GetLocalAddress(&pfrom->addr, pfrom->GetLocalServices()); + FastRandomContext insecure_rand; if (addr.IsRoutable()) { LogPrint("net", "ProcessMessages: advertising address %s\n", addr.ToString()); - pfrom->PushAddress(addr); + pfrom->PushAddress(addr, insecure_rand); } else if (IsPeerAddrLocalGood(pfrom)) { addr.SetIP(pfrom->addrLocal); LogPrint("net", "ProcessMessages: advertising address %s\n", addr.ToString()); - pfrom->PushAddress(addr); + pfrom->PushAddress(addr, insecure_rand); } } @@ -6008,8 +6010,9 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv, pfrom->vAddrToSend.clear(); vector vAddr = connman.GetAddresses(); + FastRandomContext insecure_rand; BOOST_FOREACH(const CAddress &addr, vAddr) - pfrom->PushAddress(addr); + pfrom->PushAddress(addr, insecure_rand); } @@ -6842,7 +6845,7 @@ bool SendMessages(CNode* pto, CConnman& connman) // until scheduled broadcast, then move the broadcast to within MAX_FEEFILTER_CHANGE_DELAY. else if (timeNow + MAX_FEEFILTER_CHANGE_DELAY * 1000000 < pto->nextSendTimeFeeFilter && (currentFilter < 3 * pto->lastSentFeeFilter / 4 || currentFilter > 4 * pto->lastSentFeeFilter / 3)) { - pto->nextSendTimeFeeFilter = timeNow + (insecure_rand() % MAX_FEEFILTER_CHANGE_DELAY) * 1000000; + pto->nextSendTimeFeeFilter = timeNow + GetRandInt(MAX_FEEFILTER_CHANGE_DELAY) * 1000000; } } } diff --git a/src/net.cpp b/src/net.cpp index 19dd04099..643dd806d 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -187,7 +187,8 @@ void AdvertiseLocal(CNode *pnode) if (addrLocal.IsRoutable()) { LogPrint("net", "AdvertiseLocal: advertising address %s\n", addrLocal.ToString()); - pnode->PushAddress(addrLocal); + FastRandomContext insecure_rand; + pnode->PushAddress(addrLocal, insecure_rand); } } } diff --git a/src/net.h b/src/net.h index 67f0abe4b..3417cb2ab 100644 --- a/src/net.h +++ b/src/net.h @@ -735,14 +735,14 @@ public: addrKnown.insert(_addr.GetKey()); } - void PushAddress(const CAddress& _addr) + void PushAddress(const CAddress& _addr, FastRandomContext &insecure_rand) { // Known checking here is only to save space from duplicates. // SendMessages will filter it again for knowns that were added // after addresses were pushed. if (_addr.IsValid() && !addrKnown.contains(_addr.GetKey())) { if (vAddrToSend.size() >= MAX_ADDR_TO_SEND) { - vAddrToSend[insecure_rand() % vAddrToSend.size()] = _addr; + vAddrToSend[insecure_rand.rand32() % vAddrToSend.size()] = _addr; } else { vAddrToSend.push_back(_addr); } diff --git a/src/netbase.cpp b/src/netbase.cpp index 7d7f1b678..9fe34108f 100644 --- a/src/netbase.cpp +++ b/src/netbase.cpp @@ -596,8 +596,8 @@ static bool ConnectThroughProxy(const proxyType &proxy, const std::string& strDe // do socks negotiation if (proxy.randomize_credentials) { ProxyCredentials random_auth; - random_auth.username = strprintf("%i", insecure_rand()); - random_auth.password = strprintf("%i", insecure_rand()); + static std::atomic_int counter; + random_auth.username = random_auth.password = strprintf("%i", counter++); if (!Socks5(strDest, (unsigned short)port, &random_auth, hSocket)) return false; } else { diff --git a/src/policy/fees.cpp b/src/policy/fees.cpp index 7b0e8b7d0..c07cd2eff 100644 --- a/src/policy/fees.cpp +++ b/src/policy/fees.cpp @@ -594,7 +594,7 @@ FeeFilterRounder::FeeFilterRounder(const CFeeRate& minIncrementalFee) CAmount FeeFilterRounder::round(CAmount currentMinFee) { std::set::iterator it = feeset.lower_bound(currentMinFee); - if ((it != feeset.begin() && insecure_rand() % 3 != 0) || it == feeset.end()) { + if ((it != feeset.begin() && insecure_rand.rand32() % 3 != 0) || it == feeset.end()) { it--; } return *it; diff --git a/src/policy/fees.h b/src/policy/fees.h index 463f62f71..2c1ac3b93 100644 --- a/src/policy/fees.h +++ b/src/policy/fees.h @@ -7,6 +7,7 @@ #include "amount.h" #include "uint256.h" +#include "random.h" #include #include @@ -298,5 +299,6 @@ public: private: std::set feeset; + FastRandomContext insecure_rand; }; #endif /*BITCOIN_POLICYESTIMATOR_H */ diff --git a/src/random.cpp b/src/random.cpp index d9a8cc145..aa027e49c 100644 --- a/src/random.cpp +++ b/src/random.cpp @@ -178,22 +178,21 @@ uint256 GetRandHash() return hash; } -uint32_t insecure_rand_Rz = 11; -uint32_t insecure_rand_Rw = 11; -void seed_insecure_rand(bool fDeterministic) +FastRandomContext::FastRandomContext(bool fDeterministic) { // The seed values have some unlikely fixed points which we avoid. if (fDeterministic) { - insecure_rand_Rz = insecure_rand_Rw = 11; + Rz = Rw = 11; } else { uint32_t tmp; do { GetRandBytes((unsigned char*)&tmp, 4); } while (tmp == 0 || tmp == 0x9068ffffU); - insecure_rand_Rz = tmp; + Rz = tmp; do { GetRandBytes((unsigned char*)&tmp, 4); } while (tmp == 0 || tmp == 0x464fffffU); - insecure_rand_Rw = tmp; + Rw = tmp; } } + diff --git a/src/random.h b/src/random.h index 31b80bd56..e97d2d1fb 100644 --- a/src/random.h +++ b/src/random.h @@ -28,25 +28,22 @@ uint256 GetRandHash(); void GetStrongRandBytes(unsigned char* buf, int num); /** - * Seed insecure_rand using the random pool. - * @param Deterministic Use a deterministic seed + * Fast randomness source. This is seeded once with secure random data, but + * is completely deterministic and insecure after that. + * This class is not thread-safe. */ -void seed_insecure_rand(bool fDeterministic = false); +class FastRandomContext { +public: + explicit FastRandomContext(bool fDeterministic=false); -/** - * MWC RNG of George Marsaglia - * This is intended to be fast. It has a period of 2^59.3, though the - * least significant 16 bits only have a period of about 2^30.1. - * - * @return random value - */ -extern uint32_t insecure_rand_Rz; -extern uint32_t insecure_rand_Rw; -static inline uint32_t insecure_rand(void) -{ - insecure_rand_Rz = 36969 * (insecure_rand_Rz & 65535) + (insecure_rand_Rz >> 16); - insecure_rand_Rw = 18000 * (insecure_rand_Rw & 65535) + (insecure_rand_Rw >> 16); - return (insecure_rand_Rw << 16) + insecure_rand_Rz; -} + uint32_t rand32() { + Rz = 36969 * (Rz & 65535) + (Rz >> 16); + Rw = 18000 * (Rw & 65535) + (Rw >> 16); + return (Rw << 16) + Rz; + } + + uint32_t Rz; + uint32_t Rw; +}; #endif // BITCOIN_RANDOM_H diff --git a/src/test/addrman_tests.cpp b/src/test/addrman_tests.cpp index 5ccaeb57b..adff09f75 100644 --- a/src/test/addrman_tests.cpp +++ b/src/test/addrman_tests.cpp @@ -26,7 +26,7 @@ public: void MakeDeterministic() { nKey.SetNull(); - seed_insecure_rand(true); + insecure_rand = FastRandomContext(true); } int RandomInt(int nMax) diff --git a/src/test/coins_tests.cpp b/src/test/coins_tests.cpp index e69232655..b48768613 100644 --- a/src/test/coins_tests.cpp +++ b/src/test/coins_tests.cpp @@ -3,7 +3,7 @@ // file COPYING or http://www.opensource.org/licenses/mit-license.php. #include "coins.h" -#include "random.h" +#include "test_random.h" #include "script/standard.h" #include "uint256.h" #include "utilstrencodings.h" diff --git a/src/test/crypto_tests.cpp b/src/test/crypto_tests.cpp index ff0adae1d..c7b4fb240 100644 --- a/src/test/crypto_tests.cpp +++ b/src/test/crypto_tests.cpp @@ -9,7 +9,7 @@ #include "crypto/sha512.h" #include "crypto/hmac_sha256.h" #include "crypto/hmac_sha512.h" -#include "random.h" +#include "test_random.h" #include "utilstrencodings.h" #include "test/test_bitcoin.h" diff --git a/src/test/merkle_tests.cpp b/src/test/merkle_tests.cpp index b40ab848d..66ca381ea 100644 --- a/src/test/merkle_tests.cpp +++ b/src/test/merkle_tests.cpp @@ -4,7 +4,7 @@ #include "consensus/merkle.h" #include "test/test_bitcoin.h" -#include "random.h" +#include "test_random.h" #include diff --git a/src/test/net_tests.cpp b/src/test/net_tests.cpp index 4a7d6e778..f4b576869 100644 --- a/src/test/net_tests.cpp +++ b/src/test/net_tests.cpp @@ -23,7 +23,7 @@ public: void MakeDeterministic() { nKey.SetNull(); - seed_insecure_rand(true); + insecure_rand = FastRandomContext(true); } }; diff --git a/src/test/pmt_tests.cpp b/src/test/pmt_tests.cpp index e9c869174..b7f83d38f 100644 --- a/src/test/pmt_tests.cpp +++ b/src/test/pmt_tests.cpp @@ -9,7 +9,7 @@ #include "uint256.h" #include "arith_uint256.h" #include "version.h" -#include "random.h" +#include "test_random.h" #include "test/test_bitcoin.h" #include diff --git a/src/test/prevector_tests.cpp b/src/test/prevector_tests.cpp index b8c45ca56..6cad02e73 100644 --- a/src/test/prevector_tests.cpp +++ b/src/test/prevector_tests.cpp @@ -4,7 +4,7 @@ #include #include "prevector.h" -#include "random.h" +#include "test_random.h" #include "serialize.h" #include "streams.h" @@ -27,8 +27,7 @@ class prevector_tester { typedef typename pretype::size_type Size; bool passed = true; - uint32_t insecure_rand_Rz_cache; - uint32_t insecure_rand_Rw_cache; + FastRandomContext rand_cache; template @@ -171,15 +170,14 @@ public: test(); } ~prevector_tester() { - BOOST_CHECK_MESSAGE(passed, "insecure_rand_Rz: " - << insecure_rand_Rz_cache + BOOST_CHECK_MESSAGE(passed, "insecure_rand_Rz: " + << rand_cache.Rz << ", insecure_rand_Rw: " - << insecure_rand_Rw_cache); + << rand_cache.Rw); } prevector_tester() { seed_insecure_rand(); - insecure_rand_Rz_cache = insecure_rand_Rz; - insecure_rand_Rw_cache = insecure_rand_Rw; + rand_cache = insecure_rand_ctx; } }; diff --git a/src/test/scheduler_tests.cpp b/src/test/scheduler_tests.cpp index aa12dfbd5..891ecf501 100644 --- a/src/test/scheduler_tests.cpp +++ b/src/test/scheduler_tests.cpp @@ -42,8 +42,6 @@ static void MicroSleep(uint64_t n) BOOST_AUTO_TEST_CASE(manythreads) { - seed_insecure_rand(false); - // Stress test: hundreds of microsecond-scheduled tasks, // serviced by 10 threads. // @@ -58,7 +56,7 @@ BOOST_AUTO_TEST_CASE(manythreads) boost::mutex counterMutex[10]; int counter[10] = { 0 }; - boost::random::mt19937 rng(insecure_rand()); + boost::random::mt19937 rng(42); boost::random::uniform_int_distribution<> zeroToNine(0, 9); boost::random::uniform_int_distribution<> randomMsec(-11, 1000); boost::random::uniform_int_distribution<> randomDelta(-1000, 1000); diff --git a/src/test/sighash_tests.cpp b/src/test/sighash_tests.cpp index 4a48347b7..0b1050d02 100644 --- a/src/test/sighash_tests.cpp +++ b/src/test/sighash_tests.cpp @@ -6,7 +6,7 @@ #include "data/sighash.json.h" #include "hash.h" #include "main.h" // For CheckTransaction -#include "random.h" +#include "test_random.h" #include "script/interpreter.h" #include "script/script.h" #include "serialize.h" diff --git a/src/test/skiplist_tests.cpp b/src/test/skiplist_tests.cpp index f14b902fe..b19f8fbff 100644 --- a/src/test/skiplist_tests.cpp +++ b/src/test/skiplist_tests.cpp @@ -3,7 +3,7 @@ // file COPYING or http://www.opensource.org/licenses/mit-license.php. #include "chain.h" -#include "random.h" +#include "test_random.h" #include "util.h" #include "test/test_bitcoin.h" diff --git a/src/test/test_bitcoin.cpp b/src/test/test_bitcoin.cpp index 02843d852..27ea837fb 100644 --- a/src/test/test_bitcoin.cpp +++ b/src/test/test_bitcoin.cpp @@ -27,6 +27,7 @@ #include std::unique_ptr g_connman; +FastRandomContext insecure_rand_ctx(true); extern bool fPrintToConsole; extern void noui_connect(); diff --git a/src/test/test_random.h b/src/test/test_random.h new file mode 100644 index 000000000..e61b92b7b --- /dev/null +++ b/src/test/test_random.h @@ -0,0 +1,23 @@ +// Copyright (c) 2009-2010 Satoshi Nakamoto +// Copyright (c) 2009-2014 The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. + +#ifndef BITCOIN_TEST_RANDOM_H +#define BITCOIN_TEST_RANDOM_H + +#include "random.h" + +extern FastRandomContext insecure_rand_ctx; + +static inline void seed_insecure_rand(bool fDeterministic = false) +{ + insecure_rand_ctx = FastRandomContext(fDeterministic); +} + +static inline uint32_t insecure_rand(void) +{ + return insecure_rand_ctx.rand32(); +} + +#endif diff --git a/src/test/util_tests.cpp b/src/test/util_tests.cpp index efd449874..0f1c7ab22 100644 --- a/src/test/util_tests.cpp +++ b/src/test/util_tests.cpp @@ -6,7 +6,7 @@ #include "clientversion.h" #include "primitives/transaction.h" -#include "random.h" +#include "test_random.h" #include "sync.h" #include "utilstrencodings.h" #include "utilmoneystr.h" diff --git a/src/test/versionbits_tests.cpp b/src/test/versionbits_tests.cpp index 1f86a06a3..ffc0ff6f8 100644 --- a/src/test/versionbits_tests.cpp +++ b/src/test/versionbits_tests.cpp @@ -3,7 +3,7 @@ // file COPYING or http://www.opensource.org/licenses/mit-license.php. #include "chain.h" -#include "random.h" +#include "test_random.h" #include "versionbits.h" #include "test/test_bitcoin.h" #include "chainparams.h" diff --git a/src/txmempool.cpp b/src/txmempool.cpp index 15fa6fbca..0f1c166ab 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -647,7 +647,7 @@ void CTxMemPool::check(const CCoinsViewCache *pcoins) const if (nCheckFrequency == 0) return; - if (insecure_rand() >= nCheckFrequency) + if (GetRand(std::numeric_limits::max()) >= nCheckFrequency) return; LogPrint("mempool", "Checking mempool with %u transactions and %u inputs\n", (unsigned int)mapTx.size(), (unsigned int)mapNextTx.size()); diff --git a/src/txmempool.h b/src/txmempool.h index 941644b2b..1763930ba 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -15,6 +15,7 @@ #include "indirectmap.h" #include "primitives/transaction.h" #include "sync.h" +#include "random.h" #undef foreach #include "boost/multi_index_container.hpp" diff --git a/src/wallet/test/crypto_tests.cpp b/src/wallet/test/crypto_tests.cpp index 05387f5f2..c5f55ef5f 100644 --- a/src/wallet/test/crypto_tests.cpp +++ b/src/wallet/test/crypto_tests.cpp @@ -2,7 +2,7 @@ // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. -#include "random.h" +#include "test/test_random.h" #include "utilstrencodings.h" #include "test/test_bitcoin.h" #include "wallet/crypter.h" diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index a1f69dd94..687d84e77 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -1907,7 +1907,7 @@ static void ApproximateBestSubset(vector