Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix Windows args and ArgsEscaped handling #4723

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+45 −3
Open

Fix Windows args and ArgsEscaped handling #4723

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion executor/containerdexecutor/executor_windows.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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, _ string) (specs.User, error) {
Expand Down Expand Up @@ -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)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// 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, " ")
}
10 changes: 9 additions & 1 deletion executor/oci/spec_windows.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
28 changes: 27 additions & 1 deletion frontend/dockerfile/dockerfile2llb/convert.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1662,7 +1662,16 @@ func dispatchCmd(d *dispatchState, c *instructions.CmdCommand, lint *linter.Lint
args = withShell(d.image, args)
}
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.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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.
}
return commitToHistory(&d.image, fmt.Sprintf("CMD %q", args), false, nil, d.epoch)
}

Expand All @@ -1677,6 +1686,23 @@ func dispatchEntrypoint(d *dispatchState, c *instructions.EntrypointCommand, lin
}
args = withShell(d.image, args)
}
if d.image.OS == "windows" {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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` &&
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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.cmd.IsSet {
d.image.Config.Cmd = nil
Expand Down
Loading