os: use WIN32_FIND_DATA.Reserved0 to identify symlinks

os.Stat implementation uses instructions described at
https://blogs.msdn.microsoft.com/oldnewthing/20100212-00/?p=14963/
to distinguish symlinks. In particular, it calls
GetFileAttributesEx or FindFirstFile and checks
either WIN32_FILE_ATTRIBUTE_DATA.dwFileAttributes
or WIN32_FIND_DATA.dwFileAttributes to see if
FILE_ATTRIBUTES_REPARSE_POINT flag is set.
And that seems to worked fine so far.

But now we discovered that OneDrive root folder
is determined as directory:

c:\>dir C:\Users\Alex | grep OneDrive
30/11/2017  07:25 PM    <DIR>          OneDrive
c:\>

while Go identified it as symlink.

But we did not follow Microsoft's advice to the letter - we never
checked WIN32_FIND_DATA.Reserved0. And adding that extra check
makes Go treat OneDrive as symlink. So use FindFirstFile and
WIN32_FIND_DATA.Reserved0 to determine symlinks.

Fixes #22579

Change-Id: I0cb88929eb8b47b1d24efaf1907ad5a0e20de83f
Reviewed-on: https://go-review.googlesource.com/86556
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>

src/os/dir_windows.go

@@ -46,19 +46,10 @@ func (file *File) readdir(n int) (fi []FileInfo, err error) {
 		if name == "." || name == ".." { // Useless names
 			continue
 		}
-		f := &fileStat{
-			name: name,
-			sys: syscall.Win32FileAttributeData{
-				FileAttributes: d.FileAttributes,
-				CreationTime:   d.CreationTime,
-				LastAccessTime: d.LastAccessTime,
-				LastWriteTime:  d.LastWriteTime,
-				FileSizeHigh:   d.FileSizeHigh,
-				FileSizeLow:    d.FileSizeLow,
-			},
-			path:             file.dirinfo.path,
-			appendNameToPath: true,
-		}
+		f := newFileStatFromWin32finddata(d)
+		f.name = name
+		f.path = file.dirinfo.path
+		f.appendNameToPath = true
 		n--
 		fi = append(fi, f)
 	}

src/os/os_windows_test.go

@@ -8,6 +8,7 @@ import (
 	"fmt"
 	"internal/poll"
 	"internal/syscall/windows"
+	"internal/syscall/windows/registry"
 	"internal/testenv"
 	"io"
 	"io/ioutil"
@@ -893,3 +894,88 @@ func main() {
 		}
 	}
 }
+
+func testIsDir(t *testing.T, path string, fi os.FileInfo) {
+	t.Helper()
+	if !fi.IsDir() {
+		t.Errorf("%q should be a directory", path)
+	}
+	if fi.Mode()&os.ModeSymlink != 0 {
+		t.Errorf("%q should not be a symlink", path)
+	}
+}
+
+func findOneDriveDir() (string, error) {
+	// as per https://stackoverflow.com/questions/42519624/how-to-determine-location-of-onedrive-on-windows-7-and-8-in-c
+	const onedrivekey = `SOFTWARE\Microsoft\OneDrive`
+	k, err := registry.OpenKey(registry.CURRENT_USER, onedrivekey, registry.READ)
+	if err != nil {
+		return "", fmt.Errorf("OpenKey(%q) failed: %v", onedrivekey, err)
+	}
+	defer k.Close()
+
+	path, _, err := k.GetStringValue("UserFolder")
+	if err != nil {
+		return "", fmt.Errorf("reading UserFolder failed: %v", err)
+	}
+	return path, nil
+}
+
+// TestOneDrive verifies that OneDrive folder is a directory and not a symlink.
+func TestOneDrive(t *testing.T) {
+	dir, err := findOneDriveDir()
+	if err != nil {
+		t.Skipf("Skipping, because we did not find OneDrive directory: %v", err)
+	}
+
+	// test os.Stat
+	fi, err := os.Stat(dir)
+	if err != nil {
+		t.Fatal(err)
+	}
+	testIsDir(t, dir, fi)
+
+	// test os.Lstat
+	fi, err = os.Lstat(dir)
+	if err != nil {
+		t.Fatal(err)
+	}
+	testIsDir(t, dir, fi)
+
+	// test os.File.Stat
+	f, err := os.Open(dir)
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer f.Close()
+
+	fi, err = f.Stat()
+	if err != nil {
+		t.Fatal(err)
+	}
+	testIsDir(t, dir, fi)
+
+	// test os.FileInfo returned by os.Readdir
+	parent, err := os.Open(filepath.Dir(dir))
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer parent.Close()
+
+	fis, err := parent.Readdir(-1)
+	if err != nil {
+		t.Fatal(err)
+	}
+	fi = nil
+	base := filepath.Base(dir)
+	for _, fi2 := range fis {
+		if fi2.Name() == base {
+			fi = fi2
+			break
+		}
+	}
+	if fi == nil {
+		t.Errorf("failed to find %q in its parent", dir)
+	}
+	testIsDir(t, dir, fi)
+}

