WCOW/dockerfile fontend: consistency in handling slashes slashes in C:\\

[profnandaa]

I have noticed we have some inconsistencies on how the dockerfile frontend handles the paths for Windows. I'm opening this issue quickly to track this but I'll provide better details with more tests.

So far, I've noticed this with COPY, but I need to do more tests.

Repro Steps

Dockerfile:

FROM mcr.microsoft.com/windows/nanoserver:ltsc2022
COPY hello.txt C:\
CMD ["cmd", "/C", "type C:\\hello.txt"]

Build command:

buildctl build `
    --frontend=dockerfile.v0 `
    --local context=. \ `
    --local dockerfile=. `
    --output type=image,name=docker.io/janedoe/hello-buildkit,push=false

Expected error:

[+] Building 0.3s (2/2) FINISHED
 => [internal] load build definition from Dockerfile                                                                                    0.0s
 => => transferring dockerfile: 160B                                                                                                    0.0s
 => [internal] load metadata for mcr.microsoft.com/windows/nanoserver:ltsc2022                                                          0.1s
Dockerfile:2
--------------------
   1 |         FROM mcr.microsoft.com/windows/nanoserver:ltsc2022
   2 | >>>     COPY hello.txt C:\
   3 | >>>     CMD ["cmd", "/C", "type C:\\hello.txt"]
   4 |
--------------------
error: failed to solve: failed to process "\"type": unexpected end of statement while looking for matching double-quote

Current Fix/Work-around

General rule of thumb: Use / instead of \ for paths, for a consistent experience.
For the above error, changing C:\ in COPY stanza to C:/ fixes it.

/cc. @gabriel-samfira

[TBBle]

I had started looking into this in #1621, but didn't get much further than some tests and a draft resolution approach that was determined to be the wrong approach. The branch is still around if anyone wants to look at it, as I suspected the issue was still around. See also #616 (comment)

That said, this one in particular might be not a problem with COPY per-se, but with the end-of-line continuation character. Does

FROM mcr.microsoft.com/windows/nanoserver:ltsc2022
COPY hello.txt C:\\
CMD ["cmd", "/C", "type C:\\hello.txt"]

work?

[gabriel-samfira]

I remember briefly looking at this a while ago and ending up somewhere in the shlex package. The \ character is defined as the escape character. But I really didn't dig too much into this.

While almost all Dockerfile stanzas will be fine with using / instead of \ one notable exception is the RUN stanza. Some Windows commands are really adamant about using actual \ as a path separator. In most cases, if we use:

# Actuall SHELL stanza parameters may differ
SHELL ["powershell.exe", "-Command"]

and use PowerShell as a shell instead of cmd.exe, we can get away with using / as a path delimiter for most commands. In any case, I may have some time next week to have a look.

[profnandaa]

@TBBle -- thanks for taking a look. You are right, the issue is with C:\, the backslash not being escaped, so C:\\ it is. So it's not a COPY stanza thing.
However, this dockerfile works with docker build but fails with buildctl build:

FROM mcr.microsoft.com/windows/nanoserver:ltsc2022
COPY hello.txt C:\\
CMD ["cmd", "/C", "type C:\\hello.txt"]

I get this:

buildctl build `
>>  --output type=image,name=docker.nandaa.dev/test,push=false `
>>  --progress plain "-frontend=dockerfile.v0" `
>>  --local context=C:/dev/play/dockerfiles/repro-4696 `
>>  --local dockerfile=C:\dev\play\dockerfiles\repro-4696
#1 [internal] load build definition from Dockerfile
#1 transferring dockerfile: 151B done
#1 DONE 0.0s

#2 [internal] load metadata for mcr.microsoft.com/windows/nanoserver:ltsc2022
#2 DONE 1.8s

#3 [internal] load .dockerignore
#3 transferring context: 2B done
#3 DONE 0.0s

#4 [internal] load build context
#4 transferring context: 43B done
#4 DONE 0.0s

#5 [1/2] FROM mcr.microsoft.com/windows/nanoserver:ltsc2022@sha256:6e6053f0358f9522d2d14693f9bc152f47fe04c82c53dc8c6d127a5a823c8720
#5 resolve mcr.microsoft.com/windows/nanoserver:ltsc2022@sha256:6e6053f0358f9522d2d14693f9bc152f47fe04c82c53dc8c6d127a5a823c8720 0.1s done
#5 CACHED

#6 [2/2] COPY hello.txt C:\
#6 ERROR: cleaning path: removing drive letter: UNC paths are not supported
------
 > [2/2] COPY hello.txt C:\:
