Issue #6559: fix the subprocess.Popen pass_fds implementation. Add a unittest.

Issue #7213: Change the close_fds default on Windows to better match the new
default on POSIX.  True when possible (False if stdin/stdout/stderr are
supplied).

Update the documentation to reflect all of the above.

Doc/library/subprocess.rst

@@ -28,7 +28,7 @@ Using the subprocess Module
 This module defines one class called :class:`Popen`:
 
 
-.. class:: Popen(args, bufsize=0, executable=None, stdin=None, stdout=None, stderr=None, preexec_fn=None, close_fds=_PLATFORM_DEFAULT, shell=False, cwd=None, env=None, universal_newlines=False, startupinfo=None, creationflags=0, restore_signals=True, start_new_session=False)
+.. class:: Popen(args, bufsize=0, executable=None, stdin=None, stdout=None, stderr=None, preexec_fn=None, close_fds=True, shell=False, cwd=None, env=None, universal_newlines=False, startupinfo=None, creationflags=0, restore_signals=True, start_new_session=False, pass_fds=())
 
    Arguments are:
 
@@ -153,14 +153,22 @@ This module defines one class called :class:`Popen`:
 
    If *close_fds* is true, all file descriptors except :const:`0`, :const:`1` and
    :const:`2` will be closed before the child process is executed. (Unix only).
-   The default varies by platform: :const:`False` on Windows and :const:`True`
-   on POSIX and other platforms.
+   The default varies by platform:  Always true on Unix.  On Windows it is
+   true when *stdin*/*stdout*/*stderr* are :const:`None`, false otherwise.
    On Windows, if *close_fds* is true then no handles will be inherited by the
    child process.  Note that on Windows, you cannot set *close_fds* to true and
    also redirect the standard handles by setting *stdin*, *stdout* or *stderr*.
 
-.. versionchanged:: 3.2
-   The default was changed to True on non Windows platforms.
+   .. versionchanged:: 3.2
+      The default for *close_fds* was changed from :const:`False` to
+      what is described above.
+
+   *pass_fds* is an optional sequence of file descriptors to keep open
+   between the parent and child.  Providing any *pass_fds* forces
+   *close_fds* to be :const:`True`.  (Unix only)
+
+   .. versionadded:: 3.2
+      The *pass_fds* parameter was added.
 
    If *cwd* is not ``None``, the child's current directory will be changed to *cwd*
    before it is executed.  Note that this directory is not considered when

Lib/subprocess.py

@@ -27,10 +27,10 @@ This module defines one class called Popen:
 
 class Popen(args, bufsize=0, executable=None,
             stdin=None, stdout=None, stderr=None,
-            preexec_fn=None, close_fds=_PLATFORM_DEFAULT, shell=False,
+            preexec_fn=None, close_fds=True, shell=False,
             cwd=None, env=None, universal_newlines=False,
             startupinfo=None, creationflags=0,
-            restore_signals=True, start_new_session=False):
+            restore_signals=True, start_new_session=False, pass_fds=()):
 
 
 Arguments are:
@@ -81,8 +81,11 @@ is executed.
 
 If close_fds is true, all file descriptors except 0, 1 and 2 will be
 closed before the child process is executed.  The default for close_fds
-varies by platform: False on Windows and True on all other platforms
-such as POSIX.
+varies by platform:  Always true on POSIX.  True when stdin/stdout/stderr
+are None on Windows, false otherwise.
+
+pass_fds is an optional sequence of file descriptors to keep open between the
+parent and child.  Providing any pass_fds implicitly sets close_fds to true.
 
 if shell is true, the specified command will be executed through the
 shell.
@@ -621,17 +624,14 @@ def getoutput(cmd):
     return getstatusoutput(cmd)[1]
 
 
-if mswindows:
-    _PLATFORM_DEFAULT = False
-else:
-    _PLATFORM_DEFAULT = True
+_PLATFORM_DEFAULT_CLOSE_FDS = object()
 
 
 class Popen(object):
     def __init__(self, args, bufsize=0, executable=None,
                  stdin=None, stdout=None, stderr=None,
-                 preexec_fn=None, close_fds=_PLATFORM_DEFAULT, shell=False,
-                 cwd=None, env=None, universal_newlines=False,
+                 preexec_fn=None, close_fds=_PLATFORM_DEFAULT_CLOSE_FDS,
+                 shell=False, cwd=None, env=None, universal_newlines=False,
                  startupinfo=None, creationflags=0,
                  restore_signals=True, start_new_session=False,
                  pass_fds=()):
@@ -648,12 +648,24 @@ class Popen(object):
             if preexec_fn is not None:
                 raise ValueError("preexec_fn is not supported on Windows "
                                  "platforms")
