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: MkdirAll fails when a parent folder contains trailing spaces #54040

Open
megabild opened this issue Jul 25, 2022 · 9 comments
Open

os: MkdirAll fails when a parent folder contains trailing spaces #54040

megabild opened this issue Jul 25, 2022 · 9 comments
Labels
NeedsInvestigation OS-Windows

Comments

@megabild
Copy link

@megabild megabild commented Jul 25, 2022

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

$ go version
go version go1.18.4 windows/amd64

Does this issue reproduce with the latest release?

yes

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

go env Output
$ go env
set GO111MODULE=on
set GOARCH=amd64
set GOBIN=
set GOCACHE=C:\Users\TEST\AppData\Local\go-build
set GOENV=C:\Users\TEST\AppData\Roaming\go\env
set GOEXE=.exe
set GOEXPERIMENT=
set GOFLAGS=
set GOHOSTARCH=amd64
set GOHOSTOS=windows
set GOINSECURE=
set GOMODCACHE=C:\Users\TEST\go\pkg\mod
set GONOPROXY=
set GONOSUMDB=
set GOOS=windows
set GOPATH=C:\Users\TEST\go;C:\Daten\go
set GOPRIVATE=
set GOPROXY=https://proxy.golang.org,direct
set GOROOT=C:\Program Files\Go
set GOSUMDB=sum.golang.org
set GOTMPDIR=
set GOTOOLDIR=C:\Program Files\Go\pkg\tool\windows_amd64
set GOVCS=
set GOVERSION=go1.18.4
set GCCGO=gccgo
set GOAMD64=v1
set AR=ar
set CC=gcc
set CXX=g++
set CGO_ENABLED=1
set GOMOD=C:\Daten\go\src\megabildgo\go.mod
set GOWORK=
set CGO_CFLAGS=-g -O2
set CGO_CPPFLAGS=
set CGO_CXXFLAGS=-g -O2
set CGO_FFLAGS=-g -O2
set CGO_LDFLAGS=-g -O2
set PKG_CONFIG=pkg-config
set GOGCCFLAGS=-m64 -mthreads -fmessage-length=0 -fdebug-prefix-map=C:\Users\TEST\AppData\Local\Temp\go-build1829370843=/tmp/go-build -gno-record-gcc-switches

What did you do?

err := os.MkdirAll("C:\\temp\\folder \\this one fails", 0644)
if err != nil {
fmt.Println(err)
}

What did you expect to see?

All folders being created

What did you see instead?

There is a folder "folder" without trailing space. The folder "this one fails" is not created.
an os.PathError is thrown:
mkdir C:\temp\folder \this one fails: Das System kann den angegebenen Pfad nicht finden.

@mengzhuo mengzhuo added the NeedsInvestigation label Jul 25, 2022
@mengzhuo
Copy link
Contributor

@mengzhuo mengzhuo commented Jul 25, 2022

cc @golang/windows

@qmuntal
Copy link
Contributor

@qmuntal qmuntal commented Jul 25, 2022

Windows docs have a special entry for dealing with whitespaces in files and folder names: https://docs.microsoft.com/en-us/troubleshoot/windows-client/shell-experience/file-folder-name-whitespace-characters.

The relevant part is this:

File and Folder names that begin or end with the ASCII Space (0x20) will be saved without these characters. File and Folder names that end with the ASCII Period (0x2E) character will also be saved without this character. All other trailing or leading whitespace characters are retained.

Looking at how MkdirAll is implemented on windows, it seems that we are not removing leading and trailing whitespaces from non-leaf folders. This leads to failure mode:

  1. MkdirAll("C:\temp\folder \this one fails", 0644)
  2. MkdirAll("C:\temp", 0644)
  3. MkdirAll("C:\temp\folder ", 0644) // But Windows created "C:\temp\folder"!!
  4. Mkdir("C:\temp\folder \this one fails", 0644) // Wops, parent folder does not exist

@seankhliao seankhliao changed the title os/path: MkdirAll fails when a parent folder contains trailing spaces os: MkdirAll fails when a parent folder contains trailing spaces Jul 25, 2022
@seankhliao
Copy link
Member

@seankhliao seankhliao commented Jul 25, 2022

how much normalization should we do vs having the user specify \\?\ when they need weird paths?

@qmuntal
Copy link
Contributor

@qmuntal qmuntal commented Jul 25, 2022

Normalizing this should be trivial, but I would argue against doing so. It would fix the MkdirAll, yet the underlying problem will still be there: the white-spaced path won't exist even though MkdirAll succeed, so any other operation on that path is going to fail.

We either always normalize all paths or we don't, else we will just be moving the failure to a later point, making it worst.

As @seankhliao mentioned, Windows already supports trailing whitespaces when the path is prefixed with \\?\, so the workaround is there.

Additional data point: .Net does not support this either for the sake of interoperability. dotnet/runtime#23292 (comment).

@mattn
Copy link
Member

@mattn mattn commented Jul 25, 2022

I am also against normalizing this. If Windows changes its behavior in the future, Go will have to follow the changes.

@megabild
Copy link
Author

@megabild megabild commented Jul 25, 2022

Normalizing this should be trivial, but I would argue against doing so. It would fix the MkdirAll, yet the underlying problem will still be there: the white-spaced path won't exist even though MkdirAll succeed, so any other operation on that path is going to fail.

We either always normalize all paths or we don't, else we will just be moving the failure to a later point, making it worst.

As @seankhliao mentioned, Windows already supports trailing whitespaces when the path is prefixed with \\?\, so the workaround is there.

Additional data point: .Net does not support this either for the sake of interoperability. dotnet/runtime#23292 (comment).

Creating the folder with \?\ works with MkdirAll. Can we make this the default behavior on windows? Otherwise the programmer needs to prefix the MkdirAll call if needed. I wonder if anyone even knows about that special \?\ thing.

@qmuntal
Copy link
Contributor

@qmuntal qmuntal commented Jul 25, 2022

I wonder if anyone even knows about that special \\?\ thing.

From Windows docs:

For file I/O, the "\?" prefix to a path string tells the Windows APIs to disable all string parsing and to send the string that follows it straight to the file system.

@bcmills
Copy link
Member

@bcmills bcmills commented Jul 25, 2022

We either always normalize all paths or we don't, else we will just be moving the failure to a later point, making it worse.

I agree that we should not normalize paths here, but could we at least improve the failure mode here?

(Perhaps something in the call stack could notice the leading or trailing space in the intermediate paths and fail with a clearer error message.)

@qmuntal
Copy link
Contributor

@qmuntal qmuntal commented Jul 25, 2022

I agree that we should not normalize paths here, but could we at least improve the failure mode here?

Improving the error message seems like a good idea, but I would try to avoid adding logic to detect leading/trailing whitespaces or periods and instead rely on giving general tips based on windows error codes.

For example, MkdirAll logic ensures that intermediate directories exist by calling CreateDirectory on every non-existing intermediate directory. So, if syscall.CreateDirectory returns ERROR_PATH_NOT_FOUND (see error description in docs), it means that either a concurrent process deleted the directory or that Windows did not honor the folder path in a previous CreateDirectory call.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation OS-Windows
Projects
None yet
Development

No branches or pull requests

6 participants