Enforced Windows symbolic link detection consistency with Go.

We were previously using an incorrect (or at least not completely
correct) symbolic link detection mechanism that differed from that of
the Go standard library. On Windows, where we use the os package to
drive some of our filesystem implementation, this led to spurious
symbolic link reports. This commit unifies Mutagen's symbolic link
definition with that of the Go standard library.

Note that the Go standard library still has an open issue regarding
symbolic link classification with WSL symbolic links (golang/go#42184),
so we inherit that for now, but we should be able to adjust our
definition once that issue is resolved.

Fixes #248.

Signed-off-by: Jacob Howard <[email protected]>

Author: xenoscopic

pkg/filesystem/directory_windows.go

@@ -5,6 +5,7 @@ import (
 	"path/filepath"
 	"strings"
 	"syscall"
+	"unsafe"
 
 	"github.com/pkg/errors"
 
@@ -13,6 +14,8 @@ import (
 	aclapi "github.com/hectane/go-acl/api"
 
 	osvendor "github.com/mutagen-io/mutagen/pkg/filesystem/internal/third_party/os"
+
+	fssyscall "github.com/mutagen-io/mutagen/pkg/filesystem/internal/syscall"
 )
 
 // ensureValidName verifies that the provided name does not reference the
@@ -243,27 +246,26 @@ func (d *Directory) openHandle(name string, wantDirectory bool) (string, windows
 		return "", 0, errors.Wrap(err, "unable to open path")
 	}
 
-	// Query file metadata.
-	var metadata windows.ByHandleFileInformation
-	if err := windows.GetFileInformationByHandle(handle, &metadata); err != nil {
+	// Query attribute metadata.
+	var attributes fssyscall.FileAttributeTagInfo
+	if err := windows.GetFileInformationByHandleEx(
+		handle,
+		windows.FileAttributeTagInfo,
+		(*byte)(unsafe.Pointer(&attributes)),
+		uint32(unsafe.Sizeof(attributes)),
+	); err != nil {
 		windows.CloseHandle(handle)
-		return "", 0, errors.Wrap(err, "unable to query file metadata")
-	}
-
-	// Verify that the handle does not represent a symbolic link and that the
-	// type coincides with what we want. Note that FILE_ATTRIBUTE_REPARSE_POINT
-	// can be or'd with FILE_ATTRIBUTE_DIRECTORY (since symbolic links are
-	// "typed" on Windows), so we have to explicitly exclude reparse points
-	// before checking types.
-	//
-	// TODO: Are there additional attributes upon which we should reject here?
-	// The Go os.File implementation doesn't seem to for normal os.Open
-	// operations, so I guess we don't need to either, but we should keep the
-	// option in mind.
-	if metadata.FileAttributes&windows.FILE_ATTRIBUTE_REPARSE_POINT != 0 {
+		return "", 0, errors.Wrap(err, "unable to query attribute metadata")
+	}
+
+	// Verify that the attribute metadata doesn't indicate a symbolic link.
+	if attributes.IsSymbolicLink() {
 		windows.CloseHandle(handle)
 		return "", 0, errors.New("path pointed to symbolic link")
-	} else if wantDirectory && metadata.FileAttributes&windows.FILE_ATTRIBUTE_DIRECTORY == 0 {
+	}
+
+	// Verify that the handle corresponds to a directory (if requested).
+	if wantDirectory && attributes.FileAttributes&windows.FILE_ATTRIBUTE_DIRECTORY == 0 {
 		windows.CloseHandle(handle)
 		return "", 0, errors.New("path pointed to non-directory location")
 	}

pkg/filesystem/internal/syscall/syscall_windows.go

@@ -0,0 +1,27 @@
+package syscall
+
+import (
+	"golang.org/x/sys/windows"
+)
+
+// FileAttributeTagInfo is the Go representation of FILE_ATTRIBUTE_TAG_INFO.
+type FileAttributeTagInfo struct {
+	// FileAttributes are the file attributes.
+	FileAttributes uint32
+	// ReparseTag is the file reparse tag.
+	ReparseTag uint32
+}
+
+// IsSymbolicLink returns whether or not the file attributes indicate a symbolic
+// link. This method must match the implementation of os.fileStat.isSymlink in
+// the Go standard library in order for code in the filesystem package to work
+// correctly (since it partially relies on os package functionality on Windows
+// and will see inconsistent classification behavior otherwise).
+func (i *FileAttributeTagInfo) IsSymbolicLink() bool {
+	if i.FileAttributes&windows.FILE_ATTRIBUTE_REPARSE_POINT == 0 {
+		return false
+	}
+	// TODO: Update this definition once golang/go#42184 is resolved.
+	return i.ReparseTag == windows.IO_REPARSE_TAG_SYMLINK ||
+		i.ReparseTag == windows.IO_REPARSE_TAG_MOUNT_POINT
+}

pkg/filesystem/open_windows.go

@@ -4,12 +4,15 @@ import (
 	"io"
 	"os"
 	"path/filepath"
+	"unsafe"
 
 	"github.com/pkg/errors"
 
 	"golang.org/x/sys/windows"
 
 	osvendor "github.com/mutagen-io/mutagen/pkg/filesystem/internal/third_party/os"
+
+	fssyscall "github.com/mutagen-io/mutagen/pkg/filesystem/internal/syscall"
 )
 
 // Open opens a filesystem path for traversal and operations. It will return
@@ -64,32 +67,26 @@ func Open(path string, allowSymbolicLinkLeaf bool) (io.Closer, *Metadata, error)
 		return nil, nil, errors.Wrap(err, "unable to open path")
 	}
 
-	// Query raw file metadata.
-	var rawMetadata windows.ByHandleFileInformation
-	if err := windows.GetFileInformationByHandle(handle, &rawMetadata); err != nil {
+	// Query attribute metadata.
+	var attributes fssyscall.FileAttributeTagInfo
+	if err := windows.GetFileInformationByHandleEx(
+		handle,
+		windows.FileAttributeTagInfo,
+		(*byte)(unsafe.Pointer(&attributes)),
+		uint32(unsafe.Sizeof(attributes)),
+	); err != nil {
 		windows.CloseHandle(handle)
-		return nil, nil, errors.Wrap(err, "unable to query file metadata")
+		return nil, nil, errors.Wrap(err, "unable to query attribute metadata")
 	}
 
-	// Verify that the handle does not represent a symbolic link. Even if we
-	// allow symbolic links in the leaf position of the path, we should not end
-	// up with one (it should resolve).
-	//
-	// Note that FILE_ATTRIBUTE_REPARSE_POINT can be or'd with
-	// FILE_ATTRIBUTE_DIRECTORY (since symbolic links are "typed" on Windows),
-	// so we have to explicitly exclude reparse points before checking types.
-	//
-	// TODO: Are there additional attributes upon which we should reject here?
-	// The Go os.File implementation doesn't seem to for normal os.Open
-	// operations, so I guess we don't need to either, but we should keep the
-	// option in mind.
-	if rawMetadata.FileAttributes&windows.FILE_ATTRIBUTE_REPARSE_POINT != 0 {
+	// Verify that the attribute metadata doesn't indicate a symbolic link.
+	if attributes.IsSymbolicLink() {
 		windows.CloseHandle(handle)
 		return nil, nil, ErrUnsupportedOpenType
 	}
 
 	// Determine whether or not this is a directory.
-	isDirectory := rawMetadata.FileAttributes&windows.FILE_ATTRIBUTE_DIRECTORY != 0
+	isDirectory := attributes.FileAttributes&windows.FILE_ATTRIBUTE_DIRECTORY != 0
 
 	// Handle os.File creation based on type.
 	var file *os.File