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 doc comment does not describe keys containing "=" #49886

Open
bcmills opened this issue Nov 30, 2021 · 9 comments
Open

os: Environ doc comment does not describe keys containing "=" #49886

bcmills opened this issue Nov 30, 2021 · 9 comments
Labels
Documentation ExpertNeeded NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. OS-Windows
Milestone

Comments

@bcmills
Copy link
Contributor

bcmills commented Nov 30, 2021

In investigating #49882, I noticed some other suspicious environment variables from os.Environ(). I'll write a regression test demonstrating the bug and then update here.

@bcmills bcmills added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. OS-Windows labels Nov 30, 2021
@bcmills bcmills added this to the Backlog milestone Nov 30, 2021
@bcmills bcmills added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Nov 30, 2021
@rsc
Copy link
Contributor

rsc commented Nov 30, 2021

The "=FOO" is a real environment variable on Windows and it probably needs to be preserved in os.Environ, even if it is incompatible with os.Setenv. Setenv is a helper but not necessarily capable of everything.

@bcmills
Copy link
Contributor Author

bcmills commented Nov 30, 2021

At the very least, we need to update the os.Environ docs, since they currently promise the form “key=value” and do not mention the possibility of either an empty key or one containing "=".

Moreover, it is common to use os.Environ() as the basis for the Env field in an exec.Cmd, which also does not document any specific behavior for keys beginning with =. If these variables can't be set, can they also not be transferred to subprocesses? (Or are they ignored, or can they be set only for subprocesses? If parts of Cmd.Env may be silently ignored, that also seems like a serious problem )

(@bufflig, @alexbrainman: some clarification on the standard Windows semantics might be helpful here.)

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/367850 mentions this issue: os: test that LookupEnv reports all keys found in Environ

@bcmills bcmills added Documentation ExpertNeeded and removed WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. labels Nov 30, 2021
@bcmills
Copy link
Contributor Author

bcmills commented Nov 30, 2021

Empirically, it appears that if we parse through to the second =, both LookupEnv and Setenv can handle the resulting key.

It is not obvious to me how many = symbols the caller is expected to parse through in general. (For example: should a correct consumer include arbitrarily many leading = symbols, or just one?)

@bcmills bcmills changed the title os: Environ on Windows reports variables that are incompatible with Setenv os: Environ doc comment does not describe keys containing "=" Dec 1, 2021
@bufflig
Copy link
Contributor

bufflig commented Dec 1, 2021

Microsofts own docs (like https://docs.microsoft.com/en-us/windows/win32/api/processenv/nf-processenv-getenvironmentstrings) claims that environment names can not contain '='. They do not specifically mention empty environment variable names though, so I would personally interpret =C:=C:\golang as having the key '' and the value 'C:=C:\golang' - although that is probably not what was intended...

As the windows API for creating a (sub)process takes a null terminated block of null terminated strings 'name=value\0' with possibly just the convention of "environment variable names can not contain '='", it's possible you can technically set whatever you want, completely ignoring the key/value semantics. I guess we just have to behave as closely as possible to GetEnvironmentStrings, GetEnvironmentVariable and SetEnvironmentVariable. I can write some C programs to check how they behave in these situations.

@bcmills
Copy link
Contributor Author

bcmills commented Dec 1, 2021

They do not specifically mention empty environment variable names though, so I would personally interpret =C:=C:\golang as having the key '' and the value 'C:=C:\golang' - although that is probably not what was intended...

Empirically the key in that example is =C:. (I tried both approaches in CL 367850, and the “include the leading = in the key” approach is what matched the observed behavior of LookupEnv.)

Another such key noticed during testing is =ExitCode.

I guess we just have to behave as closely as possible to GetEnvironmentStrings, GetEnvironmentVariable and SetEnvironmentVariable. I can write some C programs to check how they behave in these situations.

SGTM.

gopherbot pushed a commit that referenced this issue Dec 3, 2021
For #49886

Change-Id: Ie3a7f12a0d30ec719caf375e7be30cc4a5796c3f
Reviewed-on: https://go-review.googlesource.com/c/go/+/367850
Trust: Bryan Mills <[email protected]>
Run-TryBot: Bryan Mills <[email protected]>
Reviewed-by: Ian Lance Taylor <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
@bufflig
Copy link
Contributor

bufflig commented Dec 3, 2021

Ok, it seems to behave like this in the barest MSVC program I could muster (completely empirically, I can find no docs describing this):

Environment variables beginning with exactly one '=' are allowed, you can set them and get them with SetEnvironmentVariable and GetEnvironmentVariable. No more than one single '=' in the beginning of a variable name is allowed. It's possible the interpretation should be that the first character can be whatever, as a key can not have zero characters.

When i.e. cmd.exe prints environments, these variables are not printed, i.e. they are "hidden", but they are in the environment block returned by GetEnvironmentStrings and can be retrieved by calling GetEnvironmentVariable in subprocesses (that inherit the environment).

You can add something that in the environment block looks like "===FOO=bar", either by using your own environment block when calling CreateProcess or by doing SetEnvironmentVariable("=","=FOO=bar"). But as expected, that can only be retrieved in a subprocess as GetEnvironmentVariable("=",...).

As a fun quirk/bug, you can actually do SetEnvironmentVariable("=FOO","bar=baz"); and then GetEnvironmentVariable("=FOO=bar",...) and get "baz", but only if you do it in the same process, a subprocess can only retrieve "=FOO" if it inherited the environment. I have no idea what kind of caching makes that "work" when you do it in the same process... I don't think we need to implement that.

I think the reasonable and compatible solution is to simply allow one prefix '=' in a key, and document that that is possible in Windows.

@alexbrainman
Copy link
Member

I guess we just have to behave as closely as possible to GetEnvironmentStrings, GetEnvironmentVariable and SetEnvironmentVariable

I agree with @bufflig sentiment here. And so far I don't see any problems with our current code.

If anyone really interested to see where environment variables, like =C:, come from, see Undocumented Dynamic variables section in

https://ss64.com/nt/syntax-variables.html

and in particular

https://devblogs.microsoft.com/oldnewthing/20100506-00/?p=14133

Alex

@gopherbot
Copy link
Contributor

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 <[email protected]>
Run-TryBot: Bryan Mills <[email protected]>
Auto-Submit: Bryan Mills <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation ExpertNeeded NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. OS-Windows
Projects
None yet
Development

No branches or pull requests

5 participants