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.

Sign up for GitHub

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

Jump to bottom

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 · 6 comments
Open

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

bcmills opened this issue Apr 19, 2022 · 6 comments
Labels
NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone
Backlog

Comments

@bcmills
Copy link
Contributor

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 Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. and removed Documentation labels Apr 19, 2022
@bcmills bcmills added this to the Backlog milestone Apr 19, 2022
@beoran
Copy link

beoran commented Apr 19, 2022 •
edited

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
Contributor Author

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 commented Apr 20, 2022 •
edited

@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 Feedback is required from experts, contributors, and/or the community before a change can be made. label Apr 20, 2022
@gopherbot gopherbot removed the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Apr 20, 2022
@gopherbot
Copy link

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
@gopherbot
os/exec: preserve original order of entries in dedupEnv
aa24255 
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>
@rsc
Copy link
Contributor

rsc commented Jan 17, 2023

I believe these variables without = signs are permitted on Unix although certainly non-standard. I wouldn't want to disallow them on Unix in case we are running in a parent process that has inherited one of these oddities. If we disallow them then code using os/exec and passing through os.Environ() would start failing.

On Windows, environment variables without = cause exec to fail with an unhelpful "The parameter is incorrect." error.
On Plan 9, environment variables without = are impossible to create (each key=value means echo -n value >'#e/key').
We can and probably should disallow them there.

@bcmills bcmills mentioned this issue Aug 11, 2023
Closed
@bcmills
Copy link
Contributor Author

bcmills commented Aug 14, 2023

In #61956 (comment), @zyga points out that if non-key=value entries are permitted in os.Environ and the exec.Cmd.Env field, we should update os.TestEnvironConsistency to allow (and ignore) them as well.

@zyga zyga mentioned this issue Aug 15, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Milestone
Backlog
Development

No branches or pull requests
4 participants
@beoran @rsc @bcmills @gopherbot