net: move cs_main up in FetchBlock to fix rpc assert crash
What the developer wrote
Authored by Eugene Siegel
net: move cs_main up in FetchBlock to fix rpc assert crash
1. FetchBlock runs in a http worker thread. It acquires a PeerRef,
locks cs_main, then may later call BlockRequested which asserts
that CNodeState exists for the peer.
2. FinalizeNode may run in either the bitcoind or b-net threads. It
locks cs_main, fetches a PeerRef from RemovePeer, fetches a CNodeState,
and later removes it from m_node_states.
Because of the lock placement in FetchBlock, the http worker thread in 1)
can acquire a valid PeerRef and block while the b-net thread in 2) is
cleaning up the peer in FinalizeNode. When the worker thread later acquires
cs_main, it may crash in BlockRequested since no CNodeState exists. Fix
this by acquiring the lock earlier in FetchBlock.
The lock can be replaced with a net-specific lock when the remaining
CNodeState fields are moved to Peer.
What changed, and why it matters
This commit fixes a rare crash in Bitcoin Core's networking code. When a user called a specific RPC command to fetch a block, the program could crash with an assertion failure if a peer disconnected at exactly the wrong moment. The fix moves a lock earlier so the peer's state cannot be cleaned up while the RPC thread is still using it. It is a stability bug, not a code-execution vulnerability, and requires precise timing to trigger.
Treat as a denial-of-service/stability fix worth backporting. Nodes serving RPC should upgrade, especially if exposed to untrusted or automated RPC callers. No immediate incident response is needed beyond normal patch management.
Security signals we found
Assertion failure / crash in RPC path
Race condition between RPC worker thread and peer finalization
Missing synchronization around peer lifecycle lookup
Fix moves lock before resource acquisition
Evidence from the diff
FetchBlock in src/net_processing.cpp previously acquired cs_main after obtaining a PeerRef. This allowed a race: the RPC/http worker thread could fetch a valid PeerRef, then block before taking cs_main, while FinalizeNode (bitcoind or b-net thread) could take cs_main, remove the peer via RemovePeer, and delete its CNodeState. Once the worker thread acquired cs_main and called BlockRequested, the CNodeState was gone and an assertion fired. The patch moves LOCK(cs_main) before GetPeerRef, closing the window. The commit message explicitly frames this as fixing an ‘rpc assert crash’.
Changed components
src/net_processing.cppPeerManagerImpl::FetchBlockRPC block-fetching pathCNodeState / peer lifecycle managementInspect captured patch +6 / −2
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 71cc6ff7..ec46dd0d 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -1977,6 +1977,12 @@ util::Expected<void, std::string> PeerManagerImpl::FetchBlock(NodeId peer_id, co
{
if (m_chainman.m_blockman.LoadingBlocks()) return util::Unexpected{"Loading blocks ..."};
+ // The lock must be taken here before fetching Peer so another thread does
+ // not delete the CNodeState from under the current thread, causing an
+ // assertion failure in BlockRequested. This lock can be replaced with a
+ // net-specific lock when more of CNodeState is moved into Peer.
+ LOCK(cs_main);
+
// Ensure this peer exists and hasn't been disconnected
PeerRef peer = GetPeerRef(peer_id);
if (peer == nullptr) return util::Unexpected{"Peer does not exist"};
@@ -1984,8 +1990,6 @@ util::Expected<void, std::string> PeerManagerImpl::FetchBlock(NodeId peer_id, co
// Ignore pre-segwit peers
if (!CanServeWitnesses(*peer)) return util::Unexpected{"Pre-SegWit peer"};
- LOCK(cs_main);
-
// Forget about all prior requests
RemoveBlockRequest(block_index.GetBlockHash(), std::nullopt);
Why this scored 54/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.