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.

10 files changed, 195 insertions, 17 deletions

diff --git a/Lib/subprocess.py b/Lib/subprocess.py
index 62dfd36..729a53b 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 0000000..083b2f9
--- /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 0000000..ccae5f3
--- /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 0000000..fe6f9db
--- /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 0000000..6990637
--- /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 eaa26d2..74e7404 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 4c7c28b..4ce952f 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 36da2e5..0b5e544 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 <unistd.h>
+#include <fcntl.h>
 
 
 #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 2288cf3..4cebad2 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 0318133..fe030b3 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)