interp: make a full copy of the environment for background subshells

If a parent shell sets a variable once a background subshell is running,
the subshell should not see that change, nor should the change cause
any data races. We allowed the parent and child shells to share
a map of variables when the child was a background subshell,
which caused one of the added tests to fail often:

    --- FAIL: TestRunnerRun/#456 (0.00s)
        interp_test.go:3662: wrong output in "{ for n in {0..9}; do { echo $n; } & done; wait; } | sort":
            want: "0\n1\n2\n3\n4\n5\n6\n7\n8\n9\n"
            got:  "9\n9\n9\n9\n9\n9\n9\n9\n9\n9\n"

Moreover, the newly added background shell tests would cause a number of
"concurrent map read and write" and data race detector panics too.

overlayEnviron can be clever in sharing a variables map with the parent
only when the subshell is not being run in the background.
When the subshell is intended for background (concurrent) use,
we need to make a full copy of the variables to avoid races.

Author: mvdan

interp/api.go

@@ -869,9 +869,16 @@ func (r *Runner) Exited() bool {
 // Subshell is not safe to use concurrently with [Run]. Orchestrating this is
 // left up to the caller; no locking is performed.
 //
-// To replace e.g. stdin/out/err, do StdIO(r.stdin, r.stdout, r.stderr)(r) on
+// To replace e.g. stdin/out/err, do [StdIO](r.stdin, r.stdout, r.stderr)(r) on
 // the copy.
 func (r *Runner) Subshell() *Runner {
+	return r.subshell(true)
+}
+
+// subshell is like [Runner.subshell], but allows skipping some allocations and copies
+// when creating subshells which will not be used concurrently with the parent shell.
+// TODO(v4): we should expose this, e.g. SubshellForeground and SubshellBackground.
+func (r *Runner) subshell(background bool) *Runner {
 	if !r.didReset {
 		r.Reset()
 	}
@@ -897,10 +904,8 @@ func (r *Runner) Subshell() *Runner {
 
 		origStdout: r.origStdout, // used for process substitutions
 	}
+	r2.writeEnv = newOverlayEnviron(r.writeEnv, background)
 	// Funcs are copied, since they might be modified.
-	// Env vars aren't copied; setVar will copy lists and maps as needed.
-	oenv := &overlayEnviron{parent: r.writeEnv}
-	r2.writeEnv = oenv
 	r2.Funcs = maps.Clone(r.Funcs)
 	r2.Vars = make(map[string]expand.Variable)
 	r2.alias = maps.Clone(r.alias)

interp/interp_test.go

@@ -1422,6 +1422,19 @@ var runTests = []runTest{
 	{"echo foo | true | false & wait $!", "exit status 1"},
 	{"echo foo | false | true & wait $!", ""},
 	{"f() { false & true; }; f; wait $!", "exit status 1"},
+	// The parent and child shells should not cause data races when setting env vars.
+	{
+		"{ for n in {0..9}; do { echo $n; } & done; wait; } | sort",
+		"0\n1\n2\n3\n4\n5\n6\n7\n8\n9\n",
+	},
+	{
+		"outer=val; for n in {0..9}; do { echo $outer; } & outer=val; done; wait",
+		"val\nval\nval\nval\nval\nval\nval\nval\nval\nval\n",
+	},
+	{
+		"for n in {0..9}; do { inner=val; } & echo $inner; done",
+		"\n\n\n\n\n\n\n\n\n\n",
+	},
 
 	// bash test
 	{

interp/runner.go

@@ -61,7 +61,7 @@ func (r *Runner) fillExpandConfig(ctx context.Context) {
 				f.Close()
 				return err
 			}
-			r2 := r.Subshell()
+			r2 := r.subshell(false)
 			r2.stdout = w
 			r2.stmts(ctx, cs.Stmts)
 			r.lastExpandExit = r2.exit
@@ -99,7 +99,7 @@ func (r *Runner) fillExpandConfig(ctx context.Context) {
 				}
 			}
 
-			r2 := r.Subshell()
+			r2 := r.subshell(true)
 			stdout := r.origStdout
 			// TODO: note that `man bash` mentions that `wait` only waits for the last
 			// process substitution as long as it is $!; the logic here would mean we wait for all of them.
@@ -299,7 +299,7 @@ func (r *Runner) stmt(ctx context.Context, st *syntax.Stmt) {
 	}
 	r.exit = 0
 	if st.Background {
-		r2 := r.Subshell()
+		r2 := r.subshell(true)
 		st2 := *st
 		st2.Background = false
 		bg := bgProc{
@@ -364,7 +364,7 @@ func (r *Runner) cmd(ctx context.Context, cm syntax.Command) {
 	case *syntax.Block:
 		r.stmts(ctx, cm.Stmts)
 	case *syntax.Subshell:
-		r2 := r.Subshell()
+		r2 := r.subshell(false)
 		r2.stmts(ctx, cm.Stmts)
 		r.exit = r2.exit
 		r.setErr(r2.err)
@@ -467,7 +467,7 @@ func (r *Runner) cmd(ctx context.Context, cm syntax.Command) {
 				r.setErr(err)
 				return
 			}
-			r2 := r.Subshell()
+			r2 := r.subshell(false)
 			r2.stdout = pw
 			if cm.Op == syntax.PipeAll {
 				r2.stderr = pw

interp/vars.go

@@ -16,32 +16,47 @@ import (
 	"mvdan.cc/sh/v3/syntax"
 )
 
+func newOverlayEnviron(parent expand.Environ, background bool) *overlayEnviron {
+	oenv := &overlayEnviron{}
+	if !background {
+		oenv.parent = parent
+	} else {
+		// We could do better here if the parent is also an overlayEnviron;
+		// measure with profiles or benchmarks before we choose to do so.
+		oenv.values = make(map[string]expand.Variable)
+		maps.Insert(oenv.values, parent.Each)
+	}
+	return oenv
+}
+
 type overlayEnviron struct {
 	parent expand.Environ
 	values map[string]expand.Variable
 
 	// We need to know if the current scope is a function's scope, because
-	// functions can modify global variables.
+	// functions can modify global variables. When true, [parent] must not be nil.
 	funcScope bool
 }
 
 func (o *overlayEnviron) Get(name string) expand.Variable {
 	if vr, ok := o.values[name]; ok {
 		return vr
 	}
-	return o.parent.Get(name)
+	if o.parent != nil {
+		return o.parent.Get(name)
+	}
+	return expand.Variable{}
 }
 
 func (o *overlayEnviron) Set(name string, vr expand.Variable) error {
-	prevOverlay, inOverlay := o.values[name]
-	prev := o.parent.Get(name)
+	prev, inOverlay := o.values[name]
 	// Manipulation of a global var inside a function.
-	if o.funcScope && !vr.Local && !prevOverlay.Local {
+	if o.funcScope && !vr.Local && !prev.Local {
 		// In a function, the parent environment is ours, so it's always read-write.
 		return o.parent.(expand.WriteEnviron).Set(name, vr)
 	}
-	if inOverlay {
-		prev = prevOverlay
+	if !inOverlay && o.parent != nil {
+		prev = o.parent.Get(name)
 	}
 
 	if o.values == nil {
@@ -70,7 +85,9 @@ func (o *overlayEnviron) Set(name string, vr expand.Variable) error {
 }
 
 func (o *overlayEnviron) Each(f func(name string, vr expand.Variable) bool) {
-	o.parent.Each(f)
+	if o.parent != nil {
+		o.parent.Each(f)
+	}
 	for name, vr := range o.values {
 		if !f(name, vr) {
 			return