From 07893acbd2bd0106fd518ea862eeae137c7e02ab Mon Sep 17 00:00:00 2001 From: Daniel Canter Date: Fri, 22 Oct 2021 15:11:15 -0700 Subject: [PATCH 1/2] Fix commandline double quoting for job containers We already escape the arguments passed to us by Containerd to form a Windows style commandline, however the commandline was being split back into arguments and then passed to exec.Cmd from the go stdlib. exec.Cmd internally also does escaping, which ended up applying some extra quotes in some cases where the commandline had double/single quotes present. This change just passes the commandline as is to the Cmdline field on the Windows syscall.SysProcAttr. Go takes this field as is and doesn't do any further processing on it which is the behavior we desire. Signed-off-by: Daniel Canter (cherry picked from commit 7cb95b67220468546e50086a05d0d46d1ed48640) Signed-off-by: Daniel Canter --- internal/jobcontainers/jobcontainer.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/jobcontainers/jobcontainer.go b/internal/jobcontainers/jobcontainer.go index 504a254a39..f8b2c6d56b 100644 --- a/internal/jobcontainers/jobcontainer.go +++ b/internal/jobcontainers/jobcontainer.go @@ -253,12 +253,12 @@ func (c *JobContainer) CreateProcess(ctx context.Context, config interface{}) (_ Env: env, Dir: workDir, Path: absPath, - Args: splitArgs(commandLine), SysProcAttr: &syscall.SysProcAttr{ // CREATE_BREAKAWAY_FROM_JOB to make sure that we're not inheriting the job object (and by extension its limits) // from whatever process is going to launch the container. CreationFlags: windows.CREATE_NEW_PROCESS_GROUP | windows.CREATE_BREAKAWAY_FROM_JOB, Token: syscall.Token(token), + CmdLine: commandLine, }, } process := newProcess(cmd) From 92022f0e26b8bbcc08c01f676ce334db92e6d453 Mon Sep 17 00:00:00 2001 From: Daniel Canter Date: Mon, 25 Oct 2021 09:46:13 -0700 Subject: [PATCH 2/2] Add test for job container cmdline quoting behavior This change adds a test to verify that commandlines with quotes don't get additional quotes added on to them when combining the arguments given. Signed-off-by: Daniel Canter (cherry picked from commit bc5e9146833ced319652576721db0f052ce4c882) Signed-off-by: Daniel Canter --- test/cri-containerd/jobcontainer_test.go | 40 +++++++++++++++++++ test/cri-containerd/main.go | 2 + .../jobcontainer_cmdline/dockerfile | 11 +++++ .../test-images/jobcontainer_cmdline/main.go | 11 +++++ 4 files changed, 64 insertions(+) create mode 100644 test/cri-containerd/test-images/jobcontainer_cmdline/dockerfile create mode 100644 test/cri-containerd/test-images/jobcontainer_cmdline/main.go diff --git a/test/cri-containerd/jobcontainer_test.go b/test/cri-containerd/jobcontainer_test.go index 1113e4bae8..7468a725c6 100644 --- a/test/cri-containerd/jobcontainer_test.go +++ b/test/cri-containerd/jobcontainer_test.go @@ -1,3 +1,4 @@ +//go:build functional // +build functional package cri_containerd @@ -524,3 +525,42 @@ func Test_RunContainer_WorkingDirectory_JobContainer_WCOW(t *testing.T) { }) } } + +// Test of the fix for the behavior detailed here https://github.com/microsoft/hcsshim/issues/1199 +// The underlying issue was that we would escape the args passed to us to form a commandline, and then split the commandline back into args +// to pass to exec.Cmd in the stdlib. exec.Cmd internally does escaping of its own and thus would result in double quoting for certain +// commandlines. +func Test_DoubleQuoting_JobContainer_WCOW(t *testing.T) { + requireFeatures(t, featureWCOWProcess, featureHostProcess) + + pullRequiredImages(t, []string{imageJobContainerCmdline}) + client := newTestRuntimeClient(t) + + podctx := context.Background() + sandboxRequest := getJobContainerPodRequestWCOW(t) + + podID := runPodSandbox(t, client, podctx, sandboxRequest) + defer removePodSandbox(t, client, podctx, podID) + defer stopPodSandbox(t, client, podctx, podID) + + containerRequest := getJobContainerRequestWCOW(t, podID, sandboxRequest.Config, imageJobContainerCmdline, nil) + ctx, cancel := context.WithTimeout(context.Background(), 20*time.Second) + defer cancel() + + containerID := createContainer(t, client, ctx, containerRequest) + defer removeContainer(t, client, ctx, containerID) + startContainer(t, client, ctx, containerID) + defer stopContainer(t, client, ctx, containerID) + + execResponse := execSync(t, client, ctx, &runtime.ExecSyncRequest{ + ContainerId: containerID, + Cmd: []string{"cmdline.exe", `"quote test"`}, + }) + + expected := `cmdline.exe "quote test"` + // Check that there's no double quoting going on. + // e.g. `cmdline.exe ""quote test"" ` + if string(execResponse.Stdout) != expected { + t.Fatalf("expected cmdline for exec to be %q but got %q", expected, string(execResponse.Stdout)) + } +} diff --git a/test/cri-containerd/main.go b/test/cri-containerd/main.go index 6531efe121..ba67e7aa08 100644 --- a/test/cri-containerd/main.go +++ b/test/cri-containerd/main.go @@ -1,3 +1,4 @@ +//go:build functional // +build functional package cri_containerd @@ -55,6 +56,7 @@ const ( imageJobContainerHNS = "cplatpublic.azurecr.io/jobcontainer_hns:latest" imageJobContainerETW = "cplatpublic.azurecr.io/jobcontainer_etw:latest" imageJobContainerVHD = "cplatpublic.azurecr.io/jobcontainer_vhd:latest" + imageJobContainerCmdline = "cplatpublic.azurecr.io/jobcontainer_cmdline:latest" imageJobContainerWorkDir = "cplatpublic.azurecr.io/jobcontainer_workdir:latest" alpineAspNet = "mcr.microsoft.com/dotnet/core/aspnet:3.1-alpine3.11" alpineAspnetUpgrade = "mcr.microsoft.com/dotnet/core/aspnet:3.1.2-alpine3.11" diff --git a/test/cri-containerd/test-images/jobcontainer_cmdline/dockerfile b/test/cri-containerd/test-images/jobcontainer_cmdline/dockerfile new file mode 100644 index 0000000000..c3bfda382c --- /dev/null +++ b/test/cri-containerd/test-images/jobcontainer_cmdline/dockerfile @@ -0,0 +1,11 @@ +# Irrelevant what image version we use for job containers as there's no container <-> host OS version restraint. +FROM golang:1.15.10-nanoserver-1809 + +# Get administrator privileges +USER containeradministrator + +WORKDIR C:\\go\\src\\cmdline +COPY main.go . + +RUN go get -d -v ./... +RUN go build -mod=mod \ No newline at end of file diff --git a/test/cri-containerd/test-images/jobcontainer_cmdline/main.go b/test/cri-containerd/test-images/jobcontainer_cmdline/main.go new file mode 100644 index 0000000000..08ed020c0e --- /dev/null +++ b/test/cri-containerd/test-images/jobcontainer_cmdline/main.go @@ -0,0 +1,11 @@ +package main + +import ( + "fmt" + "os" + "strings" +) + +func main() { + fmt.Print(strings.Join(os.Args, " ")) +}