From 6fd103736109459f8f9a6e55b8a936afd9f989d9 Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Tue, 30 May 2023 16:04:05 +0100 Subject: [PATCH 1/5] gspawn: Use g_clear_fd() instead of reinventing it g_clear_fd() is documented to be async-signal safe whenever the fd is either negative or valid (which it should be here) and the error is NULL (which it always is here). Signed-off-by: Simon McVittie --- glib/gspawn.c | 88 +++++++++++++++++++-------------------------------- 1 file changed, 33 insertions(+), 55 deletions(-) diff --git a/glib/gspawn.c b/glib/gspawn.c index 2f7465b78..4d0f1e274 100644 --- a/glib/gspawn.c +++ b/glib/gspawn.c @@ -263,23 +263,6 @@ g_spawn_async (const gchar *working_directory, error); } -/* 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) -{ - if (*fd < 0) - return; - - g_close (*fd, NULL); - *fd = -1; -} - /* Some versions of OS X define READ_OK in public headers */ #undef READ_OK @@ -484,8 +467,7 @@ g_spawn_sync (const gchar *working_directory, failed = TRUE; break; case READ_EOF: - close_and_invalidate (&outpipe); - outpipe = -1; + g_clear_fd (&outpipe, NULL); break; default: break; @@ -503,8 +485,7 @@ g_spawn_sync (const gchar *working_directory, failed = TRUE; break; case READ_EOF: - close_and_invalidate (&errpipe); - errpipe = -1; + g_clear_fd (&errpipe, NULL); break; default: break; @@ -516,12 +497,9 @@ g_spawn_sync (const gchar *working_directory, } /* These should only be open still if we had an error. */ - - if (outpipe >= 0) - close_and_invalidate (&outpipe); - if (errpipe >= 0) - close_and_invalidate (&errpipe); - + g_clear_fd (&outpipe, NULL); + g_clear_fd (&errpipe, NULL); + /* Wait for child to exit, even if we have * an error pending. */ @@ -1822,7 +1800,7 @@ do_exec (gint child_err_report_fd, if (safe_dup2 (read_null, 0) < 0) write_err_and_exit (child_err_report_fd, CHILD_DUPFD_FAILED); - close_and_invalidate (&read_null); + g_clear_fd (&read_null, NULL); } /* Like with stdin above, it's possible the caller assigned @@ -1859,7 +1837,7 @@ do_exec (gint child_err_report_fd, if (safe_dup2 (write_null, 1) < 0) write_err_and_exit (child_err_report_fd, CHILD_DUPFD_FAILED); - close_and_invalidate (&write_null); + g_clear_fd (&write_null, NULL); } if (IS_STD_FILENO (stderr_fd) && stderr_fd != STDERR_FILENO) @@ -1891,7 +1869,7 @@ do_exec (gint child_err_report_fd, if (safe_dup2 (write_null, 2) < 0) write_err_and_exit (child_err_report_fd, CHILD_DUPFD_FAILED); - close_and_invalidate (&write_null); + g_clear_fd (&write_null, NULL); } /* Close all file descriptors but stdin, stdout and stderr, and any of source_fds, @@ -1982,7 +1960,7 @@ do_exec (gint child_err_report_fd, if (safe_dup2 (source_fds[i], target_fds[i]) < 0) write_err_and_exit (child_err_report_fd, CHILD_DUPFD_FAILED); - close_and_invalidate (&source_fds[i]); + g_clear_fd (&source_fds[i], NULL); } } } @@ -2272,12 +2250,12 @@ do_posix_spawn (const gchar * const *argv, out_close_fds: for (i = 0; i < num_parent_close_fds; i++) - close_and_invalidate (&parent_close_fds [i]); + g_clear_fd (&parent_close_fds[i], NULL); if (duped_source_fds != NULL) { for (i = 0; i < n_fds; i++) - close_and_invalidate (&duped_source_fds[i]); + g_clear_fd (&duped_source_fds[i], NULL); g_free (duped_source_fds); } @@ -2560,13 +2538,13 @@ fork_exec (gboolean intermediate_child, * not needed in the close_descriptors case, * though */ - close_and_invalidate (&child_err_report_pipe[0]); - close_and_invalidate (&child_pid_report_pipe[0]); + g_clear_fd (&child_err_report_pipe[0], NULL); + g_clear_fd (&child_pid_report_pipe[0], NULL); if (child_close_fds[0] != -1) { int i = -1; while (child_close_fds[++i] != -1) - close_and_invalidate (&child_close_fds[i]); + g_clear_fd (&child_close_fds[i], NULL); } if (intermediate_child) @@ -2591,7 +2569,7 @@ fork_exec (gboolean intermediate_child, } else if (grandchild_pid == 0) { - close_and_invalidate (&child_pid_report_pipe[1]); + g_clear_fd (&child_pid_report_pipe[1], NULL); do_exec (child_err_report_pipe[1], stdin_fd, stdout_fd, @@ -2618,7 +2596,7 @@ fork_exec (gboolean intermediate_child, else { write_all (child_pid_report_pipe[1], &grandchild_pid, sizeof(grandchild_pid)); - close_and_invalidate (&child_pid_report_pipe[1]); + g_clear_fd (&child_pid_report_pipe[1], NULL); _exit (0); } @@ -2660,8 +2638,8 @@ fork_exec (gboolean intermediate_child, gint n_ints = 0; /* Close the uncared-about ends of the pipes */ - close_and_invalidate (&child_err_report_pipe[1]); - close_and_invalidate (&child_pid_report_pipe[1]); + g_clear_fd (&child_err_report_pipe[1], NULL); + g_clear_fd (&child_pid_report_pipe[1], NULL); /* If we had an intermediate child, reap it */ if (intermediate_child) @@ -2783,8 +2761,8 @@ fork_exec (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_clear_fd (&child_err_report_pipe[0], NULL); + g_clear_fd (&child_pid_report_pipe[0], NULL); g_free (search_path_buffer_heap); g_free (argv_buffer_heap); @@ -2798,9 +2776,9 @@ fork_exec (gboolean intermediate_child, success: /* Close the uncared-about ends of the pipes */ - close_and_invalidate (&stdin_pipe[0]); - close_and_invalidate (&stdout_pipe[1]); - close_and_invalidate (&stderr_pipe[1]); + g_clear_fd (&stdin_pipe[0], NULL); + g_clear_fd (&stdout_pipe[1], NULL); + g_clear_fd (&stderr_pipe[1], NULL); if (stdin_pipe_out != NULL) *stdin_pipe_out = g_steal_fd (&stdin_pipe[1]); @@ -2833,17 +2811,17 @@ success: } } - close_and_invalidate (&stdin_pipe[0]); - close_and_invalidate (&stdin_pipe[1]); - close_and_invalidate (&stdout_pipe[0]); - close_and_invalidate (&stdout_pipe[1]); - close_and_invalidate (&stderr_pipe[0]); - close_and_invalidate (&stderr_pipe[1]); + g_clear_fd (&stdin_pipe[0], NULL); + g_clear_fd (&stdin_pipe[1], NULL); + g_clear_fd (&stdout_pipe[0], NULL); + g_clear_fd (&stdout_pipe[1], NULL); + g_clear_fd (&stderr_pipe[0], NULL); + g_clear_fd (&stderr_pipe[1], NULL); - close_and_invalidate (&child_err_report_pipe[0]); - close_and_invalidate (&child_err_report_pipe[1]); - close_and_invalidate (&child_pid_report_pipe[0]); - close_and_invalidate (&child_pid_report_pipe[1]); + g_clear_fd (&child_err_report_pipe[0], NULL); + g_clear_fd (&child_err_report_pipe[1], NULL); + g_clear_fd (&child_pid_report_pipe[0], NULL); + g_clear_fd (&child_pid_report_pipe[1], NULL); g_clear_pointer (&search_path_buffer_heap, g_free); g_clear_pointer (&argv_buffer_heap, g_free); From f31db7d3703cad90d22127917da3684958edf548 Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Tue, 30 May 2023 16:06:33 +0100 Subject: [PATCH 2/5] glib-unix: Add convenience API for pipes We can't easily use g_autofd with g_unix_open_pipe, because its parameter is an array of two fds that both need closing. Add an inline convenience wrapper providing the obvious semantics. Signed-off-by: Simon McVittie --- docs/reference/glib/glib-sections.txt.in | 10 ++ glib/glib-unix.c | 4 + glib/glib-unix.h | 208 +++++++++++++++++++++++ 3 files changed, 222 insertions(+) diff --git a/docs/reference/glib/glib-sections.txt.in b/docs/reference/glib/glib-sections.txt.in index 6373a13f2..a3fe735ae 100644 --- a/docs/reference/glib/glib-sections.txt.in +++ b/docs/reference/glib/glib-sections.txt.in @@ -2296,6 +2296,16 @@ G_UNIX_ERROR g_unix_open_pipe g_unix_set_fd_nonblocking + +GUnixPipe +G_UNIX_PIPE_INIT +GUnixPipeEnd +g_unix_pipe_open +g_unix_pipe_get +g_unix_pipe_steal +g_unix_pipe_close +g_unix_pipe_clear + g_unix_signal_add g_unix_signal_add_full diff --git a/glib/glib-unix.c b/glib/glib-unix.c index df5726cf0..1fd06eff0 100644 --- a/glib/glib-unix.c +++ b/glib/glib-unix.c @@ -1,5 +1,6 @@ /* GLIB - Library of useful routines for C programming * Copyright (C) 2011 Red Hat, Inc. + * Copyright 2023 Collabora Ltd. * * glib-unix.c: UNIX specific API wrappers and convenience functions * @@ -87,6 +88,9 @@ g_unix_set_error_from_errno (GError **error, * you wanted to configure `O_NONBLOCK` then that had to be done separately with * `fcntl()`. * + * Since GLib 2.80, the constants %G_UNIX_PIPE_END_READ and + * %G_UNIX_PIPE_END_WRITE can be used as mnemonic indexes in @fds. + * * It is a programmer error to call this function with unsupported flags, and a * critical warning will be raised. * diff --git a/glib/glib-unix.h b/glib/glib-unix.h index 7cf4f0dc8..ad8393a84 100644 --- a/glib/glib-unix.h +++ b/glib/glib-unix.h @@ -1,5 +1,6 @@ /* glib-unix.h - Unix specific integration * Copyright (C) 2011 Red Hat, Inc. + * Copyright 2023 Collabora Ltd. * * SPDX-License-Identifier: LGPL-2.1-or-later * @@ -33,6 +34,7 @@ #include #include +#include #ifndef G_OS_UNIX #error "This header may only be used on UNIX" @@ -120,6 +122,212 @@ GLIB_AVAILABLE_IN_2_64 struct passwd *g_unix_get_passwd_entry (const gchar *user_name, GError **error); +/** + * GUnixPipe: + * @fds: A pair of file descriptors, each negative if closed or not yet opened. + * The file descriptor with index %G_UNIX_PIPE_END_READ is readable. + * The file descriptor with index %G_UNIX_PIPE_END_WRITE is writable. + * + * A Unix pipe. The advantage of this type over `int[2]` is that it can + * be closed automatically when it goes out of scope, using `g_auto(GUnixPipe)`, + * on compilers that support that feature. + * + * Since: 2.80 + */ +GLIB_AVAILABLE_TYPE_IN_2_80 +typedef struct { + int fds[2]; +} GUnixPipe; + +/** + * GUnixPipeEnd: + * @G_UNIX_PIPE_END_READ: The readable file descriptor 0 + * @G_UNIX_PIPE_END_WRITE: The writable file descriptor 1 + * + * Mnemonic constants for the ends of a Unix pipe. + * + * Since: 2.80 + */ +GLIB_AVAILABLE_TYPE_IN_2_80 +typedef enum +{ + G_UNIX_PIPE_END_READ = 0, + G_UNIX_PIPE_END_WRITE = 1 +} GUnixPipeEnd; + +/** + * G_UNIX_PIPE_INIT: + * + * Initializer for a #GUnixPipe that has not yet been opened. + * Both of its file descriptors are initialized to `-1` (invalid), + * the same as if they had been closed. + * + * Since: 2.80 + */ +#define G_UNIX_PIPE_INIT { { -1, -1 } } GLIB_AVAILABLE_MACRO_IN_2_80 + +/* Suppress "Not available before" warnings when declaring the + * implementations */ +G_GNUC_BEGIN_IGNORE_DEPRECATIONS + +/** + * g_unix_pipe_open: + * @self: A pair of file descriptors + * @flags: Flags to pass to g_unix_open_pipe(), typically `FD_CLOEXEC` + * @error: Used to report an error on failure + * + * Open a pipe. This is the same as g_unix_open_pipe(), but uses the + * #GUnixPipe data structure. + * + * Returns: %TRUE on success + * + * Since: 2.80 + */ +GLIB_AVAILABLE_STATIC_INLINE_IN_2_80 +static inline gboolean +g_unix_pipe_open (GUnixPipe *self, + int flags, + GError **error) +{ + return g_unix_open_pipe (self->fds, flags, error); +} + +/** + * g_unix_pipe_get: + * @self: A pair of file descriptors + * @end: One of the ends of the pipe + * + * Return one of the ends of the pipe. It remains owned by @self. + * + * This function is async-signal safe (see [`signal(7)`](man:signal(7)) and + * [`signal-safety(7)`](man:signal-safety(7))), making it safe to call from a + * signal handler or a #GSpawnChildSetupFunc. + * + * This function preserves the value of `errno`. + * + * Returns: a non-negative file descriptor owned by @self, which must not + * be closed by the caller, or a negative number if the corresponding + * end of the pipe was already closed or stolen + * + * Since: 2.80 + */ +GLIB_AVAILABLE_STATIC_INLINE_IN_2_80 +static inline int +g_unix_pipe_get (GUnixPipe *self, + GUnixPipeEnd end) +{ + return self->fds[end]; +} + +/** + * g_unix_pipe_steal: + * @self: A pair of file descriptors + * @end: One of the ends of the pipe + * + * Return one of the ends of the pipe. It becomes owned by the caller, + * and the file descriptor in the data structure is set to `-1`, + * similar to g_steal_fd(). + * + * This function is async-signal safe (see [`signal(7)`](man:signal(7)) and + * [`signal-safety(7)`](man:signal-safety(7))), making it safe to call from a + * signal handler or a #GSpawnChildSetupFunc. + * + * This function preserves the value of `errno`. + * + * Returns: a non-negative file descriptor, which becomes owned by the + * caller and must be closed by the caller if required, or a negative + * number if the corresponding end of the pipe was already closed or stolen + * + * Since: 2.80 + */ +GLIB_AVAILABLE_STATIC_INLINE_IN_2_80 +static inline int +g_unix_pipe_steal (GUnixPipe *self, + GUnixPipeEnd end) +{ + return g_steal_fd (&self->fds[end]); +} + +/** + * g_unix_pipe_close: + * @self: A pair of file descriptors + * @end: One of the ends of the pipe + * @error: Optionally used to report an error on failure + * + * Close one of the ends of the pipe and set the relevant member of @fds + * to `-1` before returning, equivalent to g_clear_fd(). + * + * Like g_close(), if closing the file descriptor fails, the error is + * stored in both %errno and @error. If this function succeeds, + * %errno is undefined. + * + * This function is async-signal safe if @error is %NULL and the relevant + * member of @fds is either negative or a valid open file descriptor. + * This makes it safe to call from a signal handler or a #GSpawnChildSetupFunc + * under those conditions. + * See [`signal(7)`](man:signal(7)) and + * [`signal-safety(7)`](man:signal-safety(7)) for more details. + * + * To close both file descriptors and ignore any errors, use + * g_unix_pipe_clear() instead. + * + * Returns: %TRUE on success + * + * Since: 2.80 + */ +GLIB_AVAILABLE_STATIC_INLINE_IN_2_80 +static inline gboolean +g_unix_pipe_close (GUnixPipe *self, + GUnixPipeEnd end, + GError **error) +{ + return g_clear_fd (&self->fds[end], error); +} + +/** + * g_unix_pipe_clear: + * @self: a #GUnixPipe + * + * Close both ends of the pipe, unless they have already been closed or + * stolen. Any errors are ignored: use g_unix_pipe_close() or g_clear_fd() + * if error-handling is required. + * + * This function is async-signal safe if @error is %NULL and each member + * of @fds are either negative or a valid open file descriptor. + * As a result, it is safe to call this function or use `g_auto(GUnixPipe)` + * (on compilers that support it) in a signal handler or a + * #GSpawnChildSetupFunc, as long as those conditions are ensured to be true. + * See [`signal(7)`](man:signal(7)) and + * [`signal-safety(7)`](man:signal-safety(7)) for more details. + * + * This function preserves the value of `errno`. + * + * Since: 2.80 + */ +GLIB_AVAILABLE_STATIC_INLINE_IN_2_80 +static inline void +g_unix_pipe_clear (GUnixPipe *self) +{ + /* Don't overwrite thread-local errno if closing the fd fails */ + int errsv = errno; + + if (!g_unix_pipe_close (self, G_UNIX_PIPE_END_READ, NULL)) + { + /* ignore */ + } + + if (!g_unix_pipe_close (self, G_UNIX_PIPE_END_WRITE, NULL)) + { + /* ignore */ + } + + errno = errsv; +} + +G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC (GUnixPipe, g_unix_pipe_clear) + +G_GNUC_END_IGNORE_DEPRECATIONS + G_END_DECLS #endif /* __G_UNIX_H__ */ From 26c7b308ba3309b479e6e7dd808756c0ff6e2422 Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Wed, 31 May 2023 12:00:28 +0100 Subject: [PATCH 3/5] tests: Break out assert_fd_was_closed() into a header Signed-off-by: Simon McVittie --- docs/reference/glib/meson.build | 1 + glib/tests/fileutils.c | 22 ++------------- glib/tests/testutils.h | 50 +++++++++++++++++++++++++++++++++ 3 files changed, 53 insertions(+), 20 deletions(-) create mode 100644 glib/tests/testutils.h diff --git a/docs/reference/glib/meson.build b/docs/reference/glib/meson.build index fe34e3fff..78acc83c8 100644 --- a/docs/reference/glib/meson.build +++ b/docs/reference/glib/meson.build @@ -35,6 +35,7 @@ if get_option('gtk_doc') 'glib-init.h', 'gconstructor.h', 'gconstructorprivate.h', + 'testutils.h', 'valgrind.h', 'gutilsprivate.h', 'gvalgrind.h', diff --git a/glib/tests/fileutils.c b/glib/tests/fileutils.c index ab5b574cc..87a61339a 100644 --- a/glib/tests/fileutils.c +++ b/glib/tests/fileutils.c @@ -60,6 +60,8 @@ #define G_TEST_DIR_MODE (S_IWRITE | S_IREAD) #endif +#include "testutils.h" + #define S G_DIR_SEPARATOR_S static void @@ -2473,26 +2475,6 @@ test_win32_zero_terminate_symlink (void) #endif -static void -assert_fd_was_closed (int fd) -{ - /* We can't tell a fd was really closed without behaving as though it - * was still valid */ - if (g_test_undefined ()) - { - int result, errsv; - GWin32InvalidParameterHandler handler; - - GLIB_PRIVATE_CALL (g_win32_push_empty_invalid_parameter_handler) (&handler); - result = g_fsync (fd); - errsv = errno; - GLIB_PRIVATE_CALL (g_win32_pop_invalid_parameter_handler) (&handler); - - g_assert_cmpint (result, !=, 0); - g_assert_cmpint (errsv, ==, EBADF); - } -} - static void test_clear_fd_ebadf (void) { diff --git a/glib/tests/testutils.h b/glib/tests/testutils.h new file mode 100644 index 000000000..4a26b2824 --- /dev/null +++ b/glib/tests/testutils.h @@ -0,0 +1,50 @@ +/* + * Copyright 1995-1997 Peter Mattis, Spencer Kimball and Josh MacDonald + * Copyright 2023 Collabora Ltd. + * + * SPDX-License-Identifier: LicenseRef-old-glib-tests + * + * This work is provided "as is"; redistribution and modification + * in whole or in part, in any medium, physical or electronic is + * permitted without restriction. + * + * This work is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. + * + * In no event shall the authors or contributors be liable for any + * direct, indirect, incidental, special, exemplary, or consequential + * damages (including, but not limited to, procurement of substitute + * goods or services; loss of use, data, or profits; or business + * interruption) however caused and on any theory of liability, whether + * in contract, strict liability, or tort (including negligence or + * otherwise) arising in any way out of the use of this software, even + * if advised of the possibility of such damage. + */ + +#include "config.h" + +#include + +#include "glib-private.h" +#include "gstdio.h" + +static inline void +assert_fd_was_closed (int fd) +{ + /* We can't tell a fd was really closed without behaving as though it + * was still valid */ + if (g_test_undefined ()) + { + int result, errsv; + GWin32InvalidParameterHandler handler; + + GLIB_PRIVATE_CALL (g_win32_push_empty_invalid_parameter_handler) (&handler); + result = g_fsync (fd); + errsv = errno; + GLIB_PRIVATE_CALL (g_win32_pop_invalid_parameter_handler) (&handler); + + g_assert_cmpint (result, !=, 0); + g_assert_cmpint (errsv, ==, EBADF); + } +} From 9e20a7d0e74e5fbdf54406a824837a73cf0ba599 Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Wed, 31 May 2023 12:00:41 +0100 Subject: [PATCH 4/5] tests: Exercise GUnixPipe Signed-off-by: Simon McVittie --- glib/tests/unix.c | 116 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 116 insertions(+) diff --git a/glib/tests/unix.c b/glib/tests/unix.c index ab6efaa76..e4003f730 100644 --- a/glib/tests/unix.c +++ b/glib/tests/unix.c @@ -1,5 +1,6 @@ /* * Copyright (C) 2011 Red Hat, Inc. + * Copyright 2023 Collabora Ltd. * * SPDX-License-Identifier: LicenseRef-old-glib-tests * @@ -25,6 +26,7 @@ #include "config.h" +#include "glib-private.h" #include "glib-unix.h" #include "gstdio.h" @@ -32,6 +34,8 @@ #include #include +#include "testutils.h" + static void test_pipe (void) { @@ -122,6 +126,116 @@ test_pipe_stdio_overwrite (void) g_assert_no_error (error); } +static void +test_pipe_struct (void) +{ + GError *error = NULL; + GUnixPipe pair = G_UNIX_PIPE_INIT; + char buf[1024]; + gssize bytes_read; + gboolean res; + int read_end = -1; /* owned */ + int write_end = -1; /* unowned */ + int errsv; + + g_test_summary ("Test GUnixPipe structure"); + + res = g_unix_pipe_open (&pair, FD_CLOEXEC, &error); + g_assert_no_error (error); + g_assert_true (res); + + read_end = g_unix_pipe_steal (&pair, G_UNIX_PIPE_END_READ); + g_assert_cmpint (read_end, >=, 0); + g_assert_cmpint (g_unix_pipe_steal (&pair, G_UNIX_PIPE_END_READ), ==, -1); + g_assert_cmpint (g_unix_pipe_get (&pair, G_UNIX_PIPE_END_READ), ==, -1); + write_end = g_unix_pipe_get (&pair, G_UNIX_PIPE_END_WRITE); + g_assert_cmpint (write_end, >=, 0); + g_assert_cmpint (g_unix_pipe_get (&pair, G_UNIX_PIPE_END_WRITE), ==, write_end); + + g_assert_cmpint (write (write_end, "hello", sizeof ("hello")), ==, sizeof ("hello")); + memset (buf, 0, sizeof (buf)); + bytes_read = read (read_end, buf, sizeof(buf) - 1); + g_assert_cmpint (bytes_read, ==, sizeof ("hello")); + buf[bytes_read] = '\0'; + + /* This is one of the few errno values guaranteed by Standard C. + * We set it here to check that g_unix_pipe_clear doesn't alter errno. */ + errno = EILSEQ; + + g_unix_pipe_clear (&pair); + errsv = errno; + g_assert_cmpint (errsv, ==, EILSEQ); + + g_assert_cmpint (pair.fds[0], ==, -1); + g_assert_cmpint (pair.fds[1], ==, -1); + + /* The read end wasn't closed, because it was stolen first */ + g_clear_fd (&read_end, &error); + g_assert_no_error (error); + + /* The write end was closed, because it wasn't stolen */ + assert_fd_was_closed (write_end); + + g_assert_cmpstr (buf, ==, "hello"); +} + +static void +test_pipe_struct_auto (void) +{ +#ifdef g_autofree + int i; + + g_test_summary ("Test g_auto(GUnixPipe)"); + + /* Let g_auto close the read end, the write end, neither, or both */ + for (i = 0; i < 4; i++) + { + int read_end = -1; /* unowned */ + int write_end = -1; /* unowned */ + int errsv; + + { + g_auto(GUnixPipe) pair = G_UNIX_PIPE_INIT; + GError *error = NULL; + gboolean res; + + res = g_unix_pipe_open (&pair, FD_CLOEXEC, &error); + g_assert_no_error (error); + g_assert_true (res); + + read_end = pair.fds[G_UNIX_PIPE_END_READ]; + g_assert_cmpint (read_end, >=, 0); + write_end = pair.fds[G_UNIX_PIPE_END_WRITE]; + g_assert_cmpint (write_end, >=, 0); + + if (i & 1) + { + /* This also exercises g_unix_pipe_close() with error */ + res = g_unix_pipe_close (&pair, G_UNIX_PIPE_END_READ, &error); + g_assert_no_error (error); + g_assert_true (res); + } + + /* This also exercises g_unix_pipe_close() without error */ + if (i & 2) + g_unix_pipe_close (&pair, G_UNIX_PIPE_END_WRITE, NULL); + + /* This is one of the few errno values guaranteed by Standard C. + * We set it here to check that a g_auto(GUnixPipe) close doesn't + * alter errno. */ + errno = EILSEQ; + } + + errsv = errno; + g_assert_cmpint (errsv, ==, EILSEQ); + assert_fd_was_closed (read_end); + assert_fd_was_closed (write_end); + } +#else + g_test_skip ("g_auto not supported by compiler"); +#endif +} + static void test_error (void) { @@ -502,6 +616,8 @@ main (int argc, g_test_add_func ("/glib-unix/pipe", test_pipe); g_test_add_func ("/glib-unix/pipe/fd-cloexec", test_pipe_fd_cloexec); g_test_add_func ("/glib-unix/pipe-stdio-overwrite", test_pipe_stdio_overwrite); + g_test_add_func ("/glib-unix/pipe-struct", test_pipe_struct); + g_test_add_func ("/glib-unix/pipe-struct-auto", test_pipe_struct_auto); g_test_add_func ("/glib-unix/error", test_error); g_test_add_func ("/glib-unix/nonblocking", test_nonblocking); g_test_add_func ("/glib-unix/sighup", test_sighup); From e80b0c4e4b76b41ed19273453d51fada8455cb1a Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Wed, 31 May 2023 12:00:58 +0100 Subject: [PATCH 5/5] gspawn: Use GUnixPipe on Unix Signed-off-by: Simon McVittie --- glib/gspawn.c | 118 +++++++++++++++++++++++++------------------------- 1 file changed, 59 insertions(+), 59 deletions(-) diff --git a/glib/gspawn.c b/glib/gspawn.c index 4d0f1e274..0a7bf0a71 100644 --- a/glib/gspawn.c +++ b/glib/gspawn.c @@ -2268,6 +2268,16 @@ out_free_spawnattr: } #endif /* POSIX_SPAWN_AVAILABLE */ +static gboolean +source_fds_collide_with_pipe (const GUnixPipe *pipefd, + const int *source_fds, + gsize n_fds, + GError **error) +{ + return (_g_spawn_invalid_source_fd (pipefd->fds[G_UNIX_PIPE_END_READ], source_fds, n_fds, error) || + _g_spawn_invalid_source_fd (pipefd->fds[G_UNIX_PIPE_END_WRITE], source_fds, n_fds, error)); +} + static gboolean fork_exec (gboolean intermediate_child, const gchar *working_directory, @@ -2296,8 +2306,8 @@ fork_exec (gboolean intermediate_child, GError **error) { GPid pid = -1; - gint child_err_report_pipe[2] = { -1, -1 }; - gint child_pid_report_pipe[2] = { -1, -1 }; + GUnixPipe child_err_report_pipe = G_UNIX_PIPE_INIT; + GUnixPipe child_pid_report_pipe = G_UNIX_PIPE_INIT; guint pipe_flags = cloexec_pipes ? FD_CLOEXEC : 0; gint status; const gchar *chosen_search_path; @@ -2307,9 +2317,9 @@ fork_exec (gboolean intermediate_child, gchar **argv_buffer = NULL; gchar **argv_buffer_heap = NULL; gsize argv_buffer_len = 0; - gint stdin_pipe[2] = { -1, -1 }; - gint stdout_pipe[2] = { -1, -1 }; - gint stderr_pipe[2] = { -1, -1 }; + GUnixPipe stdin_pipe = G_UNIX_PIPE_INIT; + GUnixPipe stdout_pipe = G_UNIX_PIPE_INIT; + GUnixPipe stderr_pipe = G_UNIX_PIPE_INIT; gint child_close_fds[4] = { -1, -1, -1, -1 }; gint n_child_close_fds = 0; gint *source_fds_copy = NULL; @@ -2322,35 +2332,32 @@ fork_exec (gboolean intermediate_child, /* If pipes have been requested, open them */ if (stdin_pipe_out != NULL) { - if (!g_unix_open_pipe (stdin_pipe, pipe_flags, error)) + if (!g_unix_pipe_open (&stdin_pipe, pipe_flags, error)) goto cleanup_and_fail; - if (_g_spawn_invalid_source_fd (stdin_pipe[0], source_fds, n_fds, error) || - _g_spawn_invalid_source_fd (stdin_pipe[1], source_fds, n_fds, error)) + if (source_fds_collide_with_pipe (&stdin_pipe, source_fds, n_fds, error)) goto cleanup_and_fail; - child_close_fds[n_child_close_fds++] = stdin_pipe[1]; - stdin_fd = stdin_pipe[0]; + child_close_fds[n_child_close_fds++] = g_unix_pipe_get (&stdin_pipe, G_UNIX_PIPE_END_WRITE); + stdin_fd = g_unix_pipe_get (&stdin_pipe, G_UNIX_PIPE_END_READ); } if (stdout_pipe_out != NULL) { - if (!g_unix_open_pipe (stdout_pipe, pipe_flags, error)) + if (!g_unix_pipe_open (&stdout_pipe, pipe_flags, error)) goto cleanup_and_fail; - if (_g_spawn_invalid_source_fd (stdout_pipe[0], source_fds, n_fds, error) || - _g_spawn_invalid_source_fd (stdout_pipe[1], source_fds, n_fds, error)) + if (source_fds_collide_with_pipe (&stdout_pipe, source_fds, n_fds, error)) goto cleanup_and_fail; - child_close_fds[n_child_close_fds++] = stdout_pipe[0]; - stdout_fd = stdout_pipe[1]; + child_close_fds[n_child_close_fds++] = g_unix_pipe_get (&stdout_pipe, G_UNIX_PIPE_END_READ); + stdout_fd = g_unix_pipe_get (&stdout_pipe, G_UNIX_PIPE_END_WRITE); } if (stderr_pipe_out != NULL) { - if (!g_unix_open_pipe (stderr_pipe, pipe_flags, error)) + if (!g_unix_pipe_open (&stderr_pipe, pipe_flags, error)) goto cleanup_and_fail; - if (_g_spawn_invalid_source_fd (stderr_pipe[0], source_fds, n_fds, error) || - _g_spawn_invalid_source_fd (stderr_pipe[1], source_fds, n_fds, error)) + if (source_fds_collide_with_pipe (&stderr_pipe, source_fds, n_fds, error)) goto cleanup_and_fail; - child_close_fds[n_child_close_fds++] = stderr_pipe[0]; - stderr_fd = stderr_pipe[1]; + child_close_fds[n_child_close_fds++] = g_unix_pipe_get (&stderr_pipe, G_UNIX_PIPE_END_READ); + stderr_fd = g_unix_pipe_get (&stderr_pipe, G_UNIX_PIPE_END_WRITE); } child_close_fds[n_child_close_fds++] = -1; @@ -2488,18 +2495,16 @@ fork_exec (gboolean intermediate_child, if (n_fds > 0) memcpy (source_fds_copy, source_fds, sizeof (*source_fds) * n_fds); - if (!g_unix_open_pipe (child_err_report_pipe, pipe_flags, error)) + if (!g_unix_pipe_open (&child_err_report_pipe, pipe_flags, error)) goto cleanup_and_fail; - if (_g_spawn_invalid_source_fd (child_err_report_pipe[0], source_fds, n_fds, error) || - _g_spawn_invalid_source_fd (child_err_report_pipe[1], source_fds, n_fds, error)) + if (source_fds_collide_with_pipe (&child_err_report_pipe, source_fds, n_fds, error)) goto cleanup_and_fail; if (intermediate_child) { - if (!g_unix_open_pipe (child_pid_report_pipe, pipe_flags, error)) + if (!g_unix_pipe_open (&child_pid_report_pipe, pipe_flags, error)) goto cleanup_and_fail; - if (_g_spawn_invalid_source_fd (child_pid_report_pipe[0], source_fds, n_fds, error) || - _g_spawn_invalid_source_fd (child_pid_report_pipe[1], source_fds, n_fds, error)) + if (source_fds_collide_with_pipe (&child_pid_report_pipe, source_fds, n_fds, error)) goto cleanup_and_fail; } @@ -2538,8 +2543,8 @@ fork_exec (gboolean intermediate_child, * not needed in the close_descriptors case, * though */ - g_clear_fd (&child_err_report_pipe[0], NULL); - g_clear_fd (&child_pid_report_pipe[0], NULL); + g_unix_pipe_close (&child_err_report_pipe, G_UNIX_PIPE_END_READ, NULL); + g_unix_pipe_close (&child_pid_report_pipe, G_UNIX_PIPE_END_READ, NULL); if (child_close_fds[0] != -1) { int i = -1; @@ -2561,16 +2566,16 @@ fork_exec (gboolean intermediate_child, if (grandchild_pid < 0) { /* report -1 as child PID */ - write_all (child_pid_report_pipe[1], &grandchild_pid, - sizeof(grandchild_pid)); + write_all (g_unix_pipe_get (&child_pid_report_pipe, G_UNIX_PIPE_END_WRITE), + &grandchild_pid, sizeof(grandchild_pid)); - write_err_and_exit (child_err_report_pipe[1], + write_err_and_exit (g_unix_pipe_get (&child_err_report_pipe, G_UNIX_PIPE_END_WRITE), CHILD_FORK_FAILED); } else if (grandchild_pid == 0) { - g_clear_fd (&child_pid_report_pipe[1], NULL); - do_exec (child_err_report_pipe[1], + g_unix_pipe_close (&child_pid_report_pipe, G_UNIX_PIPE_END_WRITE, NULL); + do_exec (g_unix_pipe_get (&child_err_report_pipe, G_UNIX_PIPE_END_WRITE), stdin_fd, stdout_fd, stderr_fd, @@ -2595,8 +2600,9 @@ fork_exec (gboolean intermediate_child, } else { - write_all (child_pid_report_pipe[1], &grandchild_pid, sizeof(grandchild_pid)); - g_clear_fd (&child_pid_report_pipe[1], NULL); + write_all (g_unix_pipe_get (&child_pid_report_pipe, G_UNIX_PIPE_END_WRITE), + &grandchild_pid, sizeof(grandchild_pid)); + g_unix_pipe_close (&child_pid_report_pipe, G_UNIX_PIPE_END_WRITE, NULL); _exit (0); } @@ -2606,7 +2612,7 @@ fork_exec (gboolean intermediate_child, /* Just run the child. */ - do_exec (child_err_report_pipe[1], + do_exec (g_unix_pipe_get (&child_err_report_pipe, G_UNIX_PIPE_END_WRITE), stdin_fd, stdout_fd, stderr_fd, @@ -2638,8 +2644,8 @@ fork_exec (gboolean intermediate_child, gint n_ints = 0; /* Close the uncared-about ends of the pipes */ - g_clear_fd (&child_err_report_pipe[1], NULL); - g_clear_fd (&child_pid_report_pipe[1], NULL); + g_unix_pipe_close (&child_err_report_pipe, G_UNIX_PIPE_END_WRITE, NULL); + g_unix_pipe_close (&child_pid_report_pipe, G_UNIX_PIPE_END_WRITE, NULL); /* If we had an intermediate child, reap it */ if (intermediate_child) @@ -2657,7 +2663,7 @@ fork_exec (gboolean intermediate_child, } - if (!read_ints (child_err_report_pipe[0], + if (!read_ints (g_unix_pipe_get (&child_err_report_pipe, G_UNIX_PIPE_END_READ), buf, 2, &n_ints, error)) goto cleanup_and_fail; @@ -2738,7 +2744,7 @@ fork_exec (gboolean intermediate_child, { n_ints = 0; - if (!read_ints (child_pid_report_pipe[0], + if (!read_ints (g_unix_pipe_get (&child_pid_report_pipe, G_UNIX_PIPE_END_READ), buf, 1, &n_ints, error)) goto cleanup_and_fail; @@ -2761,8 +2767,8 @@ fork_exec (gboolean intermediate_child, } /* Success against all odds! return the information */ - g_clear_fd (&child_err_report_pipe[0], NULL); - g_clear_fd (&child_pid_report_pipe[0], NULL); + g_unix_pipe_close (&child_err_report_pipe, G_UNIX_PIPE_END_READ, NULL); + g_unix_pipe_close (&child_pid_report_pipe, G_UNIX_PIPE_END_READ, NULL); g_free (search_path_buffer_heap); g_free (argv_buffer_heap); @@ -2776,18 +2782,18 @@ fork_exec (gboolean intermediate_child, success: /* Close the uncared-about ends of the pipes */ - g_clear_fd (&stdin_pipe[0], NULL); - g_clear_fd (&stdout_pipe[1], NULL); - g_clear_fd (&stderr_pipe[1], NULL); + g_unix_pipe_close (&stdin_pipe, G_UNIX_PIPE_END_READ, NULL); + g_unix_pipe_close (&stdout_pipe, G_UNIX_PIPE_END_WRITE, NULL); + g_unix_pipe_close (&stderr_pipe, G_UNIX_PIPE_END_WRITE, NULL); if (stdin_pipe_out != NULL) - *stdin_pipe_out = g_steal_fd (&stdin_pipe[1]); + *stdin_pipe_out = g_unix_pipe_steal (&stdin_pipe, G_UNIX_PIPE_END_WRITE); if (stdout_pipe_out != NULL) - *stdout_pipe_out = g_steal_fd (&stdout_pipe[0]); + *stdout_pipe_out = g_unix_pipe_steal (&stdout_pipe, G_UNIX_PIPE_END_READ); if (stderr_pipe_out != NULL) - *stderr_pipe_out = g_steal_fd (&stderr_pipe[0]); + *stderr_pipe_out = g_unix_pipe_steal (&stderr_pipe, G_UNIX_PIPE_END_READ); return TRUE; @@ -2811,17 +2817,11 @@ success: } } - g_clear_fd (&stdin_pipe[0], NULL); - g_clear_fd (&stdin_pipe[1], NULL); - g_clear_fd (&stdout_pipe[0], NULL); - g_clear_fd (&stdout_pipe[1], NULL); - g_clear_fd (&stderr_pipe[0], NULL); - g_clear_fd (&stderr_pipe[1], NULL); - - g_clear_fd (&child_err_report_pipe[0], NULL); - g_clear_fd (&child_err_report_pipe[1], NULL); - g_clear_fd (&child_pid_report_pipe[0], NULL); - g_clear_fd (&child_pid_report_pipe[1], NULL); + g_unix_pipe_clear (&stdin_pipe); + g_unix_pipe_clear (&stdout_pipe); + g_unix_pipe_clear (&stderr_pipe); + g_unix_pipe_clear (&child_err_report_pipe); + g_unix_pipe_clear (&child_pid_report_pipe); g_clear_pointer (&search_path_buffer_heap, g_free); g_clear_pointer (&argv_buffer_heap, g_free);