Skip to content

Commit 185df84

Browse files
authored
fix(discord): nil-conn deref in listen crashes process on network failure — snapshot conn under lock, no lockless g.conn.ReadJSON (#102)
The gateway read loop called g.conn.ReadJSON directly with no lock and no nil-guard, while the reconnect/close path nils g.conn under g.mu. A network failure landing mid-iteration crashed the process with a SIGSEGV inside gorilla/websocket's NextReader on a nil *Conn. listen now snapshots g.conn under g.mu once per iteration and reads via the local snapshot; a nil snapshot returns cleanly, which reconnectLoop already treats as a signal to reconnect. This also removes the data race on g.conn between listen and the reconnect/close path.
1 parent 5158dad commit 185df84

2 files changed

Lines changed: 111 additions & 1 deletion

File tree

sdk/integrations/discord/transport.go

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -186,8 +186,16 @@ func (g *GatewayClient) listen(ctx context.Context) <-chan GatewayEvent {
186186
default:
187187
}
188188

189+
g.mu.Lock()
190+
c := g.conn
191+
g.mu.Unlock()
192+
if c == nil {
193+
g.log.Warn("discord: listen: connection is nil, ending read loop")
194+
return
195+
}
196+
189197
var event GatewayEvent
190-
if err := g.conn.ReadJSON(&event); err != nil {
198+
if err := c.ReadJSON(&event); err != nil {
191199
code := extractCloseCode(err)
192200
if code != 0 {
193201
if isFatalCloseCode(code) {
Lines changed: 102 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,102 @@
1+
package discord
2+
3+
import (
4+
"context"
5+
"io"
6+
"log/slog"
7+
"net/http"
8+
"net/http/httptest"
9+
"strings"
10+
"sync"
11+
"testing"
12+
"time"
13+
14+
"github.com/gorilla/websocket"
15+
)
16+
17+
func discardLogger() *slog.Logger {
18+
return slog.New(slog.NewTextHandler(io.Discard, nil))
19+
}
20+
21+
// TestListenNilConnNoPanic verifies that listen() does not dereference a nil
22+
// g.conn: it must return (closing its output channel) instead of panicking.
23+
func TestListenNilConnNoPanic(t *testing.T) {
24+
g := &GatewayClient{
25+
stopCh: make(chan struct{}),
26+
log: discardLogger(),
27+
}
28+
29+
ctx, cancel := context.WithCancel(context.Background())
30+
defer cancel()
31+
32+
out := g.listen(ctx)
33+
34+
select {
35+
case _, ok := <-out:
36+
if ok {
37+
t.Fatal("expected output channel to close without emitting an event")
38+
}
39+
case <-time.After(2 * time.Second):
40+
t.Fatal("listen did not return promptly on a nil connection")
41+
}
42+
}
43+
44+
// TestListenConnRace exercises listen() reading from a snapshot of g.conn
45+
// while a concurrent goroutine nils g.conn out from under it (the
46+
// reconnect/close path), the way reconnectLoop and Close do in production.
47+
// Run with -race to confirm there is no data race on g.conn.
48+
func TestListenConnRace(t *testing.T) {
49+
upgrader := websocket.Upgrader{}
50+
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
51+
conn, err := upgrader.Upgrade(w, r, nil)
52+
if err != nil {
53+
return
54+
}
55+
defer func() { _ = conn.Close() }()
56+
for {
57+
if _, _, err := conn.ReadMessage(); err != nil {
58+
return
59+
}
60+
}
61+
}))
62+
defer srv.Close()
63+
64+
wsURL := "ws" + strings.TrimPrefix(srv.URL, "http")
65+
66+
g := &GatewayClient{
67+
stopCh: make(chan struct{}),
68+
log: discardLogger(),
69+
}
70+
71+
ctx, cancel := context.WithCancel(context.Background())
72+
defer cancel()
73+
74+
for i := 0; i < 20; i++ {
75+
conn, _, err := websocket.DefaultDialer.Dial(wsURL, nil)
76+
if err != nil {
77+
t.Fatalf("dial: %v", err)
78+
}
79+
g.mu.Lock()
80+
g.conn = conn
81+
g.mu.Unlock()
82+
83+
out := g.listen(ctx)
84+
85+
var wg sync.WaitGroup
86+
wg.Add(1)
87+
go func() {
88+
defer wg.Done()
89+
g.mu.Lock()
90+
c := g.conn
91+
g.conn = nil
92+
g.mu.Unlock()
93+
if c != nil {
94+
_ = c.Close()
95+
}
96+
}()
97+
98+
for range out {
99+
}
100+
wg.Wait()
101+
}
102+
}

0 commit comments

Comments
 (0)