From b09ac10badb169828240a3b0bfaa4a428eaca969 Mon Sep 17 00:00:00 2001 From: qmuntal Date: Wed, 28 Feb 2024 16:07:27 +0100 Subject: [PATCH] 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 Reviewed-by: Bryan Mills LUCI-TryBot-Result: Go LUCI --- doc/godebug.md | 6 + doc/next/6-stdlib/99-minor/os/63703.md | 11 + .../6-stdlib/99-minor/path/filepath/63703.md | 6 + src/internal/godebugs/table.go | 1 + src/os/file_windows.go | 10 +- src/os/os_windows_test.go | 210 ++++++++++-------- src/path/filepath/path_windows_test.go | 14 +- src/runtime/metrics/doc.go | 4 + 8 files changed, 160 insertions(+), 102 deletions(-) create mode 100644 doc/next/6-stdlib/99-minor/os/63703.md diff --git a/doc/godebug.md b/doc/godebug.md index 83b4bda89af1c2..2b8852a7ec8422 100644 --- 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 new file mode 100644 index 00000000000000..581ea142ab576b --- /dev/null +++ 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`. \ No newline at end of file diff --git a/doc/next/6-stdlib/99-minor/path/filepath/63703.md b/doc/next/6-stdlib/99-minor/path/filepath/63703.md index f5dc76c46afd9b..0aa0ba6fe39a99 100644 --- 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`. \ No newline at end of file diff --git a/src/internal/godebugs/table.go b/src/internal/godebugs/table.go index a944db39aa1fb9..d5ac707a18ed5d 100644 --- 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 index 22fd9e5d403eb0..49fdd8d44d62dd 100644 --- 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,6 +350,8 @@ 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. @@ -356,7 +359,7 @@ func openSymlink(path string) (syscall.Handle, error) { // // \??\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 index 09ccbaff3be3e7..7e8b8bbf1f04b2 100644 --- 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 index 524b0d0f9252a6..2862f390d0531f 100644 --- 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 index 2a1a3055fa7f6e..e63599e0d905b8 100644 --- 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.