DragonX/dragonx
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 3 Wiki Activity

audit: mechanical fixes from the general audit (UNTESTED - see below)

Browse Source 
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) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FU87LdsJZiZkfq1eXubpeo
This commit is contained in:
DanS
2026-08-31 22:08:29 -05:00
parent c1040028e4
commit fa3a4223ec
5 changed files with 55 additions and 8 deletions
Download Patch File Download Diff File Expand all files Collapse all files

8
src/httpserver.cpp
View File

@@ -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)) {

6
src/net.h
View File

@@ -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;

20
src/rpc/blockchain.cpp
View File

@@ -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,12 +606,20 @@ UniValue getblockhashes(const UniValue& params, bool fHelp, const CPubKey& mypk)
std::vector<std::pair<uint256, unsigned int> > 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");
}
}
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;

16
src/rpc/mining.cpp
View File

@@ -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);
// 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
try {
pblocktemplate = CreateNewBlockWithKey();
} catch (...) {
ENTER_CRITICAL_SECTION(cs_main);
throw;
}
#endif
ENTER_CRITICAL_SECTION(cs_main);
if (!pblocktemplate)

5
src/wallet/rpcdump.cpp
View File

@@ -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();