From 9bc9c8e15ac6d5281a52b3628a7d48bb48852aeb Mon Sep 17 00:00:00 2001 From: Jonathan Stewmon Date: Thu, 21 Jan 2016 17:59:32 -0600 Subject: [PATCH] ensure file descriptors given to pass_fds are inheritable this patch ports a subset of the code that was added to _posixsubprocess.c and filutils.c to resolve #18571 (Implementation of the PEP 446: non-inheritable file descriptors) in cpython 3.4 Signed-off-by: Jonathan Stewmon --- _posixsubprocess.c | 26 ++++++++++++++++++ _posixsubprocess_helpers.c | 56 ++++++++++++++++++++++++++++++++++++++ subprocess32.py | 2 ++ test_subprocess32.py | 17 +++++++++++- testdata/qcat.py | 12 ++++++-- 5 files changed, 109 insertions(+), 4 deletions(-) diff --git a/_posixsubprocess.c b/_posixsubprocess.c index 3481892..9ada02b 100644 --- a/_posixsubprocess.c +++ b/_posixsubprocess.c @@ -154,6 +154,29 @@ _is_fd_in_sorted_fd_sequence(int fd, PyObject *fd_sequence) return 0; } +static int +make_inheritable(PyObject *py_fds_to_keep, int errpipe_write) +{ + Py_ssize_t i, len; + + len = PySequence_Length(py_fds_to_keep); + for (i = 0; i < len; ++i) { + PyObject* fdobj = PySequence_Fast_GET_ITEM(py_fds_to_keep, i); + long fd = PyLong_AsLong(fdobj); + assert(!PyErr_Occurred()); + assert(0 <= fd && fd <= INT_MAX); + if (fd == errpipe_write) { + /* errpipe_write is part of py_fds_to_keep. It must be closed at + exec(), but kept open in the child process until exec() is + called. */ + continue; + } + if (set_inheritable((int) fd, 1) < 0) + return -1; + } + return 0; +} + /* Get the maximum file descriptor that could be opened by this process. * This function is async signal safe for use between fork() and exec(). @@ -391,6 +414,9 @@ child_exec(char *const exec_array[], /* Buffer large enough to hold a hex integer. We can't malloc. */ char hex_errno[sizeof(saved_errno)*2+1]; + if (make_inheritable(py_fds_to_keep, errpipe_write) < 0) + goto error; + /* Close parent's pipe ends. */ if (p2cwrite != -1) { POSIX_CALL(close(p2cwrite)); diff --git a/_posixsubprocess_helpers.c b/_posixsubprocess_helpers.c index ad24b4f..ab2d23d 100644 --- a/_posixsubprocess_helpers.c +++ b/_posixsubprocess_helpers.c @@ -161,3 +161,59 @@ _Py_RestoreSignals(void) PyOS_setsig(SIGXFSZ, SIG_DFL); #endif } + + +static int +set_inheritable(int fd, int inheritable) +{ +#if defined(HAVE_SYS_IOCTL_H) && defined(FIOCLEX) && defined(FIONCLEX) + static int ioctl_works = -1; + int request; + int err; +#endif + int flags; + int res; + +#if defined(HAVE_SYS_IOCTL_H) && defined(FIOCLEX) && defined(FIONCLEX) + if (ioctl_works != 0) { + /* fast-path: ioctl() only requires one syscall */ + if (inheritable) + request = FIONCLEX; + else + request = FIOCLEX; + err = ioctl(fd, request, NULL); + if (!err) { + ioctl_works = 1; + return 0; + } + + if (errno != ENOTTY) { + return -1; + } + else { + /* Issue #22258: Here, ENOTTY means "Inappropriate ioctl for + device". The ioctl is declared but not supported by the kernel. + Remember that ioctl() doesn't work. It is the case on + Illumos-based OS for example. */ + ioctl_works = 0; + } + /* fallback to fcntl() if ioctl() does not work */ + } +#endif + + /* slow-path: fcntl() requires two syscalls */ + flags = fcntl(fd, F_GETFD); + if (flags < 0) { + return -1; + } + + if (inheritable) + flags &= ~FD_CLOEXEC; + else + flags |= FD_CLOEXEC; + res = fcntl(fd, F_SETFD, flags); + if (res < 0) { + return -1; + } + return 0; +} diff --git a/subprocess32.py b/subprocess32.py index b0f5982..542f257 100644 --- a/subprocess32.py +++ b/subprocess32.py @@ -1301,6 +1301,8 @@ def _dup2(a, b): fds_to_keep.add(errpipe_write) self._close_all_but_a_sorted_few_fds( sorted(fds_to_keep)) + for fd in fds_to_keep: + _set_cloexec(fd, False) else: self._close_fds(but=errpipe_write) diff --git a/test_subprocess32.py b/test_subprocess32.py index 2f58811..5d29989 100644 --- a/test_subprocess32.py +++ b/test_subprocess32.py @@ -1865,7 +1865,6 @@ def test_close_fds_when_max_fd_is_lowered(self): self.assertFalse(remaining_fds & open_fds, msg="Some fds were left open.") - def test_pass_fds(self): fd_status = test_support.findfile("testdata/fd_status.py") @@ -1897,6 +1896,22 @@ def test_pass_fds(self): # close_fds=False, pass_fds=(fd, ))) #self.assertIn('overriding close_fds', str(context.warning)) + def test_pass_fds_clears_close_on_exec(self): + qcat = test_support.findfile("testdata/qcat.py") + p1 = subprocess.Popen( + [sys.executable, '-c', 'import sys; sys.stdout.write("hi")'], + stdout=subprocess.PIPE + ) + p2 = subprocess.Popen( + + [sys.executable, qcat, '/dev/fd/{0}'.format(p1.stdout.fileno())], + stdout=subprocess.PIPE, + pass_fds=(p1.stdout.fileno(),) + ) + output, ignored = p2.communicate() + self.assertEqual('hi', output) + + def test_stdout_stdin_are_single_inout_fd(self): inout = open(os.devnull, "r+") try: diff --git a/testdata/qcat.py b/testdata/qcat.py index fe6f9db..2afbd6c 100644 --- a/testdata/qcat.py +++ b/testdata/qcat.py @@ -1,7 +1,13 @@ -"""When ran as a script, simulates cat with no arguments.""" +"""When ran as a script, simulates cat.""" import sys if __name__ == "__main__": - for line in sys.stdin: - sys.stdout.write(line) + if len(sys.argv) == 1: + files = [sys.stdin] + else: + files = (sys.stdin if f == '-' else open(f) for f in sys.argv[1:]) + for infile in files: + with infile: + for line in infile: + sys.stdout.write(line)