Signed-off-by: John Howard [email protected]

@johnstep @thaJeztah PTAL. @jterry75 The final answer, FYI - no changes needed in containerd land 😄

TL;DR - Make sure we use long Windows path names in the build context. Found locally during validation of go 1.11 upgrade as per #37358.

After some detailed investigation 😓 😓 😓 , it turns out TestBuildSymlinkBreakout was a false positive prior to golang 1.11 on Windows (RS1..RS5) when running locally as administrator. More specifically, it will fail when running locally as any user whose temporary directory environment variable has been shortened to an 8.3 style name. With golang 1.11, the test will fail with failed to create new directory: mkdir \\?\Volume{028cfe96-8aec-48a2-8eb4-485d1f463aa2}\Users\ADMINI~1\AppData: The system cannot find the path specified. Short Windows paths aren't fully supported on volume-style paths, dependent on the API being called. GetLongPathName (to convert from a short to long path - https://docs.microsoft.com/en-us/windows/desktop/api/fileapi/nf-fileapi-getlongpathnamew) is one such API. By the time we in the daemon manipulating the container scratch space with the filter driver loaded through a volume-style path when creating files/directories through ADD/COPY commands in the builder, it's too late to convert to a longpath (and impossible for a remote client anyway - see summary at the bottom).

As an aside - for those interested, this doesn't hit in the moby/moby RS1 CI because a) the username is jenkins rather than administrator, and b) we redirect TEMP/TMP to a folder based on git commit on a faster SSD, using an explicit long path.

The crux of the issue comes down to the default TEMP/TMP environment variables Windows uses for administrator (this is historical and has been the case AFAIK since NT 3.51):

TEMP=C:\Users\ADMINI~1\AppData\Local\Temp\1
TMP=C:\Users\ADMINI~1\AppData\Local\Temp\1

Using the following test code snippet, where a volume has already been mounted through the graphdriver:

shortPath := `\\?\Volume{578f9d63-b149-11e8-b47b-b13678394dd7}\Users\ADMINI~1`
longPath := `\\?\Volume{578f9d63-b149-11e8-b47b-b13678394dd7}\Users\Administrator`

d, e := os.Stat(shortPath)
fmt.Println("Stat short:",d,e)

f,g := os.Lstat(shortPath)
fmt.Println("Lstat short:",f,g)

h, i := os.Stat(longPath)
fmt.Println("Stat long:",h,i)

j, k:= os.Lstat(longPath)
fmt.Println("Lstat long:",j,k)

The results on go 1.11

E:\docker\test\stat>.\stat.exe
Stat short: &{ADMINI~1 16 {3823784393 30687395} {3857382589 30687395} {3857382589 30687395} 0 0 0 0 {0 0} \\?\Volume{578f9d63-b149-11e8-b47b-b13678394dd7}\Users\ADMINI~1 0 0 0 false} <nil>
Lstat short: &{ADMINI~1 16 {3823784393 30687395} {3857382589 30687395} {3857382589 30687395} 0 0 0 0 {0 0} \\?\Volume{578f9d63-b149-11e8-b47b-b13678394dd7}\Users\ADMINI~1 0 0 0 false} <nil>
Stat long: &{Administrator 16 {3823784393 30687395} {3857382589 30687395} {3857382589 30687395} 0 0 0 0 {0 0} \\?\Volume{578f9d63-b149-11e8-b47b-b13678394dd7}\Users\Administrator 0 0 0 false} <nil>
Lstat long: &{Administrator 16 {3823784393 30687395} {3857382589 30687395} {3857382589 30687395} 0 0 0 0 {0 0} \\?\Volume{578f9d63-b149-11e8-b47b-b13678394dd7}\Users\Administrator 0 0 0 false} <nil>

And on go 1.10.4 and previous

E:\docker\test\stat>.\stat.exe
Stat short: <nil> CreateFile \\?\Volume{578f9d63-b149-11e8-b47b-b13678394dd7}\Users\ADMINI~1: The system cannot find the file specified.
Lstat short: <nil> GetFileAttributesEx \\?\Volume{578f9d63-b149-11e8-b47b-b13678394dd7}\Users\ADMINI~1: The system cannot find the file specified.
Stat long: &{Administrator {16 {3823784393 30687395} {3857382589 30687395} {3857382589 30687395} 0 0} 0 {0 0} \\?\Volume{578f9d63-b149-11e8-b47b-b13678394dd7}\Users\Administrator 0 0 0 false} <nil>
Lstat long: &{Administrator {16 {3823784393 30687395} {3857382589 30687395} {3857382589 30687395} 0 0} 0 {0 0} \\?\Volume{578f9d63-b149-11e8-b47b-b13678394dd7}\Users\Administrator 0 0 0 false} <nil>

Note that both stat and lstat return success on 1.11. On 1.10.4, this causes mkdirall (https://github.com/moby/moby/blob/master/pkg/system/filesys_windows.go#L86-L94) to behave differently when creating files in the read-write scratch layer, and actually create a new ADMIN~1 directory when the Administrator directory already exists - this is incorrect. I believe that when following this through to a conclusion, it is possible some files added during the build may actually be inaccessible subsequently after committing a layer as the newly created ADMIN~1 may not be recognised by some longpath-aware APIs as a short-name for administrator (which itself is localised), and we have both Administrator and ADMIN~1 folders existing, rather than just a single folder. I haven't actually proved this one way or another though, but will leave that as an interesting exercise for the reader :) On 1.11, this causes that same mkdirall to fail due to the os.Lstat on L89 succeeding.

(The golang fix appears correct - the 1.11 result above is the correct one, and due to golang/go#22579. Fix was implemented in golang/go@e83601b#diff-f63e1a4b4377b2fe0b05011db3df9599)

There are a few ways I thought about fixing this :

• Wrap all instances of ioutil.TempDir (in Windows paths) to call GetLongPathName. Including test code, that's a LOT of paths, most of which don't cause the underlying issue. However, it's pretty error-prone to do this - someone in the future will inadvertently forget to use the wrapper.�

E:\go\src\github.com\docker\docker [master ≡ +2 ~3 -0 !]> git grep ioutil.TempDir | wc -l
320

• Submit a fix to golang so that ioutil.TempDir() calls GetLongPathName before returning. I am extremely doubtful this would be accepted, and further wouldn't work in this case as it would require an updated CLI due to the way the test is written (it's not an API test), and CI is pinned to an old version of the CLI.�

• In the client when it builds the TAR. However, that doesn't fix API callers such as here, and further CI is pinned to an old client anyway.

• Fixing in the builder when it extracts the tar from the local context is converted to a long path. This wouldn't work though for a remote client as GetLongPathName wouldn't be able to resolve as it doesn't have access to the remote machines file system.�

• Treat as a test bug with a targeted fix to call GetLongPathName after calling ioutil.TempDir. That's what's in this PR

I put the wrapper to GetLongPathName under pkg\system, as I can see this being useful to others. I might move it to go-winio at a future point in time as that's becoming an increasingly useful library of Windows APIs.

😓