go/types,types2: avoid data race to object.color_ through dot imports

As described in issue #69912, type checking dot-imported identifiers can
result in a call to objDecl on an imported object, which leads to a data
race to the color_ field.

There are multiple potential fixes for this race. Opt for avoiding the
call to objDecl altogether, rather than setting color_ during import.
The color_ field is an internal property of objects that should only be
valid during the type checking of their package. We should not be
calling objDecl on imported objects.

Fixes #69912

Change-Id: I55eb652479715f2a7ac84104db2f448091c4e7ac
Reviewed-on: https://go-review.googlesource.com/c/go/+/621637
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Robert Findley <rfindley@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>

diff --git a/src/cmd/compile/internal/importer/gcimporter_test.go b/src/cmd/compile/internal/importer/gcimporter_test.go
index 9235e4bb95612a0b3d918c6b2bb83451b025e250..a202ee10dec2bb019dd09cc12ded2fc3d55ab9e3 100644 (file)
--- a/src/cmd/compile/internal/importer/gcimporter_test.go
+++ b/src/cmd/compile/internal/importer/gcimporter_test.go
@@ -6,6 +6,7 @@ package importer
 
 import (
        "bytes"
+       "cmd/compile/internal/syntax"
        "cmd/compile/internal/types2"
        "fmt"
        "go/build"
@@ -16,6 +17,7 @@ import (
        "path/filepath"
        "runtime"
        "strings"
+       "sync"
        "testing"
        "time"
 )
@@ -593,3 +595,66 @@ func lookupObj(t *testing.T, scope *types2.Scope, name string) types2.Object {
        t.Fatalf("%s not found", name)
        return nil
 }
+
+// importMap implements the types2.Importer interface.
+type importMap map[string]*types2.Package
+
+func (m importMap) Import(path string) (*types2.Package, error) { return m[path], nil }
+
+func TestIssue69912(t *testing.T) {
+       testenv.MustHaveGoBuild(t)
+
+       // This package only handles gc export data.
+       if runtime.Compiler != "gc" {
+               t.Skipf("gc-built packages not available (compiler = %s)", runtime.Compiler)
+       }
+
+       tmpdir := t.TempDir()
+       testoutdir := filepath.Join(tmpdir, "testdata")
+       if err := os.Mkdir(testoutdir, 0700); err != nil {
+               t.Fatalf("making output dir: %v", err)
+       }
+
+       compile(t, "testdata", "issue69912.go", testoutdir, nil)
+
+       issue69912, err := Import(make(map[string]*types2.Package), "./testdata/issue69912", tmpdir, nil)
+       if err != nil {
+               t.Fatal(err)
+       }
+
+       check := func(pkgname, src string, imports importMap) (*types2.Package, error) {
+               f, err := syntax.Parse(syntax.NewFileBase(pkgname), strings.NewReader(src), nil, nil, 0)
+               if err != nil {
+                       return nil, err
+               }
+               config := &types2.Config{
+                       Importer: imports,
+               }
+               return config.Check(pkgname, []*syntax.File{f}, nil)
+       }
+
+       // Use the resulting package concurrently, via dot-imports, to exercise the
+       // race of issue #69912.
+       const pSrc = `package p
+
+import . "issue69912"
+
+type S struct {
+       f T
+}
+`
+       importer := importMap{
+               "issue69912": issue69912,
+       }
+       var wg sync.WaitGroup
+       for range 10 {
+               wg.Add(1)
+               go func() {
+                       defer wg.Done()
+                       if _, err := check("p", pSrc, importer); err != nil {
+                               t.Errorf("Check failed: %v", err)
+                       }
+               }()
+       }
+       wg.Wait()
+}

diff --git a/src/cmd/compile/internal/importer/testdata/issue69912.go b/src/cmd/compile/internal/importer/testdata/issue69912.go
new file mode 100644 (file)
index 0000000..7c60354
--- /dev/null
+++ b/src/cmd/compile/internal/importer/testdata/issue69912.go
@@ -0,0 +1,9 @@
+// Copyright 2024 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package issue69912
+
+// Define an arbitrary type name, which will be used to demonstrate
+// the race of issue #69912.
+type T int

diff --git a/src/cmd/compile/internal/types2/typexpr.go b/src/cmd/compile/internal/types2/typexpr.go
index 61ef835c8ad5904c5f71adad72c4476e548e17a0..10af5e79aaf6bedf1fe484b0077fb8bf1643d6aa 100644 (file)
--- a/src/cmd/compile/internal/types2/typexpr.go
+++ b/src/cmd/compile/internal/types2/typexpr.go
@@ -63,13 +63,16 @@ func (check *Checker) ident(x *operand, e *syntax.Name, def *TypeName, wantType
        // Type-check the object.
        // Only call Checker.objDecl if the object doesn't have a type yet
        // (in which case we must actually determine it) or the object is a
-       // TypeName and we also want a type (in which case we might detect
-       // a cycle which needs to be reported). Otherwise we can skip the
-       // call and avoid a possible cycle error in favor of the more
-       // informative "not a type/value" error that this function's caller
-       // will issue (see go.dev/issue/25790).
+       // TypeName from the current package and we also want a type (in which case
+       // we might detect a cycle which needs to be reported). Otherwise we can skip
+       // the call and avoid a possible cycle error in favor of the more informative
+       // "not a type/value" error that this function's caller will issue (see
+       // go.dev/issue/25790).
+       //
+       // Note that it is important to avoid calling objDecl on objects from other
+       // packages, to avoid races: see issue #69912.
        typ := obj.Type()
-       if typ == nil || gotType && wantType {
+       if typ == nil || (gotType && wantType && obj.Pkg() == check.pkg) {
                check.objDecl(obj, def)
                typ = obj.Type() // type must have been assigned by Checker.objDecl
        }

diff --git a/src/go/internal/gcimporter/gcimporter_test.go b/src/go/internal/gcimporter/gcimporter_test.go
index 3f6c7fb0d6196568f6f34f68a1377504b2219b69..11bd22d71760e90ddff7b1aeb829989b87afc2dd 100644 (file)
--- a/src/go/internal/gcimporter/gcimporter_test.go
+++ b/src/go/internal/gcimporter/gcimporter_test.go
@@ -14,6 +14,7 @@ import (
        "path/filepath"
        "runtime"
        "strings"
+       "sync"
        "testing"
        "time"
 
@@ -750,3 +751,68 @@ func lookupObj(t *testing.T, scope *types.Scope, name string) types.Object {
        t.Fatalf("%s not found", name)
        return nil
 }
+
+// importMap implements the types.Importer interface.
+type importMap map[string]*types.Package
+
+func (m importMap) Import(path string) (*types.Package, error) { return m[path], nil }
+
+func TestIssue69912(t *testing.T) {
+       testenv.MustHaveGoBuild(t)
+
+       // This package only handles gc export data.
+       if runtime.Compiler != "gc" {
+               t.Skipf("gc-built packages not available (compiler = %s)", runtime.Compiler)
+       }
+
+       tmpdir := t.TempDir()
+       testoutdir := filepath.Join(tmpdir, "testdata")
+       if err := os.Mkdir(testoutdir, 0700); err != nil {
+               t.Fatalf("making output dir: %v", err)
+       }
+
+       compile(t, "testdata", "issue69912.go", testoutdir, nil)
+
+       fset := token.NewFileSet()
+
+       issue69912, err := Import(fset, make(map[string]*types.Package), "./testdata/issue69912", tmpdir, nil)
+       if err != nil {
+               t.Fatal(err)
+       }
+
+       check := func(pkgname, src string, imports importMap) (*types.Package, error) {
+               f, err := parser.ParseFile(fset, "a.go", src, 0)
+               if err != nil {
+                       return nil, err
+               }
+               config := &types.Config{
+                       Importer: imports,
+               }
+               return config.Check(pkgname, fset, []*ast.File{f}, nil)
+       }
+
+       // Use the resulting package concurrently, via dot-imports, to exercise the
+       // race of issue #69912.
+       const pSrc = `package p
+
+import . "issue69912"
+
+type S struct {
+       f T
+}
+`
+       importer := importMap{
+               "issue69912": issue69912,
+       }
+       var wg sync.WaitGroup
+       for range 10 {
+               wg.Add(1)
+               go func() {
+                       defer wg.Done()
+                       if _, err := check("p", pSrc, importer); err != nil {
+                               t.Errorf("Check failed: %v", err)
+                       }
+               }()
+       }
+       wg.Wait()
+}

diff --git a/src/go/internal/gcimporter/testdata/issue69912.go b/src/go/internal/gcimporter/testdata/issue69912.go
new file mode 100644 (file)
index 0000000..7c60354
--- /dev/null
+++ b/src/go/internal/gcimporter/testdata/issue69912.go
@@ -0,0 +1,9 @@
+// Copyright 2024 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package issue69912
+
+// Define an arbitrary type name, which will be used to demonstrate
+// the race of issue #69912.
+type T int

diff --git a/src/go/types/typexpr.go b/src/go/types/typexpr.go
index c8514603b43c50c1b38056bf493267f1aa4276e5..42c2f2ed988e97b650c146870296127b7559ff07 100644 (file)
--- a/src/go/types/typexpr.go
+++ b/src/go/types/typexpr.go
@@ -63,13 +63,16 @@ func (check *Checker) ident(x *operand, e *ast.Ident, def *TypeName, wantType bo
        // Type-check the object.
        // Only call Checker.objDecl if the object doesn't have a type yet
        // (in which case we must actually determine it) or the object is a
-       // TypeName and we also want a type (in which case we might detect
-       // a cycle which needs to be reported). Otherwise we can skip the
-       // call and avoid a possible cycle error in favor of the more
-       // informative "not a type/value" error that this function's caller
-       // will issue (see go.dev/issue/25790).
+       // TypeName from the current package and we also want a type (in which case
+       // we might detect a cycle which needs to be reported). Otherwise we can skip
+       // the call and avoid a possible cycle error in favor of the more informative
+       // "not a type/value" error that this function's caller will issue (see
+       // go.dev/issue/25790).
+       //
+       // Note that it is important to avoid calling objDecl on objects from other
+       // packages, to avoid races: see issue #69912.
        typ := obj.Type()
-       if typ == nil || gotType && wantType {
+       if typ == nil || (gotType && wantType && obj.Pkg() == check.pkg) {
                check.objDecl(obj, def)
                typ = obj.Type() // type must have been assigned by Checker.objDecl
        }