Skip to content

Commit 20833b0

Browse files
author
John Howard
committed
Windows: (WCOW) Generate OCI spec that remote runtime can escape
Signed-off-by: John Howard <[email protected]> Also fixes #22874 This commit is a pre-requisite to moving moby/moby on Windows to using Containerd for its runtime. The reason for this is that the interface between moby and containerd for the runtime is an OCI spec which must be unambigious. It is the responsibility of the runtime (runhcs in the case of containerd on Windows) to ensure that arguments are escaped prior to calling into HCS and onwards to the Win32 CreateProcess call. Previously, the builder was always escaping arguments which has led to several bugs in moby. Because the local runtime in libcontainerd had context of whether or not arguments were escaped, it was possible to hack around in daemon/oci_windows.go with knowledge of the context of the call (from builder or not). With a remote runtime, this is not possible as there's rightly no context of the caller passed across in the OCI spec. Put another way, as I put above, the OCI spec must be unambigious. The other previous limitation (which leads to various subtle bugs) is that moby is coded entirely from a Linux-centric point of view. Unfortunately, Windows != Linux. Windows CreateProcess uses a command line, not an array of arguments. And it has very specific rules about how to escape a command line. Some interesting reading links about this are: https://blogs.msdn.microsoft.com/twistylittlepassagesallalike/2011/04/23/everyone-quotes-command-line-arguments-the-wrong-way/ https://stackoverflow.com/questions/31838469/how-do-i-convert-argv-to-lpcommandline-parameter-of-createprocess https://docs.microsoft.com/en-us/cpp/cpp/parsing-cpp-command-line-arguments?view=vs-2017 For this reason, the OCI spec has recently been updated to cater for more natural syntax by including a CommandLine option in Process. What does this commit do? Primary objective is to ensure that the built OCI spec is unambigious. It changes the builder so that `ArgsEscaped` as commited in a layer is only controlled by the use of CMD or ENTRYPOINT. Subsequently, when calling in to create a container from the builder, if follows a different path to both `docker run` and `docker create` using the added `ContainerCreateIgnoreImagesArgsEscaped`. This allows a RUN from the builder to control how to escape in the OCI spec. It changes the builder so that when shell form is used for RUN, CMD or ENTRYPOINT, it builds (for WCOW) a more natural command line using the original as put by the user in the dockerfile, not the parsed version as a set of args which loses fidelity. This command line is put into args[0] and `ArgsEscaped` is set to true for CMD or ENTRYPOINT. A RUN statement does not commit `ArgsEscaped` to the commited layer regardless or whether shell or exec form were used.
1 parent 85ad4b1 commit 20833b0

18 files changed

Lines changed: 276 additions & 93 deletions

