diff --git a/barnard.go b/barnard.go index 43688db..4f1e303 100644 --- a/barnard.go +++ b/barnard.go @@ -70,6 +70,10 @@ type Barnard struct { // Added for file playback FileStream *fileplayback.Player FileStreamMutex sync.Mutex + // stereoEncoder is reused across connections. Each one holds a little + // under a megabyte of encoder state, so building a fresh one per + // reconnect is pure churn; it is reset when file playback ends. + stereoEncoder gumble.AudioEncoder // Added for tone test mode (bypasses all soundcard/OpenAL) ToneTest bool @@ -91,6 +95,8 @@ type Barnard struct { reconnectStop chan struct{} reconnectStopOnce sync.Once + reconnectMutex sync.Mutex + reconnecting bool } // cleanupConnectionAudio releases connection-owned audio resources before a @@ -114,11 +120,20 @@ func (b *Barnard) cleanupConnectionAudio() { b.connectionMutex.Unlock() } -func (b *Barnard) cleanupToneTestAudio() { +// detachToneTestAudio unsubscribes the saver without closing its output, so a +// reconnect can re-attach the same file. The saver's output is opened +// exclusively and cannot be reopened. +func (b *Barnard) detachToneTestAudio() { if b.toneTestSaverDetach != nil { b.toneTestSaverDetach.Detach() b.toneTestSaverDetach = nil } +} + +// cleanupToneTestAudio detaches the saver and closes its output. Use it when +// the client is shutting down, not between connections. +func (b *Barnard) cleanupToneTestAudio() { + b.detachToneTestAudio() if b.toneTestSaver != nil { b.toneTestSaver.Stop() b.toneTestSaver = nil diff --git a/barnard.toml b/barnard.toml deleted file mode 100644 index 5273841..0000000 --- a/barnard.toml +++ /dev/null @@ -1,36 +0,0 @@ -AudioDriver = 'pipewire' -MicVolume = 1.0 -InputDevice = '' -OutputDevice = '' -DefaultServer = 'mumble.the-brannons.com:64738' -Username = 'bmc-beta' -NotifyCommand = '/usr/share/barnard/barnard-sound.sh "%event" "%who" "%what"' -NoiseSuppressionEnabled = false -Certificate = '' -RecordingFormat = 'flac' -RecordingDirectory = '~/Audio' - -[Hotkeys] -Talk = 'f8' -VolumeDown = 'f5' -VolumeUp = 'f6' -VolumeReset = 'f7' -MuteToggle = 'f4' -RecordToggle = 'ctrl_r' -Exit = 'f10' -ToggleTimestamps = 'f3' -SwitchViews = 'tab' -ScrollUp = 'pgup' -ScrollDown = 'pgdn' -AdminMenu = 'f11' -NoiseSuppressionToggle = 'f9' - -[[Servers]] -Host = 'mumble.the-brannons.com' -Port = 64738 - -[[Servers.Users]] -Username = 'bmc-beta' -Boost = 1 -Volume = 1.0 -LocallyMuted = false diff --git a/client.go b/client.go index 3d4e7a4..66d4a8a 100644 --- a/client.go +++ b/client.go @@ -61,15 +61,24 @@ func (b *Barnard) connect(reconnect bool) bool { // --- Tone test mode: skip all OpenAL; generate 440 Hz tone // --- and save incoming audio to a file. - // Open the output first. Starting transmission before this succeeds - // leaves an orphaned tone goroutine when the path is unusable. - saver, err := NewAudioFileSaver(b.ToneTestOutput) - if err != nil { - b.exitWithError(err) - return false + // The output is reserved exclusively, so it can only be opened once. + // A reconnect keeps writing to the saver opened for the first + // connection instead of failing on the file that already exists. + if b.toneTestSaver == nil { + // Open the output first. Starting transmission before this + // succeeds leaves an orphaned tone goroutine when the path is + // unusable. + saver, err := NewAudioFileSaver(b.ToneTestOutput) + if err != nil { + b.exitWithError(err) + return false + } + b.toneTestSaver = saver } - b.toneTestSaver = saver - b.toneTestSaverDetach = b.Client.Config.AttachAudio(saver) + // Detach any registration left over from the previous connection so + // the shared audio listener list does not grow once per reconnect. + b.detachToneTestAudio() + b.toneTestSaverDetach = b.Client.Config.AttachAudio(b.toneTestSaver) b.setConnected(true) if b.toneTestAutoTransmit() { @@ -99,11 +108,16 @@ func (b *Barnard) connect(reconnect bool) bool { } }) - // Initialize stereo encoder for file playback - b.Client.SetStereoEncoder(opus.NewStereoEncoder()) + // Initialize stereo encoder for file playback, reusing the one built for + // the previous connection rather than allocating another. + if b.stereoEncoder == nil { + b.stereoEncoder = opus.NewStereoEncoder() + } + b.Client.SetStereoEncoder(b.stereoEncoder) // Initialize file player b.FileStreamMutex.Lock() + previousFile := b.FileStream b.FileStream = fileplayback.New(b.Client) b.FileStream.SetErrorFunc(func(err error) { // Disable stereo when file finishes or errors @@ -113,9 +127,25 @@ func (b *Barnard) connect(reconnect bool) bool { stream.SetFilePlayer(b.FileStream) b.FileStreamMutex.Unlock() b.connectionMutex.Lock() + previousStream := b.Stream b.Stream = stream b.connectionMutex.Unlock() + // A disconnect that lands while the OpenAL devices are opening starts a + // second reconnect, so two connects can race to install a stream. The one + // that loses must be released here: an orphaned stream keeps its OpenAL + // device, its render thread and — because only Destroy detaches it — its + // entry in the shared audio listener list, so every later audio packet + // from every user is dispatched to it as well, for the life of the + // process. Release outside the locks, since Destroy waits on the + // per-user audio goroutines. + if previousFile != nil { + _ = previousFile.Stop() + } + if previousStream != nil { + previousStream.Destroy() + } + b.setConnected(true) // Dial delivers OnConnect before connect creates the OpenAL stream, so // start auto-transmit here as well for initial connections and reconnects. @@ -208,7 +238,9 @@ func (b *Barnard) OnDisconnect(e *gumble.DisconnectEvent) { close(b.toneTestStop) b.toneTestStop = nil } - b.cleanupToneTestAudio() + // Keep the saver's output open: it was reserved exclusively and a + // reconnect re-attaches the same file. It is closed on exit. + b.detachToneTestAudio() } b.Notify("disconnect", "me", reason) @@ -223,7 +255,27 @@ func (b *Barnard) OnDisconnect(e *gumble.DisconnectEvent) { b.UiTree.Rebuild() b.Ui.Refresh() }) - go b.reconnectGoroutine() + b.startReconnect() +} + +// startReconnect launches the reconnect loop unless one is already running. +// Disconnect notifications can arrive more than once for a connection, and +// every extra loop is another connect racing to install its own audio stream. +func (b *Barnard) startReconnect() { + b.reconnectMutex.Lock() + defer b.reconnectMutex.Unlock() + if b.reconnecting { + return + } + b.reconnecting = true + go func() { + defer func() { + b.reconnectMutex.Lock() + b.reconnecting = false + b.reconnectMutex.Unlock() + }() + b.reconnectGoroutine() + }() } func (b *Barnard) reconnectGoroutine() { diff --git a/extras/barnard-memwatch.sh b/extras/barnard-memwatch.sh new file mode 100755 index 0000000..c975f2d --- /dev/null +++ b/extras/barnard-memwatch.sh @@ -0,0 +1,80 @@ +#!/bin/bash +# barnard-memwatch.sh +# Description: Records barnard's memory use alongside its Go heap size, and +# captures profiles before the kernel's OOM killer can take the evidence away. +# +# Start barnard with -profile, then run this alongside it. The ratio between +# the two numbers is the diagnosis: +# +# RSS large, go_heap small -> the growth is in cgo memory (OpenAL, opus, +# rnnoise). The Go garbage collector cannot see +# it and so applies no back pressure at all. +# RSS large, go_heap large -> the growth is Go-side, and the heap profile +# this script dumps names what is holding it. +# +# Usage: barnard-memwatch.sh [output-log] +# +# Copyright 2026, Storm Dragon, +# +# This is free software; you can redistribute it and/or modify it under the +# terms of the GNU General Public License as published by the Free +# Software Foundation; either version 3, or (at your option) any later +# version. +# +# This software is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU +# General Public License for more details. + +set -u + +# barnard serves its profiles here when started with -profile. +profile_host="localhost:6060" +# Dump profiles once resident memory passes this many kilobytes. +dump_threshold_kb=2000000 +sample_interval=30 + +output="${1:-$HOME/barnard-memwatch.log}" + +pid="$(pgrep -n -x barnard)" || { + echo "barnard is not running" >&2 + exit 1 +} + +if ! curl -s -m 2 "http://${profile_host}/debug/pprof/" > /dev/null; then + echo "no profile server on ${profile_host}; start barnard with -profile" >&2 + exit 1 +fi + +echo "watching barnard (pid ${pid}), writing to ${output}" + +dumped=0 +while kill -0 "$pid" 2> /dev/null; do + rss="$(awk '/^VmRSS/{print $2}' "/proc/${pid}/status" 2> /dev/null)" + swap="$(awk '/^VmSwap/{print $2}' "/proc/${pid}/status" 2> /dev/null)" + heap="$(curl -s -m 2 "http://${profile_host}/debug/pprof/heap?debug=1" \ + | awk '/^# HeapInuse/{print $4}')" + goroutines="$(curl -s -m 2 "http://${profile_host}/debug/pprof/goroutine?debug=1" \ + | head -1 | grep -o '[0-9]*')" + threads="$(ls "/proc/${pid}/task" 2> /dev/null | wc -l)" + + printf '%s rss=%skB swap=%skB go_heap=%sB goroutines=%s threads=%s\n' \ + "$(date +%T)" "${rss:-?}" "${swap:-0}" "${heap:-?}" \ + "${goroutines:-?}" "${threads}" >> "$output" + + # Capture the evidence once, while the process is still alive to ask. + if [[ ${dumped} -eq 0 && ${rss:-0} -gt ${dump_threshold_kb} ]]; then + dumped=1 + curl -s -m 10 -o "${output}.heap" \ + "http://${profile_host}/debug/pprof/heap" + curl -s -m 10 -o "${output}.goroutine" \ + "http://${profile_host}/debug/pprof/goroutine?debug=2" + cp "/proc/${pid}/smaps_rollup" "${output}.smaps" 2> /dev/null + printf '%s *** dumped profiles at rss=%skB ***\n' \ + "$(date +%T)" "${rss}" >> "$output" + fi + + sleep "${sample_interval}" +done + +echo "barnard exited; log is in ${output}" diff --git a/fix.txt b/fix.txt deleted file mode 100644 index f1fe7f8..0000000 --- a/fix.txt +++ /dev/null @@ -1,308 +0,0 @@ -Barnard audit findings -====================== - -This list was assembled from static review of the maintained Go sources, -including Barnard, gumble, gumbleopenal, the local OpenAL binding, recording, -file playback, UI, and protocol code. Generated protobuf output and the -vendored Mumble C++ source were not treated as files to modify. - -Priority 0: security and crashers ---------------------------------- - -[x] 1. UDP crypto secrets are logged - Files: gumble/gumble/udp15.go, gumble/gumble/crypt.go - Both crypto setup paths log the AES key and IV/nonce material at debug - level. Anyone who obtains debug logs and a packet capture can decrypt - voice traffic. Remove all secret material from logs. At most log lengths, - setup success, and a non-secret connection identifier. - -[x] 2. Native UDP races TCP state mutation - Files: gumble/gumble/udp15.go, gumble/gumble/handlers.go, - gumble/gumble/audiolisteners.go - The UDP reader runs independently of the TCP read routine. It reads - Client.Users, User.decoder/audio sequence state, and audio-listener stream - maps while TCP handlers add/remove users and close stream channels. This - can cause data races, concurrent map read/write panics, or sends to a - closed stream channel. Establish one synchronization regime: protect - client user/channel state and audio stream registration with locks, and - ensure channel close/send are serialized. Do not rely on the TCP read - routine being serialized with UDP. - -[x] 3. Context actions panic on receipt and on trigger - Files: gumble/gumble/client.go, gumble/gumble/handlers.go, - gumble/gumble/contextaction.go - Client.ContextActions is never initialized, so the first - ContextActionModify_Add writes to a nil map. Further, newly created - ContextAction values do not receive client = c, so Trigger methods dereference - nil. Initialize the map in DialWithDialer and assign its owning client when - actions are created. Add handler tests for add/remove/trigger. - -[x] 4. Unknown ChannelId deadlocks the protocol reader - File: gumble/gumble/handlers.go - In handleUserState, the unknown ChannelId branch takes c.volatile.Lock() - again instead of unlocking before returning. This leaves the mutex locked - forever. Replace with one unlock (prefer defer after acquisition) and add - a malformed/out-of-order channel test. - -[x] 5. OpenAL Buffer.Delete deletes a source, not a buffer - File: gumble/go-openal/openal/buffer.go - Buffer.Delete calls C.walDeleteSource. It must call C.walDeleteBuffer. - The current code reports invalid source errors and leaks OpenAL buffers. - Add a binding test that creates and deletes a single buffer and checks - openal.Err(). - -[x] 6. UI writes are concurrent and termbox is not protected - Files: ui.go, client.go, gumble/gumbleopenal/stream.go, uiterm/*.go - Network callbacks, audio capture error callbacks, and reconnect goroutines - directly update Ui, Textview, Tree, Label, and termbox while Ui.Run updates - the same state. These types have no locks and termbox calls are not safe - from arbitrary goroutines. Route UI work through a UI-owned event queue, or - protect all state and ensure only the UI goroutine calls termbox. - -[x] 7. Terminal control sequences from server data are rendered - Files: client.go, ui.go, ui_tree.go, admin.go - HTML escaping does not remove terminal escape/control sequences. Server - supplied messages, names, comments, and channel names are displayed in the - terminal and can contain ANSI/OSC controls. Sanitize for terminal display: - remove/control-escape C0, DEL, ESC, and dangerous Unicode controls before - rendering or notifying. - -Priority 1: transport, lifecycle, and correctness ---------------------------------------------------- - -[x] 8. TCP audio is discarded before UDP is proven usable - Files: gumble/gumble/crypt.go, gumble/gumble/client.go, gumble/gumble/udp.go - udpActive is set immediately after CryptSetup. The TCP read routine then - discards UDPTunnel packets even if inbound UDP is blocked or NAT setup has - failed. Mark UDP active only after an authenticated UDP response/audio - packet (or retain TCP until confirmed), and define fallback/recovery rules. - -[x] 9. Stream capture shutdown/startup races OpenAL - File: gumble/gumbleopenal/stream.go - StopSource closes a channel but does not wait for sourceRoutine. Destroy - immediately closes the capture device, so the routine can access a closed - device. A quick stop/start can also run two capture routines at once. Use - a cancellation context plus WaitGroup/done channel; serialize Start, Stop, - reopen, and Destroy; wait before CaptureCloseDevice. - -[x] 10. Renderer can be used after it is closed - Files: gumble/gumbleopenal/stream.go, gumble/gumble/audiolisteners.go - Existing OnAudioStream goroutines can run cleanup after Destroy closes - renderCh. Their final render call then panics sending on a closed channel. - Stop and join all audio stream goroutines before renderer shutdown; make - render reject work after shutdown without panicking. - -[x] 11. Reconnect leaks the old audio stream - File: client.go - OnDisconnect starts reconnecting but never destroys the existing Stream or - stops its file player/capture routine. connect creates a new Stream and - overwrites b.Stream. Destroy/stop the old resources before reconnecting; - make disconnect cleanup idempotent. - -[x] 12. File player sessions race each other - File: fileplayback/player.go - readFileAudio repeatedly reads mutable Player stopChan/ctx/audioChan. - Stop can return and a new PlayFile can replace them while the old ffmpeg - goroutine is still running. Old audio can enter the new session and old - workers can survive. Put per-playback state in a session object with local - context, stop channel, output channel, and WaitGroup. Stop must cancel and - join that session before another begins. - -[x] 13. Recorder Stop races the encoder writer - File: recording/recorder.go - Stop closes stdin while run may be writing. A normal stop can therefore - record a closed-pipe error and be reported as failed. Have run own stdin - closure: signal stop, wait for run to finish/close stdin and Wait ffmpeg, - then return its result. Do not close stdin concurrently from Stop. - -[x] 14. Tone-test startup leaks transmission on output-file error - File: client.go - connect starts StartToneGenerator and sets Tx before NewAudioFileSaver. If - output file creation fails, the tone goroutine continues. Create the saver - first, or close/wait for the tone generator and reset Tx on every failure. - -[x] 15. Gumble ffmpeg Pause can block forever - File: gumble/gumbleffmpeg/stream.go - Pause checks StatePlaying, releases the lock, then sends on an unbuffered - pause channel. If process exits in between, no receiver remains. Redesign - around context cancellation/state guarded by a mutex and a per-run done - channel. Also synchronize Volume, which is currently read and written - without protection. - -[x] 16. Audio listener/event listener detach is not concurrency-safe - Files: gumble/gumble/listeners.go, gumble/gumble/audiolisteners.go - Event listener detach has no lock; audio detach removes streams without - closing/joining them. Concurrent attach/detach/delivery can corrupt linked - lists or strand goroutines. Use mutex-protected listener snapshots and an - idempotent detach operation. - -[x] 17. Notification commands block callers and substitution is unsafe - File: main.go - Notify sends to an unbuffered channel. The one consumer waits for each - shell command, so slow notification programs block UI/network callbacks. - Use a bounded queue and define dropping/backpressure behavior. Placeholder - replacement is sequential: a user-controlled value containing a later - placeholder can be re-expanded inside prior substituted text. Build argv - without a shell where possible, or perform non-recursive token expansion - in one pass. - -Priority 2: protocol and data correctness ------------------------------------------- - -[x] 18. UserStats FromServer fields are copied from FromClient - File: gumble/gumble/handlers.go - In handleUserStats, FromServer.Good is correct but Late/Lost/Resync read - packet.FromClient. Use packet.FromServer for all four fields and add a - regression test with differing values. - -[x] 19. Full channel link updates leave stale reverse links - File: gumble/gumble/handlers.go - A ChannelState Links replacement assigns a new channel.Links map but does - not remove channel from the Links maps of old peers. Remove reciprocal old - links before replacement and add link add/remove/full-replacement tests. - -[x] 20. Malformed protobuf fields can panic handlers - File: gumble/gumble/handlers.go - Several optional proto fields are dereferenced without validation, notably - ACL group.Name and UserList_User.UserId. Validate required fields before - dereferencing and return errInvalidProtobuf for malformed server packets. - Audit all packet pointer dereferences similarly. - -[x] 21. UDP protocol state has no complete interoperability test coverage - Files: gumble/gumble/udp15.go, gumble/gumble/udp.go - Tests are mostly local encrypt/decrypt round trips. Add captured/reference - vectors from current Mumble for CryptSetup, encrypted audio, ping, packet - loss, IV wrap, late/replayed packets, protobuf and legacy envelopes, frame - terminators, positional data, and volume adjustment. Test real UDP - fallback behavior too. - -[x] 22. Opus bitrate calculation assumes a 10 ms interval - File: gumble/opus/opus.go - bitrate is maxDataBytes * 8 * 100. For permitted 20/40/60 ms intervals it - is 2x/4x/6x too high. Calculate bits per frame divided by the actual - Config.AudioInterval, or set the bitrate once when configuration changes. - -[x] 23. AudioInterval accepts invalid values - File: gumble/gumble/config.go - AudioFrameSize truncates arbitrary intervals to a count of 10 ms frames, - while the ticker still uses the original interval. Validate and reject - values other than 10/20/40/60 ms (and validate AudioDataBytes/Buffers). - -[x] 24. Legacy/custom varint has a MinInt64 recursion failure - File: gumble/gumble/varint/write.go - Encoding math.MinInt64 evaluates -value to the same negative number and - recursively encodes forever until panic. Handle MinInt64 explicitly or - encode negatives using an unsigned magnitude without overflow. Validate - output buffer capacity in the exported encoder too. - -[x] 25. Mumble version layout documentation is wrong - File: gumble/gumble/version.go - The comment says major uses bits 0-15, but SemanticVersion and ClientVersion - use bits 16-31. Correct the documentation and add known version tests. - -Priority 3: configuration, UI, and binding hardening ------------------------------------------------------- - -[x] 26. Persisted microphone volume is unused and zero is impossible - Files: config/user_config.go, ui.go, gumble/gumbleopenal/stream.go - MicVolume is stored but never applied when a Stream is created; UI changes - do not call SaveConfig. GetMicVolume treats stored zero bits as - uninitialized and returns 1.0, so mute cannot persist. Initialize the - atomic value to 1.0 in New, apply configured volume during connect, allow - zero, and save volume changes. - -[x] 27. Config address and file error handling can panic - File: config/user_config.go - makeHostPort splits on ':' and panics for malformed addresses or IPv6. - fileExists dereferences info after non-ENOENT Stat failures. Replace with - net.SplitHostPort (with explicit default-port policy) and return/report - errors from Stat rather than dereferencing nil. - -[x] 28. Config SaveConfig panics and is not robust - File: config/user_config.go - Configuration write/rename errors panic the client. Return errors to the - caller, preserve the prior config on failure, and consider fsyncing the - temporary file/directory before rename. Avoid broad unrelated formatting - changes while fixing this. - -[x] 29. FIFO reader spins after an error - File: main.go - setup_fifo ignores all ReadBytes errors and retries immediately. It also - never closes the FIFO descriptor. Exit the reader on terminal errors, - close the descriptor, and make shutdown cancellable. - -[x] 30. Empty UI tree can panic; UI startup errors are swallowed - Files: uiterm/tree.go, uiterm/ui.go - Tree.uiKeyEvent indexes lines[activeLine] for a non-arrow key even when - no lines exist. Guard empty trees. Ui.Run returns nil when termbox.Init - fails, hiding startup failure. Return that error. Also stop/join the - PollEvent goroutine on UI shutdown and make Close nonblocking/idempotent. - -[x] 31. Text UI is not Unicode-safe and timestamp parsing is fragile - Files: uiterm/textbox.go, uiterm/textview.go - Textbox cursor positions are byte offsets but editing/display iterates - runes, so non-ASCII input can be split into invalid UTF-8. Textview assumes - every line contains ']' when timestamps are hidden and can panic otherwise. - Track rune boundaries and use safe timestamp parsing/fallbacks. - -[x] 32. OpenAL binding needs API and unsafe hardening - Files: gumble/go-openal/openal/*.go - Many public slice APIs unconditionally use &slice[0] and panic for empty - input (NewBuffers(0), Delete empty lists, SetData empty data, GetIntegerv - size zero, etc.). Add guards or documented errors. go vet reports unsafe - pointer misuse in alcCore.go; replace stored uintptr C handles with a - vetted representation/pattern and re-run vet. Listener orientation uses a - global tempSlice without synchronization; use a local fixed array. - -[x] 33. OpenAL errors are mostly ignored - Files: gumble/gumbleopenal/stream.go, gumble/go-openal/openal/*.go - Source/buffer/context/capture calls generally do not check AL/ALC errors, - so invalid device/context/buffer operations become silent audio failure. - Add checked wrapper operations for lifecycle-critical calls and surface - actionable errors to Barnard. - -[x] 34. Beep helpers panic when the external command is absent - Files: ui.go, gumble/gumbleopenal/stream.go - Both helpers panic on exec failure. Return/log an error or remove unused - helpers; a missing optional beep binary must not terminate Barnard. - -[x] 35. Admin manual-ban duration accepts negative values - Files: admin.go, gumble/gumble/bans.go - Negative minutes become a negative duration, then are cast to uint32 - seconds for the protocol, creating a huge ban duration. Reject negative - durations and validate mask/address before sending. - -[x] 36. Admin, tree, and client code read mutable maps outside Client.Do - Files: admin.go, ui_tree.go, client.go - UI code ranges Client.Users, Channels, Channel.Users, and Children while - network handlers mutate them. This overlaps issue 2 but must be fixed on - the UI side as well: take a safe snapshot under the client lock and render - the snapshot outside the lock. - -Suggested repair order ----------------------- - -1. Remove crypto secret logging; fix ContextActions initialization/client; - fix the handler deadlock and OpenAL Buffer.Delete. -2. Define client/UDP/audio-stream locking and lifecycle ownership, then add - race/integration tests for user removal, disconnect, reconnect, and UDP - fallback. -3. Make UI updates single-threaded and sanitize terminal output. -4. Repair recording/file/capture session ownership and joining. -5. Fix protocol data errors, validation, Opus interval math, and config input. -6. Harden the OpenAL binding and remaining UI/config edge cases. - -Required verification after fixes --------------------------------- - -- gofmt only touched files. -- go test ./... -- go test -race ./... -- go vet ./... with no remaining unsafe-pointer warnings. -- Add focused unit tests for every deterministic bug above. -- Add a local Mumble integration test or reproducible harness for TCP-only, - UDP success, blocked inbound UDP fallback, reconnect, user removal during - UDP audio, and native 1.5 UDP reference packets. -- Manually test capture stop/start, disconnect/reconnect, file start/stop, - recording stop, zero mic volume, Unicode/UI input, and no-beep/no-ffmpeg - failure paths. diff --git a/gumble/gumble/audiolisteners.go b/gumble/gumble/audiolisteners.go index e56a4ea..9720067 100644 --- a/gumble/gumble/audiolisteners.go +++ b/gumble/gumble/audiolisteners.go @@ -54,10 +54,12 @@ func (e *AudioListeners) Attach(listener AudioListener) Detacher { if e.head == nil { e.head = item } - if e.tail == nil { - e.tail = item - } else { + if e.tail != nil { e.tail.next = item } + // tail was previously left pointing at the first item ever attached. Once + // anything detached, the next attach linked itself onto a node that was no + // longer in the list, so that listener never received audio again. + e.tail = item return item } diff --git a/gumble/gumble/audiolisteners_regression_test.go b/gumble/gumble/audiolisteners_regression_test.go new file mode 100644 index 0000000..ac72a48 --- /dev/null +++ b/gumble/gumble/audiolisteners_regression_test.go @@ -0,0 +1,71 @@ +package gumble + +import "testing" + +type countingAudioListener struct{ streams int } + +func (l *countingAudioListener) OnAudioStream(e *AudioStreamEvent) { + l.streams++ + go func() { + for range e.C { + } + }() +} + +// Regression: an audio listener is removed from the shared list only by +// Detach. A stream that was created but never destroyed therefore stayed +// subscribed for the life of the process, and every audio packet from every +// user was dispatched to it as well — one goroutine, one packet queue and one +// set of playback buffers per orphan, per user. This test pins the fan-out +// behaviour that makes failing to detach so expensive. +func TestDispatchAudioFansOutToEveryAttachedListener(t *testing.T) { + c := &Client{Config: NewConfig(), Users: make(Users)} + user := c.Users.create(1) + + first := &countingAudioListener{} + second := &countingAudioListener{} + firstLink := c.Config.AttachAudio(first) + c.Config.AttachAudio(second) + + c.dispatchAudio(user, &AudioPacket{Client: c, Sender: user}) + if first.streams != 1 || second.streams != 1 { + t.Fatalf("expected both listeners to receive the stream, got %d and %d", + first.streams, second.streams) + } + + // Detaching must actually stop the fan-out; this is the only thing that + // keeps a replaced stream from accumulating. + firstLink.Detach() + third := &countingAudioListener{} + c.Config.AttachAudio(third) + other := c.Users.create(2) + c.dispatchAudio(other, &AudioPacket{Client: c, Sender: other}) + + if first.streams != 1 { + t.Fatalf("detached listener still received audio: %d streams", first.streams) + } + if second.streams != 2 || third.streams != 1 { + t.Fatalf("attached listeners missed the second user: %d and %d", + second.streams, third.streams) + } +} + +// Detach must also release the per-user stream channels it owns, so a +// destroyed stream does not pin its queued audio. +func TestDetachClosesPerUserStreams(t *testing.T) { + c := &Client{Config: NewConfig(), Users: make(Users)} + user := c.Users.create(1) + + listener := &countingAudioListener{} + link := c.Config.AttachAudio(listener) + c.dispatchAudio(user, &AudioPacket{Client: c, Sender: user}) + + item := c.Config.AudioListeners.head + if item == nil || len(item.streams) != 1 { + t.Fatal("expected one per-user stream before detaching") + } + link.Detach() + if len(item.streams) != 0 { + t.Fatalf("Detach left %d per-user streams behind", len(item.streams)) + } +} 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/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/config.go b/gumble/gumble/config.go index 63c9978..8cdd167 100644 --- a/gumble/gumble/config.go +++ b/gumble/gumble/config.go @@ -38,6 +38,12 @@ type Config struct { Buffers int } +// MaximumBuffers caps Config.Buffers. Each buffer holds up to one maximum +// sized audio frame and is allocated per speaking user, so a large value +// multiplied by a populated channel is a substantial amount of memory. A few +// seconds of buffering is already far more than playback needs. +const MaximumBuffers = 1024 + // NewConfig returns a new Config struct with default values set. func NewConfig() *Config { return &Config{ @@ -64,6 +70,12 @@ func (c *Config) Validate() error { if c.Buffers <= 0 { return fmt.Errorf("gumble: Buffers must be positive") } + // Buffers is allocated per speaking user, both as a queue of decoded + // frames and as OpenAL playback buffers, so an unbounded value multiplies + // straight into memory use as a channel fills up. + if c.Buffers > MaximumBuffers { + return fmt.Errorf("gumble: Buffers must be at most %d", MaximumBuffers) + } return nil } diff --git a/gumble/gumble/config_buffers_regression_test.go b/gumble/gumble/config_buffers_regression_test.go new file mode 100644 index 0000000..feef9da --- /dev/null +++ b/gumble/gumble/config_buffers_regression_test.go @@ -0,0 +1,17 @@ +package gumble + +import "testing" + +// Regression: Buffers had no upper bound, but it is allocated per speaking +// user both as a decoded-frame queue and as OpenAL playback buffers. +func TestConfigRejectsOversizedBuffers(t *testing.T) { + config := NewConfig() + config.Buffers = MaximumBuffers + 1 + if err := config.Validate(); err == nil { + t.Fatal("expected Buffers above the maximum to be rejected") + } + config.Buffers = MaximumBuffers + if err := config.Validate(); err != nil { + t.Fatalf("Buffers at the maximum should be accepted: %v", err) + } +} diff --git a/gumble/gumble/conn.go b/gumble/gumble/conn.go index ed58cfa..6703958 100644 --- a/gumble/gumble/conn.go +++ b/gumble/gumble/conn.go @@ -16,6 +16,10 @@ import ( // DefaultPort is the default port on which Mumble servers listen. const DefaultPort = 64738 +// retainedPacketBytes is the largest read buffer kept between packets. Bigger +// buffers are allocated as needed and released again afterwards. +const retainedPacketBytes = 64 * 1024 + // Conn represents a control protocol connection to a Mumble client/server. type Conn struct { sync.Mutex @@ -54,6 +58,11 @@ func (c *Conn) ReadPacket() (uint16, []byte, error) { } if pLengthInt > len(c.buffer) { c.buffer = make([]byte, pLengthInt) + } else if len(c.buffer) > retainedPacketBytes && pLengthInt <= retainedPacketBytes { + // One oversized packet — a large ACL, user list or channel comment — + // used to pin its full size for the life of the connection. Give the + // memory back once ordinary traffic resumes. + c.buffer = make([]byte, retainedPacketBytes) } if _, err := io.ReadFull(c.Conn, c.buffer[:pLengthInt]); err != nil { return 0, nil, err diff --git a/gumble/gumble/conn_buffer_regression_test.go b/gumble/gumble/conn_buffer_regression_test.go new file mode 100644 index 0000000..5b35aa7 --- /dev/null +++ b/gumble/gumble/conn_buffer_regression_test.go @@ -0,0 +1,43 @@ +package gumble + +import ( + "encoding/binary" + "net" + "testing" +) + +// Regression: the read buffer grew to the largest packet ever seen and kept +// that memory for the life of the connection. +func TestConnBufferShrinksAfterAnOversizedPacket(t *testing.T) { + server, client := net.Pipe() + defer server.Close() + conn := NewConn(client) + + big := 4 * 1024 * 1024 + go func() { + defer server.Close() + writePacket := func(length int) { + var header [6]byte + binary.BigEndian.PutUint16(header[:], 3) + binary.BigEndian.PutUint32(header[2:], uint32(length)) + server.Write(header[:]) + server.Write(make([]byte, length)) + } + writePacket(big) + writePacket(128) + }() + + if _, _, err := conn.ReadPacket(); err != nil { + t.Fatalf("reading the oversized packet: %v", err) + } + if len(conn.buffer) < big { + t.Fatalf("oversized packet should have grown the buffer, got %d", len(conn.buffer)) + } + if _, _, err := conn.ReadPacket(); err != nil { + t.Fatalf("reading the small packet: %v", err) + } + if len(conn.buffer) > retainedPacketBytes { + t.Fatalf("buffer stayed at %d bytes after a small packet, above the %d retained size", + len(conn.buffer), retainedPacketBytes) + } +} diff --git a/gumble/gumble/handlers.go b/gumble/gumble/handlers.go index 5ddcf4e..9d47dde 100644 --- a/gumble/gumble/handlers.go +++ b/gumble/gumble/handlers.go @@ -184,8 +184,10 @@ func (c *Client) handleUDPTunnel(buffer []byte) error { log.Info("handleUDPTunnel: %s session=%d seq=%d audio_len=%d final=%v buf_remain=%d", user.Name, session, seq, audioLength, isFinal, len(buffer)) - if audioLength > len(buffer) { - log.Warn("handleUDPTunnel: audio length %d > remaining buffer %d", + // A negative length would pass the upper bound check below and then panic + // on the slice expression. + if audioLength < 0 || audioLength > len(buffer) { + log.Warn("handleUDPTunnel: audio length %d out of range for buffer %d", audioLength, len(buffer)) return errInvalidProtobuf } @@ -447,6 +449,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 +492,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 { 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) +} diff --git a/gumble/gumbleopenal/stream.go b/gumble/gumbleopenal/stream.go index d8ac501..b00ba24 100644 --- a/gumble/gumbleopenal/stream.go +++ b/gumble/gumbleopenal/stream.go @@ -138,6 +138,10 @@ type Stream struct { recorderMu sync.RWMutex errorFunc func(error) // called on capture errors recorder Recorder + // streamWG tracks the per-user goroutines started by OnAudioStream. They + // release their OpenAL source and buffers through the renderer, so Destroy + // must let them finish before it tears the renderer down. + streamWG sync.WaitGroup } func New(client *gumble.Client, inputDevice *string, outputDevice *string, test bool) (*Stream, error) { @@ -393,13 +397,37 @@ func (s *Stream) getRecorder() Recorder { return s.recorder } +// destroyDrainTimeout bounds how long Destroy waits for the per-user audio +// goroutines to finish draining, so a wedged renderer cannot hang a reconnect. +const destroyDrainTimeout = 2 * time.Second + func (s *Stream) Destroy() { if s.link != nil { + // Detach closes every per-user stream channel, which ends the + // goroutines started by OnAudioStream. s.link.Detach() } + // Those goroutines delete their OpenAL source and buffers through + // s.render, which stops working the moment the renderer is closed below. + // Waiting for them here is what keeps the device's sources and buffers + // from being orphaned on every reconnect. + drained := make(chan struct{}) + go func() { + s.streamWG.Wait() + close(drained) + }() + select { + case <-drained: + case <-time.After(destroyDrainTimeout): + log.Warn("Destroy: timed out waiting for audio stream goroutines; " + + "OpenAL sources and buffers may be released only by CloseDevice") + } if s.deviceSource != nil { s.StopSource() - s.deviceSource.CaptureCloseDevice() + if !s.deviceSource.CaptureCloseDevice() { + log.Error("Destroy: closing capture device %q failed", + deviceName(s.inputDeviceName)) + } s.deviceSource = nil } if s.deviceSink != nil { @@ -417,7 +445,14 @@ func (s *Stream) Destroy() { <-s.renderDone s.contextSink = nil } - s.deviceSink.CloseDevice() + // alcCloseDevice returns ALC_FALSE and frees nothing while the device + // still has contexts, buffers or sources outstanding. Dropping the + // handle after that silently leaks the device and every buffer it + // owns, which is invisible to the Go GC, so at least report it. + if !s.deviceSink.CloseDevice() { + log.Error("Destroy: closing playback device %q failed; its OpenAL "+ + "buffers cannot be reclaimed", deviceName(s.outputDeviceName)) + } s.deviceSink = nil } } @@ -485,7 +520,9 @@ func (s *Stream) SetMicVolume(change float32, relative bool) { } func (s *Stream) OnAudioStream(e *gumble.AudioStreamEvent) { + s.streamWG.Add(1) go func(e *gumble.AudioStreamEvent) { + defer s.streamWG.Done() log.Info("audio stream started for user %s", e.User.Name) var source openal.Source var emptyBufs openal.Buffers @@ -576,6 +613,9 @@ func (s *Stream) OnAudioStream(e *gumble.AudioStreamEvent) { return nil } p := jitterBuf[0] + // Clear the slot before resliceing: the popped entries stay in + // the backing array otherwise, pinning a decoded frame each. + jitterBuf[0] = nil jitterBuf = jitterBuf[1:] jitterDuration -= audioPacketDuration(p) // Frame numbers are Mumble timestamps in 10 ms units. @@ -657,6 +697,7 @@ func (s *Stream) OnAudioStream(e *gumble.AudioStreamEvent) { jitterBuf[0].Sequence, e.User.Name, jitterNextSeq, len(jitterBuf)) } jitterDuration -= audioPacketDuration(jitterBuf[0]) + jitterBuf[0] = nil jitterBuf = jitterBuf[1:] continue } diff --git a/main.go b/main.go index 4b3696b..62f8829 100644 --- a/main.go +++ b/main.go @@ -105,6 +105,10 @@ func main() { if err != nil { handle_raw_error(err) } + if *buffers <= 0 || *buffers > gumble.MaximumBuffers { + handle_raw_error(fmt.Errorf("buffers must be between 1 and %d, got %d", + gumble.MaximumBuffers, *buffers)) + } // Set up logging var level barnlog.Level diff --git a/recording/queue_regression_test.go b/recording/queue_regression_test.go new file mode 100644 index 0000000..7c608a1 --- /dev/null +++ b/recording/queue_regression_test.go @@ -0,0 +1,46 @@ +package recording + +import "testing" + +// Regression: the per-source mix queues grew without bound. The tick drains a +// fixed chunk per source, so a stalled encoder leaves a deficit the loop never +// makes up, and the backlog only ever grew from there. +func TestRecorderQueueIsCapped(t *testing.T) { + t.Parallel() + + var queue []int16 + frame := make([]int16, 960) + // Far more audio than the encoder could have consumed. + for i := 0; i < 2000; i++ { + queue = appendCapped(queue, frame) + } + + if len(queue) > maxQueuedSamples { + t.Fatalf("queue grew to %d samples, above the %d cap", + len(queue), maxQueuedSamples) + } +} + +// Capping must keep the newest audio: dropping the newest would make the +// recording lag further behind with every overflow. +func TestRecorderQueueKeepsNewestAudio(t *testing.T) { + t.Parallel() + + var queue []int16 + // Fill past the cap with a marker in the final frame. + filler := make([]int16, maxQueuedSamples) + queue = appendCapped(queue, filler) + newest := []int16{1, 2, 3, 4} + queue = appendCapped(queue, newest) + + if len(queue) != maxQueuedSamples { + t.Fatalf("expected the queue to sit at the %d cap, got %d", + maxQueuedSamples, len(queue)) + } + tail := queue[len(queue)-len(newest):] + for i, want := range newest { + if tail[i] != want { + t.Fatalf("newest audio was dropped: tail %v, want %v", tail, newest) + } + } +} diff --git a/recording/recorder.go b/recording/recorder.go index 82d896d..c8f22ee 100644 --- a/recording/recorder.go +++ b/recording/recorder.go @@ -20,6 +20,12 @@ const ( FormatOpus = "opus" ) +// maxQueuedSamples bounds the per-source mix backlog at roughly five seconds +// of 48 kHz stereo audio. A source that runs further ahead than this is ahead +// because the encoder stalled, and no amount of retained audio recovers the +// timeline; keeping the newest is better than growing without bound. +const maxQueuedSamples = 5 * gumble.AudioSampleRate * gumble.AudioChannels + type Recorder struct { path string format string @@ -188,6 +194,19 @@ func (r *Recorder) Stop() error { return r.err } +// appendCapped adds a source's incoming samples to its mix queue, bounded at +// maxQueuedSamples. Each tick drains one fixed chunk per source, so a stalled +// encoder leaves a deficit the loop never makes up and the backlog would +// otherwise grow for as long as the recording ran. The newest audio is kept: +// discarding it instead would only push the recording further behind. +func appendCapped(queue []int16, incoming []int16) []int16 { + queue = append(queue, incoming...) + if len(queue) > maxQueuedSamples { + queue = append(queue[:0], queue[len(queue)-maxQueuedSamples:]...) + } + return queue +} + func (r *Recorder) run() { defer close(r.done) ticker := time.NewTicker(r.interval) @@ -203,7 +222,7 @@ func (r *Recorder) run() { r.closeEncoder() return case item := <-r.input: - queues[item.source] = append(queues[item.source], item.samples...) + queues[item.source] = appendCapped(queues[item.source], item.samples) case <-ticker.C: clear(chunk) for source, buffer := range queues { diff --git a/ui.go b/ui.go index 6d2964e..e266cea 100644 --- a/ui.go +++ b/ui.go @@ -445,15 +445,19 @@ func (b *Barnard) OnMicVolumeUp(ui *uiterm.Ui, key uiterm.Key) { } func (b *Barnard) OnQuitPress(ui *uiterm.Ui, key uiterm.Key) { - b.stopReconnects() - b.StopRecordingIfActive(true) - b.Client.Disconnect() - b.Ui.Close() + b.shutdown() } func (b *Barnard) CommandExit(ui *uiterm.Ui, cmd string) { + b.shutdown() +} + +// shutdown releases everything that outlives a single connection, including +// the tone test saver's output file, which reconnects deliberately keep open. +func (b *Barnard) shutdown() { b.stopReconnects() b.StopRecordingIfActive(true) + b.cleanupToneTestAudio() b.Client.Disconnect() b.Ui.Close() } diff --git a/uiterm/textview.go b/uiterm/textview.go index e03ff4e..123632e 100644 --- a/uiterm/textview.go +++ b/uiterm/textview.go @@ -72,6 +72,48 @@ func (t *Textview) ScrollBottom() { t.uiDraw() } +const ( + // maxScrollbackLines bounds the retained chat history. It used to grow for + // the life of the process, and every line added re-wrapped the whole + // buffer, so the cost of a session grew as the square of its length. This + // is far more history than a reader ever scrolls back through. + maxScrollbackLines = 10000 + // scrollbackTrimChunk is how much history is discarded once the cap is + // reached. Trimming a block at a time means the rebuild it forces happens + // once every scrollbackTrimChunk lines rather than on every line, which + // keeps the amortised cost of an append constant. + scrollbackTrimChunk = 1000 +) + +// wrapLine renders one stored line as the display lines it occupies. +func (t *Textview) wrapLine(line string, width int) []string { + l := line + if !t.showTimestamps { + // Server and local messages need not have a timestamp prefix. + if _, text, ok := strings.Cut(line, "]"); ok { + l = strings.TrimSpace(text) + } + } + var wrapped []string + // A Builder keeps this linear; appending a rune at a time to a string + // reallocates once per character. + var current strings.Builder + chars := 0 + for _, ch := range l { + if chars >= width { + wrapped = append(wrapped, current.String()) + current.Reset() + chars = 0 + } + current.WriteRune(ch) + chars++ + } + if chars > 0 { + wrapped = append(wrapped, current.String()) + } + return wrapped +} + func (t *Textview) updateParsedLines() { width := t.x1 - t.x0 @@ -83,33 +125,7 @@ func (t *Textview) updateParsedLines() { parsed := make([]string, 0, len(t.Lines)) for _, line := range t.Lines { - var l = line - if !t.showTimestamps { - // Server and local messages need not have a timestamp prefix. - if _, text, ok := strings.Cut(line, "]"); ok { - l = strings.TrimSpace(text) - } - } - current := "" - chars := 0 - reader := strings.NewReader(l) - for { - if chars >= width { - parsed = append(parsed, current) - chars = 0 - current = "" - } - if reader.Len() <= 0 { - if chars > 0 { - parsed = append(parsed, current) - } - break - } - if ch, _, err := reader.ReadRune(); err == nil { - current = current + string(ch) - chars++ - } - } + parsed = append(parsed, t.wrapLine(line, width)...) } t.parsedLines = parsed t.clampCurrentLine() @@ -117,7 +133,19 @@ func (t *Textview) updateParsedLines() { func (t *Textview) AddLine(line string) { t.Lines = append(t.Lines, line) - t.updateParsedLines() + if len(t.Lines) > maxScrollbackLines { + // Trimming invalidates the wrapped buffer and forces a rebuild, so + // discard a block rather than a single line; otherwise every append + // past the cap would re-wrap the whole buffer. + keep := maxScrollbackLines - scrollbackTrimChunk + t.Lines = append(t.Lines[:0], t.Lines[len(t.Lines)-keep:]...) + t.updateParsedLines() + } else if width := t.x1 - t.x0; width > 0 { + // Wrap just the new line. Rebuilding every stored line on each append + // is what made a long-lived session stall the terminal. + t.parsedLines = append(t.parsedLines, t.wrapLine(line, width)...) + t.clampCurrentLine() + } t.uiDraw() } diff --git a/uiterm/textview_regression_test.go b/uiterm/textview_regression_test.go new file mode 100644 index 0000000..1ef6835 --- /dev/null +++ b/uiterm/textview_regression_test.go @@ -0,0 +1,116 @@ +package uiterm + +import ( + "fmt" + "strings" + "testing" +) + +// addLine appends without drawing, so these tests need no terminal. +func addLineNoDraw(t *Textview, line string) { + t.Lines = append(t.Lines, line) + if len(t.Lines) > maxScrollbackLines { + keep := maxScrollbackLines - scrollbackTrimChunk + t.Lines = append(t.Lines[:0], t.Lines[len(t.Lines)-keep:]...) + t.updateParsedLines() + return + } + if width := t.x1 - t.x0; width > 0 { + t.parsedLines = append(t.parsedLines, t.wrapLine(line, width)...) + t.clampCurrentLine() + } +} + +// Regression: AddLine used to re-wrap every stored line on each append, which +// made the cost of a session grow as the square of its length. It now wraps +// only the new line, so that incremental result must match a full rebuild. +func TestTextviewIncrementalWrapMatchesFullRebuild(t *testing.T) { + t.Parallel() + + lines := []string{ + "short [12:00:01]", + strings.Repeat("a", 200) + " [12:00:02]", + "", + "exactly-twenty-chars", + "unicode ünïcödé line with wide content [12:00:03]", + } + + incremental := &Textview{x0: 0, x1: 20, showTimestamps: true} + for _, line := range lines { + addLineNoDraw(incremental, line) + } + + full := &Textview{x0: 0, x1: 20, showTimestamps: true} + full.Lines = append([]string(nil), lines...) + full.updateParsedLines() + + if len(incremental.parsedLines) != len(full.parsedLines) { + t.Fatalf("incremental produced %d wrapped lines, full rebuild %d", + len(incremental.parsedLines), len(full.parsedLines)) + } + for i := range full.parsedLines { + if incremental.parsedLines[i] != full.parsedLines[i] { + t.Fatalf("wrapped line %d differs: incremental %q, full %q", + i, incremental.parsedLines[i], full.parsedLines[i]) + } + } +} + +// Regression: the scrollback had no cap, so a long-lived client retained every +// line it had ever displayed. +func TestTextviewScrollbackIsCapped(t *testing.T) { + t.Parallel() + + view := &Textview{x0: 0, x1: 40, showTimestamps: true} + for i := 0; i < maxScrollbackLines+500; i++ { + addLineNoDraw(view, fmt.Sprintf("line %d", i)) + } + + if len(view.Lines) > maxScrollbackLines { + t.Fatalf("expected scrollback capped at %d lines, got %d", + maxScrollbackLines, len(view.Lines)) + } + if len(view.Lines) < maxScrollbackLines-scrollbackTrimChunk { + t.Fatalf("trim discarded more than one chunk: %d lines remain", len(view.Lines)) + } + // The newest line must survive; the oldest must not. + if got := view.Lines[len(view.Lines)-1]; got != fmt.Sprintf("line %d", maxScrollbackLines+499) { + t.Fatalf("newest line was dropped, got %q", got) + } + if view.Lines[0] == "line 0" { + t.Fatal("oldest line should have been trimmed") + } + if len(view.parsedLines) != len(view.Lines) { + t.Fatalf("wrapped buffer out of sync after trim: %d wrapped, %d stored", + len(view.parsedLines), len(view.Lines)) + } +} + +// wrapLine replaced a loop that concatenated one rune at a time; confirm the +// wrapping itself is unchanged for the boundary cases. +func TestTextviewWrapLineBoundaries(t *testing.T) { + t.Parallel() + + view := &Textview{showTimestamps: true} + for _, tc := range []struct { + line string + width int + want []string + }{ + {"", 5, nil}, + {"abc", 5, []string{"abc"}}, + {"abcde", 5, []string{"abcde"}}, + {"abcdef", 5, []string{"abcde", "f"}}, + {"abcdeabcde", 5, []string{"abcde", "abcde"}}, + } { + got := view.wrapLine(tc.line, tc.width) + if len(got) != len(tc.want) { + t.Fatalf("wrapLine(%q, %d) = %q, want %q", tc.line, tc.width, got, tc.want) + } + for i := range tc.want { + if got[i] != tc.want[i] { + t.Fatalf("wrapLine(%q, %d) = %q, want %q", tc.line, tc.width, got, tc.want) + } + } + } +} diff --git a/uiterm/tree.go b/uiterm/tree.go index 0990c72..2ec28c0 100644 --- a/uiterm/tree.go +++ b/uiterm/tree.go @@ -89,10 +89,10 @@ func (t *Tree) rebuild(preserveActive bool, sameItem func(previous, current Tree previousLine := t.activeLine lines := []renderedTreeItem{} for _, item := range t.Generator(nil) { - children := t.rebuild_rec(item, 0) - if children != nil { - lines = append(lines, children...) + if len(lines) >= maxTreeLines { + break } + lines = t.rebuild_rec(lines, item, 0) } t.lines = lines if preserveActive { @@ -113,21 +113,31 @@ func (t *Tree) rebuild(preserveActive bool, sameItem func(previous, current Tree } } -func (t *Tree) rebuild_rec(parent TreeItem, level int) []renderedTreeItem { - if parent == nil { - return nil - } - lines := []renderedTreeItem{ - renderedTreeItem{ - Level: level, - Item: parent, - }, +// A server is free to describe a channel graph in which a channel is its own +// ancestor. The generator follows parent/child links literally, so without +// these limits such a graph recurses until the process is out of memory. +// Real trees are orders of magnitude smaller than either bound. +const ( + maxTreeDepth = 64 + maxTreeLines = 100000 +) + +// rebuild_rec appends parent and its descendants to lines. Accumulating into +// one slice keeps maxTreeLines a budget for the whole tree rather than for +// each level, and avoids building a slice per node. +func (t *Tree) rebuild_rec(lines []renderedTreeItem, parent TreeItem, level int) []renderedTreeItem { + if parent == nil || level >= maxTreeDepth || len(lines) >= maxTreeLines { + return lines } + lines = append(lines, renderedTreeItem{ + Level: level, + Item: parent, + }) for _, item := range t.Generator(parent) { - children := t.rebuild_rec(item, level+1) - if children != nil { - lines = append(lines, children...) + if len(lines) >= maxTreeLines { + break } + lines = t.rebuild_rec(lines, item, level+1) } return lines } diff --git a/uiterm/tree_regression_test.go b/uiterm/tree_regression_test.go new file mode 100644 index 0000000..15bacea --- /dev/null +++ b/uiterm/tree_regression_test.go @@ -0,0 +1,65 @@ +package uiterm + +import ( + "testing" + "time" +) + +// cyclicItem reports itself as its own child, standing in for a channel graph +// in which a channel is its own ancestor. +type cyclicItem struct{ name string } + +func (i *cyclicItem) String() string { return i.name } + +func (i *cyclicItem) TreeItemStyle(fg, bg Attribute, active bool) (Attribute, Attribute) { + return fg, bg +} + +// Regression: rebuild_rec followed parent/child links with no depth limit, so +// a cyclic channel graph recursed until the process ran out of memory. A +// rebuild must now terminate and stay bounded. +func TestTreeRebuildTerminatesOnCyclicGraph(t *testing.T) { + t.Parallel() + + self := &cyclicItem{name: "loop"} + tree := Tree{ + Generator: func(item TreeItem) []TreeItem { + return []TreeItem{self} + }, + } + + done := make(chan struct{}) + go func() { + tree.rebuild(false, nil) + close(done) + }() + + select { + case <-done: + case <-timeoutAfterSeconds(10): + t.Fatal("rebuild did not terminate on a cyclic tree") + } + + if len(tree.lines) == 0 { + t.Fatal("expected the bounded rebuild to still produce lines") + } + if len(tree.lines) > maxTreeLines { + t.Fatalf("rebuild produced %d lines, above the %d cap", + len(tree.lines), maxTreeLines) + } + for _, line := range tree.lines { + if line.Level >= maxTreeDepth { + t.Fatalf("rebuild recursed to level %d, at or past the %d cap", + line.Level, maxTreeDepth) + } + } +} + +func timeoutAfterSeconds(n int) <-chan struct{} { + ch := make(chan struct{}) + go func() { + time.Sleep(time.Duration(n) * time.Second) + close(ch) + }() + return ch +}