report configuration failures instead of crashing

Return errors from the save path rather than panicking.
Every write panicked on failure, so a read-only or missing
configuration directory killed a running client. The parent directory
is created when absent, and callers now show the failure in the output
window and carry on.

Write through an unpredictable temporary file.
The old fixed ".tmp" name next to the configuration was a symlink
target an attacker could plant in advance.

Serialize configuration reads and writes.
Hotkey handlers, the audio thread, and the connection callbacks all
touch the same structure, so saves could interleave with updates.

Parse addresses with SplitHostPort.
Splitting on every colon broke IPv6 addresses and panicked outright on
an address with no port. Both now fall back to Mumble's default port.

Fail immediately when an explicitly requested config file is missing
or is not a regular file.
Silently falling back to defaults hid a mistyped -config path.

Supply defaults for the clear-output and scroll-to-top and -bottom
hotkeys.
The UI registered listeners for them but the configuration never
filled the keys in, so the bindings were nil and the keys did nothing.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
Brandon McGinty
2026-08-20 14:42:52 -04:00
co-authored by Claude Opus 5
parent 7e1faba06b
commit 17173b779b
7 changed files with 235 additions and 32 deletions
+7 -5
View File
@@ -115,10 +115,10 @@ func (b *Barnard) TreeItemKeyPress(ui *uiterm.Ui, tree *uiterm.Tree, item uiterm
users := makeUsersArray(treeItem.Channel.Users) users := makeUsersArray(treeItem.Channel.Users)
for _, u := range users { for _, u := range users {
// Explicitly set user mute state to match channel state // Explicitly set user mute state to match channel state
if channelWillBeMuted && !u.LocallyMuted() { if channelWillBeMuted != u.LocallyMuted() {
b.UserConfig.ToggleMute(u) if err := b.UserConfig.ToggleMute(u); err != nil {
} else if !channelWillBeMuted && u.LocallyMuted() { b.AddOutputLine("Mute: could not save setting: " + err.Error())
b.UserConfig.ToggleMute(u) }
} }
if source := u.AudioSource(); source != nil { if source := u.AudioSource(); source != nil {
@@ -158,7 +158,9 @@ func (b *Barnard) TreeItemKeyPress(ui *uiterm.Ui, tree *uiterm.Tree, item uiterm
if treeItem.User != nil { if treeItem.User != nil {
if key == *b.Hotkeys.MuteToggle { if key == *b.Hotkeys.MuteToggle {
// Toggle mute for single user // Toggle mute for single user
b.UserConfig.ToggleMute(treeItem.User) if err := b.UserConfig.ToggleMute(treeItem.User); err != nil {
b.AddOutputLine("Mute: could not save setting: " + err.Error())
}
if source := treeItem.User.AudioSource(); source != nil { if source := treeItem.User.AudioSource(); source != nil {
if treeItem.User.LocallyMuted() { if treeItem.User.LocallyMuted() {
source.SetGain(0) source.SetGain(0)
+91 -17
View File
@@ -6,12 +6,16 @@ import (
"git.stormux.org/storm/barnard/uiterm" "git.stormux.org/storm/barnard/uiterm"
"github.com/pelletier/go-toml/v2" "github.com/pelletier/go-toml/v2"
"io/ioutil" "io/ioutil"
"net"
"os" "os"
"path/filepath"
"strconv" "strconv"
"strings" "strings"
"sync"
) )
type Config struct { type Config struct {
mu sync.Mutex
config *exportableConfig config *exportableConfig
fn string fn string
} }
@@ -45,20 +49,40 @@ type eUser struct {
LocallyMuted bool // Changed from Muted to LocallyMuted to match User struct LocallyMuted bool // Changed from Muted to LocallyMuted to match User struct
} }
func (c *Config) SaveConfig() { // SaveConfig atomically replaces the persisted configuration. Errors are
var data []byte // returned so an unavailable directory cannot crash the client.
func (c *Config) SaveConfig() error {
c.mu.Lock()
defer c.mu.Unlock()
return c.saveConfigLocked()
}
func (c *Config) saveConfigLocked() error {
if err := os.MkdirAll(filepath.Dir(c.fn), 0700); err != nil {
return err
}
data, err := toml.Marshal(c.config) data, err := toml.Marshal(c.config)
if err != nil { if err != nil {
panic(err) return err
} }
err = ioutil.WriteFile(c.fn+".tmp", data, 0600) file, err := os.CreateTemp(filepath.Dir(c.fn), filepath.Base(c.fn)+".tmp-")
if err != nil { if err != nil {
panic(err) return err
} }
err = os.Rename(c.fn+".tmp", c.fn) tmp := file.Name()
if err != nil { defer os.Remove(tmp)
panic(err) if err := file.Chmod(0600); err != nil {
file.Close()
return err
} }
if _, err := file.Write(data); err != nil {
file.Close()
return err
}
if err := file.Close(); err != nil {
return err
}
return os.Rename(tmp, c.fn)
} }
func key(k uiterm.Key) *uiterm.Key { func key(k uiterm.Key) *uiterm.Key {
@@ -78,8 +102,11 @@ func (c *Config) LoadConfig() {
Exit: key(uiterm.KeyF10), Exit: key(uiterm.KeyF10),
ToggleTimestamps: key(uiterm.KeyF3), ToggleTimestamps: key(uiterm.KeyF3),
SwitchViews: key(uiterm.KeyTab), SwitchViews: key(uiterm.KeyTab),
ClearOutput: key(uiterm.KeyCtrlL),
ScrollUp: key(uiterm.KeyPgup), ScrollUp: key(uiterm.KeyPgup),
ScrollDown: key(uiterm.KeyPgdn), ScrollDown: key(uiterm.KeyPgdn),
ScrollToTop: key(uiterm.KeyHome),
ScrollToBottom: key(uiterm.KeyEnd),
AdminMenu: key(uiterm.KeyF11), AdminMenu: key(uiterm.KeyF11),
NoiseSuppressionToggle: key(uiterm.KeyF9), NoiseSuppressionToggle: key(uiterm.KeyF9),
} }
@@ -156,8 +183,11 @@ func (c *Config) ensureHotkeys() {
Exit: key(uiterm.KeyF10), Exit: key(uiterm.KeyF10),
ToggleTimestamps: key(uiterm.KeyF3), ToggleTimestamps: key(uiterm.KeyF3),
SwitchViews: key(uiterm.KeyTab), SwitchViews: key(uiterm.KeyTab),
ClearOutput: key(uiterm.KeyCtrlL),
ScrollUp: key(uiterm.KeyPgup), ScrollUp: key(uiterm.KeyPgup),
ScrollDown: key(uiterm.KeyPgdn), ScrollDown: key(uiterm.KeyPgdn),
ScrollToTop: key(uiterm.KeyHome),
ScrollToBottom: key(uiterm.KeyEnd),
AdminMenu: key(uiterm.KeyF11), AdminMenu: key(uiterm.KeyF11),
NoiseSuppressionToggle: key(uiterm.KeyF9), NoiseSuppressionToggle: key(uiterm.KeyF9),
} }
@@ -189,12 +219,21 @@ func (c *Config) ensureHotkeys() {
if hotkeys.SwitchViews == nil { if hotkeys.SwitchViews == nil {
hotkeys.SwitchViews = defaults.SwitchViews hotkeys.SwitchViews = defaults.SwitchViews
} }
if hotkeys.ClearOutput == nil {
hotkeys.ClearOutput = defaults.ClearOutput
}
if hotkeys.ScrollUp == nil { if hotkeys.ScrollUp == nil {
hotkeys.ScrollUp = defaults.ScrollUp hotkeys.ScrollUp = defaults.ScrollUp
} }
if hotkeys.ScrollDown == nil { if hotkeys.ScrollDown == nil {
hotkeys.ScrollDown = defaults.ScrollDown hotkeys.ScrollDown = defaults.ScrollDown
} }
if hotkeys.ScrollToTop == nil {
hotkeys.ScrollToTop = defaults.ScrollToTop
}
if hotkeys.ScrollToBottom == nil {
hotkeys.ScrollToBottom = defaults.ScrollToBottom
}
if hotkeys.AdminMenu == nil { if hotkeys.AdminMenu == nil {
hotkeys.AdminMenu = defaults.AdminMenu hotkeys.AdminMenu = defaults.AdminMenu
} }
@@ -250,14 +289,18 @@ func (c *Config) findUser(address string, username string) *eUser {
return t return t
} }
func (c *Config) ToggleMute(u *gumble.User) { func (c *Config) ToggleMute(u *gumble.User) error {
c.mu.Lock()
defer c.mu.Unlock()
j := c.findUser(u.GetClient().Config.Address, u.Name) j := c.findUser(u.GetClient().Config.Address, u.Name)
j.LocallyMuted = !j.LocallyMuted j.LocallyMuted = !j.LocallyMuted
u.SetLocallyMuted(j.LocallyMuted) u.SetLocallyMuted(j.LocallyMuted)
c.SaveConfig() return c.saveConfigLocked()
} }
func (c *Config) SetMicVolume(v float32) { func (c *Config) SetMicVolume(v float32) {
c.mu.Lock()
defer c.mu.Unlock()
t := float32(v) t := float32(v)
c.config.MicVolume = &t c.config.MicVolume = &t
} }
@@ -298,18 +341,24 @@ func (c *Config) GetCertificate() *string {
} }
func (c *Config) GetNoiseSuppressionEnabled() bool { func (c *Config) GetNoiseSuppressionEnabled() bool {
c.mu.Lock()
defer c.mu.Unlock()
if c.config.NoiseSuppressionEnabled == nil { if c.config.NoiseSuppressionEnabled == nil {
return false return false
} }
return *c.config.NoiseSuppressionEnabled return *c.config.NoiseSuppressionEnabled
} }
func (c *Config) SetNoiseSuppressionEnabled(enabled bool) { func (c *Config) SetNoiseSuppressionEnabled(enabled bool) error {
c.mu.Lock()
defer c.mu.Unlock()
c.config.NoiseSuppressionEnabled = &enabled c.config.NoiseSuppressionEnabled = &enabled
c.SaveConfig() return c.saveConfigLocked()
} }
func (c *Config) GetRecordingFormat() string { func (c *Config) GetRecordingFormat() string {
c.mu.Lock()
defer c.mu.Unlock()
if c.config.RecordingFormat == nil { if c.config.RecordingFormat == nil {
return "flac" return "flac"
} }
@@ -317,6 +366,8 @@ func (c *Config) GetRecordingFormat() string {
} }
func (c *Config) GetRecordingDirectory() string { func (c *Config) GetRecordingDirectory() string {
c.mu.Lock()
defer c.mu.Unlock()
if c.config.RecordingDirectory == nil { if c.config.RecordingDirectory == nil {
return resolvePath("~/Audio") return resolvePath("~/Audio")
} }
@@ -324,6 +375,8 @@ func (c *Config) GetRecordingDirectory() string {
} }
func (c *Config) UpdateUser(u *gumble.User) { func (c *Config) UpdateUser(u *gumble.User) {
c.mu.Lock()
defer c.mu.Unlock()
var j *eUser var j *eUser
var uc *gumble.Client var uc *gumble.Client
uc = u.GetClient() uc = u.GetClient()
@@ -339,6 +392,8 @@ func (c *Config) UpdateUser(u *gumble.User) {
} }
func (c *Config) UpdateConfig(u *gumble.User) { func (c *Config) UpdateConfig(u *gumble.User) {
c.mu.Lock()
defer c.mu.Unlock()
var j *eUser var j *eUser
j = c.findUser(u.GetClient().Config.Address, u.Name) j = c.findUser(u.GetClient().Config.Address, u.Name)
j.Boost = u.Boost() j.Boost = u.Boost()
@@ -346,6 +401,20 @@ func (c *Config) UpdateConfig(u *gumble.User) {
j.LocallyMuted = u.LocallyMuted() // Save LocallyMuted state to config j.LocallyMuted = u.LocallyMuted() // Save LocallyMuted state to config
} }
// RequireConfigFile verifies that an explicitly requested configuration file
// exists and is a regular file. The default configuration remains optional.
func RequireConfigFile(fn string) error {
path := resolvePath(fn)
info, err := os.Stat(path)
if err != nil {
return fmt.Errorf("config file %q: %w", path, err)
}
if !info.Mode().IsRegular() {
return fmt.Errorf("config file %q is not a regular file", path)
}
return nil
}
func NewConfig(fn *string) *Config { func NewConfig(fn *string) *Config {
var c *Config var c *Config
c = &Config{} c = &Config{}
@@ -367,7 +436,7 @@ func readFile(path string) []byte {
func fileExists(path string) bool { func fileExists(path string) bool {
info, err := os.Stat(path) info, err := os.Stat(path)
if os.IsNotExist(err) { if err != nil {
return false return false
} }
return !info.IsDir() return !info.IsDir()
@@ -389,11 +458,16 @@ func resolvePath(path string) string {
} }
func makeHostPort(addr string) (string, int) { func makeHostPort(addr string) (string, int) {
parts := strings.Split(addr, ":") // SplitHostPort correctly handles bracketed IPv6. Invalid or portless
host := parts[0] // addresses stay usable as a host with Mumble's default port instead of
port, err := strconv.Atoi(parts[1]) // crashing configuration operations.
host, portText, err := net.SplitHostPort(addr)
if err != nil { if err != nil {
panic(err) return strings.Trim(addr, "[]"), 64738
}
port, err := strconv.Atoi(portText)
if err != nil || port < 1 || port > 65535 {
return host, 64738
} }
return host, port return host, port
} }
+60
View File
@@ -0,0 +1,60 @@
package config
import (
"os"
"path/filepath"
"sync"
"testing"
)
func TestSaveConfigCreatesMissingParentDirectory(t *testing.T) {
parent := filepath.Join(t.TempDir(), "missing")
path := filepath.Join(parent, "barnard.toml")
cfg := NewConfig(&path)
if err := cfg.SaveConfig(); err != nil {
t.Fatal(err)
}
info, err := os.Stat(parent)
if err != nil {
t.Fatal(err)
}
if info.Mode().Perm()&0077 != 0 {
t.Fatalf("parent directory permissions = %o, want no group or other access", info.Mode().Perm())
}
}
func TestSaveConfigDoesNotUsePredictableTemporaryPath(t *testing.T) {
path := filepath.Join(t.TempDir(), "barnard.toml")
legacyTemp := path + ".tmp"
if err := os.WriteFile(legacyTemp, []byte("sentinel"), 0600); err != nil {
t.Fatal(err)
}
cfg := NewConfig(&path)
if err := cfg.SaveConfig(); err != nil {
t.Fatal(err)
}
contents, err := os.ReadFile(legacyTemp)
if err != nil {
t.Fatal(err)
}
if string(contents) != "sentinel" {
t.Fatalf("predictable temporary file was modified: %q", contents)
}
}
func TestConcurrentConfigurationUpdatesAndWrites(t *testing.T) {
path := filepath.Join(t.TempDir(), "barnard.toml")
cfg := NewConfig(&path)
var wg sync.WaitGroup
for i := 0; i < 20; i++ {
wg.Add(1)
go func(enabled bool) {
defer wg.Done()
cfg.SetNoiseSuppressionEnabled(enabled)
if err := cfg.SaveConfig(); err != nil {
t.Errorf("SaveConfig: %v", err)
}
}(i%2 == 0)
}
wg.Wait()
}
+47 -1
View File
@@ -8,6 +8,25 @@ import (
"git.stormux.org/storm/barnard/uiterm" "git.stormux.org/storm/barnard/uiterm"
) )
// Regression: an explicit -config path silently fell back to in-memory
// defaults, then overwrote the intended file on exit.
func TestRequireConfigFileRejectsMissingExplicitPath(t *testing.T) {
missing := filepath.Join(t.TempDir(), "missing.toml")
if err := RequireConfigFile(missing); err == nil {
t.Fatal("missing explicit config was accepted")
}
}
func TestRequireConfigFileRejectsNonRegularFile(t *testing.T) {
path := filepath.Join(t.TempDir(), "config.fifo")
if err := os.Mkdir(path, 0700); err != nil {
t.Fatal(err)
}
if err := RequireConfigFile(path); err == nil {
t.Fatal("directory was accepted as an explicit config file")
}
}
func TestConfigBackfillsRecordingDefaults(t *testing.T) { func TestConfigBackfillsRecordingDefaults(t *testing.T) {
dir := t.TempDir() dir := t.TempDir()
configPath := filepath.Join(dir, "barnard.toml") configPath := filepath.Join(dir, "barnard.toml")
@@ -35,8 +54,35 @@ func TestConfigBackfillsRecordingDefaults(t *testing.T) {
if got := *cfg.GetHotkeys().AdminMenu; got != uiterm.KeyF11 { if got := *cfg.GetHotkeys().AdminMenu; got != uiterm.KeyF11 {
t.Fatalf("expected admin menu f11, got %s", got) t.Fatalf("expected admin menu f11, got %s", got)
} }
for name, got := range map[string]*uiterm.Key{
"clear output": cfg.GetHotkeys().ClearOutput,
"scroll to top": cfg.GetHotkeys().ScrollToTop,
"scroll to bottom": cfg.GetHotkeys().ScrollToBottom,
} {
if got == nil {
t.Fatalf("expected %s hotkey to be backfilled", name)
}
}
if got := *cfg.GetHotkeys().ClearOutput; got != uiterm.KeyCtrlL {
t.Fatalf("expected clear output ctrl_l, got %s", got)
}
if got := *cfg.GetHotkeys().ScrollToTop; got != uiterm.KeyHome {
t.Fatalf("expected scroll to top home, got %s", got)
}
if got := *cfg.GetHotkeys().ScrollToBottom; got != uiterm.KeyEnd {
t.Fatalf("expected scroll to bottom end, got %s", got)
}
}
func TestMakeHostPortHandlesIPv6AndMalformedAddress(t *testing.T) {
host, port := makeHostPort("[2001:db8::1]:64739")
if host != "2001:db8::1" || port != 64739 {
t.Fatalf("got %q:%d", host, port)
}
host, port = makeHostPort("not-a-host-port")
if host != "not-a-host-port" || port != 64738 {
t.Fatalf("got %q:%d", host, port)
}
} }
func TestConfigUsesHomeEnvironmentForDefaultPath(t *testing.T) { func TestConfigUsesHomeEnvironmentForDefaultPath(t *testing.T) {
dir := t.TempDir() dir := t.TempDir()
t.Setenv("HOME", dir) t.Setenv("HOME", dir)
+18 -5
View File
@@ -114,6 +114,8 @@ func main() {
fifo := flag.String("fifo", "", "path of a FIFO from which to read commands") fifo := flag.String("fifo", "", "path of a FIFO from which to read commands")
serverSet := false serverSet := false
usernameSet := false usernameSet := false
configSet := false
certificateSet := false
buffers := flag.Int("buffers", 16, "number of audio buffers to use") buffers := flag.Int("buffers", 16, "number of audio buffers to use")
audioInterval := flag.Int("audio-interval", 10, "outgoing audio packet duration in ms (10, 20, 40, or 60)") audioInterval := flag.Int("audio-interval", 10, "outgoing audio packet duration in ms (10, 20, 40, or 60)")
jitterBuffer := flag.Int("jitter-buffer", 40, "incoming per-user audio buffer in ms (0, 20, 40, or 60)") jitterBuffer := flag.Int("jitter-buffer", 40, "incoming per-user audio buffer in ms (0, 20, 40, or 60)")
@@ -165,19 +167,24 @@ func main() {
}() }()
} }
userConfig := config.NewConfig(cfgfn)
certificateSet := false
flag.CommandLine.Visit(func(theFlag *flag.Flag) { flag.CommandLine.Visit(func(theFlag *flag.Flag) {
switch theFlag.Name { switch theFlag.Name {
case "server": case "server":
serverSet = true serverSet = true
case "username": case "username":
usernameSet = true usernameSet = true
case "config":
configSet = true
case "certificate": case "certificate":
certificateSet = true certificateSet = true
} }
}) })
if configSet {
if err := config.RequireConfigFile(*cfgfn); err != nil {
handle_raw_error(err)
}
}
userConfig := config.NewConfig(cfgfn)
if !serverSet { if !serverSet {
server = userConfig.GetDefaultServer() server = userConfig.GetDefaultServer()
@@ -229,13 +236,19 @@ func main() {
b.Config.IncomingAudioBuffer = selectedJitterBuffer b.Config.IncomingAudioBuffer = selectedJitterBuffer
b.Hotkeys = b.UserConfig.GetHotkeys() b.Hotkeys = b.UserConfig.GetHotkeys()
b.UserConfig.SaveConfig() if err := b.UserConfig.SaveConfig(); err != nil {
fmt.Fprintf(os.Stderr, "could not save configuration: %s\n", err)
os.Exit(1)
}
// Configure noise suppression // Configure noise suppression
enabled := b.UserConfig.GetNoiseSuppressionEnabled() enabled := b.UserConfig.GetNoiseSuppressionEnabled()
if *noiseSuppressionEnabled { if *noiseSuppressionEnabled {
enabled = true enabled = true
b.UserConfig.SetNoiseSuppressionEnabled(true) if err := b.UserConfig.SetNoiseSuppressionEnabled(true); err != nil {
fmt.Fprintf(os.Stderr, "could not save configuration: %s\n", err)
os.Exit(1)
}
} }
b.NoiseSuppressor.SetEnabled(enabled) b.NoiseSuppressor.SetEnabled(enabled)
+6 -2
View File
@@ -99,7 +99,9 @@ func (b *Barnard) OnTimestampToggle(ui *uiterm.Ui, key uiterm.Key) {
func (b *Barnard) OnNoiseSuppressionToggle(ui *uiterm.Ui, key uiterm.Key) { func (b *Barnard) OnNoiseSuppressionToggle(ui *uiterm.Ui, key uiterm.Key) {
enabled := !b.UserConfig.GetNoiseSuppressionEnabled() enabled := !b.UserConfig.GetNoiseSuppressionEnabled()
b.UserConfig.SetNoiseSuppressionEnabled(enabled) if err := b.UserConfig.SetNoiseSuppressionEnabled(enabled); err != nil {
b.AddOutputLine("Noise suppression: could not save setting: " + err.Error())
}
b.NoiseSuppressor.SetEnabled(enabled) b.NoiseSuppressor.SetEnabled(enabled)
if enabled { if enabled {
@@ -161,7 +163,9 @@ func (b *Barnard) CommandMicDown(ui *uiterm.Ui, cmd string) {
func (b *Barnard) CommandNoiseSuppressionToggle(ui *uiterm.Ui, cmd string) { func (b *Barnard) CommandNoiseSuppressionToggle(ui *uiterm.Ui, cmd string) {
enabled := !b.UserConfig.GetNoiseSuppressionEnabled() enabled := !b.UserConfig.GetNoiseSuppressionEnabled()
b.UserConfig.SetNoiseSuppressionEnabled(enabled) if err := b.UserConfig.SetNoiseSuppressionEnabled(enabled); err != nil {
b.AddOutputLine("Noise suppression: could not save setting: " + err.Error())
}
b.NoiseSuppressor.SetEnabled(enabled) b.NoiseSuppressor.SetEnabled(enabled)
if enabled { if enabled {
+6 -2
View File
@@ -63,7 +63,9 @@ func (b *Barnard) changeVolume(users []*gumble.User, change float32) {
} }
b.UserConfig.UpdateConfig(u) b.UserConfig.UpdateConfig(u)
} }
b.UserConfig.SaveConfig() if err := b.UserConfig.SaveConfig(); err != nil {
b.AddOutputLine("Volume: could not save setting: " + err.Error())
}
} }
func (b *Barnard) resetVolume(users []*gumble.User) { func (b *Barnard) resetVolume(users []*gumble.User) {
@@ -80,7 +82,9 @@ func (b *Barnard) resetVolume(users []*gumble.User) {
} }
b.UserConfig.UpdateConfig(u) b.UserConfig.UpdateConfig(u)
} }
b.UserConfig.SaveConfig() if err := b.UserConfig.SaveConfig(); err != nil {
b.AddOutputLine("Volume: could not save setting: " + err.Error())
}
} }
func makeUsersArray(users gumble.Users) []*gumble.User { func makeUsersArray(users gumble.Users) []*gumble.User {