]> Cypherpunks repositories - gostls13.git/commitdiff
os: don't normalize volumes to drive letters in os.Readlink
authorqmuntal <quimmuntal@gmail.com>
Wed, 28 Feb 2024 15:07:27 +0000 (16:07 +0100)
committerQuim Muntal <quimmuntal@gmail.com>
Mon, 4 Mar 2024 20:38:54 +0000 (20:38 +0000)
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 <mknyszek@google.com>
Reviewed-by: Bryan Mills <bcmills@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>

doc/godebug.md
doc/next/6-stdlib/99-minor/os/63703.md [new file with mode: 0644]
doc/next/6-stdlib/99-minor/path/filepath/63703.md
src/internal/godebugs/table.go
src/os/file_windows.go
src/os/os_windows_test.go
src/path/filepath/path_windows_test.go
src/runtime/metrics/doc.go

index 83b4bda89af1c2b807fb638f19e3c0822eb4b117..2b8852a7ec84224e7c52320a0f9414bb34bd1968 100644 (file)
@@ -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 (file)
index 0000000..581ea14
--- /dev/null
@@ -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
index f5dc76c46afd9b76c311c468e821a2d2c6b9e222..0aa0ba6fe39a99dbb30dca003797e0728434b691 100644 (file)
@@ -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
index a944db39aa1fb993f26cb139cf3e39611c8b7a0a..d5ac707a18ed5dc9c4da49fbd01b841051b8a3b3 100644 (file)
@@ -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"},
index 22fd9e5d403eb0ce63207710db97db01002cfb58..49fdd8d44d62dd26c4ece795a0b44624406b56cb 100644 (file)
@@ -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 {
index 09ccbaff3be3e76fcd6815ffd8ae8f4f26ed7978..7e8b8bbf1f04b2f7e2358b5529e5ac669d87ba6d 100644 (file)
@@ -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) {
index 524b0d0f9252a60eeb95cc321dd6507fbf7b2395..2862f390d0531fc5a9b65df0ed52debefd4f5769 100644 (file)
@@ -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)
        }
index 2a1a3055fa7f6ec21f64cbf09c2f09de3dfbcb3f..e63599e0d905b83d04c5cc8d626de9e36b47082b 100644 (file)
@@ -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.