Fix Windows args and `ArgsEscaped` handling #4723

tianon-sso · 2024-03-01T00:54:15Z

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: 🚀

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 · 2024-03-01T01:02:07Z

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 · 2024-03-01T21:56:58Z

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 · 2024-03-04T04:44:35Z

frontend/dockerfile/dockerfile2llb/convert.go

 	d.image.Config.Cmd = args
-	d.image.Config.ArgsEscaped = true //nolint:staticcheck // ignore SA1019: field is deprecated in OCI Image spec, but used for backward-compatibility with Docker image spec.


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

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.

The ArgsEscaped field was added in containerd a while ago and it essentially converts the behavior of the Cmd field into what CmdLine does (roughly). If ArgsEscaped is set to true, the shim expects that ENTRYPOINT or CMD is a single element array, with the command, along with any other arguments, already escaped.

For example, if we have ArgsEscaped set to true, by the time the CMD reaches the shim, it should look like this:

CMD ["\"c:\\Path to\\Some\\application.exe\" "some space delimited arg" someOtherArg"]

And that gets passed along to HcsCreateProcess. I remember having to deal with double escaping of args when I first implemented the windows executor. Back then we were sending spec.Cmd along with args. And the args were getting escaped again once they got to the shim. Details are fuzzy, but the consensus when I discussed this issue with the MSFT folks was to just send the one string as CmdLine instead of Cmd and Args.

I think this change is okay, as long as we just log the inconsistency in place of that TODO, similarly to what happens in moby now.

profnandaa · 2024-03-04T04:49:55Z

frontend/dockerfile/dockerfile2llb/convert.go

@@ -1398,6 +1407,23 @@ func dispatchEntrypoint(d *dispatchState, c *instructions.EntrypointCommand) err
 	if c.PrependShell {
 		args = withShell(d.image, args)
 	}
