From: George Adams Date: Tue, 19 Nov 2024 10:05:27 +0000 (+0000) Subject: Revert "os: check for valid Windows path when creating files" X-Git-Tag: go1.24rc1~269 X-Git-Url: http://www.git.cypherpunks.su/?a=commitdiff_plain;h=5deea4c2425fd8aa6dee642c63a1bc43e090d04b;p=gostls13.git Revert "os: check for valid Windows path when creating files" This reverts commit CL 618496. Reason for revert: https://github.com/golang/go/issues/54040#issuecomment-2485151973 Change-Id: I3bf27f7fdd475a005cb6aa190994153504e96fb5 Reviewed-on: https://go-review.googlesource.com/c/go/+/629595 Reviewed-by: Ian Lance Taylor Auto-Submit: Ian Lance Taylor Reviewed-by: Quim Muntal LUCI-TryBot-Result: Go LUCI Reviewed-by: Damien Neil --- diff --git a/src/os/export_windows_test.go b/src/os/export_windows_test.go index faab5470f7..aefbe4033e 100644 --- a/src/os/export_windows_test.go +++ b/src/os/export_windows_test.go @@ -7,9 +7,8 @@ package os // Export for testing. var ( - AddExtendedPrefix = addExtendedPrefix - NewConsoleFile = newConsoleFile - CommandLineToArgv = commandLineToArgv - AllowReadDirFileID = &allowReadDirFileID - ValidatePathForCreate = validatePathForCreate + AddExtendedPrefix = addExtendedPrefix + NewConsoleFile = newConsoleFile + CommandLineToArgv = commandLineToArgv + AllowReadDirFileID = &allowReadDirFileID ) diff --git a/src/os/file.go b/src/os/file.go index c75606d749..0341469e2d 100644 --- a/src/os/file.go +++ b/src/os/file.go @@ -305,18 +305,25 @@ func (f *File) WriteString(s string) (n int, err error) { // bits (before umask). // If there is an error, it will be of type *PathError. func Mkdir(name string, perm FileMode) error { - err := mkdir(name, perm) - if err != nil { - return &PathError{Op: "mkdir", Path: name, Err: err} + longName := fixLongPath(name) + e := ignoringEINTR(func() error { + return syscall.Mkdir(longName, syscallMode(perm)) + }) + + if e != nil { + return &PathError{Op: "mkdir", Path: name, Err: e} } + // mkdir(2) itself won't handle the sticky bit on *BSD and Solaris if !supportsCreateWithStickyBit && perm&ModeSticky != 0 { - err = setStickyBit(name) - if err != nil { + e = setStickyBit(name) + + if e != nil { Remove(name) - return err + return e } } + return nil } diff --git a/src/os/file_nonwindows.go b/src/os/file_nonwindows.go deleted file mode 100644 index d0e7843ee5..0000000000 --- a/src/os/file_nonwindows.go +++ /dev/null @@ -1,15 +0,0 @@ -// Copyright 2024 The Go Authors. All rights reserved. -// Use of this source code is governed by a BSD-style -// license that can be found in the LICENSE file. - -//go:build !windows - -package os - -import "syscall" - -func mkdir(name string, perm FileMode) error { - return ignoringEINTR(func() error { - return syscall.Mkdir(name, syscallMode(perm)) - }) -} diff --git a/src/os/file_windows.go b/src/os/file_windows.go index 0fd6c99f94..465cf5d186 100644 --- a/src/os/file_windows.go +++ b/src/os/file_windows.go @@ -17,8 +17,6 @@ import ( "unsafe" ) -var errInvalidPath = errors.New("invalid path: cannot end with a space or period") - // This matches the value in syscall/syscall_windows.go. const _UTIME_OMIT = -1 @@ -104,9 +102,6 @@ func openFileNolog(name string, flag int, perm FileMode) (*File, error) { if name == "" { return nil, &PathError{Op: "open", Path: name, Err: syscall.ENOENT} } - if flag&O_CREATE != 0 && !validatePathForCreate(name) { - return nil, &PathError{Op: "open", Path: name, Err: errInvalidPath} - } path := fixLongPath(name) r, err := syscall.Open(path, flag|syscall.O_CLOEXEC, syscallMode(perm)) if err != nil { @@ -119,14 +114,6 @@ func openDirNolog(name string) (*File, error) { return openFileNolog(name, O_RDONLY, 0) } -func mkdir(name string, perm FileMode) error { - if !validatePathForCreate(name) { - return errInvalidPath - } - longName := fixLongPath(name) - return syscall.Mkdir(longName, syscallMode(perm)) -} - func (file *file) close() error { if file == nil { return syscall.EINVAL @@ -217,9 +204,6 @@ func Remove(name string) error { } func rename(oldname, newname string) error { - if !validatePathForCreate(newname) { - return &LinkError{"rename", oldname, newname, errInvalidPath} - } e := windows.Rename(fixLongPath(oldname), fixLongPath(newname)) if e != nil { return &LinkError{"rename", oldname, newname, e} @@ -268,9 +252,6 @@ func tempDir() string { // Link creates newname as a hard link to the oldname file. // If there is an error, it will be of type *LinkError. func Link(oldname, newname string) error { - if !validatePathForCreate(newname) { - return &LinkError{"link", oldname, newname, errInvalidPath} - } n, err := syscall.UTF16PtrFromString(fixLongPath(newname)) if err != nil { return &LinkError{"link", oldname, newname, err} @@ -291,9 +272,6 @@ func Link(oldname, newname string) error { // if oldname is later created as a directory the symlink will not work. // If there is an error, it will be of type *LinkError. func Symlink(oldname, newname string) error { - if !validatePathForCreate(newname) { - return &LinkError{"symlink", oldname, newname, errInvalidPath} - } // '/' does not work in link's content oldname = filepathlite.FromSlash(oldname) diff --git a/src/os/os_windows_test.go b/src/os/os_windows_test.go index 9208fe3b16..31c379011c 100644 --- a/src/os/os_windows_test.go +++ b/src/os/os_windows_test.go @@ -1563,76 +1563,3 @@ func TestReadDirNoFileID(t *testing.T) { t.Errorf("SameFile(%v, %v) = false; want true", f2, f2s) } } - -func TestOpen_InvalidPath(t *testing.T) { - dir := t.TempDir() - - file, err := os.Open(dir + ".") - if err != nil { - t.Errorf("Open(%q) should have succeeded, got %v", dir+".", err) - } else { - file.Close() - } - - file, err = os.Open(dir + " ") - if err != nil { - t.Errorf("Open(%q) should have succeeded, got %v", dir+" ", err) - } else { - file.Close() - } -} - -func TestMkdirAll_InvalidPath(t *testing.T) { - // Parent folder contains traling spaces - path := `C:\temp\folder \this one fails` - err := os.MkdirAll(path, 0644) - if err == nil { - t.Errorf("MkdirAll(%q) should have failed", path) - } else if !strings.Contains(err.Error(), "invalid path: cannot end with a space or period") { - t.Errorf("expected errInvalidPath for path %q, got %v", path, err) - } -} - -func TestCreate_InvalidPath(t *testing.T) { - testInvalidPath(t, func(_, path string) error { - _, err := os.Create(path) - return err - }) -} - -func TestMkdir_InvalidPath(t *testing.T) { - testInvalidPath(t, func(_, path string) error { - return os.Mkdir(path, 0644) - }) -} - -func TestRename_InvalidPath(t *testing.T) { - testInvalidPath(t, os.Rename) -} - -func TestLink_InvalidPath(t *testing.T) { - testInvalidPath(t, os.Link) -} - -func TestSymlink_InvalidPath(t *testing.T) { - testInvalidPath(t, os.Symlink) -} - -func testInvalidPath(t *testing.T, fn func(src, dest string) error) { - dir := t.TempDir() - - // Test invalid paths (with trailing space and period) - invalidPaths := []string{ - filepath.Join(dir, "invalid_dir "), // path ending in space - filepath.Join(dir, "invalid_dir."), // path ending in period - } - - for _, path := range invalidPaths { - err := fn(dir, path) - if err == nil { - t.Errorf("(%q, %q) should have failed", dir, path) - } else if !strings.Contains(err.Error(), "invalid path: cannot end with a space or period") { - t.Errorf("expected errInvalidPath for path %q, got %v", path, err) - } - } -} diff --git a/src/os/path_windows.go b/src/os/path_windows.go index d72649be86..f585aa5ee6 100644 --- a/src/os/path_windows.go +++ b/src/os/path_windows.go @@ -6,7 +6,6 @@ package os import ( "internal/filepathlite" - "internal/stringslite" "internal/syscall/windows" "syscall" ) @@ -151,31 +150,3 @@ func addExtendedPrefix(path string) string { copy(buf, prefix) return syscall.UTF16ToString(buf) } - -// validatePathForCreate checks if a given path is valid for creation on a Windows system. -// It returns true if the path is considered valid, and false otherwise. -// The function performs the following checks: -// 1. If the path is empty, it is considered valid. -// 2. If the path starts with `\\?\` or \??\, it is considered valid without further checks. -// 3. Otherwise, a path ending with a space or . is invalid. -func validatePathForCreate(path string) bool { - // Check if the path is empty. - if len(path) == 0 { - return true - } - // Paths starting with \\?\ should be considered valid without further checks. - if stringslite.HasPrefix(path, `\\?\`) || stringslite.HasPrefix(path, `\??\`) { - return true - } - // Get the base name of the path to check only the last component. - base := filepathlite.Base(path) - // Check if the last character of the base name is a space or period, which is invalid. - switch base[len(base)-1] { - case ' ': - return false - case '.': - return base == "." || base == ".." - default: - return true - } -} diff --git a/src/os/path_windows_test.go b/src/os/path_windows_test.go index e1f0697979..3fa02e2a65 100644 --- a/src/os/path_windows_test.go +++ b/src/os/path_windows_test.go @@ -278,56 +278,3 @@ func BenchmarkAddExtendedPrefix(b *testing.B) { os.AddExtendedPrefix(veryLong) } } - -func TestValidatePathForCreate(t *testing.T) { - tests := []struct { - path string - pass bool - }{ - {`C:\foo`, true}, - {`C:\foo\ `, false}, - {`C:\foo\.`, true}, - {`C:\foo.`, false}, - {`C:\foo\ .`, false}, - {`C:\foo \`, false}, - {"C:/foo/", true}, - {"C:/foo", true}, - {"C:/foo/.", true}, - {"C:/foo/ .", false}, - {".", true}, - {"..", true}, - {"...", false}, - {`\\?\C:\foo`, true}, - {`\\?\C:\foo\ `, true}, - {`\\?\C:\foo\.`, true}, - {`\??\C:\foo`, true}, - {`\??\C:\foo\ `, true}, - {`\??\C:\foo\.`, true}, - {`\\server\share\path`, true}, - {"", true}, - {" ", false}, - {"C:.", true}, - {"C: ", false}, - {"C:..", true}, - {"C:...", false}, - {"foo:.", false}, - {"C:bar:..", false}, - {`\..`, true}, - {`\...`, false}, - {"/.", true}, - {"/..", true}, - {"a..", false}, - {"aa..", false}, - {"a ", false}, - {`a\ `, false}, - {`a \`, false}, - {`.\`, true}, - {`..\`, true}, - } - - for _, tt := range tests { - if os.ValidatePathForCreate(tt.path) != tt.pass { - t.Errorf("validatePathForCreate(%q) = %v, want %v", tt.path, !tt.pass, tt.pass) - } - } -}