Child process environment cannot be fully overridden on Windows

[bwh]

twisted.internet.win32eventreactor.Process.__init__ does this:

# Add the specified environment to the current environment - this is
# necessary because certain operations are only supported on Windows
# if certain environment variables are present.
env = os.environ.copy()
env.update(environment or {})

whereas twisted.internet.process.Process passes the given environment dictionary to os.execve unaltered.

The comment applies just as well as to Posix systems as to Windows (for example LD_*, HOME, locale variables), and I don't see that it justifies this inconsistency. The worst aspect of this behaviour is that it makes it impossible to un-define variables in a child process that are defined in the parent process.

[Glyph]

Yep. Definitely a bug.

[bwh]

This code was introduced in r13184 as a fix for #837.

I'll attach a patch.

[bwh]

patch against v2.1.0

[bwh]

Now I realise there's a much larger problem. In all the other spawnProcess implementations, the default environment for new processes is also empty rather than a copy of the current process's environment. This is a terrible default on both POSIX and Windows systems. See ​http://bugs.debian.org/349196 and related bug reports for an example of what happens when child processes receive an environment that is too thoroughly "sanitised".

Is there any possibility of changing the default?

[Glyph]

sudo is a piece of crap, sure, (did you know it refers specifically to PYTHONPATH, for example?) but spawnProcess will continue to present an empty environment by default, because that is more secure. Explicit is better than implicit! It is easy enough to pass the environment through with os.environ, so if your subprocess is going to care about its environment at all, please remember to pass it through.

[Glyph]

I am trying to divest myself of win32 issues here...

Looking back at #837, I'm not sure what to do. I don't know why we considered that a bug in the first place; but in any event there need to be some tests for this stuff.

[teratorn]

Thanks guys. I tend to agree with glyph that passing env variables should be explicit. Will look in to this soonish.

[teratorn]

Alright I think I'm just going to fix this by not updating the passed in environment with the environment from the current process.

The pass environment will be used as-is and will default to empty.

Branch in win32procEnv-1640

[teratorn]

Actually, as it turns out, the idea of defaulting to an empty environment is pretty bad. win32 applications will not function corretly without key environment variables set.

So, the only thing I'm going to do in the branch is to allow values in the env dict to be None, thus signifying that they should be unset from the parent environment. It is currently not possible to unset an environment variable that is set in the parent.

[bwh]

"Actually, as it turns out, the idea of defaulting to an empty environment is pretty bad. win32 applications will not function corretly without key environment variables set."

Yes, we know. However, as I pointed out in my original report, this isn't peculiar to Win32.

This environment-delta thing would solve my original problem. However it seems gratuitously inconsistent. Someone writing portable code will be forced to do:

if sys.platform == 'win32':
    env = {}
else:
    env = os.environ.copy()
# Do additions
env[...] = ...
# Do deletions
if sys.platform == 'win32':
    env[...] = None
else:
    del env[...]

Please implement consistent behaviour for Win32 and POSIX. I still think the environment should be the same as the parent process by default. This can be a security issue, but AFAIK every other process spawning function uses the same default so it should be expected behaviour. If that's not acceptable, there should be no default - env should be a required argument.

[jknight]

Replying to glyph:

spawnProcess will continue to present an empty environment by default, because that is more secure.

Before thinking about it, I bought this argument, but I don't now. Every other process spawning function defaults to current env, and lots of stuff depends on the contents of the env. So from a usability standpoint, it should default to current env. Now, what is the security problem that is fixed by not propagating the environment to subprocesses? The code you're running has to be trusted, and *your* environ had better be trusted or you're already screwed, so...

Explicit is better than implicit! It is easy enough to pass the environment through with os.environ, so if your subprocess is going to care about its environment at all, please remember to pass it through.

Both behaviors are implicit. One is implicitly clearing the env (against expectations), and the other is implicitly propagating it. All programs care about their environ, to one degree or another. So every use of spawnProcess ought to pass it through.

[teratorn]

Rebranched to win32procEnv-1640-2

[teratorn]

After further work and consideration I've changed my mind. I agree with jknight.

As he said, every other process spawning mechanism out there inherits the current environment by default, and I don't think we should have ever changed that particular convention. I also don't see any real security concerns.

The current branch changes the semantics of spawnProcess and friends such that: 1)If env is None (default) the current environment is used. 2)If env is specified it will be used without modification. The semantics are now the same on all platforms.

