bpo-32777: Fix _Py_set_inheritable async-safety in subprocess (GH-5560)

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

4 files changed, 26 insertions, 7 deletions

diff --git a/Include/fileutils.h b/Include/fileutils.h
index 21eefde..e4bf6d4 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 0000000..d5d7d7b
--- /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 ea7a3d6..dc43ffc 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 d610639..3cf8b7a 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)
 {