From 63c6481f3be3d4c29183574fa76516c4e7f54c6e Mon Sep 17 00:00:00 2001 From: Alan Donovan Date: Mon, 19 Jun 2017 13:33:23 -0400 Subject: [PATCH] go/loader: fix a data race The loader was calling (*types.Checker).Files on the "unsafe" package, a global variable. Even with zero files, this operation is not a no-op because it sets the package's "complete" flag, leading to a data race. (Because Unsafe.complete is already set at construction, the race is benign, but is reported by -race nonetheless.) Fixes golang/go#20718 Change-Id: I5a4f95be5ab4c60ea3b6c2a7fb6f1b67acbf42bc Reviewed-on: https://go-review.googlesource.com/46071 Reviewed-by: Brad Fitzpatrick --- go/loader/loader.go | 18 +++++++++++------- go/loader/loader_test.go | 14 ++++++++++++++ 2 files changed, 25 insertions(+), 7 deletions(-) diff --git a/go/loader/loader.go b/go/loader/loader.go index f69a6684..1cd4d6eb 100644 --- a/go/loader/loader.go +++ b/go/loader/loader.go @@ -1009,15 +1009,19 @@ func (imp *importer) addFiles(info *PackageInfo, files []*ast.File, cycleCheck b time.Since(imp.start), info.Pkg.Path(), len(files)) } - if info.Pkg == types.Unsafe && len(files) > 0 { - panic(`addFiles("unsafe") not permitted`) + // Don't call checker.Files on Unsafe, even with zero files, + // because it would mutate the package, which is a global. + if info.Pkg == types.Unsafe { + if len(files) > 0 { + panic(`"unsafe" package contains unexpected files`) + } + } else { + // Ignore the returned (first) error since we + // already collect them all in the PackageInfo. + info.checker.Files(files) + info.Files = append(info.Files, files...) } - // Ignore the returned (first) error since we - // already collect them all in the PackageInfo. - info.checker.Files(files) - info.Files = append(info.Files, files...) - if imp.conf.AfterTypeCheck != nil { imp.conf.AfterTypeCheck(info, files) } diff --git a/go/loader/loader_test.go b/go/loader/loader_test.go index 807f798c..02630eb2 100644 --- a/go/loader/loader_test.go +++ b/go/loader/loader_test.go @@ -800,3 +800,17 @@ func created(prog *loader.Program) string { } return strings.Join(pkgs, " ") } + +// Load package "io" twice in parallel. +// When run with -race, this is a regression test for Go issue 20718, in +// which the global "unsafe" package was modified concurrently. +func TestLoad1(t *testing.T) { loadIO(t) } +func TestLoad2(t *testing.T) { loadIO(t) } + +func loadIO(t *testing.T) { + t.Parallel() + conf := &loader.Config{ImportPkgs: map[string]bool{"io": false}} + if _, err := conf.Load(); err != nil { + t.Fatal(err) + } +}