Fix slight cmdline parsing issue for job container/misc cleanup

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

internal/jobcontainers/path_test.go

@@ -1,8 +1,10 @@
 package jobcontainers
 
 import (
+	"io/ioutil"
 	"os"
 	"os/exec"
+	"path/filepath"
 	"strings"
 	"testing"
 )
@@ -31,52 +33,159 @@ 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()
 	if err != nil {
 		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)
+
+}

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
 }