From bf4eb4b269491557f7a048fea8570f772f1ed383 Mon Sep 17 00:00:00 2001
From: Jakob Borg <jakob@nym.se>
Date: Sun, 5 Apr 2015 17:36:52 +0200
Subject: [PATCH] Copy configuration struct when sending Changed() events

Avoids data race. Copy() must be called with lock held.
---
 internal/config/config.go            | 50 ++++++++++++++++++++++++++--
 internal/config/config_test.go       | 41 +++++++++++++++++++++++
 internal/config/testdata/example.xml | 50 ++++++++++++++++++++++++++++
 internal/config/wrapper.go           | 17 +++++-----
 4 files changed, 147 insertions(+), 11 deletions(-)
 create mode 100644 internal/config/testdata/example.xml

diff --git a/internal/config/config.go b/internal/config/config.go
index f689ad6c9..45a5413b9 100644
--- a/internal/config/config.go
+++ b/internal/config/config.go
@@ -44,6 +44,30 @@ type Configuration struct {
 	OriginalVersion int `xml:"-" json:"-"` // The version we read from disk, before any conversion
 }
 
+func (orig Configuration) Copy() Configuration {
+	c := orig
+
+	// Deep copy FolderConfigurations
+	c.Folders = make([]FolderConfiguration, len(orig.Folders))
+	for i := range c.Folders {
+		c.Folders[i] = orig.Folders[i].Copy()
+	}
+
+	// Deep copy DeviceConfigurations
+	c.Devices = make([]DeviceConfiguration, len(orig.Devices))
+	for i := range c.Devices {
+		c.Devices[i] = orig.Devices[i].Copy()
+	}
+
+	c.Options = orig.Options.Copy()
+
+	// DeviceIDs are values
+	c.IgnoredDevices = make([]protocol.DeviceID, len(orig.IgnoredDevices))
+	copy(c.IgnoredDevices, orig.IgnoredDevices)
+
+	return c
+}
+
 type FolderConfiguration struct {
 	ID              string                      `xml:"id,attr" json:"id"`
 	Path            string                      `xml:"path,attr" json:"path"`
@@ -63,6 +87,13 @@ type FolderConfiguration struct {
 	deviceIDs []protocol.DeviceID
 }
 
+func (orig FolderConfiguration) Copy() FolderConfiguration {
+	c := orig
+	c.Devices = make([]FolderDeviceConfiguration, len(orig.Devices))
+	copy(c.Devices, orig.Devices)
+	return c
+}
+
 func (f *FolderConfiguration) CreateMarker() error {
 	if !f.HasMarker() {
 		marker := filepath.Join(f.Path, ".stfolder")
@@ -144,11 +175,15 @@ type DeviceConfiguration struct {
 	Introducer  bool                 `xml:"introducer,attr" json:"introducer"`
 }
 
+func (orig DeviceConfiguration) Copy() DeviceConfiguration {
+	c := orig
+	c.Addresses = make([]string, len(orig.Addresses))
+	copy(c.Addresses, orig.Addresses)
+	return c
+}
+
 type FolderDeviceConfiguration struct {
 	DeviceID protocol.DeviceID `xml:"id,attr" json:"deviceID"`
-
-	Deprecated_Name      string   `xml:"name,attr,omitempty" json:"-"`
-	Deprecated_Addresses []string `xml:"address,omitempty" json:"-"`
 }
 
 type OptionsConfiguration struct {
@@ -176,6 +211,15 @@ type OptionsConfiguration struct {
 	LimitBandwidthInLan     bool     `xml:"limitBandwidthInLan" json:"limitBandwidthInLan" default:"false"`
 }
 
+func (orig OptionsConfiguration) Copy() OptionsConfiguration {
+	c := orig
+	c.ListenAddress = make([]string, len(orig.ListenAddress))
+	copy(c.ListenAddress, orig.ListenAddress)
+	c.GlobalAnnServers = make([]string, len(orig.GlobalAnnServers))
+	copy(c.GlobalAnnServers, orig.GlobalAnnServers)
+	return c
+}
+
 type GUIConfiguration struct {
 	Enabled  bool   `xml:"enabled,attr" json:"enabled" default:"true"`
 	Address  string `xml:"address" json:"address" default:"127.0.0.1:8384"`
diff --git a/internal/config/config_test.go b/internal/config/config_test.go
index f08d3594b..584918bb4 100644
--- a/internal/config/config_test.go
+++ b/internal/config/config_test.go
@@ -7,6 +7,8 @@
 package config
 
 import (
+	"bytes"
+	"encoding/json"
 	"fmt"
 	"os"
 	"reflect"
@@ -438,3 +440,42 @@ func TestRequiresRestart(t *testing.T) {
 		t.Error("Changing GUI options requires restart")
 	}
 }
+
+func TestCopy(t *testing.T) {
+	wrapper, err := Load("testdata/example.xml", device1)
+	if err != nil {
+		t.Fatal(err)
+	}
+	cfg := wrapper.Raw()
+
+	bsOrig, err := json.MarshalIndent(cfg, "", "  ")
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	copy := cfg.Copy()
+
+	cfg.Devices[0].Addresses[0] = "wrong"
+	cfg.Folders[0].Devices[0].DeviceID = protocol.DeviceID{0, 1, 2, 3}
+	cfg.Options.ListenAddress[0] = "wrong"
+	cfg.GUI.APIKey = "wrong"
+
+	bsChanged, err := json.MarshalIndent(cfg, "", "  ")
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	bsCopy, err := json.MarshalIndent(copy, "", "  ")
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	if bytes.Compare(bsOrig, bsChanged) == 0 {
+		t.Error("Config should have changed")
+	}
+	if bytes.Compare(bsOrig, bsCopy) != 0 {
+		//ioutil.WriteFile("a", bsOrig, 0644)
+		//ioutil.WriteFile("b", bsCopy, 0644)
+		t.Error("Copy should be unchanged")
+	}
+}
diff --git a/internal/config/testdata/example.xml b/internal/config/testdata/example.xml
new file mode 100644
index 000000000..6b59457ea
--- /dev/null
+++ b/internal/config/testdata/example.xml
@@ -0,0 +1,50 @@
+<configuration version="10">
+    <folder id="default" path="~/Sync" ro="false" rescanIntervalS="60" ignorePerms="false">
+        <device id="GYRZZQB-IRNPV4Z-T7TC52W-EQYJ3TT-FDQW6MW-DFLMU42-SSSU6EM-FBK2VAY"></device>
+        <device id="P56IOI7-MZJNU2Y-IQGDREY-DM2MGTI-MGL3BXN-PQ6W5BM-TBBZ4TJ-XZWICQ2"></device>
+        <device id="ZJCXQL7-M3NP4IC-4KQ7WFU-3NANYUX-AD74QRL-Q5LJ7BH-72KYZHK-GHTAOAK"></device>
+        <versioning></versioning>
+        <lenientMtimes>false</lenientMtimes>
+        <copiers>1</copiers>
+        <pullers>16</pullers>
+        <hashers>0</hashers>
+    </folder>
+    <device id="GYRZZQB-IRNPV4Z-T7TC52W-EQYJ3TT-FDQW6MW-DFLMU42-SSSU6EM-FBK2VAY" name="win7" compression="metadata" introducer="false">
+        <address>dynamic</address>
+    </device>
+    <device id="P56IOI7-MZJNU2Y-IQGDREY-DM2MGTI-MGL3BXN-PQ6W5BM-TBBZ4TJ-XZWICQ2" name="jborg-mbp" compression="metadata" introducer="false">
+        <address>dynamic</address>
+    </device>
+    <device id="ZJCXQL7-M3NP4IC-4KQ7WFU-3NANYUX-AD74QRL-Q5LJ7BH-72KYZHK-GHTAOAK" name="anto-syncer" compression="never" introducer="false">
+        <address>dynamic</address>
+    </device>
+    <gui enabled="true" tls="false">
+        <address>0.0.0.0:8080</address>
+        <apikey>136020D511BF136020D511BF136020D511BF</apikey>
+    </gui>
+    <options>
+        <listenAddress>0.0.0.0:22000</listenAddress>
+        <globalAnnounceServer>udp4://announce.syncthing.net:22026</globalAnnounceServer>
+        <globalAnnounceServer>udp6://announce-v6.syncthing.net:22026</globalAnnounceServer>
+        <globalAnnounceEnabled>true</globalAnnounceEnabled>
+        <localAnnounceEnabled>true</localAnnounceEnabled>
+        <localAnnouncePort>21025</localAnnouncePort>
+        <localAnnounceMCAddr>[ff32::5222]:21026</localAnnounceMCAddr>
+        <maxSendKbps>0</maxSendKbps>
+        <maxRecvKbps>0</maxRecvKbps>
+        <reconnectionIntervalS>60</reconnectionIntervalS>
+        <startBrowser>false</startBrowser>
+        <upnpEnabled>true</upnpEnabled>
+        <upnpLeaseMinutes>0</upnpLeaseMinutes>
+        <upnpRenewalMinutes>30</upnpRenewalMinutes>
+        <urAccepted>-1</urAccepted>
+        <urUniqueID></urUniqueID>
+        <restartOnWakeup>true</restartOnWakeup>
+        <autoUpgradeIntervalH>0</autoUpgradeIntervalH>
+        <keepTemporariesH>24</keepTemporariesH>
+        <cacheIgnoredFiles>true</cacheIgnoredFiles>
+        <progressUpdateIntervalS>5</progressUpdateIntervalS>
+        <symlinksEnabled>true</symlinksEnabled>
+        <limitBandwidthInLan>false</limitBandwidthInLan>
+    </options>
+</configuration>
diff --git a/internal/config/wrapper.go b/internal/config/wrapper.go
index 2e3f1feff..e7cffa0e5 100644
--- a/internal/config/wrapper.go
+++ b/internal/config/wrapper.go
@@ -80,6 +80,7 @@ func (w *Wrapper) Serve() {
 		w.sMut.Lock()
 		subs := w.subs
 		w.sMut.Unlock()
+
 		for _, h := range subs {
 			h.Changed(cfg)
 		}
@@ -113,7 +114,7 @@ func (w *Wrapper) Replace(cfg Configuration) {
 	w.cfg = cfg
 	w.deviceMap = nil
 	w.folderMap = nil
-	w.replaces <- cfg
+	w.replaces <- cfg.Copy()
 }
 
 // Devices returns a map of devices. Device structures should not be changed,
@@ -141,13 +142,13 @@ func (w *Wrapper) SetDevice(dev DeviceConfiguration) {
 	for i := range w.cfg.Devices {
 		if w.cfg.Devices[i].DeviceID == dev.DeviceID {
 			w.cfg.Devices[i] = dev
-			w.replaces <- w.cfg
+			w.replaces <- w.cfg.Copy()
 			return
 		}
 	}
 
 	w.cfg.Devices = append(w.cfg.Devices, dev)
-	w.replaces <- w.cfg
+	w.replaces <- w.cfg.Copy()
 }
 
 // Devices returns a map of folders. Folder structures should not be changed,
@@ -181,13 +182,13 @@ func (w *Wrapper) SetFolder(fld FolderConfiguration) {
 	for i := range w.cfg.Folders {
 		if w.cfg.Folders[i].ID == fld.ID {
 			w.cfg.Folders[i] = fld
-			w.replaces <- w.cfg
+			w.replaces <- w.cfg.Copy()
 			return
 		}
 	}
 
 	w.cfg.Folders = append(w.cfg.Folders, fld)
-	w.replaces <- w.cfg
+	w.replaces <- w.cfg.Copy()
 }
 
 // Options returns the current options configuration object.
@@ -202,7 +203,7 @@ func (w *Wrapper) SetOptions(opts OptionsConfiguration) {
 	w.mut.Lock()
 	defer w.mut.Unlock()
 	w.cfg.Options = opts
-	w.replaces <- w.cfg
+	w.replaces <- w.cfg.Copy()
 }
 
 // GUI returns the current GUI configuration object.
@@ -217,7 +218,7 @@ func (w *Wrapper) SetGUI(gui GUIConfiguration) {
 	w.mut.Lock()
 	defer w.mut.Unlock()
 	w.cfg.GUI = gui
-	w.replaces <- w.cfg
+	w.replaces <- w.cfg.Copy()
 }
 
 // Sets the folder error state. Emits ConfigSaved to cause a GUI refresh.
@@ -236,7 +237,7 @@ func (w *Wrapper) SetFolderError(id string, err error) {
 			if errstr != w.cfg.Folders[i].Invalid {
 				w.cfg.Folders[i].Invalid = errstr
 				events.Default.Log(events.ConfigSaved, w.cfg)
-				w.replaces <- w.cfg
+				w.replaces <- w.cfg.Copy()
 			}
 			return
 		}