path/filepath, os/exec: unquote PATH elements on Windows

On Windows, directory names in PATH can be fully or partially quoted
in double quotes ('"'), but the path names as used by most APIs must
be unquoted. In addition, quoted names can contain the semicolon
(';') character, which is otherwise used as ListSeparator.

This CL changes SplitList in path/filepath and LookPath in os/exec
to only	treat unquoted semicolons as separators, and to unquote the
separated elements.

(In addition, fix harmless test bug I introduced for LookPath on Unix.)

Related discussion thread:
https://groups.google.com/d/msg/golang-nuts/PXCr10DsRb4/sawZBM7scYgJ

R=rsc, minux.ma, mccoyst, alex.brainman, iant
CC=golang-dev
https://golang.org/cl/7181047

src/pkg/os/exec/lp_unix_test.go

@@ -32,7 +32,10 @@ func TestLookPathUnixEmptyPath(t *testing.T) {
 	if err != nil {
 		t.Fatal("OpenFile failed: ", err)
 	}
-	defer f.Close()
+	err = f.Close()
+	if err != nil {
+		t.Fatal("Close failed: ", err)
+	}
 
 	pathenv := os.Getenv("PATH")
 	defer os.Setenv("PATH", pathenv)

src/pkg/os/exec/lp_windows.go

@@ -72,7 +72,7 @@ func LookPath(file string) (f string, err error) {
 		return
 	}
 	if pathenv := os.Getenv(`PATH`); pathenv != `` {
-		for _, dir := range strings.Split(pathenv, `;`) {
+		for _, dir := range splitList(pathenv) {
 			if f, err = findExecutable(dir+`\`+file, exts); err == nil {
 				return
 			}
@@ -80,3 +80,36 @@ func LookPath(file string) (f string, err error) {
 	}
 	return ``, &Error{file, ErrNotFound}
 }
+
+func splitList(path string) []string {
+	// The same implementation is used in SplitList in path/filepath;
+	// consider changing path/filepath when changing this.
+
+	if path == "" {
+		return []string{}
+	}
+
+	// Split path, respecting but preserving quotes.
+	list := []string{}
+	start := 0
+	quo := false
+	for i := 0; i < len(path); i++ {
+		switch c := path[i]; {
+		case c == '"':
+			quo = !quo
+		case c == os.PathListSeparator && !quo:
+			list = append(list, path[start:i])
+			start = i + 1
+		}
+	}
+	list = append(list, path[start:])
+
+	// Remove quotes.
+	for i, s := range list {
+		if strings.Contains(s, `"`) {
+			list[i] = strings.Replace(s, `"`, ``, -1)
+		}
+	}
+
+	return list
+}

src/pkg/path/filepath/path.go

@@ -176,10 +176,7 @@ func FromSlash(path string) string {
 // usually found in PATH or GOPATH environment variables.
 // Unlike strings.Split, SplitList returns an empty slice when passed an empty string.
 func SplitList(path string) []string {
-	if path == "" {
+	return splitList(path)
-		return []string{}
-	}
-	return strings.Split(path, string(ListSeparator))
 }
 
 // Split splits path immediately following the final Separator,

src/pkg/path/filepath/path_plan9.go

@@ -21,3 +21,10 @@ func volumeNameLen(path string) int {
 func HasPrefix(p, prefix string) bool {
 	return strings.HasPrefix(p, prefix)
 }
+
+func splitList(path string) []string {
+	if path == "" {
+		return []string{}
+	}
+	return strings.Split(path, string(ListSeparator))
+}

src/pkg/path/filepath/path_test.go

@@ -148,10 +148,36 @@ var splitlisttests = []SplitListTest{
 	{string([]byte{lsep, 'a', lsep, 'b'}), []string{"", "a", "b"}},
 }
 
+var winsplitlisttests = []SplitListTest{
+	// quoted
+	{`"a"`, []string{`a`}},
+
+	// semicolon
+	{`";"`, []string{`;`}},
+	{`"a;b"`, []string{`a;b`}},
+	{`";";`, []string{`;`, ``}},
+	{`;";"`, []string{``, `;`}},
+
+	// partially quoted
+	{`a";"b`, []string{`a;b`}},
+	{`a; ""b`, []string{`a`, ` b`}},
+	{`"a;b`, []string{`a;b`}},
+	{`""a;b`, []string{`a`, `b`}},
+	{`"""a;b`, []string{`a;b`}},
+	{`""""a;b`, []string{`a`, `b`}},
+	{`a";b`, []string{`a;b`}},
+	{`a;b";c`, []string{`a`, `b;c`}},
+	{`"a";b";c`, []string{`a`, `b;c`}},
+}
+
 func TestSplitList(t *testing.T) {
-	for _, test := range splitlisttests {
+	tests := splitlisttests
+	if runtime.GOOS == "windows" {
+		tests = append(tests, winsplitlisttests...)
+	}
+	for _, test := range tests {
 		if l := filepath.SplitList(test.list); !reflect.DeepEqual(l, test.result) {
-			t.Errorf("SplitList(%q) = %s, want %s", test.list, l, test.result)
+			t.Errorf("SplitList(%#q) = %#q, want %#q", test.list, l, test.result)
 		}
 	}
 }

src/pkg/path/filepath/path_unix.go

@@ -23,3 +23,10 @@ func volumeNameLen(path string) int {
 func HasPrefix(p, prefix string) bool {
 	return strings.HasPrefix(p, prefix)
 }
+
+func splitList(path string) []string {
+	if path == "" {
+		return []string{}
+	}
+	return strings.Split(path, string(ListSeparator))
+}

src/pkg/path/filepath/path_windows.go

@@ -70,3 +70,36 @@ func HasPrefix(p, prefix string) bool {
 	}
 	return strings.HasPrefix(strings.ToLower(p), strings.ToLower(prefix))
 }
+
+func splitList(path string) []string {
+	// The same implementation is used in LookPath in os/exec;
+	// consider changing os/exec when changing this.
+
+	if path == "" {
+		return []string{}
+	}
+
+	// Split path, respecting but preserving quotes.
+	list := []string{}
+	start := 0
+	quo := false
+	for i := 0; i < len(path); i++ {
+		switch c := path[i]; {
+		case c == '"':
+			quo = !quo
+		case c == ListSeparator && !quo:
+			list = append(list, path[start:i])
+			start = i + 1
+		}
+	}
+	list = append(list, path[start:])
+
+	// Remove quotes.
+	for i, s := range list {
+		if strings.Contains(s, `"`) {
+			list[i] = strings.Replace(s, `"`, ``, -1)
+		}
+	}
+
+	return list
+}

src/pkg/path/filepath/path_windows_test.go

@@ -0,0 +1,89 @@
+package filepath_test
+
+import (
+	"io/ioutil"
+	"os"
+	"os/exec"
+	"path/filepath"
+	"reflect"
+	"testing"
+)
+
+func TestWinSplitListTestsAreValid(t *testing.T) {
+	comspec := os.Getenv("ComSpec")
+	if comspec == "" {
+		t.Fatal("%ComSpec% must be set")
+	}
+
+	for ti, tt := range winsplitlisttests {
+		testWinSplitListTestIsValid(t, ti, tt, comspec)
+	}
+}
+
+func testWinSplitListTestIsValid(t *testing.T, ti int, tt SplitListTest,
+	comspec string) {
+
+	const (
+		cmdfile             = `printdir.cmd`
+		perm    os.FileMode = 0700
+	)
+
+	tmp, err := ioutil.TempDir("", "testWinSplitListTestIsValid")
+	if err != nil {
+		t.Fatalf("TempDir failed: %v", err)
+	}
+	defer os.RemoveAll(tmp)
+
+	for i, d := range tt.result {
+		if d == "" {
+			continue
+		}
+		if cd := filepath.Clean(d); filepath.VolumeName(cd) != "" ||
+			cd[0] == '\\' || cd == ".." || (len(cd) >= 3 && cd[0:3] == `..\`) {
+			t.Errorf("%d,%d: %#q refers outside working directory", ti, i, d)
+			return
+		}
+		dd := filepath.Join(tmp, d)
+		if _, err := os.Stat(dd); err == nil {
+			t.Errorf("%d,%d: %#q already exists", ti, i, d)
+			return
+		}
+		if err = os.MkdirAll(dd, perm); err != nil {
+			t.Errorf("%d,%d: MkdirAll(%#q) failed: %v", ti, i, dd, err)
+			return
+		}
+		fn, data := filepath.Join(dd, cmdfile), []byte("@echo "+d+"\r\n")
+		if err = ioutil.WriteFile(fn, data, perm); err != nil {
+			t.Errorf("%d,%d: WriteFile(%#q) failed: %v", ti, i, fn, err)
+			return
+		}
+	}
+
+	for i, d := range tt.result {
+		if d == "" {
+			continue
+		}
+		exp := []byte(d + "\r\n")
+		cmd := &exec.Cmd{
+			Path: comspec,
+			Args: []string{`/c`, cmdfile},
+			Env:  []string{`Path=` + tt.list},
+			Dir:  tmp,
+		}
+		out, err := cmd.Output()
+		switch {
+		case err != nil:
+			t.Errorf("%d,%d: execution error %v", ti, i, err)
+			return
+		case !reflect.DeepEqual(out, exp):
+			t.Errorf("%d,%d: expected %#q, got %#q", ti, i, exp, out)
+			return
+		default:
+			// unshadow cmdfile in next directory
+			err = os.Remove(filepath.Join(tmp, d, cmdfile))
+			if err != nil {
+				t.Fatalf("Remove test command failed: %v", err)
+			}
+		}
+	}
+}