[Python-checkins] cpython (merge 3.3 -> default): Fixes issue #16140: The subprocess module no longer double closes its

[gregory.p.smith]

http://hg.python.org/cpython/rev/d51df72dadb7
changeset:   80381:d51df72dadb7
parent:      80378:44e5b48998ef
parent:      80380:9f8b0444c8a6
user:        Gregory P. Smith <greg at krypto.org>
date:        Sun Nov 11 01:41:49 2012 -0800
summary:
  Fixes issue #16140: The subprocess module no longer double closes its
child subprocess.PIPE parent file descriptors on child error prior to
exec().

This would lead to race conditions in multithreaded programs where
another thread opened a file reusing the fd which was then closed out
from beneath it by the errant second close.

files:
  Lib/subprocess.py           |   3 -
  Lib/test/test_subprocess.py |  39 +++++++++++++++++++++++++
  Misc/NEWS                   |   3 +
  3 files changed, 42 insertions(+), 3 deletions(-)


diff --git a/Lib/subprocess.py b/Lib/subprocess.py
--- a/Lib/subprocess.py
+++ b/Lib/subprocess.py
@@ -1419,9 +1419,6 @@
                 child_exception_type = getattr(
                         builtins, exception_name.decode('ascii'),
                         SubprocessError)
-                for fd in (p2cwrite, c2pread, errread):
-                    if fd != -1:
-                        os.close(fd)
                 err_msg = err_msg.decode(errors="surrogatepass")
                 if issubclass(child_exception_type, OSError) and hex_errno:
                     errno_num = int(hex_errno, 16)
diff --git a/Lib/test/test_subprocess.py b/Lib/test/test_subprocess.py
--- a/Lib/test/test_subprocess.py
+++ b/Lib/test/test_subprocess.py
@@ -1189,6 +1189,45 @@
             self.fail("Exception raised by preexec_fn did not make it "
                       "to the parent process.")
 
+    @unittest.skipIf(not os.path.exists("/dev/zero"), "/dev/zero required.")
+    def test_preexec_errpipe_does_not_double_close_pipes(self):
+        """Issue16140: Don't double close pipes on preexec error."""
+        class SafeConstructorPopen(subprocess.Popen):
+            def __init__(self):
+                pass  # Do nothing so we can modify the instance for testing.
+            def RealPopen(self, *args, **kwargs):
+                subprocess.Popen.__init__(self, *args, **kwargs)
+        def raise_it():
+            raise subprocess.SubprocessError(
+                    "force the _execute_child() errpipe_data path.")
+
+        p = SafeConstructorPopen()
+
+        def _test_fds_execute_child_wrapper(*args, **kwargs):
+            try:
+                subprocess.Popen._execute_child(p, *args, **kwargs)
+            finally:
+                # Open a bunch of file descriptors and verify that
+                # none of them are the same as the ones the Popen
+                # instance is using for stdin/stdout/stderr.
+                devzero_fds = [os.open("/dev/zero", os.O_RDONLY)
+                               for _ in range(8)]
+                try:
+                    for fd in devzero_fds:
+                        self.assertNotIn(fd, (
+                                p.stdin.fileno(), p.stdout.fileno(),
+                                p.stderr.fileno()),
+                                msg="At least one fd was closed early.")
+                finally:
+                    map(os.close, devzero_fds)
+
+        p._execute_child = _test_fds_execute_child_wrapper
+
+        with self.assertRaises(subprocess.SubprocessError):
+            p.RealPopen([sys.executable, "-c", "pass"],
+                        stdin=subprocess.PIPE, stdout=subprocess.PIPE,
+                        stderr=subprocess.PIPE, preexec_fn=raise_it)
+
     def test_preexec_gc_module_failure(self):
         # This tests the code that disables garbage collection if the child
         # process will execute any Python.
diff --git a/Misc/NEWS b/Misc/NEWS
--- a/Misc/NEWS
+++ b/Misc/NEWS
@@ -116,6 +116,9 @@
 Library
 -------
 
+- Issue #16140: The subprocess module no longer double closes its child
+  subprocess.PIPE parent file descriptors on child error prior to exec().
+
 - Remove a bare print to stdout from the subprocess module that could have
   happened if the child process wrote garbage to its pre-exec error pipe.
 

-- 
Repository URL: http://hg.python.org/cpython