From 45121e09fdb9f6dbb19475e3c0f5a8faf88df0e4 Mon Sep 17 00:00:00 2001 From: Tianon Gravi Date: Thu, 29 Feb 2024 16:35:23 -0800 Subject: [PATCH] Fix Windows args and `ArgsEscaped` handling MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../containerdexecutor/executor_windows.go | 10 ++++++- executor/oci/spec_windows.go | 10 ++++++- frontend/dockerfile/dockerfile2llb/convert.go | 28 ++++++++++++++++++- 3 files changed, 45 insertions(+), 3 deletions(-) diff --git a/executor/containerdexecutor/executor_windows.go b/executor/containerdexecutor/executor_windows.go index 1148b03885a8..f54bf94ec507 100644 --- a/executor/containerdexecutor/executor_windows.go +++ b/executor/containerdexecutor/executor_windows.go @@ -17,6 +17,7 @@ import ( "github.com/moby/buildkit/util/windows" "github.com/opencontainers/runtime-spec/specs-go" "github.com/pkg/errors" + gowindows "golang.org/x/sys/windows" ) func getUserSpec(user, rootfsPath string) (specs.User, error) { @@ -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) + // if ArgsEscaped { spec.CommandLine = args[0] + " "; args = args[1:] } else { spec.CommandLine = "" } + // and then specs.CommandLine += down below + escaped := make([]string, len(args)) + for i, a := range args { + escaped[i] = gowindows.EscapeArg(a) + } + spec.CommandLine = strings.Join(escaped, " ") } diff --git a/executor/oci/spec_windows.go b/executor/oci/spec_windows.go index 6f1436fd7776..66aea12c7191 100644 --- a/executor/oci/spec_windows.go +++ b/executor/oci/spec_windows.go @@ -18,6 +18,7 @@ import ( "github.com/moby/buildkit/solver/pb" specs "github.com/opencontainers/runtime-spec/specs-go" "github.com/pkg/errors" + gowindows "golang.org/x/sys/windows" ) const ( @@ -25,7 +26,14 @@ const ( ) func withProcessArgs(args ...string) oci.SpecOpts { - cmdLine := strings.Join(args, " ") + // TODO handle ArgsEscaped correctly here somehow (ie, avoid re-escaping args[0] if it's true) + // if ArgsEscaped { spec.CommandLine = args[0] + " "; args = args[1:] } else { spec.CommandLine = "" } + // and then specs.CommandLine += down below + escaped := make([]string, len(args)) + for i, a := range args { + escaped[i] = gowindows.EscapeArg(a) + } + cmdLine := strings.Join(escaped, " ") // This will set Args to nil and properly set the CommandLine option // in the spec. On Windows we need to use CommandLine instead of Args. return oci.WithProcessCommandLine(cmdLine) diff --git a/frontend/dockerfile/dockerfile2llb/convert.go b/frontend/dockerfile/dockerfile2llb/convert.go index 531aa09484bd..42ac7f031e9d 100644 --- a/frontend/dockerfile/dockerfile2llb/convert.go +++ b/frontend/dockerfile/dockerfile2llb/convert.go @@ -1387,8 +1387,17 @@ func dispatchCmd(d *dispatchState, c *instructions.CmdCommand) error { if c.PrependShell { args = withShell(d.image, args) } + if d.image.OS == "windows" { + argsEscaped := c.PrependShell + // We warn here as Windows shell processing operates differently to Linux. + // Linux: /bin/sh -c "echo hello" world --> hello + // Windows: cmd /s /c "echo hello" world --> hello world + if d.image.Config.ArgsEscaped != argsEscaped && len(d.image.Config.Entrypoint) > 0 { + // TODO error or warning about mixing CMD and ENTRYPOINT having unexpected results: https://github.com/moby/moby/blob/b8aa8579cad7b9c8712327093cbc602cfb6b417f/builder/dockerfile/dispatchers.go#L443 + } + d.image.Config.ArgsEscaped = argsEscaped //nolint:staticcheck // ignore SA1019: field is deprecated in OCI Image spec, but used for backward-compatibility with Docker image spec. + } 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. d.cmdSet = true return commitToHistory(&d.image, fmt.Sprintf("CMD %q", args), false, nil, d.epoch) } @@ -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" { + argsEscaped := c.PrependShell + // This warning is a little more complex than in dispatchCmd(), as the Windows base images (similar + // universally to almost every Linux image out there) have a single .Cmd field populated so that + // `docker run --rm image` starts the default shell which would typically be sh on Linux, + // or cmd on Windows. The catch to this is that if a dockerfile had `CMD ["c:\\windows\\system32\\cmd.exe"]`, + // we wouldn't be able to tell the difference. However, that would be highly unlikely, and besides, this + // is only trying to give a helpful warning of possibly unexpected results. + 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` && + len(d.image.Config.Shell) == 0)) { + // TODO error or warning about mixing CMD and ENTRYPOINT having unexpected results: https://github.com/moby/moby/blob/b8aa8579cad7b9c8712327093cbc602cfb6b417f/builder/dockerfile/dispatchers.go#L495 + } + d.image.Config.ArgsEscaped = argsEscaped //nolint:staticcheck // ignore SA1019: field is deprecated in OCI Image spec, but used for backward-compatibility with Docker image spec. + } d.image.Config.Entrypoint = args if !d.cmdSet { d.image.Config.Cmd = nil