syscall: StartProcess fixes for windows

- StartProcess will work with relative (to attr.Dir, not
  current directory) executable filenames
- StartProcess will only work if executable filename points
  to the real file, it will not search for executable in the
  $PATH list and others (see CreateProcess manual for details)
- StartProcess argv strings can contain any characters

R=golang-dev, r
CC=golang-dev
https://golang.org/cl/4306041

src/pkg/exec/exec_test.go

@@ -118,3 +118,55 @@ func TestAddEnvVar(t *testing.T) {
 		t.Fatal("close:", err)
 	}
 }
+
+var tryargs = []string{
+	`2`,
+	`2 `,
+	"2 \t",
+	`2" "`,
+	`2 ab `,
+	`2 "ab" `,
+	`2 \ `,
+	`2 \\ `,
+	`2 \" `,
+	`2 \`,
+	`2\`,
+	`2"`,
+	`2\"`,
+	`2 "`,
+	`2 \"`,
+	``,
+	`2 ^ `,
+	`2 \^`,
+}
+
+func TestArgs(t *testing.T) {
+	for _, a := range tryargs {
+		argv := []string{
+			"awk",
+			`BEGIN{printf("%s|%s|%s",ARGV[1],ARGV[2],ARGV[3])}`,
+			"/dev/null",
+			a,
+			"EOF",
+		}
+		exe, err := LookPath(argv[0])
+		if err != nil {
+			t.Fatal("run:", err)
+		}
+		cmd, err := Run(exe, argv, nil, "", DevNull, Pipe, DevNull)
+		if err != nil {
+			t.Fatal("run:", err)
+		}
+		buf, err := ioutil.ReadAll(cmd.Stdout)
+		if err != nil {
+			t.Fatal("read:", err)
+		}
+		expect := "/dev/null|" + a + "|EOF"
+		if string(buf) != expect {
+			t.Errorf("read: got %q expect %q", buf, expect)
+		}
+		if err = cmd.Close(); err != nil {
+			t.Fatal("close:", err)
+		}
+	}
+}

src/pkg/os/os_test.go

@@ -10,6 +10,7 @@ import (
 	"io"
 	"io/ioutil"
 	. "os"
+	"path/filepath"
 	"strings"
 	"syscall"
 	"testing"
@@ -409,25 +410,12 @@ func TestRename(t *testing.T) {
 	}
 }
 
