From aae98ce774e4c22e80474e0a6f1d231792f47849 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Tue, 23 Jun 2020 10:49:44 +0100 Subject: [PATCH 1/9] tree: Fix various ableist language In almost all cases, rewording the documentation/comments made things more specific and a little clearer. Signed-off-by: Philip Withnall See: https://gitlab.gnome.org/GNOME/glib/-/merge_requests/1544#note_846645 --- gio/glocalfile.c | 2 +- gio/gmenumodel.c | 4 ++-- glib/gcharset.c | 2 +- glib/gconstructor.h | 2 +- glib/gdate.c | 12 +++++------ glib/gdate.h | 2 +- glib/gdatetime.c | 5 ++--- glib/gspawn.c | 36 ++++++++++++++++----------------- glib/gtestutils.c | 4 ++-- glib/gutils.c | 2 +- glib/gvarianttype.c | 2 +- glib/tests/regex.c | 2 +- glib/tests/spawn-singlethread.c | 14 ++++++------- gobject/gobject.c | 2 +- gobject/gsignal.c | 10 ++++----- gobject/gvalue.c | 4 ++-- 16 files changed, 52 insertions(+), 53 deletions(-) diff --git a/gio/glocalfile.c b/gio/glocalfile.c index 72ef947d4..5d01a4baf 100644 --- a/gio/glocalfile.c +++ b/gio/glocalfile.c @@ -1511,7 +1511,7 @@ g_local_file_delete (GFile *file, { int errsv = errno; - /* Posix allows EEXIST too, but the more sane error + /* Posix allows EEXIST too, but the clearer error is G_IO_ERROR_NOT_FOUND, and it's what nautilus expects */ if (errsv == EEXIST) diff --git a/gio/gmenumodel.c b/gio/gmenumodel.c index 8ca45e098..b3f3fb7d9 100644 --- a/gio/gmenumodel.c +++ b/gio/gmenumodel.c @@ -309,7 +309,7 @@ g_menu_model_real_iterate_item_attributes (GMenuModel *model, else { g_critical ("GMenuModel implementation '%s' doesn't override iterate_item_attributes() " - "and fails to return sane values from get_item_attributes()", + "and fails to return valid values from get_item_attributes()", G_OBJECT_TYPE_NAME (model)); result = NULL; } @@ -373,7 +373,7 @@ g_menu_model_real_iterate_item_links (GMenuModel *model, else { g_critical ("GMenuModel implementation '%s' doesn't override iterate_item_links() " - "and fails to return sane values from get_item_links()", + "and fails to return valid values from get_item_links()", G_OBJECT_TYPE_NAME (model)); result = NULL; } diff --git a/glib/gcharset.c b/glib/gcharset.c index 9db56f732..bb775bda4 100644 --- a/glib/gcharset.c +++ b/glib/gcharset.c @@ -315,7 +315,7 @@ g_get_console_charset (const char **charset) g_free (emsg); } } - /* fall-back to UTF-8 if the rest failed (it's a sane and universal default) */ + /* fall-back to UTF-8 if the rest failed (it's a universal default) */ if (raw == NULL) raw = "UTF-8"; diff --git a/glib/gconstructor.h b/glib/gconstructor.h index 603c2dde6..9509e595f 100644 --- a/glib/gconstructor.h +++ b/glib/gconstructor.h @@ -1,6 +1,6 @@ /* If G_HAS_CONSTRUCTORS is true then the compiler support *both* constructors and - destructors, in a sane way, including e.g. on library unload. If not you're on + destructors, in a usable way, including e.g. on library unload. If not you're on your own. Some compilers need #pragma to handle this, which does not work with macros, diff --git a/glib/gdate.c b/glib/gdate.c index b867ba9db..ec7b95bef 100644 --- a/glib/gdate.c +++ b/glib/gdate.c @@ -83,8 +83,8 @@ * * #GDate is simple to use. First you need a "blank" date; you can get a * dynamically allocated date from g_date_new(), or you can declare an - * automatic variable or array and initialize it to a sane state by - * calling g_date_clear(). A cleared date is sane; it's safe to call + * automatic variable or array and initialize it by + * calling g_date_clear(). A cleared date is safe; it's safe to call * g_date_set_dmy() and the other mutator functions to initialize the * value of a cleared date. However, a cleared date is initially * invalid, meaning that it doesn't represent a day that exists. @@ -146,7 +146,7 @@ * * If it's declared on the stack, it will contain garbage so must be * initialized with g_date_clear(). g_date_clear() makes the date invalid - * but sane. An invalid date doesn't represent a day, it's "empty." A date + * but safe. An invalid date doesn't represent a day, it's "empty." A date * becomes valid after you set it to a Julian day or you set a day, month, * and year. */ @@ -259,7 +259,7 @@ * g_date_new: * * Allocates a #GDate and initializes - * it to a sane state. The new date will + * it to a safe state. The new date will * be cleared (as if you'd called g_date_clear()) but invalid (it won't * represent an existing day). Free the return value with g_date_free(). * @@ -862,7 +862,7 @@ g_date_days_between (const GDate *d1, * @date: pointer to one or more dates to clear * @n_dates: number of dates to clear * - * Initializes one or more #GDate structs to a sane but invalid + * Initializes one or more #GDate structs to a safe but invalid * state. The cleared dates will not represent an existing date, but will * not contain garbage. Useful to init a date declared on the stack. * Validity can be tested with g_date_valid(). @@ -2055,7 +2055,7 @@ g_date_compare (const GDate *lhs, * @tm: (not nullable): struct tm to fill * * Fills in the date-related bits of a struct tm using the @date value. - * Initializes the non-date parts with something sane but meaningless. + * Initializes the non-date parts with something safe but meaningless. */ void g_date_to_struct_tm (const GDate *d, diff --git a/glib/gdate.h b/glib/gdate.h index 3bc07bf5c..65fe811fa 100644 --- a/glib/gdate.h +++ b/glib/gdate.h @@ -178,7 +178,7 @@ GLIB_AVAILABLE_IN_ALL guint g_date_get_iso8601_week_of_year (const GDate *date); /* If you create a static date struct you need to clear it to get it - * in a sane state before use. You can clear a whole array at + * in a safe state before use. You can clear a whole array at * once with the ndates argument. */ GLIB_AVAILABLE_IN_ALL diff --git a/glib/gdatetime.c b/glib/gdatetime.c index e85a00f56..f93d7d88c 100644 --- a/glib/gdatetime.c +++ b/glib/gdatetime.c @@ -935,9 +935,8 @@ g_date_time_new_from_unix (GTimeZone *tz, * time zone @tz. The time is as accurate as the system allows, to a * maximum accuracy of 1 microsecond. * - * This function will always succeed unless the system clock is set to - * truly insane values (or unless GLib is still being used after the - * year 9999). + * This function will always succeed unless GLib is still being used after the + * year 9999. * * You should release the return value by calling g_date_time_unref() * when you are done with it. diff --git a/glib/gspawn.c b/glib/gspawn.c index 85813ef00..fb7d39d2e 100644 --- a/glib/gspawn.c +++ b/glib/gspawn.c @@ -1126,7 +1126,7 @@ set_cloexec (void *data, gint fd) } static gint -sane_close (gint fd) +safe_close (gint fd) { gint ret; @@ -1141,7 +1141,7 @@ G_GNUC_UNUSED static int close_func (void *data, int fd) { if (fd >= GPOINTER_TO_INT (data)) - (void) sane_close (fd); + (void) safe_close (fd); return 0; } @@ -1232,7 +1232,7 @@ safe_fdwalk (int (*cb)(void *data, int fd), void *data) } } - sane_close (dir_fd); + safe_close (dir_fd); return res; } @@ -1290,7 +1290,7 @@ safe_closefrom (int lowfd) } static gint -sane_dup2 (gint fd1, gint fd2) +safe_dup2 (gint fd1, gint fd2) { gint ret; @@ -1302,7 +1302,7 @@ sane_dup2 (gint fd1, gint fd2) } static gint -sane_open (const char *path, gint mode) +safe_open (const char *path, gint mode) { gint ret; @@ -1349,7 +1349,7 @@ do_exec (gint child_err_report_fd, { /* dup2 can't actually fail here I don't think */ - if (sane_dup2 (stdin_fd, 0) < 0) + if (safe_dup2 (stdin_fd, 0) < 0) write_err_and_exit (child_err_report_fd, CHILD_DUP2_FAILED); @@ -1358,9 +1358,9 @@ do_exec (gint child_err_report_fd, else if (!child_inherits_stdin) { /* Keep process from blocking on a read of stdin */ - gint read_null = sane_open ("/dev/null", O_RDONLY); + gint read_null = safe_open ("/dev/null", O_RDONLY); g_assert (read_null != -1); - sane_dup2 (read_null, 0); + safe_dup2 (read_null, 0); close_and_invalidate (&read_null); } @@ -1368,7 +1368,7 @@ do_exec (gint child_err_report_fd, { /* dup2 can't actually fail here I don't think */ - if (sane_dup2 (stdout_fd, 1) < 0) + if (safe_dup2 (stdout_fd, 1) < 0) write_err_and_exit (child_err_report_fd, CHILD_DUP2_FAILED); @@ -1376,9 +1376,9 @@ do_exec (gint child_err_report_fd, } else if (stdout_to_null) { - gint write_null = sane_open ("/dev/null", O_WRONLY); + gint write_null = safe_open ("/dev/null", O_WRONLY); g_assert (write_null != -1); - sane_dup2 (write_null, 1); + safe_dup2 (write_null, 1); close_and_invalidate (&write_null); } @@ -1386,7 +1386,7 @@ do_exec (gint child_err_report_fd, { /* dup2 can't actually fail here I don't think */ - if (sane_dup2 (stderr_fd, 2) < 0) + if (safe_dup2 (stderr_fd, 2) < 0) write_err_and_exit (child_err_report_fd, CHILD_DUP2_FAILED); @@ -1394,8 +1394,8 @@ do_exec (gint child_err_report_fd, } else if (stderr_to_null) { - gint write_null = sane_open ("/dev/null", O_WRONLY); - sane_dup2 (write_null, 2); + gint write_null = safe_open ("/dev/null", O_WRONLY); + safe_dup2 (write_null, 2); close_and_invalidate (&write_null); } @@ -1408,7 +1408,7 @@ do_exec (gint child_err_report_fd, { if (child_setup == NULL) { - sane_dup2 (child_err_report_fd, 3); + safe_dup2 (child_err_report_fd, 3); set_cloexec (GINT_TO_POINTER (0), 3); safe_closefrom (4); child_err_report_fd = 3; @@ -1565,7 +1565,7 @@ do_posix_spawn (gchar **argv, else if (!child_inherits_stdin) { /* Keep process from blocking on a read of stdin */ - gint read_null = sane_open ("/dev/null", O_RDONLY | O_CLOEXEC); + gint read_null = safe_open ("/dev/null", O_RDONLY | O_CLOEXEC); g_assert (read_null != -1); parent_close_fds[num_parent_close_fds++] = read_null; @@ -1589,7 +1589,7 @@ do_posix_spawn (gchar **argv, } else if (stdout_to_null) { - gint write_null = sane_open ("/dev/null", O_WRONLY | O_CLOEXEC); + gint write_null = safe_open ("/dev/null", O_WRONLY | O_CLOEXEC); g_assert (write_null != -1); parent_close_fds[num_parent_close_fds++] = write_null; @@ -1613,7 +1613,7 @@ do_posix_spawn (gchar **argv, } else if (stderr_to_null) { - gint write_null = sane_open ("/dev/null", O_WRONLY | O_CLOEXEC); + gint write_null = safe_open ("/dev/null", O_WRONLY | O_CLOEXEC); g_assert (write_null != -1); parent_close_fds[num_parent_close_fds++] = write_null; diff --git a/glib/gtestutils.c b/glib/gtestutils.c index 1010dcc1b..18b117285 100644 --- a/glib/gtestutils.c +++ b/glib/gtestutils.c @@ -3067,7 +3067,7 @@ test_trap_clear (void) #ifdef G_OS_UNIX static int -sane_dup2 (int fd1, +safe_dup2 (int fd1, int fd2) { int ret; @@ -3319,7 +3319,7 @@ g_test_trap_fork (guint64 usec_timeout, if (fd0 < 0) g_error ("failed to open /dev/null for stdin redirection"); } - if (sane_dup2 (stdout_pipe[1], 1) < 0 || sane_dup2 (stderr_pipe[1], 2) < 0 || (fd0 >= 0 && sane_dup2 (fd0, 0) < 0)) + if (safe_dup2 (stdout_pipe[1], 1) < 0 || safe_dup2 (stderr_pipe[1], 2) < 0 || (fd0 >= 0 && safe_dup2 (fd0, 0) < 0)) { errsv = errno; g_error ("failed to dup2() in forked test program: %s", g_strerror (errsv)); diff --git a/glib/gutils.c b/glib/gutils.c index 572c06ce6..c5aef47fa 100644 --- a/glib/gutils.c +++ b/glib/gutils.c @@ -3102,7 +3102,7 @@ g_abort (void) { /* One call to break the debugger */ DebugBreak (); - /* One call in case CRT does get saner about abort() behaviour */ + /* One call in case CRT changes its abort() behaviour */ abort (); /* And one call to bind them all and terminate the program for sure */ ExitProcess (127); diff --git a/glib/gvarianttype.c b/glib/gvarianttype.c index 1a228f73b..c46f1a2c6 100644 --- a/glib/gvarianttype.c +++ b/glib/gvarianttype.c @@ -1120,7 +1120,7 @@ g_variant_type_new_tuple_slow (const GVariantType * const *items, { /* the "slow" version is needed in case the static buffer of 1024 * bytes is exceeded when running the normal version. this will - * happen only in truly insane code, so it can be slow. + * happen only with very unusually large types, so it can be slow. */ GString *string; gint i; diff --git a/glib/tests/regex.c b/glib/tests/regex.c index 56bd2d5eb..ee9cd21ca 100644 --- a/glib/tests/regex.c +++ b/glib/tests/regex.c @@ -2321,7 +2321,7 @@ main (int argc, char *argv[]) TEST_NEW_FAIL ("(*:0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEFG)XX", 0, G_REGEX_ERROR_NAME_TOO_LONG); TEST_NEW_FAIL ("\\u0100", G_REGEX_RAW | G_REGEX_JAVASCRIPT_COMPAT, G_REGEX_ERROR_CHARACTER_VALUE_TOO_LARGE); - /* These errors can't really be tested sanely: + /* These errors can't really be tested easily: * G_REGEX_ERROR_EXPRESSION_TOO_LARGE * G_REGEX_ERROR_MEMORY_ERROR * G_REGEX_ERROR_SUBPATTERN_NAME_TOO_LONG diff --git a/glib/tests/spawn-singlethread.c b/glib/tests/spawn-singlethread.c index 15f03a0c7..ab449eb95 100644 --- a/glib/tests/spawn-singlethread.c +++ b/glib/tests/spawn-singlethread.c @@ -166,7 +166,7 @@ test_spawn_async (void) * Routine if the file descriptor does not exist. */ static void -sane_close (int fd) +safe_close (int fd) { if (fd >= 0) close (fd); @@ -252,10 +252,10 @@ test_spawn_async_with_fds (void) test_pipe[0][0], test_pipe[1][1], test_pipe[2][1], &error); g_assert_no_error (error); - sane_close (test_pipe[0][0]); - sane_close (test_pipe[1][1]); + safe_close (test_pipe[0][0]); + safe_close (test_pipe[1][1]); if (fd_info[2] != STDOUT_PIPE) - sane_close (test_pipe[2][1]); + safe_close (test_pipe[2][1]); data.loop = loop; data.stdout_done = FALSE; @@ -297,10 +297,10 @@ test_spawn_async_with_fds (void) g_main_context_unref (context); g_main_loop_unref (loop); - sane_close (test_pipe[0][1]); - sane_close (test_pipe[1][0]); + safe_close (test_pipe[0][1]); + safe_close (test_pipe[1][0]); if (fd_info[2] != STDOUT_PIPE) - sane_close (test_pipe[2][0]); + safe_close (test_pipe[2][0]); } g_ptr_array_free (argv, TRUE); diff --git a/gobject/gobject.c b/gobject/gobject.c index eea40b3ae..07752cf25 100644 --- a/gobject/gobject.c +++ b/gobject/gobject.c @@ -1617,7 +1617,7 @@ object_interface_check_properties (gpointer check_data, /* We do a number of checks on the properties of an interface to * make sure that all classes implementing the interface are - * overriding the properties in a sane way. + * overriding the properties correctly. * * We do the checks in order of importance so that we can give * more useful error messages first. diff --git a/gobject/gsignal.c b/gobject/gsignal.c index 2b6b0d05b..45effa92d 100644 --- a/gobject/gsignal.c +++ b/gobject/gsignal.c @@ -2291,7 +2291,7 @@ g_signal_chain_from_overridden_handler (gpointer instance, g_free (error); /* we purposely leak the value here, it might not be - * in a sane state if an error condition occurred + * in a correct state if an error condition occurred */ while (i--) g_value_unset (param_values + i); @@ -2347,7 +2347,7 @@ g_signal_chain_from_overridden_handler (gpointer instance, g_free (error); /* we purposely leak the value here, it might not be - * in a sane state if an error condition occurred + * in a correct state if an error condition occurred */ } } @@ -3446,7 +3446,7 @@ g_signal_emit_valist (gpointer instance, g_warning ("%s: %s", G_STRLOC, error); g_free (error); /* we purposely leak the value here, it might not be - * in a sane state if an error condition occurred + * in a correct state if an error condition occurred */ } } @@ -3483,7 +3483,7 @@ g_signal_emit_valist (gpointer instance, g_free (error); /* we purposely leak the value here, it might not be - * in a sane state if an error condition occurred + * in a correct state if an error condition occurred */ while (i--) g_value_unset (param_values + i); @@ -3519,7 +3519,7 @@ g_signal_emit_valist (gpointer instance, g_free (error); /* we purposely leak the value here, it might not be - * in a sane state if an error condition occurred + * in a correct state if an error condition occurred */ } } diff --git a/gobject/gvalue.c b/gobject/gvalue.c index c30501a6b..468da2e7d 100644 --- a/gobject/gvalue.c +++ b/gobject/gvalue.c @@ -376,7 +376,7 @@ g_value_set_instance (GValue *value, g_free (error_msg); /* we purposely leak the value here, it might not be - * in a sane state if an error condition occurred + * in a correct state if an error condition occurred */ value_meminit (value, g_type); value_table->value_init (value); @@ -440,7 +440,7 @@ g_value_init_from_instance (GValue *value, g_free (error_msg); /* we purposely leak the value here, it might not be - * in a sane state if an error condition occurred + * in a correct state if an error condition occurred */ value_meminit (value, g_type); value_table->value_init (value); From 92eac2f4247c3d1a3c5aa57a44f46a346c2bd3aa Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Mon, 22 Jun 2020 12:36:32 +0100 Subject: [PATCH 2/9] 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 fb7d39d2e..9fccefb46 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 539d51836a279affcce50f3c0b6598ee507a71b7 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Mon, 22 Jun 2020 13:09:56 +0100 Subject: [PATCH 3/9] =?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 9fccefb46..6f2789d62 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 d24acc7ea5e6445a84c43b811ff2ebcd160c3264 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Mon, 22 Jun 2020 13:10:35 +0100 Subject: [PATCH 4/9] =?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 6f2789d62..a2fd58005 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 923315ef823bafbce2bce98cc599d7c39798efcf Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Mon, 22 Jun 2020 13:11:32 +0100 Subject: [PATCH 5/9] =?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 a2fd58005..8dbad9e56 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 45b8e60ac4f8cf9f38699cc9d4f81d5cb4afd00e Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Mon, 22 Jun 2020 13:40:27 +0100 Subject: [PATCH 6/9] =?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 8dbad9e56..eeb6c3e14 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 3548996c0ee6de35ac630dc7eb4f2e2b47bb2224 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Mon, 22 Jun 2020 13:59:48 +0100 Subject: [PATCH 7/9] =?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 eeb6c3e14..83420042a 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 cc8715c3b740bdac16d71ae110319c2a25a7d1d6 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Mon, 22 Jun 2020 14:15:42 +0100 Subject: [PATCH 8/9] =?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 83420042a..b11edf36f 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; @@ -2339,7 +2372,6 @@ g_execute (const gchar *file, * something went wrong executing it; return the error to our * caller. */ - g_free (freeme); return -1; } } @@ -2351,8 +2383,6 @@ g_execute (const gchar *file, * error. */ errno = EACCES; - - g_free (freeme); } /* Return the error from the last attempt (probably ENOENT). */ From f99f0c2fabafcf33bc7df6d8f4171c6d6b9220b3 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Mon, 22 Jun 2020 14:33:12 +0100 Subject: [PATCH 9/9] =?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 b11edf36f..e8141ad94 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) {