diff --git a/Include/fileutils.h b/Include/fileutils.h index 21eefdef87a2ce..e4bf6d4db95d88 100644 --- a/Include/fileutils.h +++ b/Include/fileutils.h @@ -152,6 +152,9 @@ PyAPI_FUNC(int) _Py_get_inheritable(int fd); PyAPI_FUNC(int) _Py_set_inheritable(int fd, int inheritable, int *atomic_flag_works); +PyAPI_FUNC(int) _Py_set_inheritable_async_safe(int fd, int inheritable, + int *atomic_flag_works); + PyAPI_FUNC(int) _Py_dup(int fd); #ifndef MS_WINDOWS diff --git a/Misc/NEWS.d/next/Library/2018-02-05-21-28-28.bpo-32777.C-wIXF.rst b/Misc/NEWS.d/next/Library/2018-02-05-21-28-28.bpo-32777.C-wIXF.rst new file mode 100644 index 00000000000000..d5d7d7b27dc775 --- /dev/null +++ b/Misc/NEWS.d/next/Library/2018-02-05-21-28-28.bpo-32777.C-wIXF.rst @@ -0,0 +1,3 @@ +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. diff --git a/Modules/_posixsubprocess.c b/Modules/_posixsubprocess.c index ea7a3d6c18be52..dc43ffc2e19dbc 100644 --- a/Modules/_posixsubprocess.c +++ b/Modules/_posixsubprocess.c @@ -169,7 +169,7 @@ make_inheritable(PyObject *py_fds_to_keep, int errpipe_write) called. */ continue; } - if (_Py_set_inheritable((int)fd, 1, NULL) < 0) + if (_Py_set_inheritable_async_safe((int)fd, 1, NULL) < 0) return -1; } return 0; @@ -431,21 +431,21 @@ child_exec(char *const exec_array[], dup2() removes the CLOEXEC flag but we must do it ourselves if dup2() would be a no-op (issue #10806). */ if (p2cread == 0) { - if (_Py_set_inheritable(p2cread, 1, NULL) < 0) + if (_Py_set_inheritable_async_safe(p2cread, 1, NULL) < 0) goto error; } else if (p2cread != -1) POSIX_CALL(dup2(p2cread, 0)); /* stdin */ if (c2pwrite == 1) { - if (_Py_set_inheritable(c2pwrite, 1, NULL) < 0) + if (_Py_set_inheritable_async_safe(c2pwrite, 1, NULL) < 0) goto error; } else if (c2pwrite != -1) POSIX_CALL(dup2(c2pwrite, 1)); /* stdout */ if (errwrite == 2) { - if (_Py_set_inheritable(errwrite, 1, NULL) < 0) + if (_Py_set_inheritable_async_safe(errwrite, 1, NULL) < 0) goto error; } else if (errwrite != -1) diff --git a/Python/fileutils.c b/Python/fileutils.c index d610639688ea7a..3cf8b7a8b69d73 100644 --- a/Python/fileutils.c +++ b/Python/fileutils.c @@ -913,6 +913,7 @@ _Py_stat(PyObject *path, struct stat *statbuf) } +/* This function MUST be kept async-signal-safe on POSIX when raise=0. */ static int get_inheritable(int fd, int raise) { @@ -958,6 +959,8 @@ _Py_get_inheritable(int fd) return get_inheritable(fd, 1); } + +/* This function MUST be kept async-signal-safe on POSIX when raise=0. */ static int set_inheritable(int fd, int inheritable, int raise, int *atomic_flag_works) { @@ -1014,8 +1017,10 @@ set_inheritable(int fd, int inheritable, int raise, int *atomic_flag_works) #else #if defined(HAVE_SYS_IOCTL_H) && defined(FIOCLEX) && defined(FIONCLEX) - if (ioctl_works != 0) { + if (ioctl_works != 0 && raise != 0) { /* fast-path: ioctl() only requires one syscall */ + /* caveat: raise=0 is an indicator that we must be async-signal-safe + * thus avoid using ioctl() so we skip the fast-path. */ if (inheritable) request = FIONCLEX; else @@ -1086,8 +1091,7 @@ make_non_inheritable(int fd) } /* Set the inheritable flag of the specified file descriptor. - On success: return 0, on error: raise an exception if raise is nonzero - and return -1. + On success: return 0, on error: raise an exception and return -1. If atomic_flag_works is not NULL: @@ -1108,6 +1112,15 @@ _Py_set_inheritable(int fd, int inheritable, int *atomic_flag_works) return set_inheritable(fd, inheritable, 1, atomic_flag_works); } +/* Same as _Py_set_inheritable() but on error, set errno and + don't raise an exception. + This function is async-signal-safe. */ +int +_Py_set_inheritable_async_safe(int fd, int inheritable, int *atomic_flag_works) +{ + return set_inheritable(fd, inheritable, 0, atomic_flag_works); +} + static int _Py_open_impl(const char *pathname, int flags, int gil_held) {