From 51ee270876f4e18ec579e57772e16fce146d805e Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith" Date: Mon, 13 Dec 2010 07:59:39 +0000 Subject: [PATCH] issue7213: Open the pipes used by subprocesses with the FD_CLOEXEC flag from the C code, using pipe2() when available. Adds unittests for close_fds and cloexec behaviors. --- Lib/subprocess.py | 36 +++++++----- Lib/test/subprocessdata/fd_status.py | 24 ++++++++ Lib/test/subprocessdata/input_reader.py | 7 +++ Lib/test/subprocessdata/qcat.py | 7 +++ Lib/test/subprocessdata/qgrep.py | 10 ++++ Lib/test/test_subprocess.py | 78 +++++++++++++++++++++++++ Makefile.pre.in | 2 +- Modules/_posixsubprocess.c | 44 ++++++++++++++ Tools/msi/msi.py | 2 + configure.in | 2 +- 10 files changed, 195 insertions(+), 17 deletions(-) create mode 100644 Lib/test/subprocessdata/fd_status.py create mode 100644 Lib/test/subprocessdata/input_reader.py create mode 100644 Lib/test/subprocessdata/qcat.py create mode 100644 Lib/test/subprocessdata/qgrep.py diff --git a/Lib/subprocess.py b/Lib/subprocess.py index 62dfd36c4c5..729a53b622b 100644 --- a/Lib/subprocess.py +++ b/Lib/subprocess.py @@ -390,6 +390,23 @@ else: # POSIX defines PIPE_BUF as >= 512. _PIPE_BUF = getattr(select, 'PIPE_BUF', 512) + 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) + + return fds __all__ = ["Popen", "PIPE", "STDOUT", "call", "check_call", "getstatusoutput", "getoutput", "check_output", "CalledProcessError"] @@ -1031,7 +1048,7 @@ class Popen(object): if stdin is None: pass elif stdin == PIPE: - p2cread, p2cwrite = os.pipe() + p2cread, p2cwrite = _create_pipe() elif isinstance(stdin, int): p2cread = stdin else: @@ -1041,7 +1058,7 @@ class Popen(object): if stdout is None: pass elif stdout == PIPE: - c2pread, c2pwrite = os.pipe() + c2pread, c2pwrite = _create_pipe() elif isinstance(stdout, int): c2pwrite = stdout else: @@ -1051,7 +1068,7 @@ class Popen(object): if stderr is None: pass elif stderr == PIPE: - errread, errwrite = os.pipe() + errread, errwrite = _create_pipe() elif stderr == STDOUT: errwrite = c2pwrite elif isinstance(stderr, int): @@ -1065,16 +1082,6 @@ class Popen(object): errread, errwrite) - def _set_cloexec_flag(self, fd): - try: - cloexec_flag = fcntl.FD_CLOEXEC - except AttributeError: - cloexec_flag = 1 - - old = fcntl.fcntl(fd, fcntl.F_GETFD) - fcntl.fcntl(fd, fcntl.F_SETFD, old | cloexec_flag) - - def _close_fds(self, but): os.closerange(3, but) os.closerange(but + 1, MAXFD) @@ -1116,10 +1123,9 @@ class Popen(object): # For transferring possible exec failure from child to parent. # Data format: "exception name:hex errno:description" # Pickle is not used; it is complex and involves memory allocation. - errpipe_read, errpipe_write = os.pipe() + errpipe_read, errpipe_write = _create_pipe() try: try: - self._set_cloexec_flag(errpipe_write) if _posixsubprocess: # We must avoid complex work that could involve diff --git a/Lib/test/subprocessdata/fd_status.py b/Lib/test/subprocessdata/fd_status.py new file mode 100644 index 00000000000..083b2f9ec55 --- /dev/null +++ b/Lib/test/subprocessdata/fd_status.py @@ -0,0 +1,24 @@ +"""When called as a script, print a comma-separated list of the open +file descriptors on stdout.""" + +import errno +import os +import fcntl + +try: + _MAXFD = os.sysconf("SC_OPEN_MAX") +except: + _MAXFD = 256 + +def isopen(fd): + """Return True if the fd is open, and False otherwise""" + try: + fcntl.fcntl(fd, fcntl.F_GETFD, 0) + except IOError as e: + if e.errno == errno.EBADF: + return False + raise + return True + +if __name__ == "__main__": + print(','.join(str(fd) for fd in range(0, _MAXFD) if isopen(fd))) diff --git a/Lib/test/subprocessdata/input_reader.py b/Lib/test/subprocessdata/input_reader.py new file mode 100644 index 00000000000..ccae5f3fa55 --- /dev/null +++ b/Lib/test/subprocessdata/input_reader.py @@ -0,0 +1,7 @@ +"""When called as a script, consumes the input""" + +import sys + +if __name__ = "__main__": + for line in sys.stdin: + pass diff --git a/Lib/test/subprocessdata/qcat.py b/Lib/test/subprocessdata/qcat.py new file mode 100644 index 00000000000..fe6f9db25c9 --- /dev/null +++ b/Lib/test/subprocessdata/qcat.py @@ -0,0 +1,7 @@ +"""When ran as a script, simulates cat with no arguments.""" + +import sys + +if __name__ == "__main__": + for line in sys.stdin: + sys.stdout.write(line) diff --git a/Lib/test/subprocessdata/qgrep.py b/Lib/test/subprocessdata/qgrep.py new file mode 100644 index 00000000000..69906379a9b --- /dev/null +++ b/Lib/test/subprocessdata/qgrep.py @@ -0,0 +1,10 @@ +"""When called with a single argument, simulated fgrep with a single +argument and no options.""" + +import sys + +if __name__ == "__main__": + pattern = sys.argv[1] + for line in sys.stdin: + if pattern in line: + sys.stdout.write(line) diff --git a/Lib/test/test_subprocess.py b/Lib/test/test_subprocess.py index eaa26d2ba67..74e74043895 100644 --- a/Lib/test/test_subprocess.py +++ b/Lib/test/test_subprocess.py @@ -10,6 +10,7 @@ import time import re import sysconfig import warnings +import select try: import gc except ImportError: @@ -964,6 +965,83 @@ class POSIXProcessTestCase(BaseTestCase): exitcode = subprocess.call([program, "-c", "pass"], env=envb) self.assertEqual(exitcode, 0) + def test_pipe_cloexec(self): + sleeper = support.findfile("input_reader.py", subdir="subprocessdata") + fd_status = support.findfile("fd_status.py", subdir="subprocessdata") + + p1 = subprocess.Popen([sys.executable, sleeper], + stdin=subprocess.PIPE, stdout=subprocess.PIPE, + stderr=subprocess.PIPE, close_fds=False) + + self.addCleanup(p1.communicate, b'') + + p2 = subprocess.Popen([sys.executable, fd_status], + stdout=subprocess.PIPE, close_fds=False) + + output, error = p2.communicate() + result_fds = set(map(int, output.split(b','))) + unwanted_fds = set([p1.stdin.fileno(), p1.stdout.fileno(), + p1.stderr.fileno()]) + + self.assertFalse(result_fds & unwanted_fds, + "Expected no fds from %r to be open in child, " + "found %r" % + (unwanted_fds, result_fds & unwanted_fds)) + + def test_pipe_cloexec_real_tools(self): + qcat = support.findfile("qcat.py", subdir="subprocessdata") + qgrep = support.findfile("qgrep.py", subdir="subprocessdata") + + subdata = b'zxcvbn' + data = subdata * 4 + b'\n' + + p1 = subprocess.Popen([sys.executable, qcat], + stdin=subprocess.PIPE, stdout=subprocess.PIPE, + close_fds=False) + + p2 = subprocess.Popen([sys.executable, qgrep, subdata], + stdin=p1.stdout, stdout=subprocess.PIPE, + close_fds=False) + + self.addCleanup(p1.wait) + self.addCleanup(p2.wait) + self.addCleanup(p1.terminate) + self.addCleanup(p2.terminate) + + p1.stdin.write(data) + p1.stdin.close() + + readfiles, ignored1, ignored2 = select.select([p2.stdout], [], [], 10) + + self.assertTrue(readfiles, "The child hung") + self.assertEqual(p2.stdout.read(), data) + + def test_close_fds(self): + fd_status = support.findfile("fd_status.py", subdir="subprocessdata") + + fds = os.pipe() + self.addCleanup(os.close, fds[0]) + self.addCleanup(os.close, fds[1]) + + open_fds = set(fds) + + p = subprocess.Popen([sys.executable, fd_status], + stdout=subprocess.PIPE, close_fds=False) + output, ignored = p.communicate() + remaining_fds = set(map(int, output.split(b','))) + + self.assertEqual(remaining_fds & open_fds, open_fds, + "Some fds were closed") + + p = subprocess.Popen([sys.executable, fd_status], + stdout=subprocess.PIPE, close_fds=True) + output, ignored = p.communicate() + remaining_fds = set(map(int, output.split(b','))) + + self.assertFalse(remaining_fds & open_fds, + "Some fds were left open") + self.assertIn(1, remaining_fds, "Subprocess failed") + @unittest.skipUnless(mswindows, "Windows specific tests") class Win32ProcessTestCase(BaseTestCase): diff --git a/Makefile.pre.in b/Makefile.pre.in index 4c7c28bc9cc..4ce952f45da 100644 --- a/Makefile.pre.in +++ b/Makefile.pre.in @@ -888,7 +888,7 @@ MACHDEPS= $(PLATDIR) $(EXTRAPLATDIR) XMLLIBSUBDIRS= xml xml/dom xml/etree xml/parsers xml/sax LIBSUBDIRS= tkinter tkinter/test tkinter/test/test_tkinter \ tkinter/test/test_ttk site-packages test \ - test/decimaltestdata test/xmltestdata \ + test/decimaltestdata test/xmltestdata test/subprocessdata \ test/tracedmodules test/encoded_modules \ concurrent concurrent/futures encodings \ email email/mime email/test email/test/data \ diff --git a/Modules/_posixsubprocess.c b/Modules/_posixsubprocess.c index 36da2e5fa95..0b5e54470fa 100644 --- a/Modules/_posixsubprocess.c +++ b/Modules/_posixsubprocess.c @@ -1,6 +1,10 @@ /* Authors: Gregory P. Smith & Jeffrey Yasskin */ #include "Python.h" +#ifdef HAVE_PIPE2 +#define _GNU_SOURCE +#endif #include +#include #define POSIX_CALL(call) if ((call) == -1) goto error @@ -398,6 +402,45 @@ Returns: the child process's PID.\n\ Raises: Only on an error in the parent process.\n\ "); +PyDoc_STRVAR(subprocess_cloexec_pipe_doc, +"cloexec_pipe() -> (read_end, write_end)\n\n\ +Create a pipe whose ends have the cloexec flag set."); + +static PyObject * +subprocess_cloexec_pipe(PyObject *self, PyObject *noargs) +{ + int fds[2]; + int res; +#ifdef HAVE_PIPE2 + Py_BEGIN_ALLOW_THREADS + res = pipe2(fds, O_CLOEXEC); + Py_END_ALLOW_THREADS +#else + /* We hold the GIL which offers some protection from other code calling + * fork() before the CLOEXEC flags have been set but we can't guarantee + * anything without pipe2(). */ + long oldflags; + + res = pipe(fds); + + if (res == 0) { + oldflags = fcntl(fds[0], F_GETFD, 0); + if (oldflags < 0) res = oldflags; + } + if (res == 0) + res = fcntl(fds[0], F_SETFD, oldflags | FD_CLOEXEC); + + if (res == 0) { + oldflags = fcntl(fds[1], F_GETFD, 0); + if (oldflags < 0) res = oldflags; + } + if (res == 0) + res = fcntl(fds[1], F_SETFD, oldflags | FD_CLOEXEC); +#endif + if (res != 0) + return PyErr_SetFromErrno(PyExc_OSError); + return Py_BuildValue("(ii)", fds[0], fds[1]); +} /* module level code ********************************************************/ @@ -407,6 +450,7 @@ PyDoc_STRVAR(module_doc, static PyMethodDef module_methods[] = { {"fork_exec", subprocess_fork_exec, METH_VARARGS, subprocess_fork_exec_doc}, + {"cloexec_pipe", subprocess_cloexec_pipe, METH_NOARGS, subprocess_cloexec_pipe_doc}, {NULL, NULL} /* sentinel */ }; diff --git a/Tools/msi/msi.py b/Tools/msi/msi.py index 2288cf35198..4cebad210b6 100644 --- a/Tools/msi/msi.py +++ b/Tools/msi/msi.py @@ -1035,6 +1035,8 @@ def add_files(db): if dir=='xmltestdata': lib.glob("*.xml") lib.add_file("test.xml.out") + if dir=='subprocessdata': + lib.glob("*.py") if dir=='output': lib.glob("test_*") if dir=='sndhdrdata': diff --git a/configure.in b/configure.in index 03181332d79..fe030b37a7f 100644 --- a/configure.in +++ b/configure.in @@ -4235,7 +4235,7 @@ case $ac_sys_system in OSF*) AC_MSG_ERROR(OSF* systems are deprecated unless somebody volunteers. Check http://bugs.python.org/issue8606) ;; esac - +AC_CHECK_FUNC(pipe2, AC_DEFINE(HAVE_PIPE2, 1, [Define if the OS supports pipe2()]), ) AC_SUBST(THREADHEADERS)