[Python-checkins] bpo-32270: Don't close stdin/out/err in pass_fds (GH-6242) (GH-9149)

[Gregory P. Smith]

https://github.com/python/cpython/commit/2173bb818c6c726d831b106ed0d3fad7825905dc
commit: 2173bb818c6c726d831b106ed0d3fad7825905dc
branch: 3.6
author: Miss Islington (bot) <31488909+miss-islington at users.noreply.github.com>
committer: Gregory P. Smith <greg at krypto.org>
date: 2018-09-10T23:00:47-07:00
summary:

bpo-32270: Don't close stdin/out/err in pass_fds (GH-6242) (GH-9149)

When subprocess.Popen() stdin= stdout= or stderr= handles are specified
and appear in pass_fds=, don't close the original fds after dup'ing them.

This implementation and unittest primarily came from @izbyshev (see the PR)

See also https://github.com/izbyshev/cpython/commit/b89b52f28490b69142d5c061604b3a3989cec66c

This also removes the old manual p2cread, c2pwrite, and errwrite closing logic
as inheritable flags and _close_open_fds takes care of that properly today without special treatment.

This code is within child_exec() where it is the only thread so there is no
race condition between the dup and _Py_set_inheritable_async_safe call.
(cherry picked from commit ce34410b8b67f49d8275c05d51b3ead50cf97f48)

Co-authored-by: Gregory P. Smith <greg at krypto.org> [Google]

files:
A Misc/NEWS.d/next/Library/2018-09-10-14-15-53.bpo-32270.wSJjuD.rst
M Lib/test/test_subprocess.py
M Modules/_posixsubprocess.c

diff --git a/Lib/test/test_subprocess.py b/Lib/test/test_subprocess.py
index 9ebe3228ac03..e1b24778430e 100644
--- a/Lib/test/test_subprocess.py
+++ b/Lib/test/test_subprocess.py
@@ -2493,6 +2493,36 @@ def test_pass_fds_inheritable(self):
         self.assertEqual(os.get_inheritable(inheritable), True)
         self.assertEqual(os.get_inheritable(non_inheritable), False)
 
+
+    # bpo-32270: Ensure that descriptors specified in pass_fds
+    # are inherited even if they are used in redirections.
+    # Contributed by @izbyshev.
+    def test_pass_fds_redirected(self):
+        """Regression test for https://bugs.python.org/issue32270."""
+        fd_status = support.findfile("fd_status.py", subdir="subprocessdata")
+        pass_fds = []
+        for _ in range(2):
+            fd = os.open(os.devnull, os.O_RDWR)
+            self.addCleanup(os.close, fd)
+            pass_fds.append(fd)
+
+        stdout_r, stdout_w = os.pipe()
+        self.addCleanup(os.close, stdout_r)
+        self.addCleanup(os.close, stdout_w)
+        pass_fds.insert(1, stdout_w)
+
+        with subprocess.Popen([sys.executable, fd_status],
+                              stdin=pass_fds[0],
+                              stdout=pass_fds[1],
+                              stderr=pass_fds[2],
+                              close_fds=True,
+                              pass_fds=pass_fds):
+            output = os.read(stdout_r, 1024)
+        fds = {int(num) for num in output.split(b',')}
+
+        self.assertEqual(fds, {0, 1, 2} | frozenset(pass_fds), f"output={output!a}")
+
+
     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)"],
diff --git a/Misc/NEWS.d/next/Library/2018-09-10-14-15-53.bpo-32270.wSJjuD.rst b/Misc/NEWS.d/next/Library/2018-09-10-14-15-53.bpo-32270.wSJjuD.rst
new file mode 100644
index 000000000000..83f68624c1be
--- /dev/null
+++ b/Misc/NEWS.d/next/Library/2018-09-10-14-15-53.bpo-32270.wSJjuD.rst
@@ -0,0 +1,2 @@
+The subprocess module no longer mistakenly closes redirected fds even when
+they were in pass_fds when outside of the default {0, 1, 2} set.
diff --git a/Modules/_posixsubprocess.c b/Modules/_posixsubprocess.c
index ad934dfe7d87..fe0e554618aa 100644
--- a/Modules/_posixsubprocess.c
+++ b/Modules/_posixsubprocess.c
@@ -422,10 +422,20 @@ child_exec(char *const exec_array[],
 
     /* When duping fds, if there arises a situation where one of the fds is
        either 0, 1 or 2, it is possible that it is overwritten (#12607). */
-    if (c2pwrite == 0)
+    if (c2pwrite == 0) {
         POSIX_CALL(c2pwrite = dup(c2pwrite));
-    while (errwrite == 0 || errwrite == 1)
+        /* issue32270 */
+        if (_Py_set_inheritable_async_safe(c2pwrite, 0, NULL) < 0) {
+            goto error;
+        }
+    }
+    while (errwrite == 0 || errwrite == 1) {
         POSIX_CALL(errwrite = dup(errwrite));
+        /* issue32270 */
+        if (_Py_set_inheritable_async_safe(errwrite, 0, NULL) < 0) {
+            goto error;
+        }
+    }
 
     /* Dup fds for child.
        dup2() removes the CLOEXEC flag but we must do it ourselves if dup2()
@@ -451,14 +461,8 @@ child_exec(char *const exec_array[],
     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 > 2)
-        POSIX_CALL(close(p2cread));
-    if (c2pwrite > 2 && c2pwrite != p2cread)
-        POSIX_CALL(close(c2pwrite));
-    if (errwrite != c2pwrite && errwrite != p2cread && errwrite > 2)
-        POSIX_CALL(close(errwrite));
+    /* We no longer manually close p2cread, c2pwrite, and errwrite here as
+     * _close_open_fds takes care when it is not already non-inheritable. */
 
     if (cwd)
         POSIX_CALL(chdir(cwd));