From 7a1ce51ccf39a7409713c2f8a75bec6e7958adc0 Mon Sep 17 00:00:00 2001 From: Daniel Canter Date: Thu, 22 Jul 2021 18:43:04 -0700 Subject: [PATCH] Fix relative paths (with dot) not working for job containers Relative paths supplying a dot don't make it through our commandline parsing for job containers, this change fixes that. In addition to this, this change adds logic to actually honor the working directory specified for the container. Signed-off-by: Daniel Canter --- internal/jobcontainers/jobcontainer.go | 26 +++++++++++++++----- internal/jobcontainers/path.go | 5 ++++ internal/jobcontainers/path_test.go | 33 ++++++++++++++++++++++++-- 3 files changed, 56 insertions(+), 8 deletions(-) diff --git a/internal/jobcontainers/jobcontainer.go b/internal/jobcontainers/jobcontainer.go index 191b475be9..f2d338e82f 100644 --- a/internal/jobcontainers/jobcontainer.go +++ b/internal/jobcontainers/jobcontainer.go @@ -174,14 +174,22 @@ func (c *JobContainer) CreateProcess(ctx context.Context, config interface{}) (_ return nil, errors.New("console emulation not supported for job containers") } - absPath, commandLine, err := getApplicationName(conf.CommandLine, c.sandboxMount, os.Getenv("PATH")) + // Replace any occurences of the sandbox mount point env variable in the commandline. + // %CONTAINER_SANDBOX_MOUNTPOINT%\mybinary.exe -> C:\C\123456789\mybinary.exe + commandLine := c.replaceWithMountPoint(conf.CommandLine) + + workDir := c.sandboxMount + if conf.WorkingDirectory != "" { + workDir = c.replaceWithMountPoint(conf.WorkingDirectory) + } + + // Reassign commandline here in case it needed to be quoted. For example if "foo bar baz" was supplied, and + // "foo bar.exe" exists, then return: "\"foo bar\" baz" + absPath, commandLine, err := getApplicationName(commandLine, workDir, os.Getenv("PATH")) if err != nil { return nil, errors.Wrapf(err, "failed to get application name from commandline %q", conf.CommandLine) } - commandLine = strings.ReplaceAll(commandLine, "%"+sandboxMountPointEnvVar+"%", c.sandboxMount) - commandLine = strings.ReplaceAll(commandLine, "$env:"+sandboxMountPointEnvVar, c.sandboxMount) - var token windows.Token if getUserTokenInheritAnnotation(c.spec.Annotations) { token, err = openCurrentProcessToken() @@ -205,12 +213,12 @@ func (c *JobContainer) CreateProcess(ctx context.Context, config interface{}) (_ cmd := &exec.Cmd{ Env: env, - Dir: c.sandboxMount, + 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 running this code. + // from whatever process is going to launch the container. CreationFlags: windows.CREATE_NEW_PROCESS_GROUP | windows.CREATE_BREAKAWAY_FROM_JOB, Token: syscall.Token(token), }, @@ -566,3 +574,9 @@ func systemProcessInformation() ([]*winapi.SYSTEM_PROCESS_INFORMATION, error) { return procInfos, nil } + +// Takes a string and replaces any occurences of CONTAINER_SANDBOX_MOUNT_POINT with where the containers volume is mounted. +func (c *JobContainer) replaceWithMountPoint(str string) string { + str = strings.ReplaceAll(str, "%"+sandboxMountPointEnvVar+"%", c.sandboxMount) + return strings.ReplaceAll(str, "$env:"+sandboxMountPointEnvVar, c.sandboxMount) +} diff --git a/internal/jobcontainers/path.go b/internal/jobcontainers/path.go index ba68535c3d..fe470f27c4 100644 --- a/internal/jobcontainers/path.go +++ b/internal/jobcontainers/path.go @@ -3,6 +3,7 @@ package jobcontainers import ( "fmt" "os" + "path/filepath" "strings" "github.com/Microsoft/hcsshim/internal/winapi" @@ -71,6 +72,10 @@ func getApplicationName(commandLine, workingDirectory, pathEnv string) (string, searchPath string result string ) + + // Clean the path, to get rid of any . elements + commandLine = filepath.Clean(commandLine) + // First we get the system paths concatenated with semicolons (C:\windows;C:\windows\system32;C:\windows\system;) // and use this as the basis for the directories to search for the application. systemPaths, err := getSystemPaths() diff --git a/internal/jobcontainers/path_test.go b/internal/jobcontainers/path_test.go index 9f631cff31..2bc9de5b5c 100644 --- a/internal/jobcontainers/path_test.go +++ b/internal/jobcontainers/path_test.go @@ -3,9 +3,16 @@ package jobcontainers import ( "os" "os/exec" + "strings" "testing" ) +func assertStr(t *testing.T, a string, b string) { + if !strings.EqualFold(a, b) { + t.Fatalf("expected %s, got %s", a, b) + } +} + func TestSearchPath(t *testing.T) { // Testing that relative paths work. _, err := searchPathForExe("windows\\system32\\ping", "C:\\") @@ -17,30 +24,52 @@ func TestSearchPath(t *testing.T) { if err != nil { t.Fatal(err) } + + _, err = searchPathForExe("ping", "C:\\windows\\system32") + if err != nil { + t.Fatal(err) + } } func TestGetApplicationName(t *testing.T) { + expected := "C:\\WINDOWS\\system32\\ping.exe" + cwd, err := os.Getwd() if err != nil { t.Fatal(err) } - _, _, err = getApplicationName("ping", cwd, os.Getenv("PATH")) + path, _, err := getApplicationName("ping", cwd, os.Getenv("PATH")) + if err != nil { + t.Fatal(err) + } + assertStr(t, expected, path) + + path, _, err = getApplicationName("./ping", cwd, os.Getenv("PATH")) + if err != nil { + t.Fatal(err) + } + assertStr(t, expected, path) + + path, _, err = getApplicationName(".\\ping", cwd, os.Getenv("PATH")) if err != nil { t.Fatal(err) } + assertStr(t, expected, path) // Test that we only find the first element of the commandline if the binary exists. - _, _, err = getApplicationName("ping test", cwd, os.Getenv("PATH")) + path, _, err = getApplicationName("ping test", cwd, os.Getenv("PATH")) if err != nil { t.Fatal(err) } + assertStr(t, expected, path) // Test quoted application name with an argument afterwards. path, cmdLine, err := getApplicationName("\"ping\" 127.0.0.1", cwd, os.Getenv("PATH")) if err != nil { t.Fatal(err) } + assertStr(t, expected, path) args := splitArgs(cmdLine) cmd := &exec.Cmd{