proc: Fix command-line arguments on Windows (#501)

* proc: add tests for command-line arguments

adds tests to make sure command-line arguments are passed to Launch() properly

* proc_windows: pass command-line arguments to CreateProcess()

build command-line arguments according to how the standard library does it and pass the command line along to the actual syscall on Windows.

see discussion in #479

* proc: better testing of cmd-line arguments

* proc_windows: fix a possible error-case with passing just 1 argument

previously, the command line pointer passed to sys.CreateProcess was empty, if we had 0 parameters (len(cmd) == 1, as cmd[0] is the executable, so no cmdlineGo would be created, while with any argument it would as len(cmd) > 1). This might cause problems down the road, so make sure we include the command line every time, even if it seems to work without.

* proc: improve testing of command-line arguments

test that arguments with spaces are passed on correctly and DRY failure/success condition checking in the args test

_fixtures/testargs.go

@@ -0,0 +1,20 @@
+package main
+
+import (
+	"fmt"
+	"os"
+)
+
+func main() {
+	// this test expects AT LEAST 1 argument, and the first one needs to be "test".
+	// second one is optional but if given, it should be "-passFlag"
+	fmt.Printf("received args %#v\n", os.Args)
+	if len(os.Args) < 2 {
+		panic("os.args too short!")
+	} else if os.Args[1] != "test" {
+		panic("os.args[1] is not test!")
+	}
+	if len(os.Args) >= 3 && os.Args[2] != "pass flag" {
+		panic("os.args[2] is not \"pass flag\"!")
+	}
+}

proc/proc_test.go

@@ -43,6 +43,21 @@ func withTestProcess(name string, t testing.TB, fn func(p *Process, fixture prot
 	fn(p, fixture)
 }
 
+func withTestProcessArgs(name string, t testing.TB, fn func(p *Process, fixture protest.Fixture), args []string) {
+	fixture := protest.BuildFixture(name)
+	p, err := Launch(append([]string{fixture.Path}, args...))
+	if err != nil {
+		t.Fatal("Launch():", err)
+	}
+
+	defer func() {
+		p.Halt()
+		p.Kill()
+	}()
+
+	fn(p, fixture)
+}
+
 func getRegisters(p *Process, t *testing.T) Registers {
 	regs, err := p.Registers()
 	if err != nil {
@@ -1656,6 +1671,45 @@ func TestPanicBreakpoint(t *testing.T) {
 	})
 }
 
+func TestCmdLineArgs(t *testing.T) {
+	expectSuccess := func(p *Process, fixture protest.Fixture) {
+		err := p.Continue()
+		bp := p.CurrentBreakpoint()
+		if bp != nil && bp.Name == "unrecovered-panic" {
+			t.Fatalf("testing args failed on unrecovered-panic breakpoint: %v", p.CurrentBreakpoint)
+		}
+		exit, exited := err.(ProcessExitedError)
+		if !exited {
+			t.Fatalf("Process did not exit!", err)
+		} else {
+			if exit.Status != 0 {
+				t.Fatalf("process exited with invalid status", exit.Status)
+			}
+		}
+	}
+
+	expectPanic := func(p *Process, fixture protest.Fixture) {
+		p.Continue()
+		bp := p.CurrentBreakpoint()
+		if bp == nil || bp.Name != "unrecovered-panic" {
+			t.Fatalf("not on unrecovered-panic breakpoint: %v", p.CurrentBreakpoint)
+		}
+	}
+
+	// make sure multiple arguments (including one with spaces) are passed to the binary correctly
+	withTestProcessArgs("testargs", t, expectSuccess, []string{"test"})
+	withTestProcessArgs("testargs", t, expectSuccess, []string{"test", "pass flag"})
+	// check that arguments with spaces are *only* passed correctly when correctly called
+	withTestProcessArgs("testargs", t, expectPanic, []string{"test pass", "flag"})
+	withTestProcessArgs("testargs", t, expectPanic, []string{"test", "pass", "flag"})
+	withTestProcessArgs("testargs", t, expectPanic, []string{"test pass flag"})
+	// and that invalid cases (wrong arguments or no arguments) panic
+	withTestProcess("testargs", t, expectPanic)
+	withTestProcessArgs("testargs", t, expectPanic, []string{"invalid"})
+	withTestProcessArgs("testargs", t, expectPanic, []string{"test", "invalid"})
+	withTestProcessArgs("testargs", t, expectPanic, []string{"invalid", "pass flag"})
+}
+
 func TestIssue462(t *testing.T) {
 	// Stacktrace of Goroutine 0 fails with an error
 	if runtime.GOOS == "windows" {

proc/proc_windows.go

@@ -47,9 +47,6 @@ func Launch(cmd []string) (*Process, error) {
 	if _, err := os.Stat(argv0Go); err != nil {
 		return nil, err
 	}
-
-	argv0, _ := syscall.UTF16PtrFromString(argv0Go)
-
 	// Duplicate the stdin/stdout/stderr handles
 	files := []uintptr{uintptr(syscall.Stdin), uintptr(syscall.Stdout), uintptr(syscall.Stderr)}
 	p, _ := syscall.GetCurrentProcess()
@@ -62,6 +59,32 @@ func Launch(cmd []string) (*Process, error) {
 		defer syscall.CloseHandle(syscall.Handle(fd[i]))
 	}
 
+	argv0, err := syscall.UTF16PtrFromString(argv0Go)
+	if err != nil {
+		return nil, err
+	}
+
+	// create suitable command line for CreateProcess
+	// see https://github.com/golang/go/blob/master/src/syscall/exec_windows.go#L326
+	// adapted from standard library makeCmdLine
+	// see https://github.com/golang/go/blob/master/src/syscall/exec_windows.go#L86
+	var cmdLineGo string
+	if len(cmd) >= 1 {
+		for _, v := range cmd {
+			if cmdLineGo != "" {
+				cmdLineGo += " "
+			}
+			cmdLineGo += syscall.EscapeArg(v)
+		}
+	}
+
+	var cmdLine *uint16
+	if cmdLineGo != "" {
+		if cmdLine, err = syscall.UTF16PtrFromString(cmdLineGo); err != nil {
+			return nil, err
+		}
+	}
+
 	// Initialize the startup info and create process
 	si := new(sys.StartupInfo)
 	si.Cb = uint32(unsafe.Sizeof(*si))
@@ -70,7 +93,7 @@ func Launch(cmd []string) (*Process, error) {
 	si.StdOutput = sys.Handle(fd[1])
 	si.StdErr = sys.Handle(fd[2])
 	pi := new(sys.ProcessInformation)
-	err = sys.CreateProcess(argv0, nil, nil, nil, true, DEBUGONLYTHISPROCESS, nil, nil, si, pi)
+	err = sys.CreateProcess(argv0, cmdLine, nil, nil, true, DEBUGONLYTHISPROCESS, nil, nil, si, pi)
 	if err != nil {
 		return nil, err
 	}