all: Remove potentially problematic errors from panics (fixes #5839) (#5912)

cmd/syncthing/blockprof.go

@@ -22,11 +22,15 @@ func init() {
 			panic("Couldn't find block profiler")
 		}
 		l.Debugln("Starting block profiling")
-		go saveBlockingProfiles(profiler)
+		go func() {
+			err := saveBlockingProfiles(profiler) // Only returns on error
+			l.Warnln("Block profiler failed:", err)
+			panic("Block profiler failed")
+		}()
 	}
 }
 
-func saveBlockingProfiles(profiler *pprof.Profile) {
+func saveBlockingProfiles(profiler *pprof.Profile) error {
 	runtime.SetBlockProfileRate(1)
 
 	t0 := time.Now()
@@ -35,16 +39,16 @@ func saveBlockingProfiles(profiler *pprof.Profile) {
 
 		fd, err := os.Create(fmt.Sprintf("block-%05d-%07d.pprof", syscall.Getpid(), startms))
 		if err != nil {
-			panic(err)
+			return err
 		}
 		err = profiler.WriteTo(fd, 0)
 		if err != nil {
-			panic(err)
+			return err
 		}
 		err = fd.Close()
 		if err != nil {
-			panic(err)
+			return err
 		}
-
 	}
+	return nil
 }

cmd/syncthing/heapprof.go

@@ -23,11 +23,15 @@ func init() {
 			rate = i
 		}
 		l.Debugln("Starting heap profiling")
-		go saveHeapProfiles(rate)
+		go func() {
+			err := saveHeapProfiles(rate) // Only returns on error
+			l.Warnln("Heap profiler failed:", err)
+			panic("Heap profiler failed")
+		}()
 	}
 }
 
