Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

os/exec: Cmd fails to enforce that Env entries are of the documented "key=value" form #52436

Open
bcmills opened this issue Apr 19, 2022 · 4 comments
Labels
NeedsDecision
Milestone

Comments

@bcmills
Copy link
Member

@bcmills bcmills commented Apr 19, 2022

(Noticed while investigating #49886 in the context of #50599.)

The documentation for the Env field of the exec.Cmd struct says:

$ go1.18 doc exec.Cmd.Env
package exec // import "os/exec"

type Cmd struct {
    // Env specifies the environment of the process. 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. As a special case on Windows,
    // SYSTEMROOT is always added if missing and not explicitly set to the empty
    // string.
    Env []string

    // ... other fields elided ...
}

That is consistent with os.Environ(), which is documented as:

$ go1.18 doc os.Environ
package os // import "os"

func Environ() []string
    Environ returns a copy of strings representing the environment, in the form
    "key=value".

Unfortunately, the implementation of os/exec.Cmd violates both of those documented behaviors.

As demonstrated in https://go.dev/play/p/qdlwGaJg1_f:

  • Windows-internal keys beginning with = are erroneously deduplicated. (The key =C: collapses with the key =D:, dropping the =C: entry entirely.)
  • Malformed key-value pairs are neither diagnosed nor dropped. (The string "garbage" is passed through to os.Environ() without producing an error.)
package main

import (
	"fmt"
	"log"
	"os"
	"os/exec"
)

func main() {
	if os.Getenv("SUBPROCESS") != "" {
		for _, kv := range os.Environ() {
			fmt.Printf("%q\n", kv)
		}
		os.Exit(0)
	}

	exe, err := os.Executable()
	if err != nil {
		log.Fatal(err)
	}

	cmd := exec.Command(exe)
	cmd.Stdout = os.Stdout
	cmd.Stderr = os.Stderr
	cmd.Env = []string{
		"SUBPROCESS=1",
		"FOO=bar",
		"",
		"garbage",
		"garbage=bananas",
		`=C:=C:\golang`,
		`=C:=C:\tmp`,
		`=D:=D:\other`,
		"FOO=baz",
	}
	if err := cmd.Run(); err != nil {
		log.Fatal(err)
	}
}
$ go1.18 run .
"SUBPROCESS=1"
"FOO=baz"
"garbage"
"garbage=what"
"=D:=D:\\other"
@bcmills bcmills added NeedsInvestigation and removed Documentation labels Apr 19, 2022
@bcmills bcmills added this to the Backlog milestone Apr 19, 2022
@beoran
Copy link

@beoran beoran commented Apr 19, 2022

I would like to note here that the FOO=bar form of environment strings is merely a convention on most Unix systems. (https://man7.org/linux/man-pages/man7/environ.7.html) The C execve function does also not enforce this convention (https://man7.org/linux/man-pages/man2/execve.2.html).

Therefore, it is also not strictly necessary that the Go exec package enforces this convention. Actually, we might be calling programs that use the environment in non conventional ways, which will not be possible anymore if exec enforced the convention.

I would propose to correct the documentation to reflect this

@bcmills
Copy link
Member Author

@bcmills bcmills commented Apr 20, 2022

@beoran, you are correct about the Unix format being strictly a convention — however, the os and os/exec packages are not specific to Unix, and their current documentation makes stronger claims. (For example, exec.Cmd already deduplicates entries for the same key; see #12868.)

I agree that in principle this issue could be resolve by updating the documentation.

@beoran
Copy link

@beoran beoran commented Apr 20, 2022

@bcmills If we look at os.Environ we can readily see that it does not enforce the = convention. It might be convenient that exec.Cmd does sometimes enforce the convention, but it is not consistent with os.Environ's actual implementation. So it seems a bit of a difficult situation. Even though it is not documented, there will likely be some Go programs that rely on the actual stlib behavior. Fixing the docs seems like the least invasive solution.

Of course, if I understand correctly, the Go compatibility promise does not apply to undocumented behavior, so it could also be solved by fixing the various environment related functions to enforce the convention. I'll leave it up to the Go maintainers to decide.

@bcmills bcmills added the NeedsDecision label Apr 20, 2022
@gopherbot gopherbot removed the NeedsInvestigation label Apr 20, 2022
@gopherbot
Copy link

@gopherbot gopherbot commented Apr 20, 2022

Change https://go.dev/cl/401339 mentions this issue: os/exec: preserve original order of entries in dedupEnv

gopherbot pushed a commit that referenced this issue Apr 21, 2022
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsDecision
Projects
None yet
Development

No branches or pull requests

3 participants