From 0e05ef77506e9e44a8afef67a12269ad08223354 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Mon, 22 Jun 2020 12:36:32 +0100 Subject: [PATCH 1/8] gspawn: Audit for async-signal-safety Functions called between `fork()` and `exec()` have to be async-signal-safe. Add a comment to each function which is called in that context, and `FIXME` comments to the non-async-signal-safe functions which end up being called as leaves of the call graph. The following commits will fix those `FIXME`s. See `man 7 signal-safety`. Signed-off-by: Philip Withnall Helps: #2140 --- glib/gspawn.c | 42 +++++++++++++++++++++++++++++++++++++++++- 1 file changed, 41 insertions(+), 1 deletion(-) diff --git a/glib/gspawn.c b/glib/gspawn.c index 2ad0db2af..492d41b32 100644 --- a/glib/gspawn.c +++ b/glib/gspawn.c @@ -262,6 +262,9 @@ g_spawn_async (const gchar *working_directory, /* Avoids a danger in threaded situations (calling close() * on a file descriptor twice, and another thread has * re-opened it since the first close) + * + * This function is called between fork() and exec() and hence must be + * async-signal-safe (see signal-safety(7)). */ static void close_and_invalidate (gint *fd) @@ -270,6 +273,7 @@ close_and_invalidate (gint *fd) return; else { + /* FIXME: g_close() is not async-signal-safe on failure. */ (void) g_close (*fd, NULL); *fd = -1; } @@ -1081,6 +1085,8 @@ g_spawn_check_exit_status (gint exit_status, return ret; } +/* This function is called between fork() and exec() and hence must be + * async-signal-safe (see signal-safety(7)). */ static gssize write_all (gint fd, gconstpointer vbuf, gsize to_write) { @@ -1104,6 +1110,8 @@ write_all (gint fd, gconstpointer vbuf, gsize to_write) return TRUE; } +/* This function is called between fork() and exec() and hence must be + * async-signal-safe (see signal-safety(7)). */ G_GNUC_NORETURN static void write_err_and_exit (gint fd, gint msg) @@ -1116,6 +1124,8 @@ write_err_and_exit (gint fd, gint msg) _exit (1); } +/* This function is called between fork() and exec() and hence must be + * async-signal-safe (see signal-safety(7)). */ static int set_cloexec (void *data, gint fd) { @@ -1125,6 +1135,8 @@ set_cloexec (void *data, gint fd) return 0; } +/* This function is called between fork() and exec() and hence must be + * async-signal-safe (see signal-safety(7)). */ static gint safe_close (gint fd) { @@ -1137,6 +1149,8 @@ safe_close (gint fd) return ret; } +/* This function is called between fork() and exec() and hence must be + * async-signal-safe (see signal-safety(7)). */ G_GNUC_UNUSED static int close_func (void *data, int fd) { @@ -1156,6 +1170,8 @@ struct linux_dirent64 char d_name[]; /* Filename (null-terminated) */ }; +/* This function is called between fork() and exec() and hence must be + * async-signal-safe (see signal-safety(7)). */ static gint filename_to_fd (const char *p) { @@ -1169,6 +1185,7 @@ filename_to_fd (const char *p) while ((c = *p++) != '\0') { + /* FIXME: g_ascii_isdigit() is not necessarily async-signal-safe. */ if (!g_ascii_isdigit (c)) return -1; c -= '0'; @@ -1184,6 +1201,8 @@ filename_to_fd (const char *p) } #endif +/* This function is called between fork() and exec() and hence must be + * async-signal-safe (see signal-safety(7)). */ static int safe_fdwalk (int (*cb)(void *data, int fd), void *data) { @@ -1237,7 +1256,9 @@ safe_fdwalk (int (*cb)(void *data, int fd), void *data) } /* If /proc is not mounted or not accessible we fall back to the old - * rlimit trick */ + * rlimit trick. + * + * FIXME: getrlimit() and sysconf() are not async-signal-safe. */ #endif @@ -1257,6 +1278,8 @@ safe_fdwalk (int (*cb)(void *data, int fd), void *data) #endif } +/* This function is called between fork() and exec() and hence must be + * async-signal-safe (see signal-safety(7)). */ static void safe_closefrom (int lowfd) { @@ -1289,6 +1312,8 @@ safe_closefrom (int lowfd) #endif } +/* This function is called between fork() and exec() and hence must be + * async-signal-safe (see signal-safety(7)). */ static gint safe_dup2 (gint fd1, gint fd2) { @@ -1301,6 +1326,8 @@ safe_dup2 (gint fd1, gint fd2) return ret; } +/* This function is called between fork() and exec() and hence must be + * async-signal-safe (see signal-safety(7)). */ static gint safe_open (const char *path, gint mode) { @@ -1321,6 +1348,8 @@ enum CHILD_FORK_FAILED }; +/* This function is called between fork() and exec() and hence must be + * async-signal-safe (see signal-safety(7)) until it calls exec(). */ static void do_exec (gint child_err_report_fd, gint stdin_fd, @@ -1358,6 +1387,7 @@ do_exec (gint child_err_report_fd, else if (!child_inherits_stdin) { /* Keep process from blocking on a read of stdin */ + /* FIXME: g_assert() is not async-signal-safe on failure. */ gint read_null = safe_open ("/dev/null", O_RDONLY); g_assert (read_null != -1); safe_dup2 (read_null, 0); @@ -1376,6 +1406,7 @@ do_exec (gint child_err_report_fd, } else if (stdout_to_null) { + /* FIXME: g_assert() is not async-signal-safe on failure. */ gint write_null = safe_open ("/dev/null", O_WRONLY); g_assert (write_null != -1); safe_dup2 (write_null, 1); @@ -2113,6 +2144,8 @@ cleanup_and_fail: /* Based on execvp from GNU C Library */ +/* This function is called between fork() and exec() and hence must be + * async-signal-safe (see signal-safety(7)) until it calls exec(). */ static void script_execute (const gchar *file, gchar **argv, @@ -2127,6 +2160,7 @@ script_execute (const gchar *file, { gchar **new_argv; + /* FIXME: This is not async-signal-safe */ new_argv = g_new0 (gchar*, argc + 2); /* /bin/sh and NULL */ new_argv[0] = (char *) "/bin/sh"; @@ -2147,6 +2181,8 @@ script_execute (const gchar *file, } } +/* This function is called between fork() and exec() and hence must be + * async-signal-safe (see signal-safety(7)). */ static gchar* my_strchrnul (const gchar *str, gchar c) { @@ -2157,6 +2193,8 @@ my_strchrnul (const gchar *str, gchar c) return p; } +/* This function is called between fork() and exec() and hence must be + * async-signal-safe (see signal-safety(7)) until it calls exec(). */ static gint g_execute (const gchar *file, gchar **argv, @@ -2190,6 +2228,7 @@ g_execute (const gchar *file, gsize len; gsize pathlen; + /* FIXME: getenv() is not async-signal-safe */ path = NULL; if (search_path_from_envp) path = g_environ_getenv (envp, "PATH"); @@ -2213,6 +2252,7 @@ g_execute (const gchar *file, len = strlen (file) + 1; pathlen = strlen (path); + /* FIXME: malloc() is not async-signal-safe */ freeme = name = g_malloc (pathlen + len + 1); /* Copy the file name at the top, including '\0' */ From 6f46294227f8051b4826c95a4b849e4b89f5413d Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Mon, 22 Jun 2020 13:09:56 +0100 Subject: [PATCH 2/8] =?UTF-8?q?gspawn:=20Don=E2=80=99t=20use=20g=5Fclose()?= =?UTF-8?q?=20in=20async-signal-safe=20context?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Use normal `close()` instead, which is guaranteed to be async-signal-safe. See `man 7 signal-safety`. Signed-off-by: Philip Withnall Helps: #2140 --- glib/gspawn.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/glib/gspawn.c b/glib/gspawn.c index 492d41b32..09793fe1c 100644 --- a/glib/gspawn.c +++ b/glib/gspawn.c @@ -155,6 +155,7 @@ extern char **environ; */ +static gint safe_close (gint fd); static gint g_execute (const gchar *file, gchar **argv, @@ -273,8 +274,7 @@ close_and_invalidate (gint *fd) return; else { - /* FIXME: g_close() is not async-signal-safe on failure. */ - (void) g_close (*fd, NULL); + safe_close (*fd); *fd = -1; } } From 33948929dff5f59ce70a320efee27f55479885af Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Mon, 22 Jun 2020 13:10:35 +0100 Subject: [PATCH 3/8] =?UTF-8?q?gspawn:=20Don=E2=80=99t=20use=20g=5Fascii?= =?UTF-8?q?=5Fisdigit()=20in=20async-signal-safe=20context?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit While `g_ascii_isdigit()` *is* currently async-signal-safe, it’s going to be hard to remember to keep it that way if the implementation changes in future. It seems more robust to just reimplement it here, given that it’s not much code. See `man 7 signal-safety`. Signed-off-by: Philip Withnall Helps: #2140 --- glib/gspawn.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/glib/gspawn.c b/glib/gspawn.c index 09793fe1c..458f6c8a2 100644 --- a/glib/gspawn.c +++ b/glib/gspawn.c @@ -1185,8 +1185,7 @@ filename_to_fd (const char *p) while ((c = *p++) != '\0') { - /* FIXME: g_ascii_isdigit() is not necessarily async-signal-safe. */ - if (!g_ascii_isdigit (c)) + if (c < '0' || c > '9') return -1; c -= '0'; From 1051bfe11e699244c4f376702a6c7a802a5133f1 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Mon, 22 Jun 2020 13:11:32 +0100 Subject: [PATCH 4/8] =?UTF-8?q?gspawn:=20Don=E2=80=99t=20use=20g=5Fassert(?= =?UTF-8?q?)=20in=20async-signal-safe=20context?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Use the error handling infrastructure which already exists for other failures in the async-signal-safe context. `g_assert()` is unlikely to have caused problems in practice because it is only async-signal-unsafe when the assertion condition fails. See `man 7 signal-safety`. Signed-off-by: Philip Withnall Helps: #2140 --- glib/gspawn.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/glib/gspawn.c b/glib/gspawn.c index 458f6c8a2..d062681a1 100644 --- a/glib/gspawn.c +++ b/glib/gspawn.c @@ -1386,9 +1386,10 @@ do_exec (gint child_err_report_fd, else if (!child_inherits_stdin) { /* Keep process from blocking on a read of stdin */ - /* FIXME: g_assert() is not async-signal-safe on failure. */ gint read_null = safe_open ("/dev/null", O_RDONLY); - g_assert (read_null != -1); + if (read_null < 0) + write_err_and_exit (child_err_report_fd, + CHILD_DUP2_FAILED); safe_dup2 (read_null, 0); close_and_invalidate (&read_null); } @@ -1405,9 +1406,10 @@ do_exec (gint child_err_report_fd, } else if (stdout_to_null) { - /* FIXME: g_assert() is not async-signal-safe on failure. */ gint write_null = safe_open ("/dev/null", O_WRONLY); - g_assert (write_null != -1); + if (write_null < 0) + write_err_and_exit (child_err_report_fd, + CHILD_DUP2_FAILED); safe_dup2 (write_null, 1); close_and_invalidate (&write_null); } From 84f188ae2437230a58f57fd60da579a360f48161 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Mon, 22 Jun 2020 13:40:27 +0100 Subject: [PATCH 5/8] =?UTF-8?q?gspawn:=20Don=E2=80=99t=20use=20getrlimit()?= =?UTF-8?q?=20or=20sysconf()=20in=20async-signal-safe=20context?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit They’re not safe to call in an async-signal-safe context on Linux. `sysconf()` is safe to call on FreeBSD and OpenBSD (at least), so continue doing that. This will reduce performance in the (already low performance) fallback case where `/proc` is inaccessible to a forked process on Linux, while spawning a subprocess. See `man 7 signal-safety`. Signed-off-by: Philip Withnall Helps: #2140 --- glib/gspawn.c | 41 +++++++++++++++++++++++++++++------------ 1 file changed, 29 insertions(+), 12 deletions(-) diff --git a/glib/gspawn.c b/glib/gspawn.c index d062681a1..29eaebb1f 100644 --- a/glib/gspawn.c +++ b/glib/gspawn.c @@ -1218,11 +1218,11 @@ safe_fdwalk (int (*cb)(void *data, int fd), void *data) * may be slow on non-Linux operating systems, especially on systems allowing * very high number of open file descriptors. */ - gint open_max; + gint open_max = -1; gint fd; gint res = 0; -#ifdef HAVE_SYS_RESOURCE_H +#if 0 && defined(HAVE_SYS_RESOURCE_H) struct rlimit rl; #endif @@ -1255,19 +1255,36 @@ safe_fdwalk (int (*cb)(void *data, int fd), void *data) } /* If /proc is not mounted or not accessible we fall back to the old - * rlimit trick. + * rlimit trick. */ + +#endif + +#if 0 && defined(HAVE_SYS_RESOURCE_H) + /* Use getrlimit() function provided by the system if it is known to be + * async-signal safe. * - * FIXME: getrlimit() and sysconf() are not async-signal-safe. */ - + * Currently there are no operating systems known to provide a safe + * implementation, so this section is not used for now. + */ + if (getrlimit (RLIMIT_NOFILE, &rl) == 0 && rl.rlim_max != RLIM_INFINITY) + open_max = rl.rlim_max; #endif - -#ifdef HAVE_SYS_RESOURCE_H - - if (getrlimit(RLIMIT_NOFILE, &rl) == 0 && rl.rlim_max != RLIM_INFINITY) - open_max = rl.rlim_max; - else +#if defined(__FreeBSD__) || defined(__OpenBSD__) + /* Use sysconf() function provided by the system if it is known to be + * async-signal safe. + * + * FreeBSD: sysconf() is included in the list of async-signal safe functions + * found in https://man.freebsd.org/sigaction(2). + * + * OpenBSD: sysconf() is included in the list of async-signal safe functions + * found in https://man.openbsd.org/sigaction.2. + */ + if (open_max < 0) + open_max = sysconf (_SC_OPEN_MAX); #endif - open_max = sysconf (_SC_OPEN_MAX); + /* Hardcoded fallback: the default process hard limit in Linux as of 2020 */ + if (open_max < 0) + open_max = 4096; for (fd = 0; fd < open_max; fd++) if ((res = cb (data, fd)) != 0) From 62ce66d4e79645e112c3b7f7c8cd05006df74098 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Mon, 22 Jun 2020 13:59:48 +0100 Subject: [PATCH 6/8] =?UTF-8?q?gspawn:=20Don=E2=80=99t=20use=20getenv()=20?= =?UTF-8?q?in=20async-signal-safe=20context?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Query the environment before calling `fork()` so that it doesn’t have to be called in the async-signal-safe context between `fork()` and `exec()`. See `man 7 signal-safety`. Signed-off-by: Philip Withnall Helps: #2140 --- glib/gspawn.c | 76 +++++++++++++++++++++++++-------------------------- 1 file changed, 37 insertions(+), 39 deletions(-) diff --git a/glib/gspawn.c b/glib/gspawn.c index 29eaebb1f..77355b9ac 100644 --- a/glib/gspawn.c +++ b/glib/gspawn.c @@ -158,10 +158,9 @@ extern char **environ; static gint safe_close (gint fd); static gint g_execute (const gchar *file, - gchar **argv, - gchar **envp, - gboolean search_path, - gboolean search_path_from_envp); + gchar **argv, + gchar **envp, + const gchar *search_path); static gboolean fork_exec_with_pipes (gboolean intermediate_child, const gchar *working_directory, @@ -1375,8 +1374,7 @@ do_exec (gint child_err_report_fd, gchar **argv, gchar **envp, gboolean close_descriptors, - gboolean search_path, - gboolean search_path_from_envp, + const gchar *search_path, gboolean stdout_to_null, gboolean stderr_to_null, gboolean child_inherits_stdin, @@ -1481,7 +1479,7 @@ do_exec (gint child_err_report_fd, g_execute (argv[0], file_and_argv_zero ? argv + 1 : argv, - envp, search_path, search_path_from_envp); + envp, search_path); /* Exec failed */ write_err_and_exit (child_err_report_fd, @@ -1743,6 +1741,7 @@ fork_exec_with_fds (gboolean intermediate_child, gint child_pid_report_pipe[2] = { -1, -1 }; guint pipe_flags = cloexec_pipes ? FD_CLOEXEC : 0; gint status; + const gchar *chosen_search_path; #ifdef POSIX_SPAWN_AVAILABLE if (!intermediate_child && working_directory == NULL && !close_descriptors && @@ -1793,6 +1792,29 @@ fork_exec_with_fds (gboolean intermediate_child, } #endif /* POSIX_SPAWN_AVAILABLE */ + /* Choose a search path. This has to be done before calling fork() + * as getenv() isn’t async-signal-safe (see `man 7 signal-safety`). */ + chosen_search_path = NULL; + if (search_path_from_envp) + chosen_search_path = g_environ_getenv (envp, "PATH"); + if (search_path && chosen_search_path == NULL) + chosen_search_path = g_getenv ("PATH"); + + if (chosen_search_path == NULL) + { + /* There is no 'PATH' in the environment. The default + * * search path in libc is the current directory followed by + * * the path 'confstr' returns for '_CS_PATH'. + * */ + + /* In GLib we put . last, for security, and don't use the + * * unportable confstr(); UNIX98 does not actually specify + * * what to search if PATH is unset. POSIX may, dunno. + * */ + + chosen_search_path = "/bin:/usr/bin:."; + } + if (!g_unix_open_pipe (child_err_report_pipe, pipe_flags, error)) return FALSE; @@ -1874,8 +1896,7 @@ fork_exec_with_fds (gboolean intermediate_child, argv, envp, close_descriptors, - search_path, - search_path_from_envp, + chosen_search_path, stdout_to_null, stderr_to_null, child_inherits_stdin, @@ -1904,8 +1925,7 @@ fork_exec_with_fds (gboolean intermediate_child, argv, envp, close_descriptors, - search_path, - search_path_from_envp, + chosen_search_path, stdout_to_null, stderr_to_null, child_inherits_stdin, @@ -2214,11 +2234,10 @@ my_strchrnul (const gchar *str, gchar c) /* This function is called between fork() and exec() and hence must be * async-signal-safe (see signal-safety(7)) until it calls exec(). */ static gint -g_execute (const gchar *file, - gchar **argv, - gchar **envp, - gboolean search_path, - gboolean search_path_from_envp) +g_execute (const gchar *file, + gchar **argv, + gchar **envp, + const gchar *search_path) { if (*file == '\0') { @@ -2227,7 +2246,7 @@ g_execute (const gchar *file, return -1; } - if (!(search_path || search_path_from_envp) || strchr (file, '/') != NULL) + if (search_path == NULL || strchr (file, '/') != NULL) { /* Don't search when it contains a slash. */ if (envp) @@ -2246,28 +2265,7 @@ g_execute (const gchar *file, gsize len; gsize pathlen; - /* FIXME: getenv() is not async-signal-safe */ - path = NULL; - if (search_path_from_envp) - path = g_environ_getenv (envp, "PATH"); - if (search_path && path == NULL) - path = g_getenv ("PATH"); - - if (path == NULL) - { - /* There is no 'PATH' in the environment. The default - * search path in libc is the current directory followed by - * the path 'confstr' returns for '_CS_PATH'. - */ - - /* In GLib we put . last, for security, and don't use the - * unportable confstr(); UNIX98 does not actually specify - * what to search if PATH is unset. POSIX may, dunno. - */ - - path = "/bin:/usr/bin:."; - } - + path = search_path; len = strlen (file) + 1; pathlen = strlen (path); /* FIXME: malloc() is not async-signal-safe */ From cf5af28169c899a1ce1ea36c1e8cdf55f2ab3124 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Mon, 22 Jun 2020 14:15:42 +0100 Subject: [PATCH 7/8] =?UTF-8?q?gspawn:=20Don=E2=80=99t=20use=20malloc()=20?= =?UTF-8?q?when=20searching=20for=20a=20binary?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Allocate a working buffer before calling `fork()` to avoid calling `malloc()` in the async-signal-safe context between `fork()` and `exec()`, where it’s not safe to use. In this case, the buffer is used to assemble elements from `PATH` with the binary from `argv[0]` to try executing them. See `man 7 signal-safety`. Signed-off-by: Philip Withnall Helps: #2140 --- glib/gspawn.c | 54 +++++++++++++++++++++++++++++++++++++++------------ 1 file changed, 42 insertions(+), 12 deletions(-) diff --git a/glib/gspawn.c b/glib/gspawn.c index 77355b9ac..1e0783e56 100644 --- a/glib/gspawn.c +++ b/glib/gspawn.c @@ -160,7 +160,9 @@ static gint safe_close (gint fd); static gint g_execute (const gchar *file, gchar **argv, gchar **envp, - const gchar *search_path); + const gchar *search_path, + gchar *search_path_buffer, + gsize search_path_buffer_len); static gboolean fork_exec_with_pipes (gboolean intermediate_child, const gchar *working_directory, @@ -1375,6 +1377,8 @@ do_exec (gint child_err_report_fd, gchar **envp, gboolean close_descriptors, const gchar *search_path, + gchar *search_path_buffer, + gsize search_path_buffer_len, gboolean stdout_to_null, gboolean stderr_to_null, gboolean child_inherits_stdin, @@ -1479,7 +1483,7 @@ do_exec (gint child_err_report_fd, g_execute (argv[0], file_and_argv_zero ? argv + 1 : argv, - envp, search_path); + envp, search_path, search_path_buffer, search_path_buffer_len); /* Exec failed */ write_err_and_exit (child_err_report_fd, @@ -1742,6 +1746,8 @@ fork_exec_with_fds (gboolean intermediate_child, guint pipe_flags = cloexec_pipes ? FD_CLOEXEC : 0; gint status; const gchar *chosen_search_path; + gchar *search_path_buffer = NULL; + gsize search_path_buffer_len = 0; #ifdef POSIX_SPAWN_AVAILABLE if (!intermediate_child && working_directory == NULL && !close_descriptors && @@ -1815,8 +1821,20 @@ fork_exec_with_fds (gboolean intermediate_child, chosen_search_path = "/bin:/usr/bin:."; } + /* Allocate a buffer which the fork()ed child can use to assemble potential + * paths for the binary to exec(), combining the argv[0] and elements from + * the chosen_search_path. This can’t be done in the child because malloc() + * (or alloca()) are not async-signal-safe (see `man 7 signal-safety`). + * + * Add 2 for the nul terminator and a leading `/`. */ + search_path_buffer_len = strlen (chosen_search_path) + strlen (argv[0]) + 2; + search_path_buffer = g_malloc (search_path_buffer_len); + if (!g_unix_open_pipe (child_err_report_pipe, pipe_flags, error)) - return FALSE; + { + g_free (search_path_buffer); + return FALSE; + } if (intermediate_child && !g_unix_open_pipe (child_pid_report_pipe, pipe_flags, error)) goto cleanup_and_fail; @@ -1897,6 +1915,8 @@ fork_exec_with_fds (gboolean intermediate_child, envp, close_descriptors, chosen_search_path, + search_path_buffer, + search_path_buffer_len, stdout_to_null, stderr_to_null, child_inherits_stdin, @@ -1926,6 +1946,8 @@ fork_exec_with_fds (gboolean intermediate_child, envp, close_descriptors, chosen_search_path, + search_path_buffer, + search_path_buffer_len, stdout_to_null, stderr_to_null, child_inherits_stdin, @@ -2052,7 +2074,9 @@ fork_exec_with_fds (gboolean intermediate_child, /* Success against all odds! return the information */ close_and_invalidate (&child_err_report_pipe[0]); close_and_invalidate (&child_pid_report_pipe[0]); - + + g_free (search_path_buffer); + if (child_pid) *child_pid = pid; @@ -2085,6 +2109,8 @@ fork_exec_with_fds (gboolean intermediate_child, close_and_invalidate (&child_pid_report_pipe[0]); close_and_invalidate (&child_pid_report_pipe[1]); + g_free (search_path_buffer); + return FALSE; } @@ -2237,7 +2263,9 @@ static gint g_execute (const gchar *file, gchar **argv, gchar **envp, - const gchar *search_path) + const gchar *search_path, + gchar *search_path_buffer, + gsize search_path_buffer_len) { if (*file == '\0') { @@ -2261,16 +2289,21 @@ g_execute (const gchar *file, { gboolean got_eacces = 0; const gchar *path, *p; - gchar *name, *freeme; + gchar *name; gsize len; gsize pathlen; path = search_path; len = strlen (file) + 1; pathlen = strlen (path); - /* FIXME: malloc() is not async-signal-safe */ - freeme = name = g_malloc (pathlen + len + 1); - + name = search_path_buffer; + + if (search_path_buffer_len < pathlen + len + 1) + { + errno = ENOMEM; + return -1; + } + /* Copy the file name at the top, including '\0' */ memcpy (name + pathlen + 1, file, len); name = name + pathlen; @@ -2338,7 +2371,6 @@ g_execute (const gchar *file, * something went wrong executing it; return the error to our * caller. */ - g_free (freeme); return -1; } } @@ -2350,8 +2382,6 @@ g_execute (const gchar *file, * error. */ errno = EACCES; - - g_free (freeme); } /* Return the error from the last attempt (probably ENOENT). */ From dd36248f9e46df415207681cd817596ccbb06186 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Mon, 22 Jun 2020 14:33:12 +0100 Subject: [PATCH 8/8] =?UTF-8?q?gspawn:=20Don=E2=80=99t=20use=20malloc()=20?= =?UTF-8?q?when=20running=20a=20binary=20under=20/bin/sh?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Allocate a working buffer before calling `fork()` to avoid calling `malloc()` in the async-signal-safe context between `fork()` and `exec()`, where it’s not safe to use. In this case, the buffer is used to assemble a wrapper around `argv` so it can be run under `/bin/sh`. See `man 7 signal-safety`. Signed-off-by: Philip Withnall Fixes: #2140 --- glib/gspawn.c | 82 ++++++++++++++++++++++++++++++++++----------------- 1 file changed, 55 insertions(+), 27 deletions(-) diff --git a/glib/gspawn.c b/glib/gspawn.c index 1e0783e56..e5f1413ed 100644 --- a/glib/gspawn.c +++ b/glib/gspawn.c @@ -159,6 +159,8 @@ static gint safe_close (gint fd); static gint g_execute (const gchar *file, gchar **argv, + gchar **argv_buffer, + gsize argv_buffer_len, gchar **envp, const gchar *search_path, gchar *search_path_buffer, @@ -1374,6 +1376,8 @@ do_exec (gint child_err_report_fd, gint stderr_fd, const gchar *working_directory, gchar **argv, + gchar **argv_buffer, + gsize argv_buffer_len, gchar **envp, gboolean close_descriptors, const gchar *search_path, @@ -1483,6 +1487,7 @@ do_exec (gint child_err_report_fd, g_execute (argv[0], file_and_argv_zero ? argv + 1 : argv, + argv_buffer, argv_buffer_len, envp, search_path, search_path_buffer, search_path_buffer_len); /* Exec failed */ @@ -1748,6 +1753,8 @@ fork_exec_with_fds (gboolean intermediate_child, const gchar *chosen_search_path; gchar *search_path_buffer = NULL; gsize search_path_buffer_len = 0; + gchar **argv_buffer = NULL; + gsize argv_buffer_len = 0; #ifdef POSIX_SPAWN_AVAILABLE if (!intermediate_child && working_directory == NULL && !close_descriptors && @@ -1830,9 +1837,16 @@ fork_exec_with_fds (gboolean intermediate_child, search_path_buffer_len = strlen (chosen_search_path) + strlen (argv[0]) + 2; search_path_buffer = g_malloc (search_path_buffer_len); + /* And allocate a buffer which is 2 elements longer than @argv, so that if + * script_execute() has to be called later on, it can build a wrapper argv + * array in this buffer. */ + argv_buffer_len = g_strv_length (argv) + 2; + argv_buffer = g_new (gchar *, argv_buffer_len); + if (!g_unix_open_pipe (child_err_report_pipe, pipe_flags, error)) { g_free (search_path_buffer); + g_free (argv_buffer); return FALSE; } @@ -1912,6 +1926,8 @@ fork_exec_with_fds (gboolean intermediate_child, stderr_fd, working_directory, argv, + argv_buffer, + argv_buffer_len, envp, close_descriptors, chosen_search_path, @@ -1943,6 +1959,8 @@ fork_exec_with_fds (gboolean intermediate_child, stderr_fd, working_directory, argv, + argv_buffer, + argv_buffer_len, envp, close_descriptors, chosen_search_path, @@ -2076,6 +2094,7 @@ fork_exec_with_fds (gboolean intermediate_child, close_and_invalidate (&child_pid_report_pipe[0]); g_free (search_path_buffer); + g_free (argv_buffer); if (child_pid) *child_pid = pid; @@ -2110,6 +2129,7 @@ fork_exec_with_fds (gboolean intermediate_child, close_and_invalidate (&child_pid_report_pipe[1]); g_free (search_path_buffer); + g_free (argv_buffer); return FALSE; } @@ -2210,39 +2230,37 @@ cleanup_and_fail: /* This function is called between fork() and exec() and hence must be * async-signal-safe (see signal-safety(7)) until it calls exec(). */ -static void +static gboolean script_execute (const gchar *file, gchar **argv, + gchar **argv_buffer, + gsize argv_buffer_len, gchar **envp) { /* Count the arguments. */ int argc = 0; while (argv[argc]) ++argc; - - /* Construct an argument list for the shell. */ - { - gchar **new_argv; - /* FIXME: This is not async-signal-safe */ - new_argv = g_new0 (gchar*, argc + 2); /* /bin/sh and NULL */ - - new_argv[0] = (char *) "/bin/sh"; - new_argv[1] = (char *) file; - while (argc > 0) - { - new_argv[argc + 1] = argv[argc]; - --argc; - } + /* Construct an argument list for the shell. */ + if (argc + 2 > argv_buffer_len) + return FALSE; - /* Execute the shell. */ - if (envp) - execve (new_argv[0], new_argv, envp); - else - execv (new_argv[0], new_argv); - - g_free (new_argv); - } + argv_buffer[0] = (char *) "/bin/sh"; + argv_buffer[1] = (char *) file; + while (argc > 0) + { + argv_buffer[argc + 1] = argv[argc]; + --argc; + } + + /* Execute the shell. */ + if (envp) + execve (argv_buffer[0], argv_buffer, envp); + else + execv (argv_buffer[0], argv_buffer); + + return TRUE; } /* This function is called between fork() and exec() and hence must be @@ -2262,6 +2280,8 @@ my_strchrnul (const gchar *str, gchar c) static gint g_execute (const gchar *file, gchar **argv, + gchar **argv_buffer, + gsize argv_buffer_len, gchar **envp, const gchar *search_path, gchar *search_path_buffer, @@ -2282,8 +2302,12 @@ g_execute (const gchar *file, else execv (file, argv); - if (errno == ENOEXEC) - script_execute (file, argv, envp); + if (errno == ENOEXEC && + !script_execute (file, argv, argv_buffer, argv_buffer_len, envp)) + { + errno = ENOMEM; + return -1; + } } else { @@ -2332,8 +2356,12 @@ g_execute (const gchar *file, else execv (startp, argv); - if (errno == ENOEXEC) - script_execute (startp, argv, envp); + if (errno == ENOEXEC && + !script_execute (startp, argv, argv_buffer, argv_buffer_len, envp)) + { + errno = ENOMEM; + return -1; + } switch (errno) {