windows: Use GetFinalPathNameByHandle for ResolveSymbolicLink

This change splits the definition of pkg/cri/os.ResolveSymbolicLink by
platform (windows/!windows), and switches to an alternate implementation
for Windows. This aims to fix the issue described in containerd/containerd#5405.

The previous implementation which just called filepath.EvalSymlinks has
historically had issues on Windows. One of these issues we were able to
fix in Go, but EvalSymlinks's behavior is not well specified on
Windows, and there could easily be more issues in the future, so it
seems prudent to move to a separate implementation for Windows.

The new implementation uses the Windows GetFinalPathNameByHandle API,
which takes a handle to an open file or directory and some flags, and
returns the "real" name for the object. See comments in the code for
details on the implementation.

I have tested this change with a variety of mounts and everything seems
to work as expected. Functions that make incorrect assumptions on what a
Windows path can look like may have some trouble with the \\?\ path
syntax. For instance EvalSymlinks fails when given a \\?\UNC\ path. For
this reason, the resolvePath implementation modifies the returned path
to translate to the more common form (\\?\UNC\server\share ->
\\server\share).

Signed-off-by: Kevin Parsons <kevpar@microsoft.com>
(cherry picked from commit b0d3b35b28)
Signed-off-by: Phil Estes <estesp@gmail.com>

pkg/os/os.go

@@ -20,7 +20,6 @@ import (
 	"io"
 	"io/ioutil"
 	"os"
-	"path/filepath"
 
 	"github.com/moby/sys/symlink"
 )
@@ -56,18 +55,6 @@ func (RealOS) Stat(name string) (os.FileInfo, error) {
 	return os.Stat(name)
 }
 
