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>

diff --git a/src/os/exec/env_test.go b/src/os/exec/env_test.go
index b5ac398c274db405b1e2144cb5d6dc3c0cabadfb..112f1e654a104bf81dfefc84ae828080b00b1c19 100644 (file)
--- 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 845b737e28fc773538b8c9701b44162245eda300..58f8bbf84d6bf27381f2f501a15f67cffb3264ab 100644 (file)
--- 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
 }