-            if close_fds and (stdin is not None or stdout is not None or
-                              stderr is not None):
-                raise ValueError("close_fds is not supported on Windows "
-                                 "platforms if you redirect stdin/stdout/stderr")
+            any_stdio_set = (stdin is not None or stdout is not None or
+                             stderr is not None)
+            if close_fds is _PLATFORM_DEFAULT_CLOSE_FDS:
+                if any_stdio_set:
+                    close_fds = False
+                else:
+                    close_fds = True
+            elif close_fds and any_stdio_set:
+                raise ValueError(
+                        "close_fds is not supported on Windows platforms"
+                        " if you redirect stdin/stdout/stderr")
         else:
             # POSIX
+            if close_fds is _PLATFORM_DEFAULT_CLOSE_FDS:
+                close_fds = True
+            if pass_fds and not close_fds:
+                warnings.warn("pass_fds overriding close_fds.", RuntimeWarning)
+                close_fds = True
             if startupinfo is not None:
                 raise ValueError("startupinfo is only supported on Windows "
                                  "platforms")
@@ -661,9 +673,6 @@ class Popen(object):
                 raise ValueError("creationflags is only supported on Windows "
                                  "platforms")
 
-        if pass_fds and not close_fds:
-            raise ValueError("pass_fds requires close_fds=True.")
-
         self.stdin = None
         self.stdout = None
         self.stderr = None
@@ -876,7 +885,7 @@ class Popen(object):
                            unused_restore_signals, unused_start_new_session):
             """Execute program (MS Windows version)"""
 
-            assert not pass_fds, "pass_fds not yet supported on Windows"
+            assert not pass_fds, "pass_fds not supported on Windows."
 
             if not isinstance(args, str):
                 args = list2cmdline(args)
@@ -1091,7 +1100,7 @@ class Popen(object):
             # precondition: fds_to_keep must be sorted and unique
             start_fd = 3
             for fd in fds_to_keep:
-                if fd > start_fd:
+                if fd >= start_fd:
                     os.closerange(start_fd, fd)
                     start_fd = fd + 1
             if start_fd <= MAXFD:

Lib/test/test_subprocess.py

@@ -1042,6 +1042,37 @@ class POSIXProcessTestCase(BaseTestCase):
                          "Some fds were left open")
         self.assertIn(1, remaining_fds, "Subprocess failed")
 
+    def test_pass_fds(self):
+        fd_status = support.findfile("fd_status.py", subdir="subprocessdata")
+
+        open_fds = set()
+
+        for x in range(5):
+            fds = os.pipe()
+            self.addCleanup(os.close, fds[0])
+            self.addCleanup(os.close, fds[1])
+            open_fds.update(fds)
+
+        for fd in open_fds:
+            p = subprocess.Popen([sys.executable, fd_status],
+                                 stdout=subprocess.PIPE, close_fds=True,
+                                 pass_fds=(fd, ))
+            output, ignored = p.communicate()
+
+            remaining_fds = set(map(int, output.split(b',')))
+            to_be_closed = open_fds - {fd}
+
+            self.assertIn(fd, remaining_fds, "fd to be passed not passed")
+            self.assertFalse(remaining_fds & to_be_closed,
+                             "fd to be closed passed")
+
+            # pass_fds overrides close_fds with a warning.
+            with self.assertWarns(RuntimeWarning) as context:
+                self.assertFalse(subprocess.call(
+                        [sys.executable, "-c", "import sys; sys.exit(0)"],
+                        close_fds=False, pass_fds=(fd, )))
+            self.assertIn('overriding close_fds', str(context.warning))
+
 
 @unittest.skipUnless(mswindows, "Windows specific tests")
 class Win32ProcessTestCase(BaseTestCase):

Misc/NEWS

@@ -23,8 +23,12 @@ Library
 - Issue #10107: Warn about unsaved files in IDLE on OSX.
 
 - Issue #7213: subprocess.Popen's default for close_fds has been changed.
-  It is now platform specific, keeping its default of False on Windows and
-  changing the default to True on POSIX and other platforms.
+  It is now True in most cases other than on Windows when input, output or
+  error handles are provided.
+
+- Issue #6559: subprocess.Popen has a new pass_fds parameter (actually
+  added in 3.2beta1) to allow specifying a specific list of file descriptors
+  to keep open in the child process.
 
 
 What's New in Python 3.2 Beta 1?

Modules/_posixsubprocess.c

@@ -107,7 +107,7 @@ static void child_exec(char *const exec_array[],
                 errno = 0;  /* We don't want to report an OSError. */
                 goto error;
             }
-            if (keep_fd <= start_fd)
+            if (keep_fd < start_fd)
                 continue;
             for (fd_num = start_fd; fd_num < keep_fd; ++fd_num) {
                 close(fd_num);