Skip to content

Commit c9c83ba

Browse files
pitroupitrou
committed
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.
1 parent 63ebe1c commit c9c83ba

4 files changed

Lines changed: 104 additions & 33 deletions

File tree

‎Lib/subprocess.py‎

Lines changed: 27 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -393,22 +393,22 @@ class pywintypes:
393393
# POSIX defines PIPE_BUF as >= 512.
394394
_PIPE_BUF = getattr(select, 'PIPE_BUF', 512)
395395

396+
_FD_CLOEXEC = getattr(fcntl, 'FD_CLOEXEC', 1)
397+
398+
def _set_cloexec(fd, cloexec):
399+
old = fcntl.fcntl(fd, fcntl.F_GETFD)
400+
if cloexec:
401+
fcntl.fcntl(fd, fcntl.F_SETFD, old | _FD_CLOEXEC)
402+
else:
403+
fcntl.fcntl(fd, fcntl.F_SETFD, old & ~_FD_CLOEXEC)
404+
396405
if _posixsubprocess:
397406
_create_pipe = _posixsubprocess.cloexec_pipe
398407
else:
399408
def _create_pipe():
400-
try:
401-
cloexec_flag = fcntl.FD_CLOEXEC
402-
except AttributeError:
403-
cloexec_flag = 1
404-
405409
fds = os.pipe()
406-
407-
old = fcntl.fcntl(fds[0], fcntl.F_GETFD)
408-
fcntl.fcntl(fds[0], fcntl.F_SETFD, old | cloexec_flag)
409-
old = fcntl.fcntl(fds[1], fcntl.F_GETFD)
410-
fcntl.fcntl(fds[1], fcntl.F_SETFD, old | cloexec_flag)
411-
410+
_set_cloexec(fds[0], True)
411+
_set_cloexec(fds[1], True)
412412
return fds
413413

