cmd/syncthing: Ensure myID is set by making it local (fixes #5859) (#5862)

cmd/syncthing/main.go

@@ -60,8 +60,6 @@ const (
 	maxSystemLog         = 250
 )
 
-var myID protocol.DeviceID
-
 const (
 	usage      = "syncthing [options]"
 	extraUsage = `
@@ -333,13 +331,12 @@ func main() {
 			os.Exit(exitError)
 		}
 
-		myID = protocol.NewDeviceID(cert.Certificate[0])
-		fmt.Println(myID)
+		fmt.Println(protocol.NewDeviceID(cert.Certificate[0]))
 		return
 	}
 
 	if options.browserOnly {
-		if err := openGUI(); err != nil {
+		if err := openGUI(protocol.EmptyDeviceID); err != nil {
 			l.Warnln("Failed to open web UI:", err)
 			os.Exit(exitError)
 		}
@@ -396,8 +393,8 @@ func main() {
 	}
 }
 
-func openGUI() error {
-	cfg, err := loadOrDefaultConfig()
+func openGUI(myID protocol.DeviceID) error {
+	cfg, err := loadOrDefaultConfig(myID)
 	if err != nil {
 		return err
 	}
@@ -421,31 +418,26 @@ func generate(generateDir string) error {
 		return err
 	}
 
+	var myID protocol.DeviceID
 	certFile, keyFile := filepath.Join(dir, "cert.pem"), filepath.Join(dir, "key.pem")
 	cert, err := tls.LoadX509KeyPair(certFile, keyFile)
 	if err == nil {
 		l.Warnln("Key exists; will not overwrite.")
-		l.Infoln("Device ID:", protocol.NewDeviceID(cert.Certificate[0]))
 	} else {
 		cert, err = tlsutil.NewCertificate(certFile, keyFile, tlsDefaultCommonName)
 		if err != nil {
 			return errors.Wrap(err, "create certificate")
 		}
-		myID = protocol.NewDeviceID(cert.Certificate[0])
-		if err != nil {
-			return errors.Wrap(err, "load certificate")
-		}
-		if err == nil {
-			l.Infoln("Device ID:", protocol.NewDeviceID(cert.Certificate[0]))
-		}
 	}
+	myID = protocol.NewDeviceID(cert.Certificate[0])
+	l.Infoln("Device ID:", myID)
 
 	cfgFile := filepath.Join(dir, "config.xml")
 	if _, err := os.Stat(cfgFile); err == nil {
 		l.Warnln("Config exists; will not overwrite.")
 		return nil
 	}
-	cfg, err := defaultConfig(cfgFile)
+	cfg, err := defaultConfig(cfgFile, myID)
 	if err != nil {
 		return err
 	}
@@ -479,7 +471,7 @@ func debugFacilities() string {
 }
 
 func checkUpgrade() upgrade.Release {
-	cfg, _ := loadOrDefaultConfig()
+	cfg, _ := loadOrDefaultConfig(protocol.EmptyDeviceID)
 	opts := cfg.Options()
 	release, err := upgrade.LatestRelease(opts.ReleasesURL, build.Version, opts.UpgradeToPreReleases)
 	if err != nil {
@@ -520,7 +512,7 @@ func performUpgrade(release upgrade.Release) {
 }
 
 func upgradeViaRest() error {
-	cfg, _ := loadOrDefaultConfig()
+	cfg, _ := loadOrDefaultConfig(protocol.EmptyDeviceID)
 	u, err := url.Parse(cfg.GUI().URL())
 	if err != nil {
 		return err
@@ -556,18 +548,6 @@ func upgradeViaRest() error {
 }
 
 func syncthingMain(runtimeOptions RuntimeOptions) {
-	cfg, err := loadConfigAtStartup(runtimeOptions.allowNewerConfig)
-	if err != nil {
-		l.Warnln("Failed to initialize config:", err)
-		os.Exit(exitError)
-	}
-
-	if runtimeOptions.unpaused {
-		setPauseState(cfg, false)
-	} else if runtimeOptions.paused {
-		setPauseState(cfg, true)
-	}
-
 	// Ensure that we have a certificate and key.
 	cert, err := tls.LoadX509KeyPair(
 		locations.Get(locations.CertFile),
@@ -585,6 +565,19 @@ func syncthingMain(runtimeOptions RuntimeOptions) {
 			os.Exit(1)
 		}
 	}
+	myID := protocol.NewDeviceID(cert.Certificate[0])
+
+	cfg, err := loadConfigAtStartup(runtimeOptions.allowNewerConfig, myID)
+	if err != nil {
+		l.Warnln("Failed to initialize config:", err)
+		os.Exit(exitError)
+	}
+
+	if runtimeOptions.unpaused {
+		setPauseState(cfg, false)
+	} else if runtimeOptions.paused {
+		setPauseState(cfg, true)
+	}
 
 	dbFile := locations.Get(locations.Database)
 	ldb, err := syncthing.OpenGoleveldb(dbFile)
@@ -692,22 +685,22 @@ func setupSignalHandling(app *syncthing.App) {
 	}()
 }
 
-func loadOrDefaultConfig() (config.Wrapper, error) {
+func loadOrDefaultConfig(myID protocol.DeviceID) (config.Wrapper, error) {
 	cfgFile := locations.Get(locations.ConfigFile)
 	cfg, err := config.Load(cfgFile, myID)
 
 	if err != nil {
-		cfg, err = defaultConfig(cfgFile)
+		cfg, err = defaultConfig(cfgFile, myID)
 	}
 
 	return cfg, err
 }
 
-func loadConfigAtStartup(allowNewerConfig bool) (config.Wrapper, error) {
+func loadConfigAtStartup(allowNewerConfig bool, myID protocol.DeviceID) (config.Wrapper, error) {
 	cfgFile := locations.Get(locations.ConfigFile)
 	cfg, err := config.Load(cfgFile, myID)
 	if os.IsNotExist(err) {
-		cfg, err = defaultConfig(cfgFile)
+		cfg, err = defaultConfig(cfgFile, myID)
 		if err != nil {
 			return nil, errors.Wrap(err, "failed to generate default config")
 		}
@@ -797,7 +790,7 @@ func auditWriter(auditFile string) io.Writer {
 	return fd
 }
 
-func defaultConfig(cfgFile string) (config.Wrapper, error) {
+func defaultConfig(cfgFile string, myID protocol.DeviceID) (config.Wrapper, error) {
 	newCfg, err := config.NewWithFreePorts(myID)
 	if err != nil {
 		return nil, err

cmd/syncthing/monitor.go

@@ -20,6 +20,7 @@ import (
 
 	"github.com/syncthing/syncthing/lib/locations"
 	"github.com/syncthing/syncthing/lib/osutil"
+	"github.com/syncthing/syncthing/lib/protocol"
 	"github.com/syncthing/syncthing/lib/sync"
 )
 
@@ -448,7 +449,7 @@ func childEnv() []string {
 // panicUploadMaxWait uploading panics...
 func maybeReportPanics() {
 	// Try to get a config to see if/where panics should be reported.
-	cfg, err := loadOrDefaultConfig()
+	cfg, err := loadOrDefaultConfig(protocol.EmptyDeviceID)
 	if err != nil {
 		l.Warnln("Couldn't load config; not reporting crash")
 		return

lib/config/config.go

@@ -270,6 +270,16 @@ found:
 func (cfg *Configuration) clean() error {
 	util.FillNilSlices(&cfg.Options)
 
+	// Ensure that the device list is
+	// - free from duplicates
+	// - no devices with empty ID
+	// - sorted by ID
+	// Happen before preparting folders as that needs a correct device list.
+	cfg.Devices = ensureNoDuplicateOrEmptyIDDevices(cfg.Devices)
+	sort.Slice(cfg.Devices, func(a, b int) bool {
+		return cfg.Devices[a].DeviceID.Compare(cfg.Devices[b].DeviceID) == -1
+	})
+
 	// Prepare folders and check for duplicates. Duplicates are bad and
 	// dangerous, can't currently be resolved in the GUI, and shouldn't
 	// happen when configured by the GUI. We return with an error in that
@@ -310,14 +320,6 @@ func (cfg *Configuration) clean() error {
 		existingDevices[device.DeviceID] = true
 	}
 
-	// Ensure that the device list is
-	// - free from duplicates
-	// - sorted by ID
-	cfg.Devices = ensureNoDuplicateDevices(cfg.Devices)
-	sort.Slice(cfg.Devices, func(a, b int) bool {
-		return cfg.Devices[a].DeviceID.Compare(cfg.Devices[b].DeviceID) == -1
-	})
-
 	// Ensure that the folder list is sorted by ID
 	sort.Slice(cfg.Folders, func(a, b int) bool {
 		return cfg.Folders[a].ID < cfg.Folders[b].ID
@@ -476,14 +478,14 @@ loop:
 	return devices[0:count]
 }
 
-func ensureNoDuplicateDevices(devices []DeviceConfiguration) []DeviceConfiguration {
+func ensureNoDuplicateOrEmptyIDDevices(devices []DeviceConfiguration) []DeviceConfiguration {
 	count := len(devices)
 	i := 0
 	seenDevices := make(map[protocol.DeviceID]bool)
 loop:
 	for i < count {
 		id := devices[i].DeviceID
-		if _, ok := seenDevices[id]; ok {
+		if _, ok := seenDevices[id]; ok || id == protocol.EmptyDeviceID {
 			devices[i] = devices[count-1]
 			count--
 			continue loop

lib/config/config_test.go

@@ -1100,6 +1100,32 @@ func TestDeviceConfigObservedNotNil(t *testing.T) {
 	}
 }
 
+func TestRemoveDeviceWithEmptyID(t *testing.T) {
+	cfg := Configuration{
+		Devices: []DeviceConfiguration{
+			{
+				Name: "foo",
+			},
+		},
+		Folders: []FolderConfiguration{
+			{
+				ID:      "foo",
+				Path:    "testdata",
+				Devices: []FolderDeviceConfiguration{{}},
+			},
+		},
+	}
+
+	cfg.clean()
+
+	if len(cfg.Devices) != 0 {
+		t.Error("Expected device with empty ID to be removed from config:", cfg.Devices)
+	}
+	if len(cfg.Folders[0].Devices) != 0 {
+		t.Error("Expected device with empty ID to be removed from folder")
+	}
+}
+
 // defaultConfigAsMap returns a valid default config as a JSON-decoded
 // map[string]interface{}. This is useful to override random elements and
 // re-encode into JSON.