From 05835ed81f415de68384a22f49b3bdecc906520d Mon Sep 17 00:00:00 2001 From: Simon Frei Date: Wed, 31 Jul 2019 10:53:35 +0200 Subject: [PATCH] all: Remove potentially problematic errors from panics (fixes #5839) (#5912) --- cmd/syncthing/blockprof.go | 16 +++++++++------ cmd/syncthing/heapprof.go | 16 +++++++++------ cmd/syncthing/monitor.go | 3 ++- lib/config/config.go | 3 ++- lib/locations/locations.go | 12 ++++++++---- lib/model/folder_sendrecv.go | 34 ++++++++++++++++++-------------- lib/model/sharedpullerstate.go | 36 +++++++++++----------------------- lib/model/util.go | 10 +++++++--- lib/model/utils_test.go | 18 ++++++++--------- 9 files changed, 78 insertions(+), 70 deletions(-) diff --git a/cmd/syncthing/blockprof.go b/cmd/syncthing/blockprof.go index 42156d78d..159da9b7d 100644 --- a/cmd/syncthing/blockprof.go +++ b/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 } diff --git a/cmd/syncthing/heapprof.go b/cmd/syncthing/heapprof.go index 049d7dab7..29b71a811 100644 --- a/cmd/syncthing/heapprof.go +++ b/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 diff --git a/cmd/syncthing/monitor.go b/cmd/syncthing/monitor.go index 16e2bf3e5..7ff3b7a4d 100644 --- a/cmd/syncthing/monitor.go +++ b/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() diff --git a/lib/config/config.go b/lib/config/config.go index 6c7e3eeab..f4e7c7f0a 100644 --- a/lib/config/config.go +++ b/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 diff --git a/lib/locations/locations.go b/lib/locations/locations.go index f730dc8fb..87f889c79 100644 --- a/lib/locations/locations.go +++ b/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 } diff --git a/lib/model/folder_sendrecv.go b/lib/model/folder_sendrecv.go index d47c219a1..320541854 100644 --- a/lib/model/folder_sendrecv.go +++ b/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"` diff --git a/lib/model/sharedpullerstate.go b/lib/model/sharedpullerstate.go index 45200332e..2ef491ada 100644 --- a/lib/model/sharedpullerstate.go +++ b/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 - // osutil.InWritableDir except we need to do more stuff so we duplicate it - // 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")) + if err := inWritableDir(s.tempFileInWritableDir, s.fs, s.tempName, s.ignorePerms); err != nil { + s.failLocked(err) 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 nil, err + return errors.Wrap(err, "setting perms on temp file") } } fd, err := s.fs.OpenFile(s.tempName, flags, mode) if err != nil { - s.failLocked(errors.Wrap(err, "opening temp file")) - return nil, err + return errors.Wrap(err, "opening temp file") } // 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 nil, err + return err } } } // Same fd will be used by all writers s.fd = fd - - return lockedWriterAt{&s.mut, s.fd}, nil + return nil } // fail sets the error on the puller state compose of error, and marks the diff --git a/lib/model/util.go b/lib/model/util.go index 6738f1076..52a190f2a 100644 --- a/lib/model/util.go +++ b/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) { - l.Warnln("Failed to restore directory permissions after gaining write access:", err) + if err := targetFs.Chmod(dir, info.Mode()&fs.ModePerm); err != nil && !fs.IsNotExist(err) { + logFn := l.Warnln + if ignorePerms { + logFn = l.Debugln + } + logFn("Failed to restore directory permissions after gaining write access:", err) } }() } diff --git a/lib/model/utils_test.go b/lib/model/utils_test.go index 4ffa21259..b9e628043 100644 --- a/lib/model/utils_test.go +++ b/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) }