go/types: calls setScopePos on dot-imported objects

[dominikh]

When dot importing a package, the type checker adds the exported objects of the imported package to the importing file's scope (go/types/resolver.go:298 as of 1ab063c) by calling Checker.declare. declare calls object.setScopePos, which sets the object's scopePos_ field unconditionally. Not only does this seem inherently wrong, changing the object's original scopePos_, it is also racy when loading multiple packages in parallel. This issue seems to have been introduced in d63c42d.

Since the only use of scopePos_ is in the following comparison

scope.go:		if obj := s.elems[name]; obj != nil && (!pos.IsValid() || obj.scopePos() <= pos) {

it seems unlikely that this will have caused any incorrect computations. This makes writing a hermetic minimal reproducible example difficult. However, the race condition is real. The following example exhibits a race in go/types due to setting scopePos_:

package main

import (
	"golang.org/x/tools/go/packages"
)

func main() {
	cfg := &packages.Config{
		Mode: packages.LoadAllSyntax,
		Tests: true,
	}

	for {
		packages.Load(cfg, "sandbox/bar")
	}
}

Where sandbox/bar is a package in GOPATH, as follows:

$ cat bar.go
package pkg

import . "fmt"

var _ = Println

$ cat bar_test.go 
package pkg

import . "fmt"

var _ = Println

this first loads fmt (and its transitive dependencies). It then loads sandbox/bar twice (the normal package and the test package) in parallel, which will cause the racy write of scopePos_:

==================
WARNING: DATA RACE
Write at 0x00c006961850 by goroutine 14:
  go/types.(*object).setScopePos()
      /usr/lib/go/src/go/types/object.go:155 +0x3e
  go/types.(*Func).setScopePos()
      <autogenerated>:1 +0x4d
  go/types.(*Checker).declare()
      /usr/lib/go/src/go/types/decl.go:33 +0xc1
  go/types.(*Checker).collectObjects()
      /usr/lib/go/src/go/types/resolver.go:312 +0x2b14
  go/types.(*Checker).checkFiles()
      /usr/lib/go/src/go/types/check.go:252 +0xa4
  golang.org/x/tools/go/packages.(*loader).loadPackage()
      /usr/lib/go/src/go/types/check.go:245 +0xcad
  golang.org/x/tools/go/packages.(*loader).loadRecursive.func1()
      /home/dominikh/prj/src/golang.org/x/tools/go/packages/packages.go:683 +0x258
  sync.(*Once).Do()
      /usr/lib/go/src/sync/once.go:44 +0xde
  golang.org/x/tools/go/packages.(*loader).loadRecursive()
      /home/dominikh/prj/src/golang.org/x/tools/go/packages/packages.go:670 +0x74
  golang.org/x/tools/go/packages.(*loader).refine.func2()
      /home/dominikh/prj/src/golang.org/x/tools/go/packages/packages.go:606 +0x42

Previous write at 0x00c006961850 by goroutine 13:
  go/types.(*object).setScopePos()
      /usr/lib/go/src/go/types/object.go:155 +0x3e
  go/types.(*Func).setScopePos()
      <autogenerated>:1 +0x4d
  go/types.(*Checker).declare()
      /usr/lib/go/src/go/types/decl.go:33 +0xc1
  go/types.(*Checker).collectObjects()
      /usr/lib/go/src/go/types/resolver.go:312 +0x2b14
  go/types.(*Checker).checkFiles()
      /usr/lib/go/src/go/types/check.go:252 +0xa4
  golang.org/x/tools/go/packages.(*loader).loadPackage()
      /usr/lib/go/src/go/types/check.go:245 +0xcad
  golang.org/x/tools/go/packages.(*loader).loadRecursive.func1()
      /home/dominikh/prj/src/golang.org/x/tools/go/packages/packages.go:683 +0x258
  sync.(*Once).Do()
      /usr/lib/go/src/sync/once.go:44 +0xde
  golang.org/x/tools/go/packages.(*loader).loadRecursive()
      /home/dominikh/prj/src/golang.org/x/tools/go/packages/packages.go:670 +0x74
  golang.org/x/tools/go/packages.(*loader).refine.func2()
      /home/dominikh/prj/src/golang.org/x/tools/go/packages/packages.go:606 +0x42

Goroutine 14 (running) created at:
  golang.org/x/tools/go/packages.(*loader).refine()
      /home/dominikh/prj/src/golang.org/x/tools/go/packages/packages.go:605 +0x2185
  golang.org/x/tools/go/packages.Load()
      /home/dominikh/prj/src/golang.org/x/tools/go/packages/packages.go:224 +0x1b8
  main.main()
      /tmp/foo.go:14 +0xc4

Goroutine 13 (running) created at:
  golang.org/x/tools/go/packages.(*loader).refine()
      /home/dominikh/prj/src/golang.org/x/tools/go/packages/packages.go:605 +0x2185
  golang.org/x/tools/go/packages.Load()
      /home/dominikh/prj/src/golang.org/x/tools/go/packages/packages.go:224 +0x1b8
  main.main()
      /tmp/foo.go:14 +0xc4
==================

I'm marking this for Go 1.13 as it makes thread-safe use of go/types difficult (or at least inefficient). Go tool development has picked up speed thanks to go/packages and gopls, and it'd be nice if we didn't have a race in the stdlib for the next 6 months (even though it has existed for years, and can also be exercised with the old go/loader package in a similar manner.)

/cc @griesemer as the owner of go/types
/cc @ianthehat in case he's interested.

[griesemer]

Marking release-blocker so it gets looked at soon.

[gopherbot]

Change https://golang.org/cl/183849 mentions this issue: go/types: avoid race condition with dot-imported objects

[griesemer]

Thanks, @dominikh for the detailed analysis.