Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

os: os.DirFS doesn't work with Windows UNC paths #54694

Closed
PatrLind opened this issue Aug 26, 2022 · 7 comments
Closed

os: os.DirFS doesn't work with Windows UNC paths #54694

PatrLind opened this issue Aug 26, 2022 · 7 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. OS-Windows
Milestone

Comments

@PatrLind
Copy link

What version of Go are you using (go version)?

$ go version
go version go1.19 linux/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

I build on Linux and run on Windows (Microsoft Windows [Version 10.0.19044.1889])

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/patrik/.cache/go-build"
GOENV="/home/patrik/.config/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/patrik/go/pkg/mod"
GONOPROXY="REDCATED"
GONOSUMDB="REDCATED"
GOOS="linux"
GOPATH="/home/patrik/go"
GOPRIVATE="REDACTED"
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.19"
GCCGO="gccgo"
GOAMD64="v1"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/patrik/REDACTED/go.mod"
GOWORK=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build4002045871=/tmp/go-build -gno-record-gcc-switches"

What did you do?

I created a test program that tries to walk the root dir. I test it with a normal path and also a UNC path.
The program gives the expected output on Linux using wine, but on a real Windows machine it doesn't work.
I created my own "fixed" implementation of os.DirFS that seems to solve the issue.

package main

import (
	"fmt"
	"io/fs"
	"os"
	"path/filepath"
	"runtime"
)

func main() {
	runTests()
}

func runTests() {
	type testT struct {
		name string
		fs   fs.FS
	}
	tests := []testT{
		{name: "Normal path 1", fs: os.DirFS("C:\\")},
		{name: "UNC Path 1", fs: os.DirFS("\\\\?\\C:\\")},
		{name: "Normal path 2", fs: OSDirFS("C:\\")},
		{name: "UNC Path 2", fs: OSDirFS("\\\\?\\C:\\")},
	}
	for _, t := range tests {
		fmt.Println("Testing:", t.name)
		err := testFS(t.fs)
		if err != nil {
			fmt.Println("Error:", err)
		} else {
			fmt.Println("OK")
		}
	}
}

func testFS(tfs fs.FS) error {
	err := fs.WalkDir(tfs, ".", func(path string, d fs.DirEntry, err error) error {
		if err != nil {
			return err
		}
		return fmt.Errorf("dummy")
	})
	if err != nil && err.Error() != "dummy" {
		return err
	}
	return nil
}

type osDirFS string

func OSDirFS(dir string) fs.FS {
	return osDirFS(dir)
}

func containsAny(s, chars string) bool {
	for i := 0; i < len(s); i++ {
		for j := 0; j < len(chars); j++ {
			if s[i] == chars[j] {
				return true
			}
		}
	}
	return false
}

func (dir osDirFS) Open(name string) (fs.File, error) {
	if !fs.ValidPath(name) || runtime.GOOS == "windows" && containsAny(name, `\:`) {
		return nil, &fs.PathError{Op: "open", Path: name, Err: fs.ErrInvalid}
	}

	f, err := os.Open(filepath.Join(string(dir), filepath.FromSlash(name)))
	if err != nil {
		return nil, err // nil fs.File
	}
	return f, nil
}

func (dir osDirFS) Stat(name string) (fs.FileInfo, error) {
	if !fs.ValidPath(name) || runtime.GOOS == "windows" && containsAny(name, `\:`) {
		return nil, &fs.PathError{Op: "stat", Path: name, Err: fs.ErrInvalid}
	}
	f, err := os.Stat(filepath.Join(string(dir), filepath.FromSlash(name)))
	if err != nil {
		return nil, err
	}
	return f, nil
}

What did you expect to see?

I expected the UNC path with fs.WalkDir() to work.

What did you see instead?

It gives me an error: CreateFile \\?\C:\/.: The filename, directory name, or volume label syntax is incorrect.
Seems like Windows doesn't like forward slashes in UNC paths.

Testing: Normal path 1
OK
Testing: UNC Path 1
Error: CreateFile \\?\C:\/.: The filename, directory name, or volume label syntax is incorrect.
Testing: Normal path 2
OK
Testing: UNC Path 2
OK

Suggested fix

I think the os.DirFS implementation should convert the forward style slashes of the fs.FS implementation to platform native path separators before sending the function calls off to the os.Open() and os.Stat().

@hopehook
Copy link
Member

hopehook commented Aug 26, 2022

@PatrLind Can you try replacing / with string(PathSeparator) of the Open Stat method, what happens.
From my point of view, dir and name cannot be automatically replaced by path separators, which is guaranteed by the user.

See: https://go-review.googlesource.com/c/go/+/425876

@gopherbot
Copy link

Change https://go.dev/cl/425876 mentions this issue: os: make os.DirFS work with Windows UNC paths

@seankhliao
Copy link
Member

see also #44279

@dr2chase
Copy link
Contributor

@rsc @iant, not sure what the best plan here is.
The fix recommended in the bug report does work, but creates an import cycle, which is unfortunate. A certain amount of code shuffling would fix that.

The fix recommended in https://go.dev/cl/425876 seems to not be enough.

@dr2chase dr2chase added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Aug 26, 2022
@ianlancetaylor
Copy link
Contributor

We can't import path/filepath but the functions being used are trivial, so let's just copy them into the os package. Only not literally because "os" shouldn't import "strings" either.

Does https://go.dev/cl/426094 fix the problem?

@gopherbot
Copy link

Change https://go.dev/cl/426094 mentions this issue: os: use backslashes for DirFS on Windows

@hopehook
Copy link
Member

We can't import path/filepath but the functions being used are trivial, so let's just copy them into the os package. Only not literally because "os" shouldn't import "strings" either.

Does https://go.dev/cl/426094 fix the problem?

I think it is a feasible and good solution.

@seankhliao seankhliao added this to the Go1.20 milestone Aug 27, 2022
@dmitshur dmitshur added OS-Windows NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Oct 1, 2022
@golang golang locked and limited conversation to collaborators Oct 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. OS-Windows
Projects
None yet
Development

No branches or pull requests

7 participants