lib/model: Don't panic on failed chmod-back on directory (fixes #5836) (#5896)

* lib/model: Don't panic on failed chmod-back on directory (fixes #5836)

This makes the "in writable dir"-wrapper log chmod-back errors instead
of panicking. To do that we need a logger so the function moved into the
model package which is also the only place it's used. The tests came
along.

(The test also exercised osutil.RenameOrCopy like some sort of
piggybacking. I removed that.)

lib/model/folder_sendrecv.go

@@ -594,7 +594,7 @@ func (f *sendReceiveFolder) handleDir(file protocol.FileInfo, dbUpdateChan chan<
 			// Symlinks aren't checked for conflicts.
 
 			file.Version = file.Version.Merge(curFile.Version)
-			err = osutil.InWritableDir(func(name string) error {
+			err = inWritableDir(func(name string) error {
 				return f.moveForConflict(name, file.ModifiedBy.String(), scanChan)
 			}, f.fs, curFile.Name)
 		} else {
@@ -633,7 +633,7 @@ func (f *sendReceiveFolder) handleDir(file protocol.FileInfo, dbUpdateChan chan<
 			return f.fs.Chmod(path, mode|(info.Mode()&retainBits))
 		}
 
-		if err = osutil.InWritableDir(mkdir, f.fs, file.Name); err == nil {
+		if err = inWritableDir(mkdir, f.fs, file.Name); err == nil {
 			dbUpdateChan <- dbUpdateJob{file, dbUpdateHandleDir}
 		} else {
 			f.newPullError(file.Name, errors.Wrap(err, "creating directory"))
@@ -748,7 +748,7 @@ 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 = osutil.InWritableDir(func(name string) error {
+			err = inWritableDir(func(name string) error {
 				return f.moveForConflict(name, file.ModifiedBy.String(), scanChan)
 			}, f.fs, curFile.Name)
 		} else {
@@ -769,7 +769,7 @@ func (f *sendReceiveFolder) handleSymlink(file protocol.FileInfo, dbUpdateChan c
 		return f.maybeCopyOwner(path)
 	}
 
-	if err = osutil.InWritableDir(createLink, f.fs, file.Name); err == nil {
+	if err = inWritableDir(createLink, f.fs, 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 = osutil.InWritableDir(f.versioner.Archive, f.fs, file.Name)
+		err = inWritableDir(f.versioner.Archive, f.fs, file.Name)
 	} else {
-		err = osutil.InWritableDir(f.fs.Remove, f.fs, file.Name)
+		err = inWritableDir(f.fs.Remove, f.fs, 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 = osutil.InWritableDir(f.versioner.Archive, f.fs, source.Name)
+				err = inWritableDir(f.versioner.Archive, f.fs, 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
-			osutil.InWritableDir(f.fs.Remove, f.fs, tempName)
+			inWritableDir(f.fs.Remove, f.fs, tempName)
 		}
 	} else {
 		// Copy the blocks, as we don't want to shuffle them on the FileInfo
@@ -1522,7 +1522,7 @@ 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 = osutil.InWritableDir(func(name string) error {
+			err = inWritableDir(func(name string) error {
 				return f.moveForConflict(name, file.ModifiedBy.String(), scanChan)
 			}, f.fs, curFile.Name)
 		} else {
@@ -1825,10 +1825,10 @@ func (f *sendReceiveFolder) deleteItemOnDisk(item protocol.FileInfo, scanChan ch
 		// an error.
 		// Symlinks aren't archived.
 
-		return osutil.InWritableDir(f.versioner.Archive, f.fs, item.Name)
+		return inWritableDir(f.versioner.Archive, f.fs, item.Name)
 	}
 
-	return osutil.InWritableDir(f.fs.Remove, f.fs, item.Name)
+	return inWritableDir(f.fs.Remove, f.fs, 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 := osutil.InWritableDir(f.fs.Remove, f.fs, dir)
+	err := inWritableDir(f.fs.Remove, f.fs, dir)
 	if err == nil || fs.IsNotExist(err) {
 		// It was removed or it doesn't exist to start with
 		return nil

lib/model/util.go

@@ -8,8 +8,12 @@ package model
 
 import (
 	"fmt"
+	"path/filepath"
 	"sync"
 	"time"
+
+	"github.com/pkg/errors"
+	"github.com/syncthing/syncthing/lib/fs"
 )
 
 type Holdable interface {
@@ -59,3 +63,35 @@ 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 {
+	dir := filepath.Dir(path)
+	info, err := targetFs.Stat(dir)
+	if err != nil {
+		return err
+	}
+	if !info.IsDir() {
+		return errors.New("Not a directory: " + path)
+	}
+	if info.Mode()&0200 == 0 {
+		// A non-writeable directory (for this user; we assume that's the
+		// relevant part). Temporarily change the mode so we can delete the
+		// file or directory inside it.
+		if err := targetFs.Chmod(dir, 0755); err == nil {
+			// Chmod succeeded, we should change the permissions back on the way
+			// out. If we fail we log the error as we have irrevocably messed up
+			// at this point. :( (The operation we were called to wrap has
+			// 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)
+				}
+			}()
+		}
+	}
+
+	return fn(path)
+}

lib/model/utils_test.go

@@ -0,0 +1,195 @@
+// Copyright (C) 2019 The Syncthing Authors.
+//
+// This Source Code Form is subject to the terms of the Mozilla Public
+// License, v. 2.0. If a copy of the MPL was not distributed with this file,
+// You can obtain one at https://mozilla.org/MPL/2.0/.
+
+package model
+
+import (
+	"os"
+	"runtime"
+	"testing"
+
+	"github.com/syncthing/syncthing/lib/fs"
+)
+
+func TestInWriteableDir(t *testing.T) {
+	dir := createTmpDir()
+	defer os.RemoveAll(dir)
+
+	fs := fs.NewFilesystem(fs.FilesystemTypeBasic, dir)
+
+	fs.Mkdir("testdata", 0700)
+	fs.Mkdir("testdata/rw", 0700)
+	fs.Mkdir("testdata/ro", 0500)
+
+	create := func(name string) error {
+		fd, err := fs.Create(name)
+		if err != nil {
+			return err
+		}
+		fd.Close()
+		return nil
+	}
+
+	// These should succeed
+
+	err := inWritableDir(create, fs, "testdata/file")
+	if err != nil {
+		t.Error("testdata/file:", err)
+	}
+	err = inWritableDir(create, fs, "testdata/rw/foo")
+	if err != nil {
+		t.Error("testdata/rw/foo:", err)
+	}
+	err = inWritableDir(fs.Remove, fs, "testdata/rw/foo")
+	if err != nil {
+		t.Error("testdata/rw/foo:", err)
+	}
+
+	err = inWritableDir(create, fs, "testdata/ro/foo")
+	if err != nil {
+		t.Error("testdata/ro/foo:", err)
+	}
+	err = inWritableDir(fs.Remove, fs, "testdata/ro/foo")
+	if err != nil {
+		t.Error("testdata/ro/foo:", err)
+	}
+
+	// These should not
+
+	err = inWritableDir(create, fs, "testdata/nonexistent/foo")
+	if err == nil {
+		t.Error("testdata/nonexistent/foo returned nil error")
+	}
+	err = inWritableDir(create, fs, "testdata/file/foo")
+	if err == nil {
+		t.Error("testdata/file/foo returned nil error")
+	}
+}
+
+func TestOSWindowsRemove(t *testing.T) {
+	// os.Remove should remove read only things on windows
+
+	if runtime.GOOS != "windows" {
+		t.Skipf("Tests not required")
+		return
+	}
+
+	dir := createTmpDir()
+	defer os.RemoveAll(dir)
+
+	fs := fs.NewFilesystem(fs.FilesystemTypeBasic, dir)
+	defer fs.Chmod("testdata/windows/ro/readonlynew", 0700)
+
+	create := func(name string) error {
+		fd, err := fs.Create(name)
+		if err != nil {
+			return err
+		}
+		fd.Close()
+		return nil
+	}
+
+	fs.Mkdir("testdata", 0700)
+
+	fs.Mkdir("testdata/windows", 0500)
+	fs.Mkdir("testdata/windows/ro", 0500)
+	create("testdata/windows/ro/readonly")
+	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)
+		if err != nil {
+			t.Errorf("Unexpected error %s: %s", path, err)
+		}
+	}
+}
+
+func TestOSWindowsRemoveAll(t *testing.T) {
+	// os.RemoveAll should remove read only things on windows
+
+	if runtime.GOOS != "windows" {
+		t.Skipf("Tests not required")
+		return
+	}
+
+	dir := createTmpDir()
+	defer os.RemoveAll(dir)
+
+	fs := fs.NewFilesystem(fs.FilesystemTypeBasic, dir)
+	defer fs.Chmod("testdata/windows/ro/readonlynew", 0700)
+
+	create := func(name string) error {
+		fd, err := fs.Create(name)
+		if err != nil {
+			return err
+		}
+		fd.Close()
+		return nil
+	}
+
+	fs.Mkdir("testdata", 0700)
+
+	fs.Mkdir("testdata/windows", 0500)
+	fs.Mkdir("testdata/windows/ro", 0500)
+	create("testdata/windows/ro/readonly")
+	fs.Chmod("testdata/windows/ro/readonly", 0500)
+
+	if err := fs.RemoveAll("testdata/windows"); err != nil {
+		t.Errorf("Unexpected error: %s", err)
+	}
+}
+
+func TestInWritableDirWindowsRename(t *testing.T) {
+	if runtime.GOOS != "windows" {
+		t.Skipf("Tests not required")
+		return
+	}
+
+	dir := createTmpDir()
+	defer os.RemoveAll(dir)
+
+	fs := fs.NewFilesystem(fs.FilesystemTypeBasic, dir)
+	defer fs.Chmod("testdata/windows/ro/readonlynew", 0700)
+
+	create := func(name string) error {
+		fd, err := fs.Create(name)
+		if err != nil {
+			return err
+		}
+		fd.Close()
+		return nil
+	}
+
+	fs.Mkdir("testdata", 0700)
+
+	fs.Mkdir("testdata/windows", 0500)
+	fs.Mkdir("testdata/windows/ro", 0500)
+	create("testdata/windows/ro/readonly")
+	fs.Chmod("testdata/windows/ro/readonly", 0500)
+
+	for _, path := range []string{"testdata/windows/ro/readonly", "testdata/windows/ro", "testdata/windows"} {
+		err := fs.Rename(path, path+"new")
+		if err == nil {
+			t.Skipf("seem like this test doesn't work here")
+			return
+		}
+	}
+
+	rename := func(path string) error {
+		return fs.Rename(path, path+"new")
+	}
+
+	for _, path := range []string{"testdata/windows/ro/readonly", "testdata/windows/ro", "testdata/windows"} {
+		err := inWritableDir(rename, fs, path)
+		if err != nil {
+			t.Errorf("Unexpected error %s: %s", path, err)
+		}
+		_, err = fs.Stat(path + "new")
+		if err != nil {
+			t.Errorf("Unexpected error %s: %s", path, err)
+		}
+	}
+}

lib/osutil/osutil.go

@@ -8,7 +8,6 @@
 package osutil
 
 import (
-	"errors"
 	"io"
 	"path/filepath"
 	"runtime"
@@ -81,38 +80,6 @@ func Copy(src, dst fs.Filesystem, from, to string) (err error) {
 	})
 }
 
-// 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, fs fs.Filesystem, path string) error {
-	dir := filepath.Dir(path)
-	info, err := fs.Stat(dir)
-	if err != nil {
-		return err
-	}
-	if !info.IsDir() {
-		return errors.New("Not a directory: " + path)
-	}
-	if info.Mode()&0200 == 0 {
-		// A non-writeable directory (for this user; we assume that's the
-		// relevant part). Temporarily change the mode so we can delete the
-		// file or directory inside it.
-		err = fs.Chmod(dir, 0755)
-		if err == nil {
-			defer func() {
-				err = fs.Chmod(dir, info.Mode())
-				if err != nil {
-					// We managed to change the permission bits like a
-					// millisecond ago, so it'd be bizarre if we couldn't
-					// change it back.
-					panic(err)
-				}
-			}()
-		}
-	}
-
-	return fn(path)
-}
-
 // Tries hard to succeed on various systems by temporarily tweaking directory
 // permissions and removing the destination file when necessary.
 func withPreparedTarget(filesystem fs.Filesystem, from, to string, f func() error) error {

lib/osutil/osutil_test.go

@@ -18,196 +18,6 @@ import (
 	"github.com/syncthing/syncthing/lib/osutil"
 )
 
-func TestInWriteableDir(t *testing.T) {
-	err := os.RemoveAll("testdata")
-	if err != nil {
-		t.Fatal(err)
-	}
-	defer os.RemoveAll("testdata")
-
-	fs := fs.NewFilesystem(fs.FilesystemTypeBasic, ".")
-
-	os.Mkdir("testdata", 0700)
-	os.Mkdir("testdata/rw", 0700)
-	os.Mkdir("testdata/ro", 0500)
-
-	create := func(name string) error {
-		fd, err := os.Create(name)
-		if err != nil {
-			return err
-		}
-		fd.Close()
-		return nil
-	}
-
-	// These should succeed
-
-	err = osutil.InWritableDir(create, fs, "testdata/file")
-	if err != nil {
-		t.Error("testdata/file:", err)
-	}
-	err = osutil.InWritableDir(create, fs, "testdata/rw/foo")
-	if err != nil {
-		t.Error("testdata/rw/foo:", err)
-	}
-	err = osutil.InWritableDir(os.Remove, fs, "testdata/rw/foo")
-	if err != nil {
-		t.Error("testdata/rw/foo:", err)
-	}
-
-	err = osutil.InWritableDir(create, fs, "testdata/ro/foo")
-	if err != nil {
-		t.Error("testdata/ro/foo:", err)
-	}
-	err = osutil.InWritableDir(os.Remove, fs, "testdata/ro/foo")
-	if err != nil {
-		t.Error("testdata/ro/foo:", err)
-	}
-
-	// These should not
-
-	err = osutil.InWritableDir(create, fs, "testdata/nonexistent/foo")
-	if err == nil {
-		t.Error("testdata/nonexistent/foo returned nil error")
-	}
-	err = osutil.InWritableDir(create, fs, "testdata/file/foo")
-	if err == nil {
-		t.Error("testdata/file/foo returned nil error")
-	}
-}
-
-func TestInWritableDirWindowsRemove(t *testing.T) {
-	// os.Remove should remove read only things on windows
-
-	if runtime.GOOS != "windows" {
-		t.Skipf("Tests not required")
-		return
-	}
-
-	err := os.RemoveAll("testdata")
-	if err != nil {
-		t.Fatal(err)
-	}
-	defer os.Chmod("testdata/windows/ro/readonlynew", 0700)
-	defer os.RemoveAll("testdata")
-
-	create := func(name string) error {
-		fd, err := os.Create(name)
-		if err != nil {
-			return err
-		}
-		fd.Close()
-		return nil
-	}
-
-	os.Mkdir("testdata", 0700)
-
-	os.Mkdir("testdata/windows", 0500)
-	os.Mkdir("testdata/windows/ro", 0500)
-	create("testdata/windows/ro/readonly")
-	os.Chmod("testdata/windows/ro/readonly", 0500)
-
-	fs := fs.NewFilesystem(fs.FilesystemTypeBasic, ".")
-
-	for _, path := range []string{"testdata/windows/ro/readonly", "testdata/windows/ro", "testdata/windows"} {
-		err := osutil.InWritableDir(os.Remove, fs, path)
-		if err != nil {
-			t.Errorf("Unexpected error %s: %s", path, err)
-		}
-	}
-}
-
-func TestInWritableDirWindowsRemoveAll(t *testing.T) {
-	// os.RemoveAll should remove read only things on windows
-
-	if runtime.GOOS != "windows" {
-		t.Skipf("Tests not required")
-		return
-	}
-
-	err := os.RemoveAll("testdata")
-	if err != nil {
-		t.Fatal(err)
-	}
-	defer os.Chmod("testdata/windows/ro/readonlynew", 0700)
-	defer os.RemoveAll("testdata")
-
-	create := func(name string) error {
-		fd, err := os.Create(name)
-		if err != nil {
-			return err
-		}
-		fd.Close()
-		return nil
-	}
-
-	os.Mkdir("testdata", 0700)
-
-	os.Mkdir("testdata/windows", 0500)
-	os.Mkdir("testdata/windows/ro", 0500)
-	create("testdata/windows/ro/readonly")
-	os.Chmod("testdata/windows/ro/readonly", 0500)
-
-	if err := os.RemoveAll("testdata/windows"); err != nil {
-		t.Errorf("Unexpected error: %s", err)
-	}
-}
-
-func TestInWritableDirWindowsRename(t *testing.T) {
-	if runtime.GOOS != "windows" {
-		t.Skipf("Tests not required")
-		return
-	}
-
-	err := os.RemoveAll("testdata")
-	if err != nil {
-		t.Fatal(err)
-	}
-	defer os.Chmod("testdata/windows/ro/readonlynew", 0700)
-	defer os.RemoveAll("testdata")
-
-	create := func(name string) error {
-		fd, err := os.Create(name)
-		if err != nil {
-			return err
-		}
-		fd.Close()
-		return nil
-	}
-
-	os.Mkdir("testdata", 0700)
-
-	os.Mkdir("testdata/windows", 0500)
-	os.Mkdir("testdata/windows/ro", 0500)
-	create("testdata/windows/ro/readonly")
-	os.Chmod("testdata/windows/ro/readonly", 0500)
-
-	fs := fs.NewFilesystem(fs.FilesystemTypeBasic, ".")
-
-	for _, path := range []string{"testdata/windows/ro/readonly", "testdata/windows/ro", "testdata/windows"} {
-		err := os.Rename(path, path+"new")
-		if err == nil {
-			t.Skipf("seem like this test doesn't work here")
-			return
-		}
-	}
-
-	rename := func(path string) error {
-		return osutil.RenameOrCopy(fs, fs, path, path+"new")
-	}
-
-	for _, path := range []string{"testdata/windows/ro/readonly", "testdata/windows/ro", "testdata/windows"} {
-		err := osutil.InWritableDir(rename, fs, path)
-		if err != nil {
-			t.Errorf("Unexpected error %s: %s", path, err)
-		}
-		_, err = os.Stat(path + "new")
-		if err != nil {
-			t.Errorf("Unexpected error %s: %s", path, err)
-		}
-	}
-}
-
 func TestIsDeleted(t *testing.T) {
 	type tc struct {
 		path  string