From 2bb0bfafb0beb819b7c60d9004382ce6867847c0 Mon Sep 17 00:00:00 2001 From: "Miss Islington (bot)" <31488909+miss-islington@users.noreply.github.com> Date: Mon, 5 Feb 2018 22:31:22 -0800 Subject: [PATCH] bpo-32777: Fix _Py_set_inheritable async-safety in subprocess (GH-5560) (GH-5562) 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 c1e46e94de38a92f98736af9a42d89c3975a9919) Co-authored-by: Alexey Izbyshev --- Include/fileutils.h | 3 +++ .../2018-02-05-21-28-28.bpo-32777.C-wIXF.rst | 3 +++ Modules/_posixsubprocess.c | 8 ++++---- Python/fileutils.c | 19 ++++++++++++++++--- 4 files changed, 26 insertions(+), 7 deletions(-) create mode 100644 Misc/NEWS.d/next/Library/2018-02-05-21-28-28.bpo-32777.C-wIXF.rst diff --git a/Include/fileutils.h b/Include/fileutils.h index 21eefdef87a..e4bf6d4db95 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 00000000000..d5d7d7b27dc --- /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 ea7a3d6c18b..dc43ffc2e19 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 d610639688e..3cf8b7a8b69 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) {