Make targets writeable before removal on Windows (fixes #1610)

internal/model/rwfolder.go

@@ -509,7 +509,7 @@ func (p *rwFolder) handleDir(file protocol.FileInfo) {
 	// Most likely a file/link is getting replaced with a directory.
 	// Remove the file/link and fall through to directory creation.
 	case err == nil && (!info.IsDir() || info.Mode()&os.ModeSymlink != 0):
-		err = osutil.InWritableDir(os.Remove, realName)
+		err = osutil.InWritableDir(osutil.Remove, realName)
 		if err != nil {
 			l.Infof("Puller (folder %q, dir %q): %v", p.folder, file.Name, err)
 			return
@@ -581,11 +581,11 @@ func (p *rwFolder) deleteDir(file protocol.FileInfo) {
 		files, _ := dir.Readdirnames(-1)
 		for _, file := range files {
 			if defTempNamer.IsTemporary(file) {
-				osutil.InWritableDir(os.Remove, filepath.Join(realName, file))
+				osutil.InWritableDir(osutil.Remove, filepath.Join(realName, file))
 			}
 		}
 	}
-	err = osutil.InWritableDir(os.Remove, realName)
+	err = osutil.InWritableDir(osutil.Remove, realName)
 	if err == nil || os.IsNotExist(err) {
 		p.dbUpdates <- file
 	} else {
@@ -624,7 +624,7 @@ func (p *rwFolder) deleteFile(file protocol.FileInfo) {
 	} else if p.versioner != nil {
 		err = osutil.InWritableDir(p.versioner.Archive, realName)
 	} else {
-		err = osutil.InWritableDir(os.Remove, realName)
+		err = osutil.InWritableDir(osutil.Remove, realName)
 	}
 
 	if err != nil && !os.IsNotExist(err) {
@@ -700,7 +700,7 @@ func (p *rwFolder) renameFile(source, target protocol.FileInfo) {
 		// get rid of. Attempt to delete it instead so that we make *some*
 		// progress. The target is unhandled.
 
-		err = osutil.InWritableDir(os.Remove, from)
+		err = osutil.InWritableDir(osutil.Remove, from)
 		if err != nil {
 			l.Infof("Puller (folder %q, file %q): delete %q after failed rename: %v", p.folder, target.Name, source.Name, err)
 			return
@@ -1067,7 +1067,7 @@ func (p *rwFolder) performFinish(state *sharedPullerState) {
 	// over it, hence remove it before proceeding.
 	stat, err := osutil.Lstat(state.realName)
 	if err == nil && (stat.IsDir() || stat.Mode()&os.ModeSymlink != 0) {
-		osutil.InWritableDir(os.Remove, state.realName)
+		osutil.InWritableDir(osutil.Remove, state.realName)
 	}
 	// Replace the original content with the new one
 	err = osutil.Rename(state.tempName, state.realName)

internal/osutil/osutil.go

@@ -88,6 +88,20 @@ func InWritableDir(fn func(string) error, path string) error {
 	return fn(path)
 }
 
+// On Windows, removes the read-only attribute from the target prior deletion.
+func Remove(path string) error {
+	if runtime.GOOS == "windows" {
+		info, err := os.Stat(path)
+		if err != nil {
+			return err
+		}
+		if info.Mode()&0200 == 0 {
+			os.Chmod(path, 0700)
+		}
+	}
+	return os.Remove(path)
+}
+
 func ExpandTilde(path string) (string, error) {
 	if path == "~" {
 		return getHomeDir()

internal/osutil/osutil_test.go

@@ -8,6 +8,7 @@ package osutil_test
 
 import (
 	"os"
+	"runtime"
 	"testing"
 
 	"github.com/syncthing/syncthing/internal/osutil"
@@ -68,3 +69,97 @@ func TestInWriteableDir(t *testing.T) {
 		t.Error("testdata/file/foo returned nil error")
 	}
 }
+
+func TestInWritableDirWindowsRemove(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.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)
+
+	for _, path := range []string{"testdata/windows/ro/readonly", "testdata/windows/ro", "testdata/windows"} {
+		err := os.Remove(path)
+		if err == nil {
+			t.Errorf("Expected error %s", path)
+		}
+	}
+
+	for _, path := range []string{"testdata/windows/ro/readonly", "testdata/windows/ro", "testdata/windows"} {
+		err := osutil.InWritableDir(osutil.Remove, path)
+		if err != nil {
+			t.Errorf("Unexpected error %s: %s", path, 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.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)
+
+	for _, path := range []string{"testdata/windows/ro/readonly", "testdata/windows/ro", "testdata/windows"} {
+		err := os.Rename(path, path+"new")
+		if err == nil {
+			t.Errorf("Expected error %s", path)
+		}
+	}
+
+	rename := func(path string) error {
+		return osutil.Rename(path, path+"new")
+	}
+
+	for _, path := range []string{"testdata/windows/ro/readonly", "testdata/windows/ro", "testdata/windows"} {
+		err := osutil.InWritableDir(rename, 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)
+		}
+	}
+}