diff --git a/src/httpserver.cpp b/src/httpserver.cpp index 521b7b377..492ba35bf 100644 --- a/src/httpserver.cpp +++ b/src/httpserver.cpp @@ -420,6 +420,10 @@ static void libevent_log_cb(int severity, const char *msg) LogPrint("libevent", "libevent: %s\n", msg); } +/** Cap on the combined size of an HTTP request line + headers. libevent's default is EV_SIZE_MAX, + * i.e. unbounded, and it buffers before any ACL or auth check runs. */ +static const size_t MAX_HEADERS_SIZE = 8192; + bool InitHTTPServer() { struct evhttp* http = 0; @@ -467,6 +471,10 @@ bool InitHTTPServer() evhttp_set_timeout(http, GetArg("-rpcservertimeout", DEFAULT_HTTP_SERVER_TIMEOUT)); evhttp_set_max_body_size(http, MAX_SIZE); + // libevent defaults max_headers_size to EV_SIZE_MAX, so without this a single connection can + // stream an unbounded request line / header block and grow RSS ~1:1 with bytes sent, BEFORE the + // -rpcallowip ACL or auth check runs (both happen after libevent has parsed the request). + evhttp_set_max_headers_size(http, MAX_HEADERS_SIZE); evhttp_set_gencb(http, http_request_cb, NULL); if (!HTTPBindAddresses(http)) { diff --git a/src/net.h b/src/net.h index 2627e071a..eccd36b69 100644 --- a/src/net.h +++ b/src/net.h @@ -607,7 +607,11 @@ public: // 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() && !IsAddressKnown(addr) && addr_format_supported) { + // NOTE: _addr (the address being queued), NOT addr (this peer's own address, net.h ~416). + // Testing the member made the filter constant for the connection's lifetime: once the peer's + // own address entered its addrKnown -- routine, via the remote's AdvertizeLocal -- every + // relay path to it silently no-opped until the daily addrKnown.reset(). + if (_addr.IsValid() && !IsAddressKnown(_addr) && addr_format_supported) { if (vAddrToSend.size() >= MAX_ADDR_TO_SEND) { vAddrToSend[insecure_rand() % vAddrToSend.size()] = _addr; diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp index 1fed2ca05..d59c99499 100644 --- a/src/rpc/blockchain.cpp +++ b/src/rpc/blockchain.cpp @@ -534,6 +534,10 @@ UniValue getblockdeltas(const UniValue& params, bool fHelp, const CPubKey& mypk) if (fHelp || params.size() != 1) throw runtime_error(""); + // Reads mapBlockIndex / chainActive (and, below, pcoinsTip's mutable anchor cache), + // all of which are cs_main-guarded. Every sibling RPC in this file locks; this one did not. + LOCK(cs_main); + std::string strHash = params[0].get_str(); uint256 hash(uint256S(strHash)); @@ -602,11 +606,19 @@ UniValue getblockhashes(const UniValue& params, bool fHelp, const CPubKey& mypk) std::vector > blockHashes; - if (fActiveOnly) + { + // The lock must SPAN GetTimestampIndex: with fActiveOnly it calls blockOnchainActive() for + // every row, which reads mapBlockIndex and chainActive. The previous form was + // if (fActiveOnly) + // LOCK(cs_main); + // and LOCK() declares a scoped object, so as an unbraced substatement it was constructed + // and destroyed on that line -- the walk then ran completely unsynchronised. Taken + // unconditionally here: this RPC is explorer-only and not hot, and a conditional lock is + // exactly the shape that produced the bug. LOCK(cs_main); - - if (!GetTimestampIndex(high, low, fActiveOnly, blockHashes)) { - throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "No information available for block hashes"); + if (!GetTimestampIndex(high, low, fActiveOnly, blockHashes)) { + throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "No information available for block hashes"); + } } UniValue result(UniValue::VARR); @@ -877,6 +889,10 @@ UniValue getblockmerkletree(const UniValue& params, bool fHelp, const CPubKey& m + HelpExampleRpc("getblockmerkletree", "290000") ); + // Reads mapBlockIndex / chainActive (and, below, pcoinsTip's mutable anchor cache), + // all of which are cs_main-guarded. Every sibling RPC in this file locks; this one did not. + LOCK(cs_main); + CBlockIndex* phushblockindex; uint256 blockRoot; SaplingMerkleTree tree; diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp index 0d9f15674..43f8ef83a 100644 --- a/src/rpc/mining.cpp +++ b/src/rpc/mining.cpp @@ -759,9 +759,25 @@ UniValue getblocktemplate(const UniValue& params, bool fHelp, const CPubKey& myp #ifdef ENABLE_WALLET CReserveKey reservekey(pwalletMain); LEAVE_CRITICAL_SECTION(cs_main); - pblocktemplate = CreateNewBlockWithKey(reservekey,pindexPrevNew->GetHeight()+1,HUSH_MAXGPUCOUNT,false); + // MUST re-enter cs_main before letting an exception escape. The enclosing LOCK(cs_main) is + // a scoped CMutexLock whose owns_lock is still true, so if CreateNewBlockWithKey throws + // (any wallet/BDB fault: disk full, EMFILE, a corrupt wallet.dat) its destructor unlocks an + // already-unlocked mutex during unwinding -> BOOST_VERIFY -> SIGABRT. Asserts cannot be + // compiled out here (main.cpp #errors on NDEBUG), so this aborts the daemon instead of + // returning the actionable error, and the abort happens inside unwinding so nothing is logged. + try { + pblocktemplate = CreateNewBlockWithKey(reservekey,pindexPrevNew->GetHeight()+1,HUSH_MAXGPUCOUNT,false); + } catch (...) { + ENTER_CRITICAL_SECTION(cs_main); + throw; + } #else - pblocktemplate = CreateNewBlockWithKey(); + try { + pblocktemplate = CreateNewBlockWithKey(); + } catch (...) { + ENTER_CRITICAL_SECTION(cs_main); + throw; + } #endif ENTER_CRITICAL_SECTION(cs_main); if (!pblocktemplate) diff --git a/src/wallet/rpcdump.cpp b/src/wallet/rpcdump.cpp index 7eb29e42c..f54b0fbc3 100644 --- a/src/wallet/rpcdump.cpp +++ b/src/wallet/rpcdump.cpp @@ -299,7 +299,10 @@ UniValue importprivkey(const UniValue& params, bool fHelp, const CPubKey& mypk) bool fRescan = true; if (params.size() > 2) fRescan = params[2].get_bool(); - if ( fRescan && params.size() == 4 ) + // '> 3', not '== 4': with the optional 5th (secret_key) argument present the equality test + // failed and height silently stayed 0, rescanning from genesis. Every sibling RPC in this file + // already uses the '>' form. + if ( fRescan && params.size() > 3 ) height = params[3].get_int();