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.

Author: mpl

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

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,14 +2130,15 @@ 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:]
 	}
 	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))
 		}

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)
+}

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)
+}

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