Fix Windows args and ArgsEscaped handling

[tianon-sso]

I was surprised to see ArgsEscaped being set on Linux images -- it's Windows specific and should never be set on Linux images. In a case of perfect timing, we got our first official buildkitd.exe builds and I realized the handling of this goes deeper now (involving the runtime/executors now too).

Previously to this, we were not properly escaping Windows command/arguments while constructing CommandLine, which has unexpected behavior.

To illustrate, I created a simple Go program that does nothing but fmt.Printf("%#v\n", os.Args). I installed it at C:\foo bar\args.exe and set CMD ["C:\\foo bar\\args.exe", "foo bar", "baz buzz"].

With just that, we get the expected []string{"C:\\foo bar\\args.exe", "foo bar", "baz buzz"} output from our program. However, when we also install args.exe as C:\\foo.exe, C:\\foo bar\\args.exe being unescaped at the start of CommandLine (thanks to ArgsEscaped: true) becomes ambiguous, and Windows chooses the more conservative path, and our output becomes []string{"C:\\foo", "bar\\args.exe", "foo bar", "baz buzz"} instead (even though we used the imperative/JSON form of CMD which should've avoided this!).

In the case of the new RUN support inside the builder, things were actually even worse! Our output (from RUN ["C:\\foo bar\\args.exe", "foo bar", "baz buzz"]) was instead []string{"C:\\foo", "bar\\args.exe", "foo", "bar", "baz", "buzz"} because the code was effectively just CommandLine = strings.Join(args, " "), which is definitely not enough. 😅

Several references to related discussions/code in Moby: 🚀

• https://github.com/moby/moby/pull/22868/files#discussion_r64126064
• Windows: Dockerfile RUN in JSON format fails to escape parameters moby#22874
• https://github.com/moby/moby/pull/38541/files#r265957738
• https://github.com/moby/moby/pull/38541/files#diff-115ee33702fa00530f92d6981036b1b569e804d5dbb86d357dc73359b3b714adR118
• https://github.com/moby/moby/blob/b8aa8579cad7b9c8712327093cbc602cfb6b417f/builder/dockerfile/dispatchers.go#L429-L457
• https://github.com/moby/moby/blob/b8aa8579cad7b9c8712327093cbc602cfb6b417f/builder/dockerfile/dispatchers.go#L475-L505

There are several TODOs in the code that I'm not actually sure how to resolve -- how to access ArgsEscaped from setArgs and withProcessArgs (which I'm not actually sure why Run and Exec are so separate in that containerdexecutor package 😅), and either how to emit a warning in dispatchCmd/dispatchEntrypoint or whether an error seems more appropriate (so I can actually test whether I've copied that conditional logic correctly 😂 😭 ❤️).

[tianon-sso]

Oh, and testing! There should probably be some tests for this, but I wasn't sure where to look or start. This has taken me days to work through already. 😅

[profnandaa]

Thanks for this, will take a look at this, this weekend.

[profnandaa]

Here are comments from my first pass. I'm also testing locally and will drop comments here.

Also just a general observation, first thanks for noting the TODOs! Any way we could just close on them as part of this PR? If the scope is a little wide, I'm open to splitting the work and helping.

[profnandaa]

nit: does moving this into the windows if-block have any effect on the Linux case?

[tianon-sso]

Yes, it absolutely does -- that was the original intent of my looking into this in the first place. In short, ArgsEscaped should never be set on a non-Windows image, and certainly never to true, and any case where it is gets (correctly) ignored by the runtimes anyhow. It's a 100% Windows-specific field.

[profnandaa]

nit: this block is repeated twice, L1390 and here?

[tianon-sso]

Correct -- we need separate, similar (but slightly different) handling for ENTRYPOINT vs CMD (see the linked original Moby PR/implementation).

[profnandaa]

nit: how about if the user just had cmd.exe instead of the absolute path?

[tianon-sso]

See the comment above (and the linked original PR) -- this is just to handle the special case of the default Cmd set in the published base images.

[profnandaa]

Here are comments from my first pass. I'm also testing locally and will drop comments here.

Some notes from testing

=> Is there anything you are doing to build CMD ["C:\\foo bar\\args.exe", "foo bar", "baz buzz"]? Mine doesn't build on both master and infosiftr:argsescaped:

FROM mcr.microsoft.com/windows/nanoserver:ltsc2022

RUN mkdir foo
RUN mkdir "foo bar"
# need to check how to excape spaces for COPY
COPY ./ ./
# COPY ./args.exe ./foo/args.exe
CMD [ "C:\\foo bar\\args.exe", "foo bar", "baz buzz" ]

build gives this error:

buildctl build `
>> --frontend=dockerfile.v0 `
>> --local context=. \ `
>> --local dockerfile=. `
>> --output type=image,name=docker.io/profnandaa/buildkit-args,push=true
...
...

 => ERROR [internal] load build context                                                                                                 0.1s
 => => transferring context: 123B                                                                                                       0.0s
------
 > [internal] load build context:
------
error: failed to solve: unclean path foo bar/args.exe: invalid argument

However, it's successful for docker build:

PS > docker build -t buildkit-args .
Sending build context to Docker daemon  3.827MB
Step 1/5 : FROM mcr.microsoft.com/windows/nanoserver:ltsc2022
 ---> 6ad91fb31728
Step 2/5 : RUN mkdir foo
 ---> Using cache
 ---> 522ec323b2ef
Step 3/5 : RUN mkdir "foo bar"
 ---> Using cache
 ---> 0df2959b2f98
Step 4/5 : COPY ./ ./
 ---> Using cache
 ---> 4c0a8b317934
Step 5/5 : CMD [ "C:\\foo bar\\args.exe", "foo bar", "baz buzz" ]
 ---> Using cache
 ---> 8c5e638d0180
Successfully built 8c5e638d0180
Successfully tagged buildkit-args:latest
PS > docker run --rm buildkit-args
[]string{"C:\\foo bar\\args.exe", "foo bar", "baz buzz"}

[crazy-max]

error: failed to solve: unclean path foo bar/args.exe: invalid argument

This looks related to fsutil: https://github.com/tonistiigi/fsutil/blob/7525a1af2bb545e89dc9bced785bff7a3b7f08c2/validator.go#L31

[tianon-sso]

Also just a general observation, first thanks for noting the TODOs! Any way we could just close on them as part of this PR? If the scope is a little wide, I'm open to splitting the work and helping.

Yes, absolutely! Sorry for not being more clear -- I do not intend for this to land with the TODOs as-is, but some of them are a little hairy and require some cross-function design work to pass around more data, so I didn't want to make a bunch of design decisions in a vacuum when I'm really not familiar with all this code. 😄

[tianon-sso]

Here's the Dockerfile I ended up with in my testing with CMD (which I was actually cross-building on a Linux buildkitd because fixing the Windows handling of arguments wasn't what I originally set out to do, but it should work fine on a Windows one too -- at most the GOOS/GOARCH bits might need to come out into explicit ENV instead):

FROM --platform=$BUILDPLATFORM golang AS build
COPY args.go ./
RUN GOOS=windows GOARCH=amd64 go build -o '/args.exe' ./args.go

FROM mcr.microsoft.com/windows/servercore:ltsc2022
COPY --from=build ["/args.exe","/foo bar/"]
COPY --from=build ["/args.exe","/foo.exe"]
CMD ["C:\\foo bar\\args.exe", "foo bar", "baz buzz"]

// args.go
package main

import (
        "fmt"
        "os"
)

func main() {
        fmt.Printf("%#v\n", os.Args)
}

You can find a built copy of the image at tianongravi468/test:windows-argsescaped-foobar (https://oci.dag.dev/?image=tianongravi468/test:windows-argsescaped-foobar).