From fa3a4223ec915e7e770fa41e1c755b0d1cccb9d3 Mon Sep 17 00:00:00 2001 From: DanS Date: Mon, 31 Aug 2026 22:08:29 -0500 Subject: [PATCH] audit: mechanical fixes from the general audit (UNTESTED - see below) Six independently-verified defects, each restoring an idiom the surrounding code already uses. Compile-verified only; no runtime testing has been done. Parked on a branch deliberately. net.h PushAddress: tested IsAddressKnown(addr) -- the CNode member holding THIS PEER's own address -- instead of _addr, the address being queued. The filter was therefore constant for the connection's lifetime, and once the peer's own address entered its addrKnown (routine: the remote's AdvertizeLocal reaches us and we file it) every relay path to that peer silently no-opped until the daily addrKnown.reset(). Introduced by 63ad87f69, which rewrote !addrKnown.contains(_addr.GetKey()) into !IsAddressKnown(addr). One token. rpcdump.cpp importprivkey: `params.size() == 4` meant the documented `height` argument was silently dropped whenever the optional 5th (secret_key) argument was supplied, rescanning from genesis instead -- ~3.26M blocks holding cs_main and cs_wallet. Every sibling RPC in the file already uses the `>` form. blockchain.cpp getblockhashes: `if (fActiveOnly) LOCK(cs_main);` was unbraced, and LOCK declares a scoped object, so the lock was constructed and destroyed on that line while the timestamp-index walk ran unsynchronised. Note that merely adding braces does NOT fix it -- the work is in GetTimestampIndex on the next line, which calls blockOnchainActive() per row. The lock now spans that call, taken unconditionally: a conditional lock is the exact shape that produced the bug. blockchain.cpp getblockdeltas / getblockmerkletree: no lock at all while reading mapBlockIndex, chainActive, and (getblockmerkletree) pcoinsTip's mutable anchor cache, which inserts on a miss. Every sibling RPC in the file locks. httpserver.cpp: libevent defaults max_headers_size to EV_SIZE_MAX and buffers the request line and headers BEFORE the -rpcallowip ACL or auth check runs, so a single connection could grow RSS ~1:1 with bytes sent. Capped at 8 KiB. rpc/mining.cpp getblocktemplate: LEAVE_CRITICAL_SECTION(cs_main) is followed by an unguarded CreateNewBlockWithKey. The enclosing LOCK(cs_main) is a scoped CMutexLock whose owns_lock is still true, so any wallet/BDB fault (disk full, EMFILE, corrupt wallet.dat) made its destructor unlock an already-unlocked mutex during unwinding -> BOOST_VERIFY -> SIGABRT. Asserts cannot be compiled out (main.cpp #errors on NDEBUG). The daemon aborted instead of returning the actionable error, and because the abort happened inside unwinding, nothing was logged. Now re-enters cs_main before rethrowing. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FU87LdsJZiZkfq1eXubpeo --- src/httpserver.cpp | 8 ++++++++ src/net.h | 6 +++++- src/rpc/blockchain.cpp | 24 ++++++++++++++++++++---- src/rpc/mining.cpp | 20 ++++++++++++++++++-- src/wallet/rpcdump.cpp | 5 ++++- 5 files changed, 55 insertions(+), 8 deletions(-) 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();