This *is* a backwards-incompatible change, but I don't think very many people will be adversely affected. What do you all think?

Ready for review. Don't worry, I won't merge until we have some kind of consensus from a few other core twisted people.

[Glyph]

process_envtest.py has some trailing whitespace.

Other than that, though, the changes look pretty good, and amazingly simple for something that has generated this much controversey.

I have definitely come around to thinking that propogating the environment by default is the right thing to do. It just doesn't make sense not to in most cases. In those where you are attempting to specifically provide a secure environment, the environment is already constructed explicitly.

Not to mention the fact that the most likely source of backwards incompatibility here has to do with the bug itself, which is that programs on Windows that do specify an environment will expect that the important features of the environment will be propogated to them.

The only thing I can think of to preserve compatibility here - and maybe this is a good idea - is to add a new environ keyword argument everywhere, and deprecate env, but leave the old behavior in place.

I'll discuss with other cabal members and encourage them to put some comments here. As far as code quality goes though, this definitely passes review (especially since it adds some tests for this behavior - for all the discussion of compatibility there are no tests which had to be deleted or were broken!)

[Jean-Paul Calderone]

Backwards compatibility ftw.

[Glyph]

OK, after discussion:

This should be changed to add a new "environ" keyword argument whose semantics are to always fully replace the child process's environment.

The behavior of the existing {{env}}} argument should not change.

We should, however, add a PendingDeprecationWarning for any code which passes env.

[jknight]

I'd like to note that we only just broke the win32 behavior a couple versions ago and that we're actually changing it back to what it was before (I think). Also that this scheme doesn't appear to help to move towards the really-desired behavior of inheriting the env by default. Unless that was implicit in the above comment?

[teratorn]

Yes, the default will be to inherit. Just to be clear we're talking about process spawning behavior on all platforms, not just win32. So after this change you will be able to get consistent cross-platform semantics using the environ keyword argument.

[jknight]

Replying to teratorn:

Yes, the default will be to inherit. Just to be clear we're talking about process spawning behavior on all platforms, not just win32. So after this change you will be able to get consistent cross-platform semantics using the environ keyword argument.

Okay, so let's be clear about this. The proposal is going to:

• spawnProcess(env=None, environ=None): spawns a process with os.environ as its environment (this is a change)
• spawnProcess(env={'FOO':'bar'}, environ=None): spawns a process with FOO=bar as its only environment value on non-windows, and with FOO=bar updated by the original value on windows. (this is the same)
• spawnProcess(env=None, environ={'FOO':'bar'): spawns a process with FOO=bar as its only environment on all platforms. (this is brand new)

Right?

[teratorn]

Replying to jknight:

I spoke too soon. The whole point of adding an environ keyword is to maintain backwards compatibility. That means that the default must remain the same. (empty env on non-Windows, and os.environ on Windows).

So I guess we can't have a new-style default behavior. I think that means that in order to properly deprecate env we have to mandate passing environ (or you get a DeprecationWarning).

The proposal, then, is like this:

• spawnProcess(env={}, environ=None): (default) spawns a process with an empty environment on non-Windows, and with os.environ on Windows (this is the same, except you get a DeprecationWarning)
• spawnProcess(env={'FOO':'bar'}, environ=None): spawns a process with FOO=bar as its only environment value on non-windows, and with os.environ updated by FOO=bar on windows. (this is the same, except you get a DeprecationWarning)
• spawnProcess(env={}, environ={'FOO':'bar'): spawns a process with FOO=bar as its only environment on all platforms. (this is brand new)
• spawnProcess(env={'FOO':'bar'}, environ={'FOO':'bar'): Raises an exception because passing both env and environ is ambiguous (this is brand new)

I don't really *like* making keyword arguments mandatory (mandatory in so far as you need it to avoid a DeprecationWarning). But I'm not sure how else to maintain backwards compatibility.

[teratorn]

This ticket has been superseded by #2415. Along with fixing the issues discussed here #2415 includes various other Big Ideas. The plan is to improve Twisted's process spawning/managing stuff as much as possible in one go.

[Jean-Paul Calderone]

This hasn't been superceded by #2415. Also, all plans which involve doing "as much as possible in one go" are categorically invalid. :)

[teratorn]

someone ought to have a look at this, or resolve the ticket, or resolve and make several more tickets - i'm not even bothering to review this, just removing myself as owner..