From de4745541105bd4866feb9169595997b4fe832ae Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith" Date: Wed, 17 May 2023 08:59:45 -0700 Subject: [PATCH 1/6] gh-104372: Cleanup _posixsubprocess `make_inheritable` for async signal safety and no GIL requirement (#104518) Move all of the Python C API calls into the parent process up front instead of doing PyLong_AsLong and PyErr_Occurred and PyTuple_GET from the post-fork/vfork child process. Much of this was long overdue. We shouldn't have been using PyTuple and PyLong APIs within all of these low level functions anyways. (cherry picked from commit c649df63e0d052044a4660101d5769ff46ae9234) --- ...-05-17-08-01-36.gh-issue-104372.jpoWs6.rst | 1 + Modules/_posixsubprocess.c | 125 +++++++++++++----- 2 files changed, 92 insertions(+), 34 deletions(-) create mode 100644 Misc/NEWS.d/next/Library/2023-05-17-08-01-36.gh-issue-104372.jpoWs6.rst diff --git a/Misc/NEWS.d/next/Library/2023-05-17-08-01-36.gh-issue-104372.jpoWs6.rst b/Misc/NEWS.d/next/Library/2023-05-17-08-01-36.gh-issue-104372.jpoWs6.rst new file mode 100644 index 00000000000000..c228f503aab4bd --- /dev/null +++ b/Misc/NEWS.d/next/Library/2023-05-17-08-01-36.gh-issue-104372.jpoWs6.rst @@ -0,0 +1 @@ +Refactored the ``_posixsubprocess`` internals to avoid Python C API usage between fork and exec when marking ``pass_fds=`` file descriptors inheritable. diff --git a/Modules/_posixsubprocess.c b/Modules/_posixsubprocess.c index 9132f13e8166a7..61c9bec8a60c54 100644 --- a/Modules/_posixsubprocess.c +++ b/Modules/_posixsubprocess.c @@ -138,16 +138,17 @@ _sanity_check_python_fd_sequence(PyObject *fd_sequence) /* Is fd found in the sorted Python Sequence? */ static int -_is_fd_in_sorted_fd_sequence(int fd, PyObject *fd_sequence) +_is_fd_in_sorted_fd_sequence(int fd, int *fd_sequence, + Py_ssize_t fd_sequence_len) { /* Binary search. */ Py_ssize_t search_min = 0; - Py_ssize_t search_max = PyTuple_GET_SIZE(fd_sequence) - 1; + Py_ssize_t search_max = fd_sequence_len - 1; if (search_max < 0) return 0; do { long middle = (search_min + search_max) / 2; - long middle_fd = PyLong_AsLong(PyTuple_GET_ITEM(fd_sequence, middle)); + long middle_fd = fd_sequence[middle]; if (fd == middle_fd) return 1; if (fd > middle_fd) @@ -158,8 +159,18 @@ _is_fd_in_sorted_fd_sequence(int fd, PyObject *fd_sequence) return 0; } +/* + * Do all the Python C API calls in the parent process to turn the pass_fds + * "py_fds_to_keep" tuple into a C array. The caller owns allocation and + * freeing of the array. + * + * On error an unknown number of array elements may have been filled in. + * A Python exception has been set when an error is returned. + * + * Returns: -1 on error, 0 on success. + */ static int -make_inheritable(PyObject *py_fds_to_keep, int errpipe_write) +convert_fds_to_keep_to_c(PyObject *py_fds_to_keep, int *c_fds_to_keep) { Py_ssize_t i, len; @@ -167,15 +178,37 @@ make_inheritable(PyObject *py_fds_to_keep, int errpipe_write) for (i = 0; i < len; ++i) { PyObject* fdobj = PyTuple_GET_ITEM(py_fds_to_keep, i); long fd = PyLong_AsLong(fdobj); - assert(!PyErr_Occurred()); - assert(0 <= fd && fd <= INT_MAX); + if (PyErr_Occurred()) { + return -1; + } + if (fd < 0 || fd > INT_MAX) { + PyErr_SetString(PyExc_ValueError, + "fd out of range in fds_to_keep."); + return -1; + } + c_fds_to_keep[i] = (int)fd; + } + return 0; +} + + +/* This function must be async-signal-safe as it is called from child_exec() + * after fork() or vfork(). + */ +static int +make_inheritable(int *c_fds_to_keep, Py_ssize_t len, int errpipe_write) +{ + Py_ssize_t i; + + for (i = 0; i < len; ++i) { + int fd = c_fds_to_keep[i]; if (fd == errpipe_write) { - /* errpipe_write is part of py_fds_to_keep. It must be closed at + /* errpipe_write is part of fds_to_keep. It must be closed at exec(), but kept open in the child process until exec() is called. */ continue; } - if (_Py_set_inheritable_async_safe((int)fd, 1, NULL) < 0) + if (_Py_set_inheritable_async_safe(fd, 1, NULL) < 0) return -1; } return 0; @@ -211,7 +244,7 @@ safe_get_max_fd(void) /* Close all file descriptors in the given range except for those in - * py_fds_to_keep by invoking closer on each subrange. + * fds_to_keep by invoking closer on each subrange. * * If end_fd == -1, it's guessed via safe_get_max_fd(), but it isn't * possible to know for sure what the max fd to go up to is for @@ -221,19 +254,18 @@ safe_get_max_fd(void) static int _close_range_except(int start_fd, int end_fd, - PyObject *py_fds_to_keep, + int *fds_to_keep, + Py_ssize_t fds_to_keep_len, int (*closer)(int, int)) { if (end_fd == -1) { end_fd = Py_MIN(safe_get_max_fd(), INT_MAX); } - Py_ssize_t num_fds_to_keep = PyTuple_GET_SIZE(py_fds_to_keep); Py_ssize_t keep_seq_idx; - /* As py_fds_to_keep is sorted we can loop through the list closing + /* As fds_to_keep is sorted we can loop through the list closing * fds in between any in the keep list falling within our range. */ - for (keep_seq_idx = 0; keep_seq_idx < num_fds_to_keep; ++keep_seq_idx) { - PyObject* py_keep_fd = PyTuple_GET_ITEM(py_fds_to_keep, keep_seq_idx); - int keep_fd = PyLong_AsLong(py_keep_fd); + for (keep_seq_idx = 0; keep_seq_idx < fds_to_keep_len; ++keep_seq_idx) { + int keep_fd = fds_to_keep[keep_seq_idx]; if (keep_fd < start_fd) continue; if (closer(start_fd, keep_fd - 1) != 0) @@ -273,7 +305,7 @@ _brute_force_closer(int first, int last) } /* Close all open file descriptors in the range from start_fd and higher - * Do not close any in the sorted py_fds_to_keep list. + * Do not close any in the sorted fds_to_keep list. * * This version is async signal safe as it does not make any unsafe C library * calls, malloc calls or handle any locks. It is _unfortunate_ to be forced @@ -288,14 +320,16 @@ _brute_force_closer(int first, int last) * it with some cpp #define magic to work on other OSes as well if you want. */ static void -_close_open_fds_safe(int start_fd, PyObject* py_fds_to_keep) +_close_open_fds_safe(int start_fd, int *fds_to_keep, Py_ssize_t fds_to_keep_len) { int fd_dir_fd; fd_dir_fd = _Py_open_noraise(FD_DIR, O_RDONLY); if (fd_dir_fd == -1) { /* No way to get a list of open fds. */ - _close_range_except(start_fd, -1, py_fds_to_keep, _brute_force_closer); + _close_range_except(start_fd, -1, + fds_to_keep, fds_to_keep_len, + _brute_force_closer); return; } else { char buffer[sizeof(struct linux_dirent64)]; @@ -314,7 +348,8 @@ _close_open_fds_safe(int start_fd, PyObject* py_fds_to_keep) if ((fd = _pos_int_from_ascii(entry->d_name)) < 0) continue; /* Not a number. */ if (fd != fd_dir_fd && fd >= start_fd && - !_is_fd_in_sorted_fd_sequence(fd, py_fds_to_keep)) { + !_is_fd_in_sorted_fd_sequence(fd, fds_to_keep, + fds_to_keep_len)) { close(fd); } } @@ -335,7 +370,7 @@ _unsafe_closer(int first, int last) } /* Close all open file descriptors from start_fd and higher. - * Do not close any in the sorted py_fds_to_keep tuple. + * Do not close any in the sorted fds_to_keep tuple. * * This function violates the strict use of async signal safe functions. :( * It calls opendir(), readdir() and closedir(). Of these, the one most @@ -348,11 +383,13 @@ _unsafe_closer(int first, int last) * http://womble.decadent.org.uk/readdir_r-advisory.html */ static void -_close_open_fds_maybe_unsafe(int start_fd, PyObject* py_fds_to_keep) +_close_open_fds_maybe_unsafe(int start_fd, int *fds_to_keep, + Py_ssize_t fds_to_keep_len) { DIR *proc_fd_dir; #ifndef HAVE_DIRFD - while (_is_fd_in_sorted_fd_sequence(start_fd, py_fds_to_keep)) { + while (_is_fd_in_sorted_fd_sequence(start_fd, fds_to_keep, + fds_to_keep_len)) { ++start_fd; } /* Close our lowest fd before we call opendir so that it is likely to @@ -371,7 +408,8 @@ _close_open_fds_maybe_unsafe(int start_fd, PyObject* py_fds_to_keep) proc_fd_dir = opendir(FD_DIR); if (!proc_fd_dir) { /* No way to get a list of open fds. */ - _close_range_except(start_fd, -1, py_fds_to_keep, _unsafe_closer); + _close_range_except(start_fd, -1, fds_to_keep, fds_to_keep_len, + _unsafe_closer); } else { struct dirent *dir_entry; #ifdef HAVE_DIRFD @@ -385,14 +423,16 @@ _close_open_fds_maybe_unsafe(int start_fd, PyObject* py_fds_to_keep) if ((fd = _pos_int_from_ascii(dir_entry->d_name)) < 0) continue; /* Not a number. */ if (fd != fd_used_by_opendir && fd >= start_fd && - !_is_fd_in_sorted_fd_sequence(fd, py_fds_to_keep)) { + !_is_fd_in_sorted_fd_sequence(fd, fds_to_keep, + fds_to_keep_len)) { close(fd); } errno = 0; } if (errno) { /* readdir error, revert behavior. Highly Unlikely. */ - _close_range_except(start_fd, -1, py_fds_to_keep, _unsafe_closer); + _close_range_except(start_fd, -1, fds_to_keep, fds_to_keep_len, + _unsafe_closer); } closedir(proc_fd_dir); } @@ -420,16 +460,16 @@ _close_range_closer(int first, int last) #endif static void -_close_open_fds(int start_fd, PyObject* py_fds_to_keep) +_close_open_fds(int start_fd, int *fds_to_keep, Py_ssize_t fds_to_keep_len) { #ifdef HAVE_ASYNC_SAFE_CLOSE_RANGE if (_close_range_except( - start_fd, INT_MAX, py_fds_to_keep, + start_fd, INT_MAX, fds_to_keep, fds_to_keep_len, _close_range_closer) == 0) { return; } #endif - _close_open_fds_fallback(start_fd, py_fds_to_keep); + _close_open_fds_fallback(start_fd, fds_to_keep, fds_to_keep_len); } #ifdef VFORK_USABLE @@ -522,7 +562,7 @@ child_exec(char *const exec_array[], int call_setgroups, size_t groups_size, const gid_t *groups, int call_setuid, uid_t uid, int child_umask, const void *child_sigmask, - PyObject *py_fds_to_keep, + int *fds_to_keep, Py_ssize_t fds_to_keep_len, PyObject *preexec_fn, PyObject *preexec_fn_args_tuple) { @@ -532,7 +572,7 @@ child_exec(char *const exec_array[], /* Buffer large enough to hold a hex integer. We can't malloc. */ char hex_errno[sizeof(saved_errno)*2+1]; - if (make_inheritable(py_fds_to_keep, errpipe_write) < 0) + if (make_inheritable(fds_to_keep, fds_to_keep_len, errpipe_write) < 0) goto error; /* Close parent's pipe ends. */ @@ -652,7 +692,7 @@ child_exec(char *const exec_array[], /* close FDs after executing preexec_fn, which might open FDs */ if (close_fds) { /* TODO HP-UX could use pstat_getproc() if anyone cares about it. */ - _close_open_fds(3, py_fds_to_keep); + _close_open_fds(3, fds_to_keep, fds_to_keep_len); } /* This loop matches the Lib/os.py _execvpe()'s PATH search when */ @@ -726,7 +766,7 @@ do_fork_exec(char *const exec_array[], int call_setgroups, size_t groups_size, const gid_t *groups, int call_setuid, uid_t uid, int child_umask, const void *child_sigmask, - PyObject *py_fds_to_keep, + int *fds_to_keep, Py_ssize_t fds_to_keep_len, PyObject *preexec_fn, PyObject *preexec_fn_args_tuple) { @@ -777,7 +817,8 @@ do_fork_exec(char *const exec_array[], close_fds, restore_signals, call_setsid, pgid_to_set, call_setgid, gid, call_setgroups, groups_size, groups, call_setuid, uid, child_umask, child_sigmask, - py_fds_to_keep, preexec_fn, preexec_fn_args_tuple); + fds_to_keep, fds_to_keep_len, + preexec_fn, preexec_fn_args_tuple); _exit(255); return 0; /* Dead code to avoid a potential compiler warning. */ } @@ -810,6 +851,8 @@ subprocess_fork_exec(PyObject *module, PyObject *args) int need_after_fork = 0; int saved_errno = 0; int allow_vfork; + int *c_fds_to_keep = NULL; + Py_ssize_t fds_to_keep_len = PyTuple_GET_SIZE(py_fds_to_keep); if (!PyArg_ParseTuple( args, "OOpO!OOiiiiiiiiii" _Py_PARSE_PID "OOOiOp:fork_exec", @@ -983,6 +1026,15 @@ subprocess_fork_exec(PyObject *module, PyObject *args) #endif /* HAVE_SETREUID */ } + c_fds_to_keep = PyMem_RawMalloc(fds_to_keep_len * sizeof(int)); + if (c_fds_to_keep == NULL) { + PyErr_SetString(PyExc_MemoryError, "failed to malloc c_fds_to_keep"); + goto cleanup; + } + if (convert_fds_to_keep_to_c(py_fds_to_keep, c_fds_to_keep) < 0) { + goto cleanup; + } + /* This must be the last thing done before fork() because we do not * want to call PyOS_BeforeFork() if there is any chance of another * error leading to the cleanup: code without calling fork(). */ @@ -1025,7 +1077,8 @@ subprocess_fork_exec(PyObject *module, PyObject *args) close_fds, restore_signals, call_setsid, pgid_to_set, call_setgid, gid, call_setgroups, num_groups, groups, call_setuid, uid, child_umask, old_sigmask, - py_fds_to_keep, preexec_fn, preexec_fn_args_tuple); + c_fds_to_keep, fds_to_keep_len, + preexec_fn, preexec_fn_args_tuple); /* Parent (original) process */ if (pid == -1) { @@ -1055,6 +1108,10 @@ subprocess_fork_exec(PyObject *module, PyObject *args) PyOS_AfterFork_Parent(); cleanup: + if (c_fds_to_keep != NULL) { + PyMem_RawFree(c_fds_to_keep); + } + if (saved_errno != 0) { errno = saved_errno; /* We can't call this above as PyOS_AfterFork_Parent() calls back From 0991dae3be99ead6bcbed002558139c13fcbee66 Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith" Date: Mon, 22 May 2023 19:01:50 -0700 Subject: [PATCH 2/6] [3.11] gh-104372: Cleanup _posixsubprocess `make_inheritable` for async signal safety and no GIL requirement (GH-104518) Move all of the Python C API calls into the parent process up front instead of doing PyLong_AsLong and PyErr_Occurred and PyTuple_GET from the post-fork/vfork child process. Much of this was long overdue. We shouldn't have been using PyTuple and PyLong APIs within all of these low level functions anyways.. (cherry picked from commit c649df63e0d052044a4660101d5769ff46ae9234) Co-authored-by: Gregory P. Smith From c6842903927385dc317544d0521b89d81724279b Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith" Date: Mon, 22 May 2023 19:08:11 -0700 Subject: [PATCH 3/6] Don't use Raw malloc & free. Backport of d1732feea0eadd4ccc3516440d0c071be0093dec to 3.11 from #104697. --- Modules/_posixsubprocess.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Modules/_posixsubprocess.c b/Modules/_posixsubprocess.c index 61c9bec8a60c54..fb2f28fdeaa4e2 100644 --- a/Modules/_posixsubprocess.c +++ b/Modules/_posixsubprocess.c @@ -1026,7 +1026,7 @@ subprocess_fork_exec(PyObject *module, PyObject *args) #endif /* HAVE_SETREUID */ } - c_fds_to_keep = PyMem_RawMalloc(fds_to_keep_len * sizeof(int)); + c_fds_to_keep = PyMem_Malloc(fds_to_keep_len * sizeof(int)); if (c_fds_to_keep == NULL) { PyErr_SetString(PyExc_MemoryError, "failed to malloc c_fds_to_keep"); goto cleanup; @@ -1109,7 +1109,7 @@ subprocess_fork_exec(PyObject *module, PyObject *args) cleanup: if (c_fds_to_keep != NULL) { - PyMem_RawFree(c_fds_to_keep); + PyMem_Free(c_fds_to_keep); } if (saved_errno != 0) { From 0ecbcba92a0e4efa7b075bf01b0bf898fc42aee0 Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith" Date: Mon, 22 May 2023 19:15:30 -0700 Subject: [PATCH 4/6] Fix the backport for pre-argument-clinic life. --- Modules/_posixsubprocess.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Modules/_posixsubprocess.c b/Modules/_posixsubprocess.c index fb2f28fdeaa4e2..19f711ff45ecd5 100644 --- a/Modules/_posixsubprocess.c +++ b/Modules/_posixsubprocess.c @@ -852,7 +852,7 @@ subprocess_fork_exec(PyObject *module, PyObject *args) int saved_errno = 0; int allow_vfork; int *c_fds_to_keep = NULL; - Py_ssize_t fds_to_keep_len = PyTuple_GET_SIZE(py_fds_to_keep); + Py_ssize_t fds_to_keep_len = 0; if (!PyArg_ParseTuple( args, "OOpO!OOiiiiiiiiii" _Py_PARSE_PID "OOOiOp:fork_exec", @@ -881,6 +881,7 @@ subprocess_fork_exec(PyObject *module, PyObject *args) PyErr_SetString(PyExc_ValueError, "bad value(s) in fds_to_keep"); return NULL; } + fds_to_keep_len = PyTuple_GET_SIZE(py_fds_to_keep); PyInterpreterState *interp = PyInterpreterState_Get(); const PyConfig *config = _PyInterpreterState_GetConfig(interp); From c058694ed9ca17201f52c682cbc88b08bf6fe36f Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith" Date: Tue, 23 May 2023 12:02:02 -0700 Subject: [PATCH 5/6] cleanup to keep the backport more diff concise. --- Modules/_posixsubprocess.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/Modules/_posixsubprocess.c b/Modules/_posixsubprocess.c index 19f711ff45ecd5..edf3ab843146fe 100644 --- a/Modules/_posixsubprocess.c +++ b/Modules/_posixsubprocess.c @@ -852,7 +852,6 @@ subprocess_fork_exec(PyObject *module, PyObject *args) int saved_errno = 0; int allow_vfork; int *c_fds_to_keep = NULL; - Py_ssize_t fds_to_keep_len = 0; if (!PyArg_ParseTuple( args, "OOpO!OOiiiiiiiiii" _Py_PARSE_PID "OOOiOp:fork_exec", @@ -881,7 +880,6 @@ subprocess_fork_exec(PyObject *module, PyObject *args) PyErr_SetString(PyExc_ValueError, "bad value(s) in fds_to_keep"); return NULL; } - fds_to_keep_len = PyTuple_GET_SIZE(py_fds_to_keep); PyInterpreterState *interp = PyInterpreterState_Get(); const PyConfig *config = _PyInterpreterState_GetConfig(interp); @@ -1027,6 +1025,7 @@ subprocess_fork_exec(PyObject *module, PyObject *args) #endif /* HAVE_SETREUID */ } + Py_ssize_t fds_to_keep_len = PyTuple_GET_SIZE(py_fds_to_keep); c_fds_to_keep = PyMem_Malloc(fds_to_keep_len * sizeof(int)); if (c_fds_to_keep == NULL) { PyErr_SetString(PyExc_MemoryError, "failed to malloc c_fds_to_keep"); From c364eaaff40810cf6440c876bcc495002dfcbefb Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith" Date: Tue, 23 May 2023 16:43:55 -0700 Subject: [PATCH 6/6] only check PyErr_Occurred if it returned -1 --- Modules/_posixsubprocess.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Modules/_posixsubprocess.c b/Modules/_posixsubprocess.c index edf3ab843146fe..ad9daaede430bc 100644 --- a/Modules/_posixsubprocess.c +++ b/Modules/_posixsubprocess.c @@ -178,7 +178,7 @@ convert_fds_to_keep_to_c(PyObject *py_fds_to_keep, int *c_fds_to_keep) for (i = 0; i < len; ++i) { PyObject* fdobj = PyTuple_GET_ITEM(py_fds_to_keep, i); long fd = PyLong_AsLong(fdobj); - if (PyErr_Occurred()) { + if (fd == -1 && PyErr_Occurred()) { return -1; } if (fd < 0 || fd > INT_MAX) {