lib/model: Fix panic when auto-accepting existing paused folder (fixes #4636)

This no longer pokes at model internals, and only touches the config.
As a result, model handles this in CommitConfiguration, which restarts
the folders if things change, which repopulate m.folderDevice, m.deviceFolder
and other interal mappings.

GitHub-Pull-Request: https://github.com/syncthing/syncthing/pull/4639

lib/model/model.go

@@ -1096,15 +1096,17 @@ func (m *Model) handleIntroductions(introducerCfg config.DeviceConfiguration, cm
 	foldersDevices := make(folderDeviceSet)
 
 	for _, folder := range cm.Folders {
-		// We don't have this folder, skip.
-		if _, ok := m.folderDevices[folder.ID]; !ok {
-			continue
-		}
-
 		// Adds devices which we do not have, but the introducer has
 		// for the folders that we have in common. Also, shares folders
 		// with devices that we have in common, yet are currently not sharing
 		// the folder.
+
+		fcfg, ok := m.cfg.Folder(folder.ID)
+		if !ok {
+			// Don't have this folder, carry on.
+			continue
+		}
+
 	nextDevice:
 		for _, device := range folder.Devices {
 			// No need to share with self.
@@ -1118,22 +1120,29 @@ func (m *Model) handleIntroductions(introducerCfg config.DeviceConfiguration, cm
 				// The device is currently unknown. Add it to the config.
 				m.introduceDevice(device, introducerCfg)
 				changed = true
-			}
-
-			for _, er := range m.deviceFolders[device.ID] {
-				if er == folder.ID {
+			} else {
+				for _, dev := range fcfg.DeviceIDs() {
+					if dev == device.ID {
 						// We already share the folder with this device, so
 						// nothing to do.
 						continue nextDevice
 					}
 				}
+			}
 
 			// We don't yet share this folder with this device. Add the device
 			// to sharing list of the folder.
 			l.Infof("Sharing folder %s with %v (vouched for by introducer %v)", folder.Description(), device.ID, introducerCfg.DeviceID)
-			m.shareFolderWithDeviceLocked(device.ID, folder.ID, introducerCfg.DeviceID)
+			fcfg.Devices = append(fcfg.Devices, config.FolderDeviceConfiguration{
+				DeviceID:     device.ID,
+				IntroducedBy: introducerCfg.DeviceID,
+			})
 			changed = true
 		}
+
+		if changed {
+			m.cfg.SetFolder(fcfg)
+		}
 	}
 
 	return foldersDevices, changed
@@ -1195,7 +1204,7 @@ func (m *Model) handleDeintroductions(introducerCfg config.DeviceConfiguration,
 // handleAutoAccepts handles adding and sharing folders for devices that have
 // AutoAcceptFolders set to true.
 func (m *Model) handleAutoAccepts(deviceCfg config.DeviceConfiguration, folder protocol.Folder) bool {
-	if _, ok := m.cfg.Folder(folder.ID); !ok {
+	if cfg, ok := m.cfg.Folder(folder.ID); !ok {
 		defaultPath := m.cfg.Options().DefaultFolderPath
 		defaultPathFs := fs.NewFilesystem(fs.FilesystemTypeBasic, defaultPath)
 		for _, path := range []string{folder.Label, folder.ID} {
@@ -1204,7 +1213,9 @@ func (m *Model) handleAutoAccepts(deviceCfg config.DeviceConfiguration, folder p
 			}
 
 			fcfg := config.NewFolderConfiguration(m.id, folder.ID, folder.Label, fs.FilesystemTypeBasic, filepath.Join(defaultPath, path))
-
+			fcfg.Devices = append(fcfg.Devices, config.FolderDeviceConfiguration{
+				DeviceID: deviceCfg.DeviceID,
+			})
 			// Need to wait for the waiter, as this calls CommitConfiguration,
 			// which sets up the folder and as we return from this call,
 			// ClusterConfig starts poking at m.folderFiles and other things
@@ -1212,30 +1223,27 @@ func (m *Model) handleAutoAccepts(deviceCfg config.DeviceConfiguration, folder p
 			w, _ := m.cfg.SetFolder(fcfg)
 			w.Wait()
 
-			// This needs to happen under a lock.
-			m.fmut.Lock()
-			w = m.shareFolderWithDeviceLocked(deviceCfg.DeviceID, folder.ID, protocol.DeviceID{})
-			m.fmut.Unlock()
-			w.Wait()
 			l.Infof("Auto-accepted %s folder %s at path %s", deviceCfg.DeviceID, folder.Description(), fcfg.Path)
 			return true
 		}
 		l.Infof("Failed to auto-accept folder %s from %s due to path conflict", folder.Description(), deviceCfg.DeviceID)
 		return false
-	}
-
-	// Folder does not exist yet.
-	if m.folderSharedWith(folder.ID, deviceCfg.DeviceID) {
+	} else {
+		for _, device := range cfg.DeviceIDs() {
+			if device == deviceCfg.DeviceID {
+				// Already shared nothing todo.
 				return false
 			}
-
-	m.fmut.Lock()
-	w := m.shareFolderWithDeviceLocked(deviceCfg.DeviceID, folder.ID, protocol.DeviceID{})
-	m.fmut.Unlock()
+		}
+		cfg.Devices = append(cfg.Devices, config.FolderDeviceConfiguration{
+			DeviceID: deviceCfg.DeviceID,
+		})
+		w, _ := m.cfg.SetFolder(cfg)
 		w.Wait()
 		l.Infof("Shared %s with %s due to auto-accept", folder.ID, deviceCfg.DeviceID)
 		return true
 	}
+}
 
 func (m *Model) introduceDevice(device protocol.Device, introducerCfg config.DeviceConfiguration) {
 	addresses := []string{"dynamic"}
@@ -1265,19 +1273,6 @@ func (m *Model) introduceDevice(device protocol.Device, introducerCfg config.Dev
 	m.cfg.SetDevice(newDeviceCfg)
 }
 
-func (m *Model) shareFolderWithDeviceLocked(deviceID protocol.DeviceID, folder string, introducer protocol.DeviceID) config.Waiter {
-	m.deviceFolders[deviceID] = append(m.deviceFolders[deviceID], folder)
-	m.folderDevices.set(deviceID, folder)
-
-	folderCfg := m.cfg.Folders()[folder]
-	folderCfg.Devices = append(folderCfg.Devices, config.FolderDeviceConfiguration{
-		DeviceID:     deviceID,
-		IntroducedBy: introducer,
-	})
-	w, _ := m.cfg.SetFolder(folderCfg)
-	return w
-}
-
 // Closed is called when a connection has been closed
 func (m *Model) Closed(conn protocol.Connection, err error) {
 	device := conn.ID()

lib/model/model_test.go

@@ -60,6 +60,9 @@ func init() {
 	defaultConfig = config.Wrap("/tmp/test", _defaultConfig)
 	defaultAutoAcceptCfg = config.Configuration{
 		Devices: []config.DeviceConfiguration{
+			{
+				DeviceID: protocol.LocalDeviceID, // self
+			},
 			{
 				DeviceID:          device1,
 				AutoAcceptFolders: true,
@@ -110,8 +113,10 @@ func newState(cfg config.Configuration) (*config.Wrapper, *Model) {
 
 	m := NewModel(wcfg, protocol.LocalDeviceID, "syncthing", "dev", db, nil)
 	for _, folder := range cfg.Folders {
+		if !folder.Paused {
 			m.AddFolder(folder)
 		}
+	}
 	m.ServeBackground()
 	m.AddConnection(&fakeConnection{id: device1}, protocol.HelloResult{})
 	return wcfg, m
@@ -990,7 +995,9 @@ func TestIntroducer(t *testing.T) {
 func TestAutoAcceptRejected(t *testing.T) {
 	// Nothing happens if AutoAcceptFolders not set
 	tcfg := defaultAutoAcceptCfg.Copy()
-	tcfg.Devices[0].AutoAcceptFolders = false
+	for i := range tcfg.Devices {
+		tcfg.Devices[i].AutoAcceptFolders = false
+	}
 	wcfg, m := newState(tcfg)
 	id := srand.String(8)
 	defer os.RemoveAll(filepath.Join("testdata", id))
@@ -1058,6 +1065,7 @@ func TestAutoAcceptExistingFolder(t *testing.T) {
 	id := srand.String(8)
 	idOther := srand.String(8) // To check that path does not get changed.
 	defer os.RemoveAll(filepath.Join("testdata", id))
+	defer os.RemoveAll(filepath.Join("testdata", idOther))
 
 	tcfg := defaultAutoAcceptCfg.Copy()
 	tcfg.Folders = []config.FolderConfiguration{
@@ -1218,6 +1226,115 @@ func TestAutoAcceptFallsBackToID(t *testing.T) {
 	}
 }
 
+func TestAutoAcceptPausedWhenFolderConfigChanged(t *testing.T) {
+	// Existing folder
+	id := srand.String(8)
+	idOther := srand.String(8) // To check that path does not get changed.
+	defer os.RemoveAll(filepath.Join("testdata", id))
+	defer os.RemoveAll(filepath.Join("testdata", idOther))
+
+	tcfg := defaultAutoAcceptCfg.Copy()
+	fcfg := config.NewFolderConfiguration(protocol.LocalDeviceID, id, "", fs.FilesystemTypeBasic, filepath.Join("testdata", idOther))
+	fcfg.Paused = true
+	// The order of devices here is wrong (cfg.clean() sorts them), which will cause the folder to restart.
+	// Because of the restart, folder gets removed from m.deviceFolder, which means that generateClusterConfig will not panic.
+	// This wasn't an issue before, yet keeping the test case to prove that it still isn't.
+	fcfg.Devices = append(fcfg.Devices, config.FolderDeviceConfiguration{
+		DeviceID: device1,
+	})
+	tcfg.Folders = []config.FolderConfiguration{fcfg}
+	wcfg, m := newState(tcfg)
+	if _, ok := wcfg.Folder(id); !ok || m.folderSharedWith(id, device1) {
+		t.Error("missing folder, or shared", id)
+	}
+	if _, ok := m.folderRunners[id]; ok {
+		t.Fatal("folder running?")
+	}
+
+	m.ClusterConfig(device1, protocol.ClusterConfig{
+		Folders: []protocol.Folder{
+			{
+				ID:    id,
+				Label: id,
+			},
+		},
+	})
+	m.generateClusterConfig(device1)
+
+	if fcfg, ok := wcfg.Folder(id); !ok {
+		t.Error("missing folder")
+	} else if fcfg.Path != filepath.Join("testdata", idOther) {
+		t.Error("folder path changed")
+	} else {
+		for _, dev := range fcfg.DeviceIDs() {
+			if dev == device1 {
+				return
+			}
+		}
+		t.Error("device missing")
+	}
+
+	if _, ok := m.folderDevices[id]; ok {
+		t.Error("folder started")
+	}
+}
+
+func TestAutoAcceptPausedWhenFolderConfigNotChanged(t *testing.T) {
+	// Existing folder
+	id := srand.String(8)
+	idOther := srand.String(8) // To check that path does not get changed.
+	defer os.RemoveAll(filepath.Join("testdata", id))
+	defer os.RemoveAll(filepath.Join("testdata", idOther))
+
+	tcfg := defaultAutoAcceptCfg.Copy()
+	fcfg := config.NewFolderConfiguration(protocol.LocalDeviceID, id, "", fs.FilesystemTypeBasic, filepath.Join("testdata", idOther))
+	fcfg.Paused = true
+	// The new folder is exactly the same as the one constructed by handleAutoAccept, which means
+	// the folder will not be restarted (even if it's paused), yet handleAutoAccept used to add the folder
+	// to m.deviceFolders which had caused panics when calling generateClusterConfig, as the folder
+	// did not have a file set.
+	fcfg.Devices = append([]config.FolderDeviceConfiguration{
+		{
+			DeviceID: device1,
+		},
+	}, fcfg.Devices...) // Need to ensure this device order to avoid folder restart.
+	tcfg.Folders = []config.FolderConfiguration{fcfg}
+	wcfg, m := newState(tcfg)
+	if _, ok := wcfg.Folder(id); !ok || m.folderSharedWith(id, device1) {
+		t.Error("missing folder, or shared", id)
+	}
+	if _, ok := m.folderRunners[id]; ok {
+		t.Fatal("folder running?")
+	}
+
+	m.ClusterConfig(device1, protocol.ClusterConfig{
+		Folders: []protocol.Folder{
+			{
+				ID:    id,
+				Label: id,
+			},
+		},
+	})
+	m.generateClusterConfig(device1)
+
+	if fcfg, ok := wcfg.Folder(id); !ok {
+		t.Error("missing folder")
+	} else if fcfg.Path != filepath.Join("testdata", idOther) {
+		t.Error("folder path changed")
+	} else {
+		for _, dev := range fcfg.DeviceIDs() {
+			if dev == device1 {
+				return
+			}
+		}
+		t.Error("device missing")
+	}
+
+	if _, ok := m.folderDevices[id]; ok {
+		t.Error("folder started")
+	}
+}
+
 func changeIgnores(t *testing.T, m *Model, expected []string) {
 	arrEqual := func(a, b []string) bool {
 		if len(a) != len(b) {