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>

diff --git a/src/go/build/build.go b/src/go/build/build.go
index 14a03fc0d4a91573c10d0d4398c5b08ae2fca848..5016405ab5f9aff53ac7307ee68501b83f8a17b6 100644 (file)
--- a/src/go/build/build.go
+++ b/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
                }
        }

diff --git a/src/go/build/build_test.go b/src/go/build/build_test.go
index 92c3fe3764cdf0ececddd1967770ecfd28850349..3b7e312a070272bf7de6a7a3cfddcf3b6a490299 100644 (file)
--- a/src/go/build/build_test.go
+++ b/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)
+               }
+       }
+}