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: Environ is corrupted on windows #61956

Closed
zyga opened this issue Aug 11, 2023 · 12 comments
Closed

os: Environ is corrupted on windows #61956

zyga opened this issue Aug 11, 2023 · 12 comments
Labels
NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. OS-Windows WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.
Milestone

Comments

@zyga
Copy link

zyga commented Aug 11, 2023

What version of Go are you using (go version)?

$ go version
go version go1.21.0 windows/arm64

Does this issue reproduce with the latest release?

This is the latest release at this time, so yes.go

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
set GO111MODULE=
set GOARCH=arm64
set GOBIN=
set GOCACHE=C:\Users\me\AppData\Local\go-build
set GOENV=C:\Users\me\AppData\Roaming\go\env
set GOEXE=.exe
set GOEXPERIMENT=
set GOFLAGS=
set GOHOSTARCH=arm64
set GOHOSTOS=windows
set GOINSECURE=
set GOMODCACHE=C:\Users\me\go\pkg\mod
set GONOPROXY=
set GONOSUMDB=
set GOOS=windows
set GOPATH=C:\Users\me\go
set GOPRIVATE=
set GOPROXY=https://proxy.golang.org,direct
set GOROOT=C:\Program Files\Go
set GOSUMDB=sum.golang.org
set GOTMPDIR=
set GOTOOLCHAIN=auto
set GOTOOLDIR=C:\Program Files\Go\pkg\tool\windows_arm64
set GOVCS=
set GOVERSION=go1.21.0
set GCCGO=gccgo
set AR=ar
set CC=gcc
set CXX=g++
set CGO_ENABLED=0
set GOMOD=C:\Users\me\Code\go-rauc\go.mod
set GOWORK=C:\Users\me\Code\go.work
set CGO_CFLAGS=-O2 -g
set CGO_CPPFLAGS=
set CGO_CXXFLAGS=-O2 -g
set CGO_FFLAGS=-O2 -g
set CGO_LDFLAGS=-O2 -g
set PKG_CONFIG=pkg-config
set GOGCCFLAGS=-fno-caret-diagnostics -Qunused-arguments -Wl,--no-gc-sections -fmessage-length=0 -ffile-prefix-map=C:\Users\me\AppData\Local\Temp\go-build1324737207=/tmp/go-build -gno-record-gcc-switches

What did you do?

I called os.Environ on a vanilla installation of Windows on aarch64 aka arm64:

package main

import (
	"log"
	"os"
)

func main() {
	for i, env := range os.Environ() {
		log.Printf("os.Environ()[%d] = %q", i, env)
	}
}

What did you expect to see?

Normal set of key=value pairs.

What did you see instead?

The first few entries seem malformed (each printed string is from fmt with %q)

C:\Users\me\Code\go-issue>go-issue.exe
2023/08/11 23:11:40 os.Environ()[0] = "=C:=C:\\Users\\me\\Code\\go-issue"
2023/08/11 23:11:40 os.Environ()[1] = "=ExitCode=00000000"
2023/08/11 23:11:40 os.Environ()[2] = "ALLUSERSPROFILE=C:\\ProgramData"
2023/08/11 23:11:40 os.Environ()[3] = "APPDATA=C:\\Users\\me\\AppData\\Roaming"
2023/08/11 23:11:40 os.Environ()[4] = "CommonProgramFiles=C:\\Program Files\\Common Files"
2023/08/11 23:11:40 os.Environ()[5] = "CommonProgramFiles(Arm)=C:\\Program Files (Arm)\\Common Files"
2023/08/11 23:11:40 os.Environ()[6] = "CommonProgramFiles(x86)=C:\\Program Files (x86)\\Common Files"
2023/08/11 23:11:40 os.Environ()[7] = "CommonProgramW6432=C:\\Program Files\\Common Files"
...

The same program executed from powershell:

PS C:\Users\me\Code\go-issue> .\go-issue.exe
2023/08/11 23:12:35 os.Environ()[0] = "ALLUSERSPROFILE=C:\\ProgramData"
2023/08/11 23:12:35 os.Environ()[1] = "APPDATA=C:\\Users\\me\\AppData\\Roaming"
2023/08/11 23:12:35 os.Environ()[2] = "CommonProgramFiles=C:\\Program Files\\Common Files"
2023/08/11 23:12:35 os.Environ()[3] = "CommonProgramFiles(Arm)=C:\\Program Files (Arm)\\Common Files"
2023/08/11 23:12:35 os.Environ()[4] = "CommonProgramFiles(x86)=C:\\Program Files (x86)\\Common Files"
2023/08/11 23:12:35 os.Environ()[5] = "CommonProgramW6432=C:\\Program Files\\Common Files"
...
@zyga zyga changed the title affected/package: os os.Environ is corrupted on windows/arm64 in cmd.exe, works okay in powershell affected/package: os os.Environ is corrupted on windows in cmd.exe, works okay in powershell Aug 11, 2023
@zyga
Copy link
Author

zyga commented Aug 11, 2023

I've since verified the same behavior exists on Windows 11 amd64.

@qmuntal qmuntal changed the title affected/package: os os.Environ is corrupted on windows in cmd.exe, works okay in powershell os: os.Environ is corrupted on windows in cmd.exe, works okay in powershell Aug 11, 2023
@qmuntal qmuntal added OS-Windows NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Aug 11, 2023
@qmuntal
Copy link
Contributor

qmuntal commented Aug 11, 2023