+	if d.image.OS == "windows" {


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

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

profnandaa · 2024-03-04T04:53:45Z

frontend/dockerfile/dockerfile2llb/convert.go

+		if d.image.Config.ArgsEscaped != argsEscaped &&
+			(len(d.image.Config.Cmd) > 1 ||
+				(len(d.image.Config.Cmd) == 1 &&
+					strings.ToLower(d.image.Config.Cmd[0]) != `c:\windows\system32\cmd.exe` &&


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

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 · 2024-03-04T11:47:07Z

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 · 2024-03-04T11:52:42Z

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 · 2024-03-04T21:37:51Z

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 · 2024-03-04T21:44:41Z

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).

tonistiigi · 2024-05-28T22:50:31Z

@gabriel-samfira @profnandaa What's the state of this? Is it something we can include in v0.14?

gabriel-samfira · 2024-05-29T04:22:51Z

Will look at this in a couple of hours. Sorry for completely missing the notifications from this.

gabriel-samfira · 2024-05-29T07:44:03Z

I created a Dockerfile with the following contents:

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

RUN mkdir foo
RUN mkdir "foo bar"

COPY ["/args.exe","/foo bar/"]
COPY ["/args.exe","/foo.exe"]
COPY ["/args.exe","/"]

# Notice that when the command you're running has a space in it, you need to quote
# the entire line. The RUN stanza does not get escaped, so you need to take care of escaping
# yourself.
# Make sure that:
#
# cmd.exe /S /C <everything that comes after RUN>
#
# is correct. 
RUN ""c:\\foo bar\\args.exe" "foo bar" "baz buzz""

# This should run fine.
RUN C:\\args.exe "foo bar" "baz buzz" hello

CMD ["c:\\foo bar\\args.exe", "foo bar", "baz buzz"]

I then built the dockerfile:

PS C:\Users\Administrator\args> buildctl build --frontend=dockerfile.v0 --progress plain --local context=. --local dockerfile=. --output type=image,name=docker.io/gsamfira/buildkit-args,push=true
#1 [internal] load build definition from Dockerfile
#1 transferring dockerfile: 371B done
#1 DONE 0.1s

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

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

#4 [1/8] FROM mcr.microsoft.com/windows/nanoserver:ltsc2022@sha256:ac6a7571d5a404398e2f734d92f9b8f580a4fe1e6ae1820a61c5f138b1bdeff3
#4 resolve mcr.microsoft.com/windows/nanoserver:ltsc2022@sha256:ac6a7571d5a404398e2f734d92f9b8f580a4fe1e6ae1820a61c5f138b1bdeff3 0.1s done
#4 DONE 0.1s

#5 [internal] load build context
#5 transferring context: 2.00MB 0.0s done
#5 DONE 0.1s

#4 [1/8] FROM mcr.microsoft.com/windows/nanoserver:ltsc2022@sha256:ac6a7571d5a404398e2f734d92f9b8f580a4fe1e6ae1820a61c5f138b1bdeff3
#4 extracting sha256:6ef672c2d22f72854c3c475bef3811e110f08f4b049cb0023435b993651ea048
#4 extracting sha256:6ef672c2d22f72854c3c475bef3811e110f08f4b049cb0023435b993651ea048 8.8s done
#4 DONE 8.9s

#6 [2/8] RUN mkdir foo
#6 DONE 1.7s

#7 [3/8] RUN mkdir "foo bar"
#7 DONE 1.9s

#8 [4/8] COPY [/args.exe,/foo bar/]
#8 DONE 0.9s

#9 [5/8] COPY [/args.exe,/foo.exe]
#9 DONE 0.3s

#10 [6/8] COPY [/args.exe,/]
#10 DONE 0.3s

#11 [7/8] RUN ""c:\foo bar\args.exe" "foo bar" "baz buzz""
#11 1.138 []string{"c:\\\\foo bar\\\\args.exe", "foo bar", "baz buzz"}
#11 DONE 1.5s

#12 [8/8] RUN C:\args.exe "foo bar" "baz buzz" hello
#12 1.796 []string{"C:\\\\args.exe", "foo bar", "baz buzz", "hello"}
#12 DONE 2.1s

#13 exporting to image
#13 exporting layers
#13 exporting layers 1.8s done
#13 exporting manifest sha256:2ce4ddc37f834278b51294103cb772b9ecde9e4dc3bb80b3b5410b21d18e92f2 0.0s done
#13 exporting config sha256:9d785e5c076ff553a7b01f57acc72a70f0cf4b13b986d8288933ac888f94640a 0.0s done
#13 naming to docker.io/gsamfira/buildkit-args done
#13 pushing layers
#13 ...

#14 [auth] gsamfira/buildkit-args:pull,push token for registry-1.docker.io
#14 DONE 0.0s

#13 exporting to image
#13 pushing layers 3.9s done
#13 pushing manifest for docker.io/gsamfira/buildkit-args:latest@sha256:2ce4ddc37f834278b51294103cb772b9ecde9e4dc3bb80b3b5410b21d18e92f2
#13 pushing manifest for docker.io/gsamfira/buildkit-args:latest@sha256:2ce4ddc37f834278b51294103cb772b9ecde9e4dc3bb80b3b5410b21d18e92f2 0.4s done
#13 DONE 6.2s

If I inspect the image:

PS C:\Users\Administrator> docker inspect docker.io/gsamfira/buildkit-args
[
    {
        "Id": "sha256:9d785e5c076ff553a7b01f57acc72a70f0cf4b13b986d8288933ac888f94640a",
        "RepoTags": [
            "gsamfira/buildkit-args:latest"
        ],
        "RepoDigests": [
            "gsamfira/buildkit-args@sha256:2ce4ddc37f834278b51294103cb772b9ecde9e4dc3bb80b3b5410b21d18e92f2"
        ],
        "Parent": "",
        "Comment": "buildkit.dockerfile.v0",
        "Created": "2024-05-29T00:38:14.803361-07:00",
        "DockerVersion": "",
        "Author": "",
        "Config": {
            "Hostname": "",
            "Domainname": "",
            "User": "ContainerUser",
            "AttachStdin": false,
            "AttachStdout": false,
            "AttachStderr": false,
            "Tty": false,
            "OpenStdin": false,
            "StdinOnce": false,
            "Env": [
                "PATH=c:\\Windows\\System32;c:\\Windows"
            ],
            "Cmd": [
                "c:\\foo bar\\args.exe",
                "foo bar",
                "baz buzz"
            ],
            "ArgsEscaped": true,
            "Image": "",
            "Volumes": null,
            "WorkingDir": "",
            "Entrypoint": null,
            "OnBuild": null,
            "Labels": null
        },
        "Architecture": "amd64",
        "Os": "windows",
        "OsVersion": "10.0.20348.2461",
        "Size": 304141881,
        "GraphDriver": {
            "Data": {
                "dir": "C:\\ProgramData\\docker\\windowsfilter\\7fa65018234eac621261aa25e94452e9ee3ada5d614835069ffef166303416ec"
            },
            "Name": "windowsfilter"
        },
        "RootFS": {
            "Type": "layers",
            "Layers": [
                "sha256:84f6522b491493a414e7db5cca32dfaff6cfbf9b3b3fa1565699b84b7d0ce71a",
                "sha256:f61517395295ff1a667dc9f085c8353d994b7044b5c2d0fa98ec14bfe7590940",
                "sha256:ea07b12033b8110232b7a14bf9e918ffedb6d3bafff3cfe122cba3e36bf013e4",
                "sha256:32e6c5b9f7837e88508509fd7a08659b052df4ed376c22981e37619b890ae106",
                "sha256:c078682e46f376efeda987f86d9433f2d06b4de6bda78007bc3cf6a1c115c8fd",
                "sha256:2635bb703e549b57eee78570dab40edc9f58c134eb0ef72113d9608222d43e06",
                "sha256:5c2d2ae98ff2199772ffe2eaeabfe6f4cca0667893d7ba30c27aab56b2fa80ee",
                "sha256:18746cee89bb1daa0bce117c0f3ad1dd8b032620dc97e5f26719cbd421042880"
            ]
        },
        "Metadata": {
            "LastTagTime": "0001-01-01T00:00:00Z"
        }
    }
]

And when I run it:

PS C:\Users\Administrator> docker run --rm docker.io/gsamfira/buildkit-args:latest
[]string{"c:\\foo bar\\args.exe", "foo bar", "baz buzz"}
PS C:\Users\Administrator>

The behavior of RUN is expected and it has to do with the fact that there is no sane way to implement escaping, especially considering that SHELL can be specified to replace cmd.exe, and powershell has different escaping semantics than cmd. So the decission was probably made to just pass the entire string we pass to the RUN stanza as is. This means we need to escape the string ourselves. The above shows a working example of a command that has a space in it.

The CMD stanza seems to work as expected even with the released binary I used to test the above.

I will add some comments regarding the ArgsEscaped field a bit later today. I need to step out for a bit.

gabriel-samfira · 2024-05-29T07:48:11Z

For reference, this is my current environment:

PS C:\Users\Administrator> containerd.exe -version
containerd github.com/containerd/containerd v1.7.13 7c3aca7a610df76212171d200ca3811ff6096eb8
PS C:\Users\Administrator> buildkitd.exe -version
buildkitd github.com/moby/buildkit v0.13.0-rc2 596ef8f01e11e15889576a88ffa4c7f92fa44518
PS C:\Users\Administrator> docker version
Client:
 Version:           26.1.3
 API version:       1.45
 Go version:        go1.21.10
 Git commit:        b72abbb
 Built:             Thu May 16 08:34:37 2024
 OS/Arch:           windows/amd64
 Context:           default

Server: Docker Engine - Community
 Engine:
  Version:          26.1.3
  API version:      1.45 (minimum version 1.24)
  Go version:       go1.21.10
  Git commit:       8e96db1
  Built:            Thu May 16 08:33:14 2024
  OS/Arch:          windows/amd64
  Experimental:     false

PS C:\Users\Administrator> gcim win32_operatingsystem | select Caption,BuildNumber,Version

Caption                                             BuildNumber Version
-------                                             ----------- -------
Microsoft Windows Server 2022 Datacenter Evaluation 20348       10.0.20348

gabriel-samfira · 2024-05-29T12:19:12Z

Ahh, I also had a foo folder, that's why it didn't pick up the foo.exe file that was also present there. When just foo.exe is present, then yes, the issue manifests, but to disambiguate, we either have to escape the command itself, or use something like:

CMD ["\"c:\\foo bar\\args.exe\"", "foo bar", "baz buzz"]

Which I admit is atrocious user experience. This was an edge case that I sadly did not think about 😞.

But for this case to be solved, I believe it's enough to escape just the first element of CMD.

gabriel-samfira

The executor changes are not really needed as far as I can tell. But the ones in dockerfile2llb seem okay.

gabriel-samfira · 2024-05-29T13:19:20Z

executor/containerdexecutor/executor_windows.go

@@ -102,5 +103,12 @@ func (d *containerState) getTaskOpts() ([]containerd.NewTaskOpts, error) {
 }

 func setArgs(spec *specs.Process, args []string) {
-	spec.CommandLine = strings.Join(args, " ")
+	// TODO handle ArgsEscaped correctly here somehow (ie, avoid re-escaping args[0] if it's true)


On my system, with the default shell that buildkitd uses (cmd /S /C) this change will make the following RUN stanza:

RUN C:\\args.exe "foo bar" "baz buzz" hello

result in the following:

#9 [5/5] RUN C:\args.exe "foo bar" "baz buzz" hello #9 1.021 []string{"C:\\\\args.exe", "\"foo", "bar\"", "\"baz", "buzz\"", "hello"} #9 DONE 1.4s

And if we use a space in the command:

RUN "c:\\foo bar\\args.exe" "foo bar" "baz buzz" hello

results in the following error:

#9 [5/6] RUN "c:\foo bar\args.exe" "foo bar" "baz buzz" hello #9 1.054 '\"c:\\foo bar\\args.exe\"' is not recognized as an internal or external command, #9 1.054 operable program or batch file. #9 ERROR: process "cmd /S /C \"c:\\\\foo bar\\\\args.exe\" \"foo bar\" \"baz buzz\" hello" did not complete successfully: exit code: 1 time="2024-05-29T06:14:56-07:00" level=debug msg="stopping session" spanID=44e1bed51aee5c31 traceID=76af8ec835d4f23d69fb3cbf608e24e5 ------ > [5/6] RUN "c:\foo bar\args.exe" "foo bar" "baz buzz" hello: 1.054 '\"c:\\foo bar\\args.exe\"' is not recognized as an internal or external command, 1.054 operable program or batch file. ------ Dockerfile:18 -------------------- 16 | # 17 | # is correct. 18 | >>> RUN "c:\\foo bar\\args.exe" "foo bar" "baz buzz" hello 19 | 20 | # This should run fine. -------------------- error: failed to solve: process "cmd /S /C \"c:\\\\foo bar\\\\args.exe\" \"foo bar\" \"baz buzz\" hello" did not complete successfully: exit code: 1

So escaping the args before sending them as part of the CmdLine, will break the default shell which is undesirable. The string in the RUN stanza gets passed to the default shell as is, as one string. As long as the command is already escaped in the Dockerfile, it should work. See bellow.

Without the change:

# Notice that the whole line is quoted in this case RUN ""c:\\foo bar\\args.exe" "foo bar" "baz buzz" hello"

results in:

#9 [5/6] RUN ""c:\foo bar\args.exe" "foo bar" "baz buzz" hello" #9 1.188 []string{"c:\\\\foo bar\\\\args.exe", "foo bar", "baz buzz", "hello"} #9 DONE 1.5s

And:

# No quotes needed for the entire line RUN C:\\args.exe "foo bar" "baz buzz" hello

results in:

#10 [6/6] RUN C:\args.exe "foo bar" "baz buzz" hello #10 1.914 []string{"C:\\\\args.exe", "foo bar", "baz buzz", "hello"} #10 DONE 2.3s

If we replace the default shell with something more forgiving, escaping every arg does not make a difference:

FROM mcr.microsoft.com/powershell:lts-7.2-nanoserver-ltsc2022 RUN mkdir "foo bar" SHELL ["C:\\Program Files\\PowerShell\\pwsh.exe", "-Command", "$ErrorActionPreference = 'Stop'; $ProgressPreference = 'SilentlyContinue';"] COPY ["/args.exe","/foo bar/"] COPY ["/args.exe","/foo.exe"] COPY ["/args.exe","/"] RUN (Get-Command pwsh.exe).Source RUN & 'c:\\foo bar\\args.exe' 'foo bar' 'baz buzz' # Just like the above, but with linux style path separators RUN & 'c:/foo bar/args.exe' 'foo bar' 'baz buzz' # Another example that showcases why escaping is difficult on Windows # A combination of both windows and linux path separators RUN & 'c:/foo bar\\args.exe' 'foo bar' 'baz buzz' # And another one that omits the drive letter RUN & '/foo bar\\args.exe' 'foo bar' 'baz buzz' RUN C:/args.exe 'foo bar' 'baz buzz' # RUN ls CMD ["c:\\foo bar\\args.exe", "foo bar", "baz buzz"]

gabriel-samfira · 2024-05-29T13:57:28Z

frontend/dockerfile/dockerfile2llb/convert.go

 	d.image.Config.Cmd = args
-	d.image.Config.ArgsEscaped = true //nolint:staticcheck // ignore SA1019: field is deprecated in OCI Image spec, but used for backward-compatibility with Docker image spec.


The ArgsEscaped field was added in containerd a while ago and it essentially converts the behavior of the Cmd field into what CmdLine does (roughly). If ArgsEscaped is set to true, the shim expects that ENTRYPOINT or CMD is a single element array, with the command, along with any other arguments, already escaped.

For example, if we have ArgsEscaped set to true, by the time the CMD reaches the shim, it should look like this:

CMD ["\"c:\\Path to\\Some\\application.exe\" "some space delimited arg" someOtherArg"]

And that gets passed along to HcsCreateProcess. I remember having to deal with double escaping of args when I first implemented the windows executor. Back then we were sending spec.Cmd along with args. And the args were getting escaped again once they got to the shim. Details are fuzzy, but the consensus when I discussed this issue with the MSFT folks was to just send the one string as CmdLine instead of Cmd and Args.

I think this change is okay, as long as we just log the inconsistency in place of that TODO, similarly to what happens in moby now.

tianon-sso · 2024-05-29T17:13:27Z

Ahh, I also had a foo folder, that's why it didn't pick up the foo.exe file that was also present there. When just foo.exe is present, then yes, the issue manifests, but to disambiguate, we either have to escape the command itself, or use something like:
CMD ["\"c:\\foo bar\\args.exe\"", "foo bar", "baz buzz"]
Which I admit is atrocious user experience. This was an edge case that I sadly did not think about 😞.

But for this case to be solved, I believe it's enough to escape just the first element of CMD.

The whole point of the JSON syntax in the first place is to ask the runtime to properly escape the arguments for you. 😅

(I realize that's a bit at odds with how Windows works, but it is the existing convention, also honored by the "classic" builder correctly)

gabriel-samfira · 2024-05-29T18:20:49Z

Ahh, I also had a foo folder, that's why it didn't pick up the foo.exe file that was also present there. When just foo.exe is present, then yes, the issue manifests, but to disambiguate, we either have to escape the command itself, or use something like:
CMD ["\"c:\\foo bar\\args.exe\"", "foo bar", "baz buzz"]
Which I admit is atrocious user experience. This was an edge case that I sadly did not think about 😞.
But for this case to be solved, I believe it's enough to escape just the first element of CMD.
The whole point of the JSON syntax in the first place is to ask the runtime to properly escape the arguments for you. 😅

(I realize that's a bit at odds with how Windows works, but it is the existing convention, also honored by the "classic" builder correctly)

Yes, Of course. Ignore that comment. There is no way that is a desirable resolution. I need to use my inner voice for some things.

FWIW, the changes you made to handle ArgsEscaped, seem to fix the CMD/ENTRYPOINT issue. If you add those warning messages in place of the TODOs, I think it's fine. The executor changes seem to break the default shell.

I think we can do a better job when parsing the RUN stanza and potentially find a better way to run those RUN commands on Windows, but it may include a more complex change than just escaping arguments, and may be better suited for a separate PR. It would help if we tracked the not so friendly UX you pointed out, in a separate issue.

Edit: Currently the default shell on Windows is defined as:

[]string{"cmd", "/S", "/C"}

The /S character here is important, as it allows us to encase any string in double quotes and that string will then be run as a single command, as is, without change. The only action cmd.exe takes is to remove the leading and ending quote, the rest remains unchanged. This is why:

RUN ""c:\\foo bar\\args.exe" "foo bar" "baz buzz" hello"

works, although it looks ugly. With the /S switch, this becomes "c:\\foo bar\\args.exe" "foo bar" "baz buzz" hello and then it's run.

thompson-shaun · 2024-06-28T13:21:19Z

@gabriel-samfira did we still want the requested changes made or is the PR good as is?

gabriel-samfira · 2024-06-28T13:33:25Z

@thompson-shaun

This PR doesn't really fix the issue. A proper fix will need to touch llb.Meta and propagate the ArgsEscaped bool all the way into the executor.

When we get an imperative/JSON form of the RUN stanza, we need to pass the args as spec.Args when in JSON form and as spec.CommandLine when we have a non JSON form of the RUN stanza.

Otherwise we fix one case but break the other.

@profnandaa Would you mind having a look at this? I am currently on PTO.

profnandaa · 2024-10-14T06:46:46Z

Sorry for my delay, scheduled to look into this, this week.

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. 😅 See the PR for several references to related discussions/code in Moby. 🚀 Signed-off-by: Tianon Gravi <[email protected]>

tianon · 2024-11-12T05:14:50Z

Rebased (especially to fix the conflict caused by 89ce746 / #4913)

I guess now the "warning" could be added to the linter apparatus? I believe that's all that's missing here, but I don't know (and I need someone else to carry it over the line or at the very least tell me how to surface the warning in an acceptable way).

profnandaa · 2024-11-13T06:52:26Z

@tianon -- currently on it, sorry took long to TAL. BTW, curious, why didn't you do this PR from your tianon Github account?

profnandaa · 2024-11-13T07:57:28Z

Just reporting that I can repro the issue. I have these two images one built with docker build and another with buildctl:

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

ENV DEST="foo bar"

RUN mkdir "foo bar"
COPY ./main.exe ./foo.exe
COPY ./main.exe ./$DEST/args.exe

RUN dir

CMD ["C:\\foo bar\\args.exe", "foo bar", "baz buzz"]

Build with buidctl and load to docker (make sure is in the CMD shell not PS):

buildctl build --frontend dockerfile.v0 --local context=. --local dockerfile=. --output type=docker,name=<image-name>:<tag> | docker load

CMD> docker run args-escaped
[]string{"C:\\foo bar\\args.exe", "foo bar", "baz buzz"}

CMD> docker run args-escaped-bk
[]string{"C:\\foo", "bar\\args.exe", "foo bar", "baz buzz"}

However, with your changes, I still get the regression:

CMD> buildctl build --frontend dockerfile.v0 --local context=. --local dockerfile=. --output type=docker,name=args-escaped-bk-fixed --progress plain | docker load

CMD> docker run args-escaped-bk-fixed
[]string{"C:\\foo", "bar\\args.exe", "foo bar", "baz buzz"}

Am I missing something?

tonistiigi requested review from gabriel-samfira, profnandaa and TBBle March 1, 2024 17:12

tianon mentioned this pull request Mar 1, 2024

Add ArgsEscaped field to image config opencontainers/image-spec#892

Merged

profnandaa reviewed Mar 4, 2024

View reviewed changes

crazy-max added the area/windows label Mar 4, 2024

crazy-max mentioned this pull request Mar 20, 2024

WCOW: Build context with nested directories fails with "unclean path" #4741

Closed

thompson-shaun added area/windows-lcow area/windows-wcow and removed area/windows-lcow labels May 9, 2024

gabriel-samfira requested changes May 29, 2024

View reviewed changes

thompson-shaun added kind/bug accepted labels Jun 6, 2024

thompson-shaun added this to the v0.future milestone Jun 6, 2024

thompson-shaun modified the milestones: v0.future, v0.15.0 Jun 13, 2024

thompson-shaun modified the milestones: v0.15.0, v0.16.0 Jul 1, 2024

thompson-shaun modified the milestones: v0.16.0, v0.future Aug 8, 2024

tianon mentioned this pull request Nov 12, 2024

Apply BuildKit patch to remove ArgsEscaped from built images tianon/dockerfiles#846

Merged

tianon force-pushed the argsescaped branch from 45121e0 to 5454573 Compare November 12, 2024 05:12

github-actions bot added area/dockerfile area/frontend area/executor labels Nov 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Windows args and `ArgsEscaped` handling #4723

Fix Windows args and `ArgsEscaped` handling #4723

tianon-sso commented Mar 1, 2024

tianon-sso commented Mar 1, 2024

profnandaa commented Mar 1, 2024

profnandaa left a comment

profnandaa Mar 4, 2024

tianon-sso Mar 4, 2024

gabriel-samfira May 29, 2024

profnandaa Mar 4, 2024

tianon-sso Mar 4, 2024

profnandaa Mar 4, 2024

tianon-sso Mar 4, 2024

profnandaa commented Mar 4, 2024

crazy-max commented Mar 4, 2024 •

edited

Loading

tianon-sso commented Mar 4, 2024

tianon-sso commented Mar 4, 2024

tonistiigi commented May 28, 2024

gabriel-samfira commented May 29, 2024

gabriel-samfira commented May 29, 2024

gabriel-samfira commented May 29, 2024

gabriel-samfira commented May 29, 2024

gabriel-samfira left a comment

gabriel-samfira May 29, 2024

gabriel-samfira May 29, 2024

tianon-sso commented May 29, 2024

gabriel-samfira commented May 29, 2024 •

edited

Loading

thompson-shaun commented Jun 28, 2024

gabriel-samfira commented Jun 28, 2024 •

edited

Loading

profnandaa commented Oct 14, 2024

tianon commented Nov 12, 2024

profnandaa commented Nov 13, 2024

profnandaa commented Nov 13, 2024 •

edited

Loading

		d.image.Config.Cmd = args
		d.image.Config.ArgsEscaped = true //nolint:staticcheck // ignore SA1019: field is deprecated in OCI Image spec, but used for backward-compatibility with Docker image spec.

Fix Windows args and ArgsEscaped handling #4723

Are you sure you want to change the base?

Fix Windows args and ArgsEscaped handling #4723

Conversation

tianon-sso commented Mar 1, 2024

tianon-sso commented Mar 1, 2024

profnandaa commented Mar 1, 2024

profnandaa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

profnandaa commented Mar 4, 2024

Some notes from testing

crazy-max commented Mar 4, 2024 • edited Loading

tianon-sso commented Mar 4, 2024

tianon-sso commented Mar 4, 2024

tonistiigi commented May 28, 2024

gabriel-samfira commented May 29, 2024

gabriel-samfira commented May 29, 2024

gabriel-samfira commented May 29, 2024

gabriel-samfira commented May 29, 2024

gabriel-samfira left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tianon-sso commented May 29, 2024

gabriel-samfira commented May 29, 2024 • edited Loading

thompson-shaun commented Jun 28, 2024

gabriel-samfira commented Jun 28, 2024 • edited Loading

profnandaa commented Oct 14, 2024

tianon commented Nov 12, 2024

profnandaa commented Nov 13, 2024

profnandaa commented Nov 13, 2024 • edited Loading

Fix Windows args and `ArgsEscaped` handling #4723

Fix Windows args and `ArgsEscaped` handling #4723

crazy-max commented Mar 4, 2024 •

edited

Loading

gabriel-samfira commented May 29, 2024 •

edited

Loading

gabriel-samfira commented Jun 28, 2024 •

edited

Loading

profnandaa commented Nov 13, 2024 •

edited

Loading