Skip to content

Commit b90c685

Browse files
miss-islingtonizbyshev
authored andcommitted
bpo-32777: Fix _Py_set_inheritable async-safety in subprocess (GH-5560) (GH-5563)
Fix a rare but potential pre-exec child process deadlock in subprocess on POSIX systems when marking file descriptors inheritable on exec in the child process. This bug appears to have been introduced in 3.4 with the inheritable file descriptors support. This also changes Python/fileutils.c `set_inheritable` to use the "slow" two `fcntl` syscall path instead of the "fast" single `ioctl` syscall path when asked to be async signal safe (by way of being asked not to raise exceptions). `ioctl` is not a POSIX async-signal-safe approved function. ref: http://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html (cherry picked from commit c1e46e9) Co-authored-by: Alexey Izbyshev <izbyshev@users.noreply.github.com>
1 parent 7bd5a75 commit b90c685

File tree

4 files changed

+26
-7
lines changed

4 files changed

+26
-7
lines changed

Include/fileutils.h

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -121,6 +121,9 @@ PyAPI_FUNC(int) _Py_get_inheritable(int fd);
121121
PyAPI_FUNC(int) _Py_set_inheritable(int fd, int inheritable,
122122
int *atomic_flag_works);
123123

124+
PyAPI_FUNC(int) _Py_set_inheritable_async_safe(int fd, int inheritable,
125+
int *atomic_flag_works);
126+
124127
PyAPI_FUNC(int) _Py_dup(int fd);
125128

126129
#ifndef MS_WINDOWS
Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
Fix a rare but potential pre-exec child process deadlock in subprocess on
2+
POSIX systems when marking file descriptors inheritable on exec in the child
3+
process. This bug appears to have been introduced in 3.4.

Modules/_posixsubprocess.c

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -169,7 +169,7 @@ make_inheritable(PyObject *py_fds_to_keep, int errpipe_write)
169169
called. */
170170
continue;
171171
}
172-
if (_Py_set_inheritable((int)fd, 1, NULL) < 0)
172+
if (_Py_set_inheritable_async_safe((int)fd, 1, NULL) < 0)
173173
return -1;
174174
}
175175
return 0;
@@ -431,21 +431,21 @@ child_exec(char *const exec_array[],
431431
dup2() removes the CLOEXEC flag but we must do it ourselves if dup2()
432432
would be a no-op (issue #10806). */
433433
if (p2cread == 0) {
434-
if (_Py_set_inheritable(p2cread, 1, NULL) < 0)
434+
if (_Py_set_inheritable_async_safe(p2cread, 1, NULL) < 0)
435435
goto error;
436436
}
437437
else if (p2cread != -1)
438438
POSIX_CALL(dup2(p2cread, 0)); /* stdin */
439439

440440
if (c2pwrite == 1) {
441-
if (_Py_set_inheritable(c2pwrite, 1, NULL) < 0)
441+
if (_Py_set_inheritable_async_safe(c2pwrite, 1, NULL) < 0)
442442
goto error;
443443
}
444444
else if (c2pwrite != -1)
445445
POSIX_CALL(dup2(c2pwrite, 1)); /* stdout */
446446

447447
if (errwrite == 2) {
448-
if (_Py_set_inheritable(errwrite, 1, NULL) < 0)
448+
if (_Py_set_inheritable_async_safe(errwrite, 1, NULL) < 0)
449449
goto error;
450450
}
451451
else if (errwrite != -1)

Python/fileutils.c

Lines changed: 16 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -806,6 +806,7 @@ _Py_stat(PyObject *path, struct stat *statbuf)
806806
}
807807

808808

809+
/* This function MUST be kept async-signal-safe on POSIX when raise=0. */
809810
static int
810811
get_inheritable(int fd, int raise)
811812
{
@@ -851,6 +852,8 @@ _Py_get_inheritable(int fd)
851852
return get_inheritable(fd, 1);
852853
}
853854

855+
856+
/* This function MUST be kept async-signal-safe on POSIX when raise=0. */
854857
static int
855858
set_inheritable(int fd, int inheritable, int raise, int *atomic_flag_works)
856859
{
@@ -907,8 +910,10 @@ set_inheritable(int fd, int inheritable, int raise, int *atomic_flag_works)
907910
#else
908911

909912
#if defined(HAVE_SYS_IOCTL_H) && defined(FIOCLEX) && defined(FIONCLEX)
910-
if (ioctl_works != 0) {
913+
if (ioctl_works != 0 && raise != 0) {
911914
/* fast-path: ioctl() only requires one syscall */
915+
/* caveat: raise=0 is an indicator that we must be async-signal-safe
916+
* thus avoid using ioctl() so we skip the fast-path. */
912917
if (inheritable)
913918
request = FIONCLEX;
914919
else
@@ -979,8 +984,7 @@ make_non_inheritable(int fd)
979984
}
980985

981986
/* Set the inheritable flag of the specified file descriptor.
982-
On success: return 0, on error: raise an exception if raise is nonzero
983-
and return -1.
987+
On success: return 0, on error: raise an exception and return -1.
984988
985989
If atomic_flag_works is not NULL:
986990
@@ -1001,6 +1005,15 @@ _Py_set_inheritable(int fd, int inheritable, int *atomic_flag_works)
10011005
return set_inheritable(fd, inheritable, 1, atomic_flag_works);
10021006
}
10031007

1008+
/* Same as _Py_set_inheritable() but on error, set errno and
1009+
don't raise an exception.
1010+
This function is async-signal-safe. */
1011+
int
1012+
_Py_set_inheritable_async_safe(int fd, int inheritable, int *atomic_flag_works)
1013+
{
1014+
return set_inheritable(fd, inheritable, 0, atomic_flag_works);
1015+
}
1016+
10041017
static int
10051018
_Py_open_impl(const char *pathname, int flags, int gil_held)
10061019
{

0 commit comments

Comments
 (0)