os: don't normalize volumes to drive letters in os.Readlink

This CL updates os.Readlink so it no longer tries to normalize volumes to drive letters, which was not always even possible. This behavior is controlled by the `winreadlinkvolume` setting. For Go 1.23, it defaults to `winreadlinkvolume=1`. Previous versions default to `winreadlinkvolume=0`. Fixes #63703. Cq-Include-Trybots: luci.golang.try:gotip-windows-amd64-longtest,gotip-windows-arm64 Change-Id: Icd6fabbc8f0b78e23a82eef8db89940e89e9222d Reviewed-on: https://go-review.googlesource.com/c/go/+/567735 Reviewed-by: Michael Knyszek <[email protected]> Reviewed-by: Bryan Mills <[email protected]> LUCI-TryBot-Result: Go LUCI <[email protected]>
golang · Mar 4, 2024 · b09ac10 · b09ac10
1 parent 2b22fc1
commit b09ac10
Show file tree

Hide file tree

Showing 8 changed files with 160 additions and 102 deletions.
diff --git a/doc/godebug.md b/doc/godebug.md
@@ -139,6 +139,12 @@ At previous versions (`winsymlink=0`), mount points are treated as symlinks,
 and other reparse points with non-default [`os.ModeType`](/pkg/os#ModeType) bits
 (such as [`os.ModeDir`](/pkg/os#ModeDir)) do not have the `ModeIrregular` bit set.
 
+Go 1.23 changed [`os.Readlink`](/pkg/os#Readlink) and [`filepath.EvalSymlinks`](/pkg/path/filepath#EvalSymlinks)
+to avoid trying to normalize volumes to drive letters, which was not always even possible.
+This behavior is controlled by the `winreadlinkvolume` setting.
+For Go 1.23, it defaults to `winreadlinkvolume=1`.
+Previous versions default to `winreadlinkvolume=0`.
+
 ### Go 1.22
 
 Go 1.22 adds a configurable limit to control the maximum acceptable RSA key size

diff --git a/doc/next/6-stdlib/99-minor/os/63703.md b/doc/next/6-stdlib/99-minor/os/63703.md
@@ -0,0 +1,11 @@
+On Windows, the [`os.Readlink`](/os#Readlink) function no longer tries
+to resolve mount points to a canonical path.
+This behavior is controlled by the `winsymlink` setting.
+For Go 1.23, it defaults to `winsymlink=1`.
+Previous versions default to `winsymlink=0`.
+
+On Windows, [`os.Readlink`](/pkg/path/filepath#EvalSymlinks) no longer tries
+to normalize volumes to drive letters, which was not always even possible.
+This behavior is controlled by the `winreadlinkvolume` setting.
+For Go 1.23, it defaults to `winreadlinkvolume=1`.
+Previous versions default to `winreadlinkvolume=0`.
diff --git a/doc/next/6-stdlib/99-minor/path/filepath/63703.md b/doc/next/6-stdlib/99-minor/path/filepath/63703.md
@@ -3,3 +3,9 @@ mount points, which was a source of many inconsistencies and bugs.
 This behavior is controlled by the `winsymlink` setting.
 For Go 1.23, it defaults to `winsymlink=1`.
 Previous versions default to `winsymlink=0`.
+
+On Windows, [`filepath.EvalSymlinks`](/pkg/path/filepath#EvalSymlinks) no longer tries
+to normalize volumes to drive letters, which was not always even possible.
+This behavior is controlled by the `winreadlinkvolume` setting.
+For Go 1.23, it defaults to `winreadlinkvolume=1`.
+Previous versions default to `winreadlinkvolume=0`.
diff --git a/src/internal/godebugs/table.go b/src/internal/godebugs/table.go
@@ -49,6 +49,7 @@ var All = []Info{
 	{Name: "tlsmaxrsasize", Package: "crypto/tls"},
 	{Name: "tlsrsakex", Package: "crypto/tls", Changed: 22, Old: "1"},
 	{Name: "tlsunsafeekm", Package: "crypto/tls", Changed: 22, Old: "1"},
+	{Name: "winreadlinkvolume", Package: "os", Changed: 22, Old: "0"},
 	{Name: "winsymlink", Package: "os", Changed: 22, Old: "0"},
 	{Name: "x509sha1", Package: "crypto/x509"},
 	{Name: "x509usefallbackroots", Package: "crypto/x509"},

diff --git a/src/os/file_windows.go b/src/os/file_windows.go
@@ -6,6 +6,7 @@ package os
 
 import (
 	"errors"
+	"internal/godebug"
 	"internal/poll"
 	"internal/syscall/windows"
 	"runtime"
@@ -349,14 +350,16 @@ func openSymlink(path string) (syscall.Handle, error) {
 	return h, nil
 }
 
+var winreadlinkvolume = godebug.New("winreadlinkvolume")
+
 // normaliseLinkPath converts absolute paths returned by
 // DeviceIoControl(h, FSCTL_GET_REPARSE_POINT, ...)
 // into paths acceptable by all Windows APIs.
 // For example, it converts
 //
 //	\??\C:\foo\bar into C:\foo\bar
 //	\??\UNC\foo\bar into \\foo\bar
-//	\??\Volume{abc}\ into C:\
+//	\??\Volume{abc}\ into \\?\Volume{abc}\
 func normaliseLinkPath(path string) (string, error) {
 	if len(path) < 4 || path[:4] != `\??\` {
 		// unexpected path, return it as is
@@ -371,7 +374,10 @@ func normaliseLinkPath(path string) (string, error) {
 		return `\\` + s[4:], nil
 	}
 
-	// handle paths, like \??\Volume{abc}\...
+	// \??\Volume{abc}\
+	if winreadlinkvolume.Value() != "0" {
+		return `\\?\` + path[4:], nil
+	}
 
 	h, err := openSymlink(path)
 	if err != nil {

diff --git a/src/os/os_windows_test.go b/src/os/os_windows_test.go
@@ -29,6 +29,7 @@ import (
 )
 
 var winsymlink = godebug.New("winsymlink")
+var winreadlinkvolume = godebug.New("winreadlinkvolume")
 
 // For TestRawConnReadWrite.
 type syscallDescriptor = syscall.Handle
@@ -1252,110 +1253,123 @@ func TestRootDirAsTemp(t *testing.T) {
 	}
 }
 
-func testReadlink(t *testing.T, path, want string) {
-	got, err := os.Readlink(path)
-	if err != nil {
-		t.Error(err)
-		return
-	}
-	if got != want {
-		t.Errorf(`Readlink(%q): got %q, want %q`, path, got, want)
-	}
-}
-
-func mklink(t *testing.T, link, target string) {
-	output, err := testenv.Command(t, "cmd", "/c", "mklink", link, target).CombinedOutput()
-	if err != nil {
-		t.Fatalf("failed to run mklink %v %v: %v %q", link, target, err, output)
-	}
-}
-
-func mklinkj(t *testing.T, link, target string) {
-	output, err := testenv.Command(t, "cmd", "/c", "mklink", "/J", link, target).CombinedOutput()
-	if err != nil {
-		t.Fatalf("failed to run mklink %v %v: %v %q", link, target, err, output)
-	}
-}
-
-func mklinkd(t *testing.T, link, target string) {
-	output, err := testenv.Command(t, "cmd", "/c", "mklink", "/D", link, target).CombinedOutput()
+// replaceDriveWithVolumeID returns path with its volume name replaced with
+// the mounted volume ID. E.g. C:\foo -> \\?\Volume{GUID}\foo.
+func replaceDriveWithVolumeID(t *testing.T, path string) string {
+	t.Helper()
+	cmd := testenv.Command(t, "cmd", "/c", "mountvol", filepath.VolumeName(path), "/L")
+	out, err := cmd.CombinedOutput()
 	if err != nil {
-		t.Fatalf("failed to run mklink %v %v: %v %q", link, target, err, output)
+		t.Fatalf("%v: %v\n%s", cmd, err, out)
 	}
+	vol := strings.Trim(string(out), " \n\r")
+	return filepath.Join(vol, path[len(filepath.VolumeName(path)):])
 }
 
-func TestWindowsReadlink(t *testing.T) {
-	tmpdir, err := os.MkdirTemp("", "TestWindowsReadlink")
-	if err != nil {
-		t.Fatal(err)
-	}
-	defer os.RemoveAll(tmpdir)
-
-	// Make sure tmpdir is not a symlink, otherwise tests will fail.
-	tmpdir, err = filepath.EvalSymlinks(tmpdir)
-	if err != nil {
-		t.Fatal(err)
-	}
-	chdir(t, tmpdir)
-
-	vol := filepath.VolumeName(tmpdir)
-	output, err := testenv.Command(t, "cmd", "/c", "mountvol", vol, "/L").CombinedOutput()
-	if err != nil {
-		t.Fatalf("failed to run mountvol %v /L: %v %q", vol, err, output)
-	}
-	ntvol := strings.Trim(string(output), " \n\r")
-
-	dir := filepath.Join(tmpdir, "dir")
-	err = os.MkdirAll(dir, 0777)
-	if err != nil {
-		t.Fatal(err)
-	}
-
-	absdirjlink := filepath.Join(tmpdir, "absdirjlink")
-	mklinkj(t, absdirjlink, dir)
-	testReadlink(t, absdirjlink, dir)
-
-	ntdirjlink := filepath.Join(tmpdir, "ntdirjlink")
-	mklinkj(t, ntdirjlink, ntvol+absdirjlink[len(filepath.VolumeName(absdirjlink)):])
-	testReadlink(t, ntdirjlink, absdirjlink)
-
-	ntdirjlinktolink := filepath.Join(tmpdir, "ntdirjlinktolink")
-	mklinkj(t, ntdirjlinktolink, ntvol+absdirjlink[len(filepath.VolumeName(absdirjlink)):])
-	testReadlink(t, ntdirjlinktolink, absdirjlink)
-
-	mklinkj(t, "reldirjlink", "dir")
-	testReadlink(t, "reldirjlink", dir) // relative directory junction resolves to absolute path
-
-	// Make sure we have sufficient privilege to run mklink command.
-	testenv.MustHaveSymlink(t)
-
-	absdirlink := filepath.Join(tmpdir, "absdirlink")
-	mklinkd(t, absdirlink, dir)
-	testReadlink(t, absdirlink, dir)
-
-	ntdirlink := filepath.Join(tmpdir, "ntdirlink")
-	mklinkd(t, ntdirlink, ntvol+absdirlink[len(filepath.VolumeName(absdirlink)):])
-	testReadlink(t, ntdirlink, absdirlink)
-
-	mklinkd(t, "reldirlink", "dir")
-	testReadlink(t, "reldirlink", "dir")
+func TestReadlink(t *testing.T) {
+	tests := []struct {
+		junction bool
+		dir      bool
+		drive    bool
+		relative bool
+	}{
+		{junction: true, dir: true, drive: true, relative: false},
+		{junction: true, dir: true, drive: false, relative: false},
+		{junction: true, dir: true, drive: false, relative: true},
+		{junction: false, dir: true, drive: true, relative: false},
+		{junction: false, dir: true, drive: false, relative: false},
+		{junction: false, dir: true, drive: false, relative: true},
+		{junction: false, dir: false, drive: true, relative: false},
+		{junction: false, dir: false, drive: false, relative: false},
+		{junction: false, dir: false, drive: false, relative: true},
+	}
+	for _, tt := range tests {
+		tt := tt
+		var name string
+		if tt.junction {
+			name = "junction"
+		} else {
+			name = "symlink"
+		}
+		if tt.dir {
+			name += "_dir"
+		} else {
+			name += "_file"
+		}
+		if tt.drive {
+			name += "_drive"
+		} else {
+			name += "_volume"
+		}
+		if tt.relative {
+			name += "_relative"
+		} else {
+			name += "_absolute"
+		}
 
-	file := filepath.Join(tmpdir, "file")
-	err = os.WriteFile(file, []byte(""), 0666)
-	if err != nil {
-		t.Fatal(err)
+		t.Run(name, func(t *testing.T) {
+			if !tt.relative {
+				t.Parallel()
+			}
+			// Make sure tmpdir is not a symlink, otherwise tests will fail.
+			tmpdir, err := filepath.EvalSymlinks(t.TempDir())
+			if err != nil {
+				t.Fatal(err)
+			}
+			link := filepath.Join(tmpdir, "link")
+			target := filepath.Join(tmpdir, "target")
+			if tt.dir {
+				if err := os.MkdirAll(target, 0777); err != nil {
+					t.Fatal(err)
+				}
+			} else {
+				if err := os.WriteFile(target, nil, 0666); err != nil {
+					t.Fatal(err)
+				}
+			}
+			var want string
+			if tt.relative {
+				relTarget := filepath.Base(target)
+				if tt.junction {
+					want = target // relative directory junction resolves to absolute path
+				} else {
+					want = relTarget
+				}
+				chdir(t, tmpdir)
+				link = filepath.Base(link)
+				target = relTarget
+			} else {
+				if tt.drive {
+					want = target
+				} else {
+					volTarget := replaceDriveWithVolumeID(t, target)
+					if winreadlinkvolume.Value() == "0" {
+						want = target
+					} else {
+						want = volTarget
+					}
+					target = volTarget
+				}
+			}
+			if tt.junction {
+				cmd := testenv.Command(t, "cmd", "/c", "mklink", "/J", link, target)
+				if out, err := cmd.CombinedOutput(); err != nil {
+					t.Fatalf("%v: %v\n%s", cmd, err, out)
+				}
+			} else {
+				if err := os.Symlink(target, link); err != nil {
+					t.Fatalf("Symlink(%#q, %#q): %v", target, link, err)
+				}
+			}
+			got, err := os.Readlink(link)
+			if err != nil {
+				t.Fatal(err)
+			}
+			if got != want {
+				t.Fatalf("Readlink(%#q) = %#q; want %#q", target, got, want)
+			}
+		})
 	}
-
-	filelink := filepath.Join(tmpdir, "filelink")
-	mklink(t, filelink, file)
-	testReadlink(t, filelink, file)
-
-	linktofilelink := filepath.Join(tmpdir, "linktofilelink")
-	mklink(t, linktofilelink, ntvol+filelink[len(filepath.VolumeName(filelink)):])
-	testReadlink(t, linktofilelink, filelink)
-
-	mklink(t, "relfilelink", "file")
-	testReadlink(t, "relfilelink", "file")
 }
 
 func TestOpenDirTOCTOU(t *testing.T) {

diff --git a/src/path/filepath/path_windows_test.go b/src/path/filepath/path_windows_test.go
@@ -530,6 +530,7 @@ func createMountPartition(t *testing.T, vhd string, args string) []byte {
 }
 
 var winsymlink = godebug.New("winsymlink")
+var winreadlinkvolume = godebug.New("winreadlinkvolume")
 
 func TestEvalSymlinksJunctionToVolumeID(t *testing.T) {
 	// Test that EvalSymlinks resolves a directory junction which
@@ -617,7 +618,11 @@ func TestNTNamespaceSymlink(t *testing.T) {
 	}
 	var want string
 	if winsymlink.Value() == "0" {
-		want = vol + `\`
+		if winreadlinkvolume.Value() == "0" {
+			want = vol + `\`
+		} else {
+			want = target
+		}
 	} else {
 		want = dirlink
 	}
@@ -646,7 +651,12 @@ func TestNTNamespaceSymlink(t *testing.T) {
 	if err != nil {
 		t.Fatal(err)
 	}
-	want = file
+
+	if winreadlinkvolume.Value() == "0" {
+		want = file
+	} else {
+		want = target
+	}
 	if got != want {
 		t.Errorf(`EvalSymlinks(%q): got %q, want %q`, filelink, got, want)
 	}

diff --git a/src/runtime/metrics/doc.go b/src/runtime/metrics/doc.go
@@ -319,6 +319,10 @@ Below is the full list of supported metrics, ordered lexicographically.
 		The number of non-default behaviors executed by the crypto/tls
 		package due to a non-default GODEBUG=tlsunsafeekm=... setting.
 
+	/godebug/non-default-behavior/winreadlinkvolume:events
+		The number of non-default behaviors executed by the os package
+		due to a non-default GODEBUG=winreadlinkvolume=... setting.
+
 	/godebug/non-default-behavior/winsymlink:events
 		The number of non-default behaviors executed by the os package
 		due to a non-default GODEBUG=winsymlink=... setting.