From 70279f84463e799eb0d98c9cb2026fbea4fc75e8 Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Wed, 28 Oct 2020 11:04:47 +0000 Subject: [PATCH 1/2] gdbus: Cope with sending fds in a message that takes multiple writes Suppose we are sending a 5K message with fds (so data->blob points to 5K of data, data->blob_size is 5K, and fd_list is non-null), but the kernel is only accepting up to 4K with each sendmsg(). The first time we get into write_message_continue_writing(), data->total_written will be 0. We will try to write the entire message, plus the attached file descriptors; or if the stream doesn't support fd-passing (not a socket), we need to fail with "Tried sending a file descriptor on unsupported stream". Because the kernel didn't accept the entire message, we come back in. This time, we won't enter the Unix-specific block that involves sending fds, because now data->total_written is 4K, and it would be wrong to try to attach the same fds again. However, we also need to avoid failing with "Tried sending a file descriptor on unsupported stream" in this case. We just want to write out the data of the rest of the message, starting from (blob + total_written) (in this exaple, the last 1K). Resolves: https://gitlab.gnome.org/GNOME/glib/-/issues/2074 Signed-off-by: Simon McVittie --- gio/gdbusprivate.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/gio/gdbusprivate.c b/gio/gdbusprivate.c index 5c980b40b..2551e4791 100644 --- a/gio/gdbusprivate.c +++ b/gio/gdbusprivate.c @@ -1085,8 +1085,11 @@ write_message_continue_writing (MessageToWriteData *data) else { #ifdef G_OS_UNIX - if (fd_list != NULL) + if (data->total_written == 0 && fd_list != NULL) { + /* We were trying to write byte 0 of the message, which needs + * the fd list to be attached to it, but this connection doesn't + * support doing that. */ g_task_return_new_error (task, G_IO_ERROR, G_IO_ERROR_FAILED, From e5cee9ce5acab2d4e51f3f461e726dd8f373a873 Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Wed, 28 Oct 2020 11:05:39 +0000 Subject: [PATCH 2/2] gio/tests/gdbus-peer: Exercise fds attached to a large message This incidentally also exercises the intended pattern for sending fds in a D-Bus message: the fd list is meant to contain exactly those fds that are referenced by a handle (type 'h') in the body of the message, with numeric handle value n corresponding to g_unix_fd_list_peek_fds(...)[n]. Being able to send and receive file descriptors that are not referenced by a handle (as in OpenFile here) is a quirk of the GDBus API, and while it's entirely possible in the wire protocol, other D-Bus implementations like libdbus and sd-bus typically don't provide APIs that make this possible. Reproduces: https://gitlab.gnome.org/GNOME/glib/-/issues/2074 Signed-off-by: Simon McVittie --- gio/tests/gdbus-peer.c | 197 ++++++++++++++++++++++++++++------------- 1 file changed, 134 insertions(+), 63 deletions(-) diff --git a/gio/tests/gdbus-peer.c b/gio/tests/gdbus-peer.c index ca53528d2..23a22981a 100644 --- a/gio/tests/gdbus-peer.c +++ b/gio/tests/gdbus-peer.c @@ -76,6 +76,12 @@ typedef struct gboolean signal_received; } PeerData; +/* This needs to be enough to usually take more than one write(), + * to reproduce + * . + * 1 MiB ought to be enough. */ +#define BIG_MESSAGE_ARRAY_SIZE (1024 * 1024) + static const gchar *test_interface_introspection_xml = "" " " @@ -88,6 +94,11 @@ static const gchar *test_interface_introspection_xml = " " " " " " + " " + " " + " " + " " + " " " " " " " " @@ -164,7 +175,8 @@ test_interface_method_call (GDBusConnection *connection, g_dbus_method_invocation_return_value (invocation, NULL); } - else if (g_strcmp0 (method_name, "OpenFile") == 0) + else if (g_strcmp0 (method_name, "OpenFile") == 0 || + g_strcmp0 (method_name, "OpenFileWithBigMessage") == 0) { #ifdef G_OS_UNIX const gchar *path; @@ -190,6 +202,21 @@ test_interface_method_call (GDBusConnection *connection, g_object_unref (fd_list); g_object_unref (invocation); + if (g_strcmp0 (method_name, "OpenFileWithBigMessage") == 0) + { + char *junk; + + junk = g_new0 (char, BIG_MESSAGE_ARRAY_SIZE); + g_dbus_message_set_body (reply, + g_variant_new ("(h@ay)", + 0, + g_variant_new_fixed_array (G_VARIANT_TYPE_BYTE, + junk, + BIG_MESSAGE_ARRAY_SIZE, + 1))); + g_free (junk); + } + error = NULL; g_dbus_connection_send_message (connection, reply, @@ -723,6 +750,7 @@ do_test_peer (void) const gchar *s; GThread *service_thread; gulong signal_handler_id; + gsize i; memset (&data, '\0', sizeof (PeerData)); data.current_connections = g_ptr_array_new_with_free_func (g_object_unref); @@ -843,73 +871,116 @@ do_test_peer (void) g_assert_cmpint (data.num_method_calls, ==, 3); g_signal_handler_disconnect (proxy, signal_handler_id); - /* check for UNIX fd passing */ + /* + * Check for UNIX fd passing. + * + * The first time through, we use a very simple method call. Note that + * because this does not have a G_VARIANT_TYPE_HANDLE in the message body + * to refer to the fd, it is a GDBus-specific idiom that would not + * interoperate with libdbus or sd-bus + * (see ). + * + * The second time, we call a method that returns a fd attached to a + * large message, to reproduce + * . It also happens + * to follow the more usual pattern for D-Bus messages containing a + * G_VARIANT_TYPE_HANDLE to refer to attached fds. + */ + for (i = 0; i < 2; i++) + { #ifdef G_OS_UNIX - { - GDBusMessage *method_call_message; - GDBusMessage *method_reply_message; - GUnixFDList *fd_list; - gint fd; - gchar *buf; - gsize len; - gchar *buf2; - gsize len2; - const char *testfile = g_test_get_filename (G_TEST_DIST, "file.c", NULL); + GDBusMessage *method_call_message; + GDBusMessage *method_reply_message; + GUnixFDList *fd_list; + gint fd; + gchar *buf; + gsize len; + gchar *buf2; + gsize len2; + const char *testfile = g_test_get_filename (G_TEST_DIST, "file.c", NULL); + const char *method = "OpenFile"; + GVariant *body; - method_call_message = g_dbus_message_new_method_call (NULL, /* name */ - "/org/gtk/GDBus/PeerTestObject", - "org.gtk.GDBus.PeerTestInterface", - "OpenFile"); - g_dbus_message_set_body (method_call_message, g_variant_new ("(s)", testfile)); - error = NULL; - method_reply_message = g_dbus_connection_send_message_with_reply_sync (c, - method_call_message, - G_DBUS_SEND_MESSAGE_FLAGS_NONE, - -1, - NULL, /* out_serial */ - NULL, /* cancellable */ - &error); - g_assert_no_error (error); - g_assert (g_dbus_message_get_message_type (method_reply_message) == G_DBUS_MESSAGE_TYPE_METHOD_RETURN); - fd_list = g_dbus_message_get_unix_fd_list (method_reply_message); - g_assert (fd_list != NULL); - g_assert_cmpint (g_unix_fd_list_get_length (fd_list), ==, 1); - error = NULL; - fd = g_unix_fd_list_get (fd_list, 0, &error); - g_assert_no_error (error); - g_object_unref (method_call_message); - g_object_unref (method_reply_message); + if (i == 1) + method = "OpenFileWithBigMessage"; - error = NULL; - len = 0; - buf = read_all_from_fd (fd, &len, &error); - g_assert_no_error (error); - g_assert (buf != NULL); - close (fd); + method_call_message = g_dbus_message_new_method_call (NULL, /* name */ + "/org/gtk/GDBus/PeerTestObject", + "org.gtk.GDBus.PeerTestInterface", + method); + g_dbus_message_set_body (method_call_message, g_variant_new ("(s)", testfile)); + error = NULL; + method_reply_message = g_dbus_connection_send_message_with_reply_sync (c, + method_call_message, + G_DBUS_SEND_MESSAGE_FLAGS_NONE, + -1, + NULL, /* out_serial */ + NULL, /* cancellable */ + &error); + g_assert_no_error (error); + g_assert (g_dbus_message_get_message_type (method_reply_message) == G_DBUS_MESSAGE_TYPE_METHOD_RETURN); - error = NULL; - g_file_get_contents (testfile, - &buf2, - &len2, - &error); - g_assert_no_error (error); - g_assert_cmpmem (buf, len, buf2, len2); - g_free (buf2); - g_free (buf); - } + body = g_dbus_message_get_body (method_reply_message); + + if (i == 1) + { + gint32 handle = -1; + GVariant *junk = NULL; + + g_assert_cmpstr (g_variant_get_type_string (body), ==, "(hay)"); + g_variant_get (body, "(h@ay)", &handle, &junk); + g_assert_cmpint (handle, ==, 0); + g_assert_cmpuint (g_variant_n_children (junk), ==, BIG_MESSAGE_ARRAY_SIZE); + g_variant_unref (junk); + } + else + { + g_assert_null (body); + } + + fd_list = g_dbus_message_get_unix_fd_list (method_reply_message); + g_assert (fd_list != NULL); + g_assert_cmpint (g_unix_fd_list_get_length (fd_list), ==, 1); + error = NULL; + fd = g_unix_fd_list_get (fd_list, 0, &error); + g_assert_no_error (error); + g_object_unref (method_call_message); + g_object_unref (method_reply_message); + + error = NULL; + len = 0; + buf = read_all_from_fd (fd, &len, &error); + g_assert_no_error (error); + g_assert (buf != NULL); + close (fd); + + error = NULL; + g_file_get_contents (testfile, + &buf2, + &len2, + &error); + g_assert_no_error (error); + g_assert_cmpmem (buf, len, buf2, len2); + g_free (buf2); + g_free (buf); #else - error = NULL; - result = g_dbus_proxy_call_sync (proxy, - "OpenFile", - g_variant_new ("(s)", "boo"), - G_DBUS_CALL_FLAGS_NONE, - -1, - NULL, /* GCancellable */ - &error); - g_assert_error (error, G_IO_ERROR, G_IO_ERROR_DBUS_ERROR); - g_assert (result == NULL); - g_error_free (error); + /* We do the same number of iterations on non-Unix, so that + * the method call count will match. In this case we use + * OpenFile both times, because the difference between this + * and OpenFileWithBigMessage is only relevant on Unix. */ + error = NULL; + result = g_dbus_proxy_call_sync (proxy, + "OpenFile", + g_variant_new ("(s)", "boo"), + G_DBUS_CALL_FLAGS_NONE, + -1, + NULL, /* GCancellable */ + &error); + g_assert_error (error, G_IO_ERROR, G_IO_ERROR_DBUS_ERROR); + g_assert (result == NULL); + g_error_free (error); #endif /* G_OS_UNIX */ + } /* Check that g_socket_get_credentials() work - (though this really * should be in socket.c) @@ -1017,7 +1088,7 @@ do_test_peer (void) g_variant_get (result, "(&s)", &s); g_assert_cmpstr (s, ==, "You greeted me with 'Hey Again Peer!'."); g_variant_unref (result); - g_assert_cmpint (data.num_method_calls, ==, 5); + g_assert_cmpint (data.num_method_calls, ==, 6); #if 0 /* TODO: THIS TEST DOESN'T WORK YET */