Re: [PATCH] s-s-d: use close_range if possible

[Guillem Jover <guillem@debian.org>]

Hi!

On Fri, 2024-09-13 at 09:34:17 +0800, Mix wrote:
> This commit introduces a new `close_all_fds` function in s-s-d to close all file
> descriptors prior to starting a new process. The function leverages the
> `close_range` system call if it is supported (available in glibc >= 2.34 and
> kernel >= 5.9). If `close_range` is not available, the function falls back to
> iterating over each file descriptor individually.
> 
> These changes address the issue where s-s-d could freeze when the system has a
> relatively high `max_fds` setting.
> ---
> this change might be useful for systems with a high `max_fds` setting, i was
> found this problem while trying to run s-s-d in docker container since it have
> default max_fds set to 1073741816 and it will take a long time to close all fds

Ah, good idea, thanks for the patch!

> maybe this patch should be backported to previous version of s-s-d?

Hmm, right, I'll think about it, and if so I'll mark the commit as a
stable candidate.

> diff --git a/utils/start-stop-daemon.c b/utils/start-stop-daemon.c
> index 8899f0c03..029733b0e 100644
> --- a/utils/start-stop-daemon.c
> +++ b/utils/start-stop-daemon.c
> @@ -496,14 +496,26 @@ parse_unsigned(const char *string, int base, int *value_r)
>  	return 0;
>  }
>  
> -static long
> -get_open_fd_max(void)
> +static void
> +close_all_fds(void)
>  {
> +	long max_fd;
> +	int close_range_ret = ENOSYS;
> +
>  #ifdef HAVE_GETDTABLESIZE
> -	return getdtablesize();
> +	max_fd = getdtablesize();
>  #else
> -	return sysconf(_SC_OPEN_MAX);
> +	max_fd = sysconf(_SC_OPEN_MAX);
>  #endif
> +
> +#ifdef HAVE_CLOSE_RANGE
> +	close_range_ret = close_range(3, max_fd, 0);
> +#endif
> +	if (close_range_ret == ENOSYS) {

This is problematic as the function will return -1 on error, and not
the errno value. So if the libc supports the syscall, but the kernel
does not (and the libc does not have fallback code), then we might get
a failure due to ENOSYS, but not detect it. I think checking for
success is better here.

> +		long fd;
> +		for (fd = max_fd - 1; fd >= 3; fd--)
> +			close(fd);
> +	}
>  }

I think I'll go instead with the attached change, which uses
closefrom() as the main entry point, because that's way more portable,
and if it is supported by the system avoids more code.

Will do some tests on the various non-Linux systems I've got access
to, and then queue the patch for my next push.

Thanks,
Guillem

diff --git c/configure.ac i/configure.ac
index 3f1a86362..987f9efd4 100644
--- c/configure.ac
+++ i/configure.ac
@@ -213,6 +213,8 @@ AC_CHECK_FUNCS([\
 AC_CHECK_FUNCS([\
   setsid \
   getdtablesize \
+  closefrom \
+  close_range \
   getprocs64 \
   getprogname \
   getexecname \
diff --git c/utils/start-stop-daemon.c i/utils/start-stop-daemon.c
index 8899f0c03..d11ad2930 100644
--- c/utils/start-stop-daemon.c
+++ i/utils/start-stop-daemon.c
@@ -496,6 +496,7 @@ parse_unsigned(const char *string, int base, int *value_r)
 	return 0;
 }
 
+#ifndef HAVE_CLOSEFROM
 static long
 get_open_fd_max(void)
 {
@@ -506,6 +507,22 @@ get_open_fd_max(void)
 #endif
 }
 
+static void
+closefrom(int lowfd)
+{
+	long maxfd = get_open_fd_max();
+	int i;
+
+#ifdef HAVE_CLOSE_RANGE
+	if (close_range(lowfd, maxfd, 0) == 0)
+		return;
+#endif
+
+	for (i = maxfd - 1; i >= lowfd; --i)
+		close(i);
+}
+#endif
+
 #ifndef HAVE_SETSID
 static void
 detach_controlling_tty(void)
@@ -2660,13 +2677,10 @@ do_start(int argc, char **argv)
 		dup2(output_fd, 2); /* stderr */
 	}
 	if (background && close_io) {
-		int i;
-
 		dup2(devnull_fd, 0); /* stdin */
 
-		 /* Now close all extra fds. */
-		for (i = get_open_fd_max() - 1; i >= 3; --i)
-			close(i);
+		/* Now close all extra fds. */
+		closefrom(3);
 	}
 	execv(startas, argv);
 	fatale("unable to start %s", startas);