internal/fs: don't clone symlinks on windows

copyFile calls copySymlink on Windows which fails if the user doesn't
have the required permission. This is a very common case since symlinks
are used heavily on Windows.

This change renames copySymlink to cloneSymlink to clarify the intention
and skips calling it when on Windows to fallback to copy the file
content instead.

Fixes golang#773

Signed-off-by: Ibrahim AshShohail <[email protected]>

Author: ibrasho

internal/fs/fs.go

@@ -9,6 +9,7 @@ import (
 	"io/ioutil"
 	"os"
 	"path/filepath"
+	"runtime"
 	"strings"
 	"unicode"
 
@@ -269,12 +270,16 @@ func CopyDir(src, dst string) error {
 // of the source file. The file mode will be copied from the source and
 // the copied data is synced/flushed to stable storage.
 func copyFile(src, dst string) (err error) {
-	if sym, err := IsSymlink(src); err != nil {
-		return err
-	} else if sym {
-		err := copySymlink(src, dst)
+	sym, err := IsSymlink(src)
+	if err != nil {
 		return err
 	}
+	// Creating symlinks on Windows require an additional permission regular
+	// users aren't granted usually. So we skip cloning the symlink nd copy the
+	// file content as a fall back instead.
+	if sym && runtime.GOOS != "windows" {
+		return cloneSymlink(src, dst)
+	}
 
 	in, err := os.Open(src)
 	if err != nil {
@@ -314,17 +319,17 @@ func copyFile(src, dst string) (err error) {
 	return
 }
 
-// copySymlink will resolve the src symlink and create a new symlink in dst.
-// If src is a relative symlink, dst will also be a relative symlink.
-func copySymlink(src, dst string) error {
-	resolved, err := os.Readlink(src)
+// cloneSymlink will create a new symlink that points to the resolved path of sl.
+// If sl is a relative symlink, dst will also be a relative symlink.
+func cloneSymlink(sl, dst string) error {
+	resolved, err := os.Readlink(sl)
 	if err != nil {
 		return errors.Wrap(err, "failed to resolve symlink")
 	}
 
 	err = os.Symlink(resolved, dst)
 	if err != nil {
-		return errors.Wrapf(err, "failed to create symlink %s to %s", src, resolved)
+		return errors.Wrapf(err, "failed to create symlink %s to %s", dst, resolved)
 	}
 
 	return nil

internal/fs/fs_test.go

@@ -499,71 +499,48 @@ func TestCopyFile(t *testing.T) {
 }
 
 func TestCopyFileSymlink(t *testing.T) {
-	dir, err := ioutil.TempDir("", "dep")
-	if err != nil {
-		t.Fatal(err)
-	}
-	defer os.RemoveAll(dir)
-
-	srcPath := filepath.Join(dir, "src")
-	symlinkPath := filepath.Join(dir, "symlink")
-	dstPath := filepath.Join(dir, "dst")
-
-	srcf, err := os.Create(srcPath)
-	if err != nil {
-		t.Fatal(err)
-	}
-	srcf.Close()
-
-	if err = os.Symlink(srcPath, symlinkPath); err != nil {
-		t.Fatalf("could not create symlink: %s", err)
-	}
-
-	if err = copyFile(symlinkPath, dstPath); err != nil {
-		t.Fatalf("failed to copy symlink: %s", err)
-	}
-
-	resolvedPath, err := os.Readlink(dstPath)
-	if err != nil {
-		t.Fatalf("could not resolve symlink: %s", err)
-	}
-
-	if resolvedPath != srcPath {
-		t.Fatalf("resolved path is incorrect. expected %s, got %s", srcPath, resolvedPath)
-	}
-}
+	h := test.NewHelper(t)
+	defer h.Cleanup()
+	h.TempDir(".")
 
-func TestCopyFileSymlinkToDirectory(t *testing.T) {
-	dir, err := ioutil.TempDir("", "dep")
-	if err != nil {
-		t.Fatal(err)
+	testcases := map[string]string{
+		filepath.Join("./testdata/symlinks/file-symlink"):    filepath.Join(h.Path("."), "dst-file"),
+		filepath.Join("./testdata/symlinks/dir-symlink"):     filepath.Join(h.Path("."), "dst-dir"),
+		filepath.Join("./testdata/symlinks/invalid-symlink"): filepath.Join(h.Path("."), "invalid-symlink"),
 	}
-	defer os.RemoveAll(dir)
 
-	srcPath := filepath.Join(dir, "src")
-	symlinkPath := filepath.Join(dir, "symlink")
-	dstPath := filepath.Join(dir, "dst")
+	for symlink, dst := range testcases {
+		var err error
+		if err = copyFile(symlink, dst); err != nil {
+			t.Fatalf("failed to copy symlink: %s", err)
+		}
 
-	err = os.MkdirAll(srcPath, 0777)
-	if err != nil {
-		t.Fatal(err)
-	}
+		var want, got string
 
-	if err = os.Symlink(srcPath, symlinkPath); err != nil {
-		t.Fatalf("could not create symlink: %v", err)
-	}
+		if runtime.GOOS == "window" {
+			// Creating symlinks on Windows require an additional permission
+			// regular users aren't granted usually. So we copy the file
+			// content as a fall back instead of creating a real symlink.
+			srcb, err := ioutil.ReadFile(symlink)
+			h.Must(err)
+			dstb, err := ioutil.ReadFile(dst)
+			h.Must(err)
 
-	if err = copyFile(symlinkPath, dstPath); err != nil {
-		t.Fatalf("failed to copy symlink: %s", err)
-	}
+			want = string(srcb)
+			got = string(dstb)
+		} else {
+			want, err = os.Readlink(symlink)
+			h.Must(err)
 
-	resolvedPath, err := os.Readlink(dstPath)
-	if err != nil {
-		t.Fatalf("could not resolve symlink: %s", err)
-	}
+			got, err = os.Readlink(dst)
+			if err != nil {
+				t.Fatalf("could not resolve symlink: %s", err)
+			}
+		}
 
-	if resolvedPath != srcPath {
-		t.Fatalf("resolved path is incorrect. expected %s, got %s", srcPath, resolvedPath)
+		if want != got {
+			t.Fatalf("resolved path is incorrect. expected %s, got %s", want, got)
+		}
 	}
 }
 

internal/fs/testdata/symlinks/dir-symlink

@@ -0,0 +1 @@
+../../testdata

internal/fs/testdata/symlinks/file-symlink

@@ -0,0 +1 @@
+../test.file

internal/fs/testdata/symlinks/invalid-symlink

@@ -0,0 +1 @@
+/non/existing/file