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
NeedsFix The path to resolution is known, but the work has not been done. OS-Windows
Milestone

Comments

@PatrLind
Copy link

PatrLind commented Aug 26, 2022

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

gopherbot commented Aug 26, 2022

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

@seankhliao
Copy link
Member

seankhliao commented Aug 26, 2022

see also #44279

@dr2chase
Copy link
Contributor

dr2chase commented Aug 26, 2022

@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

ianlancetaylor commented Aug 27, 2022

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

gopherbot commented Aug 27, 2022

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

@hopehook
Copy link
Member

hopehook commented Aug 27, 2022

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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