Skip to content

Commit 51ee270

Browse files
gpsheadgpshead
committed
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.
1 parent f560485 commit 51ee270

File tree

10 files changed

+195
-17
lines changed

10 files changed

+195
-17
lines changed

‎Lib/subprocess.py‎

Lines changed: 21 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -390,6 +390,23 @@ class pywintypes:
390390
# POSIX defines PIPE_BUF as >= 512.
391391
_PIPE_BUF = getattr(select, 'PIPE_BUF', 512)
392392

393+
if _posixsubprocess:
394+
_create_pipe = _posixsubprocess.cloexec_pipe
395+
else:
396+
def _create_pipe():
397+
try:
398+
cloexec_flag = fcntl.FD_CLOEXEC
399+
except AttributeError:
400+
cloexec_flag = 1
401+
402+
fds = os.pipe()
403+
404+
old = fcntl.fcntl(fds[0], fcntl.F_GETFD)
405+
fcntl.fcntl(fds[0], fcntl.F_SETFD, old | cloexec_flag)
406+
old = fcntl.fcntl(fds[1], fcntl.F_GETFD)
407+
fcntl.fcntl(fds[1], fcntl.F_SETFD, old | cloexec_flag)
408+
409+
return fds
393410

394411
__all__ = ["Popen", "PIPE", "STDOUT", "call", "check_call", "getstatusoutput",
395412
"getoutput", "check_output", "CalledProcessError"]
@@ -1031,7 +1048,7 @@ def _get_handles(self, stdin, stdout, stderr):
10311048
if stdin is None:
10321049
pass
10331050
elif stdin == PIPE:
1034-
p2cread, p2cwrite = os.pipe()
1051+
p2cread, p2cwrite = _create_pipe()
10351052
elif isinstance(stdin, int):
10361053
p2cread = stdin
10371054
else:
@@ -1041,7 +1058,7 @@ def _get_handles(self, stdin, stdout, stderr):
10411058
if stdout is None:
10421059
pass
10431060
elif stdout == PIPE:
1044-
c2pread, c2pwrite = os.pipe()
1061+
c2pread, c2pwrite = _create_pipe()
10451062
elif isinstance(stdout, int):
10461063
c2pwrite = stdout
10471064
else:
@@ -1051,7 +1068,7 @@ def _get_handles(self, stdin, stdout, stderr):
10511068
if stderr is None:
10521069
pass
10531070
elif stderr == PIPE:
1054-
errread, errwrite = os.pipe()
1071+
errread, errwrite = _create_pipe()
10551072
elif stderr == STDOUT:
10561073
errwrite = c2pwrite
10571074
elif isinstance(stderr, int):
@@ -1065,16 +1082,6 @@ def _get_handles(self, stdin, stdout, stderr):
10651082
errread, errwrite)
10661083

10671084

