os/exec: remove duplicate environment variables in Cmd.Start

Nobody intends to have duplicates anyway because it's so undefined
and everything handles it so poorly.

Removing duplicates automatically simplifies code and makes existing
code do what people already expect.

Fixes #12868

Change-Id: I95eeba8c59ff94d0f018012a6f4e031aaabfd5d9
Reviewed-on: https://go-review.googlesource.com/37586
Run-TryBot: Brad Fitzpatrick <[email protected]>
TryBot-Result: Gobot Gobot <[email protected]>
Reviewed-by: Ian Lance Taylor <[email protected]>

Author: bradfitz

src/os/exec/env_test.go

@@ -0,0 +1,39 @@
+// Copyright 2017 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package exec
+
+import (
+	"reflect"
+	"testing"
+)
+
+func TestDedupEnv(t *testing.T) {
+	tests := []struct {
+		noCase bool
+		in     []string
+		want   []string
+	}{
+		{
+			noCase: true,
+			in:     []string{"k1=v1", "k2=v2", "K1=v3"},
+			want:   []string{"K1=v3", "k2=v2"},
+		},
+		{
+			noCase: false,
+			in:     []string{"k1=v1", "K1=V2", "k1=v3"},
+			want:   []string{"k1=v3", "K1=V2"},
+		},
+		{
+			in:   []string{"=a", "=b", "foo", "bar"},
+			want: []string{"=b", "foo", "bar"},
+		},
+	}
+	for _, tt := range tests {
+		got := dedupEnvCase(tt.noCase, tt.in)
+		if !reflect.DeepEqual(got, tt.want) {
+			t.Errorf("Dedup(%v, %q) = %q; want %q", tt.noCase, tt.in, got, tt.want)
+		}
+	}
+}

src/os/exec/example_test.go

@@ -12,6 +12,7 @@ import (
 	"io"
 	"io/ioutil"
 	"log"
+	"os"
 	"os/exec"
 	"strings"
 	"time"
@@ -37,6 +38,17 @@ func ExampleCommand() {
 	fmt.Printf("in all caps: %q\n", out.String())
 }
 
+func ExampleCommand_environment() {
+	cmd := exec.Command("prog")
+	cmd.Env = append(os.Environ(),
+		"FOO=duplicate_value", // ignored
+		"FOO=actual_value",    // this value is used
+	)
+	if err := cmd.Run(); err != nil {
+		log.Fatal(err)
+	}
+}
+
 func ExampleCmd_Output() {
 	out, err := exec.Command("date").Output()
 	if err != nil {

src/os/exec/exec.go

@@ -55,7 +55,11 @@ type Cmd struct {
 	Args []string
 
 	// Env specifies the environment of the process.
-	// If Env is nil, Run uses the current process's environment.
+	// Each entry is of the form "key=value".
+	// If Env is nil, the new process uses the current process's
+	// environment.
+	// If Env contains duplicate environment keys, only the last
+	// value in the slice for each duplicate key is used.
 	Env []string
 
 	// Dir specifies the working directory of the command.
@@ -354,7 +358,7 @@ func (c *Cmd) Start() error {
 	c.Process, err = os.StartProcess(c.Path, c.argv(), &os.ProcAttr{
 		Dir:   c.Dir,
 		Files: c.childFiles,
-		Env:   c.envv(),
+		Env:   dedupEnv(c.envv()),
 		Sys:   c.SysProcAttr,
 	})
 	if err != nil {
@@ -712,3 +716,35 @@ func minInt(a, b int) int {
 	}
 	return b
 }
+
+// dedupEnv returns a copy of env with any duplicates removed, in favor of
+// later values.
+// Items not of the normal environment "key=value" form are preserved unchanged.
+func dedupEnv(env []string) []string {
+	return dedupEnvCase(runtime.GOOS == "windows", env)
+}
+
+// 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 {
+	out := make([]string, 0, len(env))
+	saw := map[string]int{} // key => index into out
+	for _, kv := range env {
+		eq := strings.Index(kv, "=")
+		if eq < 0 {
+			out = append(out, kv)
+			continue
+		}
+		k := kv[:eq]
+		if caseInsensitive {
+			k = strings.ToLower(k)
+		}
+		if dupIdx, isDup := saw[k]; isDup {
+			out[dupIdx] = kv
+			continue
+		}
+		saw[k] = len(out)
+		out = append(out, kv)
+	}
+	return out
+}

src/os/exec/exec_test.go

@@ -693,6 +693,11 @@ func TestHelperProcess(*testing.T) {
 			iargs = append(iargs, s)
 		}
 		fmt.Println(iargs...)
+	case "echoenv":
+		for _, s := range args {
+			fmt.Println(os.Getenv(s))
+		}
+		os.Exit(0)
 	case "cat":
 		if len(args) == 0 {
 			io.Copy(os.Stdout, os.Stdin)
@@ -1043,3 +1048,18 @@ func TestContextCancel(t *testing.T) {
 		t.Logf("exit status: %v", err)
 	}
 }
+
+// test that environment variables are de-duped.
+func TestDedupEnvEcho(t *testing.T) {
+	testenv.MustHaveExec(t)
+
+	cmd := helperCommand(t, "echoenv", "FOO")
+	cmd.Env = append(cmd.Env, "FOO=bad", "FOO=good")
+	out, err := cmd.CombinedOutput()
+	if err != nil {
+		t.Fatal(err)
+	}
+	if got, want := strings.TrimSpace(string(out)), "good"; got != want {
+		t.Errorf("output = %q; want %q", got, want)
+	}
+}