From 9c4f44f70acadd70fcdb0a0d5d66cefbc9da20bf Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith" Date: Tue, 15 Mar 2011 14:56:39 -0400 Subject: [PATCH 1/5] Fix issue #11432. if the stdin pipe is the same file descriptor as either stdout or stderr in the _posixsubprocess C extension module it would unintentionally close the fds and raise an error. --- Modules/_posixsubprocess.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Modules/_posixsubprocess.c b/Modules/_posixsubprocess.c index 0f85da97cea..bf10cbb43e1 100644 --- a/Modules/_posixsubprocess.c +++ b/Modules/_posixsubprocess.c @@ -99,10 +99,10 @@ static void child_exec(char *const exec_array[], if (p2cread > 2) { POSIX_CALL(close(p2cread)); } - if (c2pwrite > 2) { + if (c2pwrite > 2 && c2pwrite != p2cread) { POSIX_CALL(close(c2pwrite)); } - if (errwrite != c2pwrite && errwrite > 2) { + if (errwrite != c2pwrite && errwrite != p2cread && errwrite > 2) { POSIX_CALL(close(errwrite)); } From e14e9c2218dc449a374a31a5bc4f9e1a82ef61fa Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith" Date: Tue, 15 Mar 2011 14:55:17 -0400 Subject: [PATCH 2/5] Add unittests demonstrating issue #11432. --- Lib/test/test_subprocess.py | 36 +++++++++++++++++++++--------------- 1 file changed, 21 insertions(+), 15 deletions(-) diff --git a/Lib/test/test_subprocess.py b/Lib/test/test_subprocess.py index 73f44ad93af..46e50c350bf 100644 --- a/Lib/test/test_subprocess.py +++ b/Lib/test/test_subprocess.py @@ -3,6 +3,7 @@ from test import support import subprocess import sys import signal +import io import os import errno import tempfile @@ -1186,6 +1187,24 @@ class POSIXProcessTestCase(BaseTestCase): close_fds=False, pass_fds=(fd, ))) self.assertIn('overriding close_fds', str(context.warning)) + def test_stdout_stdin_are_single_inout_fd(self): + with io.open(os.devnull, "r+") as inout: + p = subprocess.Popen([sys.executable, "-c", "import sys; sys.exit(0)"], + stdout=inout, stdin=inout) + p.wait() + + def test_stdout_stderr_are_single_inout_fd(self): + with io.open(os.devnull, "r+") as inout: + p = subprocess.Popen([sys.executable, "-c", "import sys; sys.exit(0)"], + stdout=inout, stderr=inout) + p.wait() + + def test_stderr_stdin_are_single_inout_fd(self): + with io.open(os.devnull, "r+") as inout: + p = subprocess.Popen([sys.executable, "-c", "import sys; sys.exit(0)"], + stderr=inout, stdin=inout) + p.wait() + def test_wait_when_sigchild_ignored(self): # NOTE: sigchild_ignore.py may not be an effective test on all OSes. sigchild_ignore = support.findfile("sigchild_ignore.py", @@ -1458,19 +1477,6 @@ class ContextManagerTests(ProcessTestCase): raise c.exception -def test_main(): - unit_tests = (ProcessTestCase, - POSIXProcessTestCase, - Win32ProcessTestCase, - ProcessTestCasePOSIXPurePython, - CommandTests, - ProcessTestCaseNoPoll, - HelperFunctionTests, - CommandsWithSpaces, - ContextManagerTests) - - support.run_unittest(*unit_tests) - support.reap_children() - if __name__ == "__main__": - test_main() + unittest.main() + support.reap_children() From 8f7724f9a4ec57d689a436a064e2e047b2ad0d97 Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith" Date: Tue, 15 Mar 2011 15:24:43 -0400 Subject: [PATCH 3/5] merge d71476b9a55d from tip, use start_new_session instead of os.setsid. --- Lib/webbrowser.py | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/Lib/webbrowser.py b/Lib/webbrowser.py index e369acb1df6..415f12ac5f3 100644 --- a/Lib/webbrowser.py +++ b/Lib/webbrowser.py @@ -228,15 +228,9 @@ class UnixBrowser(BaseBrowser): else: # for TTY browsers, we need stdin/out inout = None - # if possible, put browser in separate process group, so - # keyboard interrupts don't affect browser as well as Python - setsid = getattr(os, 'setsid', None) - if not setsid: - setsid = getattr(os, 'setpgrp', None) - p = subprocess.Popen(cmdline, close_fds=True, stdin=inout, stdout=(self.redirect_stdout and inout or None), - stderr=inout, preexec_fn=setsid) + stderr=inout, start_new_session=True) if remote: # wait five secons. If the subprocess is not finished, the # remote invocation has (hopefully) started a new instance. From 961e0e85c007677ba4406382d6951a39487d440c Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith" Date: Tue, 15 Mar 2011 15:43:39 -0400 Subject: [PATCH 4/5] revert the test_main() change from 08daf3ef6509 so that regrtest continues to run this properly. --- Lib/test/test_subprocess.py | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/Lib/test/test_subprocess.py b/Lib/test/test_subprocess.py index 46e50c350bf..3cc387b6a3d 100644 --- a/Lib/test/test_subprocess.py +++ b/Lib/test/test_subprocess.py @@ -1477,6 +1477,19 @@ class ContextManagerTests(ProcessTestCase): raise c.exception +def test_main(): + unit_tests = (ProcessTestCase, + POSIXProcessTestCase, + Win32ProcessTestCase, + ProcessTestCasePOSIXPurePython, + CommandTests, + ProcessTestCaseNoPoll, + HelperFunctionTests, + CommandsWithSpaces, + ContextManagerTests) + + support.run_unittest(*unit_tests) + support.reap_children() + if __name__ == "__main__": unittest.main() - support.reap_children() From 28edfeedb5fc4fc64190cfcc0d32494a8a84c4cb Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith" Date: Tue, 15 Mar 2011 16:02:10 -0400 Subject: [PATCH 5/5] issue 11432 news entry. --- Misc/NEWS | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/Misc/NEWS b/Misc/NEWS index adc719bf2f5..c1100650352 100644 --- a/Misc/NEWS +++ b/Misc/NEWS @@ -10,6 +10,10 @@ What's New in Python 3.2.1? Core and Builtins ----------------- +- Issue #11432: A bug was introduced in subprocess.Popen on posix systems with + 3.2.0 where the stdout or stderr file descriptor being the same as the stdin + file descriptor would raise an exception. webbrowser.open would fail. fixed. + - Issue #11450: Don't truncate hg version info in Py_GetBuildInfo() when there are many tags (e.g. when using mq). Patch by Nadeem Vawda.