[Python-checkins] bpo-32844: Fix a subprocess misredirection of a low fd (GH5689)

Miss Islington (bot) webhook-mailer at python.org
Mon Mar 26 16:43:49 EDT 2018


https://github.com/python/cpython/commit/57db13e582ad269d6e067fe934122207cc992739
commit: 57db13e582ad269d6e067fe934122207cc992739
branch: 3.6
author: Miss Islington (bot) <31488909+miss-islington at users.noreply.github.com>
committer: GitHub <noreply at github.com>
date: 2018-03-26T13:43:46-07:00
summary:

bpo-32844: Fix a subprocess misredirection of a low fd (GH5689)


bpo-32844: subprocess: Fix a potential misredirection of a low fd to stderr.

When redirecting, subprocess attempts to achieve the following state:
each fd to be redirected to is less than or equal to the fd
it is redirected from, which is necessary because redirection
occurs in the ascending order of destination descriptors.
It fails to do so in a couple of corner cases,
for example, if 1 is redirected to 2 and 0 is closed in the parent.
(cherry picked from commit 0e7144b064a19493a146af94175a087b3888c37b)

Co-authored-by: Alexey Izbyshev <izbyshev at users.noreply.github.com>

files:
A Misc/NEWS.d/next/Library/2018-02-28-13-08-00.bpo-32844.u8tnAe.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 aca1411cdfb5..9ebe3228ac03 100644
--- a/Lib/test/test_subprocess.py
+++ b/Lib/test/test_subprocess.py
@@ -6,6 +6,7 @@
 import platform
 import signal
 import io
+import itertools
 import os
 import errno
 import tempfile
@@ -2108,6 +2109,55 @@ def test_swap_fds(self):
         self.check_swap_fds(2, 0, 1)
         self.check_swap_fds(2, 1, 0)
 
+    def _check_swap_std_fds_with_one_closed(self, from_fds, to_fds):
+        saved_fds = self._save_fds(range(3))
+        try:
+            for from_fd in from_fds:
+                with tempfile.TemporaryFile() as f:
+                    os.dup2(f.fileno(), from_fd)
+
+            fd_to_close = (set(range(3)) - set(from_fds)).pop()
+            os.close(fd_to_close)
+
+            arg_names = ['stdin', 'stdout', 'stderr']
+            kwargs = {}
+            for from_fd, to_fd in zip(from_fds, to_fds):
+                kwargs[arg_names[to_fd]] = from_fd
+
+            code = textwrap.dedent(r'''
+                import os, sys
+                skipped_fd = int(sys.argv[1])
+                for fd in range(3):
+                    if fd != skipped_fd:
+                        os.write(fd, str(fd).encode('ascii'))
+            ''')
+
+            skipped_fd = (set(range(3)) - set(to_fds)).pop()
+
+            rc = subprocess.call([sys.executable, '-c', code, str(skipped_fd)],
+                                 **kwargs)
+            self.assertEqual(rc, 0)
+
+            for from_fd, to_fd in zip(from_fds, to_fds):
+                os.lseek(from_fd, 0, os.SEEK_SET)
+                read_bytes = os.read(from_fd, 1024)
+                read_fds = list(map(int, read_bytes.decode('ascii')))
+                msg = textwrap.dedent(f"""
+                    When testing {from_fds} to {to_fds} redirection,
+                    parent descriptor {from_fd} got redirected
+                    to descriptor(s) {read_fds} instead of descriptor {to_fd}.
+                """)
+                self.assertEqual([to_fd], read_fds, msg)
+        finally:
+            self._restore_fds(saved_fds)
+
+    # Check that subprocess can remap std fds correctly even
+    # if one of them is closed (#32844).
+    def test_swap_std_fds_with_one_closed(self):
+        for from_fds in itertools.combinations(range(3), 2):
+            for to_fds in itertools.permutations(range(3), 2):
+                self._check_swap_std_fds_with_one_closed(from_fds, to_fds)
+
     def test_surrogates_error_message(self):
         def prepare():
             raise ValueError("surrogate:\uDCff")
diff --git a/Misc/NEWS.d/next/Library/2018-02-28-13-08-00.bpo-32844.u8tnAe.rst b/Misc/NEWS.d/next/Library/2018-02-28-13-08-00.bpo-32844.u8tnAe.rst
new file mode 100644
index 000000000000..67412fe5ba46
--- /dev/null
+++ b/Misc/NEWS.d/next/Library/2018-02-28-13-08-00.bpo-32844.u8tnAe.rst
@@ -0,0 +1,2 @@
+Fix wrong redirection of a low descriptor (0 or 1) to stderr in subprocess
+if another low descriptor is closed.
diff --git a/Modules/_posixsubprocess.c b/Modules/_posixsubprocess.c
index 05a08eb87765..ad934dfe7d87 100644
--- a/Modules/_posixsubprocess.c
+++ b/Modules/_posixsubprocess.c
@@ -424,7 +424,7 @@ child_exec(char *const exec_array[],
        either 0, 1 or 2, it is possible that it is overwritten (#12607). */
     if (c2pwrite == 0)
         POSIX_CALL(c2pwrite = dup(c2pwrite));
-    if (errwrite == 0 || errwrite == 1)
+    while (errwrite == 0 || errwrite == 1)
         POSIX_CALL(errwrite = dup(errwrite));
 
     /* Dup fds for child.



More information about the Python-checkins mailing list