diff options
author | Gregory P. Smith <greg@krypto.org> | 2018-09-10 17:46:22 -0700 |
---|---|---|
committer | GitHub <noreply@github.com> | 2018-09-10 17:46:22 -0700 |
commit | ce34410b8b67f49d8275c05d51b3ead50cf97f48 (patch) | |
tree | 08e99370a122056aa4101d670e505217961c4058 | |
parent | 880d42a3b247306f67837aa95e23f7c3471a30a3 (diff) | |
download | cpython-git-ce34410b8b67f49d8275c05d51b3ead50cf97f48.tar.gz |
bpo-32270: Don't close stdin/out/err in pass_fds (GH-6242)
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.
-rw-r--r-- | Lib/test/test_subprocess.py | 30 | ||||
-rw-r--r-- | Misc/NEWS.d/next/Library/2018-09-10-14-15-53.bpo-32270.wSJjuD.rst | 2 | ||||
-rw-r--r-- | Modules/_posixsubprocess.c | 24 |
3 files changed, 46 insertions, 10 deletions
diff --git a/Lib/test/test_subprocess.py b/Lib/test/test_subprocess.py index 4719773b67..8419061b2a 100644 --- a/Lib/test/test_subprocess.py +++ b/Lib/test/test_subprocess.py @@ -2529,6 +2529,36 @@ class POSIXProcessTestCase(BaseTestCase): 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 0000000000..83f68624c1 --- /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 0150fcb097..aeb10f9ecf 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)); |