From 2d37239510ed6f2e005a1bd980ff6155438995a0 Mon Sep 17 00:00:00 2001 From: Sebastian Schwarz Date: Sun, 15 Sep 2019 12:53:38 +0800 Subject: [PATCH 1/4] gspawn: Optimize fd closing on AIX and BSDs Instead of calling close or fcntl on all possible file descriptors, which is slow on systems with very high limit or even no limit on open file descriptors, we can use closefrom or fcntl with F_CLOSEM to close all unwanted file descriptors with a single system call. This change only improves the performance when GSpawnChildSetupFunc is NULL because there are applications known to abuse GSpawnChildSetupFunc to unset FD_CLOEXEC on file descriptors. Since the change mentioned above requires closing file descriptors directly, it cannot be used when the caller may want to keep some of them open. This patch was written by Sebastian Schwarz and uploaded to https://gitlab.gnome.org/GNOME/glib/merge_requests/574. It was later modified by Ting-Wei Lan to address code review issues. Fixes: https://gitlab.gnome.org/GNOME/glib/issues/1638 --- glib/gspawn.c | 98 ++++++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 81 insertions(+), 17 deletions(-) diff --git a/glib/gspawn.c b/glib/gspawn.c index 7dd4bdcbbf..ea5e150344 100644 --- a/glib/gspawn.c +++ b/glib/gspawn.c @@ -47,7 +47,7 @@ #include #endif /* HAVE_SYS_RESOURCE_H */ -#ifdef __linux__ +#if defined(__linux__) || defined(__DragonFly__) #include /* for syscall and SYS_getdents64 */ #endif @@ -1126,6 +1126,28 @@ set_cloexec (void *data, gint fd) return 0; } +static gint +sane_close (gint fd) +{ + gint ret; + + retry: + ret = close (fd); + if (ret < 0 && errno == EINTR) + goto retry; + + return ret; +} + +G_GNUC_UNUSED static int +close_func (void *data, int fd) +{ + if (fd >= GPOINTER_TO_INT (data)) + (void) sane_close (fd); + + return 0; +} + #ifndef HAVE_FDWALK #ifdef __linux__ struct linux_dirent64 @@ -1200,7 +1222,7 @@ fdwalk (int (*cb)(void *data, int fd), void *data) } } - close (dir_fd); + sane_close (dir_fd); return res; } @@ -1223,7 +1245,39 @@ fdwalk (int (*cb)(void *data, int fd), void *data) return res; } +#endif /* HAVE_FDWALK */ + +static void +safe_closefrom (int lowfd) +{ +#if defined(__FreeBSD__) || defined(__OpenBSD__) + /* Use closefrom function provided by the system if it is known to be + * async-signal safe. + * + * FreeBSD: closefrom is included in the list of async-signal safe functions + * found in https://man.freebsd.org/sigaction(2). + * + * OpenBSD: closefrom is not included in the list, but a direct system call + * should be safe to use. + */ + (void) closefrom (lowfd); +#elif defined(__DragonFly__) + /* It is unclear whether closefrom function included in DragonFlyBSD libc_r + * is safe to use because it calls a lot of library functions. It is also + * unclear whether libc_r itself is still being used. Therefore, we do a + * direct system call here ourselves to avoid possible issues. + */ + (void) syscall (SYS_closefrom, lowfd); +#elif defined(F_CLOSEM) + /* NetBSD and AIX have a special fcntl command which does the same thing as + * closefrom. NetBSD also includes closefrom function, which seems to be a + * simple wrapper of the fcntl command. + */ + (void) fcntl (lowfd, F_CLOSEM); +#else + (void) fdwalk (close_func, GINT_TO_POINTER (lowfd)); #endif +} static gint sane_dup2 (gint fd1, gint fd2) @@ -1280,21 +1334,6 @@ do_exec (gint child_err_report_fd, if (working_directory && chdir (working_directory) < 0) write_err_and_exit (child_err_report_fd, CHILD_CHDIR_FAILED); - - /* Close all file descriptors but stdin stdout and stderr as - * soon as we exec. Note that this includes - * child_err_report_fd, which keeps the parent from blocking - * forever on the other end of that pipe. - */ - if (close_descriptors) - { - fdwalk (set_cloexec, GINT_TO_POINTER(3)); - } - else - { - /* We need to do child_err_report_fd anyway */ - set_cloexec (GINT_TO_POINTER(0), child_err_report_fd); - } /* Redirect pipes as required */ @@ -1351,6 +1390,31 @@ do_exec (gint child_err_report_fd, sane_dup2 (write_null, 2); close_and_invalidate (&write_null); } + + /* Close all file descriptors but stdin, stdout and stderr + * before we exec. Note that this includes + * child_err_report_fd, which keeps the parent from blocking + * forever on the other end of that pipe. + */ + if (close_descriptors) + { + if (child_setup == NULL) + { + sane_dup2 (child_err_report_fd, 3); + set_cloexec (GINT_TO_POINTER (0), 3); + safe_closefrom (4); + child_err_report_fd = 3; + } + else + { + fdwalk (set_cloexec, GINT_TO_POINTER (3)); + } + } + else + { + /* We need to do child_err_report_fd anyway */ + set_cloexec (GINT_TO_POINTER (0), child_err_report_fd); + } /* Call user function just before we exec */ if (child_setup) -- GitLab From 8af823c8e5f5c164a3b0e5ac3ccdafd647c1ba2b Mon Sep 17 00:00:00 2001 From: Ting-Wei Lan Date: Fri, 20 Sep 2019 22:36:30 +0800 Subject: [PATCH 2/4] gspawn: Use fdwalk provided by system only when it is known to be safe All uses of fdwalk in gspawn are between fork and exec, which means only async-signal safe functions can be called if the parent process has multiple threads. Since fdwalk is not a standard API, we should not assume it is safe to use unless the manual of the system explicitly says it is async-signal safe. Fixes: #1638 --- glib/gspawn.c | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/glib/gspawn.c b/glib/gspawn.c index ea5e150344..f17559a480 100644 --- a/glib/gspawn.c +++ b/glib/gspawn.c @@ -1148,7 +1148,6 @@ close_func (void *data, int fd) return 0; } -#ifndef HAVE_FDWALK #ifdef __linux__ struct linux_dirent64 { @@ -1188,8 +1187,21 @@ filename_to_fd (const char *p) #endif static int -fdwalk (int (*cb)(void *data, int fd), void *data) +safe_fdwalk (int (*cb)(void *data, int fd), void *data) { +#if 0 + /* Use fdwalk function provided by the system if it is known to be + * async-signal safe. + * + * Currently there are no operating systems known to provide a safe + * implementation, so this section is not used for now. + */ + return fdwalk (cb, data); +#else + /* Fallback implementation of fdwalk. It should be async-signal safe, but it + * may be slow on non-Linux operating systems, especially on systems allowing + * very high number of open file descriptors. + */ gint open_max; gint fd; gint res = 0; @@ -1244,8 +1256,8 @@ fdwalk (int (*cb)(void *data, int fd), void *data) break; return res; +#endif } -#endif /* HAVE_FDWALK */ static void safe_closefrom (int lowfd) @@ -1275,7 +1287,7 @@ safe_closefrom (int lowfd) */ (void) fcntl (lowfd, F_CLOSEM); #else - (void) fdwalk (close_func, GINT_TO_POINTER (lowfd)); + (void) safe_fdwalk (close_func, GINT_TO_POINTER (lowfd)); #endif } @@ -1407,7 +1419,7 @@ do_exec (gint child_err_report_fd, } else { - fdwalk (set_cloexec, GINT_TO_POINTER (3)); + safe_fdwalk (set_cloexec, GINT_TO_POINTER (3)); } } else -- GitLab From eae72c3597276bf7cda994d5262ec0421fd4c4f1 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Thu, 26 Sep 2019 14:10:36 +0100 Subject: [PATCH 3/4] gspawn: Rewrite some retry loops to use `while` rather than `goto` This introduces no functional changes, but does make the code easier to understand. Signed-off-by: Philip Withnall --- glib/gspawn.c | 21 +++++++++------------ 1 file changed, 9 insertions(+), 12 deletions(-) diff --git a/glib/gspawn.c b/glib/gspawn.c index f17559a480..2c154da5d6 100644 --- a/glib/gspawn.c +++ b/glib/gspawn.c @@ -1131,10 +1131,9 @@ sane_close (gint fd) { gint ret; - retry: - ret = close (fd); - if (ret < 0 && errno == EINTR) - goto retry; + do + ret = close (fd); + while (ret < 0 && errno == EINTR); return ret; } @@ -1296,10 +1295,9 @@ sane_dup2 (gint fd1, gint fd2) { gint ret; - retry: - ret = dup2 (fd1, fd2); - if (ret < 0 && errno == EINTR) - goto retry; + do + ret = dup2 (fd1, fd2); + while (ret < 0 && errno == EINTR); return ret; } @@ -1309,10 +1307,9 @@ sane_open (const char *path, gint mode) { gint ret; - retry: - ret = open (path, mode); - if (ret < 0 && errno == EINTR) - goto retry; + do + ret = open (path, mode); + while (ret < 0 && errno == EINTR); return ret; } -- GitLab From 1097b50c1c46c43b6b6af9aeefed447c6eb23b06 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Thu, 26 Sep 2019 14:13:01 +0100 Subject: [PATCH 4/4] gspawn: Retry on EBUSY errors from dup2() `man dup2` says that on Linux, dup2() can return `EBUSY` if the operation needs to be retried (in addition to returning `EINTR` in other cases where it needs to be retried). Signed-off-by: Philip Withnall --- glib/gspawn.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/glib/gspawn.c b/glib/gspawn.c index 2c154da5d6..bcbaec78fc 100644 --- a/glib/gspawn.c +++ b/glib/gspawn.c @@ -1297,7 +1297,7 @@ sane_dup2 (gint fd1, gint fd2) do ret = dup2 (fd1, fd2); - while (ret < 0 && errno == EINTR); + while (ret < 0 && (errno == EINTR || errno == EBUSY)); return ret; } -- GitLab