cmd/go/internal/generate: stop premature variable substitution in com…

…mands go:generate commands passed no arguments are currently subject to premature variable substitution due to mistakenly assuming append guarantees a copy. The change fixes this by forcing a slice copy at each invocation of a command. The previous code assumed that append would always generate a copy of its inputs. However, append wouldn't create a copy if there was no need to increase capacity and it would just return the original input slice. This resulted in premature variable substitutions in the "master word list" of generate commands, thus yielding incorrect results across multiple invocations of the same command when the body contained substitutions e.g. environment variables, moreover these can change during the lifetime of go:generate processing a file. Note that this behavior would not manifest itself if any arguments were passed to the command, because append would make a copy of the slice as it needed to increase its capacity. The "hacky" work-around was to always pass at least one argument to any command, even if the command ignores it. e.g., //go:generate MyNoArgsCmd ' ' This CL fixes that issue and removes the need for the hack mentioned above. Fixes #31608 Change-Id: I782ac2234bd7035a37f61c101ee4aee38ed8d29f GitHub-Last-Rev: 796d343 GitHub-Pull-Request: #31527 Reviewed-on: https://go-review.googlesource.com/c/go/+/172580 Run-TryBot: Jay Conrod <[email protected]> TryBot-Result: Gobot Gobot <[email protected]> Reviewed-by: Jay Conrod <[email protected]>
golang · Apr 22, 2019 · e6ae4e8 · e6ae4e8
1 parent 43001a0
commit e6ae4e8
Show file tree

Hide file tree

