From 17173b779b76ebe771189d2b7c7dc8b3548cdcba Mon Sep 17 00:00:00 2001 From: Brandon McGinty Date: Thu, 20 Aug 2026 14:30:11 -0400 Subject: [PATCH] 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 --- barnard.go | 12 ++-- config/user_config.go | 108 +++++++++++++++++++++++++++----- config/user_config_save_test.go | 60 ++++++++++++++++++ config/user_config_test.go | 48 +++++++++++++- main.go | 23 +++++-- ui.go | 8 ++- ui_tree.go | 8 ++- 7 files changed, 235 insertions(+), 32 deletions(-) create mode 100644 config/user_config_save_test.go diff --git a/barnard.go b/barnard.go index 7b52299..3dac560 100644 --- a/barnard.go +++ b/barnard.go @@ -115,10 +115,10 @@ func (b *Barnard) TreeItemKeyPress(ui *uiterm.Ui, tree *uiterm.Tree, item uiterm users := makeUsersArray(treeItem.Channel.Users) for _, u := range users { // Explicitly set user mute state to match channel state - if channelWillBeMuted && !u.LocallyMuted() { - b.UserConfig.ToggleMute(u) - } else if !channelWillBeMuted && u.LocallyMuted() { - b.UserConfig.ToggleMute(u) + if channelWillBeMuted != u.LocallyMuted() { + if err := b.UserConfig.ToggleMute(u); err != nil { + b.AddOutputLine("Mute: could not save setting: " + err.Error()) + } } 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 key == *b.Hotkeys.MuteToggle { // 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 treeItem.User.LocallyMuted() { source.SetGain(0) diff --git a/config/user_config.go b/config/user_config.go index 5451565..3fdfdc4 100644 --- a/config/user_config.go +++ b/config/user_config.go @@ -6,12 +6,16 @@ import ( "git.stormux.org/storm/barnard/uiterm" "github.com/pelletier/go-toml/v2" "io/ioutil" + "net" "os" + "path/filepath" "strconv" "strings" + "sync" ) type Config struct { + mu sync.Mutex config *exportableConfig fn string } @@ -45,20 +49,40 @@ type eUser struct { LocallyMuted bool // Changed from Muted to LocallyMuted to match User struct } -func (c *Config) SaveConfig() { - var data []byte +// SaveConfig atomically replaces the persisted configuration. Errors are +// 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) 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 { - panic(err) + return err } - err = os.Rename(c.fn+".tmp", c.fn) - if err != nil { - panic(err) + tmp := file.Name() + defer os.Remove(tmp) + 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 { @@ -78,8 +102,11 @@ func (c *Config) LoadConfig() { Exit: key(uiterm.KeyF10), ToggleTimestamps: key(uiterm.KeyF3), SwitchViews: key(uiterm.KeyTab), + ClearOutput: key(uiterm.KeyCtrlL), ScrollUp: key(uiterm.KeyPgup), ScrollDown: key(uiterm.KeyPgdn), + ScrollToTop: key(uiterm.KeyHome), + ScrollToBottom: key(uiterm.KeyEnd), AdminMenu: key(uiterm.KeyF11), NoiseSuppressionToggle: key(uiterm.KeyF9), } @@ -156,8 +183,11 @@ func (c *Config) ensureHotkeys() { Exit: key(uiterm.KeyF10), ToggleTimestamps: key(uiterm.KeyF3), SwitchViews: key(uiterm.KeyTab), + ClearOutput: key(uiterm.KeyCtrlL), ScrollUp: key(uiterm.KeyPgup), ScrollDown: key(uiterm.KeyPgdn), + ScrollToTop: key(uiterm.KeyHome), + ScrollToBottom: key(uiterm.KeyEnd), AdminMenu: key(uiterm.KeyF11), NoiseSuppressionToggle: key(uiterm.KeyF9), } @@ -189,12 +219,21 @@ func (c *Config) ensureHotkeys() { if hotkeys.SwitchViews == nil { hotkeys.SwitchViews = defaults.SwitchViews } + if hotkeys.ClearOutput == nil { + hotkeys.ClearOutput = defaults.ClearOutput + } if hotkeys.ScrollUp == nil { hotkeys.ScrollUp = defaults.ScrollUp } if hotkeys.ScrollDown == nil { hotkeys.ScrollDown = defaults.ScrollDown } + if hotkeys.ScrollToTop == nil { + hotkeys.ScrollToTop = defaults.ScrollToTop + } + if hotkeys.ScrollToBottom == nil { + hotkeys.ScrollToBottom = defaults.ScrollToBottom + } if hotkeys.AdminMenu == nil { hotkeys.AdminMenu = defaults.AdminMenu } @@ -250,14 +289,18 @@ func (c *Config) findUser(address string, username string) *eUser { 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.LocallyMuted = !j.LocallyMuted u.SetLocallyMuted(j.LocallyMuted) - c.SaveConfig() + return c.saveConfigLocked() } func (c *Config) SetMicVolume(v float32) { + c.mu.Lock() + defer c.mu.Unlock() t := float32(v) c.config.MicVolume = &t } @@ -298,18 +341,24 @@ func (c *Config) GetCertificate() *string { } func (c *Config) GetNoiseSuppressionEnabled() bool { + c.mu.Lock() + defer c.mu.Unlock() if c.config.NoiseSuppressionEnabled == nil { return false } 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.SaveConfig() + return c.saveConfigLocked() } func (c *Config) GetRecordingFormat() string { + c.mu.Lock() + defer c.mu.Unlock() if c.config.RecordingFormat == nil { return "flac" } @@ -317,6 +366,8 @@ func (c *Config) GetRecordingFormat() string { } func (c *Config) GetRecordingDirectory() string { + c.mu.Lock() + defer c.mu.Unlock() if c.config.RecordingDirectory == nil { return resolvePath("~/Audio") } @@ -324,6 +375,8 @@ func (c *Config) GetRecordingDirectory() string { } func (c *Config) UpdateUser(u *gumble.User) { + c.mu.Lock() + defer c.mu.Unlock() var j *eUser var uc *gumble.Client uc = u.GetClient() @@ -339,6 +392,8 @@ func (c *Config) UpdateUser(u *gumble.User) { } func (c *Config) UpdateConfig(u *gumble.User) { + c.mu.Lock() + defer c.mu.Unlock() var j *eUser j = c.findUser(u.GetClient().Config.Address, u.Name) j.Boost = u.Boost() @@ -346,6 +401,20 @@ func (c *Config) UpdateConfig(u *gumble.User) { 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 { var c *Config c = &Config{} @@ -367,7 +436,7 @@ func readFile(path string) []byte { func fileExists(path string) bool { info, err := os.Stat(path) - if os.IsNotExist(err) { + if err != nil { return false } return !info.IsDir() @@ -389,11 +458,16 @@ func resolvePath(path string) string { } func makeHostPort(addr string) (string, int) { - parts := strings.Split(addr, ":") - host := parts[0] - port, err := strconv.Atoi(parts[1]) + // SplitHostPort correctly handles bracketed IPv6. Invalid or portless + // addresses stay usable as a host with Mumble's default port instead of + // crashing configuration operations. + host, portText, err := net.SplitHostPort(addr) 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 } diff --git a/config/user_config_save_test.go b/config/user_config_save_test.go new file mode 100644 index 0000000..6fee7ad --- /dev/null +++ b/config/user_config_save_test.go @@ -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() +} diff --git a/config/user_config_test.go b/config/user_config_test.go index a82d078..2b01c81 100644 --- a/config/user_config_test.go +++ b/config/user_config_test.go @@ -8,6 +8,25 @@ import ( "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) { dir := t.TempDir() configPath := filepath.Join(dir, "barnard.toml") @@ -35,8 +54,35 @@ func TestConfigBackfillsRecordingDefaults(t *testing.T) { if got := *cfg.GetHotkeys().AdminMenu; got != uiterm.KeyF11 { 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) { dir := t.TempDir() t.Setenv("HOME", dir) diff --git a/main.go b/main.go index 57381e2..f11e015 100644 --- a/main.go +++ b/main.go @@ -114,6 +114,8 @@ func main() { fifo := flag.String("fifo", "", "path of a FIFO from which to read commands") serverSet := false usernameSet := false + configSet := false + certificateSet := false 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)") 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) { switch theFlag.Name { case "server": serverSet = true case "username": usernameSet = true + case "config": + configSet = true case "certificate": certificateSet = true } }) + if configSet { + if err := config.RequireConfigFile(*cfgfn); err != nil { + handle_raw_error(err) + } + } + userConfig := config.NewConfig(cfgfn) if !serverSet { server = userConfig.GetDefaultServer() @@ -229,13 +236,19 @@ func main() { b.Config.IncomingAudioBuffer = selectedJitterBuffer 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 enabled := b.UserConfig.GetNoiseSuppressionEnabled() if *noiseSuppressionEnabled { 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) diff --git a/ui.go b/ui.go index c83e31a..ac7b9c0 100644 --- a/ui.go +++ b/ui.go @@ -99,7 +99,9 @@ func (b *Barnard) OnTimestampToggle(ui *uiterm.Ui, key uiterm.Key) { func (b *Barnard) OnNoiseSuppressionToggle(ui *uiterm.Ui, key uiterm.Key) { 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) if enabled { @@ -161,7 +163,9 @@ func (b *Barnard) CommandMicDown(ui *uiterm.Ui, cmd string) { func (b *Barnard) CommandNoiseSuppressionToggle(ui *uiterm.Ui, cmd string) { 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) if enabled { diff --git a/ui_tree.go b/ui_tree.go index 7057777..87ca153 100644 --- a/ui_tree.go +++ b/ui_tree.go @@ -63,7 +63,9 @@ func (b *Barnard) changeVolume(users []*gumble.User, change float32) { } 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) { @@ -80,7 +82,9 @@ func (b *Barnard) resetVolume(users []*gumble.User) { } 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 {