From 5d7573d7fc94ec47706c67167de2bf150418ac46 Mon Sep 17 00:00:00 2001 From: Daniel Canter Date: Fri, 13 Aug 2021 18:28:51 -0700 Subject: [PATCH] Fix slight cmdline parsing issue for job container/misc cleanup I was using filepath.Clean to remove any initial '.' paths in the commandline. However it changes all forward slashes to backslashes, which hinders commandlines like the following: "cmd /c ping 127.0.0.1" as the /c argument will be altered. Misc. cleanup involves cleaning up some comments, changing to FreeLibrary instead of CloseHandle after loading kernel32 for signals, and checking if the stdio pipes are nil before closing them. Signed-off-by: Daniel Canter --- internal/jobcontainers/path.go | 9 +- internal/jobcontainers/path_test.go | 125 ++++++++++++++++++++++++++-- internal/jobcontainers/process.go | 27 ++++-- 3 files changed, 141 insertions(+), 20 deletions(-) diff --git a/internal/jobcontainers/path.go b/internal/jobcontainers/path.go index fe470f27c4..ae1f3caeff 100644 --- a/internal/jobcontainers/path.go +++ b/internal/jobcontainers/path.go @@ -16,7 +16,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 +73,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() @@ -107,7 +104,7 @@ func getApplicationName(commandLine, workingDirectory, pathEnv string) (string, if index == -1 { return "", "", errors.New("no ending quotation mark found in command") } - path, err := searchPathForExe(commandLine[1:index+1], searchPath) + path, err := searchPathForExe(filepath.Clean(commandLine[1:index+1]), searchPath) if err != nil { return "", "", err } @@ -130,7 +127,7 @@ func getApplicationName(commandLine, workingDirectory, pathEnv string) (string, // if foo.exe is successfully found we will stop and return with the full path to 'foo.exe'. If foo doesn't succeed we // then try 'foo bar.exe' and 'foo bar baz.exe'. for argsIndex < len(args) { - trialName += args[argsIndex] + trialName += filepath.Clean(args[argsIndex]) fullPath, err := searchPathForExe(trialName, searchPath) if err == nil { result = fullPath diff --git a/internal/jobcontainers/path_test.go b/internal/jobcontainers/path_test.go index 2bc9de5b5c..64da3882a7 100644 --- a/internal/jobcontainers/path_test.go +++ b/internal/jobcontainers/path_test.go @@ -1,8 +1,10 @@ package jobcontainers import ( + "io/ioutil" "os" "os/exec" + "path/filepath" "strings" "testing" ) @@ -31,7 +33,7 @@ func TestSearchPath(t *testing.T) { } } -func TestGetApplicationName(t *testing.T) { +func TestGetApplicationNamePing(t *testing.T) { expected := "C:\\WINDOWS\\system32\\ping.exe" cwd, err := os.Getwd() @@ -39,44 +41,151 @@ func TestGetApplicationName(t *testing.T) { t.Fatal(err) } - path, _, err := getApplicationName("ping", cwd, os.Getenv("PATH")) + pathEnv := os.Getenv("PATH") + + path, _, err := getApplicationName("ping", cwd, pathEnv) + if err != nil { + t.Fatal(err) + } + assertStr(t, expected, path) + + path, _, err = getApplicationName("./ping", cwd, pathEnv) if err != nil { t.Fatal(err) } assertStr(t, expected, path) - path, _, err = getApplicationName("./ping", cwd, os.Getenv("PATH")) + path, _, err = getApplicationName(".\\ping", cwd, pathEnv) if err != nil { t.Fatal(err) } assertStr(t, expected, path) - path, _, err = getApplicationName(".\\ping", cwd, os.Getenv("PATH")) + // Test relative path with different cwd + newCwd := `C:\Windows\` + _, _, err = getApplicationName("./system32/ping", newCwd, pathEnv) if err != nil { t.Fatal(err) } assertStr(t, expected, path) + pingWithCmd := "cmd /c ping 127.0.0.1" + path, cmdLine, err := getApplicationName("cmd /c ping 127.0.0.1", cwd, pathEnv) + if err != nil { + t.Fatal(err) + } + assertStr(t, cmdLine, pingWithCmd) + assertStr(t, "C:\\windows\\system32\\cmd.exe", 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")) + path, _, err = getApplicationName("ping test", cwd, pathEnv) 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")) + path, cmdLine, err = getApplicationName("\"ping\" 127.0.0.1", cwd, pathEnv) if err != nil { t.Fatal(err) } assertStr(t, expected, path) - args := splitArgs(cmdLine) cmd := &exec.Cmd{ Path: path, - Args: args, + Args: splitArgs(cmdLine), } if err := cmd.Run(); err != nil { t.Fatal(err) } } + +func TestGetApplicationNameRandomBinary(t *testing.T) { + pathEnv := os.Getenv("PATH") + + tempDir, err := ioutil.TempDir("", "") + if err != nil { + t.Fatal(err) + } + defer os.RemoveAll(tempDir) + + // 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) + } + + test2Exe := filepath.Join(tempDir, "test 2.exe") + _, err = os.Create(test2Exe) + if err != nil { + t.Fatal(err) + } + + exeWithSpace := filepath.Join(tempDir, "exe with space.exe") + _, err = os.Create(exeWithSpace) + if err != nil { + t.Fatal(err) + } + + // 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" + path, _, err := getApplicationName("exe with space.exe", filepath.Dir(testExe), os.Getenv("PATH")) + if err != nil { + t.Fatal(err) + } + assertStr(t, exeWithSpace, path) + + // See if we can successfully find "exe with space.exe" with quoting, it should try "exe with space.exe" only. + path, _, err = getApplicationName("\"exe with space.exe\"", filepath.Dir(testExe), os.Getenv("PATH")) + if err != nil { + t.Fatal(err) + } + assertStr(t, exeWithSpace, path) + + // Try a quoted commandline, so that we find the actual "C:\rest\of\the\path\test 2.exe" binary + path, _, err = getApplicationName("\"test 2.exe\"", filepath.Dir(test2Exe), os.Getenv("PATH")) + if err != nil { + t.Fatal(err) + } + assertStr(t, test2Exe, path) + + // We should find the test.exe binary, and the 2 will be treated as an argument in this case + path, _, err = getApplicationName("test 2", filepath.Dir(test2Exe), os.Getenv("PATH")) + if err != nil { + t.Fatal(err) + } + assertStr(t, testExe, path) + + // Test relative path with the current working directory set to the directory that contains the binary. + path, _, err = getApplicationName("./test.exe", filepath.Dir(testExe), pathEnv) + if err != nil { + t.Fatal(err) + } + assertStr(t, testExe, path) + + // Test relative path with backslashes with the current working directory set to the directory that contains the binary. + path, _, err = getApplicationName(".\\test.exe", filepath.Dir(testExe), pathEnv) + if err != nil { + t.Fatal(err) + } + assertStr(t, testExe, path) + + // Test no file extension + path, _, err = getApplicationName(testExe[0:len(testExe)-4], filepath.Dir(testExe), pathEnv) + if err != nil { + t.Fatal(err) + } + assertStr(t, testExe, path) + + // Add test binary path to PATH and try to find it by just 'test.exe' + if err := os.Setenv("PATH", os.Getenv("PATH")+filepath.Dir(testExe)); err != nil { + t.Fatal(err) + } + path, _, err = getApplicationName("test.exe", filepath.Dir(testExe), os.Getenv("PATH")) + if err != nil { + t.Fatal(err) + } + assertStr(t, testExe, path) + +} diff --git a/internal/jobcontainers/process.go b/internal/jobcontainers/process.go index 2cccc8a1d3..d9401684cc 100644 --- a/internal/jobcontainers/process.go +++ b/internal/jobcontainers/process.go @@ -92,21 +92,30 @@ func (p *JobProcess) Signal(ctx context.Context, options interface{}) (bool, err func (p *JobProcess) CloseStdin(ctx context.Context) error { p.stdioLock.Lock() defer p.stdioLock.Unlock() - return p.stdin.Close() + if p.stdin != nil { + return p.stdin.Close() + } + return nil } // CloseStdout closes the stdout pipe of the process. func (p *JobProcess) CloseStdout(ctx context.Context) error { p.stdioLock.Lock() defer p.stdioLock.Unlock() - return p.stdout.Close() + if p.stdout != nil { + return p.stdout.Close() + } + return nil } // CloseStderr closes the stderr pipe of the process. func (p *JobProcess) CloseStderr(ctx context.Context) error { p.stdioLock.Lock() defer p.stdioLock.Unlock() - return p.stderr.Close() + if p.stderr != nil { + return p.stderr.Close() + } + return nil } // Wait waits for the process to exit. If the process has already exited returns @@ -217,7 +226,9 @@ func signalProcess(pid uint32, signal int) error { if err != nil { return errors.Wrap(err, "failed to open process") } - defer windows.Close(hProc) + defer func() { + _ = windows.Close(hProc) + }() // We can't use GenerateConsoleCtrlEvent since that only supports CTRL_C_EVENT and CTRL_BREAK_EVENT. // Instead, to handle an arbitrary signal we open a CtrlRoutine thread inside the target process and @@ -231,7 +242,9 @@ func signalProcess(pid uint32, signal int) error { if err != nil { return errors.Wrap(err, "failed to load kernel32 library") } - defer windows.Close(k32) + defer func() { + _ = windows.FreeLibrary(k32) + }() proc, err := windows.GetProcAddress(k32, "CtrlRoutine") if err != nil { @@ -242,6 +255,8 @@ func signalProcess(pid uint32, signal int) error { if err != nil { return errors.Wrapf(err, "failed to open remote thread in target process %d", pid) } - defer windows.Close(threadHandle) + defer func() { + _ = windows.Close(threadHandle) + }() return nil }