-func TestStartProcess(t *testing.T) {
-	var cmd, expect string
-	var args []string
+func exec(t *testing.T, dir, cmd string, args []string, expect string) {
 	r, w, err := Pipe()
 	if err != nil {
 		t.Fatalf("Pipe: %v", err)
 	}
-	attr := &ProcAttr{Files: []*File{nil, w, Stderr}}
-	if syscall.OS == "windows" {
-		cmd = Getenv("COMSPEC")
-		args = []string{Getenv("COMSPEC"), "/c cd"}
-		attr.Dir = Getenv("SystemRoot")
-		expect = Getenv("SystemRoot") + "\r\n"
-	} else {
-		cmd = "/bin/pwd"
-		args = []string{"pwd"}
-		attr.Dir = "/"
-		expect = "/\n"
-	}
+	attr := &ProcAttr{Dir: dir, Files: []*File{nil, w, Stderr}}
 	p, err := StartProcess(cmd, args, attr)
 	if err != nil {
 		t.Fatalf("StartProcess: %v", err)
@@ -439,12 +427,34 @@ func TestStartProcess(t *testing.T) {
 	io.Copy(&b, r)
 	output := b.String()
 	if output != expect {
-		args[0] = cmd
-		t.Errorf("exec %q returned %q wanted %q", strings.Join(args, " "), output, expect)
+		t.Errorf("exec %q returned %q wanted %q",
+			strings.Join(append([]string{cmd}, args...), " "), output, expect)
 	}
 	p.Wait(0)
 }
 
+func TestStartProcess(t *testing.T) {
+	var dir, cmd, le string
+	var args []string
+	if syscall.OS == "windows" {
+		le = "\r\n"
+		cmd = Getenv("COMSPEC")
+		dir = Getenv("SystemRoot")
+		args = []string{"/c", "cd"}
+	} else {
+		le = "\n"
+		cmd = "/bin/pwd"
+		dir = "/"
+		args = []string{}
+	}
+	cmddir, cmdbase := filepath.Split(cmd)
+	args = append([]string{cmdbase}, args...)
+	// Test absolute executable path.
+	exec(t, dir, cmd, args, dir+le)
+	// Test relative executable path.
+	exec(t, cmddir, cmdbase, args, filepath.Clean(cmddir)+le)
+}
+
 func checkMode(t *testing.T, path string, mode uint32) {
 	dir, err := Stat(path)
 	if err != nil {

src/pkg/syscall/exec_windows.go

@@ -11,48 +11,92 @@ import (
 	"utf16"
 )
 
-// Windows doesn't have a good concept of just Exec in the documented API.
-// However, the kernel32 CreateProcess does a good job with
-// ForkExec.
-
 var ForkLock sync.RWMutex
 
-// Joins an array of string with sep
-// From the "strings" package.  Modified.
-func stringJoin(a []string, sep string, escape escapeFunc) string {
-	if len(a) == 0 {
-		return ""
+// escape rewrites command line argument s as prescribed
+// in http://msdn.microsoft.com/en-us/library/ms880421.
+// This function returns "" (2 double quotes) if s is empty.
+// Alternatively, these transformations are done:
+// - every back slash (\) is doubled, but only if immediately
+//   followed by double quote (");
+// - every double quote (") is escaped by back slash (\);
+// - finally, s is wrapped with double quotes (arg -> "arg"),
+//   but only if there is space or tab inside s.
+func escape(s string) string {
+	if len(s) == 0 {
+		return "\"\""
+	}
+	n := len(s)
+	hasSpace := false
+	for i := 0; i < len(s); i++ {
+		switch s[i] {
+		case '"', '\\':
+			n++
+		case ' ', '\t':
+			hasSpace = true
+		}
 	}
-	if len(a) == 1 {
-		return a[0]
+	if hasSpace {
+		n += 2
 	}
-	n := len(sep) * (len(a) - 1)
-	for i := 0; i < len(a); i++ {
-		a[i] = escape(a[i])
-		n += len(a[i])
+	if n == len(s) {
+		return s
 	}
 
-	b := make([]byte, n)
-	bp := 0
-	for i := 0; i < len(a); i++ {
-		s := a[i]
-		for j := 0; j < len(s); j++ {
-			b[bp] = s[j]
-			bp++
+	qs := make([]byte, n)
+	j := 0
+	if hasSpace {
+		qs[j] = '"'
+		j++
+	}
+	slashes := 0
+	for i := 0; i < len(s); i++ {
+		switch s[i] {
+		default:
+			slashes = 0
+			qs[j] = s[i]
+		case '\\':
+			slashes++
+			qs[j] = s[i]
+		case '"':
+			for ; slashes > 0; slashes-- {
+				qs[j] = '\\'
+				j++
+			}
+			qs[j] = '\\'
+			j++
+			qs[j] = s[i]
+		}
+		j++
+	}
+	if hasSpace {
+		for ; slashes > 0; slashes-- {
+			qs[j] = '\\'
+			j++
 		}
-		if i+1 < len(a) {
-			s = sep
-			for j := 0; j < len(s); j++ {
-				b[bp] = s[j]
-				bp++
+		qs[j] = '"'
+		j++
 	}
+	return string(qs[:j])
+}
+
+// makeCmdLine builds a command line out of args by escaping "special"
+// characters and joining the arguments with spaces.
+func makeCmdLine(args []string) string {
+	var s string
+	for _, v := range args {
+		if s != "" {
+			s += " "
 		}
+		s += escape(v)
 	}
-	return string(b)
+	return s
 }
 
-//Env block is a sequence of null terminated strings followed by a null.
-//Last bytes are two unicode nulls, or four null bytes.
+// createEnvBlock converts an array of environment strings into
+// the representation required by CreateProcess: a sequence of NUL
+// terminated strings followed by a nil.
+// Last bytes are two UCS-2 NULs, or four NUL bytes.
 func createEnvBlock(envv []string) *uint16 {
 	if len(envv) == 0 {
 		return &utf16.Encode([]int("\x00\x00"))[0]
@@ -76,42 +120,100 @@ func createEnvBlock(envv []string) *uint16 {
 	return &utf16.Encode([]int(string(b)))[0]
 }
 
-type escapeFunc func(s string) string
+func CloseOnExec(fd int) {
+	SetHandleInformation(int32(fd), HANDLE_FLAG_INHERIT, 0)
+}
 
-//escapes quotes by " -> ""
-//Also string -> "string"
-func escapeAddQuotes(s string) string {
-	//normal ascii char, one byte wide
-	rune := byte('"')
-	l := len(s)
-	n := 0
-	for i := 0; i < l; i++ {
-		if s[i] == rune {
-			n++
+func SetNonblock(fd int, nonblocking bool) (errno int) {
+	return 0
+}
+
+// getFullPath retrieves the full path of the specified file.
+// Just a wrapper for Windows GetFullPathName api.
+func getFullPath(name string) (path string, err int) {
+	p := StringToUTF16Ptr(name)
+	buf := make([]uint16, 100)
+	n, err := GetFullPathName(p, uint32(len(buf)), &buf[0], nil)
+	if err != 0 {
+		return "", err
 	}
+	if n > uint32(len(buf)) {
+		// Windows is asking for bigger buffer.
+		buf = make([]uint16, n)
+		n, err = GetFullPathName(p, uint32(len(buf)), &buf[0], nil)
+		if err != 0 {
+			return "", err
 		}
-	qs := make([]byte, l+n+2)
-
-	qs[0] = rune
-	j := 1
-	for i := 0; i < l; i++ {
-		qs[i+j] = s[i]
-		if s[i] == rune {
-			j++
-			qs[i+j] = rune
+		if n > uint32(len(buf)) {
+			return "", EINVAL
 		}
 	}
-	qs[len(qs)-1] = rune
-	return string(qs)
+	return UTF16ToString(buf[:n]), 0
 }
 
+func isSlash(c uint8) bool {
+	return c == '\\' || c == '/'
+}
 
-func CloseOnExec(fd int) {
-	SetHandleInformation(int32(fd), HANDLE_FLAG_INHERIT, 0)
+func normalizeDir(dir string) (name string, err int) {
+	ndir, err := getFullPath(dir)
+	if err != 0 {
+		return "", err
+	}
+	if len(ndir) > 2 && isSlash(ndir[0]) && isSlash(ndir[1]) {
+		// dir cannot have \\server\share\path form
+		return "", EINVAL
+	}
+	return ndir, 0
 }
 
-func SetNonblock(fd int, nonblocking bool) (errno int) {
-	return 0
+func volToUpper(ch int) int {
+	if 'a' <= ch && ch <= 'z' {
+		ch += 'A' - 'a'
+	}
+	return ch
+}
+
+func joinExeDirAndFName(dir, p string) (name string, err int) {
+	if len(p) == 0 {
+		return "", EINVAL
+	}
+	if len(p) > 2 && isSlash(p[0]) && isSlash(p[1]) {
+		// \\server\share\path form
+		return p, 0
+	}
+	if len(p) > 1 && p[1] == ':' {
+		// has drive letter
+		if len(p) == 2 {
+			return "", EINVAL
+		}
+		if isSlash(p[2]) {
+			return p, 0
+		} else {
+			d, err := normalizeDir(dir)
+			if err != 0 {
+				return "", err
+			}
+			if volToUpper(int(p[0])) == volToUpper(int(d[0])) {
+				return getFullPath(d + "\\" + p[2:])
+			} else {
+				return getFullPath(p)
+			}
+		}
+	} else {
+		// no drive letter
+		d, err := normalizeDir(dir)
+		if err != 0 {
+			return "", err
+		}
+		if isSlash(p[0]) {
+			return getFullPath(d[:2] + p)
+		} else {
+			return getFullPath(d + "\\" + p)
+		}
+	}
+	// we shouldn't be here
+	return "", EINVAL
 }
 
 type ProcAttr struct {
@@ -122,9 +224,10 @@ type ProcAttr struct {
 
 var zeroAttributes ProcAttr
 
-// TODO(kardia): Add trace
-//The command and arguments are passed via the Command line parameter.
 func StartProcess(argv0 string, argv []string, attr *ProcAttr) (pid, handle int, err int) {
+	if len(argv0) == 0 {
+		return 0, 0, EWINDOWS
+	}
 	if attr == nil {
 		attr = &zeroAttributes
 	}
@@ -132,25 +235,31 @@ func StartProcess(argv0 string, argv []string, attr *ProcAttr) (pid, handle int,
 		return 0, 0, EWINDOWS
 	}
 
-	// CreateProcess will throw an error if the dir is not set to a valid dir
-	// thus get the working dir if dir is empty.
-	dir := attr.Dir
-	if dir == "" {
-		if wd, ok := Getwd(); ok == 0 {
-			dir = wd
+	if len(attr.Dir) != 0 {
+		// StartProcess assumes that argv0 is relative to attr.Dir,
+		// because it implies Chdir(attr.Dir) before executing argv0.
+		// Windows CreateProcess assumes the opposite: it looks for
+		// argv0 relative to the current directory, and, only once the new
+		// process is started, it does Chdir(attr.Dir). We are adjusting
+		// for that difference here by making argv0 absolute.
+		var err int
+		argv0, err = joinExeDirAndFName(attr.Dir, argv0)
+		if err != 0 {
+			return 0, 0, err
 		}
 	}
-	fd := attr.Files
-
-	startupInfo := new(StartupInfo)
-	processInfo := new(ProcessInformation)
+	argv0p := StringToUTF16Ptr(argv0)
 
-	GetStartupInfo(startupInfo)
+	var argvp *uint16
+	s := makeCmdLine(argv)
+	if len(s) != 0 {
+		argvp = StringToUTF16Ptr(s)
+	}
 
-	startupInfo.Flags = STARTF_USESTDHANDLES
-	startupInfo.StdInput = 0
-	startupInfo.StdOutput = 0
-	startupInfo.StdErr = 0
+	var dirp *uint16
+	if len(attr.Dir) != 0 {
+		dirp = StringToUTF16Ptr(attr.Dir)
+	}
 
 	// Acquire the fork lock so that no other threads
 	// create new fds that are not yet close-on-exec
@@ -158,54 +267,35 @@ func StartProcess(argv0 string, argv []string, attr *ProcAttr) (pid, handle int,
 	ForkLock.Lock()
 	defer ForkLock.Unlock()
 
-	var currentProc, _ = GetCurrentProcess()
-	if len(fd) > 0 && fd[0] > 0 {
-		err := DuplicateHandle(currentProc, int32(fd[0]), currentProc, &startupInfo.StdInput, 0, true, DUPLICATE_SAME_ACCESS)
+	p, _ := GetCurrentProcess()
+	fd := make([]int32, len(attr.Files))
+	for i, _ := range attr.Files {
+		if attr.Files[i] > 0 {
+			err := DuplicateHandle(p, int32(attr.Files[i]), p, &fd[i], 0, true, DUPLICATE_SAME_ACCESS)
 			if err != 0 {
 				return 0, 0, err
 			}
-		defer CloseHandle(int32(startupInfo.StdInput))
-	}
-	if len(fd) > 1 && fd[1] > 0 {
-		err := DuplicateHandle(currentProc, int32(fd[1]), currentProc, &startupInfo.StdOutput, 0, true, DUPLICATE_SAME_ACCESS)
-		if err != 0 {
-			return 0, 0, err
+			defer CloseHandle(int32(fd[i]))
 		}
-		defer CloseHandle(int32(startupInfo.StdOutput))
 	}
-	if len(fd) > 2 && fd[2] > 0 {
-		err := DuplicateHandle(currentProc, int32(fd[2]), currentProc, &startupInfo.StdErr, 0, true, DUPLICATE_SAME_ACCESS)
+	si := new(StartupInfo)
+	GetStartupInfo(si)
+	si.Flags = STARTF_USESTDHANDLES
+	si.StdInput = fd[0]
+	si.StdOutput = fd[1]
+	si.StdErr = fd[2]
+
+	pi := new(ProcessInformation)
+
+	err = CreateProcess(argv0p, argvp, nil, nil, true, CREATE_UNICODE_ENVIRONMENT, createEnvBlock(attr.Env), dirp, si, pi)
 	if err != 0 {
 		return 0, 0, err
 	}
-		defer CloseHandle(int32(startupInfo.StdErr))
-	}
-	if len(argv) == 0 {
-		argv = []string{""}
-	}
-	// argv0 must not be longer then 256 chars
-	// but the entire cmd line can have up to 32k chars (msdn)
-	err = CreateProcess(
-		nil,
-		StringToUTF16Ptr(escapeAddQuotes(argv0)+" "+stringJoin(argv[1:], " ", escapeAddQuotes)),
-		nil,  //ptr to struct lpProcessAttributes
-		nil,  //ptr to struct lpThreadAttributes
-		true, //bInheritHandles
-		CREATE_UNICODE_ENVIRONMENT, //Flags
-		createEnvBlock(attr.Env),   //env block, NULL uses parent env
-		StringToUTF16Ptr(dir),
-		startupInfo,
-		processInfo)
-
-	if err == 0 {
-		pid = int(processInfo.ProcessId)
-		handle = int(processInfo.Process)
-		CloseHandle(processInfo.Thread)
-	}
-	return
+	defer CloseHandle(pi.Thread)
+
+	return int(pi.ProcessId), int(pi.Process), 0
 }
 
-// Ordinary exec.
 func Exec(argv0 string, argv []string, envv []string) (err int) {
 	return EWINDOWS
 }

src/pkg/syscall/syscall_windows.go

@@ -135,7 +135,7 @@ func NewCallback(fn interface{}) uintptr
 //sys	CreateIoCompletionPort(filehandle int32, cphandle int32, key uint32, threadcnt uint32) (handle int32, errno int)
 //sys	GetQueuedCompletionStatus(cphandle int32, qty *uint32, key *uint32, overlapped **Overlapped, timeout uint32) (errno int)
 //sys	CancelIo(s uint32) (errno int)
-//sys	CreateProcess(appName *int16, commandLine *uint16, procSecurity *int16, threadSecurity *int16, inheritHandles bool, creationFlags uint32, env *uint16, currentDir *uint16, startupInfo *StartupInfo, outProcInfo *ProcessInformation) (errno int) = CreateProcessW
+//sys	CreateProcess(appName *uint16, commandLine *uint16, procSecurity *SecurityAttributes, threadSecurity *SecurityAttributes, inheritHandles bool, creationFlags uint32, env *uint16, currentDir *uint16, startupInfo *StartupInfo, outProcInfo *ProcessInformation) (errno int) = CreateProcessW
 //sys	OpenProcess(da uint32, inheritHandle bool, pid uint32) (handle uint32, errno int)
 //sys	GetExitCodeProcess(handle uint32, exitcode *uint32) (errno int)
 //sys	GetStartupInfo(startupInfo *StartupInfo) (errno int) = GetStartupInfoW
@@ -160,6 +160,7 @@ func NewCallback(fn interface{}) uintptr
 //sys	LocalFree(hmem uint32) (handle uint32, errno int) [failretval!=0]
 //sys	SetHandleInformation(handle int32, mask uint32, flags uint32) (errno int)
 //sys	FlushFileBuffers(handle int32) (errno int)
+//sys	GetFullPathName(path *uint16, buflen uint32, buf *uint16, fname **uint16) (n uint32, errno int) = kernel32.GetFullPathNameW
 
 // syscall interface implementation for other packages
 

src/pkg/syscall/zsyscall_windows_386.go

-// mksyscall_windows.sh -l32 syscall_windows.go syscall_windows_386.go
+// mksyscall_windows.pl -l32 syscall_windows.go syscall_windows_386.go
 // MACHINE GENERATED BY THE COMMAND ABOVE; DO NOT EDIT
 
 package syscall
@@ -69,6 +69,7 @@ var (
 	procLocalFree                  = getSysProcAddr(modkernel32, "LocalFree")
 	procSetHandleInformation       = getSysProcAddr(modkernel32, "SetHandleInformation")
 	procFlushFileBuffers           = getSysProcAddr(modkernel32, "FlushFileBuffers")
+	procGetFullPathNameW           = getSysProcAddr(modkernel32, "GetFullPathNameW")
 	procWSAStartup                 = getSysProcAddr(modwsock32, "WSAStartup")
 	procWSACleanup                 = getSysProcAddr(modwsock32, "WSACleanup")
 	procsocket                     = getSysProcAddr(modwsock32, "socket")
@@ -515,7 +516,7 @@ func CancelIo(s uint32) (errno int) {
 	return
 }
 
-func CreateProcess(appName *int16, commandLine *uint16, procSecurity *int16, threadSecurity *int16, inheritHandles bool, creationFlags uint32, env *uint16, currentDir *uint16, startupInfo *StartupInfo, outProcInfo *ProcessInformation) (errno int) {
+func CreateProcess(appName *uint16, commandLine *uint16, procSecurity *SecurityAttributes, threadSecurity *SecurityAttributes, inheritHandles bool, creationFlags uint32, env *uint16, currentDir *uint16, startupInfo *StartupInfo, outProcInfo *ProcessInformation) (errno int) {
 	var _p0 uint32
 	if inheritHandles {
 		_p0 = 1
@@ -885,6 +886,21 @@ func FlushFileBuffers(handle int32) (errno int) {
 	return
 }
 
+func GetFullPathName(path *uint16, buflen uint32, buf *uint16, fname **uint16) (n uint32, errno int) {
+	r0, _, e1 := Syscall6(procGetFullPathNameW, 4, uintptr(unsafe.Pointer(path)), uintptr(buflen), uintptr(unsafe.Pointer(buf)), uintptr(unsafe.Pointer(fname)), 0, 0)
+	n = uint32(r0)
+	if n == 0 {
+		if e1 != 0 {
+			errno = int(e1)
+		} else {
+			errno = EINVAL
+		}
+	} else {
+		errno = 0
+	}
+	return
+}
+
 func WSAStartup(verreq uint32, data *WSAData) (sockerrno int) {
 	r0, _, _ := Syscall(procWSAStartup, 2, uintptr(verreq), uintptr(unsafe.Pointer(data)), 0)
 	sockerrno = int(r0)