From 0e4d8c2478f37757574fae24d232a7975d48c911 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Wed, 6 Jul 2022 16:02:13 +0200 Subject: [PATCH 1/5] gbacktrace: Ensure return values of write, dup and fgets are used Make sure that we did not error when doing write or read operations, and in such case, just exit reporting the error. --- glib/gbacktrace.c | 89 ++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 76 insertions(+), 13 deletions(-) diff --git a/glib/gbacktrace.c b/glib/gbacktrace.c index cb02c8a1b..ae4bdec0c 100644 --- a/glib/gbacktrace.c +++ b/glib/gbacktrace.c @@ -45,6 +45,7 @@ #include #ifdef G_OS_UNIX +#include #include #include #ifdef HAVE_SYS_SELECT_H @@ -175,9 +176,14 @@ g_on_error_query (const gchar *prg_name) fflush (stdout); if (isatty(0) && isatty(1)) - fgets (buf, 8, stdin); + { + if (fgets (buf, 8, stdin) == NULL) + _exit (0); + } else - strcpy (buf, "E\n"); + { + strcpy (buf, "E\n"); + } if ((buf[0] == 'E' || buf[0] == 'e') && buf[1] == '\n') @@ -314,6 +320,59 @@ stack_trace_sigchld (int signum) #define BUFSIZE 1024 +static inline const char * +get_strerror (char *buffer, gsize n) +{ +#ifdef HAVE_STRERROR_R + return strerror_r (errno, buffer, n); +#else + const char *error_str = strerror (errno); + if (!error_str) + return NULL; + + strncpy (buffer, error_str, n); + return buffer; +#endif +} + +static gssize +checked_write (int fd, gconstpointer buf, gsize n) +{ + gssize written = write (fd, buf, n); + + if (written == -1) + { + char msg[BUFSIZE] = {0}; + char error_str[BUFSIZE / 2] = {0}; + + get_strerror (error_str, sizeof (error_str) - 1); + snprintf (msg, sizeof (msg) - 1, "Unable to write to fd %d: %s", fd, error_str); + perror (msg); + _exit (0); + } + + return written; +} + +static int +checked_dup (int fd) +{ + int new_fd = dup (fd); + + if (new_fd == -1) + { + char msg[BUFSIZE] = {0}; + char error_str[BUFSIZE / 2] = {0}; + + get_strerror (error_str, sizeof (error_str) - 1); + snprintf (msg, sizeof (msg) - 1, "Unable to duplicate fd %d: %s", fd, error_str); + perror (msg); + _exit (0); + } + + return new_fd; +} + static void stack_trace (const char * const *args) { @@ -351,9 +410,12 @@ stack_trace (const char * const *args) (void) fcntl (old_err, F_SETFD, getfd | FD_CLOEXEC); } - close (0); dup (in_fd[0]); /* set the stdin to the in pipe */ - close (1); dup (out_fd[1]); /* set the stdout to the out pipe */ - close (2); dup (out_fd[1]); /* set the stderr to the out pipe */ + close (0); + checked_dup (in_fd[0]); /* set the stdin to the in pipe */ + close (1); + checked_dup (out_fd[1]); /* set the stdout to the out pipe */ + close (2); + checked_dup (out_fd[1]); /* set the stderr to the out pipe */ execvp (args[0], (char **) args); /* exec gdb */ @@ -361,7 +423,8 @@ stack_trace (const char * const *args) if (old_err != -1) { close (2); - dup (old_err); + /* We can ignore the return value here as we're failing anyways */ + (void) !dup (old_err); } perror ("exec " DEBUGGER " failed"); _exit (0); @@ -376,14 +439,14 @@ stack_trace (const char * const *args) FD_SET (out_fd[0], &fdset); #ifdef USE_LLDB - write (in_fd[1], "bt\n", 3); - write (in_fd[1], "p x = 0\n", 8); - write (in_fd[1], "process detach\n", 15); - write (in_fd[1], "quit\n", 5); + checked_write (in_fd[1], "bt\n", 3); + checked_write (in_fd[1], "p x = 0\n", 8); + checked_write (in_fd[1], "process detach\n", 15); + checked_write (in_fd[1], "quit\n", 5); #else - write (in_fd[1], "backtrace\n", 10); - write (in_fd[1], "p x = 0\n", 8); - write (in_fd[1], "quit\n", 5); + checked_write (in_fd[1], "backtrace\n", 10); + checked_write (in_fd[1], "p x = 0\n", 8); + checked_write (in_fd[1], "quit\n", 5); #endif idx = 0; From a5390002fc0805fd6c3827e737d39c99979ba1b0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Wed, 6 Jul 2022 16:04:47 +0200 Subject: [PATCH 2/5] gio-tool-mount: Exit with error in case we can't read from stdin It's a fatal situation so we can just exit without caring much. --- gio/gio-tool-mount.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/gio/gio-tool-mount.c b/gio/gio-tool-mount.c index 7ab36c125..4d4a45092 100644 --- a/gio/gio-tool-mount.c +++ b/gio/gio-tool-mount.c @@ -112,7 +112,8 @@ prompt_for (const char *prompt, const char *default_value, gboolean echo) #endif - fgets(data, sizeof (data), stdin); + if (!fgets (data, sizeof (data), stdin)) + g_error ("Failed to read from standard input"); #ifdef HAVE_TERMIOS_H if (restore_flags) From 7a382438aa5a521473ecc740319d92d69e5f45ca Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Wed, 6 Jul 2022 16:06:12 +0200 Subject: [PATCH 3/5] glib/tests: Ensure that calls to write, system, symlink and pipe are checked Assert that calls to such system calls are returning the expected values --- glib/tests/fileutils.c | 12 ++++++------ glib/tests/io-channel-basic.c | 8 ++++---- glib/tests/mainloop.c | 4 ++-- glib/tests/spawn-multithreaded.c | 8 ++++---- glib/tests/unix.c | 2 +- 5 files changed, 17 insertions(+), 17 deletions(-) diff --git a/glib/tests/fileutils.c b/glib/tests/fileutils.c index 819700958..2f38a3282 100644 --- a/glib/tests/fileutils.c +++ b/glib/tests/fileutils.c @@ -1469,7 +1469,7 @@ test_file_test (void) fd = g_file_open_tmp (NULL, &name, &error); g_assert_no_error (error); - write (fd, "a", 1); + g_assert_cmpint (write (fd, "a", 1), ==, 1); g_assert_cmpint (g_fsync (fd), ==, 0); close (fd); @@ -1477,7 +1477,7 @@ test_file_test (void) result = g_file_test (name, G_FILE_TEST_IS_SYMLINK); g_assert_false (result); - symlink (name, "symlink"); + g_assert_no_errno (symlink (name, "symlink")); result = g_file_test ("symlink", G_FILE_TEST_IS_SYMLINK); g_assert_true (result); unlink ("symlink"); @@ -1500,7 +1500,7 @@ test_set_contents (void) fd = g_file_open_tmp (NULL, &name, &error); g_assert_no_error (error); - write (fd, "a", 1); + g_assert_cmpint (write (fd, "a", 1), ==, 1); g_assert_cmpint (g_fsync (fd), ==, 0); close (fd); @@ -1593,7 +1593,7 @@ test_set_contents_full (void) fd = g_file_open_tmp (NULL, &file_name, &error); g_assert_no_error (error); - write (fd, "a", 1); + g_assert_cmpint (write (fd, "a", 1), ==, 1); g_assert_no_errno (g_fsync (fd)); close (fd); @@ -1726,7 +1726,7 @@ test_set_contents_full_read_only_file (void) * existing file permissions. */ fd = g_file_open_tmp (NULL, &file_name, &error); g_assert_no_error (error); - write (fd, "a", 1); + g_assert_cmpint (write (fd, "a", 1), ==, 1); g_assert_no_errno (g_fsync (fd)); close (fd); g_assert_no_errno (g_chmod (file_name, 0400)); /* S_IREAD */ @@ -1800,7 +1800,7 @@ test_set_contents_full_read_only_directory (void) file_name = g_build_filename (dir_name, "file", NULL); fd = g_open (file_name, O_CREAT | O_RDWR, 0644); g_assert_cmpint (fd, >=, 0); - write (fd, "a", 1); + g_assert_cmpint (write (fd, "a", 1), ==, 1); g_assert_no_errno (g_fsync (fd)); close (fd); diff --git a/glib/tests/io-channel-basic.c b/glib/tests/io-channel-basic.c index 142e38bec..c1a46cd07 100644 --- a/glib/tests/io-channel-basic.c +++ b/glib/tests/io-channel-basic.c @@ -320,7 +320,7 @@ spawn_process (int children_nb) /* Spawn new Unix process */ cmdline = g_strdup_printf ("%s --child %d:%d &", exec_name, pipe_to_sub[0], pipe_from_sub[1]); - system (cmdline); + g_assert_no_errno (system (cmdline)); #endif g_free (cmdline); @@ -375,9 +375,9 @@ run_process (int argc, char *argv[]) buf[j] = ' ' + ((buflen + j) % 95); g_debug ("io-channel-basic: child writing %d+%d bytes to %d", (int) (sizeof (i) + sizeof (buflen)), buflen, writefd); - write (writefd, &i, sizeof (i)); - write (writefd, &buflen, sizeof (buflen)); - write (writefd, buf, buflen); + g_assert_cmpint (write (writefd, &i, sizeof (i)), ==, sizeof (i)); + g_assert_cmpint (write (writefd, &buflen, sizeof (buflen)), ==, sizeof (buflen)); + g_assert_cmpint (write (writefd, buf, buflen), ==, buflen); #ifdef G_OS_WIN32 if (i % 10 == 0) diff --git a/glib/tests/mainloop.c b/glib/tests/mainloop.c index fd3cac1b9..105bec87e 100644 --- a/glib/tests/mainloop.c +++ b/glib/tests/mainloop.c @@ -1467,8 +1467,8 @@ test_source_unix_fd_api (void) gint fds_a[2]; gint fds_b[2]; - pipe (fds_a); - pipe (fds_b); + g_assert_cmpint (pipe (fds_a), ==, 0); + g_assert_cmpint (pipe (fds_b), ==, 0); source_a = g_source_new (&no_funcs, sizeof (FlagSource)); source_b = g_source_new (&no_funcs, sizeof (FlagSource)); diff --git a/glib/tests/spawn-multithreaded.c b/glib/tests/spawn-multithreaded.c index 8dbc7bfbb..9e399b43d 100644 --- a/glib/tests/spawn-multithreaded.c +++ b/glib/tests/spawn-multithreaded.c @@ -157,9 +157,9 @@ test_spawn_childs (void) main_loop = g_main_loop_new (NULL, FALSE); #ifdef G_OS_WIN32 - system ("cd ."); + g_assert_no_errno (system ("cd .")); #else - system ("true"); + g_assert_no_errno (system ("true")); #endif n_alive = 2; @@ -200,9 +200,9 @@ test_spawn_childs_threads (void) main_loop = g_main_loop_new (NULL, FALSE); #ifdef G_OS_WIN32 - system ("cd ."); + g_assert_no_errno (system ("cd .")); #else - system ("true"); + g_assert_no_errno (system ("true")); #endif n_alive = 2; diff --git a/glib/tests/unix.c b/glib/tests/unix.c index 7639d066a..2112cab6b 100644 --- a/glib/tests/unix.c +++ b/glib/tests/unix.c @@ -40,7 +40,7 @@ test_pipe (void) g_assert (res); g_assert_no_error (error); - write (pipefd[1], "hello", sizeof ("hello")); + g_assert_cmpint (write (pipefd[1], "hello", sizeof ("hello")), ==, sizeof ("hello")); memset (buf, 0, sizeof (buf)); bytes_read = read (pipefd[0], buf, sizeof(buf) - 1); g_assert_cmpint (bytes_read, >, 0); From b61cd872f17fa1012508c9924e2be046e39cb5f1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Wed, 6 Jul 2022 16:07:43 +0200 Subject: [PATCH 4/5] build: Threat unused-result warnings as errors We don't have any in code now, so we should definitely ensure that we won't introduce anymore unguarded calls. --- meson.build | 1 + 1 file changed, 1 insertion(+) diff --git a/meson.build b/meson.build index 6f83a72cf..27600672b 100644 --- a/meson.build +++ b/meson.build @@ -473,6 +473,7 @@ if cc.get_id() == 'gcc' or cc.get_id() == 'clang' '-Werror=init-self', '-Werror=missing-include-dirs', '-Werror=pointer-arith', + '-Werror=unused-result', ] warning_c_args = warning_common_args + [ From 8c5dac1eb225325498a8c054cf1ef004885926a5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Wed, 6 Jul 2022 17:28:31 +0200 Subject: [PATCH 5/5] glib/tests/fileutils: use g_assert_no_errno instead of manual checking --- glib/tests/fileutils.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/glib/tests/fileutils.c b/glib/tests/fileutils.c index 2f38a3282..247bcef1c 100644 --- a/glib/tests/fileutils.c +++ b/glib/tests/fileutils.c @@ -1895,8 +1895,8 @@ test_read_link (void) g_assert_nonnull (file); fclose (file); - g_assert_cmpint (symlink (filename, link1), ==, 0); - g_assert_cmpint (symlink (link1, link2), ==, 0); + g_assert_no_errno (symlink (filename, link1)); + g_assert_no_errno (symlink (link1, link2)); error = NULL; data = g_file_read_link (link1, &error);