Issue 13524: subprocess on Windows
http://bugs.python.org/issue13524 Someone raised issue13524 yesterday to illustrate that a subprocess will crash immediately if an environment block is passed which does not contain a valid SystemRoot environment variable. Note that the calling (Python) process is unaffected; this isn't - strictly - a Python crash. The issue is essentially a Windows one where a fairly unusual cornercase -- passing an empty environment -- has unforseen effects. The smallest reproducible example is this: import os, sys import subprocess subprocess.Popen( [sys.executable], env={} ) and it can be prevented like this: import os, sys import subprocess subprocess.Popen( [sys.executable], env={"SystemRoot" : os.environ['SystemRoot']} ) There's a blog post here which gives a worked example: http://jpassing.com/2009/12/28/the-hidden-danger-of-forgetting-to-specify-sy... but as the author points out, nowhere on MSDN is there a warning that SystemRoot is mandatory. (And, in effect, it's not as it would just be possible to write code which had no need of it). So... what's our take on this? As I see it we could: 1) Do nothing: it's the caller's responsibility to understand the complications of the chosen Operating System. 2) Add a doc warning (ironically, considering the recent to-and-fro on doc warnings in this very module). 3) Add a check into the subprocess.Popen code which would raise some exception if the environment block is empty (or doesn't contain SystemRoot) on the grounds that this probably wasn't what the user thought they were doing. 4) Automatically add an entry for SystemRoot to the env block if it's not present already. It's tempting to opt for (1) and if we were exposing an API called CreateProcess which mimicked the underlying Windows API I would be inclined to go that way. But we're abstracting a little bit away from that and I think that that layer of abstraction carries its own responsibilities. Option (3) seems to give the best balance. It *is* a cornercase, but at the same time it's easy to misunderstand that the env block you're passing in *replaces* rather than *augments* that of the current process. Thoughts? TJG
On Sun, Dec 4, 2011 at 8:59 PM, Tim Golden <mail@timgolden.me.uk> wrote:
So... what's our take on this? As I see it we could:
1) Do nothing: it's the caller's responsibility to understand the complications of the chosen Operating System.
2) Add a doc warning (ironically, considering the recent to-and-fro on doc warnings in this very module).
3) Add a check into the subprocess.Popen code which would raise some exception if the environment block is empty (or doesn't contain SystemRoot) on the grounds that this probably wasn't what the user thought they were doing.
4) Automatically add an entry for SystemRoot to the env block if it's not present already.
It's tempting to opt for (1) and if we were exposing an API called CreateProcess which mimicked the underlying Windows API I would be inclined to go that way. But we're abstracting a little bit away from that and I think that that layer of abstraction carries its own responsibilities.
Option (3) seems to give the best balance. It *is* a cornercase, but at the same time it's easy to misunderstand that the env block you're passing in *replaces* rather than *augments* that of the current process.
There's actually two questions to be answered: 1. What should we do in 3.2 and 2.7? 2. Should we do anything more in 3.3? Raising an exception is not really an appropriate response for any of them - running without SystemRoot actually works fine in most cases, so raising an exception could break currently working code. As the blog post noted, it's only some specific modules that don't work if SystemRoot is not set. Should we really be inserting workarounds in subprocess for buggy platform code that doesn't fall back to a sensible default if a particular environment variable isn't set? So, I don't think this is really a subprocess problem at all. It's a platform bug on Windows that means the 'random' module may fail if SystemRoot is not set in the environment. So, I think the right approach is to: 1. Unset 'SystemRoot' in a windows shell 2. Run the test suite and observe the scale of the breakage 3. Then either: - figure out a workaround that allows us to set an appropriate default value for SystemRoot if needed (depending on the scope of the problem, either do this at interpreter startup, or only in affected modules) - if no feasible workaround is found, detect the failures related to this problem and report a more meaningful error message Either way, add explicit tests to the test suite to ensure that affected modules behave as expected when SystemRoot is not set. Cheers, Nick. -- Nick Coghlan | ncoghlan@gmail.com | Brisbane, Australia
On 04/12/2011 11:42, Nick Coghlan wrote:
There's actually two questions to be answered: 1. What should we do in 3.2 and 2.7? 2. Should we do anything more in 3.3?
Agreed.
1. Unset 'SystemRoot' in a windows shell 2. Run the test suite and observe the scale of the breakage
Sorry; something I should have highlighted in the earlier post. Behaviour varies between Windows versions. On WinXP, if you unset SystemRoot in a cmd shell, you won't be able to run the test suite: Python won't even start up. On Win7 Python will start but, eg, the random module will fail. This is actually a separate issue: how much of Python will work without a valid SystemRoot. The OP's issue was that if you use subprocess to start an arbitrary process (you get the same problem if you try "notepad.exe") and pass it an env block without a valid SystemRoot then that process will likely fail to start up. And it won't be obvious why. The case where someone tries to run Python (in general) without a valid SystemRoot is a tiny cornercase and you'd be quite right to push that back and say "Don't do that". I don't believe we have to test for it or add code to work around it. While I put the idea forward, I agree that an exception is more likely than not to break existing code. I just can't see any clear alternative, apart from option 1: we do nothing. TJG
On 4 December 2011 12:20, Tim Golden <mail@timgolden.me.uk> wrote:
On 04/12/2011 11:42, Nick Coghlan wrote:
There's actually two questions to be answered: 1. What should we do in 3.2 and 2.7? 2. Should we do anything more in 3.3?
See below...
This is actually a separate issue: how much of Python will work without a valid SystemRoot. The OP's issue was that if you use subprocess to start an arbitrary process (you get the same problem if you try "notepad.exe") and pass it an env block without a valid SystemRoot then that process will likely fail to start up. And it won't be obvious why.
I'm not 100% clear on the problem here. From how I'm reading things, the problem is that not supplying SystemRoot will cause (some or all) invocations of subprocess.Popen to fail - it's not specific to starting Python. In that case, it seems to me that it's an OS issue, but one that we should work around. My feeling is that option 4 is best - set SystemRoot to its current value if it's not been set by the user. This leaves the user unable to set an environment with SystemRoot missing, but if the OS fails to handle that properly, then I'm OK with that limitation. As regards the version question above, I'd take the view that as an OS issue, it's OK to leave it unchanged in 2.7 and 3.2, but add the above to 3.3. Paul.
On 04/12/2011 12:41, Paul Moore wrote:
I'm not 100% clear on the problem here. From how I'm reading things, the problem is that not supplying SystemRoot will cause (some or all) invocations of subprocess.Popen to fail - it's not specific to starting Python.
That's basically the situation.
My feeling is that option 4 is best - set SystemRoot to its current value if it's not been set by the user. This leaves the user unable to set an environment with SystemRoot missing, but if the OS fails to handle that properly, then I'm OK with that limitation.
FWIW if we went this route we could set it if it's missing but that still allows the user to set it to blank. I'm just a little bit wary of altering the environment which the user believes has been set. TJG
That's why I'm suggesting we look specifically at the cases where *Python* misbehaves in an empty environment on Windows. Those are legitimately our issue. The problem in *general* is a platform one, so I don't think it makes sense for us to modify the environment that has explicitly been passed in (e.g. how would you test running without SystemRoot if subprocess added it automatically?). An extra parameter in the already confusing Popen signature wouldn't be clearer than explicitly copying os.environ and modifying it. -- Nick Coghlan (via Gmail on Android, so likely to be more terse than usual) On Dec 4, 2011 10:22 PM, "Tim Golden" <mail@timgolden.me.uk> wrote:
On 04/12/2011 11:42, Nick Coghlan wrote:
There's actually two questions to be answered: 1. What should we do in 3.2 and 2.7? 2. Should we do anything more in 3.3?
Agreed.
1. Unset 'SystemRoot' in a windows shell
2. Run the test suite and observe the scale of the breakage
Sorry; something I should have highlighted in the earlier post. Behaviour varies between Windows versions. On WinXP, if you unset SystemRoot in a cmd shell, you won't be able to run the test suite: Python won't even start up. On Win7 Python will start but, eg, the random module will fail.
This is actually a separate issue: how much of Python will work without a valid SystemRoot. The OP's issue was that if you use subprocess to start an arbitrary process (you get the same problem if you try "notepad.exe") and pass it an env block without a valid SystemRoot then that process will likely fail to start up. And it won't be obvious why.
The case where someone tries to run Python (in general) without a valid SystemRoot is a tiny cornercase and you'd be quite right to push that back and say "Don't do that". I don't believe we have to test for it or add code to work around it.
While I put the idea forward, I agree that an exception is more likely than not to break existing code. I just can't see any clear alternative, apart from option 1: we do nothing.
TJG ______________________________**_________________ Python-Dev mailing list Python-Dev@python.org http://mail.python.org/**mailman/listinfo/python-dev<http://mail.python.org/mailman/listinfo/python-dev> Unsubscribe: http://mail.python.org/**mailman/options/python-dev/** ncoghlan%40gmail.com<http://mail.python.org/mailman/options/python-dev/ncoghlan%40gmail.com>
On 04/12/2011, Tim Golden <mail@timgolden.me.uk> wrote:
Someone raised issue13524 yesterday to illustrate that a subprocess will crash immediately if an environment block is passed which does not contain a valid SystemRoot environment variable.
...
2) Add a doc warning (ironically, considering the recent to-and-fro on doc warnings in this very module).
From the bug, the problem with the reporter's code is he passes a dict with the one value he cares about as `env` to subprocess.Popen without realising that it will prevent the inheriting of the current environment. Your suggested fix for him also has an issue, it changes
There appears to already be such a warning, added because of a similar earlier bug: <http://bugs.python.org/issue3440> Really this is a problem with the subprocess api making a common case harder to do than necessary. If you read the documentation, you'll get it right, but that's not ideal: <http://sourcefrog.net/weblog/software/aesthetics/interface-levels.html> the environment of the parent process without resetting it. Instead you need something like: e = dict(os.environ) e['PATH_TO_MY_APPS'] = "path/to/my/apps" The bzrlib TestCase has a method using subprocess that provides an `env_changes` argument. With that, it's much easier to override or remove just one variable without accidentally clearing the current environment. Martin
On 12/4/2011 5:59 AM, Tim Golden wrote:
http://bugs.python.org/issue13524
Someone raised issue13524 yesterday to illustrate that a subprocess will crash immediately if an environment block is passed which does not contain a valid SystemRoot environment variable.
Note that the calling (Python) process is unaffected; this isn't - strictly - a Python crash. The issue is essentially a Windows one where a fairly unusual cornercase -- passing an empty environment -- has unforseen effects.
The smallest reproducible example is this:
import os, sys import subprocess subprocess.Popen( [sys.executable], env={} )
and it can be prevented like this:
import os, sys import subprocess subprocess.Popen( [sys.executable], env={"SystemRoot" : os.environ['SystemRoot']} )
There's a blog post here which gives a worked example:
http://jpassing.com/2009/12/28/the-hidden-danger-of-forgetting-to-specify-sy...
but as the author points out, nowhere on MSDN is there a warning that SystemRoot is mandatory. (And, in effect, it's not as it would just be possible to write code which had no need of it).
So... what's our take on this? As I see it we could:
1) Do nothing: it's the caller's responsibility to understand the complications of the chosen Operating System.
2) Add a doc warning (ironically, considering the recent to-and-fro on doc warnings in this very module).
3) Add a check into the subprocess.Popen code which would raise some exception if the environment block is empty (or doesn't contain SystemRoot) on the grounds that this probably wasn't what the user thought they were doing.
4) Automatically add an entry for SystemRoot to the env block if it's not present already.
It's tempting to opt for (1) and if we were exposing an API called CreateProcess which mimicked the underlying Windows API I would be inclined to go that way. But we're abstracting a little bit away from that and I think that that layer of abstraction carries its own responsibilities.
Option (3) seems to give the best balance. It *is* a cornercase, but at the same time it's easy to misunderstand that the env block you're passing in *replaces* rather than *augments* that of the current process.
Thoughts?
My inclination would be #4 on Windows, certainly for 3.3, unless there is a clear reason not to. For 2.7/3.2, at least say (not warn, just say) in the doc that that a subprocess on Windows may require that SystemRoot be set. The blog post says the problem is worse on Win 7. So it is not going away. The blog post has a comment from Martin Loewis a year ago linking to http://mail.python.org/pipermail/python-dev/2010-November/105866.html That thread refers to a bug that was not posted on the tracker. This makes at least three (including #3440). -- Terry Jan Reedy
On Mon, Dec 5, 2011 at 7:08 AM, Terry Reedy <tjreedy@udel.edu> wrote:
My inclination would be #4 on Windows, certainly for 3.3, unless there is a clear reason not to.
Yes, there is: that environment is the *exact* environment that should be passed to the child processes. It's not our place to go implicitly adding things to it. If MS aren't willing to add SystemRoot automatically in CreateProcess (despite releasing libraries that require it to be set), there's no way we should be adding it for them. Fixing our stuff (like importing the random module) to work to at least some degree even if SystemRoot isn't set should definitely be done, but beyond that a comment in the docs pointing out the problem (i.e. MS releasing things that require SystemRoot be set without updating CreateProcess to ensure that it *is* set) is as far as we should go. Regards, Nick. -- Nick Coghlan | ncoghlan@gmail.com | Brisbane, Australia
Thoughts?
Apparently, there are at least two "users" of SystemRoot: - side-by-side (fusion?) apparently uses it to locate the WinSxS folder, at least on some Windows releases, - certain registry keys contain SystemRoot, in particular the path names of crypto providers (this apparently is XP only, and fixed on Windows 7) I agree with Nick that we shouldn't do anything except perhaps for documentation changes. There are many other environment variables whose absence could also cause failures to run the executable, such as PATH, LD_LIBRARY_PATH, etc. Even not passing DISPLAY may cause the subprocess to fail starting. IOW, users should "normally" pass all environment variables, and only augment it with any specific additions and deletions that they know are needed for the subprocess. If a user deliberately passes a small set of environment variables (e.g. none), we must assume that it was deliberate, and that any resulting failures are desired. People do such stuff for security reasons, and side-stepping their enforcement is not appropriate for Python to do. Regards, Martin
On 05/12/2011 08:10, "Martin v. Löwis" wrote:
I agree with Nick that we shouldn't do anything except perhaps for documentation changes. There are many other environment variables whose absence could also cause failures to run the executable, such as PATH, LD_LIBRARY_PATH, etc. Even not passing DISPLAY may cause the subprocess to fail starting.
IOW, users should "normally" pass all environment variables, and only augment it with any specific additions and deletions that they know are needed for the subprocess. If a user deliberately passes a small set of environment variables (e.g. none), we must assume that it was deliberate, and that any resulting failures are desired. People do such stuff for security reasons, and side-stepping their enforcement is not appropriate for Python to do.
Having slept on this I must confess that this is pretty much the conclusion I'd come to: we can't do anything in code which is guaranteed to be correct in every case. The best we can do is document. And, as Martin Packman pointed out (and I had missed), this particular condition is already documented, at least enough to point a user to. We could probably do with a HOWTO (or blog post or whatever) on using subprocess on Windows, not least because a fair amount of the docs are Unix-centric and actually very slightly confusing for naive Windows-based developers. I think my proposal now is: do nothing. I'm aware that Nick Coghlan has been making fairly extensive changes to the subprocess docs recently and I don't I can propose anything on this matter which amounts to more than shuffling the pieces around. TJG
On Mon, Dec 5, 2011 at 7:01 PM, Tim Golden <mail@timgolden.me.uk> wrote:
We could probably do with a HOWTO (or blog post or whatever) on using subprocess on Windows, not least because a fair amount of the docs are Unix-centric and actually very slightly confusing for naive Windows-based developers.
I think my proposal now is: do nothing. I'm aware that Nick Coghlan has been making fairly extensive changes to the subprocess docs recently and I don't I can propose anything on this matter which amounts to more than shuffling the pieces around.
The subprocess module could probably do with a HOWTO, full stop. Subprocess invocation is something where platform details are always going to matter a lot, and there are subtle details even on Unix that are confusing (e.g. I have a command in my current project that I've only managed to get working by running it via the shell - I still don't know why direct invocation of the binary with the appropriate arguments doesn't work). At the moment, we're still trying to cram an entire essay on subprocess invocation into the subprocess.Popen constructor definition, which is far from optimal. Cheers, Nick. -- Nick Coghlan | ncoghlan@gmail.com | Brisbane, Australia
participants (6)
-
"Martin v. Löwis" -
Martin Packman -
Nick Coghlan -
Paul Moore -
Terry Reedy -
Tim Golden