Fix Windows args and ArgsEscaped handling

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]>

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, " ")
 }

executor/oci/spec_windows.go

@@ -18,14 +18,22 @@ 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 (
 	tracingSocketPath = "//./pipe/otel-grpc"
 )
 
 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)

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