Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions consensus/propagation/have_wants.go
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,8 @@ func (blockProp *Reactor) handleHaves(peer p2p.ID, haves *proptypes.HaveParts) {

if p := blockProp.getPeer(peer); p != nil {
for _, index := range hc.GetTrueIndices() {
// avoid blocking if a single peer is backed up. This means that they
// are sending us too many haves
select {
case <-p.ctx.Done():
return
Expand All @@ -98,6 +100,7 @@ func (blockProp *Reactor) handleHaves(peer p2p.ID, haves *proptypes.HaveParts) {
index: uint32(index),
}:
p.RequestsReady()
default:
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Missing required changelog entry for this bug fix PR

Both CLAUDE.md and CONTRIBUTING.md require every PR to include a changelog entry at .changelog/unreleased/{category}/{issue-or-pr-number}-{description}.md. This PR (a bug fix adding a default case to prevent reactor blocking) does not include a changelog entry under .changelog/unreleased/bug-fixes/. The commit message references PR #2813, so a file like .changelog/unreleased/bug-fixes/2813-prevent-handlehaves-reactor-halt.md is expected.

Prompt for agents
The repository rules in CLAUDE.md and CONTRIBUTING.md both require a changelog entry for every PR. This is a bug fix, so a file should be created at .changelog/unreleased/bug-fixes/2813-prevent-handlehaves-reactor-halt.md (or using the PR number 2941). The file should contain a description like:

- [consensus/propagation] \#2813 Add default case to handleHaves select to prevent a malicious peer from blocking the propagation reactor's message-processing goroutine (@contributor)

See existing examples in .changelog/unreleased/ for formatting guidance.
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

}
}
}
Expand Down
63 changes: 63 additions & 0 deletions consensus/propagation/have_wants_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,69 @@ func TestInvalidHavePartHash(t *testing.T) {
assert.Nil(t, p1)
}

// TestHandleHavesDoesNotBlock verifies that handleHaves does not block the
// calling goroutine when the peer's receivedHaves channel is full. This is a
// regression test for a vulnerability where a missing default case in the
// select statement could allow a malicious peer to halt the propagation
// reactor's message-processing goroutine.
func TestHandleHavesDoesNotBlock(t *testing.T) {
p2pCfg := cfg.DefaultP2PConfig()
nodes := 2
reactors, _ := createTestReactors(nodes, p2pCfg, false, "")
r1, r2 := reactors[0], reactors[1]

cleanup, _, sm, pv := state.SetupTestCaseWithPrivVal(t)
t.Cleanup(func() {
cleanup(t)
})
prop, ps, _, metaData := createTestProposal(t, sm, pv, 1, 0, 2, 1000000)
parityBlock, lastLen, err := types.Encode(ps, types.BlockPartSizeBytes)
require.NoError(t, err)
partHashes := extractHashes(ps, parityBlock)
proofs := extractProofs(ps, parityBlock)
cb := &proptypes.CompactBlock{
Proposal: *prop,
LastLen: uint32(lastLen),
Signature: cmtrand.Bytes(64),
BpHash: parityBlock.Hash(),
Blobs: metaData,
PartsHashes: partHashes,
}
cb.SetProofCache(proofs)

added := r1.AddProposal(cb)
require.True(t, added)

p1 := r1.getPeer(r2.self)
require.NotNil(t, p1)

// Fill the peer's receivedHaves channel to capacity.
for i := 0; i < cap(p1.receivedHaves); i++ {
p1.receivedHaves <- request{height: 1, round: 0, index: uint32(i % len(partHashes))}
}
require.Equal(t, cap(p1.receivedHaves), len(p1.receivedHaves))

// Call handleHaves from a goroutine. With the default case, this should
// return almost immediately. Without it, this would block indefinitely.
done := make(chan struct{})
go func() {
defer close(done)
haves := &proptypes.HaveParts{
Height: prop.Height,
Round: prop.Round,
Parts: []proptypes.PartMetaData{{Index: 0, Hash: partHashes[0]}},
}
r1.handleHaves(r2.self, haves)
}()

select {
case <-done:
// handleHaves returned without blocking.
case <-time.After(time.Second):
t.Fatal("handleHaves blocked when receivedHaves channel was full")
}
}

func TestCountRemainingParts(t *testing.T) {
tests := []struct {
name string
Expand Down
Loading