path/filepath: EvalSymlinks doesn't work for Windows long filenames

[sethkoehler]

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

$ go version
go version go1.15 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=                                                                                                  
set GOARCH=amd64                                                                                                  
set GOBIN=                                                                                                        
set GOCACHE=C:\Users\sethkoehler\AppData\Local\go-build                                                           
set GOENV=C:\Users\sethkoehler\AppData\Roaming\go\env                                                             
set GOEXE=.exe                                                                                                    
set GOFLAGS=                                                                                                      
set GOHOSTARCH=amd64                                                                                              
set GOHOSTOS=windows                                                                                              
set GOINSECURE=                                                                                                   
set GOMODCACHE=C:\Users\sethkoehler\go\pkg\mod                                                                    
set GONOPROXY=                                                                                                    
set GONOSUMDB=                                                                                                    
set GOOS=windows                                                                                                  
set GOPATH=C:\Users\sethkoehler\go                                                                                
set GOPRIVATE=                                                                                                    
set GOPROXY=https://proxy.golang.org,direct                                                                       
set GOROOT=c:\go                                                                                                  
set GOSUMDB=sum.golang.org                                                                                        
set GOTMPDIR=                                                                                                     
set GOTOOLDIR=c:\go\pkg\tool\windows_amd64                                                                        
set GCCGO=gccgo                                                                                                   
set AR=ar                                                                                                         
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 -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=C:\Users\SETHKO~1\AppData\Local\Temp\go-build584735666=/tmp/go-build -gno-record-gcc-switches

What did you do?

Ran filepath.EvalSymlinks on filename of the form C:\abc...\abc.txt where this filepath was ~300 chars (anything over 259 should work). Did this with and without the \\?\ prefix.

What did you expect to see?

The symlink resolved (which does happen with os.Readlink).

What did you see instead?

"The system cannot find the path specified."

Reproducer

I believe the issue is simply that toNorm needs to implicitly add a \\?\ prefix to a long path (>= 260 chars including drive letter).

I've attached a standalone file where I copied the relevant parts of the filepath library (you can just run it as a standard go test). Notice that on lines 48-64, where we do what EvalSymlinks does internally for Windows, that walkSymlinks does not keep the \\?\ portion on the filename, even if a path is passed in with this prefix (which makes that workaround useless for EvalSymlinks :( ).

Also notice that the separated walkSymlinks and toNorm calls part of the test passes (but then the subsequent call to EvalSymlinks does not), because I add the \\?\ prefix back when calling toNorm (so it can handle long paths just fine it appears if using the prefix).

Note that the only relevant parts of this file are up to line 90 (the rest is standard library copied from Go): evalsymlinks_test.go.zip

[networkimprov]

EvalSymlinks (and other filesystem APIs) have a variety of long-standing problems on Windows.

But that aside, it should almost never be used in practice, see #40180.

[sethkoehler]

Thanks for the link! I read through the reasons why this should almost never be used in practice and I'd say none of those reasons apply to our situation. We're running some test code within a docker container, which should not be remounting paths or any other such shenanigans. We also aren't showing the result of this path to any user, but rather using it to ensure whether one path is contained in another.

For example, given /a/b, and a symlink /c pointing to /a, we need to determine that /c/b/f/../g is indeed still in /a/b, while /c/b/../f/g is not. I think EvalSymlinks seems perfect for this job, and I'd effectively end up rewriting it to handle all the cases of intermediate paths being symlinks if I can't use it.

