Skip to content

Commit a9959fe

Browse files
mergify[bot]rootulpclaude
authored
fix: guard nil syncer dereference in statesync Reactor.Receive (backport #2883) (#2931)
Closes https://dashboard.hackenproof.com/manager/companies/celestia/celestia/reports/CELESTIA-239 ## Summary - A connected P2P peer could crash any non-state-syncing node (the default operating mode) by sending a single `SnapshotsResponse` with `Chunks > MaxSnapshotChunks`. The `validateMsg` error path in `Reactor.Receive` called `r.syncer.RejectPeer()` before checking whether `r.syncer` was nil, causing an unrecovered panic that terminates the node process. - Added a nil guard with mutex protection around the `RejectPeer` call so oversized snapshot messages are safely rejected when no state sync is active. - Added a regression test that confirms the panic without the fix and passes with it. ## Test plan - [x] New test `TestReactor_Receive_OversizedSnapshotResponse_NilSyncer` verifies no panic when `r.syncer` is nil and an oversized `SnapshotsResponse` is received - [x] Full `./statesync/...` test suite passes - [x] `go vet ./statesync/...` clean 🤖 Generated with [Claude Code](https://claude.com/claude-code) --- <a href="https://app.devin.ai/review/celestiaorg/celestia-core/pull/2883" target="_blank"> <picture> <source media="(prefers-color-scheme: dark)" srcset="https://static.devin.ai/assets/gh-open-in-devin-review-dark.svg?v=1"> <img src="https://static.devin.ai/assets/gh-open-in-devin-review-light.svg?v=1" alt="Open with Devin"> </picture> </a> <hr>This is an automatic backport of pull request #2883 done by [Mergify](https://mergify.com). <!-- devin-review-badge-begin --> --- <a href="https://app.devin.ai/review/celestiaorg/celestia-core/pull/2931" target="_blank"> <picture> <source media="(prefers-color-scheme: dark)" srcset="https://static.devin.ai/assets/gh-open-in-devin-review-dark.svg?v=1"> <img src="https://static.devin.ai/assets/gh-open-in-devin-review-light.svg?v=1" alt="Open with Devin"> </picture> </a> <!-- devin-review-badge-end --> Co-authored-by: Rootul P <rootulp@gmail.com> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent aaf484d commit a9959fe

3 files changed

Lines changed: 64 additions & 1 deletion

File tree

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
- `[statesync]` Guard against nil syncer dereference in Reactor.Receive
2+
when processing oversized SnapshotsResponse messages while no state sync
3+
is in progress.

statesync/reactor.go

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -115,7 +115,11 @@ func (r *Reactor) Receive(e p2p.Envelope) {
115115
err := validateMsg(e.Message, r.cfg.MaxSnapshotChunks)
116116
if err != nil {
117117
if errors.Is(err, ErrExceedsMaxSnapshotChunks) {
118-
r.syncer.RejectPeer(e.Src)
118+
r.mtx.RLock()
119+
if r.syncer != nil {
120+
r.syncer.RejectPeer(e.Src)
121+
}
122+
r.mtx.RUnlock()
119123
}
120124
r.Logger.Error("Invalid message", "peer", e.Src, "msg", e.Message, "err", err)
121125
r.Switch.StopPeerForError(e.Src, err, r.String())

statesync/reactor_test.go

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package statesync
22

33
import (
4+
"net"
45
"testing"
56
"time"
67

@@ -96,6 +97,61 @@ func TestReactor_Receive_ChunkRequest(t *testing.T) {
9697
}
9798
}
9899

100+
// TestReactor_Receive_OversizedSnapshotResponse_NilSyncer verifies that
101+
// receiving a SnapshotsResponse with Chunks > MaxSnapshotChunks does not
102+
// nil-dereference r.syncer when no state sync is in progress.
103+
func TestReactor_Receive_OversizedSnapshotResponse_NilSyncer(t *testing.T) {
104+
cfg := config.DefaultStateSyncConfig()
105+
conn := &proxymocks.AppConnSnapshot{}
106+
107+
peer := &p2pmocks.Peer{}
108+
peer.On("ID").Return(p2p.ID("attacker"))
109+
peer.On("IsRunning").Return(true)
110+
peer.On("Stop").Return(nil)
111+
peer.On("FlushStop").Return()
112+
peer.On("String").Return("peer-attacker")
113+
peer.On("RemoteAddr").Return(&net.TCPAddr{IP: net.IP{127, 0, 0, 1}, Port: 26656})
114+
peer.On("CloseConn").Return(nil)
115+
peer.On("NodeInfo").Return(p2p.DefaultNodeInfo{})
116+
peer.On("SetRemovalFailed").Return()
117+
peer.On("GetRemovalFailed").Return(false)
118+
peer.On("IsPersistent").Return(false)
119+
peer.On("IsOutbound").Return(false)
120+
peer.On("SocketAddr").Return(nil)
121+
peer.On("HasIPChanged").Return(false)
122+
123+
r := NewReactor(*cfg, conn, nil, NopMetrics())
124+
125+
sw := p2p.MakeSwitch(
126+
config.DefaultP2PConfig(),
127+
0,
128+
func(i int, sw *p2p.Switch) *p2p.Switch { return sw },
129+
)
130+
r.SetSwitch(sw)
131+
132+
err := r.Start()
133+
require.NoError(t, err)
134+
t.Cleanup(func() {
135+
if err := r.Stop(); err != nil {
136+
t.Error(err)
137+
}
138+
})
139+
140+
// This should NOT panic even though r.syncer is nil.
141+
require.NotPanics(t, func() {
142+
r.Receive(p2p.Envelope{
143+
ChannelID: SnapshotChannel,
144+
Src: peer,
145+
Message: &ssproto.SnapshotsResponse{
146+
Height: 1,
147+
Format: 1,
148+
Chunks: cfg.MaxSnapshotChunks + 1,
149+
Hash: []byte{1},
150+
},
151+
})
152+
})
153+
}
154+
99155
func TestReactor_Receive_SnapshotsRequest(t *testing.T) {
100156
testcases := map[string]struct {
101157
snapshots []*abci.Snapshot

0 commit comments

Comments
 (0)