From d22086ef5e921ee416e929d693f237971569869e Mon Sep 17 00:00:00 2001 From: "Bryan C. Mills" Date: Thu, 26 Mar 2020 23:10:32 -0400 Subject: [PATCH] cmd/go/internal/work: disallow testgo binary from installing to GOROOT Installing to GOROOT makes tests non-parallelizable, since each test depends on the installed contents of GOROOT already being up-to-date and may reasonably assume that those contents do not change over the course of the test. Fixes #37573 Updates #30316 Change-Id: I2afe95ad11347bee3bb7c2d77a657db6d691cf05 Reviewed-on: https://go-review.googlesource.com/c/go/+/225897 Reviewed-by: Michael Matloob --- src/cmd/go/go_test.go | 1 + src/cmd/go/internal/work/exec.go | 49 ++++++++++++++++--- src/cmd/go/internal/work/testgo.go | 33 ++++++++++++- src/cmd/go/script_test.go | 1 + src/cmd/go/testdata/script/README | 1 + src/cmd/go/testdata/script/get_update_all.txt | 4 +- 6 files changed, 79 insertions(+), 10 deletions(-) diff --git a/src/cmd/go/go_test.go b/src/cmd/go/go_test.go index 641cab8ddd..39e387b9e4 100644 --- a/src/cmd/go/go_test.go +++ b/src/cmd/go/go_test.go @@ -199,6 +199,7 @@ func TestMain(m *testing.M) { return strings.TrimSpace(string(out)) } testGOROOT = goEnv("GOROOT") + os.Setenv("TESTGO_GOROOT", testGOROOT) // The whole GOROOT/pkg tree was installed using the GOHOSTOS/GOHOSTARCH // toolchain (installed in GOROOT/pkg/tool/GOHOSTOS_GOHOSTARCH). diff --git a/src/cmd/go/internal/work/exec.go b/src/cmd/go/internal/work/exec.go index d781ad2306..dbe31a6016 100644 --- a/src/cmd/go/internal/work/exec.go +++ b/src/cmd/go/internal/work/exec.go @@ -8,11 +8,6 @@ package work import ( "bytes" - "cmd/go/internal/base" - "cmd/go/internal/cache" - "cmd/go/internal/cfg" - "cmd/go/internal/load" - "cmd/go/internal/str" "encoding/json" "errors" "fmt" @@ -30,6 +25,12 @@ import ( "strings" "sync" "time" + + "cmd/go/internal/base" + "cmd/go/internal/cache" + "cmd/go/internal/cfg" + "cmd/go/internal/load" + "cmd/go/internal/str" ) // actionList returns the list of actions in the dag rooted at root @@ -490,6 +491,10 @@ func (b *Builder) build(a *Action) (err error) { return nil } + if err := allowInstall(a); err != nil { + return err + } + // make target directory dir, _ := filepath.Split(a.Target) if dir != "" { @@ -1192,6 +1197,10 @@ func (b *Builder) link(a *Action) (err error) { return err } + if err := allowInstall(a); err != nil { + return err + } + // make target directory dir, _ := filepath.Split(a.Target) if dir != "" { @@ -1366,6 +1375,10 @@ func (b *Builder) getPkgConfigFlags(p *load.Package) (cflags, ldflags []string, } func (b *Builder) installShlibname(a *Action) error { + if err := allowInstall(a); err != nil { + return err + } + // TODO: BuildN a1 := a.Deps[0] err := ioutil.WriteFile(a.Target, []byte(filepath.Base(a1.Target)+"\n"), 0666) @@ -1416,6 +1429,10 @@ func (b *Builder) linkShared(a *Action) (err error) { } defer b.flushOutput(a) + if err := allowInstall(a); err != nil { + return err + } + if err := b.Mkdir(a.Objdir); err != nil { return err } @@ -1481,8 +1498,12 @@ func BuildInstallFunc(b *Builder, a *Action) (err error) { // advertise it by touching the mtimes (usually the libraries are up // to date). if !a.buggyInstall && !b.IsCmdList { - now := time.Now() - os.Chtimes(a.Target, now, now) + if cfg.BuildN { + b.Showcmd("", "touch %s", a.Target) + } else if err := allowInstall(a); err == nil { + now := time.Now() + os.Chtimes(a.Target, now, now) + } } return nil } @@ -1493,6 +1514,9 @@ func BuildInstallFunc(b *Builder, a *Action) (err error) { a.built = a1.built return nil } + if err := allowInstall(a); err != nil { + return err + } if err := b.Mkdir(a.Objdir); err != nil { return err @@ -1522,6 +1546,13 @@ func BuildInstallFunc(b *Builder, a *Action) (err error) { return b.moveOrCopyFile(a.Target, a1.built, perm, false) } +// allowInstall returns a non-nil error if this invocation of the go command is +// allowed to install a.Target. +// +// (The build of cmd/go running under its own test is forbidden from installing +// to its original GOROOT.) +var allowInstall = func(*Action) error { return nil } + // cleanup removes a's object dir to keep the amount of // on-disk garbage down in a large build. On an operating system // with aggressive buffering, cleaning incrementally like @@ -1685,6 +1716,10 @@ func (b *Builder) installHeader(a *Action) error { return nil } + if err := allowInstall(a); err != nil { + return err + } + dir, _ := filepath.Split(a.Target) if dir != "" { if err := b.Mkdir(dir); err != nil { diff --git a/src/cmd/go/internal/work/testgo.go b/src/cmd/go/internal/work/testgo.go index 3e623c6621..931f49a069 100644 --- a/src/cmd/go/internal/work/testgo.go +++ b/src/cmd/go/internal/work/testgo.go @@ -8,10 +8,41 @@ package work -import "os" +import ( + "cmd/go/internal/cfg" + "cmd/go/internal/search" + "fmt" + "os" + "path/filepath" + "runtime" +) func init() { if v := os.Getenv("TESTGO_VERSION"); v != "" { runtimeVersion = v } + + if testGOROOT := os.Getenv("TESTGO_GOROOT"); testGOROOT != "" { + // Disallow installs to the GOROOT from which testgo was built. + // Installs to other GOROOTs — such as one set explicitly within a test — are ok. + allowInstall = func(a *Action) error { + if cfg.BuildN { + return nil + } + + rel := search.InDir(a.Target, testGOROOT) + if rel == "" { + return nil + } + + callerPos := "" + if _, file, line, ok := runtime.Caller(1); ok { + if shortFile := search.InDir(file, filepath.Join(testGOROOT, "src")); shortFile != "" { + file = shortFile + } + callerPos = fmt.Sprintf("%s:%d: ", file, line) + } + return fmt.Errorf("%stestgo must not write to GOROOT (installing to %s)", callerPos, filepath.Join("GOROOT", rel)) + } + } } diff --git a/src/cmd/go/script_test.go b/src/cmd/go/script_test.go index 87afb6aec8..ebadce867b 100644 --- a/src/cmd/go/script_test.go +++ b/src/cmd/go/script_test.go @@ -126,6 +126,7 @@ func (ts *testScript) setup() { "GOPROXY=" + proxyURL, "GOPRIVATE=", "GOROOT=" + testGOROOT, + "TESTGO_GOROOT=" + testGOROOT, "GOSUMDB=" + testSumDBVerifierKey, "GONOPROXY=", "GONOSUMDB=", diff --git a/src/cmd/go/testdata/script/README b/src/cmd/go/testdata/script/README index f4c92e65ab..e22ddcaf2e 100644 --- a/src/cmd/go/testdata/script/README +++ b/src/cmd/go/testdata/script/README @@ -34,6 +34,7 @@ Scripts also have access to these other environment variables: GOPATH=$WORK/gopath GOPROXY= GOROOT= + TESTGO_GOROOT= HOME=/no-home PATH= TMPDIR=$WORK/tmp diff --git a/src/cmd/go/testdata/script/get_update_all.txt b/src/cmd/go/testdata/script/get_update_all.txt index 1f2f5bf1ab..d0b9860ade 100644 --- a/src/cmd/go/testdata/script/get_update_all.txt +++ b/src/cmd/go/testdata/script/get_update_all.txt @@ -3,5 +3,5 @@ [!net] skip -go get -u .../ -! stderr 'duplicate loads of' # make sure old packages are removed from cache \ No newline at end of file +go get -u -n .../ +! stderr 'duplicate loads of' # make sure old packages are removed from cache -- 2.50.0