diff --git a/gumble/gumble/channel_cycle_regression_test.go b/gumble/gumble/channel_cycle_regression_test.go new file mode 100644 index 0000000..5d05631 --- /dev/null +++ b/gumble/gumble/channel_cycle_regression_test.go @@ -0,0 +1,84 @@ +package gumble + +import ( + "testing" + + "git.stormux.org/storm/barnard/gumble/gumble/MumbleProto" + "google.golang.org/protobuf/proto" +) + +// Regression: handleChannelState accepted any parent the server named, so a +// channel could be made its own ancestor. Everything that walks the resulting +// Parent/Children graph then recurses until it exhausts memory. +func TestChannelStateRejectsSelfParent(t *testing.T) { + c := &Client{Config: NewConfig(), Channels: make(Channels)} + root := c.Channels.create(0) + child := c.Channels.create(1) + child.Parent = root + root.Children[child.ID] = child + + id, parent := child.ID, child.ID + data, _ := proto.Marshal(&MumbleProto.ChannelState{ChannelId: &id, Parent: &parent}) + if err := c.handleChannelState(data); err != nil { + t.Fatal(err) + } + + if child.Parent == child { + t.Fatal("channel was made its own parent") + } + if _, ok := child.Children[child.ID]; ok { + t.Fatal("channel was made its own child") + } + if child.Parent != root { + t.Fatal("the rejected move should have left the original parent intact") + } +} + +// A channel must not be reparented under one of its own descendants either. +func TestChannelStateRejectsDescendantParent(t *testing.T) { + c := &Client{Config: NewConfig(), Channels: make(Channels)} + root := c.Channels.create(0) + middle := c.Channels.create(1) + leaf := c.Channels.create(2) + middle.Parent, root.Children[middle.ID] = root, middle + leaf.Parent, middle.Children[leaf.ID] = middle, leaf + + id, parent := middle.ID, leaf.ID + data, _ := proto.Marshal(&MumbleProto.ChannelState{ChannelId: &id, Parent: &parent}) + if err := c.handleChannelState(data); err != nil { + t.Fatal(err) + } + + if middle.Parent != root { + t.Fatal("a cyclic reparent was applied instead of ignored") + } + if isChannelDescendant(middle.Parent, middle) { + t.Fatal("channel graph is cyclic") + } +} + +// A legitimate move must still be applied. +func TestChannelStateAllowsNonCyclicMove(t *testing.T) { + c := &Client{Config: NewConfig(), Channels: make(Channels)} + root := c.Channels.create(0) + a := c.Channels.create(1) + b := c.Channels.create(2) + a.Parent, root.Children[a.ID] = root, a + b.Parent, root.Children[b.ID] = root, b + + id, parent := b.ID, a.ID + data, _ := proto.Marshal(&MumbleProto.ChannelState{ChannelId: &id, Parent: &parent}) + if err := c.handleChannelState(data); err != nil { + t.Fatal(err) + } + + if b.Parent != a { + t.Fatal("a valid reparent was rejected") + } + if a.Children[b.ID] != b { + t.Fatal("child link missing after a valid reparent") + } + if _, ok := root.Children[b.ID]; ok { + t.Fatal("stale child link left on the old parent") + } +} diff --git a/gumble/gumble/handlers.go b/gumble/gumble/handlers.go index 5ddcf4e..b893dd7 100644 --- a/gumble/gumble/handlers.go +++ b/gumble/gumble/handlers.go @@ -447,6 +447,23 @@ func (c *Client) handleChannelRemove(buffer []byte) error { return nil } +// maxChannelDepth bounds ancestry walks over a channel tree that may already +// be cyclic. Real Mumble trees are far shallower than this. +const maxChannelDepth = 1024 + +// isChannelDescendant reports whether candidate is channel itself or sits +// below it in the channel tree. The walk is bounded so an already-cyclic +// graph cannot hang the caller. +func isChannelDescendant(candidate, channel *Channel) bool { + for i := 0; candidate != nil && i <= maxChannelDepth; i++ { + if candidate == channel { + return true + } + candidate = candidate.Parent + } + return false +} + func (c *Client) handleChannelState(buffer []byte) error { var packet MumbleProto.ChannelState if err := proto.Unmarshal(buffer, &packet); err != nil { @@ -473,16 +490,25 @@ func (c *Client) handleChannelState(buffer []byte) error { } event.Channel = channel if packet.Parent != nil { - if channel.Parent != nil { - delete(channel.Parent.Children, channelID) - } newParent := c.Channels[*packet.Parent] - if newParent != channel.Parent { - event.Type |= ChannelChangeMoved - } - channel.Parent = newParent - if channel.Parent != nil { - channel.Parent.Children[channel.ID] = channel + // Reparenting a channel under itself or one of its own + // descendants makes Parent/Children cyclic, and anything that + // walks the tree then recurses until it exhausts memory. Ignore + // the move rather than corrupt the channel graph. + if isChannelDescendant(newParent, channel) { + log.Warn("handleChannelState: ignoring cyclic parent %d for channel %d", + *packet.Parent, channelID) + } else { + if channel.Parent != nil { + delete(channel.Parent.Children, channelID) + } + if newParent != channel.Parent { + event.Type |= ChannelChangeMoved + } + channel.Parent = newParent + if channel.Parent != nil { + channel.Parent.Children[channel.ID] = channel + } } } if packet.Name != nil {