From 1d3088effdcb03612dd03eb24feee4aa92070a63 Mon Sep 17 00:00:00 2001 From: "Bryan C. Mills" Date: Tue, 24 Jan 2023 09:05:36 -0500 Subject: [PATCH] os: make Lstat for symlinks on Windows consistent with POSIX This also makes path/filepath.Walk more consistent between Windows and POSIX platforms. According to https://pubs.opengroup.org/onlinepubs/9699919799.2013edition/basedefs/V1_chap04.html#tag_04_12 symlinks in a path that includes a trailing slash must be resolved before a function acts on that path. POSIX defines an lstat function, whereas the Win32 API does not, so Go's os.Lstat should follow the (defined) POSIX semantics instead of doing something arbitrarily different. CL 134195 added a test for the correct POSIX behavior when os.Lstat is called on a symlink. However, the test turned out to be broken on Windows, and when it was fixed (in CL 143578) it was fixed with different Lstat behavior on Windows than on all other platforms that support symlinks. In #50807 we are attempting to provide consistent symlink behavior for cmd/go. This unnecessary platform difference, if left uncorrected, will make that fix much more difficult. CL 460595 reworked the implementation of Stat and Lstat on Windows, and with the new implementation this fix is straightforward. For #50807. Updates #27225. Change-Id: Ia28821aa4aab6cefa021da2d9b803506cdb2621b Reviewed-on: https://go-review.googlesource.com/c/go/+/463177 Reviewed-by: Quim Muntal Auto-Submit: Bryan Mills TryBot-Result: Gopher Robot Reviewed-by: Alex Brainman Reviewed-by: Russ Cox Run-TryBot: Bryan Mills --- .../testdata/script/list_goroot_symlink.txt | 6 +- src/os/stat_test.go | 7 +- src/os/stat_windows.go | 11 +++- src/path/filepath/path_test.go | 64 +++++++++++++++++++ 4 files changed, 77 insertions(+), 11 deletions(-) diff --git a/src/cmd/go/testdata/script/list_goroot_symlink.txt b/src/cmd/go/testdata/script/list_goroot_symlink.txt index 8e50e4beab..989a8c2dd5 100644 --- a/src/cmd/go/testdata/script/list_goroot_symlink.txt +++ b/src/cmd/go/testdata/script/list_goroot_symlink.txt @@ -45,10 +45,8 @@ stdout $WORK${/}lib${/}goroot exec $WORK/lib/goroot/bin/go list -f '{{.ImportPath}}: {{.Dir}}' encoding/binary stdout '^encoding/binary: '$WORK${/}lib${/}goroot${/}src${/}encoding${/}binary'$' - # BUG(#50807): This doesn't work on Windows for some reason — perhaps - # a bug in the Windows Lstat implementation with trailing separators? -[!GOOS:windows] exec $WORK/lib/goroot/bin/go list -f '{{.ImportPath}}: {{.Dir}}' std -[!GOOS:windows] stdout '^encoding/binary: '$WORK${/}lib${/}goroot${/}src${/}encoding${/}binary'$' +exec $WORK/lib/goroot/bin/go list -f '{{.ImportPath}}: {{.Dir}}' std +stdout '^encoding/binary: '$WORK${/}lib${/}goroot${/}src${/}encoding${/}binary'$' # Most path lookups in GOROOT are not sensitive to symlinks. However, patterns # involving '...' wildcards must use Walk to check the GOROOT tree, which makes diff --git a/src/os/stat_test.go b/src/os/stat_test.go index 72621f257b..96019699aa 100644 --- a/src/os/stat_test.go +++ b/src/os/stat_test.go @@ -9,7 +9,6 @@ import ( "io/fs" "os" "path/filepath" - "runtime" "testing" ) @@ -279,11 +278,7 @@ func TestSymlinkWithTrailingSlash(t *testing.T) { } dirlinkWithSlash := dirlink + string(os.PathSeparator) - if runtime.GOOS == "windows" { - testSymlinkStats(t, dirlinkWithSlash, true) - } else { - testDirStats(t, dirlinkWithSlash) - } + testDirStats(t, dirlinkWithSlash) fi1, err := os.Stat(dir) if err != nil { diff --git a/src/os/stat_windows.go b/src/os/stat_windows.go index 7ac9f7b860..033c3b9353 100644 --- a/src/os/stat_windows.go +++ b/src/os/stat_windows.go @@ -123,5 +123,14 @@ func statNolog(name string) (FileInfo, error) { // lstatNolog implements Lstat for Windows. func lstatNolog(name string) (FileInfo, error) { - return stat("Lstat", name, false) + followSymlinks := false + if name != "" && IsPathSeparator(name[len(name)-1]) { + // We try to implement POSIX semantics for Lstat path resolution + // (per https://pubs.opengroup.org/onlinepubs/9699919799.2013edition/basedefs/V1_chap04.html#tag_04_12): + // symlinks before the last separator in the path must be resolved. Since + // the last separator in this case follows the last path element, we should + // follow symlinks in the last path element. + followSymlinks = true + } + return stat("Lstat", name, followSymlinks) } diff --git a/src/path/filepath/path_test.go b/src/path/filepath/path_test.go index e6a9270909..672d7e6261 100644 --- a/src/path/filepath/path_test.go +++ b/src/path/filepath/path_test.go @@ -818,6 +818,70 @@ func TestWalkFileError(t *testing.T) { } } +func TestWalkSymlinkRoot(t *testing.T) { + testenv.MustHaveSymlink(t) + + td := t.TempDir() + dir := filepath.Join(td, "dir") + if err := os.MkdirAll(filepath.Join(td, "dir"), 0755); err != nil { + t.Fatal(err) + } + touch(t, filepath.Join(dir, "foo")) + + link := filepath.Join(td, "link") + if err := os.Symlink("dir", link); err != nil { + t.Fatal(err) + } + + // Per https://pubs.opengroup.org/onlinepubs/9699919799.2013edition/basedefs/V1_chap04.html#tag_04_12: + // “A pathname that contains at least one non- character and that ends + // with one or more trailing characters shall not be resolved + // successfully unless the last pathname component before the trailing + // characters names an existing directory [...].” + // + // Since Walk does not traverse symlinks itself, its behavior should depend on + // whether the path passed to Walk ends in a slash: if it does not end in a slash, + // Walk should report the symlink itself (since it is the last pathname component); + // but if it does end in a slash, Walk should walk the directory to which the symlink + // refers (since it must be fully resolved before walking). + for _, tt := range []struct { + desc string + root string + want []string + }{ + { + desc: "no slash", + root: link, + want: []string{link}, + }, + { + desc: "slash", + root: link + string(filepath.Separator), + want: []string{link, filepath.Join(link, "foo")}, + }, + } { + tt := tt + t.Run(tt.desc, func(t *testing.T) { + var walked []string + err := filepath.Walk(tt.root, func(path string, info fs.FileInfo, err error) error { + if err != nil { + return err + } + t.Logf("%#q: %v", path, info.Mode()) + walked = append(walked, filepath.Clean(path)) + return nil + }) + if err != nil { + t.Fatal(err) + } + + if !reflect.DeepEqual(walked, tt.want) { + t.Errorf("Walk(%#q) visited %#q; want %#q", tt.root, walked, tt.want) + } + }) + } +} + var basetests = []PathTest{ {"", "."}, {".", "."}, -- 2.48.1