From d0dc7171d64a74041555e850e95ac4965618405e Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Wed, 16 Aug 2023 14:26:14 +0100 Subject: [PATCH 1/2] glib-unix: Accept O_CLOEXEC as well as FD_CLOEXEC in g_unix_open_pipe() This is one step towards rectifying the mistake of using `FD_CLOEXEC` in the first place. Eventually we may deprecate support for `FD_CLOEXEC`, as the `O_*` flags better match the underlying `pipe()` API. See discussion on https://gitlab.gnome.org/GNOME/glib/-/merge_requests/3459#note_1779264 Signed-off-by: Philip Withnall --- glib/glib-unix.c | 24 ++++++++++++++++-------- glib/tests/unix.c | 29 +++++++++++++++++++++++++++++ 2 files changed, 45 insertions(+), 8 deletions(-) diff --git a/glib/glib-unix.c b/glib/glib-unix.c index 0532a500c..df5726cf0 100644 --- a/glib/glib-unix.c +++ b/glib/glib-unix.c @@ -82,15 +82,18 @@ g_unix_set_error_from_errno (GError **error, * uses the pipe2() system call, which atomically creates a pipe with * the configured flags. * - * As of GLib 2.78, the supported flags are `FD_CLOEXEC` and `O_NONBLOCK`. Prior - * to GLib 2.78, only `FD_CLOEXEC` was supported — if you wanted to configure - * `O_NONBLOCK` then that had to be done separately with `fcntl()`. + * As of GLib 2.78, the supported flags are `O_CLOEXEC`/`FD_CLOEXEC` (see below) + * and `O_NONBLOCK`. Prior to GLib 2.78, only `FD_CLOEXEC` was supported — if + * you wanted to configure `O_NONBLOCK` then that had to be done separately with + * `fcntl()`. * * It is a programmer error to call this function with unsupported flags, and a * critical warning will be raised. * - * This function does not take `O_CLOEXEC`, it takes `FD_CLOEXEC` as if - * for fcntl(); these are different on Linux/glibc. + * As of GLib 2.78, it is preferred to pass `O_CLOEXEC` in, rather than + * `FD_CLOEXEC`, as that matches the underlying `pipe()` API more closely. Prior + * to 2.78, only `FD_CLOEXEC` was supported. Support for `FD_CLOEXEC` may be + * deprecated and removed in future. * * Returns: %TRUE on success, %FALSE if not (and errno will be set). * @@ -101,11 +104,16 @@ g_unix_open_pipe (int *fds, int flags, GError **error) { - /* We only support FD_CLOEXEC and O_NONBLOCK */ - g_return_val_if_fail ((flags & (FD_CLOEXEC | O_NONBLOCK)) == flags, FALSE); + /* We only support O_CLOEXEC/FD_CLOEXEC and O_NONBLOCK */ + g_return_val_if_fail ((flags & (O_CLOEXEC | FD_CLOEXEC | O_NONBLOCK)) == flags, FALSE); + +#if O_CLOEXEC != FD_CLOEXEC && !defined(G_DISABLE_CHECKS) + if (flags & FD_CLOEXEC) + g_debug ("g_unix_open_pipe() called with FD_CLOEXEC; please migrate to using O_CLOEXEC instead"); +#endif if (!g_unix_open_pipe_internal (fds, - (flags & FD_CLOEXEC) != 0, + (flags & (O_CLOEXEC | FD_CLOEXEC)) != 0, (flags & O_NONBLOCK) != 0)) return g_unix_set_error_from_errno (error, errno); diff --git a/glib/tests/unix.c b/glib/tests/unix.c index 90345eda0..12f4a609f 100644 --- a/glib/tests/unix.c +++ b/glib/tests/unix.c @@ -57,6 +57,34 @@ test_pipe (void) g_assert (g_str_has_prefix (buf, "hello")); } +static void +test_pipe_fd_cloexec (void) +{ + GError *error = NULL; + int pipefd[2]; + char buf[1024]; + gssize bytes_read; + gboolean res; + + g_test_summary ("Test that FD_CLOEXEC is still accepted as an argument to g_unix_open_pipe()"); + g_test_bug ("https://gitlab.gnome.org/GNOME/glib/-/merge_requests/3459"); + + res = g_unix_open_pipe (pipefd, FD_CLOEXEC, &error); + g_assert (res); + g_assert_no_error (error); + + 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); + buf[bytes_read] = '\0'; + + close (pipefd[0]); + close (pipefd[1]); + + g_assert_true (g_str_has_prefix (buf, "hello")); +} + static void test_pipe_stdio_overwrite (void) { @@ -472,6 +500,7 @@ main (int argc, g_test_init (&argc, &argv, NULL); 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/error", test_error); g_test_add_func ("/glib-unix/nonblocking", test_nonblocking); From 07c4b6c68e4f280c816200cf1335e7dec103f25c Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Wed, 16 Aug 2023 14:31:02 +0100 Subject: [PATCH 2/2] glib: Pass O_CLOEXEC rather than FD_CLOEXEC to g_unix_open_pipe() See the previous commit. Signed-off-by: Philip Withnall --- gio/gfile.c | 2 +- gio/gtestdbus.c | 2 +- gio/tests/gdbus-close-pending.c | 4 ++-- gio/tests/gsubprocess.c | 8 ++++---- gio/tests/unix-fd.c | 2 +- glib/gtester.c | 2 +- glib/gwakeup.c | 2 +- glib/tests/spawn-singlethread.c | 2 +- glib/tests/unix.c | 6 +++--- 9 files changed, 15 insertions(+), 15 deletions(-) diff --git a/gio/gfile.c b/gio/gfile.c index 0e6d4c819..16ff09fe3 100644 --- a/gio/gfile.c +++ b/gio/gfile.c @@ -3188,7 +3188,7 @@ splice_stream_with_progress (GInputStream *in, fd_in = g_file_descriptor_based_get_fd (G_FILE_DESCRIPTOR_BASED (in)); fd_out = g_file_descriptor_based_get_fd (G_FILE_DESCRIPTOR_BASED (out)); - if (!g_unix_open_pipe (buffer, FD_CLOEXEC, error)) + if (!g_unix_open_pipe (buffer, O_CLOEXEC, error)) return FALSE; /* Try a 1MiB buffer for improved throughput. If that fails, use the default diff --git a/gio/gtestdbus.c b/gio/gtestdbus.c index 9ff74e653..d75b4395a 100644 --- a/gio/gtestdbus.c +++ b/gio/gtestdbus.c @@ -604,7 +604,7 @@ make_pipe (gint pipe_fds[2], GError **error) { #if defined(G_OS_UNIX) - return g_unix_open_pipe (pipe_fds, FD_CLOEXEC, error); + return g_unix_open_pipe (pipe_fds, O_CLOEXEC, error); #elif defined(G_OS_WIN32) if (_pipe (pipe_fds, 4096, _O_BINARY) < 0) { diff --git a/gio/tests/gdbus-close-pending.c b/gio/tests/gdbus-close-pending.c index 7d4d6c9ab..61f1faa79 100644 --- a/gio/tests/gdbus-close-pending.c +++ b/gio/tests/gdbus-close-pending.c @@ -306,9 +306,9 @@ test_once (Fixture *f, GDBusMessage *message; gboolean pipe_res; - pipe_res = g_unix_open_pipe (f->server_to_client, FD_CLOEXEC, &f->error); + pipe_res = g_unix_open_pipe (f->server_to_client, O_CLOEXEC, &f->error); g_assert (pipe_res); - pipe_res = g_unix_open_pipe (f->client_to_server, FD_CLOEXEC, &f->error); + pipe_res = g_unix_open_pipe (f->client_to_server, O_CLOEXEC, &f->error); g_assert (pipe_res); f->server_iostream = my_io_stream_new_for_fds (f->client_to_server[0], diff --git a/gio/tests/gsubprocess.c b/gio/tests/gsubprocess.c index 695012c7d..45fdf93b2 100644 --- a/gio/tests/gsubprocess.c +++ b/gio/tests/gsubprocess.c @@ -1750,9 +1750,9 @@ do_test_pass_fd (GSubprocessFlags flags, char *basic_fd_str; char *needdup_fd_str; - g_unix_open_pipe (basic_pipefds, FD_CLOEXEC, error); + g_unix_open_pipe (basic_pipefds, O_CLOEXEC, error); g_assert_no_error (local_error); - g_unix_open_pipe (needdup_pipefds, FD_CLOEXEC, error); + g_unix_open_pipe (needdup_pipefds, O_CLOEXEC, error); g_assert_no_error (local_error); basic_fd_str = g_strdup_printf ("%d", basic_pipefds[1]); @@ -1854,10 +1854,10 @@ do_test_fd_conflation (GSubprocessFlags flags, return; } - g_unix_open_pipe (unused_pipefds, FD_CLOEXEC, &error); + g_unix_open_pipe (unused_pipefds, O_CLOEXEC, &error); g_assert_no_error (error); - g_unix_open_pipe (pipefds, FD_CLOEXEC, &error); + g_unix_open_pipe (pipefds, O_CLOEXEC, &error); g_assert_no_error (error); /* The fds should be sequential since we are in a new process. */ diff --git a/gio/tests/unix-fd.c b/gio/tests/unix-fd.c index ecf1aea37..62c17df14 100644 --- a/gio/tests/unix-fd.c +++ b/gio/tests/unix-fd.c @@ -63,7 +63,7 @@ test_fd_list (void) s = _pipe (sv, 4096, _O_NOINHERIT | _O_BINARY); g_assert_cmpint (s, ==, 0); #else - g_unix_open_pipe (sv, FD_CLOEXEC, &err); + g_unix_open_pipe (sv, O_CLOEXEC, &err); g_assert_no_error (err); #endif list = g_unix_fd_list_new_from_array (sv, -1); diff --git a/glib/gtester.c b/glib/gtester.c index af1f29164..24f84f6c9 100644 --- a/glib/gtester.c +++ b/glib/gtester.c @@ -305,7 +305,7 @@ launch_test_binary (const char *binary, gboolean loop_pending; gint i = 0; - if (!g_unix_open_pipe (report_pipe, FD_CLOEXEC, &error)) + if (!g_unix_open_pipe (report_pipe, O_CLOEXEC, &error)) { if (subtest_mode_fatal) g_error ("Failed to open pipe for test binary: %s: %s", binary, error->message); diff --git a/glib/gwakeup.c b/glib/gwakeup.c index a3283a3ff..1674304f0 100644 --- a/glib/gwakeup.c +++ b/glib/gwakeup.c @@ -160,7 +160,7 @@ g_wakeup_new (void) /* for any failure, try a pipe instead */ #endif - if (!g_unix_open_pipe (wakeup->fds, FD_CLOEXEC | O_NONBLOCK, &error)) + if (!g_unix_open_pipe (wakeup->fds, O_CLOEXEC | O_NONBLOCK, &error)) g_error ("Creating pipes for GWakeup: %s", error->message); if (!g_unix_set_fd_nonblocking (wakeup->fds[0], TRUE, &error) || diff --git a/glib/tests/spawn-singlethread.c b/glib/tests/spawn-singlethread.c index 8d9ddef5f..33b44d650 100644 --- a/glib/tests/spawn-singlethread.c +++ b/glib/tests/spawn-singlethread.c @@ -237,7 +237,7 @@ test_spawn_async_with_fds (void) break; case PIPE: #ifdef G_OS_UNIX - g_unix_open_pipe (test_pipe[j], FD_CLOEXEC, &error); + g_unix_open_pipe (test_pipe[j], O_CLOEXEC, &error); g_assert_no_error (error); #else g_assert_cmpint (_pipe (test_pipe[j], 4096, _O_BINARY), >=, 0); diff --git a/glib/tests/unix.c b/glib/tests/unix.c index 12f4a609f..ab6efaa76 100644 --- a/glib/tests/unix.c +++ b/glib/tests/unix.c @@ -41,7 +41,7 @@ test_pipe (void) gssize bytes_read; gboolean res; - res = g_unix_open_pipe (pipefd, FD_CLOEXEC, &error); + res = g_unix_open_pipe (pipefd, O_CLOEXEC, &error); g_assert (res); g_assert_no_error (error); @@ -103,7 +103,7 @@ test_pipe_stdio_overwrite (void) g_close (STDIN_FILENO, &error); g_assert_no_error (error); - res = g_unix_open_pipe (pipefd, FD_CLOEXEC, &error); + res = g_unix_open_pipe (pipefd, O_CLOEXEC, &error); g_assert_no_error (error); g_assert_true (res); @@ -143,7 +143,7 @@ test_nonblocking (void) gboolean res; int flags; - res = g_unix_open_pipe (pipefd, FD_CLOEXEC, &error); + res = g_unix_open_pipe (pipefd, O_CLOEXEC, &error); g_assert (res); g_assert_no_error (error);