[Python-checkins] bpo-32777: Fix _Py_set_inheritable async-safety in subprocess (GH-5560)

[Gregory P. Smith]

https://github.com/python/cpython/commit/c1e46e94de38a92f98736af9a42d89c3975a9919
commit: c1e46e94de38a92f98736af9a42d89c3975a9919
branch: master
author: Alexey Izbyshev <izbyshev at users.noreply.github.com>
committer: Gregory P. Smith <greg at krypto.org>
date: 2018-02-05T22:09:34-08:00
summary:

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

files:
A Misc/NEWS.d/next/Library/2018-02-05-21-28-28.bpo-32777.C-wIXF.rst
M Include/fileutils.h
M Modules/_posixsubprocess.c
M Python/fileutils.c

diff --git a/Include/fileutils.h b/Include/fileutils.h
index 21eefdef87a2..e4bf6d4db95d 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 000000000000..d5d7d7b27dc7
--- /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 ea7a3d6c18be..dc43ffc2e19d 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 d610639688ea..3cf8b7a8b69d 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)
 {