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

syscall: sort environment passed to CreateProcess / CreateProcessAsUser #29530

Open
petemoore opened this issue Jan 3, 2019 · 6 comments
Open

syscall: sort environment passed to CreateProcess / CreateProcessAsUser #29530

petemoore opened this issue Jan 3, 2019 · 6 comments
Assignees
Milestone

Comments

@petemoore
Copy link

@petemoore petemoore commented Jan 3, 2019

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

go version go1.11.2 darwin/amd64

Does this issue reproduce with the latest release?

Yes.

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

go envOutput
$ go env
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/pmoore/Library/Caches/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/pmoore/go"
GOPROXY=""
GORACE=""
GOROOT="/usr/local/Cellar/go/1.11.2/libexec"
GOTMPDIR=""
GOTOOLDIR="/usr/local/Cellar/go/1.11.2/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/v9/mll6p_rj5h94dt_m5m8j0f9c0000gn/T/go-build797508603=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

I looked at https://github.com/golang/go/blob/go1.11.4/src/syscall/exec_windows.go#L97-L122

What did you expect to see?

I expected to see the env vars sorted alphabetically by name, with case-insensitive sort, Unicode order, without regard to locale.

What did you see instead?

The code does not sort the environment variable entries. The MSDN docs state:

All strings in the environment block must be sorted alphabetically by name. The sort is case-insensitive, Unicode order, without regard to locale. Because the equal sign is a separator, it must not be used in the name of an environment variable.

Note, this hasn't caused me any problems - but it seems like this could cause problems with any Windows kernel functions that expect the env to be sorted.

@petemoore
Copy link
Author

@petemoore petemoore commented Jan 3, 2019

This isn't actually a documentation bug (I see this issue just received the Documentation label) but a potential golang standard library bug for Windows platform.

@dominikh dominikh removed the Documentation label Jan 3, 2019
@dominikh dominikh added this to the Go1.12 milestone Jan 3, 2019
@dominikh
Copy link
Member

@dominikh dominikh commented Jan 3, 2019

Evil machine overlords etc etc…

@dominikh dominikh changed the title MSDN docs state that environment blocks passed to CreateProcess / CreateProcessAsUser should be sorted syscall: sort environment passed to CreateProcess / CreateProcessAsUser Jan 3, 2019
@dominikh dominikh removed the Documentation label Jan 3, 2019
@alexbrainman
Copy link
Member

@alexbrainman alexbrainman commented Jan 5, 2019

This SGTM.

I don't see the downside of this change. But others might disagree.

Alex

@andybons andybons modified the milestones: Go1.12, Go1.13 Feb 12, 2019
@gopherbot
Copy link

@gopherbot gopherbot commented May 1, 2019

Change https://golang.org/cl/160828 mentions this issue: syscall: perform environment variable sort for createEnvBlock

@odeke-em odeke-em self-assigned this May 1, 2019
@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented May 6, 2019

If this isn't causing any known problems, I think we should wait until Go 1.14 (and early-in-cycle).

@odeke-em
Copy link
Member

@odeke-em odeke-em commented May 6, 2019

SGTM!

@odeke-em odeke-em modified the milestones: Go1.13, Go1.14 May 6, 2019
@rsc rsc modified the milestones: Go1.14, Backlog Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
8 participants
You can’t perform that action at this time.