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 }