From 8818cc8885526b79dbe14d3bbb6f44a28435ce5c Mon Sep 17 00:00:00 2001 From: Didier Spezia Date: Sun, 25 Oct 2015 20:18:17 +0000 Subject: [PATCH] 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 --- src/go/build/build.go | 37 +++++++++++++++++++++++++++++-------- src/go/build/build_test.go | 24 +++++++++++++++++++++++- 2 files changed, 52 insertions(+), 9 deletions(-) diff --git a/src/go/build/build.go b/src/go/build/build.go index 14a03fc0d4..5016405ab5 100644 --- 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 92c3fe3764..3b7e312a07 100644 --- 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) + } + } +} -- 2.48.1