]> git.ipfire.org Git - thirdparty/systemd.git/commitdiff
basic/fd-util: sort the 'except' array in place
authorZbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl>
Fri, 23 Jul 2021 08:51:14 +0000 (10:51 +0200)
committerZbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl>
Fri, 23 Jul 2021 09:37:44 +0000 (11:37 +0200)
We need a sorted list of fds to skip over when closing. We would allocate a
copy of the passed array to do the sort. But all callers construct a temporary
array to pass to us, so it is pointless to copy it again.

close_all_fds/safe_fork_full/namespace_fork/fork_agent are changed to pass
a non-const int array. I checked all users, and all callers are fine with
the array being sorted.

The function was returning some number (sometimes 1, sometimes the extent
of the range passed over to close_range(), ???). Anyway, all callers only
check for error, so let's return 0 on success.

src/basic/fd-util.c
src/basic/fd-util.h
src/basic/process-util.c
src/basic/process-util.h

index 008f474344df51189021b86429d10e57553fb35c..99b517d8c2f0090e433caac6a7db9408ce59a39e 100644 (file)
@@ -208,10 +208,9 @@ static int get_max_fd(void) {
         return (int) (m - 1);
 }
 
-int close_all_fds(const int except[], size_t n_except) {
+int close_all_fds(int except[], size_t n_except) {
         static bool have_close_range = true; /* Assume we live in the future */
         _cleanup_closedir_ DIR *d = NULL;
-        struct dirent *de;
         int r = 0;
 
         assert(n_except == 0 || except);
@@ -227,129 +226,104 @@ int close_all_fds(const int except[], size_t n_except) {
                         /* Close everything. Yay! */
 
                         if (close_range(3, -1, 0) >= 0)
-                                return 1;
+                                return 0;
 
-                        if (!ERRNO_IS_NOT_SUPPORTED(errno) && !ERRNO_IS_PRIVILEGE(errno))
+                        if (ERRNO_IS_NOT_SUPPORTED(errno) || ERRNO_IS_PRIVILEGE(errno))
+                                have_close_range = false;
+                        else
                                 return -errno;
 
-                        have_close_range = false;
                 } else {
-                        _cleanup_free_ int *sorted_malloc = NULL;
-                        size_t n_sorted;
-                        int *sorted;
-
-                        assert(n_except < SIZE_MAX);
-                        n_sorted = n_except + 1;
-
-                        if (n_sorted > 64) /* Use heap for large numbers of fds, stack otherwise */
-                                sorted = sorted_malloc = new(int, n_sorted);
-                        else
-                                sorted = newa(int, n_sorted);
-
-                        if (sorted) {
-                                int c = 0;
-
-                                memcpy(sorted, except, n_except * sizeof(int));
-
-                                /* Let's add fd 2 to the list of fds, to simplify the loop below, as this
-                                 * allows us to cover the head of the array the same way as the body */
-                                sorted[n_sorted-1] = 2;
+                        typesafe_qsort(except, n_except, cmp_int);
 
-                                typesafe_qsort(sorted, n_sorted, cmp_int);
+                        for (size_t i = 0; i < n_except; i++) {
+                                int start = i == 0 ? 2 : MAX(except[i-1], 2); /* The first three fds shall always remain open */
+                                int end = MAX(except[i], 2);
 
-                                for (size_t i = 0; i < n_sorted-1; i++) {
-                                        int start, end;
+                                assert(end >= start);
 
-                                        start = MAX(sorted[i], 2); /* The first three fds shall always remain open */
-                                        end = MAX(sorted[i+1], 2);
-
-                                        assert(end >= start);
-
-                                        if (end - start <= 1)
-                                                continue;
-
-                                        /* Close everything between the start and end fds (both of which shall stay open) */
-                                        if (close_range(start + 1, end - 1, 0) < 0) {
-                                                if (!ERRNO_IS_NOT_SUPPORTED(errno) && !ERRNO_IS_PRIVILEGE(errno))
-                                                        return -errno;
+                                if (end - start <= 1)
+                                        continue;
 
+                                /* Close everything between the start and end fds (both of which shall stay open) */
+                                if (close_range(start + 1, end - 1, 0) < 0) {
+                                        if (ERRNO_IS_NOT_SUPPORTED(errno) || ERRNO_IS_PRIVILEGE(errno))
                                                 have_close_range = false;
-                                                break;
-                                        }
-
-                                        c += end - start - 1;
+                                        else
+                                                return -errno;
+                                        goto opendir_fallback;
                                 }
+                        }
 
-                                if (have_close_range) {
-                                        /* The loop succeeded. Let's now close everything beyond the end */
+                        /* The loop succeeded. Let's now close everything beyond the end */
 
-                                        if (sorted[n_sorted-1] >= INT_MAX) /* Dont let the addition below overflow */
-                                                return c;
+                        if (except[n_except-1] >= INT_MAX) /* Don't let the addition below overflow */
+                                return 0;
 
-                                        if (close_range(sorted[n_sorted-1] + 1, -1, 0) >= 0)
-                                                return c + 1;
+                        int start = MAX(except[n_except-1], 2);
 
-                                        if (!ERRNO_IS_NOT_SUPPORTED(errno) && !ERRNO_IS_PRIVILEGE(errno))
-                                                return -errno;
+                        if (close_range(start + 1, -1, 0) >= 0)
+                                return 0;
 
-                                        have_close_range = false;
-                                }
-                        }
+                        if (ERRNO_IS_NOT_SUPPORTED(errno) || ERRNO_IS_PRIVILEGE(errno))
+                                have_close_range = false;
+                        else
+                                return -errno;
                 }
-
-                /* Fallback on OOM or if close_range() is not supported */
         }
 
+        /* Fallback for when close_range() is not supported */
+ opendir_fallback:
         d = opendir("/proc/self/fd");
-        if (!d) {
-                int fd, max_fd;
+        if (d) {
+                struct dirent *de;
 
-                /* When /proc isn't available (for example in chroots) the fallback is brute forcing through
-                 * the fd table */
+                FOREACH_DIRENT(de, d, return -errno) {
+                        int fd = -1, q;
 
-                max_fd = get_max_fd();
-                if (max_fd < 0)
-                        return max_fd;
+                        if (safe_atoi(de->d_name, &fd) < 0)
+                                /* Let's better ignore this, just in case */
+                                continue;
 
-                /* Refuse to do the loop over more too many elements. It's better to fail immediately than to
-                 * spin the CPU for a long time. */
-                if (max_fd > MAX_FD_LOOP_LIMIT)
-                        return log_debug_errno(SYNTHETIC_ERRNO(EPERM),
-                                               "/proc/self/fd is inaccessible. Refusing to loop over %d potential fds.",
-                                               max_fd);
+                        if (fd < 3)
+                                continue;
 
-                for (fd = 3; fd >= 0; fd = fd < max_fd ? fd + 1 : -1) {
-                        int q;
+                        if (fd == dirfd(d))
+                                continue;
 
                         if (fd_in_set(fd, except, n_except))
                                 continue;
 
                         q = close_nointr(fd);
-                        if (q < 0 && q != -EBADF && r >= 0)
+                        if (q < 0 && q != -EBADF && r >= 0) /* Valgrind has its own FD and doesn't want to have it closed */
                                 r = q;
                 }
 
                 return r;
         }
 
-        FOREACH_DIRENT(de, d, return -errno) {
-                int fd = -1, q;
+        /* Fallback for when /proc isn't available (for example in chroots) by brute-forcing through the file
+         * descriptor table. */
 
-                if (safe_atoi(de->d_name, &fd) < 0)
-                        /* Let's better ignore this, just in case */
-                        continue;
+        int max_fd = get_max_fd();
+        if (max_fd < 0)
+                return max_fd;
 
-                if (fd < 3)
-                        continue;
+        /* Refuse to do the loop over more too many elements. It's better to fail immediately than to
+         * spin the CPU for a long time. */
+        if (max_fd > MAX_FD_LOOP_LIMIT)
+                return log_debug_errno(SYNTHETIC_ERRNO(EPERM),
+                                       "/proc/self/fd is inaccessible. Refusing to loop over %d potential fds.",
+                                       max_fd);
 
-                if (fd == dirfd(d))
-                        continue;
+        for (int fd = 3; fd >= 0; fd = fd < max_fd ? fd + 1 : -1) {
+                int q;
 
                 if (fd_in_set(fd, except, n_except))
                         continue;
 
                 q = close_nointr(fd);
-                if (q < 0 && q != -EBADF && r >= 0) /* Valgrind has its own FD and doesn't want to have it closed */
+                if (q < 0 && q != -EBADF && r >= 0)
                         r = q;
         }
 
index 9529a4723d39fa2060acd233de735eee348babc8..d3c2944d869a69a26d94f1167aa423b86093734c 100644 (file)
@@ -56,7 +56,7 @@ DEFINE_TRIVIAL_CLEANUP_FUNC_FULL(DIR*, closedir, NULL);
 int fd_nonblock(int fd, bool nonblock);
 int fd_cloexec(int fd, bool cloexec);
 
-int close_all_fds(const int except[], size_t n_except);
+int close_all_fds(int except[], size_t n_except);
 
 int same_fd(int a, int b);
 
index 14259ea8dfe4a99371afe1537c0cd8e4eaf677cb..30a608231ead5e17d8c1b31337819463a972121d 100644 (file)
@@ -1271,7 +1271,7 @@ static void restore_sigsetp(sigset_t **ssp) {
 
 int safe_fork_full(
                 const char *name,
-                const int except_fds[],
+                int except_fds[],
                 size_t n_except_fds,
                 ForkFlags flags,
                 pid_t *ret_pid) {
@@ -1466,7 +1466,7 @@ int safe_fork_full(
 int namespace_fork(
                 const char *outer_name,
                 const char *inner_name,
-                const int except_fds[],
+                int except_fds[],
                 size_t n_except_fds,
                 ForkFlags flags,
                 int pidns_fd,
@@ -1482,7 +1482,8 @@ int namespace_fork(
          * process. This ensures that we are fully a member of the destination namespace, with pidns an all, so that
          * /proc/self/fd works correctly. */
 
-        r = safe_fork_full(outer_name, except_fds, n_except_fds, (flags|FORK_DEATHSIG) & ~(FORK_REOPEN_LOG|FORK_NEW_MOUNTNS|FORK_MOUNTNS_SLAVE), ret_pid);
+        r = safe_fork_full(outer_name, except_fds, n_except_fds,
+                           (flags|FORK_DEATHSIG) & ~(FORK_REOPEN_LOG|FORK_NEW_MOUNTNS|FORK_MOUNTNS_SLAVE), ret_pid);
         if (r < 0)
                 return r;
         if (r == 0) {
@@ -1517,7 +1518,7 @@ int namespace_fork(
         return 1;
 }
 
-int fork_agent(const char *name, const int except[], size_t n_except, pid_t *ret_pid, const char *path, ...) {
+int fork_agent(const char *name, int except[], size_t n_except, pid_t *ret_pid, const char *path, ...) {
         bool stdout_is_tty, stderr_is_tty;
         size_t n, i;
         va_list ap;
index 0e064de85e838572a277f081f98b65d83a18399f..73717dd3a4fa99814d4e1fae7dce8d7e11e0162e 100644 (file)
@@ -168,15 +168,15 @@ typedef enum ForkFlags {
         FORK_NEW_USERNS         = 1 << 13, /* Run child in its own user namespace */
 } ForkFlags;
 
-int safe_fork_full(const char *name, const int except_fds[], size_t n_except_fds, ForkFlags flags, pid_t *ret_pid);
+int safe_fork_full(const char *name, int except_fds[], size_t n_except_fds, ForkFlags flags, pid_t *ret_pid);
 
 static inline int safe_fork(const char *name, ForkFlags flags, pid_t *ret_pid) {
         return safe_fork_full(name, NULL, 0, flags, ret_pid);
 }
 
-int namespace_fork(const char *outer_name, const char *inner_name, const int except_fds[], size_t n_except_fds, ForkFlags flags, int pidns_fd, int mntns_fd, int netns_fd, int userns_fd, int root_fd, pid_t *ret_pid);
+int namespace_fork(const char *outer_name, const char *inner_name, int except_fds[], size_t n_except_fds, ForkFlags flags, int pidns_fd, int mntns_fd, int netns_fd, int userns_fd, int root_fd, pid_t *ret_pid);
 
-int fork_agent(const char *name, const int except[], size_t n_except, pid_t *pid, const char *path, ...) _sentinel_;
+int fork_agent(const char *name, int except[], size_t n_except, pid_t *pid, const char *path, ...) _sentinel_;
 
 int set_oom_score_adjust(int value);