From 5161f03f024f86137cce7c9d19f4e118334fd9a1 Mon Sep 17 00:00:00 2001 From: Audrius Butkevicius Date: Thu, 26 Jul 2018 22:14:12 +0100 Subject: [PATCH] lib/config: Fix aliased append, copy config inputs and outputs (fixes #5063) (#5069) --- lib/config/config.go | 1 + lib/config/guiconfiguration.go | 4 +++ lib/config/wrapper.go | 59 ++++++++++++++++------------------ lib/model/model_test.go | 29 +++++++++++++++++ 4 files changed, 61 insertions(+), 32 deletions(-) diff --git a/lib/config/config.go b/lib/config/config.go index 86e99140..2fcb6a74 100644 --- a/lib/config/config.go +++ b/lib/config/config.go @@ -154,6 +154,7 @@ func (cfg Configuration) Copy() Configuration { } newCfg.Options = cfg.Options.Copy() + newCfg.GUI = cfg.GUI.Copy() // DeviceIDs are values newCfg.IgnoredDevices = make([]protocol.DeviceID, len(cfg.IgnoredDevices)) diff --git a/lib/config/guiconfiguration.go b/lib/config/guiconfiguration.go index 7e87bf39..7dd24d7e 100644 --- a/lib/config/guiconfiguration.go +++ b/lib/config/guiconfiguration.go @@ -93,3 +93,7 @@ func (c GUIConfiguration) IsValidAPIKey(apiKey string) bool { return false } } + +func (c GUIConfiguration) Copy() GUIConfiguration { + return c +} diff --git a/lib/config/wrapper.go b/lib/config/wrapper.go index 4908adb2..1cbd6beb 100644 --- a/lib/config/wrapper.go +++ b/lib/config/wrapper.go @@ -142,7 +142,7 @@ func (w *Wrapper) RawCopy() Configuration { func (w *Wrapper) Replace(cfg Configuration) (Waiter, error) { w.mut.Lock() defer w.mut.Unlock() - return w.replaceLocked(cfg) + return w.replaceLocked(cfg.Copy()) } func (w *Wrapper) replaceLocked(to Configuration) (Waiter, error) { @@ -154,7 +154,7 @@ func (w *Wrapper) replaceLocked(to Configuration) (Waiter, error) { for _, sub := range w.subs { l.Debugln(sub, "verifying configuration") - if err := sub.VerifyConfiguration(from, to); err != nil { + if err := sub.VerifyConfiguration(from.Copy(), to.Copy()); err != nil { l.Debugln(sub, "rejected config:", err) return noopWaiter{}, err } @@ -164,7 +164,7 @@ func (w *Wrapper) replaceLocked(to Configuration) (Waiter, error) { w.deviceMap = nil w.folderMap = nil - return w.notifyListeners(from, to), nil + return w.notifyListeners(from.Copy(), to.Copy()), nil } func (w *Wrapper) notifyListeners(from, to Configuration) Waiter { @@ -172,7 +172,7 @@ func (w *Wrapper) notifyListeners(from, to Configuration) Waiter { wg.Add(len(w.subs)) for _, sub := range w.subs { go func(commiter Committer) { - w.notifyListener(commiter, from.Copy(), to.Copy()) + w.notifyListener(commiter, from, to) wg.Done() }(sub) } @@ -187,15 +187,14 @@ func (w *Wrapper) notifyListener(sub Committer, from, to Configuration) { } } -// Devices returns a map of devices. Device structures should not be changed, -// other than for the purpose of updating via SetDevice(). +// Devices returns a map of devices. func (w *Wrapper) Devices() map[protocol.DeviceID]DeviceConfiguration { w.mut.Lock() defer w.mut.Unlock() if w.deviceMap == nil { w.deviceMap = make(map[protocol.DeviceID]DeviceConfiguration, len(w.cfg.Devices)) for _, dev := range w.cfg.Devices { - w.deviceMap[dev.DeviceID] = dev + w.deviceMap[dev.DeviceID] = dev.Copy() } } return w.deviceMap @@ -213,13 +212,13 @@ func (w *Wrapper) SetDevices(devs []DeviceConfiguration) (Waiter, error) { replaced = false for newIndex := range newCfg.Devices { if newCfg.Devices[newIndex].DeviceID == devs[oldIndex].DeviceID { - newCfg.Devices[newIndex] = devs[oldIndex] + newCfg.Devices[newIndex] = devs[oldIndex].Copy() replaced = true break } } if !replaced { - newCfg.Devices = append(newCfg.Devices, devs[oldIndex]) + newCfg.Devices = append(newCfg.Devices, devs[oldIndex].Copy()) } } @@ -238,19 +237,14 @@ func (w *Wrapper) RemoveDevice(id protocol.DeviceID) (Waiter, error) { defer w.mut.Unlock() newCfg := w.cfg.Copy() - removed := false for i := range newCfg.Devices { if newCfg.Devices[i].DeviceID == id { newCfg.Devices = append(newCfg.Devices[:i], newCfg.Devices[i+1:]...) - removed = true - break + return w.replaceLocked(newCfg) } } - if !removed { - return noopWaiter{}, nil - } - return w.replaceLocked(newCfg) + return noopWaiter{}, nil } // Folders returns a map of folders. Folder structures should not be changed, @@ -261,7 +255,7 @@ func (w *Wrapper) Folders() map[string]FolderConfiguration { if w.folderMap == nil { w.folderMap = make(map[string]FolderConfiguration, len(w.cfg.Folders)) for _, fld := range w.cfg.Folders { - w.folderMap[fld.ID] = fld + w.folderMap[fld.ID] = fld.Copy() } } return w.folderMap @@ -271,7 +265,7 @@ func (w *Wrapper) Folders() map[string]FolderConfiguration { func (w *Wrapper) FolderList() []FolderConfiguration { w.mut.Lock() defer w.mut.Unlock() - return append([]FolderConfiguration(nil), w.cfg.Folders...) + return w.cfg.Copy().Folders } // SetFolder adds a new folder to the configuration, or overwrites an existing @@ -281,17 +275,15 @@ func (w *Wrapper) SetFolder(fld FolderConfiguration) (Waiter, error) { defer w.mut.Unlock() newCfg := w.cfg.Copy() - replaced := false + for i := range newCfg.Folders { if newCfg.Folders[i].ID == fld.ID { newCfg.Folders[i] = fld - replaced = true - break + return w.replaceLocked(newCfg) } } - if !replaced { - newCfg.Folders = append(w.cfg.Folders, fld) - } + + newCfg.Folders = append(newCfg.Folders, fld) return w.replaceLocked(newCfg) } @@ -300,7 +292,7 @@ func (w *Wrapper) SetFolder(fld FolderConfiguration) (Waiter, error) { func (w *Wrapper) Options() OptionsConfiguration { w.mut.Lock() defer w.mut.Unlock() - return w.cfg.Options + return w.cfg.Options.Copy() } // SetOptions replaces the current options configuration object. @@ -308,7 +300,7 @@ func (w *Wrapper) SetOptions(opts OptionsConfiguration) (Waiter, error) { w.mut.Lock() defer w.mut.Unlock() newCfg := w.cfg.Copy() - newCfg.Options = opts + newCfg.Options = opts.Copy() return w.replaceLocked(newCfg) } @@ -316,7 +308,7 @@ func (w *Wrapper) SetOptions(opts OptionsConfiguration) (Waiter, error) { func (w *Wrapper) GUI() GUIConfiguration { w.mut.Lock() defer w.mut.Unlock() - return w.cfg.GUI + return w.cfg.GUI.Copy() } // SetGUI replaces the current GUI configuration object. @@ -324,7 +316,7 @@ func (w *Wrapper) SetGUI(gui GUIConfiguration) (Waiter, error) { w.mut.Lock() defer w.mut.Unlock() newCfg := w.cfg.Copy() - newCfg.GUI = gui + newCfg.GUI = gui.Copy() return w.replaceLocked(newCfg) } @@ -360,7 +352,7 @@ func (w *Wrapper) Device(id protocol.DeviceID) (DeviceConfiguration, bool) { defer w.mut.Unlock() for _, device := range w.cfg.Devices { if device.DeviceID == id { - return device, true + return device.Copy(), true } } return DeviceConfiguration{}, false @@ -372,7 +364,7 @@ func (w *Wrapper) Folder(id string) (FolderConfiguration, bool) { defer w.mut.Unlock() for _, folder := range w.cfg.Folders { if folder.ID == id { - return folder, true + return folder.Copy(), true } } return FolderConfiguration{}, false @@ -380,6 +372,9 @@ func (w *Wrapper) Folder(id string) (FolderConfiguration, bool) { // Save writes the configuration to disk, and generates a ConfigSaved event. func (w *Wrapper) Save() error { + w.mut.Lock() + defer w.mut.Unlock() + fd, err := osutil.CreateAtomic(w.path) if err != nil { l.Debugln("CreateAtomic:", err) @@ -403,7 +398,7 @@ func (w *Wrapper) Save() error { func (w *Wrapper) GlobalDiscoveryServers() []string { var servers []string - for _, srv := range w.cfg.Options.GlobalAnnServers { + for _, srv := range w.Options().GlobalAnnServers { switch srv { case "default": servers = append(servers, DefaultDiscoveryServers...) @@ -420,7 +415,7 @@ func (w *Wrapper) GlobalDiscoveryServers() []string { func (w *Wrapper) ListenAddresses() []string { var addresses []string - for _, addr := range w.cfg.Options.ListenAddresses { + for _, addr := range w.Options().ListenAddresses { switch addr { case "default": addresses = append(addresses, DefaultListenAddresses...) diff --git a/lib/model/model_test.go b/lib/model/model_test.go index 7996ff0c..aab184b1 100644 --- a/lib/model/model_test.go +++ b/lib/model/model_test.go @@ -1101,6 +1101,35 @@ func TestIssue4897(t *testing.T) { } } +func TestIssue5063(t *testing.T) { + wcfg, m := newState(defaultAutoAcceptCfg) + + addAndVerify := func(wg *sync.WaitGroup) { + id := srand.String(8) + m.ClusterConfig(device1, protocol.ClusterConfig{ + Folders: []protocol.Folder{ + { + ID: id, + Label: id, + }, + }, + }) + os.RemoveAll(filepath.Join("testdata", id)) + wg.Done() + if fcfg, ok := wcfg.Folder(id); !ok || !fcfg.SharedWith(device1) { + t.Error("expected shared", id) + } + } + + wg := &sync.WaitGroup{} + for i := 0; i <= 10; i++ { + wg.Add(1) + go addAndVerify(wg) + } + + wg.Wait() +} + func TestAutoAcceptRejected(t *testing.T) { // Nothing happens if AutoAcceptFolders not set tcfg := defaultAutoAcceptCfg.Copy()