414414
__all__ = ["Popen", "PIPE", "STDOUT", "call", "check_call", "getstatusoutput",
@@ -1194,23 +1194,25 @@ def _execute_child(self, args, executable, preexec_fn, close_fds,
11941194
os.close(errpipe_read)
11951195

11961196
# Dup fds for child
1197-
if p2cread != -1:
1198-
os.dup2(p2cread, 0)
1199-
if c2pwrite != -1:
1200-
os.dup2(c2pwrite, 1)
1201-
if errwrite != -1:
1202-
os.dup2(errwrite, 2)
1197+
def _dup2(a, b):
1198+
# dup2() removes the CLOEXEC flag but
1199+
# we must do it ourselves if dup2()
1200+
# would be a no-op (issue #10806).
1201+
if a == b:
1202+
_set_cloexec(a, False)
1203+
elif a != -1:
1204+
os.dup2(a, b)
1205+
_dup2(p2cread, 0)
1206+
_dup2(c2pwrite, 1)
1207+
_dup2(errwrite, 2)
12031208

12041209
# Close pipe fds. Make sure we don't close the
12051210
# same fd more than once, or standard fds.
1206-
if p2cread != -1 and p2cread not in (0,):
1207-
os.close(p2cread)
1208-
if (c2pwrite != -1 and
1209-
c2pwrite not in (p2cread, 1)):
1210-
os.close(c2pwrite)
1211-
if (errwrite != -1 and
1212-
errwrite not in (p2cread, c2pwrite, 2)):
1213-
os.close(errwrite)
1211+
closed = set()
1212+
for fd in [p2cread, c2pwrite, errwrite]:
1213+
if fd > 2 and fd not in closed:
1214+
os.close(fd)
1215+
closed.add(fd)
12141216

12151217
# Close all other fds, if asked for
12161218
if close_fds:

‎Lib/test/test_subprocess.py‎

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -903,6 +903,58 @@ def test_terminate(self):
903903
self.assertStderrEqual(stderr, b'')
904904
self.assertEqual(p.wait(), -signal.SIGTERM)
905905

906+
def check_close_std_fds(self, fds):
907+
# Issue #9905: test that subprocess pipes still work properly with
908+
# some standard fds closed
909+
stdin = 0
910+
newfds = []
911+
for a in fds:
912+
b = os.dup(a)
913+
newfds.append(b)
914+
if a == 0:
915+
stdin = b
916+
try:
917+
for fd in fds:
918+
os.close(fd)
919+
out, err = subprocess.Popen([sys.executable, "-c",
920+
'import sys;'
921+
'sys.stdout.write("apple");'
922+
'sys.stdout.flush();'
923+
'sys.stderr.write("orange")'],
924+
stdin=stdin,
925+
stdout=subprocess.PIPE,
926+
stderr=subprocess.PIPE).communicate()
927+
err = support.strip_python_stderr(err)
928+
self.assertEqual((out, err), (b'apple', b'orange'))
929+
finally:
930+
for b, a in zip(newfds, fds):
931+
os.dup2(b, a)
932+
for b in newfds:
933+
os.close(b)
934+
935+
def test_close_fd_0(self):
936+
self.check_close_std_fds([0])
937+
938+
def test_close_fd_1(self):
939+
self.check_close_std_fds([1])
940+
941+
def test_close_fd_2(self):
942+
self.check_close_std_fds([2])
943+
944+
def test_close_fds_0_1(self):
945+
self.check_close_std_fds([0, 1])
946+
947+
def test_close_fds_0_2(self):
948+
self.check_close_std_fds([0, 2])
949+
950+
def test_close_fds_1_2(self):
951+
self.check_close_std_fds([1, 2])
952+
953+
def test_close_fds_0_1_2(self):
954+
# Issue #10806: test that subprocess pipes still work properly with
955+
# all standard fds closed.
956+
self.check_close_std_fds([0, 1, 2])
957+
906958
def test_surrogates_error_message(self):
907959
def prepare():
908960
raise ValueError("surrogate:\uDCff")

‎Misc/NEWS‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,10 @@ Core and Builtins
2323
Library
2424
-------
2525

26+
- Issue #10806, issue #9905: Fix subprocess pipes when some of the standard
27+
file descriptors (0, 1, 2) are closed in the parent process. Initial
28+
patch by Ross Lagerwall.
29+
2630
- `unittest.TestCase` can be instantiated without a method name; for simpler
2731
exploration from the interactive interpreter.
2832

‎Modules/_posixsubprocess.c‎

Lines changed: 21 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -69,27 +69,40 @@ static void child_exec(char *const exec_array[],
6969
}
7070
POSIX_CALL(close(errpipe_read));
7171

72-
/* Dup fds for child. */
73-
if (p2cread != -1) {
72+
/* Dup fds for child.
73+
dup2() removes the CLOEXEC flag but we must do it ourselves if dup2()
74+
would be a no-op (issue #10806). */
75+
if (p2cread == 0) {
76+
int old = fcntl(p2cread, F_GETFD);
77+
if (old != -1)
78+
fcntl(p2cread, F_SETFD, old & ~FD_CLOEXEC);
79+
} else if (p2cread != -1) {
7480
POSIX_CALL(dup2(p2cread, 0)); /* stdin */
7581
}
76-
if (c2pwrite != -1) {
82+
if (c2pwrite == 1) {
83+
int old = fcntl(c2pwrite, F_GETFD);
84+
if (old != -1)
85+
fcntl(c2pwrite, F_SETFD, old & ~FD_CLOEXEC);
86+
} else if (c2pwrite != -1) {
7787
POSIX_CALL(dup2(c2pwrite, 1)); /* stdout */
7888
}
79-
if (errwrite != -1) {
89+
if (errwrite == 2) {
90+
int old = fcntl(errwrite, F_GETFD);
91+
if (old != -1)
92+
fcntl(errwrite, F_SETFD, old & ~FD_CLOEXEC);
93+
} else if (errwrite != -1) {
8094
POSIX_CALL(dup2(errwrite, 2)); /* stderr */
8195
}
8296

8397
/* Close pipe fds. Make sure we don't close the same fd more than */
8498
/* once, or standard fds. */
85-
if (p2cread != -1 && p2cread != 0) {
99+
if (p2cread > 2) {
86100
POSIX_CALL(close(p2cread));
87101
}
88-
if (c2pwrite != -1 && c2pwrite != p2cread && c2pwrite != 1) {
102+
if (c2pwrite > 2) {
89103
POSIX_CALL(close(c2pwrite));
90104
}
91-
if (errwrite != -1 && errwrite != p2cread &&
92-
errwrite != c2pwrite && errwrite != 2) {
105+
if (errwrite != c2pwrite && errwrite > 2) {
93106
POSIX_CALL(close(errwrite));
94107
}
95108

0 commit comments

Comments
 (0)