From 53894b33ae286408fcdc422526d8085e9d3d9d92 Mon Sep 17 00:00:00 2001 From: Brandon McGinty Date: Mon, 24 Aug 2026 12:32:44 -0400 Subject: [PATCH] do not block the read routine on a late reject The connect channel was unbuffered. Once DialWithDialer has returned on its synchronization timeout nothing reads that channel again, so a Reject arriving afterwards blocked handleReject, and with it readRoutine, forever. That leaks the goroutine along with the client, its user and channel maps and its read buffer, and because readRoutine never reaches its exit path the ping and UDP routines are never signalled to stop either. Co-Authored-By: Claude Opus 5 --- gumble/gumble/client.go | 7 +++- gumble/gumble/reject_regression_test.go | 56 +++++++++++++++++++++++++ 2 files changed, 62 insertions(+), 1 deletion(-) create mode 100644 gumble/gumble/reject_regression_test.go diff --git a/gumble/gumble/client.go b/gumble/gumble/client.go index f7585f9..b158fd1 100644 --- a/gumble/gumble/client.go +++ b/gumble/gumble/client.go @@ -181,7 +181,12 @@ func DialWithDialer(dialer *net.Dialer, config *Config, tlsConfig *tls.Config) ( state: uint32(StateConnected), - connect: make(chan *RejectError), + // Buffered: once DialWithDialer returns on its synchronization + // timeout nothing reads this channel again, and an unbuffered send + // from handleReject would block readRoutine forever, leaking the + // goroutine along with the client, its user and channel maps and its + // multi-megabyte read buffer. + connect: make(chan *RejectError, 1), end: make(chan struct{}), } diff --git a/gumble/gumble/reject_regression_test.go b/gumble/gumble/reject_regression_test.go new file mode 100644 index 0000000..7e4f186 --- /dev/null +++ b/gumble/gumble/reject_regression_test.go @@ -0,0 +1,56 @@ +package gumble + +import ( + "io" + "net" + "testing" + "time" + + "git.stormux.org/storm/barnard/gumble/gumble/MumbleProto" + "google.golang.org/protobuf/proto" +) + +// Regression: the connect channel was unbuffered, so a Reject arriving after +// DialWithDialer had already returned on its synchronization timeout blocked +// readRoutine forever, leaking that goroutine and the whole client with it. +func TestHandleRejectDoesNotBlockWithoutAReceiver(t *testing.T) { + c := &Client{ + Config: NewConfig(), + Users: make(Users), + connect: make(chan *RejectError, 1), + state: uint32(StateConnected), + } + c.Conn = NewConn(nopConn{}) + + reason := "server is full" + data, _ := proto.Marshal(&MumbleProto.Reject{Reason: &reason}) + + done := make(chan struct{}) + go func() { + _ = c.handleReject(data) + close(done) + }() + + select { + case <-done: + case <-timeoutChan(): + t.Fatal("handleReject blocked with no reader on the connect channel") + } +} + +// nopConn is a net.Conn that discards everything, so handleReject's Close call +// has something to act on. +type nopConn struct{} + +func (nopConn) Read(b []byte) (int, error) { return 0, io.EOF } +func (nopConn) Write(b []byte) (int, error) { return len(b), nil } +func (nopConn) Close() error { return nil } +func (nopConn) LocalAddr() net.Addr { return nil } +func (nopConn) RemoteAddr() net.Addr { return nil } +func (nopConn) SetDeadline(t time.Time) error { return nil } +func (nopConn) SetReadDeadline(t time.Time) error { return nil } +func (nopConn) SetWriteDeadline(t time.Time) error { return nil } + +func timeoutChan() <-chan time.Time { + return time.After(10 * time.Second) +}