From faf6386e915e40ab59bea363f06bb5d057f5c216 Mon Sep 17 00:00:00 2001 From: Aleks Petrov Date: Tue, 14 Jul 2026 17:54:56 +0200 Subject: [PATCH] fix(discord): snapshot g.conn under lock in listen to stop nil-deref crash (GH-101) 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. --- sdk/integrations/discord/transport.go | 10 +- sdk/integrations/discord/transport_test.go | 102 +++++++++++++++++++++ 2 files changed, 111 insertions(+), 1 deletion(-) create mode 100644 sdk/integrations/discord/transport_test.go diff --git a/sdk/integrations/discord/transport.go b/sdk/integrations/discord/transport.go index 81851fa..418dd30 100644 --- a/sdk/integrations/discord/transport.go +++ b/sdk/integrations/discord/transport.go @@ -186,8 +186,16 @@ func (g *GatewayClient) listen(ctx context.Context) <-chan GatewayEvent { default: } + g.mu.Lock() + c := g.conn + g.mu.Unlock() + if c == nil { + g.log.Warn("discord: listen: connection is nil, ending read loop") + return + } + var event GatewayEvent - if err := g.conn.ReadJSON(&event); err != nil { + if err := c.ReadJSON(&event); err != nil { code := extractCloseCode(err) if code != 0 { if isFatalCloseCode(code) { diff --git a/sdk/integrations/discord/transport_test.go b/sdk/integrations/discord/transport_test.go new file mode 100644 index 0000000..1e6caef --- /dev/null +++ b/sdk/integrations/discord/transport_test.go @@ -0,0 +1,102 @@ +package discord + +import ( + "context" + "io" + "log/slog" + "net/http" + "net/http/httptest" + "strings" + "sync" + "testing" + "time" + + "github.com/gorilla/websocket" +) + +func discardLogger() *slog.Logger { + return slog.New(slog.NewTextHandler(io.Discard, nil)) +} + +// TestListenNilConnNoPanic verifies that listen() does not dereference a nil +// g.conn: it must return (closing its output channel) instead of panicking. +func TestListenNilConnNoPanic(t *testing.T) { + g := &GatewayClient{ + stopCh: make(chan struct{}), + log: discardLogger(), + } + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + out := g.listen(ctx) + + select { + case _, ok := <-out: + if ok { + t.Fatal("expected output channel to close without emitting an event") + } + case <-time.After(2 * time.Second): + t.Fatal("listen did not return promptly on a nil connection") + } +} + +// TestListenConnRace exercises listen() reading from a snapshot of g.conn +// while a concurrent goroutine nils g.conn out from under it (the +// reconnect/close path), the way reconnectLoop and Close do in production. +// Run with -race to confirm there is no data race on g.conn. +func TestListenConnRace(t *testing.T) { + upgrader := websocket.Upgrader{} + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + conn, err := upgrader.Upgrade(w, r, nil) + if err != nil { + return + } + defer func() { _ = conn.Close() }() + for { + if _, _, err := conn.ReadMessage(); err != nil { + return + } + } + })) + defer srv.Close() + + wsURL := "ws" + strings.TrimPrefix(srv.URL, "http") + + g := &GatewayClient{ + stopCh: make(chan struct{}), + log: discardLogger(), + } + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + for i := 0; i < 20; i++ { + conn, _, err := websocket.DefaultDialer.Dial(wsURL, nil) + if err != nil { + t.Fatalf("dial: %v", err) + } + g.mu.Lock() + g.conn = conn + g.mu.Unlock() + + out := g.listen(ctx) + + var wg sync.WaitGroup + wg.Add(1) + go func() { + defer wg.Done() + g.mu.Lock() + c := g.conn + g.conn = nil + g.mu.Unlock() + if c != nil { + _ = c.Close() + } + }() + + for range out { + } + wg.Wait() + } +}