From 657fd10e03623a693da9272d05a1cdaf393d8469 Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith" Date: Sun, 25 Mar 2018 12:30:26 -0700 Subject: [PATCH 1/4] bpo-32270: Don't close stdin/out/err in pass_fds 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. --- Modules/_posixsubprocess.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/Modules/_posixsubprocess.c b/Modules/_posixsubprocess.c index 0150fcb0970c0e5..17cafdcfb9b61e1 100644 --- a/Modules/_posixsubprocess.c +++ b/Modules/_posixsubprocess.c @@ -453,11 +453,13 @@ child_exec(char *const exec_array[], /* Close pipe fds. Make sure we don't close the same fd more than */ /* once, or standard fds. */ - if (p2cread > 2) + if (p2cread > 2 && !_is_fd_in_sorted_fd_sequence(p2cread, py_fds_to_keep)) POSIX_CALL(close(p2cread)); - if (c2pwrite > 2 && c2pwrite != p2cread) + if (c2pwrite > 2 && c2pwrite != p2cread && + !_is_fd_in_sorted_fd_sequence(c2pwrite, py_fds_to_keep)) POSIX_CALL(close(c2pwrite)); - if (errwrite != c2pwrite && errwrite != p2cread && errwrite > 2) + if (errwrite != c2pwrite && errwrite != p2cread && errwrite > 2 && + !_is_fd_in_sorted_fd_sequence(errwrite, py_fds_to_keep)) POSIX_CALL(close(errwrite)); if (cwd) From 6b7077eac5b090f10972388cabcdd8c837da2401 Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith" Date: Mon, 10 Sep 2018 13:27:32 -0700 Subject: [PATCH 2/4] Adds a unittest from @izbyshev. Taken from https://github.com/izbyshev/cpython/commit/b89b52f28490b69142d5c061604b3a3989cec66c per the suggestion in the BPO issue comment. --- Lib/test/test_subprocess.py | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/Lib/test/test_subprocess.py b/Lib/test/test_subprocess.py index 4719773b67b7d91..8419061b2a90134 100644 --- a/Lib/test/test_subprocess.py +++ b/Lib/test/test_subprocess.py @@ -2529,6 +2529,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)"], From 3561c67d54b94ac151b7433c142005918da30ae5 Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith" Date: Mon, 10 Sep 2018 14:02:02 -0700 Subject: [PATCH 3/4] Set c2pwrite & errwrite non-inheritable when duped Based on code review by izbyshev@ and 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 them properly today. 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. --- Modules/_posixsubprocess.c | 26 ++++++++++++++------------ 1 file changed, 14 insertions(+), 12 deletions(-) diff --git a/Modules/_posixsubprocess.c b/Modules/_posixsubprocess.c index 17cafdcfb9b61e1..aeb10f9ecfe47c9 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,16 +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 && !_is_fd_in_sorted_fd_sequence(p2cread, py_fds_to_keep)) - POSIX_CALL(close(p2cread)); - if (c2pwrite > 2 && c2pwrite != p2cread && - !_is_fd_in_sorted_fd_sequence(c2pwrite, py_fds_to_keep)) - POSIX_CALL(close(c2pwrite)); - if (errwrite != c2pwrite && errwrite != p2cread && errwrite > 2 && - !_is_fd_in_sorted_fd_sequence(errwrite, py_fds_to_keep)) - 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)); From 4941a125502842d296e1c58140b13da3aa240bc0 Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith" Date: Mon, 10 Sep 2018 14:15:57 -0700 Subject: [PATCH 4/4] NEWS! --- .../next/Library/2018-09-10-14-15-53.bpo-32270.wSJjuD.rst | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 Misc/NEWS.d/next/Library/2018-09-10-14-15-53.bpo-32270.wSJjuD.rst 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 000000000000000..83f68624c1be82b --- /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.