From 87850d38abe55e2e7bd4523725c6bdfef203f7b2 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Daniel=20Mart=C3=AD?= <mvdan@mvdan.cc>
Date: Wed, 15 Nov 2023 21:30:09 +0000
Subject: [PATCH] interp: support subshells with FuncEnviron as Env

FuncEnviron can't support listing all env vars via Environ.Each,
and Runner.Subshell relied on that to make a full copy of an Environ.

Instead of trying to make the full copy upfront, which might not work,
the runner now makes a copy of each variable value in setVar
when it is about to be modified, to prevent subshells from modifying
the values in the parent shell.

This method is still not perfect, hence the added TODOs,
but it's certainly more correct as we fix a significant footgun.

Fixes #1043.
---
 interp/api.go         | 24 +++---------------------
 interp/interp_test.go | 10 ++++++++++
 interp/vars.go        | 30 +++++++++++++++++-------------
 3 files changed, 30 insertions(+), 34 deletions(-)

diff --git a/interp/api.go b/interp/api.go
index 9492a7817..aed36e9e0 100644
--- a/interp/api.go
+++ b/interp/api.go
@@ -814,27 +814,9 @@ func (r *Runner) Subshell() *Runner {
 
 		origStdout: r.origStdout, // used for process substitutions
 	}
-	// Env vars and funcs are copied, since they might be modified.
-	// TODO(v4): lazy copying? it would probably be enough to add a
-	// copyOnWrite bool field to Variable, then a Modify method that must be
-	// used when one needs to modify a variable. ideally with some way to
-	// catch direct modifications without the use of Modify and panic,
-	// perhaps via a check when getting or setting vars at some level.
-	oenv := &overlayEnviron{parent: expand.ListEnviron()}
-	r.writeEnv.Each(func(name string, vr expand.Variable) bool {
-		vr2 := vr
-		// Make deeper copies of List and Map, but ensure that they remain nil
-		// if they are nil in vr.
-		vr2.List = append([]string(nil), vr.List...)
-		if vr.Map != nil {
-			vr2.Map = make(map[string]string, len(vr.Map))
-			for k, vr := range vr.Map {
-				vr2.Map[k] = vr
-			}
-		}
-		oenv.Set(name, vr2)
-		return true
-	})
+	// 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 = make(map[string]*syntax.Stmt, len(r.Funcs))
 	for k, v := range r.Funcs {
diff --git a/interp/interp_test.go b/interp/interp_test.go
index 5c1f15253..eb271da18 100644
--- a/interp/interp_test.go
+++ b/interp/interp_test.go
@@ -3868,6 +3868,16 @@ func TestRunnerOpts(t *testing.T) {
 			"set bar_interp_missing; echo $@",
 			"bar_interp_missing\n",
 		},
+		{
+			opts(interp.Env(expand.FuncEnviron(func(name string) string {
+				if name == "foo" {
+					return "bar"
+				}
+				return ""
+			}))),
+			"(echo $foo); echo x | echo $foo",
+			"bar\nbar\n",
+		},
 	}
 	p := syntax.NewParser()
 	for _, c := range cases {
diff --git a/interp/vars.go b/interp/vars.go
index feda4285b..454470949 100644
--- a/interp/vars.go
+++ b/interp/vars.go
@@ -31,21 +31,19 @@ func (o *overlayEnviron) Get(name string) expand.Variable {
 }
 
 func (o *overlayEnviron) Set(name string, vr expand.Variable) error {
-	// Manipulation of a global var inside a function
+	// Manipulation of a global var inside a function.
 	if o.funcScope && !vr.Local && !o.values[name].Local {
-		// "foo=bar" on a global var in a function updates the global scope
 		if vr.IsSet() {
-			return o.parent.(expand.WriteEnviron).Set(name, vr)
-		}
-		// "foo=bar" followed by "export foo" or "readonly foo"
-		if vr.Exported || vr.ReadOnly {
+			// "foo=bar" on a global var in a function updates the global scope
+		} else if vr.Exported || vr.ReadOnly {
+			// "foo=bar" followed by "export foo" or "readonly foo"
 			prev := o.Get(name)
 			prev.Exported = prev.Exported || vr.Exported
 			prev.ReadOnly = prev.ReadOnly || vr.ReadOnly
 			vr = prev
-			return o.parent.(expand.WriteEnviron).Set(name, vr)
 		}
-		// "unset" is handled below
+		// In a function, the parent environment is ours, so it's always read-write.
+		return o.parent.(expand.WriteEnviron).Set(name, vr)
 	}
 
 	prev := o.Get(name)
@@ -71,10 +69,6 @@ func (o *overlayEnviron) Set(name string, vr expand.Variable) error {
 			return nil
 		}
 		delete(o.values, name)
-		if writeEnv, _ := o.parent.(expand.WriteEnviron); writeEnv != nil {
-			writeEnv.Set(name, vr)
-			return nil
-		}
 	} else if prev.Exported {
 		// variable is set and was marked as exported
 		vr.Exported = true
@@ -234,7 +228,9 @@ func (r *Runner) setVar(name string, index syntax.ArithmExpr, vr expand.Variable
 	case expand.String:
 		list = append(list, cur.Str)
 	case expand.Indexed:
-		list = cur.List
+		// TODO: slices.Clone
+		// TODO: only clone when inside a subshell and getting a var from outside for the first time
+		list = append([]string(nil), cur.List...)
 	case expand.Associative:
 		// if the existing variable is already an AssocArray, try our
 		// best to convert the key to a string
@@ -243,6 +239,14 @@ func (r *Runner) setVar(name string, index syntax.ArithmExpr, vr expand.Variable
 			return
 		}
 		k := r.literal(w)
+
+		// TODO: maps.Clone
+		// TODO: only clone when inside a subshell and getting a var from outside for the first time
+		m2 := make(map[string]string, len(cur.Map))
+		for k, vr := range cur.Map {
+			m2[k] = vr
+		}
+		cur.Map = m2
 		cur.Map[k] = valStr
 		r.setVarInternal(name, cur)
 		return