From 1f7a813c0df1b2a1c1526e54c5ee4aa940c58d14 Mon Sep 17 00:00:00 2001
From: Ariel Mashraki <ariel@mashraki.co.il>
Date: Mon, 14 Oct 2019 00:23:09 +0300
Subject: [PATCH] tools/internal/imports: fix data race in packageInfo

Before this commit, when running imports.Process concurrently, the program
panics with a fatal error due to concurrent map iterations and map writes.

This CL fixes this by adding a copy of the map to the packageInfo structure.

Fixed #34895

Change-Id: If009e6108813f86495c7e20e69739186b8b236d7
Reviewed-on: https://go-review.googlesource.com/c/tools/+/200865
Run-TryBot: Heschi Kreinick <heschi@google.com>
Reviewed-by: Heschi Kreinick <heschi@google.com>
---
 internal/imports/fix.go      | 15 ++++++++---
 internal/imports/fix_test.go | 48 ++++++++++++++++++++++++++++++++++--
 2 files changed, 58 insertions(+), 5 deletions(-)

diff --git a/internal/imports/fix.go b/internal/imports/fix.go
index bcfbb07ed87..177e13ca1e2 100644
--- a/internal/imports/fix.go
+++ b/internal/imports/fix.go
@@ -475,9 +475,9 @@ func (p *pass) assumeSiblingImportsValid() {
 		}
 		for left, rights := range refs {
 			if imp, ok := importsByName[left]; ok {
-				if _, ok := stdlib[imp.ImportPath]; ok {
+				if m, ok := stdlib[imp.ImportPath]; ok {
 					// We have the stdlib in memory; no need to guess.
-					rights = stdlib[imp.ImportPath]
+					rights = copyExports(m)
 				}
 				p.addCandidate(imp, &packageInfo{
 					// no name; we already know it.
@@ -712,9 +712,10 @@ func cmdDebugStr(cmd *exec.Cmd) string {
 
 func addStdlibCandidates(pass *pass, refs references) {
 	add := func(pkg string) {
+		exports := copyExports(stdlib[pkg])
 		pass.addCandidate(
 			&ImportInfo{ImportPath: pkg},
-			&packageInfo{name: path.Base(pkg), exports: stdlib[pkg]})
+			&packageInfo{name: path.Base(pkg), exports: exports})
 	}
 	for left := range refs {
 		if left == "rand" {
@@ -1383,3 +1384,11 @@ type visitFn func(node ast.Node) ast.Visitor
 func (fn visitFn) Visit(node ast.Node) ast.Visitor {
 	return fn(node)
 }
+
+func copyExports(pkg map[string]bool) map[string]bool {
+	m := make(map[string]bool, len(pkg))
+	for k, v := range pkg {
+		m[k] = v
+	}
+	return m
+}
diff --git a/internal/imports/fix_test.go b/internal/imports/fix_test.go
index 8132954af5c..bb9e7694fa3 100644
--- a/internal/imports/fix_test.go
+++ b/internal/imports/fix_test.go
@@ -11,6 +11,7 @@ import (
 	"path/filepath"
 	"runtime"
 	"strings"
+	"sync"
 	"testing"
 
 	"golang.org/x/tools/go/packages/packagestest"
@@ -1680,8 +1681,9 @@ func (t *goimportTest) processNonModule(file string, contents []byte, opts *Opti
 	if opts == nil {
 		opts = &Options{Comments: true, TabIndent: true, TabWidth: 8}
 	}
-	opts.Env = t.env
-	opts.Env.Debug = *testDebug
+	// ProcessEnv is not safe for concurrent use. Make a copy.
+	env := *t.env
+	opts.Env = &env
 	return Process(file, contents, opts)
 }
 
@@ -2539,3 +2541,45 @@ func TestStdLibGetCandidates(t *testing.T) {
 		t.Errorf("expected to find candidate with path: %q, name: %q next in ordered scan of results`", want[wantIdx].wantPkg, want[wantIdx].wantName)
 	}
 }
+
+// Tests #34895: process should not panic on concurrent calls.
+func TestConcurrentProcess(t *testing.T) {
+	testConfig{
+		module: packagestest.Module{
+			Name: "foo.com",
+			Files: fm{
+				"p/first.go": `package foo
+
+func _() {
+	fmt.Println()
+}
+`,
+				"p/second.go": `package foo
+
+import "fmt"
+
+func _() {
+	fmt.Println()
+	imports.Bar() // not imported.
+}
+`,
+			},
+		},
+	}.test(t, func(t *goimportTest) {
+		var (
+			n  = 10
+			wg sync.WaitGroup
+		)
+		wg.Add(n)
+		for i := 0; i < n; i++ {
+			go func() {
+				defer wg.Done()
+				_, err := t.process("foo.com", "p/first.go", nil, nil)
+				if err != nil {
+					t.Fatal(err)
+				}
+			}()
+		}
+		wg.Wait()
+	})
+}