path/filepath: Clean() incorrect trunc for volume-guid paths on Windows

[lowenna]

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

$ go version
go version go1.12rc1 windows/amd64

Does this issue reproduce with the latest release?

Yes, and is a change in behaviour from 1.11.x

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

go env Output

$ go env

PS E:\docker\build\golang1.12> go env
set GOARCH=amd64
set GOBIN=
set GOCACHE=C:\Users\Administrator\AppData\Local\go-build
set GOEXE=.exe
set GOFLAGS=
set GOHOSTARCH=amd64
set GOHOSTOS=windows
set GOOS=windows
set GOPATH=e:\go
set GOPROXY=
set GORACE=
set GOROOT=C:\Go
set GOTMPDIR=
set GOTOOLDIR=C:\Go\pkg\tool\windows_amd64
set GCCGO=gccgo
set CC=gcc
set CXX=g++
set CGO_ENABLED=1
set GOMOD=
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\ADMINI~1\AppData\Local\Temp\1\go-build584421990=/tmp/go-build -gno-record-gcc-switches
PS E:\docker\build\golang1.12>

@alexbrainman

This is the root cause of failures on Windows for upgrading github.com/moby/moby to golang 1.12. (See moby/moby#38404).

The cause appears to be the fix for #27791: d1f7470

In the MSDN documentation for "volume-guid"-style naming, Windows should have a trailing backslash (https://docs.microsoft.com/en-us/windows/desktop/FileIO/naming-a-volume). The specific quote is

To solve this problem, the operating system uses volume GUID paths to identify volumes. These are strings of this form:
"\\?\Volume{GUID}\"
where GUID is a globally unique identifier (GUID) that identifies the volume.

To simplify demonstration of the issue, run this test code on golang 1.11.5 and 1.12RC1 (or 1.12Beta)

package main

import (
	"fmt"
	"path/filepath"
)

func main() {
	p := `\\?\Volume{7650f9bc-33f8-11e9-9bf2-e06fe09706b2}\`
	fmt.Println(filepath.Clean(p))
}

On go 1.11.5

E:\docker\build\golang1.12> .\golang1.12.exe
\\?\Volume{7650f9bc-33f8-11e9-9bf2-e06fe09706b2}\ <nil>
E:\docker\build\golang1.12>

On go 1.12RC1

PS E:\docker\build\golang1.12> .\golang1.12.exe
\\?\Volume{7650f9bc-33f8-11e9-9bf2-e06fe09706b2} <nil>
PS E:\docker\build\golang1.12>

Specifically, the trailing backslash is now omitted making an invalid filepath following the MSDN documentation.

[thaJeztah]

@bcmills @mikioh can this be added to the 1.12 milestone (as it caused a regression)?

[mikioh]

I added the requested label and NeedsInvestigation temporarily.
@ianlancetaylor, can you please triage this issue?

[ianlancetaylor]

CC @QtRoS @alexbrainman

This does seem to have been introduced by https://golang.org/cl/137055 for #27991. I don't understand the fix. The issue says that Clean(\\somepath\dir\) should return \\somepath\dir, which seems reasonable although I don't know if it's actually correct. And I'm not sure what to make of the MSDN docs, which use a trailing slash for everything.

Given that this broke something, we should probably just roll it back and consider trying again for 1.13.

[gopherbot]

Change https://golang.org/cl/163077 mentions this issue: path/filepath: revert "fix Windows-specific Clean bug"

[ianlancetaylor]

I sent https://golang.org/cl/163077 but I don't really know if it's right. It is, at least, likely to be safe.

[lowenna]

Thanks @ianlancetaylor. I'm happy to try and figure out the right fix in conjunction with yourself and @alexbrainman for 1.13 if you would like. However, at RC1 of 1.12, I tend to agree that reverting the previous fix is probably the safest bet at this stage rather than trying to put a potentially faulty fix in.

I believe you are correct that a "traditional" UNC-style path Clean("\\server\share\") should return "\\server\share" based on the description of filepath.Clean(). Specifically Clean returns the shortest path name equivalent to path...

For volume-guid paths, the rule is, somewhat annoyingly, but hey - I didn't create this API!, slightly different. It stems from this sentence in https://docs.microsoft.com/en-us/windows/desktop/FileIO/naming-a-volume: All volume and mounted folder functions that take a volume GUID path as an input parameter require the trailing backslash. In other words, (excluding the CreateFile API exception), a volume-guid style path requires the trailing backslash when there's nothing following it.

So I believe (although haven't done full validation of this), the following should be the expected behaviour when calling Clean on volume-GUID style paths to get the shortest (valid) path name:

• \\?\Volume{GUID}\ --> \\?\Volume{GUID}\ (backslash remains otherwise invalid)
• \\?\Volume{GUID}\folder\ --> \\?\Volume{GUID}\folder (backslash stripped)*
• \\?\Volume{GUID}\folder\..\folder2\file.txt --> \\?\Volume{GUID}\folder2\file.txt

The one marked * is where I'm not entirely sure as there's some ambiguity reading the documentation, but is certainly what golang 1.11 does, and does not appear to have any issues I am aware of. The way I read the docs is that it's referring to when it's "just" a volume-guid style path, without any further relative paths added - ie just the \\?\Volume{GUID}\, not \\?\Volume{GUID}\something\else\here.txt

[gopherbot]

Change https://golang.org/cl/163078 mentions this issue: [release-branch.go1.12] path/filepath: revert "fix Windows-specific Clean bug"

[QtRoS]

@ianlancetaylor sorry for incomplete fix. Maybe changing method's documentation will be helpful in addiction to revert? I believe that current description is not correct, because of it some kind of misunderstanding and wrong expectations is possible.

[ianlancetaylor]

It is not obvious to me that the documentation is wrong. Perhaps \\server\share\ could be considered a kind of root directory. But I don't know much about Windows.

[alexbrainman]

I sent https://golang.org/cl/163077 but I don't really know if it's right. It is, at least, likely to be safe.

Thank you @ianlancetaylor for reverting promptly - sounds like the safest option. I will think about what to do here, and report back.

Alex

[lowenna]

@alexbrainman Hey Alex - am I interpreting gerrit correctly and the reversion didn't make into go 1.12 released yesterday? https://go-review.googlesource.com/c/go/+/163078 still shows active. So I guess I have to wait for 1.12.1? Thanks.

[alexbrainman]

@alexbrainman Hey Alex - am I interpreting gerrit correctly and the reversion didn't make into go 1.12 released yesterday? https://go-review.googlesource.com/c/go/+/163078 still shows active. So I guess I have to wait for 1.12.1?

I think you are correct.

There were 2 CLs merged - none of them made to go1.12.

https://golang.org/cl/163077 is merged to master branch.

And https://go-review.googlesource.com/c/go/+/163078 is merged to release-branch.go1.12 branch. There are 3 commits (including CL 163078) on top of go1.12 tag on that branch at this moment. That branch will be used to make go1.12.1.

Alex

[alexbrainman]

We still need to merge fix for #27791 (originally it was fixed by https://golang.org/cl/137055 but then reversed).

This time we need to make sure that fix for #27791 does not break paths that start with \\?\Volume{...}.

We can just adjust filepath.Clean carefully enough, so not to break volume-guid paths (we will still need new test for filepath.Clean(\\?\Volume{...}).

Alternatively, we could make sure that everything in path/filepath and os package handles volume-guid path first, and then fix #27791.

I was considering what is involved in doing the latter, and I think it is quite involved.

If we are to handle all non-DOS paths. we would have to handle (see https://docs.microsoft.com/en-us/windows/desktop/fileio/naming-a-file for details):

• Win32 File Namespaces = file paths start with \\?\
• Win32 Device Namespaces = file paths start with \\.\

Below are some random questions / issues I can think of in this area. There will be many more, like this.

---

What would VolumeName return for paths starting with \\?\ ?

\\?\C:\Users\Alex - ( VolumeName(C:\Users\Alex) returns C:)

\\?\UTC\server\share - ( VolumeName(\\server\share) returns \\server\share )

\\?\Volume{ABC}\

\\.\COM1

... probably many others, that I did not considered.

---

VolumeName was originally introduced to handle relative paths. But none of \\?\ or \\.\ can be relative. So VolumeName has no meaning for these.

And relative path can only start with "drive letter", like C: or D:, so VolumeName function name is wrong now - it should have being called DriveLetter or something.

---

There are many functions in path/filepath package that needs to be fixed, if we are to support paths starting with \\?\.

We also have many non-exported functions in os package (like isAbs, and many others), that needs to be fixed too.

A lot of work.

Maybe we could move all current implementation into internal package to avoid duplication.

---

I just submitted fix to make Readlink(\\?\Volume{ABC}) work (see CL 164201). My CL makes symlink targets, like \\?\Volume{ABC}, walk extra step past file that looks like \\?\Volume{ABC}, and into the file that looks like C:\....

But, if we are to support paths, like \\?\Volume{ABC}, maybe my CL should not do that extra step.

The CL will be released in go1.13. Maybe we should adjust my CL before go1.13 is released to prevent future compatibility problems.

---

Both / and \ in paths are OK to use in os and path/filepath at this moment.

But paths starting with \\?\ will not allow / in the paths.

---

Same.

Both . and .. in paths are OK to use in os and path/filepath.

Paths starting with \\?\ will not allow . or .. in the paths.

---

Path like \\?\Volume{ABC} is used for windows-arm - windows-arm temporary directory is stored inside of symlink to \\?\Volume{ABC} (see #29746 (comment) for details). Perhaps path like \\?\Volume{ABC} will become more common.

---

So should we just make small change in filepath.Clean, or should we make all code in path/filepath and os handle paths like \\?\ ?

/cc @bradfitz and @mattn for their opinion.

Alex