Avoid deadlock on unknown user channel

This commit is contained in:
Brandon McGinty (chatgpt)
2026-08-09 14:22:46 -04:00
committed by Brandon McGinty
parent f67cc657eb
commit 3f1e5d42ad
3 changed files with 31 additions and 3 deletions
+2 -2
View File
@@ -37,14 +37,14 @@ Priority 0: security and crashers
nil. Initialize the map in DialWithDialer and assign its owning client when nil. Initialize the map in DialWithDialer and assign its owning client when
actions are created. Add handler tests for add/remove/trigger. actions are created. Add handler tests for add/remove/trigger.
4. Unknown ChannelId deadlocks the protocol reader [x] 4. Unknown ChannelId deadlocks the protocol reader
File: gumble/gumble/handlers.go File: gumble/gumble/handlers.go
In handleUserState, the unknown ChannelId branch takes c.volatile.Lock() In handleUserState, the unknown ChannelId branch takes c.volatile.Lock()
again instead of unlocking before returning. This leaves the mutex locked again instead of unlocking before returning. This leaves the mutex locked
forever. Replace with one unlock (prefer defer after acquisition) and add forever. Replace with one unlock (prefer defer after acquisition) and add
a malformed/out-of-order channel test. a malformed/out-of-order channel test.
5. OpenAL Buffer.Delete deletes a source, not a buffer [x] 5. OpenAL Buffer.Delete deletes a source, not a buffer
File: gumble/go-openal/openal/buffer.go File: gumble/go-openal/openal/buffer.go
Buffer.Delete calls C.walDeleteSource. It must call C.walDeleteBuffer. Buffer.Delete calls C.walDeleteSource. It must call C.walDeleteBuffer.
The current code reports invalid source errors and leaks OpenAL buffers. The current code reports invalid source errors and leaks OpenAL buffers.
+1 -1
View File
@@ -660,7 +660,7 @@ func (c *Client) handleUserState(buffer []byte) error {
} }
newChannel := c.Channels[*packet.ChannelId] newChannel := c.Channels[*packet.ChannelId]
if newChannel == nil { if newChannel == nil {
c.volatile.Lock() c.volatile.Unlock()
return errInvalidProtobuf return errInvalidProtobuf
} }
if newChannel != user.Channel { if newChannel != user.Channel {
+28
View File
@@ -0,0 +1,28 @@
package gumble
import (
"testing"
"time"
"git.stormux.org/storm/barnard/gumble/gumble/MumbleProto"
"google.golang.org/protobuf/proto"
)
// Regression: an out-of-order user move to an unknown channel used to lock
// volatile twice, permanently deadlocking subsequent protocol handling.
func TestUserStateUnknownChannelDoesNotDeadlock(t *testing.T) {
c := &Client{Config: NewConfig(), Users: make(Users), Channels: make(Channels)}
c.Users.create(1)
id, channel := uint32(1), uint32(99)
data, _ := proto.Marshal(&MumbleProto.UserState{Session: &id, ChannelId: &channel})
if err := c.handleUserState(data); err != errInvalidProtobuf {
t.Fatalf("got %v", err)
}
locked := make(chan struct{})
go func() { c.volatile.Lock(); c.volatile.Unlock(); close(locked) }()
select {
case <-locked:
case <-time.After(time.Second):
t.Fatal("volatile lock was left locked")
}
}