From a2f56431ea38c92b754271e7e6d95d51f1fdbb2a Mon Sep 17 00:00:00 2001 From: mpl Date: Mon, 14 Sep 2020 16:22:04 +0200 Subject: [PATCH] interp: fix data race for composite literal creation This change fixes two data races related to composite literal creation. The first one isn't controversial as it is just about initializing the variable that contains the values in the right place, i.e. within the n.exec, so that this variable is local to each potential goroutine, instead of being racily shared by all goroutines. The second one is more worrying, i.e. having to protect the node typ with a mutex, because a call to func (t *itype) refType actually modifies the itype itself, which means it is not concurrent safe. The change seems to work, and does not seem to introduce regression, but it is still a concern as it probably is a sign that more similar guarding has to be done in several other places. --- interp/interp_eval_test.go | 45 +++++++++++++++++++ interp/run.go | 14 +++++- .../concurrent/composite/composite_lit.go | 19 ++++++++ .../concurrent/composite/composite_sparse.go | 19 ++++++++ interp/type.go | 2 + 5 files changed, 98 insertions(+), 1 deletion(-) create mode 100644 interp/testdata/concurrent/composite/composite_lit.go create mode 100644 interp/testdata/concurrent/composite/composite_sparse.go diff --git a/interp/interp_eval_test.go b/interp/interp_eval_test.go index 4d7804e0..d7ded3f8 100644 --- a/interp/interp_eval_test.go +++ b/interp/interp_eval_test.go @@ -1111,6 +1111,51 @@ func TestConcurrentEvals3(t *testing.T) { } } +func TestConcurrentComposite1(t *testing.T) { + testConcurrentComposite(t, "./testdata/concurrent/composite/composite_lit.go") +} + +func TestConcurrentComposite2(t *testing.T) { + testConcurrentComposite(t, "./testdata/concurrent/composite/composite_sparse.go") +} + +func testConcurrentComposite(t *testing.T, filePath string) { + pin, pout := io.Pipe() + i := interp.New(interp.Options{Stdout: pout}) + i.Use(stdlib.Symbols) + + errc := make(chan error) + var output string + go func() { + sc := bufio.NewScanner(pin) + k := 0 + for sc.Scan() { + output += sc.Text() + k++ + if k > 1 { + break + } + } + errc <- nil + }() + + if _, err := i.EvalPath(filePath); err != nil { + t.Fatal(err) + } + + _ = pin.Close() + _ = pout.Close() + + if err := <-errc; err != nil { + t.Fatal(err) + } + + expected := "{hello}{hello}" + if output != expected { + t.Fatalf("unexpected output, want %q, got %q", expected, output) + } +} + func TestEvalScanner(t *testing.T) { type testCase struct { desc string diff --git a/interp/run.go b/interp/run.go index 07bdb813..a052815b 100644 --- a/interp/run.go +++ b/interp/run.go @@ -7,6 +7,7 @@ import ( "go/constant" "log" "reflect" + "sync" "unsafe" ) @@ -2069,6 +2070,8 @@ func doCompositeLit(n *node, hasType bool) { if typ.cat == ptrT || typ.cat == aliasT { typ = typ.val } + var mu sync.Mutex + typ.mu = &mu child := n.child if hasType { child = n.child[1:] @@ -2095,7 +2098,12 @@ func doCompositeLit(n *node, hasType bool) { i := n.findex l := n.level n.exec = func(f *frame) bltn { + // TODO: it seems fishy that the typ might be modified post-compilation, and + // hence that several goroutines might be using the same typ that they all modify. + // We probably need to revisit that. + typ.mu.Lock() a := reflect.New(typ.TypeOf()).Elem() + typ.mu.Unlock() for i, v := range values { a.Field(i).Set(v(f)) } @@ -2122,6 +2130,8 @@ func doCompositeSparse(n *node, hasType bool) { if typ.cat == ptrT || typ.cat == aliasT { typ = typ.val } + var mu sync.Mutex + typ.mu = &mu child := n.child if hasType { child = n.child[1:] @@ -2129,7 +2139,6 @@ func doCompositeSparse(n *node, hasType bool) { destInterface := destType(n).cat == interfaceT values := make(map[int]func(*frame) reflect.Value) - a, _ := typ.zero() for _, c := range child { c1 := c.child[1] field := typ.fieldIndex(c.child[0].ident) @@ -2149,6 +2158,9 @@ func doCompositeSparse(n *node, hasType bool) { } n.exec = func(f *frame) bltn { + typ.mu.Lock() + a, _ := typ.zero() + typ.mu.Unlock() for i, v := range values { a.Field(i).Set(v(f)) } diff --git a/interp/testdata/concurrent/composite/composite_lit.go b/interp/testdata/concurrent/composite/composite_lit.go new file mode 100644 index 00000000..53d5e147 --- /dev/null +++ b/interp/testdata/concurrent/composite/composite_lit.go @@ -0,0 +1,19 @@ +package main + +import ( + "time" +) + +type foo struct { + bar string +} + +func main() { + for i := 0; i < 2; i++ { + go func() { + a := foo{"hello"} + println(a) + }() + } + time.Sleep(time.Second) +} diff --git a/interp/testdata/concurrent/composite/composite_sparse.go b/interp/testdata/concurrent/composite/composite_sparse.go new file mode 100644 index 00000000..6788fc2b --- /dev/null +++ b/interp/testdata/concurrent/composite/composite_sparse.go @@ -0,0 +1,19 @@ +package main + +import ( + "time" +) + +type foo struct { + bar string +} + +func main() { + for i := 0; i < 2; i++ { + go func() { + a := foo{bar: "hello"} + println(a) + }() + } + time.Sleep(time.Second) +} diff --git a/interp/type.go b/interp/type.go index e5f12757..f644a25a 100644 --- a/interp/type.go +++ b/interp/type.go @@ -6,6 +6,7 @@ import ( "path/filepath" "reflect" "strconv" + "sync" ) // tcat defines interpreter type categories. @@ -104,6 +105,7 @@ type structField struct { // itype defines the internal representation of types in the interpreter. type itype struct { + mu *sync.Mutex cat tcat // Type category field []structField // Array of struct fields if structT or interfaceT key *itype // Type of key element if MapT or nil