os/exec: environment variable de-duplication breaks old behaviour

[rogpeppe]

go version devel +2bd6360 Wed Mar 8 03:24:44 2017 +0000 linux/amd64

The change reviewed in https://go-review.googlesource.com/c/37586/
has broken some of our tests because Go's behaviour when
importing environment variables is to use the first instance
of the variable, not the last (unlike bash, for example), so
when some code that appends a duplicate variable to the
environment and then executes a Go program, the
variable seen by the program is now the last value
where before it was the first.

To demonstrate the issue, build the following two programs:

# tst1.go
package main

import (
	"fmt"
	"os"
)

func main() {
	fmt.Printf("xxx=%s\n", os.Getenv("xxx"))
}

second program:

# tst2.go
package main

import (
	"os"
	"os/exec"
)

func main() {
	env := []string{"xxx=a", "xxx=b"}
	c := exec.Command("tst1")
	c.Env = env
	c.Stdout = os.Stdout
	c.Run()
}

Try running tst2 with different versions of Go. I'd expect to see "x=b" in both cases
but old versions of Go print "x=a" instead.

[mvdan]

I would also think that the latest duplicate would be kept. Perhaps worth noting that elsewhere in the standard library, we keep the first of the duplicates, such as in url parameters or http headers.

to use the first instance of the variable, not the last (unlike bash, for example)

Could you elaborate on how bash behaves? Or do you mean the following?

$ a=foo a=bar; echo $a
bar

For the sake of completeness, both dash and BusyBox's ash behave the same.

[rogpeppe]

@mvdan Change exec.Command("tst1") to exec.Command("/bin/sh", "-c", "echo $xxx") to see what shells do - I think they usually use the last variable found. It's an unfortunate difference from Go's behaviour which means that de-duplicating environment variables is never going to be fully backwardly compatible. It's arguable that we should just fix our tests to avoid the duplication, but I thought it worth raising the issue here anyway.

[mvdan]

is never going to be fully backwardly compatible.

I don't understand - the os/exec deduplication was only introduced in February, so it hasn't been released yet. We can change it without breaking backwards compatibility.

[bradfitz]

When you use your shell,

$ xxx=a xxx=b ./cmd

The shell is stripping all but the last pair and only running ./cmd with the last.

Observe (on Linux):

package main

import (
        "fmt"
        "io/ioutil"
        "log"
        "os"
)

func main() {
        slurp, err := ioutil.ReadFile("/proc/self/environ")
        if err != nil {
                log.Fatal(err)
        }
        fmt.Printf("%q\n", slurp)
        fmt.Printf("%q\n", os.Getenv("xxx"))
}

$ xxx=a xxx=b ./printenv 
"xxx=b\x00XDG_SESSION_ID=1\x00SHELL=/bin/bash\x00TERM=screen\x00SSH_CLIENT=...

So at tip, Go is now doing what shells are doing. Go doesn't have the option of making os.Getenv change. And we don't want to change os/exec, because that would break the common pattern of appending to cmd.Env, and be inconsistent with shells.

We probably should've done this from day 1. Sorry. My fault. I never knew about shells doing this until recently.

I think the best option, @rogpeppe, is to update your tests. None of this was documented or guaranteed in the past, neither in Go nor in shell or C, AFAIK.

[robpike]

xxx=a xxx=b cat /proc/self environ suffices.

I agree it's a bug fix and should stay.