netsync: require peer argument in fetchHeaderBlocks
What the developer wrote
Authored by Calvin Kim
netsync: require peer argument in fetchHeaderBlocks
Refactor fetchHeaderBlocks and buildBlockRequest to take an explicit
peer parameter instead of implicitly using sm.syncPeer. This makes the
caller responsible for choosing which peer to fetch from and adds a nil
guard to prevent a panic if the sync peer has been cleared.
What changed, and why it matters
This change is a defensive refactor in btcd's network synchronization code. It makes a block-downloading helper function take the target peer as an explicit argument instead of silently relying on a shared 'sync peer' field. It also adds checks so the function does nothing if that peer is missing, preventing a program crash (panic) in cases where the sync peer has been disconnected or cleared. There is no direct evidence this fixes an actively exploitable remote vulnerability, but it removes a crash path during peer churn.
Treat as a hardening/defensive fix. Review whether other functions in netsync/manager.go still implicitly use sm.syncPeer without nil checks, and consider adding similar explicit-peer or guard patterns if they are called during peer state transitions. No urgent patch deployment is indicated unless the panic has been observed in production.
Security signals we found
nil-pointer dereference crash path removed
defensive guard added for cleared sync peer
refactor reduces reliance on mutable shared state (sm.syncPeer)
peer churn / race condition hardening
Evidence from the diff
The commit modifies netsync/manager.go so that fetchHeaderBlocks and buildBlockRequest accept a *peerpkg.Peer parameter rather than reading sm.syncPeer internally. It adds nil guards: fetchHeaderBlocks returns early with a warning if peer is nil, and buildBlockRequest returns an empty getdata message. Callers in handleBlockMsg and handleHeadersMsg now pass the peer explicitly. The change prevents nil-pointer dereferences when the sync peer is cleared while block requests are being constructed or sent. The test file is updated to pass the sync peer explicitly.
Changed components
netsync/manager.gonetsync/manager_test.goSyncManager.fetchHeaderBlocksSyncManager.buildBlockRequesthandleBlockMsghandleHeadersMsgInspect captured patch +21 / −11
diff --git a/netsync/manager.go b/netsync/manager.go
index d8fd1fb..47e60e9 100644
--- a/netsync/manager.go
+++ b/netsync/manager.go
@@ -962,7 +962,7 @@ func (sm *SyncManager) handleBlockMsg(bmsg *blockMsg) {
if !isCheckpointBlock {
if sm.startHeader != nil &&
len(state.requestedBlocks) < minInFlightBlocks {
- sm.fetchHeaderBlocks()
+ sm.fetchHeaderBlocks(sm.syncPeer)
}
return
}
@@ -1003,12 +1003,17 @@ func (sm *SyncManager) handleBlockMsg(bmsg *blockMsg) {
}
}
-// fetchHeaderBlocks creates and sends a request to the syncPeer for the next
+// fetchHeaderBlocks creates and sends a request to the given peer for the next
// list of blocks to be downloaded based on the current list of headers.
-func (sm *SyncManager) fetchHeaderBlocks() {
- gdmsg := sm.buildBlockRequest()
+func (sm *SyncManager) fetchHeaderBlocks(peer *peerpkg.Peer) {
+ if peer == nil {
+ log.Warnf("fetchHeaderBlocks called with a nil peer")
+ return
+ }
+
+ gdmsg := sm.buildBlockRequest(peer)
if len(gdmsg.InvList) > 0 {
- sm.syncPeer.QueueMessage(gdmsg, nil)
+ peer.QueueMessage(gdmsg, nil)
}
}
@@ -1020,7 +1025,12 @@ func (sm *SyncManager) fetchHeaderBlocks() {
// When the best header chain has diverged (e.g. due to a reorg),
// blocks between the fork point and the current height on the new
// chain are different and must also be downloaded.
-func (sm *SyncManager) buildBlockRequest() *wire.MsgGetData {
+func (sm *SyncManager) buildBlockRequest(peer *peerpkg.Peer) *wire.MsgGetData {
+ // Return early if the peer is nil.
+ if peer == nil {
+ return wire.NewMsgGetDataSizeHint(0)
+ }
+
_, bestHeaderHeight := sm.chain.BestHeader()
forkHeight := sm.chain.BestChainHeaderForkHeight()
if bestHeaderHeight < forkHeight {
@@ -1057,15 +1067,15 @@ func (sm *SyncManager) buildBlockRequest() *wire.MsgGetData {
continue
}
- syncPeerState := sm.peerStates[sm.syncPeer]
+ peerState := sm.peerStates[peer]
sm.requestedBlocks[*hash] = struct{}{}
- syncPeerState.requestedBlocks[*hash] = struct{}{}
+ peerState.requestedBlocks[*hash] = struct{}{}
// If we're fetching from a witness enabled peer
// post-fork, then ensure that we receive all the
// witness data in the blocks.
- if sm.syncPeer.IsWitnessEnabled() {
+ if peer.IsWitnessEnabled() {
iv.Type = wire.InvTypeWitnessBlock
}
@@ -1137,7 +1147,7 @@ func (sm *SyncManager) handleHeadersMsg(hmsg *headersMsg) {
log.Infof("downloaded headers to %v(%v) from peer %v "+
"-- now fetching blocks",
bestHeaderHash, bestHeaderHeight, hmsg.peer.String())
- sm.fetchHeaderBlocks()
+ sm.fetchHeaderBlocks(peer)
}
// handleNotFoundMsg handles notfound messages from all peers.
diff --git a/netsync/manager_test.go b/netsync/manager_test.go
index 05c6cac..ddfa3b8 100644
--- a/netsync/manager_test.go
+++ b/netsync/manager_test.go
@@ -389,7 +389,7 @@ func TestBuildBlockRequestSkipsInflightBlocks(t *testing.T) {
syncPeerState.requestedBlocks[*hash] = struct{}{}
}
- gdmsg := sm.buildBlockRequest()
+ gdmsg := sm.buildBlockRequest(syncPeer)
// Collect the hashes from the getdata message.
got := make(map[chainhash.Hash]struct{}, len(gdmsg.InvList))
Why this scored 42/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.