Thanks for reporting. I can reproduce a similar issue in windows/amd64 with go1.20 using PowerShell too:

PS C:\Users\qmuntaldiaz\code\gotest> go run .
2023/08/11 16:20:09 os.Environ()[0] = "=::=::\\"
...

@qmuntal qmuntal changed the title os: os.Environ is corrupted on windows in cmd.exe, works okay in powershell os: os.Environ is corrupted on windows Aug 11, 2023
@qmuntal qmuntal changed the title os: os.Environ is corrupted on windows os: Environ is corrupted on windows Aug 11, 2023
@qmuntal
Copy link
Contributor

qmuntal commented Aug 11, 2023

Found this excellent article from Raymond Chen, titled What are these strange =C: environment variables?: https://devblogs.microsoft.com/oldnewthing/20100506-00/?p=14133.

TLDR: Go is not inventing nor misprocessing environment variables, =C:=C: and friends are returned by the OS.

We could filter out variables starting with =, but I'm not sure it worth doing, someone could be expecting them. Quoting Raymon:

What should you do about these variables, then?

Nothing. Just let them be and do their jobs. I’m just mentioning them here so you won’t freak out when you see them.

@alexbrainman

@qmuntal qmuntal added NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Aug 11, 2023
@zyga
Copy link
Author

zyga commented Aug 11, 2023

I don't mind having them but I think there are some things that ought to be done about os.Clearenv and os.Setenv, as one cannot set them or clear and restore them.

Perhaps we should have a way to snapshot and restore environment, including weird things like that, reliably, without having to do it through the key=value interface?

@zyga
Copy link
Author

zyga commented Aug 11, 2023

Thinking about this some more and looking at

func Clearenv() {
I would argue that os.Clearenv should probably not clear them (perhaps there ought to be a Clearenv2 with this different semantics to retain compatibility). This is in line with the intent of the feature and should retain the simple semantics of key=value pairs that is cross-platform.

@zyga
Copy link
Author

zyga commented Aug 11, 2023

If that is too tricky then perhaps we could have a comment on os.Clearenv, that one cannot round-trip with os.Clearenv and os.Setenv to get the same environment that one had before, on some platforms, so that os.Clearenv should be used carefully - with a link to the article referenced in the Go source tree.

@bcmills
Copy link
Contributor

bcmills commented Aug 11, 2023

See previously:

Note in particular that the deduplication logic for os/exec.Cmd was changed to handle these keys more correctly in https://go.dev/cl/401339.

@bcmills
Copy link
Contributor

bcmills commented Aug 11, 2023

I think there are some things that ought to be done about os.Clearenv and os.Setenv, as one cannot set them or clear and restore them.

Could you give some more detail on that?

The test I added in https://go.dev/cl/367850 does attempt to verify that os.Setenv works for those keys (or at least does not error out for them), and it looks like Clearenv is handling them correctly to.

@bcmills bcmills added this to the Backlog milestone Aug 11, 2023
@bcmills bcmills added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Aug 11, 2023
@zyga
Copy link
Author

zyga commented Aug 12, 2023

@bcmills apologies, I misunderstood how to work with environment variables with a leading-=.

For anyone in the future looking at this, the entry is still ought to be a key=value pair, just that the key has a leading equals sign.

With this clarification all the stdlib functions work correctly:

package main

import (
	"log"
	"os"
	"strings"
)

func main() {
	before := os.Environ()
	os.Clearenv()
	for _, ent := range before {
		idx := strings.IndexByte(ent, '=')
		bias := 0
		if idx == 0 {
			idx = strings.IndexByte(ent[1:], '=')
			bias = 1 // ----------------^
		}
		if idx < 0 {
			log.Fatalf("Environment entry %q is missing an =", ent)
		}
		idx += bias
		k, v := ent[:idx], ent[idx+1:]
		log.Printf("Setting environment variable %q to %q", k, v)
		if err := os.Setenv(k, v); err != nil {
			log.Fatalf("Cannot set environment variable %q to %q: %v", k, v, err)
		}
	}
	after := os.Environ()
	if len(before) != len(after) {
		log.Fatalf("Environment size differs %d != %d", len(before), len(after))
	}
	for i := range before {
		if before[i] != after[i] {
			log.Fatalf("Environment entry %d differs: %q != %q", i, before[i], after[i])
		}
	}
}

This leaves me with only one suggestion, perhaps there ought to be an os.SplitEnv that knows how to correctly split a key=value pair with leading =

One small remark about the test added in https://go-review.googlesource.com/c/go/+/367850 -- I think it handles missing, non-leading = incorrectly and would fail to report an error on environment with an entry like =foo, because it unconditionally adds one to the index once the leading = was skipped over.

@zyga
Copy link
Author

zyga commented Aug 12, 2023

For reference, my implementation of splitEnv: https://gitlab.com/zygoon/go-cmdr/-/merge_requests/63/diffs

@bcmills
Copy link
Contributor

bcmills commented Aug 14, 2023

perhaps there ought to be an os.SplitEnv that knows how to correctly split a key=value pair with leading =.

That sounds reasonable to me. Want to file a separate proposal?

@bcmills
Copy link
Contributor

bcmills commented Aug 14, 2023

Duplicate of #49886

@bcmills bcmills marked this as a duplicate of #49886 Aug 14, 2023
@bcmills bcmills closed this as not planned Won't fix, can't repro, duplicate, stale Aug 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. OS-Windows WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.
Projects
None yet
Development

No branches or pull requests

3 participants