os/exec: preserve original order of entries in dedupEnv

Once #50599 is implemented, the entries will be observable via the
Environ method. I find it confusing for later entries in the list to
jump arbitrarily far forward based on entries for the same key that no
longer exist.

This also fixes the deduplication logic for the degenerate Windows
keys observed in #49886, which were previously deduplicated as empty
keys.

(It does not do anything about the even-more-degenerate keys observed
in #52436.)

For #50599.

Change-Id: Ia7cd2200ec34ccc4b9d18631cb513194dc420c25
Reviewed-on: https://go-review.googlesource.com/c/go/+/401339
Reviewed-by: Ian Lance Taylor <iant@google.com>
Run-TryBot: Bryan Mills <bcmills@google.com>
Auto-Submit: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>

2 files changed, 45 insertions, 10 deletions

diff --git a/src/os/exec/env_test.go b/src/os/exec/env_test.go
index b5ac398c27..112f1e654a 100644
--- a/src/os/exec/env_test.go
+++ b/src/os/exec/env_test.go
@@ -18,17 +18,29 @@ func TestDedupEnv(t *testing.T) {
 		{
 			noCase: true,
 			in:     []string{"k1=v1", "k2=v2", "K1=v3"},
-			want:   []string{"K1=v3", "k2=v2"},
+			want:   []string{"k2=v2", "K1=v3"},
 		},
 		{
 			noCase: false,
 			in:     []string{"k1=v1", "K1=V2", "k1=v3"},
-			want:   []string{"k1=v3", "K1=V2"},
+			want:   []string{"K1=V2", "k1=v3"},
 		},
 		{
 			in:   []string{"=a", "=b", "foo", "bar"},
 			want: []string{"=b", "foo", "bar"},
 		},
+		{
+			// #49886: preserve weird Windows keys with leading "=" signs.
+			noCase: true,
+			in:     []string{`=C:=C:\golang`, `=D:=D:\tmp`, `=D:=D:\`},
+			want:   []string{`=C:=C:\golang`, `=D:=D:\`},
+		},
+		{
+			// #52436: preserve invalid key-value entries (for now).
+			// (Maybe filter them out or error out on them at some point.)
+			in:   []string{"dodgy", "entries"},
+			want: []string{"dodgy", "entries"},
+		},
 	}
 	for _, tt := range tests {
 		got := dedupEnvCase(tt.noCase, tt.in)
diff --git a/src/os/exec/exec.go b/src/os/exec/exec.go
index 845b737e28..58f8bbf84d 100644
--- a/src/os/exec/exec.go
+++ b/src/os/exec/exec.go
@@ -745,24 +745,47 @@ func dedupEnv(env []string) []string {
 // dedupEnvCase is dedupEnv with a case option for testing.
 // If caseInsensitive is true, the case of keys is ignored.
 func dedupEnvCase(caseInsensitive bool, env []string) []string {
+	// Construct the output in reverse order, to preserve the
+	// last occurrence of each key.
 	out := make([]string, 0, len(env))
-	saw := make(map[string]int, len(env)) // key => index into out
-	for _, kv := range env {
-		k, _, ok := strings.Cut(kv, "=")
-		if !ok {
-			out = append(out, kv)
+	saw := make(map[string]bool, len(env))
+	for n := len(env); n > 0; n-- {
+		kv := env[n-1]
+
+		i := strings.Index(kv, "=")
+		if i == 0 {
+			// We observe in practice keys with a single leading "=" on Windows.
+			// TODO(#49886): Should we consume only the first leading "=" as part
+			// of the key, or parse through arbitrarily many of them until a non-"="?
+			i = strings.Index(kv[1:], "=") + 1
+		}
+		if i < 0 {
+			if kv != "" {
+				// The entry is not of the form "key=value" (as it is required to be).
+				// Leave it as-is for now.
+				// TODO(#52436): should we strip or reject these bogus entries?
+				out = append(out, kv)
+			}
 			continue
 		}
+		k := kv[:i]
 		if caseInsensitive {
 			k = strings.ToLower(k)
 		}
-		if dupIdx, isDup := saw[k]; isDup {
-			out[dupIdx] = kv
+		if saw[k] {
 			continue
 		}
-		saw[k] = len(out)
+
+		saw[k] = true
 		out = append(out, kv)
 	}
+
+	// Now reverse the slice to restore the original order.
+	for i := 0; i < len(out)/2; i++ {
+		j := len(out) - i - 1
+		out[i], out[j] = out[j], out[i]
+	}
+
 	return out
 }