diff --git a/lib/model/folder_sendrecv.go b/lib/model/folder_sendrecv.go index 4161c33da..d47c219a1 100644 --- a/lib/model/folder_sendrecv.go +++ b/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 diff --git a/lib/model/util.go b/lib/model/util.go index cb75580bb..6738f1076 100644 --- a/lib/model/util.go +++ b/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) +} diff --git a/lib/model/utils_test.go b/lib/model/utils_test.go new file mode 100644 index 000000000..4ffa21259 --- /dev/null +++ b/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) + } + } +} diff --git a/lib/osutil/osutil.go b/lib/osutil/osutil.go index a05f9ff60..0c514a49d 100644 --- a/lib/osutil/osutil.go +++ b/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 { diff --git a/lib/osutil/osutil_test.go b/lib/osutil/osutil_test.go index f367a56ee..880074276 100644 --- a/lib/osutil/osutil_test.go +++ b/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