From 76c63b50c130e519118ee74cc1081c55040212f5 Mon Sep 17 00:00:00 2001 From: Daniel Canter Date: Fri, 13 Aug 2021 18:28:51 -0700 Subject: [PATCH] Remove filepath.Clean usage + touchup job containers path tests This change does a couple of things for the path resolution logic. 1. I was using filepath.Clean to remove any initial '.' paths in the command line to handle relative paths with a dot. However it changes all forward slashes to backslashes, which hinders command lines like the following: `"cmd /c ping 127.0.0.1"` as the /c argument will be altered. Windows Server containers don't handle relative paths with a dot, so it doesn't seem necessary to support them for job containers either, so just get rid of the call entirely. 2. Remove empty spaces off the end of the cmdline if it's quoted. There was an empty space in this case as I was using strings.Join to join the arguments after the quoted element. This had no effects on actual usage/functionality, but to compare commandlines for tests, this made things easier. 3. When replacing instances of %CONTAINER_SANDBOX_MOUNT_POINT% in the commandline, the path of the volume we were replacing it with had a trailing forward slash, so you'd end up with C:\C\12345678abcdefg\\\mybinary.exe (two forward slashes). This also didn't have much of an effect on anything, but just to clean things up. 4. Lastly, this change also refactors the job container path tests as they were a bit unwieldy and were due for the t.Run treatment. This moves the tests to being run via t.Run and a slice of configs to test different path, working directory, env matrixes. Also adds some new test cases to try out. Signed-off-by: Daniel Canter --- internal/jobcontainers/jobcontainer.go | 4 +- internal/jobcontainers/path.go | 8 +- internal/jobcontainers/path_test.go | 187 +++++++++++++++++++++---- 3 files changed, 164 insertions(+), 35 deletions(-) diff --git a/internal/jobcontainers/jobcontainer.go b/internal/jobcontainers/jobcontainer.go index 576f8420aa..86a98e26c2 100644 --- a/internal/jobcontainers/jobcontainer.go +++ b/internal/jobcontainers/jobcontainer.go @@ -577,6 +577,6 @@ func systemProcessInformation() ([]*winapi.SYSTEM_PROCESS_INFORMATION, error) { // 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) + str = strings.ReplaceAll(str, "%"+sandboxMountPointEnvVar+"%", c.sandboxMount[:len(c.sandboxMount)-1]) + return strings.ReplaceAll(str, "$env:"+sandboxMountPointEnvVar, c.sandboxMount[:len(c.sandboxMount)-1]) } diff --git a/internal/jobcontainers/path.go b/internal/jobcontainers/path.go index fe470f27c4..94895ee249 100644 --- a/internal/jobcontainers/path.go +++ b/internal/jobcontainers/path.go @@ -3,7 +3,6 @@ package jobcontainers import ( "fmt" "os" - "path/filepath" "strings" "github.com/Microsoft/hcsshim/internal/winapi" @@ -16,7 +15,7 @@ import ( // getApplicationName resolves a given command line string and returns the path to the executable that should be launched, and // an adjusted commandline if needed. The resolution logic may appear overcomplicated but is designed to match the logic used by -// standard Windows containers, as well as that used by CreateProcess (see notes for the lpApplicationName parameter). +// Windows Server containers, as well as that used by CreateProcess (see notes for the lpApplicationName parameter). // // The logic follows this set of steps: // - Construct a list of searchable paths to find the application. This includes the standard Windows system paths @@ -73,9 +72,6 @@ func getApplicationName(commandLine, workingDirectory, pathEnv string) (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() @@ -156,7 +152,7 @@ func getApplicationName(commandLine, workingDirectory, pathEnv string) (string, if quoteCmdLine { trialName = "\"" + trialName + "\"" trialName += " " + strings.Join(args[argsIndex+1:], " ") - adjustedCommandLine = trialName + adjustedCommandLine = strings.TrimSpace(trialName) // Take off trailing space at beginning and end. } return result, adjustedCommandLine, nil diff --git a/internal/jobcontainers/path_test.go b/internal/jobcontainers/path_test.go index 2bc9de5b5c..f19ffb0cbd 100644 --- a/internal/jobcontainers/path_test.go +++ b/internal/jobcontainers/path_test.go @@ -1,8 +1,9 @@ package jobcontainers import ( + "io/ioutil" "os" - "os/exec" + "path/filepath" "strings" "testing" ) @@ -31,52 +32,184 @@ func TestSearchPath(t *testing.T) { } } -func TestGetApplicationName(t *testing.T) { - expected := "C:\\WINDOWS\\system32\\ping.exe" +type config struct { + name string + commandLine string + workDir string + pathEnv string + expectedApplicationName string + expectedCmdline string +} - cwd, err := os.Getwd() - if err != nil { - t.Fatal(err) +func runGetApplicationNameTests(t *testing.T, tests []*config) { + for _, cfg := range tests { + t.Run(cfg.name, func(t *testing.T) { + path, cmdLine, err := getApplicationName(cfg.commandLine, cfg.workDir, cfg.pathEnv) + if err != nil { + t.Fatal(err) + } + assertStr(t, cfg.expectedCmdline, cmdLine) + assertStr(t, cfg.expectedApplicationName, path) + }) } +} - path, _, err := getApplicationName("ping", cwd, os.Getenv("PATH")) - if err != nil { - t.Fatal(err) +func TestGetApplicationNamePing(t *testing.T) { + expected := "C:\\WINDOWS\\system32\\ping.exe" + + tests := []*config{ + { + name: "Ping", + commandLine: "ping", + workDir: "C:\\", + pathEnv: "C:\\windows\\system32", + expectedCmdline: "ping", + expectedApplicationName: expected, + }, + { + name: "Ping_Relative_Forward_Slash", + commandLine: "system32/ping", + workDir: "C:\\windows\\", + pathEnv: "C:\\windows\\system32", + expectedCmdline: "system32/ping", + expectedApplicationName: expected, + }, + { + name: "Ping_Relative_Back_Slash", + commandLine: "system32\\ping", + workDir: "C:\\windows", + pathEnv: "C:\\windows\\system32", + expectedCmdline: "system32\\ping", + expectedApplicationName: expected, + }, + { + name: "Ping_Cwd_Windows_Directory", + commandLine: "system32\\ping", + workDir: "C:\\Windows", + pathEnv: "C:\\windows\\system32", + expectedCmdline: "system32\\ping", + expectedApplicationName: expected, + }, + { + name: "Ping_With_Cwd", + commandLine: "cmd /c ping 127.0.0.1", + workDir: "C:\\", + pathEnv: "C:\\windows\\system32", + expectedCmdline: "cmd /c ping 127.0.0.1", + expectedApplicationName: "C:\\windows\\system32\\cmd.exe", + }, + { + name: "Ping_With_Cwd_Relative_Path", + commandLine: "system32\\cmd /c ping 127.0.0.1", + workDir: "C:\\windows\\", + pathEnv: "C:\\windows\\system32", + expectedCmdline: "system32\\cmd /c ping 127.0.0.1", + expectedApplicationName: "C:\\windows\\system32\\cmd.exe", + }, + { + name: "Ping_With_Space", + commandLine: "ping test", + workDir: "C:\\", + pathEnv: "C:\\windows\\system32", + expectedCmdline: "ping test", + expectedApplicationName: expected, + }, + { + name: "Ping_With_Quote", + commandLine: "\"ping\" 127.0.0.1", + workDir: "C:\\", + pathEnv: "C:\\windows\\system32", + expectedCmdline: "\"ping\" 127.0.0.1", + expectedApplicationName: expected, + }, } - assertStr(t, expected, path) - path, _, err = getApplicationName("./ping", cwd, os.Getenv("PATH")) + runGetApplicationNameTests(t, tests) +} + +func TestGetApplicationNameRandomBinary(t *testing.T) { + tempDir, err := ioutil.TempDir("", "") if err != nil { t.Fatal(err) } - assertStr(t, expected, path) + defer os.RemoveAll(tempDir) - path, _, err = getApplicationName(".\\ping", cwd, os.Getenv("PATH")) + // Create fake executables in a temporary directory to use for the below tests. + testExe := filepath.Join(tempDir, "test.exe") + _, err = os.Create(testExe) 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. - path, _, err = getApplicationName("ping test", cwd, os.Getenv("PATH")) + test2Exe := filepath.Join(tempDir, "test 2.exe") + _, err = os.Create(test2Exe) 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")) + exeWithSpace := filepath.Join(tempDir, "exe with space.exe") + _, err = os.Create(exeWithSpace) if err != nil { t.Fatal(err) } - assertStr(t, expected, path) - args := splitArgs(cmdLine) - cmd := &exec.Cmd{ - Path: path, - Args: args, - } - if err := cmd.Run(); err != nil { - t.Fatal(err) + tests := []*config{ + // See if we can successfully find "exe with space.exe" with no quoting, it should first try "exe.exe", then "exe with.exe" and then finally + // "exe with space.exe" + { + name: "Spaces_With_No_Quoting", + commandLine: "exe with space.exe", + workDir: filepath.Dir(testExe), + pathEnv: "", + expectedCmdline: "\"exe with space.exe\"", + expectedApplicationName: exeWithSpace, + }, + // See if we can successfully find "exe with space.exe" with quoting, it should try "exe with space.exe" only. + { + name: "Spaces_With_Quoting", + commandLine: "\"exe with space.exe\"", + workDir: filepath.Dir(testExe), + pathEnv: "", + expectedCmdline: "\"exe with space.exe\"", + expectedApplicationName: exeWithSpace, + }, + // Try a quoted commandline, so that we find the actual "C:\rest\of\the\path\test 2.exe" binary + { + name: "Test2_Binary_With_Quotes", + commandLine: "\"test 2.exe\"", + workDir: filepath.Dir(test2Exe), + pathEnv: "", + expectedCmdline: "\"test 2.exe\"", + expectedApplicationName: test2Exe, + }, + // We should find the test.exe binary, and the 2 will be treated as an argument in this case + { + name: "Test2_Binary_No_Quotes", + commandLine: "test 2", + workDir: filepath.Dir(test2Exe), + pathEnv: "", + expectedCmdline: "test 2", + expectedApplicationName: testExe, + }, + // Test finding test binary with no file extension + { + name: "Test_Binary_No_File_Extension", + commandLine: testExe[0 : len(testExe)-4], + workDir: filepath.Dir(testExe), + pathEnv: "", + expectedCmdline: testExe[0 : len(testExe)-4], + expectedApplicationName: testExe, + }, + // Test finding the test binary with the PATH containing the directory it lives in. + { + name: "Test_Binary_With_Path_Containing_Location", + commandLine: "test", + workDir: "C:\\", + pathEnv: filepath.Dir(testExe), + expectedCmdline: "test", + expectedApplicationName: testExe, + }, } + + runGetApplicationNameTests(t, tests) }