Opened 16 years ago
Last modified 4 years ago
#1640 defect new
Child process environment cannot be fully overridden on Windows
Reported by: | bwh | Owned by: | |
---|---|---|---|
Priority: | normal | Milestone: | |
Component: | core | Keywords: | win32 |
Cc: | jknight, Trent.Nelson, Jean-Paul Calderone | Branch: | |
Author: |
Description
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.
Attachments (1)
Change History (28)
comment:1 Changed 16 years ago by
Type: | enhancement → defect |
---|
comment:2 Changed 16 years ago by
Status: | new → assigned |
---|
comment:3 Changed 16 years ago by
comment:4 Changed 16 years ago by
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?
comment:5 follow-up: 13 Changed 16 years ago by
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.
comment:6 Changed 16 years ago by
Owner: | changed from Glyph to teratorn |
---|---|
Status: | assigned → new |
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.
comment:7 Changed 16 years ago by
Thanks guys. I tend to agree with glyph that passing env variables should be explicit. Will look in to this soonish.
comment:8 Changed 16 years ago by
Cc: | teratorn added |
---|---|
Keywords: | win32 added |
Status: | new → assigned |
comment:9 Changed 16 years ago by
Owner: | teratorn deleted |
---|---|
Status: | assigned → new |
comment:10 Changed 16 years ago by
Owner: | set to 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
comment:11 Changed 16 years ago by
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.
comment:12 Changed 16 years ago by
"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.
comment:13 Changed 16 years ago by
Cc: | jknight added |
---|
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.
comment:15 Changed 15 years ago by
Keywords: | review added |
---|
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.
comment:16 Changed 15 years ago by
Owner: | teratorn deleted |
---|
comment:17 Changed 15 years ago by
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!)
comment:19 Changed 15 years ago by
Keywords: | review removed |
---|---|
Owner: | set to teratorn |
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
.
comment:20 Changed 15 years ago by
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?
comment:21 follow-up: 22 Changed 15 years ago by
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.
comment:22 follow-up: 23 Changed 15 years ago by
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?
comment:23 Changed 15 years ago by
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.
comment:24 Changed 15 years ago by
comment:25 Changed 14 years ago by
Cc: | Trent.Nelson added |
---|
comment:26 Changed 14 years ago by
Cc: | Jean-Paul Calderone added |
---|
This hasn't been superceded by #2415. Also, all plans which involve doing "as much as possible in one go" are categorically invalid. :)
comment:27 Changed 4 years ago by
Cc: | teratorn removed |
---|---|
Owner: | teratorn deleted |
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..
Yep. Definitely a bug.