-func saveHeapProfiles(rate int) {
+func saveHeapProfiles(rate int) error {
 	runtime.MemProfileRate = rate
 	var memstats, prevMemstats runtime.MemStats
 
@@ -38,21 +42,21 @@ func saveHeapProfiles(rate int) {
 		if memstats.HeapInuse > prevMemstats.HeapInuse {
 			fd, err := os.Create(name + ".tmp")
 			if err != nil {
-				panic(err)
+				return err
 			}
 			err = pprof.WriteHeapProfile(fd)
 			if err != nil {
-				panic(err)
+				return err
 			}
 			err = fd.Close()
 			if err != nil {
-				panic(err)
+				return err
 			}
 
 			os.Remove(name) // Error deliberately ignored
 			err = os.Rename(name+".tmp", name)
 			if err != nil {
-				panic(err)
+				return err
 			}
 
 			prevMemstats = memstats

cmd/syncthing/monitor.go

@@ -102,7 +102,8 @@ func monitorMain(runtimeOptions RuntimeOptions) {
 		l.Infoln("Starting syncthing")
 		err = cmd.Start()
 		if err != nil {
-			panic(err)
+			l.Warnln("Error starting the main Syncthing process:", err)
+			panic("Error starting the main Syncthing process")
 		}
 
 		stdoutMut.Lock()

lib/config/config.go

@@ -109,7 +109,8 @@ func New(myID protocol.DeviceID) Configuration {
 
 	// Can't happen.
 	if err := cfg.prepare(myID); err != nil {
-		panic("bug: error in preparing new folder: " + err.Error())
+		l.Warnln("bug: error in preparing new folder:", err)
+		panic("error in preparing new folder")
 	}
 
 	return cfg

lib/locations/locations.go

@@ -46,7 +46,8 @@ const (
 func init() {
 	err := expandLocations()
 	if err != nil {
-		panic(err)
+		fmt.Println(err)
+		panic("Failed to expand locations at init time")
 	}
 }
 
@@ -124,7 +125,8 @@ func defaultConfigDir() string {
 	case "darwin":
 		dir, err := fs.ExpandTilde("~/Library/Application Support/Syncthing")
 		if err != nil {
-			panic(err)
+			fmt.Println(err)
+			panic("Failed to get default config dir")
 		}
 		return dir
 
@@ -134,7 +136,8 @@ func defaultConfigDir() string {
 		}
 		dir, err := fs.ExpandTilde("~/.config/syncthing")
 		if err != nil {
-			panic(err)
+			fmt.Println(err)
+			panic("Failed to get default config dir")
 		}
 		return dir
 	}
@@ -144,7 +147,8 @@ func defaultConfigDir() string {
 func homeDir() string {
 	home, err := fs.ExpandTilde("~")
 	if err != nil {
-		panic(err)
+		fmt.Println(err)
+		panic("Failed to get user home dir")
 	}
 	return home
 }

lib/model/folder_sendrecv.go

@@ -594,9 +594,9 @@ func (f *sendReceiveFolder) handleDir(file protocol.FileInfo, dbUpdateChan chan<
 			// Symlinks aren't checked for conflicts.
 
 			file.Version = file.Version.Merge(curFile.Version)
-			err = inWritableDir(func(name string) error {
+			err = f.inWritableDir(func(name string) error {
 				return f.moveForConflict(name, file.ModifiedBy.String(), scanChan)
-			}, f.fs, curFile.Name)
+			}, curFile.Name)
 		} else {
 			err = f.deleteItemOnDisk(curFile, scanChan)
 		}
@@ -633,7 +633,7 @@ func (f *sendReceiveFolder) handleDir(file protocol.FileInfo, dbUpdateChan chan<
 			return f.fs.Chmod(path, mode|(info.Mode()&retainBits))
 		}
 
-		if err = inWritableDir(mkdir, f.fs, file.Name); err == nil {
+		if err = f.inWritableDir(mkdir, file.Name); err == nil {
 			dbUpdateChan <- dbUpdateJob{file, dbUpdateHandleDir}
 		} else {
 			f.newPullError(file.Name, errors.Wrap(err, "creating directory"))
@@ -748,9 +748,9 @@ func (f *sendReceiveFolder) handleSymlink(file protocol.FileInfo, dbUpdateChan c
 			// Directories and symlinks aren't checked for conflicts.
 
 			file.Version = file.Version.Merge(curFile.Version)
-			err = inWritableDir(func(name string) error {
+			err = f.inWritableDir(func(name string) error {
 				return f.moveForConflict(name, file.ModifiedBy.String(), scanChan)
-			}, f.fs, curFile.Name)
+			}, curFile.Name)
 		} else {
 			err = f.deleteItemOnDisk(curFile, scanChan)
 		}
@@ -769,7 +769,7 @@ func (f *sendReceiveFolder) handleSymlink(file protocol.FileInfo, dbUpdateChan c
 		return f.maybeCopyOwner(path)
 	}
 
-	if err = inWritableDir(createLink, f.fs, file.Name); err == nil {
+	if err = f.inWritableDir(createLink, file.Name); err == nil {
 		dbUpdateChan <- dbUpdateJob{file, dbUpdateHandleSymlink}
 	} else {
 		f.newPullError(file.Name, errors.Wrap(err, "symlink create"))
@@ -869,9 +869,9 @@ func (f *sendReceiveFolder) deleteFileWithCurrent(file, cur protocol.FileInfo, h
 	}
 
 	if f.versioner != nil && !cur.IsSymlink() {
-		err = inWritableDir(f.versioner.Archive, f.fs, file.Name)
+		err = f.inWritableDir(f.versioner.Archive, file.Name)
 	} else {
-		err = inWritableDir(f.fs.Remove, f.fs, file.Name)
+		err = f.inWritableDir(f.fs.Remove, file.Name)
 	}
 
 	if err == nil || fs.IsNotExist(err) {
@@ -971,7 +971,7 @@ func (f *sendReceiveFolder) renameFile(cur, source, target protocol.FileInfo, db
 		if err == nil {
 			err = osutil.Copy(f.fs, f.fs, source.Name, tempName)
 			if err == nil {
-				err = inWritableDir(f.versioner.Archive, f.fs, source.Name)
+				err = f.inWritableDir(f.versioner.Archive, source.Name)
 			}
 		}
 	} else {
@@ -1078,7 +1078,7 @@ func (f *sendReceiveFolder) handleFile(file protocol.FileInfo, copyChan chan<- c
 			// Otherwise, discard the file ourselves in order for the
 			// sharedpuller not to panic when it fails to exclusively create a
 			// file which already exists
-			inWritableDir(f.fs.Remove, f.fs, tempName)
+			f.inWritableDir(f.fs.Remove, tempName)
 		}
 	} else {
 		// Copy the blocks, as we don't want to shuffle them on the FileInfo
@@ -1522,9 +1522,9 @@ func (f *sendReceiveFolder) performFinish(file, curFile protocol.FileInfo, hasCu
 			// Directories and symlinks aren't checked for conflicts.
 
 			file.Version = file.Version.Merge(curFile.Version)
-			err = inWritableDir(func(name string) error {
+			err = f.inWritableDir(func(name string) error {
 				return f.moveForConflict(name, file.ModifiedBy.String(), scanChan)
-			}, f.fs, curFile.Name)
+			}, curFile.Name)
 		} else {
 			err = f.deleteItemOnDisk(curFile, scanChan)
 		}
@@ -1825,10 +1825,10 @@ func (f *sendReceiveFolder) deleteItemOnDisk(item protocol.FileInfo, scanChan ch
 		// an error.
 		// Symlinks aren't archived.
 
-		return inWritableDir(f.versioner.Archive, f.fs, item.Name)
+		return f.inWritableDir(f.versioner.Archive, item.Name)
 	}
 
-	return inWritableDir(f.fs.Remove, f.fs, item.Name)
+	return f.inWritableDir(f.fs.Remove, item.Name)
 }
 
 // deleteDirOnDisk attempts to delete a directory. It checks for files/dirs inside
@@ -1879,7 +1879,7 @@ func (f *sendReceiveFolder) deleteDirOnDisk(dir string, scanChan chan<- string)
 		f.fs.RemoveAll(del)
 	}
 
-	err := inWritableDir(f.fs.Remove, f.fs, dir)
+	err := f.inWritableDir(f.fs.Remove, dir)
 	if err == nil || fs.IsNotExist(err) {
 		// It was removed or it doesn't exist to start with
 		return nil
@@ -1963,6 +1963,10 @@ func (f *sendReceiveFolder) maybeCopyOwner(path string) error {
 	return nil
 }
 
+func (f *sendReceiveFolder) inWritableDir(fn func(string) error, path string) error {
+	return inWritableDir(fn, f.fs, path, f.IgnorePerms)
+}
+
 // A []FileError is sent as part of an event and will be JSON serialized.
 type FileError struct {
 	Path string `json:"path"`

lib/model/sharedpullerstate.go

@@ -8,7 +8,6 @@ package model
 
 import (
 	"io"
-	"path/filepath"
 	"time"
 
 	"github.com/pkg/errors"
@@ -92,25 +91,16 @@ func (s *sharedPullerState) tempFile() (io.WriterAt, error) {
 		return lockedWriterAt{&s.mut, s.fd}, nil
 	}
 
-	// Ensure that the parent directory is writable. This is
+	if err := inWritableDir(s.tempFileInWritableDir, s.fs, s.tempName, s.ignorePerms); err != nil {
-	// osutil.InWritableDir except we need to do more stuff so we duplicate it
+		s.failLocked(err)
-	// here.
-	dir := filepath.Dir(s.tempName)
-	if info, err := s.fs.Stat(dir); err != nil {
-		s.failLocked(errors.Wrap(err, "ensuring parent dir is writeable"))
 		return nil, err
-	} else if info.Mode()&0200 == 0 {
-		err := s.fs.Chmod(dir, 0755)
-		if !s.ignorePerms && err == nil {
-			defer func() {
-				err := s.fs.Chmod(dir, info.Mode()&fs.ModePerm)
-				if err != nil {
-					panic(err)
-				}
-			}()
-		}
 	}
 
+	return lockedWriterAt{&s.mut, s.fd}, nil
+}
+
+// tempFileInWritableDir should only be called from tempFile.
+func (s *sharedPullerState) tempFileInWritableDir(_ string) error {
 	// The permissions to use for the temporary file should be those of the
 	// final file, except we need user read & write at minimum. The
 	// permissions will be set to the final value later, but in the meantime
@@ -140,14 +130,12 @@ func (s *sharedPullerState) tempFile() (io.WriterAt, error) {
 		// what the umask dictates.
 
 		if err := s.fs.Chmod(s.tempName, mode); err != nil {
-			s.failLocked(errors.Wrap(err, "setting perms on temp file"))
+			return errors.Wrap(err, "setting perms on temp file")
-			return nil, err
 		}
 	}
 	fd, err := s.fs.OpenFile(s.tempName, flags, mode)
 	if err != nil {
-		s.failLocked(errors.Wrap(err, "opening temp file"))
+		return errors.Wrap(err, "opening temp file")
-		return nil, err
 	}
 
 	// Hide the temporary file
@@ -177,16 +165,14 @@ func (s *sharedPullerState) tempFile() (io.WriterAt, error) {
 					l.Debugln("failed to remove temporary file:", remErr)
 				}
 
-				s.failLocked(err)
+				return err
-				return nil, err
 			}
 		}
 	}
 
 	// Same fd will be used by all writers
 	s.fd = fd
-
+	return nil
-	return lockedWriterAt{&s.mut, s.fd}, nil
 }
 
 // fail sets the error on the puller state compose of error, and marks the

lib/model/util.go

@@ -66,7 +66,7 @@ func (d *deadlockDetector) Watch(name string, mut sync.Locker) {
 
 // inWritableDir calls fn(path), while making sure that the directory
 // containing `path` is writable for the duration of the call.
-func inWritableDir(fn func(string) error, targetFs fs.Filesystem, path string) error {
+func inWritableDir(fn func(string) error, targetFs fs.Filesystem, path string, ignorePerms bool) error {
 	dir := filepath.Dir(path)
 	info, err := targetFs.Stat(dir)
 	if err != nil {
@@ -86,8 +86,12 @@ func inWritableDir(fn func(string) error, targetFs fs.Filesystem, path string) e
 			// succeeded or failed on its own so returning an error to the
 			// caller is inappropriate.)
 			defer func() {
-				if err := targetFs.Chmod(dir, info.Mode()); err != nil && !fs.IsNotExist(err) {
+				if err := targetFs.Chmod(dir, info.Mode()&fs.ModePerm); err != nil && !fs.IsNotExist(err) {
-					l.Warnln("Failed to restore directory permissions after gaining write access:", err)
+					logFn := l.Warnln
+					if ignorePerms {
+						logFn = l.Debugln
+					}
+					logFn("Failed to restore directory permissions after gaining write access:", err)
 				}
 			}()
 		}

lib/model/utils_test.go

@@ -35,35 +35,35 @@ func TestInWriteableDir(t *testing.T) {
 
 	// These should succeed
 
-	err := inWritableDir(create, fs, "testdata/file")
+	err := inWritableDir(create, fs, "testdata/file", false)
 	if err != nil {
 		t.Error("testdata/file:", err)
 	}
-	err = inWritableDir(create, fs, "testdata/rw/foo")
+	err = inWritableDir(create, fs, "testdata/rw/foo", false)
 	if err != nil {
 		t.Error("testdata/rw/foo:", err)
 	}
-	err = inWritableDir(fs.Remove, fs, "testdata/rw/foo")
+	err = inWritableDir(fs.Remove, fs, "testdata/rw/foo", false)
 	if err != nil {
 		t.Error("testdata/rw/foo:", err)
 	}
 
-	err = inWritableDir(create, fs, "testdata/ro/foo")
+	err = inWritableDir(create, fs, "testdata/ro/foo", false)
 	if err != nil {
 		t.Error("testdata/ro/foo:", err)
 	}
-	err = inWritableDir(fs.Remove, fs, "testdata/ro/foo")
+	err = inWritableDir(fs.Remove, fs, "testdata/ro/foo", false)
 	if err != nil {
 		t.Error("testdata/ro/foo:", err)
 	}
 
 	// These should not
 
-	err = inWritableDir(create, fs, "testdata/nonexistent/foo")
+	err = inWritableDir(create, fs, "testdata/nonexistent/foo", false)
 	if err == nil {
 		t.Error("testdata/nonexistent/foo returned nil error")
 	}
-	err = inWritableDir(create, fs, "testdata/file/foo")
+	err = inWritableDir(create, fs, "testdata/file/foo", false)
 	if err == nil {
 		t.Error("testdata/file/foo returned nil error")
 	}
@@ -100,7 +100,7 @@ func TestOSWindowsRemove(t *testing.T) {
 	fs.Chmod("testdata/windows/ro/readonly", 0500)
 
 	for _, path := range []string{"testdata/windows/ro/readonly", "testdata/windows/ro", "testdata/windows"} {
-		err := inWritableDir(fs.Remove, fs, path)
+		err := inWritableDir(fs.Remove, fs, path, false)
 		if err != nil {
 			t.Errorf("Unexpected error %s: %s", path, err)
 		}
@@ -183,7 +183,7 @@ func TestInWritableDirWindowsRename(t *testing.T) {
 	}
 
 	for _, path := range []string{"testdata/windows/ro/readonly", "testdata/windows/ro", "testdata/windows"} {
-		err := inWritableDir(rename, fs, path)
+		err := inWritableDir(rename, fs, path, false)
 		if err != nil {
 			t.Errorf("Unexpected error %s: %s", path, err)
 		}