-// ResolveSymbolicLink will follow any symbolic links
-func (RealOS) ResolveSymbolicLink(path string) (string, error) {
-	info, err := os.Lstat(path)
-	if err != nil {
-		return "", err
-	}
-	if info.Mode()&os.ModeSymlink != os.ModeSymlink {
-		return path, nil
-	}
-	return filepath.EvalSymlinks(path)
-}
-
 // FollowSymlinkInScope will call symlink.FollowSymlinkInScope.
 func (RealOS) FollowSymlinkInScope(path, scope string) (string, error) {
 	return symlink.FollowSymlinkInScope(path, scope)

pkg/os/os_unix.go

@@ -19,6 +19,9 @@
 package os
 
 import (
+	"os"
+	"path/filepath"
+
 	"github.com/containerd/containerd/mount"
 )
 
@@ -29,3 +32,15 @@ type UNIX interface {
 	Unmount(target string) error
 	LookupMount(path string) (mount.Info, error)
 }
+
+// ResolveSymbolicLink will follow any symbolic links
+func (RealOS) ResolveSymbolicLink(path string) (string, error) {
+	info, err := os.Lstat(path)
+	if err != nil {
+		return "", err
+	}
+	if info.Mode()&os.ModeSymlink != os.ModeSymlink {
+		return path, nil
+	}
+	return filepath.EvalSymlinks(path)
+}

pkg/os/os_windows.go

@@ -0,0 +1,180 @@
+/*
+   Copyright The containerd Authors.
+
+   Licensed under the Apache License, Version 2.0 (the "License");
+   you may not use this file except in compliance with the License.
+   You may obtain a copy of the License at
+
+       http://www.apache.org/licenses/LICENSE-2.0
+
+   Unless required by applicable law or agreed to in writing, software
+   distributed under the License is distributed on an "AS IS" BASIS,
+   WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+   See the License for the specific language governing permissions and
+   limitations under the License.
+*/
+
+package os
+
+import (
+	"os"
+	"strings"
+	"sync"
+	"unicode/utf16"
+
+	"golang.org/x/sys/windows"
+)
+
+// openPath takes a path, opens it, and returns the resulting handle.
+// It works for both file and directory paths.
+//
+// We are not able to use builtin Go functionality for opening a directory path:
+// - os.Open on a directory returns a os.File where Fd() is a search handle from FindFirstFile.
+// - syscall.Open does not provide a way to specify FILE_FLAG_BACKUP_SEMANTICS, which is needed to
+//   open a directory.
+// We could use os.Open if the path is a file, but it's easier to just use the same code for both.
+// Therefore, we call windows.CreateFile directly.
+func openPath(path string) (windows.Handle, error) {
+	u16, err := windows.UTF16PtrFromString(path)
+	if err != nil {
+		return 0, err
+	}
+	h, err := windows.CreateFile(
+		u16,
+		0,
+		windows.FILE_SHARE_READ|windows.FILE_SHARE_WRITE|windows.FILE_SHARE_DELETE,
+		nil,
+		windows.OPEN_EXISTING,
+		windows.FILE_FLAG_BACKUP_SEMANTICS, // Needed to open a directory handle.
+		0)
+	if err != nil {
+		return 0, &os.PathError{
+			Op:   "CreateFile",
+			Path: path,
+			Err:  err,
+		}
+	}
+	return h, nil
+}
+
+// GetFinalPathNameByHandle flags.
+//nolint:golint
+const (
+	cFILE_NAME_OPENED = 0x8
+
+	cVOLUME_NAME_DOS  = 0x0
+	cVOLUME_NAME_GUID = 0x1
+)
+
+var pool = sync.Pool{
+	New: func() interface{} {
+		// Size of buffer chosen somewhat arbitrarily to accommodate a large number of path strings.
+		// MAX_PATH (260) + size of volume GUID prefix (49) + null terminator = 310.
+		b := make([]uint16, 310)
+		return &b
+	},
+}
+
+// getFinalPathNameByHandle facilitates calling the Windows API GetFinalPathNameByHandle
+// with the given handle and flags. It transparently takes care of creating a buffer of the
+// correct size for the call.
+func getFinalPathNameByHandle(h windows.Handle, flags uint32) (string, error) {
+	b := *(pool.Get().(*[]uint16))
+	defer func() { pool.Put(&b) }()
+	for {
+		n, err := windows.GetFinalPathNameByHandle(h, &b[0], uint32(len(b)), flags)
+		if err != nil {
+			return "", err
+		}
+		// If the buffer wasn't large enough, n will be the total size needed (including null terminator).
+		// Resize and try again.
+		if n > uint32(len(b)) {
+			b = make([]uint16, n)
+			continue
+		}
+		// If the buffer is large enough, n will be the size not including the null terminator.
+		// Convert to a Go string and return.
+		return string(utf16.Decode(b[:n])), nil
+	}
+}
+
+// resolvePath implements path resolution for Windows. It attempts to return the "real" path to the
+// file or directory represented by the given path.
+// The resolution works by using the Windows API GetFinalPathNameByHandle, which takes a handle and
+// returns the final path to that file.
+func resolvePath(path string) (string, error) {
+	h, err := openPath(path)
+	if err != nil {
+		return "", err
+	}
+	defer windows.CloseHandle(h)
+
+	// We use the Windows API GetFinalPathNameByHandle to handle path resolution. GetFinalPathNameByHandle
+	// returns a resolved path name for a file or directory. The returned path can be in several different
+	// formats, based on the flags passed. There are several goals behind the design here:
+	// - Do as little manual path manipulation as possible. Since Windows path formatting can be quite
+	//   complex, we try to just let the Windows APIs handle that for us.
+	// - Retain as much compatibility with existing Go path functions as we can. In particular, we try to
+	//   ensure paths returned from resolvePath can be passed to EvalSymlinks.
+	//
+	// First, we query for the VOLUME_NAME_GUID path of the file. This will return a path in the form
+	// "\\?\Volume{8a25748f-cf34-4ac6-9ee2-c89400e886db}\dir\file.txt". If the path is a UNC share
+	// (e.g. "\\server\share\dir\file.txt"), then the VOLUME_NAME_GUID query will fail with ERROR_PATH_NOT_FOUND.
+	// In this case, we will next try a VOLUME_NAME_DOS query. This query will return a path for a UNC share
+	// in the form "\\?\UNC\server\share\dir\file.txt". This path will work with most functions, but EvalSymlinks
+	// fails on it. Therefore, we rewrite the path to the form "\\server\share\dir\file.txt" before returning it.
+	// This path rewrite may not be valid in all cases (see the notes in the next paragraph), but those should
+	// be very rare edge cases, and this case wouldn't have worked with EvalSymlinks anyways.
+	//
+	// The "\\?\" prefix indicates that no path parsing or normalization should be performed by Windows.
+	// Instead the path is passed directly to the object manager. The lack of parsing means that "." and ".." are
+	// interpreted literally and "\"" must be used as a path separator. Additionally, because normalization is
+	// not done, certain paths can only be represented in this format. For instance, "\\?\C:\foo." (with a trailing .)
+	// cannot be written as "C:\foo.", because path normalization will remove the trailing ".".
+	//
+	// We use FILE_NAME_OPENED instead of FILE_NAME_NORMALIZED, as FILE_NAME_NORMALIZED can fail on some
+	// UNC paths based on access restrictions. The additional normalization done is also quite minimal in
+	// most cases.
+	//
+	// Querying for VOLUME_NAME_DOS first instead of VOLUME_NAME_GUID would yield a "nicer looking" path in some cases.
+	// For instance, it could return "\\?\C:\dir\file.txt" instead of "\\?\Volume{8a25748f-cf34-4ac6-9ee2-c89400e886db}\dir\file.txt".
+	// However, we query for VOLUME_NAME_GUID first for two reasons:
+	// - The volume GUID path is more stable. A volume's mount point can change when it is remounted, but its
+	//   volume GUID should not change.
+	// - If the volume is mounted at a non-drive letter path (e.g. mounted to "C:\mnt"), then VOLUME_NAME_DOS
+	//   will return the mount path. EvalSymlinks fails on a path like this due to a bug.
+	//
+	// References:
+	// - GetFinalPathNameByHandle: https://docs.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-getfinalpathnamebyhandlea
+	// - Naming Files, Paths, and Namespaces: https://docs.microsoft.com/en-us/windows/win32/fileio/naming-a-file
+	// - Naming a Volume: https://docs.microsoft.com/en-us/windows/win32/fileio/naming-a-volume
+
+	rPath, err := getFinalPathNameByHandle(h, cFILE_NAME_OPENED|cVOLUME_NAME_GUID)
+	if err == windows.ERROR_PATH_NOT_FOUND {
+		// ERROR_PATH_NOT_FOUND is returned from the VOLUME_NAME_GUID query if the path is a
+		// network share (UNC path). In this case, query for the DOS name instead, then translate
+		// the returned path to make it more palatable to other path functions.
+		rPath, err = getFinalPathNameByHandle(h, cFILE_NAME_OPENED|cVOLUME_NAME_DOS)
+		if err != nil {
+			return "", err
+		}
+		if strings.HasPrefix(rPath, `\\?\UNC\`) {
+			// Convert \\?\UNC\server\share -> \\server\share. The \\?\UNC syntax does not work with
+			// some Go filepath functions such as EvalSymlinks. In the future if other components
+			// move away from EvalSymlinks and use GetFinalPathNameByHandle instead, we could remove
+			// this path munging.
+			rPath = `\\` + rPath[len(`\\?\UNC\`):]
+		}
+	} else if err != nil {
+		return "", err
+	}
+	return rPath, nil
+}
+
+// ResolveSymbolicLink will follow any symbolic links
+func (RealOS) ResolveSymbolicLink(path string) (string, error) {
+	// filepath.EvalSymlinks does not work very well on Windows, so instead we resolve the path
+	// via resolvePath which uses GetFinalPathNameByHandle. This returns either a path prefixed with `\\?\`,
+	// or a remote share path in the form \\server\share. These should work with most Go and Windows APIs.
+	return resolvePath(path)
+}

pkg/os/os_windows_test.go

@@ -0,0 +1,258 @@
+/*
+   Copyright The containerd Authors.
+
+   Licensed under the Apache License, Version 2.0 (the "License");
+   you may not use this file except in compliance with the License.
+   You may obtain a copy of the License at
+
+       http://www.apache.org/licenses/LICENSE-2.0
+
+   Unless required by applicable law or agreed to in writing, software
+   distributed under the License is distributed on an "AS IS" BASIS,
+   WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+   See the License for the specific language governing permissions and
+   limitations under the License.
+*/
+
+package os
+
+import (
+	"context"
+	"fmt"
+	"io/ioutil"
+	"os"
+	"path/filepath"
+	"strconv"
+	"strings"
+	"syscall"
+	"testing"
+
+	"github.com/Microsoft/go-winio/vhd"
+	"github.com/Microsoft/hcsshim/computestorage"
+	"github.com/Microsoft/hcsshim/osversion"
+	"golang.org/x/sys/windows"
+	"golang.org/x/sys/windows/registry"
+)
+
+// Getting build number via osversion.Build() requires the program to be properly manifested, which
+// requires importing `github.com/Microsoft/hcsshim/test/functional/manifest`, which is a dependency
+// we want to avoid here. Instead, since this is just test code, simply parse the build number from
+// the registry.
+func getWindowsBuildNumber() (int, error) {
+	k, err := registry.OpenKey(registry.LOCAL_MACHINE, `SOFTWARE\Microsoft\Windows NT\CurrentVersion`, registry.QUERY_VALUE)
+	if err != nil {
+		return 0, fmt.Errorf("read CurrentVersion reg key: %s", err)
+	}
+	defer k.Close()
+	buildNumStr, _, err := k.GetStringValue("CurrentBuild")
+	if err != nil {
+		return 0, fmt.Errorf("read CurrentBuild reg value: %s", err)
+	}
+	buildNum, err := strconv.Atoi(buildNumStr)
+	if err != nil {
+		return 0, err
+	}
+	return buildNum, nil
+}
+
+func makeSymlink(t *testing.T, oldName string, newName string) {
+	if err := os.Symlink(oldName, newName); err != nil {
+		t.Fatalf("creating symlink: %s", err)
+	}
+}
+
+func getVolumeGUIDPath(t *testing.T, path string) string {
+	h, err := openPath(path)
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer windows.CloseHandle(h)
+	final, err := getFinalPathNameByHandle(h, cFILE_NAME_OPENED|cVOLUME_NAME_GUID)
+	if err != nil {
+		t.Fatal(err)
+	}
+	return final
+}
+
+func openDisk(path string) (windows.Handle, error) {
+	u16, err := windows.UTF16PtrFromString(path)
+	if err != nil {
+		return 0, err
+	}
+	h, err := windows.CreateFile(
+		u16,
+		windows.GENERIC_READ|windows.GENERIC_WRITE,
+		windows.FILE_SHARE_READ|windows.FILE_SHARE_WRITE,
+		nil,
+		windows.OPEN_EXISTING,
+		windows.FILE_ATTRIBUTE_NORMAL|windows.FILE_FLAG_NO_BUFFERING,
+		0)
+	if err != nil {
+		return 0, &os.PathError{
+			Op:   "CreateFile",
+			Path: path,
+			Err:  err,
+		}
+	}
+	return h, nil
+}
+
+func formatVHD(vhdHandle windows.Handle) error {
+	h := vhdHandle
+	// Pre-19H1 HcsFormatWritableLayerVhd expects a disk handle.
+	// On newer builds it expects a VHD handle instead.
+	// Open a handle to the VHD's disk object if needed.
+	build, err := getWindowsBuildNumber()
+	if err != nil {
+		return err
+	}
+	if build < osversion.V19H1 {
+		diskPath, err := vhd.GetVirtualDiskPhysicalPath(syscall.Handle(h))
+		if err != nil {
+			return err
+		}
+		diskHandle, err := openDisk(diskPath)
+		if err != nil {
+			return err
+		}
+		defer windows.CloseHandle(diskHandle)
+		h = diskHandle
+	}
+	// Formatting a disk directly in Windows is a pain, so we use FormatWritableLayerVhd to do it.
+	// It has a side effect of creating a sandbox directory on the formatted volume, but it's safe
+	// to just ignore that for our purposes here.
+	if err := computestorage.FormatWritableLayerVhd(context.Background(), h); err != nil {
+		return err
+	}
+	return nil
+}
+
+// Creates a VHD with a NTFS volume. Returns the volume path.
+func setupVHDVolume(t *testing.T, vhdPath string) string {
+	vhdHandle, err := vhd.CreateVirtualDisk(vhdPath, vhd.VirtualDiskAccessNone, vhd.CreateVirtualDiskFlagNone, &vhd.CreateVirtualDiskParameters{
+		Version: 2,
+		Version2: vhd.CreateVersion2{
+			MaximumSize:      5 * 1024 * 1024 * 1024, // 5GB, thin provisioned
+			BlockSizeInBytes: 1 * 1024 * 1024,        // 1MB
+		},
+	})
+	if err != nil {
+		t.Fatal(err)
+	}
+	t.Cleanup(func() {
+		windows.CloseHandle(windows.Handle(vhdHandle))
+	})
+	if err := vhd.AttachVirtualDisk(vhdHandle, vhd.AttachVirtualDiskFlagNone, &vhd.AttachVirtualDiskParameters{Version: 1}); err != nil {
+		t.Fatal(err)
+	}
+	t.Cleanup(func() {
+		if err := vhd.DetachVirtualDisk(vhdHandle); err != nil {
+			t.Fatal(err)
+		}
+	})
+	if err := formatVHD(windows.Handle(vhdHandle)); err != nil {
+		t.Fatalf("failed to format VHD: %s", err)
+	}
+	// Get the path for the volume that was just created on the disk.
+	volumePath, err := computestorage.GetLayerVhdMountPath(context.Background(), windows.Handle(vhdHandle))
+	if err != nil {
+		t.Fatal(err)
+	}
+	return volumePath
+}
+
+func writeFile(t *testing.T, path string, content []byte) {
+	if err := ioutil.WriteFile(path, content, 0644); err != nil {
+		t.Fatal(err)
+	}
+}
+
+func mountVolume(t *testing.T, volumePath string, mountPoint string) {
+	// Create the mount point directory.
+	if err := os.Mkdir(mountPoint, 0644); err != nil {
+		t.Fatal(err)
+	}
+	t.Cleanup(func() {
+		if err := os.Remove(mountPoint); err != nil {
+			t.Fatal(err)
+		}
+	})
+	// Volume path must end in a slash.
+	if !strings.HasSuffix(volumePath, `\`) {
+		volumePath += `\`
+	}
+	volumePathU16, err := windows.UTF16PtrFromString(volumePath)
+	if err != nil {
+		t.Fatal(err)
+	}
+	// Mount point must end in a slash.
+	if !strings.HasSuffix(mountPoint, `\`) {
+		mountPoint += `\`
+	}
+	mountPointU16, err := windows.UTF16PtrFromString(mountPoint)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if err := windows.SetVolumeMountPoint(mountPointU16, volumePathU16); err != nil {
+		t.Fatalf("failed to mount %s onto %s: %s", volumePath, mountPoint, err)
+	}
+	t.Cleanup(func() {
+		if err := windows.DeleteVolumeMountPoint(mountPointU16); err != nil {
+			t.Fatalf("failed to delete mount on %s: %s", mountPoint, err)
+		}
+	})
+}
+
+func TestResolvePath(t *testing.T) {
+	// Set up some data to be used by the test cases.
+	volumePathC := getVolumeGUIDPath(t, `C:\`)
+	dir := t.TempDir()
+
+	makeSymlink(t, `C:\windows`, filepath.Join(dir, "lnk1"))
+	makeSymlink(t, `\\localhost\c$\windows`, filepath.Join(dir, "lnk2"))
+
+	volumePathVHD1 := setupVHDVolume(t, filepath.Join(dir, "foo.vhdx"))
+	writeFile(t, filepath.Join(volumePathVHD1, "data.txt"), []byte("test content 1"))
+	makeSymlink(t, filepath.Join(volumePathVHD1, "data.txt"), filepath.Join(dir, "lnk3"))
+
+	volumePathVHD2 := setupVHDVolume(t, filepath.Join(dir, "bar.vhdx"))
+	writeFile(t, filepath.Join(volumePathVHD2, "data.txt"), []byte("test content 2"))
+	makeSymlink(t, filepath.Join(volumePathVHD2, "data.txt"), filepath.Join(dir, "lnk4"))
+	mountVolume(t, volumePathVHD2, filepath.Join(dir, "mnt"))
+
+	for _, tc := range []struct {
+		input       string
+		expected    string
+		description string
+	}{
+		{`C:\windows`, volumePathC + `Windows`, "local path"},
+		{filepath.Join(dir, "lnk1"), volumePathC + `Windows`, "symlink to local path"},
+		{`\\localhost\c$\windows`, `\\localhost\c$\windows`, "UNC path"},
+		{filepath.Join(dir, "lnk2"), `\\localhost\c$\windows`, "symlink to UNC path"},
+		{filepath.Join(volumePathVHD1, "data.txt"), filepath.Join(volumePathVHD1, "data.txt"), "volume with no mount point"},
+		{filepath.Join(dir, "lnk3"), filepath.Join(volumePathVHD1, "data.txt"), "symlink to volume with no mount point"},
+		{filepath.Join(dir, "mnt", "data.txt"), filepath.Join(volumePathVHD2, "data.txt"), "volume with mount point"},
+		{filepath.Join(dir, "lnk4"), filepath.Join(volumePathVHD2, "data.txt"), "symlink to volume with mount point"},
+	} {
+		t.Run(tc.description, func(t *testing.T) {
+			actual, err := resolvePath(tc.input)
+			if err != nil {
+				t.Fatalf("resolvePath should return no error, but %v", err)
+			}
+			if actual != tc.expected {
+				t.Fatalf("expected %v but got %v", tc.expected, actual)
+			}
+			// Make sure EvalSymlinks works with the resolved path, as an extra safety measure.
+			p, err := filepath.EvalSymlinks(actual)
+			if err != nil {
+				t.Fatalf("EvalSymlinks should return no error, but %v", err)
+			}
+			// As an extra-extra safety, check that resolvePath(x) == EvalSymlinks(resolvePath(x)).
+			// EvalSymlinks normalizes UNC path casing, but resolvePath doesn't, so compare with
+			// case-insensitivity here.
+			if !strings.EqualFold(actual, p) {
+				t.Fatalf("EvalSymlinks should resolve to the same path. Expected %v but got %v", actual, p)
+			}
+		})
+	}
+}