src/os/stat_windows.go

@@ -5,9 +5,7 @@
 package os
 
 import (
-	"internal/syscall/windows"
 	"syscall"
-	"unsafe"
 )
 
 // Stat returns the FileInfo structure describing file.
@@ -34,26 +32,12 @@ func (file *File) Stat() (FileInfo, error) {
 		return &fileStat{name: basename(file.name), filetype: ft}, nil
 	}
 
-	var d syscall.ByHandleFileInformation
-	err = file.pfd.GetFileInformationByHandle(&d)
+	fs, err := newFileStatFromGetFileInformationByHandle(file.name, file.pfd.Sysfd)
 	if err != nil {
-		return nil, &PathError{"GetFileInformationByHandle", file.name, err}
+		return nil, err
 	}
-	return &fileStat{
-		name: basename(file.name),
-		sys: syscall.Win32FileAttributeData{
-			FileAttributes: d.FileAttributes,
-			CreationTime:   d.CreationTime,
-			LastAccessTime: d.LastAccessTime,
-			LastWriteTime:  d.LastWriteTime,
-			FileSizeHigh:   d.FileSizeHigh,
-			FileSizeLow:    d.FileSizeLow,
-		},
-		filetype: ft,
-		vol:      d.VolumeSerialNumber,
-		idxhi:    d.FileIndexHigh,
-		idxlo:    d.FileIndexLow,
-	}, nil
+	fs.filetype = ft
+	return fs, err
 }
 
 // statNolog implements Stat for Windows.
