Issue #10806, issue #9905: Fix subprocess pipes when some of the standard

file descriptors (0, 1, 2) are closed in the parent process.  Initial
patch by Ross Lagerwall.

4 files changed, 104 insertions, 33 deletions

diff --git a/Lib/subprocess.py b/Lib/subprocess.py
index c12898d..7d3f77f 100644
--- a/Lib/subprocess.py
+++ b/Lib/subprocess.py
@@ -393,22 +393,22 @@ else:
     # POSIX defines PIPE_BUF as >= 512.
     _PIPE_BUF = getattr(select, 'PIPE_BUF', 512)
 
+    _FD_CLOEXEC = getattr(fcntl, 'FD_CLOEXEC', 1)
+
+    def _set_cloexec(fd, cloexec):
+        old = fcntl.fcntl(fd, fcntl.F_GETFD)
+        if cloexec:
+            fcntl.fcntl(fd, fcntl.F_SETFD, old | _FD_CLOEXEC)
+        else:
+            fcntl.fcntl(fd, fcntl.F_SETFD, old & ~_FD_CLOEXEC)
+
     if _posixsubprocess:
         _create_pipe = _posixsubprocess.cloexec_pipe
     else:
         def _create_pipe():
-            try:
-                cloexec_flag = fcntl.FD_CLOEXEC
-            except AttributeError:
-                cloexec_flag = 1
-
             fds = os.pipe()
