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 <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>

diff --git a/src/os/exec/env_test.go b/src/os/exec/env_test.go
new file mode 100644 (file)
index 0000000..b5ac398
--- /dev/null
+++ b/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)
+               }
+       }
+}

diff --git a/src/os/exec/example_test.go b/src/os/exec/example_test.go
index 5ccb21af6adc11bd09ecdfe55a3cceb867aac197..b70b99032523c299ac84d260f17b9cb4621af56c 100644 (file)
--- a/src/os/exec/example_test.go
+++ b/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 {

diff --git a/src/os/exec/exec.go b/src/os/exec/exec.go
index c4c5168b98074a7ee8c0e0baa99b3f7bab6782f2..2bfc34f5caa1203ee88070283b5f9131bd6a6987 100644 (file)
--- a/src/os/exec/exec.go
+++ b/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
+}

diff --git a/src/os/exec/exec_test.go b/src/os/exec/exec_test.go
index 13bc39794b08a3af194292ef1957f156f3ff9f18..7b69db7c76723f3064f529f5fadef078d55812e5 100644 (file)
--- a/src/os/exec/exec_test.go
+++ b/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)
+       }
+}