‎api/types/container/config.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,7 @@ type Config struct {
5454
Env []string // List of environment variable to set in the container
5555
Cmd strslice.StrSlice // Command to run when starting the container
5656
Healthcheck *HealthConfig `json:",omitempty"` // Healthcheck describes how to check the container is healthy
57-
ArgsEscaped bool `json:",omitempty"` // True if command is already escaped (Windows specific)
57+
ArgsEscaped bool `json:",omitempty"` // True if command is already escaped (meaning treat as a command line) (Windows specific).
5858
Image string // Name of the image as it was passed by the operator (e.g. could be symbolic)
5959
Volumes map[string]struct{} // List of volumes (mounts) used for the container
6060
WorkingDir string // Current directory (PWD) in the command will be launched

‎builder/builder.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -60,8 +60,8 @@ type ImageBackend interface {
6060
type ExecBackend interface {
6161
// ContainerAttachRaw attaches to container.
6262
ContainerAttachRaw(cID string, stdin io.ReadCloser, stdout, stderr io.Writer, stream bool, attached chan struct{}) error
63-
// ContainerCreate creates a new Docker container and returns potential warnings
64-
ContainerCreate(config types.ContainerCreateConfig) (container.ContainerCreateCreatedBody, error)
63+
// ContainerCreateIgnoreImagesArgsEscaped creates a new Docker container and returns potential warnings
64+
ContainerCreateIgnoreImagesArgsEscaped(config types.ContainerCreateConfig) (container.ContainerCreateCreatedBody, error)
6565
// ContainerRm removes a container specified by `id`.
6666
ContainerRm(name string, config *types.ContainerRmConfig) error
6767
// ContainerKill stops the container execution abruptly.

‎builder/dockerfile/containerbackend.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@ func newContainerManager(docker builder.ExecBackend) *containerManager {
2929

3030
// Create a container
3131
func (c *containerManager) Create(runConfig *container.Config, hostConfig *container.HostConfig) (container.ContainerCreateCreatedBody, error) {
32-
container, err := c.backend.ContainerCreate(types.ContainerCreateConfig{
32+
container, err := c.backend.ContainerCreateIgnoreImagesArgsEscaped(types.ContainerCreateConfig{
3333
Config: runConfig,
3434
HostConfig: hostConfig,
3535
})

‎builder/dockerfile/dispatchers.go‎

Lines changed: 36 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,6 @@ import (
1616

1717
"github.com/containerd/containerd/platforms"
1818
"github.com/docker/docker/api"
19-
"github.com/docker/docker/api/types/container"
2019
"github.com/docker/docker/api/types/strslice"
2120
"github.com/docker/docker/builder"
2221
"github.com/docker/docker/errdefs"
@@ -330,14 +329,6 @@ func dispatchWorkdir(d dispatchRequest, c *instructions.WorkdirCommand) error {
330329
return d.builder.commitContainer(d.state, containerID, runConfigWithCommentCmd)
331330
}
332331

333-
func resolveCmdLine(cmd instructions.ShellDependantCmdLine, runConfig *container.Config, os string) []string {
334-
result := cmd.CmdLine
335-
if cmd.PrependShell && result != nil {
336-
result = append(getShell(runConfig, os), result...)
337-
}
338-
return result
339-
}
340-
341332
// RUN some command yo
342333
//
343334
// run a command and commit the image. Args are automatically prepended with
@@ -353,7 +344,7 @@ func dispatchRun(d dispatchRequest, c *instructions.RunCommand) error {
353344
return system.ErrNotSupportedOperatingSystem
354345
}
355346
stateRunConfig := d.state.runConfig
356-
cmdFromArgs := resolveCmdLine(c.ShellDependantCmdLine, stateRunConfig, d.state.operatingSystem)
347+
cmdFromArgs, argsEscaped := resolveCmdLine(c.ShellDependantCmdLine, stateRunConfig, d.state.operatingSystem, c.Name(), c.String())
357348
buildArgs := d.state.buildArgs.FilterAllowed(stateRunConfig.Env)
358349

359350
saveCmd := cmdFromArgs
@@ -363,20 +354,19 @@ func dispatchRun(d dispatchRequest, c *instructions.RunCommand) error {
363354

364355
runConfigForCacheProbe := copyRunConfig(stateRunConfig,
365356
withCmd(saveCmd),
357+
withArgsEscaped(argsEscaped),
366358
withEntrypointOverride(saveCmd, nil))
367359
if hit, err := d.builder.probeCache(d.state, runConfigForCacheProbe); err != nil || hit {
368360
return err
369361
}
370362

371363
runConfig := copyRunConfig(stateRunConfig,
372364
withCmd(cmdFromArgs),
365+
withArgsEscaped(argsEscaped),
373366
withEnv(append(stateRunConfig.Env, buildArgs...)),
374367
withEntrypointOverride(saveCmd, strslice.StrSlice{""}),
375368
withoutHealthcheck())
376369

377-
// set config as already being escaped, this prevents double escaping on windows
378-
runConfig.ArgsEscaped = true
379-
380370
cID, err := d.builder.create(runConfig)
381371
if err != nil {
382372
return err
@@ -399,6 +389,12 @@ func dispatchRun(d dispatchRequest, c *instructions.RunCommand) error {
399389
return err
400390
}
401391

392+
// Don't persist the argsEscaped value in the committed image. Use the original
393+
// from previous build steps (only CMD and ENTRYPOINT persist this).
394+
if d.state.operatingSystem == "windows" {
395+
runConfigForCacheProbe.ArgsEscaped = stateRunConfig.ArgsEscaped
396+
}
397+
402398
return d.builder.commitContainer(d.state, cID, runConfigForCacheProbe)
403399
}
404400

@@ -434,15 +430,23 @@ func prependEnvOnCmd(buildArgs *BuildArgs, buildArgVars []string, cmd strslice.S
434430
//
435431
func dispatchCmd(d dispatchRequest, c *instructions.CmdCommand) error {
436432
runConfig := d.state.runConfig
437-
cmd := resolveCmdLine(c.ShellDependantCmdLine, runConfig, d.state.operatingSystem)
433+
cmd, argsEscaped := resolveCmdLine(c.ShellDependantCmdLine, runConfig, d.state.operatingSystem, c.Name(), c.String())
434+
435+
// We warn here as Windows shell processing operates differently to Linux.
436+
// Linux: /bin/sh -c "echo hello" world --> hello
437+
// Windows: cmd /s /c "echo hello" world --> hello world
438+
if d.state.operatingSystem == "windows" &&
439+
len(runConfig.Entrypoint) > 0 &&
440+
d.state.runConfig.ArgsEscaped != argsEscaped {
441+
fmt.Fprintf(d.builder.Stderr, " ---> [Warning] Shell-form ENTRYPOINT and exec-form CMD may have unexpected results\n")
442+
}
443+
438444
runConfig.Cmd = cmd
439-
// set config as already being escaped, this prevents double escaping on windows
440-
runConfig.ArgsEscaped = true
445+
runConfig.ArgsEscaped = argsEscaped
441446

442447
if err := d.builder.commit(d.state, fmt.Sprintf("CMD %q", cmd)); err != nil {
443448
return err
444449
}
445-
446450
if len(c.ShellDependantCmdLine.CmdLine) != 0 {
447451
d.state.cmdSet = true
448452
}
@@ -477,8 +481,22 @@ func dispatchHealthcheck(d dispatchRequest, c *instructions.HealthCheckCommand)
477481
//
478482
func dispatchEntrypoint(d dispatchRequest, c *instructions.EntrypointCommand) error {
479483
runConfig := d.state.runConfig
480-
cmd := resolveCmdLine(c.ShellDependantCmdLine, runConfig, d.state.operatingSystem)
484+
cmd, argsEscaped := resolveCmdLine(c.ShellDependantCmdLine, runConfig, d.state.operatingSystem, c.Name(), c.String())
485+
486+
// This warning is a little more complex than in dispatchCmd(), as the Windows base images (similar
487+
// universally to almost every Linux image out there) have a single .Cmd field populated so that
488+
// `docker run --rm image` starts the default shell which would typically be sh on Linux,
489+
// or cmd on Windows. The catch to this is that if a dockerfile had `CMD ["c:\\windows\\system32\\cmd.exe"]`,
490+
// we wouldn't be able to tell the difference. However, that would be highly unlikely, and besides, this
491+
// is only trying to give a helpful warning of possibly unexpected results.
492+
if d.state.operatingSystem == "windows" &&
493+
d.state.runConfig.ArgsEscaped != argsEscaped &&
494+
((len(runConfig.Cmd) == 1 && strings.ToLower(runConfig.Cmd[0]) != `c:\windows\system32\cmd.exe` && len(runConfig.Shell) == 0) || (len(runConfig.Cmd) > 1)) {
495+
fmt.Fprintf(d.builder.Stderr, " ---> [Warning] Shell-form CMD and exec-form ENTRYPOINT may have unexpected results\n")
496+
}
497+
481498
runConfig.Entrypoint = cmd
499+
runConfig.ArgsEscaped = argsEscaped
482500
if !d.state.cmdSet {
483501
runConfig.Cmd = nil
484502
}

‎builder/dockerfile/dispatchers_test.go‎

Lines changed: 28 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import (
44
"bytes"
55
"context"
66
"runtime"
7+
"strings"
78
"testing"
89

910
"github.com/docker/docker/api/types"
@@ -15,6 +16,7 @@ import (
1516
"github.com/docker/docker/pkg/system"
1617
"github.com/docker/go-connections/nat"
1718
"github.com/moby/buildkit/frontend/dockerfile/instructions"
19+
"github.com/moby/buildkit/frontend/dockerfile/parser"
1820
"github.com/moby/buildkit/frontend/dockerfile/shell"
1921
"gotest.tools/assert"
2022
is "gotest.tools/assert/cmp"
@@ -436,7 +438,14 @@ func TestRunWithBuildArgs(t *testing.T) {
436438

437439
runConfig := &container.Config{}
438440
origCmd := strslice.StrSlice([]string{"cmd", "in", "from", "image"})
439-
cmdWithShell := strslice.StrSlice(append(getShell(runConfig, runtime.GOOS), "echo foo"))
441+
442+
var cmdWithShell strslice.StrSlice
443+
if runtime.GOOS == "windows" {
444+
cmdWithShell = strslice.StrSlice([]string{strings.Join(append(getShell(runConfig, runtime.GOOS), []string{"echo foo"}...), " ")})
445+
} else {
446+
cmdWithShell = strslice.StrSlice(append(getShell(runConfig, runtime.GOOS), "echo foo"))
447+
}
448+
440449
envVars := []string{"|1", "one=two"}
441450
cachedCmd := strslice.StrSlice(append(envVars, cmdWithShell...))
442451

@@ -478,13 +487,24 @@ func TestRunWithBuildArgs(t *testing.T) {
478487
err := initializeStage(sb, from)
479488
assert.NilError(t, err)
480489
sb.state.buildArgs.AddArg("one", strPtr("two"))
481-
run := &instructions.RunCommand{
482-
ShellDependantCmdLine: instructions.ShellDependantCmdLine{
483-
CmdLine: strslice.StrSlice{"echo foo"},
484-
PrependShell: true,
485-
},
486-
}
487-
assert.NilError(t, dispatch(sb, run))
490+
491+
// This is hugely annoying. On the Windows side, it relies on the
492+
// RunCommand being able to emit String() and Name() (as implemented by
493+
// withNameAndCode). Unfortunately, that is internal, and no way to directly
494+
// set. However, we can fortunately use ParseInstruction in the instructions
495+
// package to parse a fake node which can be used as our instructions.RunCommand
496+
// instead.
497+
node := &parser.Node{
498+
Original: `RUN echo foo`,
499+
Value: "run",
500+
}
501+
runint, err := instructions.ParseInstruction(node)
502+
assert.NilError(t, err)
503+
runinst := runint.(*instructions.RunCommand)
504+
runinst.CmdLine = strslice.StrSlice{"echo foo"}
505+
runinst.PrependShell = true
506+
507+
assert.NilError(t, dispatch(sb, runinst))
488508

489509
// Check that runConfig.Cmd has not been modified by run
490510
assert.Check(t, is.DeepEqual(origCmd, sb.state.runConfig.Cmd))

‎builder/dockerfile/dispatchers_unix.go‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,9 @@ import (
66
"errors"
77
"os"
88
"path/filepath"
9+
10+
"github.com/docker/docker/api/types/container"
11+
"github.com/moby/buildkit/frontend/dockerfile/instructions"
912
)
1013

1114
// normalizeWorkdir normalizes a user requested working directory in a
@@ -21,3 +24,13 @@ func normalizeWorkdir(_ string, current string, requested string) (string, error
2124
}
2225
return requested, nil
2326
}
27+
28+
// resolveCmdLine takes a command line arg set and optionally prepends a platform-specific
29+
// shell in front of it.
30+
func resolveCmdLine(cmd instructions.ShellDependantCmdLine, runConfig *container.Config, os, _, _ string) ([]string, bool) {
31+
result := cmd.CmdLine
32+
if cmd.PrependShell && result != nil {
33+
result = append(getShell(runConfig, os), result...)
34+
}
35+
return result, false
36+
}

‎builder/dockerfile/dispatchers_windows.go‎

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,9 @@ import (
99
"regexp"
1010
"strings"
1111

12+
"github.com/docker/docker/api/types/container"
1213
"github.com/docker/docker/pkg/system"
14+
"github.com/moby/buildkit/frontend/dockerfile/instructions"
1315
)
1416

1517
var pattern = regexp.MustCompile(`^[a-zA-Z]:\.$`)
@@ -93,3 +95,47 @@ func normalizeWorkdirWindows(current string, requested string) (string, error) {
9395
// Upper-case drive letter
9496
return (strings.ToUpper(string(requested[0])) + requested[1:]), nil
9597
}
98+
99+
// resolveCmdLine takes a command line arg set and optionally prepends a platform-specific
100+
// shell in front of it. It returns either an array of arguments and an indication that
101+
// the arguments are not yet escaped; Or, an array containing a single command line element
102+
// along with an indication that the arguments are escaped so the runtime shouldn't escape.
103+
//
104+
// A better solution could be made, but it would be exceptionally invasive throughout
105+
// many parts of the daemon which are coded assuming Linux args array only only, not taking
106+
// account of Windows-natural command line semantics and it's argv handling. Put another way,
107+
// while what is here is good-enough, it could be improved, but would be highly invasive.
108+
//
109+
// The commands when this function is called are RUN, ENTRYPOINT and CMD.
110+
func resolveCmdLine(cmd instructions.ShellDependantCmdLine, runConfig *container.Config, os, command, original string) ([]string, bool) {
111+
112+
// Make sure we return an empty array if there is no cmd.CmdLine
113+
if len(cmd.CmdLine) == 0 {
114+
return []string{}, runConfig.ArgsEscaped
115+
}
116+
117+
if os == "windows" { // ie WCOW
118+
if cmd.PrependShell {
119+
// WCOW shell-form. Return a single-element array containing the original command line prepended with the shell.
120+
// Also indicate that it has not been escaped (so will be passed through directly to HCS). Note that
121+
// we go back to the original un-parsed command line in the dockerfile line, strip off both the command part of
122+
// it (RUN/ENTRYPOINT/CMD), and also strip any leading white space. IOW, we deliberately ignore any prior parsing
123+
// so as to ensure it is treated exactly as a command line. For those interested, `RUN mkdir "c:/foo"` is a particularly
124+
// good example of why this is necessary if you fancy debugging how cmd.exe and its builtin mkdir works. (Windows
125+
// doesn't have a mkdir.exe, and I'm guessing cmd.exe has some very long unavoidable and unchangeable historical
126+
// design decisions over how both its built-in echo and mkdir are coded. Probably more too.)
127+
original = original[len(command):] // Strip off the command
128+
original = strings.TrimLeft(original, " \t\v\n") // Strip of leading whitespace
129+
return []string{strings.Join(getShell(runConfig, os), " ") + " " + original}, true
130+
}
131+
132+
// WCOW JSON/"exec" form.
133+
return cmd.CmdLine, false
134+
}
135+
136+
// LCOW - use args as an array, same as LCOL.
137+
if cmd.PrependShell && cmd.CmdLine != nil {
138+
return append(getShell(runConfig, os), cmd.CmdLine...), false
139+
}
140+
return cmd.CmdLine, false
141+
}

‎builder/dockerfile/internals.go‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -308,6 +308,12 @@ func withCmd(cmd []string) runConfigModifier {
308308
}
309309
}
310310

311+
func withArgsEscaped(argsEscaped bool) runConfigModifier {
312+
return func(runConfig *container.Config) {
313+
runConfig.ArgsEscaped = argsEscaped
314+
}
315+
}
316+
311317
// withCmdComment sets Cmd to a nop comment string. See withCmdCommentString for
312318
// why there are two almost identical versions of this.
313319
func withCmdComment(comment string, platform string) runConfigModifier {

‎builder/dockerfile/internals_windows.go‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -83,7 +83,6 @@ func lookupNTAccount(builder *Builder, accountName string, state *dispatchState)
8383
runConfig := copyRunConfig(state.runConfig,
8484
withCmdCommentString("internal run to obtain NT account information.", optionsPlatform.OS))
8585

86-
runConfig.ArgsEscaped = true
8786
runConfig.Cmd = []string{targetExecutable, "getaccountsid", accountName}
8887

8988
hostConfig := &container.HostConfig{Mounts: []mount.Mount{

‎builder/dockerfile/mockbackend_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@ func (m *MockBackend) ContainerAttachRaw(cID string, stdin io.ReadCloser, stdout
2828
return nil
2929
}
3030

31-
func (m *MockBackend) ContainerCreate(config types.ContainerCreateConfig) (container.ContainerCreateCreatedBody, error) {
31+
func (m *MockBackend) ContainerCreateIgnoreImagesArgsEscaped(config types.ContainerCreateConfig) (container.ContainerCreateCreatedBody, error) {
3232
if m.containerCreateFunc != nil {
3333
return m.containerCreateFunc(config)
3434
}

0 commit comments

Comments
 (0)