------
Dockerfile:2
--------------------
   1 |     FROM mcr.microsoft.com/windows/nanoserver:ltsc2022
   2 | >>> COPY hello.txt C:\\
   3 |     CMD ["cmd", "/C", "type C:\\hello.txt"]
   4 |
--------------------
error: failed to solve: cleaning path: removing drive letter: UNC paths are not supported

[TBBle]

That error suggests we're failing at https://github.com/moby/buildkit/blob/master/util/system/path.go#L190-L193.

I suspect, based on rough recollection of #1621 investigations, that the parse correctly produced C:\, but a use of path.clean or locally-coded equivalent on that string later produced C:\/, and that was normalised to \/, and that was given to CheckSystemDriveAndRemoveDriveLetter, which in that linked code turns it into // and hence looks like a (null) UNC path. We don't see this in Docker's built-in builder because it doesn't go through so many layers (no dockerfile2llb) so it doesn't hit this issue. (I suspect in Docker we are simply using filepath.clean because we don't actually support cross-OS image builds, so the daemon can assume hostOS == targetOS and use filepath directly.)

I don't think we saw this when building Windows containers on Linux though, but I don't remember why.

The discussion in #1621 for this sort of issue was that dockerfile2llb should take care of this sort of thing, and LLB should only see unix-style paths. During container build steps that aren't RUN, we can assume only C: is present, so the "strip drive letter" logic could probably also be moved into the dockerfile2llb layer (and barf there if someone tries to COPY to a different drive letter, or UNC path, etc.)

(Edit: Just noticed that this failure is probably happening during dockerfile2llb processing (dispatchCopy calling system.NormalizePath I think) so structurally it's already trying to fix the paths before they're seen by LLB. The "Fix should be in dockerfile2llb" comment was about my attempted fix in #1621, which was in the instruction parser, i.e. earlier in the stack. Also, a bunch of code has moved since then, so my fix might not even be applicable. My test-case was in dockerfile2llb at least.)

Also, I'll note that my repro in #1621 was with WORKDIR, so that one's also worth including in tests. I imagine things like the --mount argument's target path in RUN have similar risks, but they might just be flowing through in to execution arguments verbatim.

[profnandaa]

@TBBle -- yes, looks close. Found this on my debugging, Dest: "/\\":

> github.com/moby/buildkit/solver/llbsolver/ops.(*FileOpSolver).getInput() C:/dev/container-core/buildkit/solver/llbsolver/ops/file.go:412 (hits goroutine(764):1 total:1) (PC: 0x229c996)
   411:
=> 412: func (s *FileOpSolver) getInput(ctx context.Context, idx int, inputs []fileoptypes.Ref, actions []*pb.FileAction, g session.Group) (input, error) {
...
(dlv) p actions[0].Action
github.com/moby/buildkit/solver/pb.isFileAction_Action(*github.com/moby/buildkit/solver/pb.FileAction_Copy) *{
        Copy: *github.com/moby/buildkit/solver/pb.FileActionCopy {
                Src: "/hello.txt",
                Dest: "/\\",
                Owner: *github.com/moby/buildkit/solver/pb.ChownOpt nil,
                Mode: -1,
                FollowSymlink: true,
                DirCopyContents: true,
                AttemptUnpackDockerCompatibility: false,
                CreateDestPath: true,
                AllowWildcard: true,
                AllowEmptyWildcard: true,
                Timestamp: -1,
                IncludePatterns: []string len: 0, cap: 0, nil,
                ExcludePatterns: []string len: 0, cap: 0, nil,
                AlwaysReplaceExistingDestPaths: false,},}
(dlv)

Further up:

> github.com/moby/buildkit/client/llb.(*fileActionCopy).toProtoAction() C:/dev/container-core/buildkit/client/llb/fileop.go:526 (PC: 0x17adb8a)
   521:         if err != nil {
   522:                 return nil, err
   523:         }
   524:         c := &pb.FileActionCopy{
   525:                 Src:                              src,
=> 526:                 Dest:                             normalizePath(parent, a.dest, true),
   527:                 Owner:                            a.info.ChownOpt.marshal(base),
   528:                 IncludePatterns:                  a.info.IncludePatterns,
   529:                 ExcludePatterns:                  a.info.ExcludePatterns,
   530:                 AllowWildcard:                    a.info.AllowWildcard,
   531:                 AllowEmptyWildcard:               a.info.AllowEmptyWildcard,
(dlv) p a.dest
"/\\"

Looks like normalizePath() isn't doing a good job here; I'm sending a fix.