std::process::Command::env_clear is unusable on Windows

[meskill]

Followed from the issue that was closed without an actual solution.

Problem

Windows requires some system envs in order to work and be able to run most of the programs and services. Without them they will fail like mentioned here

Possible solution

As described in comments to previous issue rust std could provide that system required envs in order to make env_clear more usable as it has done in libuv and go

[ChrisDenton]

Nominating this for t-libs-api discussion. Some notes:

• The error in the previous issue was caused by an improperly terminated environment block when empty (it was terminating with \0 when it needed to be \0\0). This was fixed.
• The documentation for env_clear says: "...it will prevent the spawned child process from inheriting any environment variable from its parent process".
• It is possible to run Windows executables without any environment variables. However, there are indeed many system functions or even dlls that require at least some. This is an implementation detail and not documented.
• As noted above, Go's equivalent to env_clear makes an exception for SystemRoot. We could do the same.
• Alternatively we could reset the environment to the user's default as suggested here. But this is very far from a "clear" environment so may be better as another method.

[m-ou-se]

@rustbot ping windows

What should Command::env_clear do on Windows? Should it completely clear the entire environment of the about-to-be-spawned process (as on Unix), or should it leave SystemRoot (and perhaps others) alone? Alternatively, should there be an additional method?

[rustbot]

Hey Windows Group! This bug has been identified as a good "Windows candidate".
In case it's useful, here are some instructions for tackling these sorts of
bugs. Maybe take a look?
Thanks! <3

cc @albertlarsan68 @arlosi @ChrisDenton @danielframpton @gdr-at-ms @kennykerr @luqmana @lzybkr @nico-abram @retep998 @rylev @sivadeilra @wesleywiser

[dtolnay]

The Go prior art: https://pkg.go.dev/os/exec#Cmd

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
}

env_clear looks like:

cmd := exec.Command(...)
cmd.Env = []string{}

[waynr]

Something I don't see in the discussion here is reference to this line in the Command::env_clear docs:

Clears all explicitly set environment variables and prevents inheriting any parent process environment variables.

Clears all explicitly set environment variables. It doesn't say anything about clearing all environment variables entirely. It even clarifies further:

This method will remove all explicitly added environment variables set via Command::env or Command::envs.

Doesn't this mean that any variables inherited from the current process's parent process should be left alone? I haven't looked at the implementation at all so I don't know what it actually does on any platform other than Windows.

[ChrisDenton]

I think the second part of the sentence is quite explicit, no:

...and prevents inheriting any parent process environment variables...

In addition, it will prevent the spawned child process from inheriting any environment variable from its parent process.

[waynr]

Doh! Thanks for pointing that out 🤦

[joshtriplett]

We discussed this in today's @rust-lang/libs-api meeting.

@ChrisDenton made the point that specific DLLs (notably advapi) require this.

Based on our discussion in the meeting, we felt there were two potentially reasonable alternatives:

1. Change the existing method to preserve SYSTEMROOT. This would assume that nobody relies on or cares about fully clearing the environment including this variable.

2. Add a new method (name to be bikeshedded) that acts like env_clear but preserves SYSTEMROOT. This would be the conservative option; the downside would then be having two methods and requiring people to think about which to call.

[ChrisDenton]

Ah sorry, correction: it's not advapi32 itself that requires SystemRoot, but it can load additional DLLs that in turn do require it. This can vary depending on the exact functions that are used (go was using CryptAcquireContext).

[ChrisDenton]

I think the most pertinent issue for Rust is that TcpListener is broken without the environment variable. And really anything that opens a socket.

EDIT: The actual issue is that some system functions lazily load DLLs using paths stored in the registry. These paths are loaded from strings stored in the registry and expanded using ExpandEnvironmentStrings. which replaces e.g. %systemroot% with the SYSTEMROOT environment variable.

[joshtriplett]

One interesting alternative here (not sure if it's better, just another possibility) would be for Rust's startup code to check if SYSTEMROOT is set and set it if it isn't. That would make Rust work, but wouldn't help code launched by Rust.

[ChrisDenton]

That assumes that Rust's startup code is run. I think we don't tend to assume that because there's many ways in which it might not be.

[joshtriplett]

@ChrisDenton Could also run it dynamically when invoking functions that need it.

[ChrisDenton]

Ok, after considering this some more, here's wot I think.

Usually I'd either want a (hypothetical) env_reset() that resets to a default state or else I'd want a (hypothetical) env_replace(new_env) that clears the current environment and replaces it with a known good state (for reproduciblity). In either case there wouldn't be a problem. I say this to emphasise that the case for wanting to just clear the environment is outside my wheelhouse. But I do understand it.

That said, the point I do strongly agree with is that functions in std should, as far as is reasonable¹, "just work" cross-platform without needing special platform-specific knowledge. Not having SystemRoot defined will break many programs (i.e. they won't work) therefore my recommendation would be to agree with setting it ourselves after env_clear.

If someone really really wants a clear environment, they can remove it with env_remove. This is admittedly a platform specific quirk that we should document, but I think this trade-off is more reasonable as I doubt having SystemRoot defined would break anything in practice, unlike the opposite.

I'm not a fan of std dynamically setting the process environment before function calls. That seems brittle and will only work in very specific situations (e.g. when people go through std first).

Footnotes

1. Admittedly that "as far as is reasonable" is doing a lot of heavy lifting. E.g. emulating Unix behaviour on Windows can sometimes cause a lot more headaches than just accepting platform differences. Ideally the API would be designed to account for the differences from the start but that's much harder and not always possible. ↩

[joshtriplett]

@ChrisDenton Sounds reasonable. It seems unlikely that clearing SystemRoot would form part of any sandboxing scheme (considering that effective sandboxing should actually remove access to the directory named in SystemRoot entirely). So preserving it across env_clear seems unlikely to cause a problem, and we could consider an env_clear_all or similar if there's a demand for it.

[ChrisDenton]

We wouldn't even necessarily have to preserve it. We could reset it to the value returned by GetSystemDirectoryW.