From da04324dc0b36aa6ee76095559688c2da5c05682 Mon Sep 17 00:00:00 2001 From: Alexey Izbyshev Date: Tue, 6 Feb 2018 04:22:27 +0300 Subject: [PATCH 1/4] bpo-32777: subprocess: Fix usage of _Py_set_inheritable() in child_exec() _Py_set_inheritable() raises a Python-level exception on error and thus is not async-signal-safe, but child_exec() must use only async-signal-safe functions because it's executed between fork() and exec(). Fix this by introducing a non-raising version of _Py_set_inheritable() and using it throughout child_exec(). --- Include/fileutils.h | 3 +++ Modules/_posixsubprocess.c | 8 ++++---- Python/fileutils.c | 12 ++++++++++-- 3 files changed, 17 insertions(+), 6 deletions(-) diff --git a/Include/fileutils.h b/Include/fileutils.h index 21eefdef87a2ce..4b05a1f6ca764f 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_noraise(int fd, int inheritable, + int *atomic_flag_works); + PyAPI_FUNC(int) _Py_dup(int fd); #ifndef MS_WINDOWS diff --git a/Modules/_posixsubprocess.c b/Modules/_posixsubprocess.c index ea7a3d6c18be52..cd49871eb95ae4 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_noraise((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_noraise(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_noraise(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_noraise(errwrite, 1, NULL) < 0) goto error; } else if (errwrite != -1) diff --git a/Python/fileutils.c b/Python/fileutils.c index d610639688ea7a..d793f56c2926d6 100644 --- a/Python/fileutils.c +++ b/Python/fileutils.c @@ -1086,8 +1086,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 +1107,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_noraise(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) { From 3afee5ece957876e4f98269e4c2e1c2f21392d63 Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith" Date: Mon, 5 Feb 2018 21:28:36 -0800 Subject: [PATCH 2/4] Add a NEWS entry describing the scope. --- .../next/Library/2018-02-05-21-28-28.bpo-32777.C-wIXF.rst | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 Misc/NEWS.d/next/Library/2018-02-05-21-28-28.bpo-32777.C-wIXF.rst 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. From c983e655ab1ef3c8f40b0392d6e676df9cb370eb Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith" Date: Mon, 5 Feb 2018 21:29:30 -0800 Subject: [PATCH 3/4] Note the async-signal-safety & don't use ioctl. ioctl() is not async-signal-safe, so avoid using that "fast"-path when we are asked not to raise an exception (currently the only indicator the API has). --- Python/fileutils.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/Python/fileutils.c b/Python/fileutils.c index d793f56c2926d6..bbb752b81c0174 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 From 94f16b93f2e4038ce3c8da7f8e4a8d4aae2fe71a Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith" Date: Mon, 5 Feb 2018 21:34:57 -0800 Subject: [PATCH 4/4] Rename _Py_set_inheritable_noraise to _async_safe It better describes the actual purpose of the API. --- Include/fileutils.h | 4 ++-- Modules/_posixsubprocess.c | 8 ++++---- Python/fileutils.c | 2 +- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/Include/fileutils.h b/Include/fileutils.h index 4b05a1f6ca764f..e4bf6d4db95d88 100644 --- a/Include/fileutils.h +++ b/Include/fileutils.h @@ -152,8 +152,8 @@ 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_noraise(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); diff --git a/Modules/_posixsubprocess.c b/Modules/_posixsubprocess.c index cd49871eb95ae4..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_noraise((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_noraise(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_noraise(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_noraise(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 bbb752b81c0174..3cf8b7a8b69d73 100644 --- a/Python/fileutils.c +++ b/Python/fileutils.c @@ -1116,7 +1116,7 @@ _Py_set_inheritable(int fd, int inheritable, int *atomic_flag_works) don't raise an exception. This function is async-signal-safe. */ int -_Py_set_inheritable_noraise(int fd, int inheritable, int *atomic_flag_works) +_Py_set_inheritable_async_safe(int fd, int inheritable, int *atomic_flag_works) { return set_inheritable(fd, inheritable, 0, atomic_flag_works); }