diff --git a/Include/internal/pycore_fileutils.h b/Include/internal/pycore_fileutils.h index 3ce8108e4e04f1..b1fe06ebbd7a01 100644 --- a/Include/internal/pycore_fileutils.h +++ b/Include/internal/pycore_fileutils.h @@ -216,6 +216,8 @@ PyAPI_FUNC(int) _Py_GetLocaleconvNumeric( PyObject **decimal_point, PyObject **thousands_sep); +PyAPI_FUNC(int) _Py_closerange_fast_async_safe(int first, int last); +PyAPI_FUNC(void) _Py_closerange_fallback(int first, int last); PyAPI_FUNC(void) _Py_closerange(int first, int last); PyAPI_FUNC(wchar_t*) _Py_GetLocaleEncoding(void); diff --git a/Misc/NEWS.d/next/Library/2022-05-04-11-54-37.gh-issue-92301.eqjoYX.rst b/Misc/NEWS.d/next/Library/2022-05-04-11-54-37.gh-issue-92301.eqjoYX.rst new file mode 100644 index 00000000000000..b0b0502bf0a75f --- /dev/null +++ b/Misc/NEWS.d/next/Library/2022-05-04-11-54-37.gh-issue-92301.eqjoYX.rst @@ -0,0 +1,2 @@ +Prefer ``close_range()`` to iterating over procfs for file descriptor +closing in :mod:`subprocess` for better performance. diff --git a/Modules/_posixsubprocess.c b/Modules/_posixsubprocess.c index 2440609e31bcc2..575646f72f0fe7 100644 --- a/Modules/_posixsubprocess.c +++ b/Modules/_posixsubprocess.c @@ -210,18 +210,23 @@ safe_get_max_fd(void) } -/* Close all file descriptors in the range from start_fd and higher - * except for those in py_fds_to_keep. If the range defined by - * [start_fd, safe_get_max_fd()) is large this will take a long - * time as it calls close() on EVERY possible fd. +/* Close all file descriptors in the given range except for those in + * py_fds_to_keep by invoking closer on each subrange. * - * It isn't possible to know for sure what the max fd to go up to - * is for processes with the capability of raising their maximum. + * 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 + * processes with the capability of raising their maximum, or in case + * a process opened a high fd and then lowered its maximum. */ -static void -_close_fds_by_brute_force(long start_fd, PyObject *py_fds_to_keep) +static int +_close_range_except(int start_fd, + int end_fd, + PyObject *py_fds_to_keep, + int (*closer)(int, int)) { - long end_fd = safe_get_max_fd(); + 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 @@ -231,12 +236,15 @@ _close_fds_by_brute_force(long start_fd, PyObject *py_fds_to_keep) int keep_fd = PyLong_AsLong(py_keep_fd); if (keep_fd < start_fd) continue; - _Py_closerange(start_fd, keep_fd - 1); + if (closer(start_fd, keep_fd - 1) != 0) + return -1; start_fd = keep_fd + 1; } if (start_fd <= end_fd) { - _Py_closerange(start_fd, end_fd); + if (closer(start_fd, end_fd) != 0) + return -1; } + return 0; } @@ -255,6 +263,16 @@ struct linux_dirent64 { char d_name[256]; /* Filename (null-terminated) */ }; +static int +_brute_force_closer(int first, int last) +{ + for (int i = first; i <= last; i++) { + /* Ignore errors */ + (void)close(i); + } + return 0; +} + /* 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. * @@ -278,7 +296,8 @@ _close_open_fds_safe(int start_fd, PyObject* py_fds_to_keep) 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_fds_by_brute_force(start_fd, py_fds_to_keep); + /* Can't use _Py_closerange_fallback since it can be unsafe. */ + _close_range_except(start_fd, -1, py_fds_to_keep, _brute_force_closer); return; } else { char buffer[sizeof(struct linux_dirent64)]; @@ -306,10 +325,16 @@ _close_open_fds_safe(int start_fd, PyObject* py_fds_to_keep) } } -#define _close_open_fds _close_open_fds_safe +#define _close_open_fds_fallback _close_open_fds_safe #else /* NOT (defined(__linux__) && defined(HAVE_SYS_SYSCALL_H)) */ +static int +_unsafe_closer(int first, int last) +{ + _Py_closerange_fallback(first, last); + return 0; +} /* Close all open file descriptors from start_fd and higher. * Do not close any in the sorted py_fds_to_keep tuple. @@ -325,7 +350,7 @@ _close_open_fds_safe(int start_fd, PyObject* py_fds_to_keep) * http://womble.decadent.org.uk/readdir_r-advisory.html */ static void -_close_open_fds_maybe_unsafe(long start_fd, PyObject* py_fds_to_keep) +_close_open_fds_maybe_unsafe(int start_fd, PyObject* py_fds_to_keep) { DIR *proc_fd_dir; #ifndef HAVE_DIRFD @@ -348,7 +373,7 @@ _close_open_fds_maybe_unsafe(long 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_fds_by_brute_force(start_fd, py_fds_to_keep); + _close_range_except(start_fd, -1, py_fds_to_keep, _unsafe_closer); } else { struct dirent *dir_entry; #ifdef HAVE_DIRFD @@ -369,16 +394,26 @@ _close_open_fds_maybe_unsafe(long start_fd, PyObject* py_fds_to_keep) } if (errno) { /* readdir error, revert behavior. Highly Unlikely. */ - _close_fds_by_brute_force(start_fd, py_fds_to_keep); + _close_range_except(start_fd, -1, py_fds_to_keep, _unsafe_closer); } closedir(proc_fd_dir); } } -#define _close_open_fds _close_open_fds_maybe_unsafe +#define _close_open_fds_fallback _close_open_fds_maybe_unsafe #endif /* else NOT (defined(__linux__) && defined(HAVE_SYS_SYSCALL_H)) */ +static void +_close_open_fds(int start_fd, PyObject* py_fds_to_keep) +{ + if (_close_range_except( + start_fd, INT_MAX, py_fds_to_keep, + _Py_closerange_fast_async_safe) == 0) { + return; + } + _close_open_fds_fallback(start_fd, py_fds_to_keep); +} #ifdef VFORK_USABLE /* Reset dispositions for all signals to SIG_DFL except for ignored diff --git a/Python/fileutils.c b/Python/fileutils.c index 4f7f8944a72da5..f07fe04f78e9c6 100644 --- a/Python/fileutils.c +++ b/Python/fileutils.c @@ -2617,27 +2617,30 @@ _fdwalk_close_func(void *lohi, int fd) } #endif /* USE_FDWALK */ -/* Closes all file descriptors in [first, last], ignoring errors. */ -void -_Py_closerange(int first, int last) +int +_Py_closerange_fast_async_safe(int first, int last) { - first = Py_MAX(first, 0); - _Py_BEGIN_SUPPRESS_IPH #ifdef HAVE_CLOSE_RANGE if (close_range(first, last, 0) == 0) { /* close_range() ignores errors when it closes file descriptors. * Possible reasons of an error return are lack of kernel support * or denial of the underlying syscall by a seccomp sandbox on Linux. * Fallback to other methods in case of any error. */ + return 0; } - else #endif /* HAVE_CLOSE_RANGE */ + return -1; +} + +void +_Py_closerange_fallback(int first, int last) +{ + _Py_BEGIN_SUPPRESS_IPH #ifdef USE_CLOSEFROM if (last >= sysconf(_SC_OPEN_MAX)) { /* Any errors encountered while closing file descriptors are ignored */ closefrom(first); } - else #endif /* USE_CLOSEFROM */ #ifdef USE_FDWALK { @@ -2656,3 +2659,15 @@ _Py_closerange(int first, int last) #endif /* USE_FDWALK */ _Py_END_SUPPRESS_IPH } + +/* Closes all file descriptors in [first, last], ignoring errors. */ +void +_Py_closerange(int first, int last) +{ + first = Py_MAX(first, 0); + if (_Py_closerange_fast_async_safe(first, last) == 0) { + return; + } + _Py_closerange_fallback(first, last); +} +