Showing 2 changed files with 204 additions and 1 deletion.
diff --git a/src/cmd/go/internal/generate/generate.go b/src/cmd/go/internal/generate/generate.go
@@ -374,7 +374,12 @@ Words:
 	// Substitute command if required.
 	if len(words) > 0 && g.commands[words[0]] != nil {
 		// Replace 0th word by command substitution.
-		words = append(g.commands[words[0]], words[1:]...)
+		//
+		// Force a copy of the command definition to
+		// ensure words doesn't end up as a reference
+		// to the g.commands content.
+		tmpCmdWords := append([]string(nil), (g.commands[words[0]])...)
+		words = append(tmpCmdWords, words[1:]...)
 	}
 	// Substitute environment variables.
 	for i, word := range words {

diff --git a/src/cmd/go/internal/generate/generate_test.go b/src/cmd/go/internal/generate/generate_test.go
@@ -5,6 +5,7 @@
 package generate
 
 import (
+	"os"
 	"reflect"
 	"runtime"
 	"testing"
@@ -15,6 +16,15 @@ type splitTest struct {
 	out []string
 }
 
+// Same as above, except including source line number to set
+type splitTestWithLine struct {
+	in         string
+	out        []string
+	lineNumber int
+}
+
+const anyLineNo = 0
+
 var splitTests = []splitTest{
 	{"", nil},
 	{"x", []string{"x"}},
@@ -54,3 +64,191 @@ func TestGenerateCommandParse(t *testing.T) {
 		}
 	}
 }
+
+// These environment variables will be undefined before the splitTestWithLine tests
+var undefEnvList = []string{
+	"_XYZZY_",
+}
+
+// These environment variables will be defined before the splitTestWithLine tests
+var defEnvMap = map[string]string{
+	"_PLUGH_": "SomeVal",
+	"_X":      "Y",
+}
+
+// TestGenerateCommandShortHand - similar to TestGenerateCommandParse,
+// except:
+//    1. if the result starts with -command, record that shorthand
+//       before moving on to the next test.
+//    2. If a source line number is specified, set that in the parser
+//       before executing the test.  i.e., execute the split as if it
+//       processing that source line.
+func TestGenerateCommandShorthand(t *testing.T) {
+	g := &Generator{
+		r:        nil, // Unused here.
+		path:     "/usr/ken/sys/proc.go",
+		dir:      "/usr/ken/sys",
+		file:     "proc.go",
+		pkg:      "sys",
+		commands: make(map[string][]string),
+	}
+
+	var inLine string
+	var expected, got []string
+
+	g.setEnv()
+
+	// Set up the system environment variables
+	for i := range undefEnvList {
+		os.Unsetenv(undefEnvList[i])
+	}
+	for k := range defEnvMap {
+		os.Setenv(k, defEnvMap[k])
+	}
+
+	// simple command from environment variable
+	inLine = "//go:generate -command CMD0 \"ab${_X}cd\""
+	expected = []string{"-command", "CMD0", "abYcd"}
+	got = g.split(inLine + "\n")
+
+	if !reflect.DeepEqual(got, expected) {
+		t.Errorf("split(%q): got %q expected %q", inLine, got, expected)
+	}
+
+	// try again, with an extra level of indirection (should leave variable in command)
+	inLine = "//go:generate -command CMD0 \"ab${DOLLAR}{_X}cd\""
+	expected = []string{"-command", "CMD0", "ab${_X}cd"}
+	got = g.split(inLine + "\n")
+
+	if !reflect.DeepEqual(got, expected) {
+		t.Errorf("split(%q): got %q expected %q", inLine, got, expected)
+	}
+
+	// Now the interesting part, record that output as a command
+	g.setShorthand(got)
+
+	// see that the command still substitutes correctly from env. variable
+	inLine = "//go:generate CMD0"
+	expected = []string{"abYcd"}
+	got = g.split(inLine + "\n")
+
+	if !reflect.DeepEqual(got, expected) {
+		t.Errorf("split(%q): got %q expected %q", inLine, got, expected)
+	}
+
+	// Now change the value of $X and see if the recorded definition is
+	// still intact (vs. having the $_X already substituted out)
+
+	os.Setenv("_X", "Z")
+	inLine = "//go:generate CMD0"
+	expected = []string{"abZcd"}
+	got = g.split(inLine + "\n")
+
+	if !reflect.DeepEqual(got, expected) {
+		t.Errorf("split(%q): got %q expected %q", inLine, got, expected)
+	}
+
+	// What if the variable is now undefined?  Should be empty substitution.
+
+	os.Unsetenv("_X")
+	inLine = "//go:generate CMD0"
+	expected = []string{"abcd"}
+	got = g.split(inLine + "\n")
+
+	if !reflect.DeepEqual(got, expected) {
+		t.Errorf("split(%q): got %q expected %q", inLine, got, expected)
+	}
+
+	// Try another undefined variable as an extra check
+	os.Unsetenv("_Z")
+	inLine = "//go:generate -command CMD1 \"ab${_Z}cd\""
+	expected = []string{"-command", "CMD1", "abcd"}
+	got = g.split(inLine + "\n")
+
+	if !reflect.DeepEqual(got, expected) {
+		t.Errorf("split(%q): got %q expected %q", inLine, got, expected)
+	}
+
+	g.setShorthand(got)
+
+	inLine = "//go:generate CMD1"
+	expected = []string{"abcd"}
+	got = g.split(inLine + "\n")
+
+	if !reflect.DeepEqual(got, expected) {
+		t.Errorf("split(%q): got %q expected %q", inLine, got, expected)
+	}
+
+	const val = "someNewValue"
+	os.Setenv("_Z", val)
+
+	// try again with the properly-escaped variable.
+
+	inLine = "//go:generate -command CMD2 \"ab${DOLLAR}{_Z}cd\""
+	expected = []string{"-command", "CMD2", "ab${_Z}cd"}
+	got = g.split(inLine + "\n")
+
+	if !reflect.DeepEqual(got, expected) {
+		t.Errorf("split(%q): got %q expected %q", inLine, got, expected)
+	}
+
+	g.setShorthand(got)
+
+	inLine = "//go:generate CMD2"
+	expected = []string{"ab" + val + "cd"}
+	got = g.split(inLine + "\n")
+
+	if !reflect.DeepEqual(got, expected) {
+		t.Errorf("split(%q): got %q expected %q", inLine, got, expected)
+	}
+}
+
+// Command-related tests for TestGenerateCommandShortHand2
+// -- Note line numbers included to check substitutions from "build-in" variable - $GOLINE
+var splitTestsLines = []splitTestWithLine{
+	{"-command TEST1 $GOLINE", []string{"-command", "TEST1", "22"}, 22},
+	{"-command TEST2 ${DOLLAR}GOLINE", []string{"-command", "TEST2", "$GOLINE"}, 26},
+	{"TEST1", []string{"22"}, 33},
+	{"TEST2", []string{"66"}, 66},
+	{"TEST1 ''", []string{"22", "''"}, 99},
+	{"TEST2 ''", []string{"44", "''"}, 44},
+}
+
+// TestGenerateCommandShortHand - similar to TestGenerateCommandParse,
+// except:
+//    1. if the result starts with -command, record that shorthand
+//       before moving on to the next test.
+//    2. If a source line number is specified, set that in the parser
+//       before executing the test.  i.e., execute the split as if it
+//       processing that source line.
+func TestGenerateCommandShortHand2(t *testing.T) {
+	g := &Generator{
+		r:        nil, // Unused here.
+		path:     "/usr/ken/sys/proc.go",
+		dir:      "/usr/ken/sys",
+		file:     "proc.go",
+		pkg:      "sys",
+		commands: make(map[string][]string),
+	}
+	g.setEnv()
+	for _, test := range splitTestsLines {
+		// if the test specified a line number, reflect that
+		if test.lineNumber != anyLineNo {
+			g.lineNum = test.lineNumber
+			g.setEnv()
+		}
+		// First with newlines.
+		got := g.split("//go:generate " + test.in + "\n")
+		if !reflect.DeepEqual(got, test.out) {
+			t.Errorf("split(%q): got %q expected %q", test.in, got, test.out)
+		}
+		// Then with CRLFs, thank you Windows.
+		got = g.split("//go:generate " + test.in + "\r\n")
+		if !reflect.DeepEqual(got, test.out) {
+			t.Errorf("split(%q): got %q expected %q", test.in, got, test.out)
+		}
+		if got[0] == "-command" { // record commands
+			g.setShorthand(got)
+		}
+	}
+}