@@ -68,91 +52,27 @@ func statNolog(name string) (FileInfo, error) {
 	if err != nil {
 		return nil, &PathError{"Stat", name, err}
 	}
-	// Apparently (see https://golang.org/issues/19922#issuecomment-300031421)
-	// GetFileAttributesEx is fastest approach to get file info.
-	// It does not work for symlinks. But symlinks are rare,
-	// so try GetFileAttributesEx first.
-	var fs fileStat
-	err = syscall.GetFileAttributesEx(namep, syscall.GetFileExInfoStandard, (*byte)(unsafe.Pointer(&fs.sys)))
-	if err == nil && fs.sys.FileAttributes&syscall.FILE_ATTRIBUTE_REPARSE_POINT == 0 {
-		fs.path = name
-		if !isAbs(fs.path) {
-			fs.path, err = syscall.FullPath(fs.path)
-			if err != nil {
-				return nil, &PathError{"FullPath", name, err}
-			}
+	fs, err := newFileStatFromGetFileAttributesExOrFindFirstFile(name, namep)
+	if err != nil {
+		return nil, err
+	}
+	if !fs.isSymlink() {
+		err = fs.updatePathAndName(name)
+		if err != nil {
+			return nil, err
 		}
-		fs.name = basename(name)
-		return &fs, nil
+		return fs, nil
 	}
 	// Use Windows I/O manager to dereference the symbolic link, as per
 	// https://blogs.msdn.microsoft.com/oldnewthing/20100212-00/?p=14963/
 	h, err := syscall.CreateFile(namep, 0, 0, nil,
 		syscall.OPEN_EXISTING, syscall.FILE_FLAG_BACKUP_SEMANTICS, 0)
 	if err != nil {
-		if err == windows.ERROR_SHARING_VIOLATION {
-			// try FindFirstFile now that CreateFile failed
-			return statWithFindFirstFile(name, namep)
-		}
 		return nil, &PathError{"CreateFile", name, err}
 	}
 	defer syscall.CloseHandle(h)
 
-	var d syscall.ByHandleFileInformation
-	err = syscall.GetFileInformationByHandle(h, &d)
-	if err != nil {
-		return nil, &PathError{"GetFileInformationByHandle", name, err}
-	}
-	return &fileStat{
-		name: basename(name),
-		sys: syscall.Win32FileAttributeData{
-			FileAttributes: d.FileAttributes,
-			CreationTime:   d.CreationTime,
-			LastAccessTime: d.LastAccessTime,
-			LastWriteTime:  d.LastWriteTime,
-			FileSizeHigh:   d.FileSizeHigh,
-			FileSizeLow:    d.FileSizeLow,
-		},
-		vol:   d.VolumeSerialNumber,
-		idxhi: d.FileIndexHigh,
-		idxlo: d.FileIndexLow,
-		// fileStat.path is used by os.SameFile to decide if it needs
-		// to fetch vol, idxhi and idxlo. But these are already set,
-		// so set fileStat.path to "" to prevent os.SameFile doing it again.
-		// Also do not set fileStat.filetype, because it is only used for
-		// console and stdin/stdout. But you cannot call os.Stat for these.
-	}, nil
-}
-
-// statWithFindFirstFile is used by Stat to handle special case of statting
-// c:\pagefile.sys. We might discover that other files need similar treatment.
-func statWithFindFirstFile(name string, namep *uint16) (FileInfo, error) {
-	var fd syscall.Win32finddata
-	h, err := syscall.FindFirstFile(namep, &fd)
-	if err != nil {
-		return nil, &PathError{"FindFirstFile", name, err}
-	}
-	syscall.FindClose(h)
-
-	fullpath := name
-	if !isAbs(fullpath) {
-		fullpath, err = syscall.FullPath(fullpath)
-		if err != nil {
-			return nil, &PathError{"FullPath", name, err}
-		}
-	}
-	return &fileStat{
-		name: basename(name),
-		path: fullpath,
-		sys: syscall.Win32FileAttributeData{
-			FileAttributes: fd.FileAttributes,
-			CreationTime:   fd.CreationTime,
-			LastAccessTime: fd.LastAccessTime,
-			LastWriteTime:  fd.LastWriteTime,
-			FileSizeHigh:   fd.FileSizeHigh,
-			FileSizeLow:    fd.FileSizeLow,
-		},
-	}, nil
+	return newFileStatFromGetFileInformationByHandle(name, h)
 }
 
 // lstatNolog implements Lstat for Windows.
@@ -163,25 +83,17 @@ func lstatNolog(name string) (FileInfo, error) {
 	if name == DevNull {
 		return &devNullStat, nil
 	}
-	fs := &fileStat{name: basename(name)}
-	namep, e := syscall.UTF16PtrFromString(fixLongPath(name))
-	if e != nil {
-		return nil, &PathError{"Lstat", name, e}
+	namep, err := syscall.UTF16PtrFromString(fixLongPath(name))
+	if err != nil {
+		return nil, &PathError{"Lstat", name, err}
 	}
-	e = syscall.GetFileAttributesEx(namep, syscall.GetFileExInfoStandard, (*byte)(unsafe.Pointer(&fs.sys)))
-	if e != nil {
-		if e != windows.ERROR_SHARING_VIOLATION {
-			return nil, &PathError{"GetFileAttributesEx", name, e}
-		}
-		// try FindFirstFile now that GetFileAttributesEx failed
-		return statWithFindFirstFile(name, namep)
+	fs, err := newFileStatFromGetFileAttributesExOrFindFirstFile(name, namep)
+	if err != nil {
+		return nil, err
 	}
-	fs.path = name
-	if !isAbs(fs.path) {
-		fs.path, e = syscall.FullPath(fs.path)
-		if e != nil {
-			return nil, &PathError{"FullPath", name, e}
-		}
+	err = fs.updatePathAndName(name)
+	if err != nil {
+		return nil, err
 	}
 	return fs, nil
 }

src/os/types_windows.go

@@ -5,16 +5,30 @@
 package os
 
 import (
+	"internal/syscall/windows"
 	"sync"
 	"syscall"
 	"time"
+	"unsafe"
 )
 
 // A fileStat is the implementation of FileInfo returned by Stat and Lstat.
 type fileStat struct {
-	name     string
-	sys      syscall.Win32FileAttributeData
-	filetype uint32 // what syscall.GetFileType returns
+	name string
+
+	// from ByHandleFileInformation, Win32FileAttributeData and Win32finddata
+	FileAttributes uint32
+	CreationTime   syscall.Filetime
+	LastAccessTime syscall.Filetime
+	LastWriteTime  syscall.Filetime
+	FileSizeHigh   uint32
+	FileSizeLow    uint32
+
+	// from Win32finddata
+	Reserved0 uint32
+
+	// what syscall.GetFileType returns
+	filetype uint32
 
 	// used to implement SameFile
 	sync.Mutex
@@ -25,23 +39,126 @@ type fileStat struct {
 	appendNameToPath bool
 }
 
+// newFileStatFromGetFileInformationByHandle calls GetFileInformationByHandle
+// to gather all required information about the file handle h.
+func newFileStatFromGetFileInformationByHandle(path string, h syscall.Handle) (fs *fileStat, err error) {
+	var d syscall.ByHandleFileInformation
+	err = syscall.GetFileInformationByHandle(h, &d)
+	if err != nil {
+		return nil, &PathError{"GetFileInformationByHandle", path, err}
+	}
+	return &fileStat{
+		name:           basename(path),
+		FileAttributes: d.FileAttributes,
+		CreationTime:   d.CreationTime,
+		LastAccessTime: d.LastAccessTime,
+		LastWriteTime:  d.LastWriteTime,
+		FileSizeHigh:   d.FileSizeHigh,
+		FileSizeLow:    d.FileSizeLow,
+		vol:            d.VolumeSerialNumber,
+		idxhi:          d.FileIndexHigh,
+		idxlo:          d.FileIndexLow,
+		// fileStat.path is used by os.SameFile to decide if it needs
+		// to fetch vol, idxhi and idxlo. But these are already set,
+		// so set fileStat.path to "" to prevent os.SameFile doing it again.
+	}, nil
+}
+
+// newFileStatFromWin32finddata copies all required information
+// from syscall.Win32finddata d into the newly created fileStat.
+func newFileStatFromWin32finddata(d *syscall.Win32finddata) *fileStat {
+	return &fileStat{
+		FileAttributes: d.FileAttributes,
+		CreationTime:   d.CreationTime,
+		LastAccessTime: d.LastAccessTime,
+		LastWriteTime:  d.LastWriteTime,
+		FileSizeHigh:   d.FileSizeHigh,
+		FileSizeLow:    d.FileSizeLow,
+		Reserved0:      d.Reserved0,
+	}
+}
+
+// newFileStatFromGetFileAttributesExOrFindFirstFile calls GetFileAttributesEx
+// and FindFirstFile to gather all required information about the provided file path pathp.
+func newFileStatFromGetFileAttributesExOrFindFirstFile(path string, pathp *uint16) (*fileStat, error) {
+	// As suggested by Microsoft, use GetFileAttributes() to acquire the file information,
+	// and if it's a reparse point use FindFirstFile() to get the tag:
+	// https://msdn.microsoft.com/en-us/library/windows/desktop/aa363940(v=vs.85).aspx
+	// Notice that always calling FindFirstFile can create performance problems
+	// (https://golang.org/issues/19922#issuecomment-300031421)
+	var fa syscall.Win32FileAttributeData
+	err := syscall.GetFileAttributesEx(pathp, syscall.GetFileExInfoStandard, (*byte)(unsafe.Pointer(&fa)))
+	if err == nil && fa.FileAttributes&syscall.FILE_ATTRIBUTE_REPARSE_POINT == 0 {
+		// Not a symlink.
+		return &fileStat{
+			FileAttributes: fa.FileAttributes,
+			CreationTime:   fa.CreationTime,
+			LastAccessTime: fa.LastAccessTime,
+			LastWriteTime:  fa.LastWriteTime,
+			FileSizeHigh:   fa.FileSizeHigh,
+			FileSizeLow:    fa.FileSizeLow,
+		}, nil
+	}
+	// We might have symlink here. But some directories also have
+	// FileAttributes FILE_ATTRIBUTE_REPARSE_POINT bit set.
+	// For example, OneDrive directory is like that
+	// (see golang.org/issue/22579 for details).
+	// So use FindFirstFile instead to distinguish directories like
+	// OneDrive from real symlinks (see instructions described at
+	// https://blogs.msdn.microsoft.com/oldnewthing/20100212-00/?p=14963/
+	// and in particular bits about using both FileAttributes and
+	// Reserved0 fields).
+	var fd syscall.Win32finddata
+	sh, err := syscall.FindFirstFile(pathp, &fd)
+	if err != nil {
+		return nil, &PathError{"FindFirstFile", path, err}
+	}
+	syscall.FindClose(sh)
+
+	return newFileStatFromWin32finddata(&fd), nil
+}
+
+func (fs *fileStat) updatePathAndName(name string) error {
+	fs.path = name
+	if !isAbs(fs.path) {
+		var err error
+		fs.path, err = syscall.FullPath(fs.path)
+		if err != nil {
+			return &PathError{"FullPath", name, err}
+		}
+	}
+	fs.name = basename(name)
+	return nil
+}
+
+func (fs *fileStat) isSymlink() bool {
+	// Use instructions described at
+	// https://blogs.msdn.microsoft.com/oldnewthing/20100212-00/?p=14963/
+	// to recognize whether it's a symlink.
+	if fs.FileAttributes&syscall.FILE_ATTRIBUTE_REPARSE_POINT == 0 {
+		return false
+	}
+	return fs.Reserved0 == syscall.IO_REPARSE_TAG_SYMLINK ||
+		fs.Reserved0 == windows.IO_REPARSE_TAG_MOUNT_POINT
+}
+
 func (fs *fileStat) Size() int64 {
-	return int64(fs.sys.FileSizeHigh)<<32 + int64(fs.sys.FileSizeLow)
+	return int64(fs.FileSizeHigh)<<32 + int64(fs.FileSizeLow)
 }
 
 func (fs *fileStat) Mode() (m FileMode) {
 	if fs == &devNullStat {
 		return ModeDevice | ModeCharDevice | 0666
 	}
-	if fs.sys.FileAttributes&syscall.FILE_ATTRIBUTE_READONLY != 0 {
+	if fs.FileAttributes&syscall.FILE_ATTRIBUTE_READONLY != 0 {
 		m |= 0444
 	} else {
 		m |= 0666
 	}
-	if fs.sys.FileAttributes&syscall.FILE_ATTRIBUTE_REPARSE_POINT != 0 {
+	if fs.isSymlink() {
 		return m | ModeSymlink
 	}
-	if fs.sys.FileAttributes&syscall.FILE_ATTRIBUTE_DIRECTORY != 0 {
+	if fs.FileAttributes&syscall.FILE_ATTRIBUTE_DIRECTORY != 0 {
 		m |= ModeDir | 0111
 	}
 	switch fs.filetype {
@@ -54,11 +171,20 @@ func (fs *fileStat) Mode() (m FileMode) {
 }
 
 func (fs *fileStat) ModTime() time.Time {
-	return time.Unix(0, fs.sys.LastWriteTime.Nanoseconds())
+	return time.Unix(0, fs.LastWriteTime.Nanoseconds())
 }
 
 // Sys returns syscall.Win32FileAttributeData for file fs.
-func (fs *fileStat) Sys() interface{} { return &fs.sys }
+func (fs *fileStat) Sys() interface{} {
+	return &syscall.Win32FileAttributeData{
+		FileAttributes: fs.FileAttributes,
+		CreationTime:   fs.CreationTime,
+		LastAccessTime: fs.LastAccessTime,
+		LastWriteTime:  fs.LastWriteTime,
+		FileSizeHigh:   fs.FileSizeHigh,
+		FileSizeLow:    fs.FileSizeLow,
+	}
+}
 
 func (fs *fileStat) loadFileId() error {
 	fs.Lock()