pythongh-92301: subprocess: Prefer close_range() to procfs-based fd c…

…losing

close_range() is much faster for large number of file descriptors, e.g.
4 times faster for 1000 descriptors in a Linux 5.16-based environment.

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);

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.

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

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);
+}
+