go/build: fix cgo ${SRCDIR} substitution when that variable contains spaces

When the source directory path contains spaces, cgo directives
cannot be properly validated:

$ pwd
/root/src/issue 11868

$ cat main.go
package main
//#cgo CFLAGS: -I${SRCDIR}/../../include
import "C"
func main() {
}

$ go build
can't load package: package issue 11868: /root/src/issue 11868/main.go:
 malformed #cgo argument: -I/root/src/issue 11868/../../include

Make sure spaces are tolerated in ${SRCDIR} when this variable
is expanded. This applies to ${SRCDIR} only. Shell safety
checks are still done in the same exact way for anything else.

Fixes #11868

Change-Id: I93d1d2b5ab167caa7ae353fe46fb8f69f1f06969
Reviewed-on: https://go-review.googlesource.com/16302
Reviewed-by: Russ Cox <rsc@golang.org>

src/go/build/build.go

@@ -1127,9 +1127,9 @@ func (ctxt *Context) saveCgo(filename string, di *Package, cg *ast.CommentGroup)
 		if err != nil {
 			return fmt.Errorf("%s: invalid #cgo line: %s", filename, orig)
 		}
+		var ok bool
 		for i, arg := range args {
-			arg = expandSrcDir(arg, di.Dir)
-			if !safeCgoName(arg) {
+			if arg, ok = expandSrcDir(arg, di.Dir); !ok {
 				return fmt.Errorf("%s: malformed #cgo argument: %s", filename, arg)
 			}
 			args[i] = arg
@@ -1153,25 +1153,46 @@ func (ctxt *Context) saveCgo(filename string, di *Package, cg *ast.CommentGroup)
 	return nil
 }
 
-func expandSrcDir(str string, srcdir string) string {
+// expandSrcDir expands any occurrence of ${SRCDIR}, making sure
+// the result is safe for the shell.
+func expandSrcDir(str string, srcdir string) (string, bool) {
 	// "\" delimited paths cause safeCgoName to fail
 	// so convert native paths with a different delimeter
-	// to "/" before starting (eg: on windows)
+	// to "/" before starting (eg: on windows).
 	srcdir = filepath.ToSlash(srcdir)
-	return strings.Replace(str, "${SRCDIR}", srcdir, -1)
+
+	// Spaces are tolerated in ${SRCDIR}, but not anywhere else.
+	chunks := strings.Split(str, "${SRCDIR}")
+	if len(chunks) < 2 {
+		return str, safeCgoName(str, false)
+	}
+	ok := true
+	for _, chunk := range chunks {
+		ok = ok && (chunk == "" || safeCgoName(chunk, false))
+	}
+	ok = ok && (srcdir == "" || safeCgoName(srcdir, true))
+	res := strings.Join(chunks, srcdir)
+	return res, ok && res != ""
 }
 
 // NOTE: $ is not safe for the shell, but it is allowed here because of linker options like -Wl,$ORIGIN.
 // We never pass these arguments to a shell (just to programs we construct argv for), so this should be okay.
 // See golang.org/issue/6038.
-var safeBytes = []byte("+-.,/0123456789=ABCDEFGHIJKLMNOPQRSTUVWXYZ_abcdefghijklmnopqrstuvwxyz:$")
+const safeString = "+-.,/0123456789=ABCDEFGHIJKLMNOPQRSTUVWXYZ_abcdefghijklmnopqrstuvwxyz:$"
+const safeSpaces = " "
 
-func safeCgoName(s string) bool {
+var safeBytes = []byte(safeSpaces + safeString)
+
+func safeCgoName(s string, spaces bool) bool {
 	if s == "" {
 		return false
 	}
+	safe := safeBytes
+	if !spaces {
+		safe = safe[len(safeSpaces):]
+	}
 	for i := 0; i < len(s); i++ {
-		if c := s[i]; c < 0x80 && bytes.IndexByte(safeBytes, c) < 0 {
+		if c := s[i]; c < 0x80 && bytes.IndexByte(safe, c) < 0 {
 			return false
 		}
 	}

src/go/build/build_test.go

@@ -267,7 +267,7 @@ var expandSrcDirTests = []struct {
 
 func TestExpandSrcDir(t *testing.T) {
 	for _, test := range expandSrcDirTests {
-		output := expandSrcDir(test.input, expandSrcDirPath)
+		output, _ := expandSrcDir(test.input, expandSrcDirPath)
 		if output != test.expected {
 			t.Errorf("%q expands to %q with SRCDIR=%q when %q is expected", test.input, output, expandSrcDirPath, test.expected)
 		} else {
@@ -275,3 +275,25 @@ func TestExpandSrcDir(t *testing.T) {
 		}
 	}
 }
+
+func TestShellSafety(t *testing.T) {
+	tests := []struct {
+		input, srcdir, expected string
+		result                  bool
+	}{
+		{"-I${SRCDIR}/../include", "/projects/src/issue 11868", "-I/projects/src/issue 11868/../include", true},
+		{"-X${SRCDIR}/1,${SRCDIR}/2", "/projects/src/issue 11868", "-X/projects/src/issue 11868/1,/projects/src/issue 11868/2", true},
+		{"-I/tmp -I/tmp", "/tmp2", "-I/tmp -I/tmp", false},
+		{"-I/tmp", "/tmp/[0]", "-I/tmp", true},
+		{"-I${SRCDIR}/dir", "/tmp/[0]", "-I/tmp/[0]/dir", false},
+	}
+	for _, test := range tests {
+		output, ok := expandSrcDir(test.input, test.srcdir)
+		if ok != test.result {
+			t.Errorf("Expected %t while %q expands to %q with SRCDIR=%q; got %t", test.result, test.input, output, test.srcdir, ok)
+		}
+		if output != test.expected {
+			t.Errorf("Expected %q while %q expands with SRCDIR=%q; got %q", test.expected, test.input, test.srcdir, output)
+		}
+	}
+}