1068-
def _set_cloexec_flag(self, fd):
1069-
try:
1070-
cloexec_flag = fcntl.FD_CLOEXEC
1071-
except AttributeError:
1072-
cloexec_flag = 1
1073-
1074-
old = fcntl.fcntl(fd, fcntl.F_GETFD)
1075-
fcntl.fcntl(fd, fcntl.F_SETFD, old | cloexec_flag)
1076-
1077-
10781085
def _close_fds(self, but):
10791086
os.closerange(3, but)
10801087
os.closerange(but + 1, MAXFD)
@@ -1116,10 +1123,9 @@ def _execute_child(self, args, executable, preexec_fn, close_fds,
11161123
# For transferring possible exec failure from child to parent.
11171124
# Data format: "exception name:hex errno:description"
11181125
# Pickle is not used; it is complex and involves memory allocation.
1119-
errpipe_read, errpipe_write = os.pipe()
1126+
errpipe_read, errpipe_write = _create_pipe()
11201127
try:
11211128
try:
1122-
self._set_cloexec_flag(errpipe_write)
11231129

11241130
if _posixsubprocess:
11251131
# We must avoid complex work that could involve

‎Lib/test/subprocessdata/fd_status.py‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
"""When called as a script, print a comma-separated list of the open
2+
file descriptors on stdout."""
3+
4+
import errno
5+
import os
6+
import fcntl
7+
8+
try:
9+
_MAXFD = os.sysconf("SC_OPEN_MAX")
10+
except:
11+
_MAXFD = 256
12+
13+
def isopen(fd):
14+
"""Return True if the fd is open, and False otherwise"""
15+
try:
16+
fcntl.fcntl(fd, fcntl.F_GETFD, 0)
17+
except IOError as e:
18+
if e.errno == errno.EBADF:
19+
return False
20+
raise
21+
return True
22+
23+
if __name__ == "__main__":
24+
print(','.join(str(fd) for fd in range(0, _MAXFD) if isopen(fd)))

‎Lib/test/subprocessdata/input_reader.py‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
"""When called as a script, consumes the input"""
2+
3+
import sys
4+
5+
if __name__ = "__main__":
6+
for line in sys.stdin:
7+
pass

‎Lib/test/subprocessdata/qcat.py‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
"""When ran as a script, simulates cat with no arguments."""
2+
3+
import sys
4+
5+
if __name__ == "__main__":
6+
for line in sys.stdin:
7+
sys.stdout.write(line)

‎Lib/test/subprocessdata/qgrep.py‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
"""When called with a single argument, simulated fgrep with a single
2+
argument and no options."""
3+
4+
import sys
5+
6+
if __name__ == "__main__":
7+
pattern = sys.argv[1]
8+
for line in sys.stdin:
9+
if pattern in line:
10+
sys.stdout.write(line)

‎Lib/test/test_subprocess.py‎

Lines changed: 78 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
import re
1111
import sysconfig
1212
import warnings
13+
import select
1314
try:
1415
import gc
1516
except ImportError:
@@ -964,6 +965,83 @@ def test_bytes_program(self):
964965
exitcode = subprocess.call([program, "-c", "pass"], env=envb)
965966
self.assertEqual(exitcode, 0)
966967

968+
def test_pipe_cloexec(self):
969+
sleeper = support.findfile("input_reader.py", subdir="subprocessdata")
970+
fd_status = support.findfile("fd_status.py", subdir="subprocessdata")
971+
972+
p1 = subprocess.Popen([sys.executable, sleeper],
973+
stdin=subprocess.PIPE, stdout=subprocess.PIPE,
974+
stderr=subprocess.PIPE, close_fds=False)
975+
976+
self.addCleanup(p1.communicate, b'')
977+
978+
p2 = subprocess.Popen([sys.executable, fd_status],
979+
stdout=subprocess.PIPE, close_fds=False)
980+
981+
output, error = p2.communicate()
982+
result_fds = set(map(int, output.split(b',')))
983+
unwanted_fds = set([p1.stdin.fileno(), p1.stdout.fileno(),
984+
p1.stderr.fileno()])
985+
986+
self.assertFalse(result_fds & unwanted_fds,
987+
"Expected no fds from %r to be open in child, "
988+
"found %r" %
989+
(unwanted_fds, result_fds & unwanted_fds))
990+
991+
def test_pipe_cloexec_real_tools(self):
992+
qcat = support.findfile("qcat.py", subdir="subprocessdata")
993+
qgrep = support.findfile("qgrep.py", subdir="subprocessdata")
994+
995+
subdata = b'zxcvbn'
996+
data = subdata * 4 + b'\n'
997+
998+
p1 = subprocess.Popen([sys.executable, qcat],
999+
stdin=subprocess.PIPE, stdout=subprocess.PIPE,
1000+
close_fds=False)
1001+
1002+
p2 = subprocess.Popen([sys.executable, qgrep, subdata],
1003+
stdin=p1.stdout, stdout=subprocess.PIPE,
1004+
close_fds=False)
1005+
1006+
self.addCleanup(p1.wait)
1007+
self.addCleanup(p2.wait)
1008+
self.addCleanup(p1.terminate)
1009+
self.addCleanup(p2.terminate)
1010+
1011+
p1.stdin.write(data)
1012+
p1.stdin.close()
1013+
1014+
readfiles, ignored1, ignored2 = select.select([p2.stdout], [], [], 10)
1015+
1016+
self.assertTrue(readfiles, "The child hung")
1017+
self.assertEqual(p2.stdout.read(), data)
1018+
1019+
def test_close_fds(self):
1020+
fd_status = support.findfile("fd_status.py", subdir="subprocessdata")
1021+
1022+
fds = os.pipe()
1023+
self.addCleanup(os.close, fds[0])
1024+
self.addCleanup(os.close, fds[1])
1025+
1026+
open_fds = set(fds)
1027+
1028+
p = subprocess.Popen([sys.executable, fd_status],
1029+
stdout=subprocess.PIPE, close_fds=False)
1030+
output, ignored = p.communicate()
1031+
remaining_fds = set(map(int, output.split(b',')))
1032+
1033+
self.assertEqual(remaining_fds & open_fds, open_fds,
1034+
"Some fds were closed")
1035+
1036+
p = subprocess.Popen([sys.executable, fd_status],
1037+
stdout=subprocess.PIPE, close_fds=True)
1038+
output, ignored = p.communicate()
1039+
remaining_fds = set(map(int, output.split(b',')))
1040+
1041+
self.assertFalse(remaining_fds & open_fds,
1042+
"Some fds were left open")
1043+
self.assertIn(1, remaining_fds, "Subprocess failed")
1044+
9671045

9681046
@unittest.skipUnless(mswindows, "Windows specific tests")
9691047
class Win32ProcessTestCase(BaseTestCase):

‎Makefile.pre.in‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -888,7 +888,7 @@ MACHDEPS= $(PLATDIR) $(EXTRAPLATDIR)
888888
XMLLIBSUBDIRS= xml xml/dom xml/etree xml/parsers xml/sax
889889
LIBSUBDIRS= tkinter tkinter/test tkinter/test/test_tkinter \
890890
tkinter/test/test_ttk site-packages test \
891-
test/decimaltestdata test/xmltestdata \
891+
test/decimaltestdata test/xmltestdata test/subprocessdata \
892892
test/tracedmodules test/encoded_modules \
893893
concurrent concurrent/futures encodings \
894894
email email/mime email/test email/test/data \

‎Modules/_posixsubprocess.c‎

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,10 @@
11
/* Authors: Gregory P. Smith & Jeffrey Yasskin */
22
#include "Python.h"
3+
#ifdef HAVE_PIPE2
4+
#define _GNU_SOURCE
5+
#endif
36
#include <unistd.h>
7+
#include <fcntl.h>
48

59

610
#define POSIX_CALL(call) if ((call) == -1) goto error
@@ -398,6 +402,45 @@ Returns: the child process's PID.\n\
398402
Raises: Only on an error in the parent process.\n\
399403
");
400404

405+
PyDoc_STRVAR(subprocess_cloexec_pipe_doc,
406+
"cloexec_pipe() -> (read_end, write_end)\n\n\
407+
Create a pipe whose ends have the cloexec flag set.");
408+
409+
static PyObject *
410+
subprocess_cloexec_pipe(PyObject *self, PyObject *noargs)
411+
{
412+
int fds[2];
413+
int res;
414+
#ifdef HAVE_PIPE2
415+
Py_BEGIN_ALLOW_THREADS
416+
res = pipe2(fds, O_CLOEXEC);
417+
Py_END_ALLOW_THREADS
418+
#else
419+
/* We hold the GIL which offers some protection from other code calling
420+
* fork() before the CLOEXEC flags have been set but we can't guarantee
421+
* anything without pipe2(). */
422+
long oldflags;
423+
424+
res = pipe(fds);
425+
426+
if (res == 0) {
427+
oldflags = fcntl(fds[0], F_GETFD, 0);
428+
if (oldflags < 0) res = oldflags;
429+
}
430+
if (res == 0)
431+
res = fcntl(fds[0], F_SETFD, oldflags | FD_CLOEXEC);
432+
433+
if (res == 0) {
434+
oldflags = fcntl(fds[1], F_GETFD, 0);
435+
if (oldflags < 0) res = oldflags;
436+
}
437+
if (res == 0)
438+
res = fcntl(fds[1], F_SETFD, oldflags | FD_CLOEXEC);
439+
#endif
440+
if (res != 0)
441+
return PyErr_SetFromErrno(PyExc_OSError);
442+
return Py_BuildValue("(ii)", fds[0], fds[1]);
443+
}
401444

402445
/* module level code ********************************************************/
403446

@@ -407,6 +450,7 @@ PyDoc_STRVAR(module_doc,
407450

408451
static PyMethodDef module_methods[] = {
409452
{"fork_exec", subprocess_fork_exec, METH_VARARGS, subprocess_fork_exec_doc},
453+
{"cloexec_pipe", subprocess_cloexec_pipe, METH_NOARGS, subprocess_cloexec_pipe_doc},
410454
{NULL, NULL} /* sentinel */
411455
};
412456

‎Tools/msi/msi.py‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1035,6 +1035,8 @@ def add_files(db):
10351035
if dir=='xmltestdata':
10361036
lib.glob("*.xml")
10371037
lib.add_file("test.xml.out")
1038+
if dir=='subprocessdata':
1039+
lib.glob("*.py")
10381040
if dir=='output':
10391041
lib.glob("test_*")
10401042
if dir=='sndhdrdata':

‎configure.in‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4235,7 +4235,7 @@ case $ac_sys_system in
42354235
OSF*) AC_MSG_ERROR(OSF* systems are deprecated unless somebody volunteers. Check http://bugs.python.org/issue8606) ;;
42364236
esac
42374237

4238-
4238+
AC_CHECK_FUNC(pipe2, AC_DEFINE(HAVE_PIPE2, 1, [Define if the OS supports pipe2()]), )
42394239

42404240
AC_SUBST(THREADHEADERS)
42414241

0 commit comments

Comments
 (0)