-
-            old = fcntl.fcntl(fds[0], fcntl.F_GETFD)
-            fcntl.fcntl(fds[0], fcntl.F_SETFD, old | cloexec_flag)
-            old = fcntl.fcntl(fds[1], fcntl.F_GETFD)
-            fcntl.fcntl(fds[1], fcntl.F_SETFD, old | cloexec_flag)
-
+            _set_cloexec(fds[0], True)
+            _set_cloexec(fds[1], True)
             return fds
 
 __all__ = ["Popen", "PIPE", "STDOUT", "call", "check_call", "getstatusoutput",
@@ -1194,23 +1194,25 @@ class Popen(object):
                                 os.close(errpipe_read)
 
                                 # Dup fds for child
-                                if p2cread != -1:
-                                    os.dup2(p2cread, 0)
-                                if c2pwrite != -1:
-                                    os.dup2(c2pwrite, 1)
-                                if errwrite != -1:
-                                    os.dup2(errwrite, 2)
+                                def _dup2(a, b):
+                                    # dup2() removes the CLOEXEC flag but
+                                    # we must do it ourselves if dup2()
+                                    # would be a no-op (issue #10806).
+                                    if a == b:
+                                        _set_cloexec(a, False)
+                                    elif a != -1:
+                                        os.dup2(a, b)
+                                _dup2(p2cread, 0)
+                                _dup2(c2pwrite, 1)
+                                _dup2(errwrite, 2)
 
                                 # Close pipe fds.  Make sure we don't close the
                                 # same fd more than once, or standard fds.
-                                if p2cread != -1 and p2cread not in (0,):
-                                    os.close(p2cread)
-                                if (c2pwrite != -1 and
-                                    c2pwrite not in (p2cread, 1)):
-                                    os.close(c2pwrite)
-                                if (errwrite != -1 and
-                                    errwrite not in (p2cread, c2pwrite, 2)):
-                                    os.close(errwrite)
+                                closed = set()
+                                for fd in [p2cread, c2pwrite, errwrite]:
+                                    if fd > 2 and fd not in closed:
+                                        os.close(fd)
+                                        closed.add(fd)
 
                                 # Close all other fds, if asked for
                                 if close_fds:
diff --git a/Lib/test/test_subprocess.py b/Lib/test/test_subprocess.py
index 8b2699d..7b9e2b4 100644
--- a/Lib/test/test_subprocess.py
+++ b/Lib/test/test_subprocess.py
@@ -903,6 +903,58 @@ class POSIXProcessTestCase(BaseTestCase):
         self.assertStderrEqual(stderr, b'')
         self.assertEqual(p.wait(), -signal.SIGTERM)
 
+    def check_close_std_fds(self, fds):
+        # Issue #9905: test that subprocess pipes still work properly with
+        # some standard fds closed
+        stdin = 0
+        newfds = []
+        for a in fds:
+            b = os.dup(a)
+            newfds.append(b)
+            if a == 0:
+                stdin = b
+        try:
+            for fd in fds:
+                os.close(fd)
+            out, err = subprocess.Popen([sys.executable, "-c",
+                              'import sys;'
+                              'sys.stdout.write("apple");'
+                              'sys.stdout.flush();'
+                              'sys.stderr.write("orange")'],
+                       stdin=stdin,
+                       stdout=subprocess.PIPE,
+                       stderr=subprocess.PIPE).communicate()
+            err = support.strip_python_stderr(err)
+            self.assertEqual((out, err), (b'apple', b'orange'))
+        finally:
+            for b, a in zip(newfds, fds):
+                os.dup2(b, a)
+            for b in newfds:
+                os.close(b)
+
+    def test_close_fd_0(self):
+        self.check_close_std_fds([0])
+
+    def test_close_fd_1(self):
+        self.check_close_std_fds([1])
+
+    def test_close_fd_2(self):
+        self.check_close_std_fds([2])
+
+    def test_close_fds_0_1(self):
+        self.check_close_std_fds([0, 1])
+
+    def test_close_fds_0_2(self):
+        self.check_close_std_fds([0, 2])
+
+    def test_close_fds_1_2(self):
+        self.check_close_std_fds([1, 2])
+
+    def test_close_fds_0_1_2(self):
+        # Issue #10806: test that subprocess pipes still work properly with
+        # all standard fds closed.
+        self.check_close_std_fds([0, 1, 2])
+
     def test_surrogates_error_message(self):
         def prepare():
             raise ValueError("surrogate:\uDCff")
diff --git a/Misc/NEWS b/Misc/NEWS
index 930b47e..899eab0 100644
--- a/Misc/NEWS
+++ b/Misc/NEWS
@@ -23,6 +23,10 @@ Core and Builtins
 Library
 -------
 
+- Issue #10806, issue #9905: Fix subprocess pipes when some of the standard
+  file descriptors (0, 1, 2) are closed in the parent process.  Initial
+  patch by Ross Lagerwall.
+
 - `unittest.TestCase` can be instantiated without a method name; for simpler
   exploration from the interactive interpreter.
 
diff --git a/Modules/_posixsubprocess.c b/Modules/_posixsubprocess.c
index a175d42..0f85da9 100644
--- a/Modules/_posixsubprocess.c
+++ b/Modules/_posixsubprocess.c
@@ -69,27 +69,40 @@ static void child_exec(char *const exec_array[],
     }
     POSIX_CALL(close(errpipe_read));
 
-    /* Dup fds for child. */
-    if (p2cread != -1) {
+    /* Dup fds for child.
+       dup2() removes the CLOEXEC flag but we must do it ourselves if dup2()
+       would be a no-op (issue #10806). */
+    if (p2cread == 0) {
+        int old = fcntl(p2cread, F_GETFD);
+        if (old != -1)
+            fcntl(p2cread, F_SETFD, old & ~FD_CLOEXEC);
+    } else if (p2cread != -1) {
         POSIX_CALL(dup2(p2cread, 0));  /* stdin */
     }
-    if (c2pwrite != -1) {
+    if (c2pwrite == 1) {
+        int old = fcntl(c2pwrite, F_GETFD);
+        if (old != -1)
+            fcntl(c2pwrite, F_SETFD, old & ~FD_CLOEXEC);
+    } else if (c2pwrite != -1) {
         POSIX_CALL(dup2(c2pwrite, 1));  /* stdout */
     }
-    if (errwrite != -1) {
+    if (errwrite == 2) {
+        int old = fcntl(errwrite, F_GETFD);
+        if (old != -1)
+            fcntl(errwrite, F_SETFD, old & ~FD_CLOEXEC);
+    } else if (errwrite != -1) {
         POSIX_CALL(dup2(errwrite, 2));  /* stderr */
     }
 
     /* Close pipe fds.  Make sure we don't close the same fd more than */
     /* once, or standard fds. */
-    if (p2cread != -1 && p2cread != 0) {
+    if (p2cread > 2) {
         POSIX_CALL(close(p2cread));
     }
-    if (c2pwrite != -1 && c2pwrite != p2cread && c2pwrite != 1) {
+    if (c2pwrite > 2) {
         POSIX_CALL(close(c2pwrite));
     }
-    if (errwrite != -1 && errwrite != p2cread &&
-        errwrite != c2pwrite && errwrite != 2) {
+    if (errwrite != c2pwrite && errwrite > 2) {
         POSIX_CALL(close(errwrite));
     }