From: Shawn Elliott Date: Mon, 22 Apr 2019 14:45:19 +0000 (+0000) Subject: cmd/go/internal/generate: stop premature variable substitution in commands X-Git-Tag: go1.13beta1~602 X-Git-Url: http://www.git.cypherpunks.su/?a=commitdiff_plain;h=e6ae4e86ad59c45f302a8828e77e6c234307fce4;p=gostls13.git cmd/go/internal/generate: stop premature variable substitution in commands 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: 796d3430191f183c123c450a60b4a7987cc85e20 GitHub-Pull-Request: golang/go#31527 Reviewed-on: https://go-review.googlesource.com/c/go/+/172580 Run-TryBot: Jay Conrod TryBot-Result: Gobot Gobot Reviewed-by: Jay Conrod --- diff --git a/src/cmd/go/internal/generate/generate.go b/src/cmd/go/internal/generate/generate.go index 38c8274b40..19597c7a33 100644 --- 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 index defc15387f..b546218a3c 100644 --- 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) + } + } +}