The good news here is that, as showed in my example, the two subfunctions that EvalSymlinks calls both actually work with long paths, they just have different expectations of the format (the walkSymlinks doesn't care about \\?\, and returns paths without it, the toNorm function however expects long paths to start with \\?\). So, for a minimal fix, we could just add the following code to https://golang.org/src/path/filepath/symlink_windows.go?h=toNorm#L114 (which would be Windows only and EvalSymlinks specific:

if len(newPath) >= 259 && newPath[:4] != `\\?\` {
    newPath = `\\?\` + newPath
}

Or alternatively, is there another way to get such a contains check for folders that takes symlinks into account without simply rewriting EvalSymlinks from scratch?

[networkimprov]

os.Chdir() and os.Getwd() might be another option.

[sethkoehler]

That's an interesting idea, though I suppose I'd need to make sure none of the other go-routines were depending on the current directory (so EvalSymlinks still seems a bit cleaner, but I'll consider whether this will work for us).

[networkimprov]

cc @ericwj for ideas

[cagedmantis]

Thank you for creating an issue @sethkoehler. If you believe that your suggested fix will resolve your issue, please feel free to create a CL.

/cc @robpike @rsc

[sethkoehler]

Sounds good, I'll look into the procedure for creating a CL, thanks!

[ericwj]

Many API's including EvalSymlinks have the issue that they are not thread-safe for depending on global static state through the current working directory. Also take a look at the list of failures EvalSymlinks produces in the issue linked by @networkimprov. That list pretty much means that although whatever you may be able to fix in any test scenario is likely not fixing your issue for systems that have a real-world configuration. If you are writing this specifically for Windows, it might be necessary to use GetFinalPathNameByHandle yourself and perhaps a few other related Win32 API's than to try and play whack-a-mole with the go filepath implementation, but beware that it might be harder than it looks seeing how wrong Google got it. path\filepath was written disregarding a series of basic concepts and with wrong assumptions. Trying to fix it with a small patch is not prudent imho.

[sethkoehler]

Thanks for all the comments...after a bit of discussion, I think I'm going to go ahead with a PR unless there are strong objections. While whack-a-mole certainly doesn't sound fun, there's also the "leave it better than you found it" principle in the balance that can slowly iterate towards better. I think the main issue I'd been thinking about with the threaded behavior w.r.t. to changing the working directory of the process was that before we weren't doing that, which means multiple go routines could be running completely orthogonal os/exec.Cmd's (which use the working directory if it isn't specified), so I was just reminding myself that if I went that route, I'd need to check that we actually specified the working directory on all of those (and probably for good measure check other uses of the os and exec packages), but I don't suspect it'd be a problem. Thanks again!

[sethkoehler]

I should say...unless you know of a substantial rewrite of this package to address the issues coming imminently...in which case yes, I'd naturally defer to that :P

[ericwj]

You may be able to short-circuit whatever why you have to check this by writing a test file with a random name which is required to be created rather than overwritten and seeing whether it is present at the alternate path containing ... Yet obviously that only works if your app has write permissions on the target directory.

In the meantime for path\filepath to be redesigned and rewritten, @networkimprov has promised me to mobilize relevant people to look into the huge list of tests with issues that I made Google aware of and have these be tested during continuous integration builds of go. So first of all poke him to start doing that. The next step we've envisioned is to create an issue listing all problems with path\filepath that we are aware of, with general design problems and an especially long list of issues that are Windows specific. Watch for that and get people to upvote that. Same for the EvalSymlinks issue linked above, which you can already upvote today.

[networkimprov]

Actually, Seth, I think this is your best bet: #37113

The only folks I can really mobilize are other contributors to an open source project which is heavily dependent on the filesystem API and Windows port. And only when the project reaches a point where we can invest in Go fixes. Tho I can't be sure we'd find reviewers for such fixes; they might end up in a third-party library.

[sethkoehler]

Ok, I see that in the other issue you referenced, there was the notion of using the Windows API directly, and that someone might just do that when they add a canonicalize-path function (and then EvalSymlinks gets deprecated), so that would work for us too (and perhaps make fixing this function less useful then indeed). Ok, I think we'll wait for that instead then (as I agree, calling into the Windows API is probably a better long-term solution).

[sethkoehler]

Duplicate of #37113

[networkimprov]

In the meantime, you can call the Winapi yourself via package golang.org/x/sys/windows

[sethkoehler]

Yeah, I think that's a reasonable strategy, thanks!