diff --git a/Lib/subprocess.py b/Lib/subprocess.py index c12898de168..7d3f77f71df 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 8b2699db05b..7b9e2b46d81 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 930b47ed9c6..899eab0cc86 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 a175d426100..0f85da97cea 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)); }