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 00000000000..c228f503aab --- /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 2bf83db0e22..75965d338d5 100644 --- a/Modules/_posixsubprocess.c +++ b/Modules/_posixsubprocess.c @@ -160,16 +160,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) @@ -180,8 +181,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; @@ -189,15 +200,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; @@ -233,7 +266,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 @@ -243,19 +276,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) @@ -295,7 +327,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 @@ -310,14 +342,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)]; @@ -336,7 +370,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); } } @@ -357,7 +392,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 @@ -370,11 +405,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 @@ -393,7 +430,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 @@ -407,14 +445,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); } @@ -442,16 +482,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 @@ -544,7 +584,7 @@ child_exec(char *const exec_array[], Py_ssize_t extra_group_size, const gid_t *extra_groups, 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) { @@ -554,7 +594,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. */ @@ -676,7 +716,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 */ @@ -750,7 +790,7 @@ do_fork_exec(char *const exec_array[], Py_ssize_t extra_group_size, const gid_t *extra_groups, 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) { @@ -801,7 +841,8 @@ do_fork_exec(char *const exec_array[], close_fds, restore_signals, call_setsid, pgid_to_set, gid, extra_group_size, extra_groups, 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. */ } @@ -881,6 +922,8 @@ subprocess_fork_exec_impl(PyObject *module, PyObject *process_args, Py_ssize_t extra_group_size = 0; int need_after_fork = 0; int saved_errno = 0; + int *c_fds_to_keep = NULL; + Py_ssize_t fds_to_keep_len = PyTuple_GET_SIZE(py_fds_to_keep); PyInterpreterState *interp = PyInterpreterState_Get(); if ((preexec_fn != Py_None) && (interp != PyInterpreterState_Main())) { @@ -1031,6 +1074,15 @@ subprocess_fork_exec_impl(PyObject *module, PyObject *process_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(). */ @@ -1073,7 +1125,8 @@ subprocess_fork_exec_impl(PyObject *module, PyObject *process_args, close_fds, restore_signals, call_setsid, pgid_to_set, gid, extra_group_size, extra_groups, 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 == (pid_t)-1) { @@ -1103,6 +1156,10 @@ subprocess_fork_exec_impl(PyObject *module, PyObject *process_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