From fc597fa5f99810564c963c23cd37605d581fea89 Mon Sep 17 00:00:00 2001 From: Michael Catanzaro Date: Tue, 11 Jun 2019 22:35:19 -0500 Subject: [PATCH 1/7] gdbus: support unix:dir= addresses unix:dir= addresses are exactly the same as unix:tmpdir= addresses, already supported by GDBus, except they forbid use of abstract sockets. This is convenient for situations where abstract sockets are impermissible, such as when a D-Bus client inside a network namespace needs to connect to a server running in a different network namespace. An abstract socket cannot be shared between two processes in different network namespaces. Applications could use unix:path= addresses instead, so this is only a convenience, but there's no good reason not to support unix:dir=. Currently it is not supported simply because unix:dir= is a relatively recent addition to the D-Bus spec. --- gio/gdbusaddress.c | 13 ++++++++----- gio/gdbusserver.c | 13 ++++++++----- gio/tests/gdbus-addresses.c | 4 ++++ 3 files changed, 20 insertions(+), 10 deletions(-) diff --git a/gio/gdbusaddress.c b/gio/gdbusaddress.c index 1f7d19396..e88695139 100644 --- a/gio/gdbusaddress.c +++ b/gio/gdbusaddress.c @@ -127,12 +127,14 @@ is_valid_unix (const gchar *address_entry, GList *keys; GList *l; const gchar *path; + const gchar *dir; const gchar *tmpdir; const gchar *abstract; ret = FALSE; keys = NULL; path = NULL; + dir = NULL; tmpdir = NULL; abstract = NULL; @@ -142,6 +144,8 @@ is_valid_unix (const gchar *address_entry, const gchar *key = l->data; if (g_strcmp0 (key, "path") == 0) path = g_hash_table_lookup (key_value_pairs, key); + else if (g_strcmp0 (key, "dir") == 0) + dir = g_hash_table_lookup (key_value_pairs, key); else if (g_strcmp0 (key, "tmpdir") == 0) tmpdir = g_hash_table_lookup (key_value_pairs, key); else if (g_strcmp0 (key, "abstract") == 0) @@ -158,9 +162,8 @@ is_valid_unix (const gchar *address_entry, } } - if ((path != NULL && tmpdir != NULL) || - (tmpdir != NULL && abstract != NULL) || - (abstract != NULL && path != NULL)) + /* Exactly one key must be set */ + if ((path != NULL) + (dir != NULL) + (tmpdir != NULL) + (abstract != NULL) > 1) { g_set_error (error, G_IO_ERROR, @@ -169,12 +172,12 @@ is_valid_unix (const gchar *address_entry, address_entry); goto out; } - else if (path == NULL && tmpdir == NULL && abstract == NULL) + else if (path == NULL && dir == NULL && tmpdir == NULL && abstract == NULL) { g_set_error (error, G_IO_ERROR, G_IO_ERROR_INVALID_ARGUMENT, - _("Address “%s” is invalid (need exactly one of path, tmpdir or abstract keys)"), + _("Address “%s” is invalid (need exactly one of path, dir, tmpdir, or abstract keys)"), address_entry); goto out; } diff --git a/gio/gdbusserver.c b/gio/gdbusserver.c index b3459ce47..2f137ecca 100644 --- a/gio/gdbusserver.c +++ b/gio/gdbusserver.c @@ -641,7 +641,7 @@ random_ascii (void) return ret; } -/* note that address_entry has already been validated => exactly one of path, tmpdir or abstract keys are set */ +/* note that address_entry has already been validated => exactly one of path, dir, tmpdir, or abstract keys are set */ static gboolean try_unix (GDBusServer *server, const gchar *address_entry, @@ -650,6 +650,7 @@ try_unix (GDBusServer *server, { gboolean ret; const gchar *path; + const gchar *dir; const gchar *tmpdir; const gchar *abstract; GSocketAddress *address; @@ -658,6 +659,7 @@ try_unix (GDBusServer *server, address = NULL; path = g_hash_table_lookup (key_value_pairs, "path"); + dir = g_hash_table_lookup (key_value_pairs, "dir"); tmpdir = g_hash_table_lookup (key_value_pairs, "tmpdir"); abstract = g_hash_table_lookup (key_value_pairs, "abstract"); @@ -665,20 +667,21 @@ try_unix (GDBusServer *server, { address = g_unix_socket_address_new (path); } - else if (tmpdir != NULL) + else if (dir != NULL || tmpdir != NULL) { gint n; GString *s; GError *local_error; retry: - s = g_string_new (tmpdir); + s = g_string_new (tmpdir != NULL ? tmpdir : dir); g_string_append (s, "/dbus-"); for (n = 0; n < 8; n++) g_string_append_c (s, random_ascii ()); - /* prefer abstract namespace if available */ - if (g_unix_socket_address_abstract_names_supported ()) + /* prefer abstract namespace if available for tmpdir: addresses + * abstract namespace is disallowed for dir: addresses */ + if (tmpdir != NULL && g_unix_socket_address_abstract_names_supported ()) address = g_unix_socket_address_new_with_type (s->str, -1, G_UNIX_SOCKET_ADDRESS_ABSTRACT); diff --git a/gio/tests/gdbus-addresses.c b/gio/tests/gdbus-addresses.c index d6a5c2360..b7c81fc06 100644 --- a/gio/tests/gdbus-addresses.c +++ b/gio/tests/gdbus-addresses.c @@ -113,9 +113,13 @@ test_unix_address (void) g_assert_false (g_dbus_is_address ("unix:path=/foo;abstract=/bar")); assert_is_supported_address ("unix:path=/tmp/concrete;unix:abstract=/tmp/abstract"); assert_is_supported_address ("unix:tmpdir=/tmp"); + assert_is_supported_address ("unix:dir=/tmp"); assert_not_supported_address ("unix:tmpdir=/tmp,path=/tmp"); assert_not_supported_address ("unix:tmpdir=/tmp,abstract=/tmp/foo"); + assert_not_supported_address ("unix:tmpdir=/tmp,dir=/tmp"); assert_not_supported_address ("unix:path=/tmp,abstract=/tmp/foo"); + assert_not_supported_address ("unix:path=/tmp,dir=/tmp"); + assert_not_supported_address ("unix:abstract=/tmp/foo,dir=/tmp"); assert_not_supported_address ("unix:"); #endif } From 99b580a0b2b6da98ca9563df2309cddde988c005 Mon Sep 17 00:00:00 2001 From: Michael Catanzaro Date: Wed, 12 Jun 2019 13:29:52 -0500 Subject: [PATCH 2/7] gdbus: Stop server on dispose This is not going to have much any effect currently since stop() just disconnects a signal handler (that is going to be disconnected in finalize anyway) and stops the socket service (that is going to be destroyed in finalize), but it makes sense to do here for robustness. --- gio/gdbusserver.c | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/gio/gdbusserver.c b/gio/gdbusserver.c index 2f137ecca..1bdb8260c 100644 --- a/gio/gdbusserver.c +++ b/gio/gdbusserver.c @@ -161,6 +161,17 @@ static void initable_iface_init (GInitableIface *initable_iface); G_DEFINE_TYPE_WITH_CODE (GDBusServer, g_dbus_server, G_TYPE_OBJECT, G_IMPLEMENT_INTERFACE (G_TYPE_INITABLE, initable_iface_init)) +static void +g_dbus_server_dispose (GObject *object) +{ + GDBusServer *server = G_DBUS_SERVER (object); + + if (server->active) + g_dbus_server_stop (server); + + G_OBJECT_CLASS (g_dbus_server_parent_class)->dispose (object); +} + static void g_dbus_server_finalize (GObject *object) { @@ -270,6 +281,7 @@ g_dbus_server_class_init (GDBusServerClass *klass) { GObjectClass *gobject_class = G_OBJECT_CLASS (klass); + gobject_class->dispose = g_dbus_server_dispose; gobject_class->finalize = g_dbus_server_finalize; gobject_class->set_property = g_dbus_server_set_property; gobject_class->get_property = g_dbus_server_get_property; From f5631ecb942744cea0db9260ff4ac82e695f1463 Mon Sep 17 00:00:00 2001 From: Michael Catanzaro Date: Wed, 12 Jun 2019 13:38:51 -0500 Subject: [PATCH 3/7] gdbus: improve an error message Namespace is one word. --- gio/gdbusserver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gio/gdbusserver.c b/gio/gdbusserver.c index 1bdb8260c..d504f6251 100644 --- a/gio/gdbusserver.c +++ b/gio/gdbusserver.c @@ -728,7 +728,7 @@ try_unix (GDBusServer *server, g_set_error_literal (error, G_IO_ERROR, G_IO_ERROR_NOT_SUPPORTED, - _("Abstract name space not supported")); + _("Abstract namespace not supported")); goto out; } address = g_unix_socket_address_new_with_type (abstract, From beac9fe211c1024622433700c138cde677f2054f Mon Sep 17 00:00:00 2001 From: Michael Catanzaro Date: Wed, 12 Jun 2019 13:55:06 -0500 Subject: [PATCH 4/7] gdbus: Clean up sockets and nonces from filesystem When we close the GDBusServer, it should remove any non-abstract Unix sockets or TCP nonce files it created from the filesystem. Fixes #1808 --- gio/gdbusserver.c | 28 +++++++++++++++++++++------- gio/tests/gdbus-peer.c | 6 +++++- 2 files changed, 26 insertions(+), 8 deletions(-) diff --git a/gio/gdbusserver.c b/gio/gdbusserver.c index d504f6251..6bb4b1a4c 100644 --- a/gio/gdbusserver.c +++ b/gio/gdbusserver.c @@ -101,6 +101,7 @@ struct _GDBusServer gchar *client_address; + gchar *unix_socket_path; GSocketListener *listener; gboolean is_using_listener; gulong run_signal_handler_id; @@ -194,10 +195,20 @@ g_dbus_server_finalize (GObject *object) memset (server->nonce, '\0', 16); g_free (server->nonce); } - /* we could unlink the nonce file but I don't - * think it's really worth the effort/risk - */ - g_free (server->nonce_file); + + if (server->unix_socket_path) + { + if (g_unlink (server->unix_socket_path) != 0) + g_warning ("Failed to delete %s: %s", server->unix_socket_path, g_strerror (errno)); + g_free (server->unix_socket_path); + } + + if (server->nonce_file) + { + if (g_unlink (server->nonce_file) != 0) + g_warning ("Failed to delete %s: %s", server->nonce_file, g_strerror (errno)); + g_free (server->nonce_file); + } g_main_context_unref (server->main_context_at_construction); @@ -768,9 +779,12 @@ try_unix (GDBusServer *server, break; case G_UNIX_SOCKET_ADDRESS_PATH: - server->client_address = g_strdup_printf ("unix:path=%s", - g_unix_socket_address_get_path (G_UNIX_SOCKET_ADDRESS (address))); - break; + { + const char *address_path = g_unix_socket_address_get_path (G_UNIX_SOCKET_ADDRESS (address)); + server->client_address = g_strdup_printf ("unix:path=%s", address_path); + server->unix_socket_path = g_strdup (address_path); + break; + } default: g_assert_not_reached (); diff --git a/gio/tests/gdbus-peer.c b/gio/tests/gdbus-peer.c index c21b9e9f2..3a6c94829 100644 --- a/gio/tests/gdbus-peer.c +++ b/gio/tests/gdbus-peer.c @@ -1351,12 +1351,16 @@ test_nonce_tcp (void) g_error_free (error); g_assert (c == NULL); - g_free (nonce_file); + /* Recreate the nonce-file so we can ensure the server deletes it when stopped. */ + g_assert_cmpint (g_creat (nonce_file, 0600), !=, -1); g_dbus_server_stop (server); g_object_unref (server); server = NULL; + g_assert_false (g_file_test (nonce_file, G_FILE_TEST_EXISTS)); + g_free (nonce_file); + g_main_loop_quit (service_loop); g_thread_join (service_thread); From eca16677c0b576f6a403648d97faa79e1f8b0d3a Mon Sep 17 00:00:00 2001 From: Michael Catanzaro Date: Wed, 12 Jun 2019 14:49:34 -0500 Subject: [PATCH 5/7] gdbus: Fix minor leak in peer test This has to be freed even on Windows. --- gio/tests/gdbus-peer.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/gio/tests/gdbus-peer.c b/gio/tests/gdbus-peer.c index 3a6c94829..0e0183923 100644 --- a/gio/tests/gdbus-peer.c +++ b/gio/tests/gdbus-peer.c @@ -1773,8 +1773,7 @@ main (int argc, g_main_loop_unref (loop); g_free (test_guid); g_dbus_node_info_unref (introspection_data); - if (is_unix) - g_free (tmp_address); + g_free (tmp_address); if (tmpdir) { g_rmdir (tmpdir); From 16cdda5d35b609666225a906e1c07b01c08ea197 Mon Sep 17 00:00:00 2001 From: Michael Catanzaro Date: Wed, 12 Jun 2019 16:11:04 -0500 Subject: [PATCH 6/7] gdbus: run peer test multiple times with different addresses This ensures that D-Bus connections established with unix:dir and unix:path addresses actually work properly. Previously, we only tested unix:tmpdir and TCP addresses. --- gio/tests/gdbus-peer.c | 103 ++++++++++++++++++++++++++++++++--------- 1 file changed, 82 insertions(+), 21 deletions(-) diff --git a/gio/tests/gdbus-peer.c b/gio/tests/gdbus-peer.c index 0e0183923..c46340386 100644 --- a/gio/tests/gdbus-peer.c +++ b/gio/tests/gdbus-peer.c @@ -53,6 +53,7 @@ static gboolean is_unix = TRUE; static gboolean is_unix = FALSE; #endif +static gchar *tmpdir = NULL; static gchar *tmp_address = NULL; static gchar *test_guid = NULL; static GMutex service_loop_lock; @@ -267,6 +268,58 @@ on_proxy_signal_received_with_name_set (GDBusProxy *proxy, /* ---------------------------------------------------------------------------------------------------- */ +static void +setup_test_address (void) +{ + if (is_unix) + { + g_test_message ("Testing with unix:tmpdir address"); + if (g_unix_socket_address_abstract_names_supported ()) + tmp_address = g_strdup ("unix:tmpdir=/tmp/gdbus-test-"); + else + { + tmpdir = g_dir_make_tmp ("gdbus-test-XXXXXX", NULL); + tmp_address = g_strdup_printf ("unix:tmpdir=%s", tmpdir); + } + } + else + tmp_address = g_strdup ("nonce-tcp:"); +} + +#ifdef G_OS_UNIX +static void +setup_dir_test_address (void) +{ + g_test_message ("Testing with unix:dir address"); + tmpdir = g_dir_make_tmp ("gdbus-test-XXXXXX", NULL); + tmp_address = g_strdup_printf ("unix:dir=%s", tmpdir); +} + +static void +setup_path_test_address (void) +{ + g_test_message ("Testing with unix:path address"); + tmpdir = g_dir_make_tmp ("gdbus-test-XXXXXX", NULL); + tmp_address = g_strdup_printf ("unix:path=%s/gdbus-peer-socket", tmpdir); +} +#endif + +static void +teardown_test_address (void) +{ + g_free (tmp_address); + if (tmpdir) + { + /* Ensuring the rmdir succeeds also ensures any sockets created on the + * filesystem are also deleted. + */ + g_assert_cmpint (g_rmdir (tmpdir), ==, 0); + g_clear_pointer (&tmpdir, g_free); + } +} + +/* ---------------------------------------------------------------------------------------------------- */ + static gboolean on_authorize_authenticated_peer (GDBusAuthObserver *observer, GIOStream *stream, @@ -656,7 +709,7 @@ read_all_from_fd (gint fd, gsize *out_len, GError **error) #endif static void -test_peer (void) +do_test_peer (void) { GDBusConnection *c; GDBusConnection *c2; @@ -977,6 +1030,27 @@ test_peer (void) g_thread_join (service_thread); } +static void +test_peer (void) +{ + /* Run this test multiple times using different address formats to ensure + * they all work. + */ + setup_test_address (); + do_test_peer (); + teardown_test_address (); + +#ifdef G_OS_UNIX + setup_dir_test_address (); + do_test_peer (); + teardown_test_address (); + + setup_path_test_address (); + do_test_peer (); + teardown_test_address (); +#endif +} + /* ---------------------------------------------------------------------------------------------------- */ typedef struct @@ -1116,6 +1190,8 @@ delayed_message_processing (void) GThread *service_thread; guint n; + setup_test_address (); + data = g_new0 (DmpData, 1); service_thread = g_thread_new ("dmp", @@ -1160,6 +1236,7 @@ delayed_message_processing (void) g_main_loop_quit (data->loop); g_thread_join (service_thread); dmp_data_free (data); + teardown_test_address (); } /* ---------------------------------------------------------------------------------------------------- */ @@ -1620,6 +1697,8 @@ codegen_test_peer (void) GVariant *value; const gchar *s; + setup_test_address (); + /* bring up a server - we run the server in a different thread to avoid deadlocks */ service_thread = g_thread_new ("codegen_test_peer", codegen_service_thread_func, @@ -1723,6 +1802,8 @@ codegen_test_peer (void) g_object_unref (animal1); g_object_unref (animal2); g_thread_join (service_thread); + + teardown_test_address (); } /* ---------------------------------------------------------------------------------------------------- */ @@ -1734,7 +1815,6 @@ main (int argc, { gint ret; GDBusNodeInfo *introspection_data = NULL; - gchar *tmpdir = NULL; g_test_init (&argc, &argv, NULL); @@ -1744,19 +1824,6 @@ main (int argc, test_guid = g_dbus_generate_guid (); - if (is_unix) - { - if (g_unix_socket_address_abstract_names_supported ()) - tmp_address = g_strdup ("unix:tmpdir=/tmp/gdbus-test-"); - else - { - tmpdir = g_dir_make_tmp ("gdbus-test-XXXXXX", NULL); - tmp_address = g_strdup_printf ("unix:tmpdir=%s", tmpdir); - } - } - else - tmp_address = g_strdup ("nonce-tcp:"); - /* all the tests rely on a shared main loop */ loop = g_main_loop_new (NULL, FALSE); @@ -1773,12 +1840,6 @@ main (int argc, g_main_loop_unref (loop); g_free (test_guid); g_dbus_node_info_unref (introspection_data); - g_free (tmp_address); - if (tmpdir) - { - g_rmdir (tmpdir); - g_free (tmpdir); - } return ret; } From 30524fbdb5260523e7afdf2f30933f62e8e641a9 Mon Sep 17 00:00:00 2001 From: Michael Catanzaro Date: Thu, 13 Jun 2019 13:04:17 -0500 Subject: [PATCH 7/7] gdbusserver: properly escape all components of server address https://gitlab.gnome.org/GNOME/glib/merge_requests/911#note_530668 --- gio/gdbusserver.c | 26 ++++++++++++++++---------- 1 file changed, 16 insertions(+), 10 deletions(-) diff --git a/gio/gdbusserver.c b/gio/gdbusserver.c index 6bb4b1a4c..26f75d4b3 100644 --- a/gio/gdbusserver.c +++ b/gio/gdbusserver.c @@ -769,27 +769,30 @@ try_unix (GDBusServer *server, /* Fill out client_address if the connection attempt worked */ if (ret) { + const char *address_path; + char *escaped_path; + server->is_using_listener = TRUE; + address_path = g_unix_socket_address_get_path (G_UNIX_SOCKET_ADDRESS (address)); + escaped_path = g_dbus_address_escape_value (address_path); switch (g_unix_socket_address_get_address_type (G_UNIX_SOCKET_ADDRESS (address))) { case G_UNIX_SOCKET_ADDRESS_ABSTRACT: - server->client_address = g_strdup_printf ("unix:abstract=%s", - g_unix_socket_address_get_path (G_UNIX_SOCKET_ADDRESS (address))); + server->client_address = g_strdup_printf ("unix:abstract=%s", escaped_path); break; case G_UNIX_SOCKET_ADDRESS_PATH: - { - const char *address_path = g_unix_socket_address_get_path (G_UNIX_SOCKET_ADDRESS (address)); - server->client_address = g_strdup_printf ("unix:path=%s", address_path); - server->unix_socket_path = g_strdup (address_path); - break; - } + server->client_address = g_strdup_printf ("unix:path=%s", escaped_path); + server->unix_socket_path = g_strdup (address_path); + break; default: g_assert_not_reached (); break; } + + g_free (escaped_path); } g_object_unref (address); } @@ -881,6 +884,7 @@ try_tcp (GDBusServer *server, gsize bytes_written; gsize bytes_remaining; char *file_escaped; + char *host_escaped; server->nonce = g_new0 (guchar, 16); for (n = 0; n < 16; n++) @@ -920,11 +924,13 @@ try_tcp (GDBusServer *server, } if (!g_close (fd, error)) goto out; - file_escaped = g_uri_escape_string (server->nonce_file, "/\\", FALSE); + host_escaped = g_dbus_address_escape_value (host); + file_escaped = g_dbus_address_escape_value (server->nonce_file); server->client_address = g_strdup_printf ("nonce-tcp:host=%s,port=%d,noncefile=%s", - host, + host_escaped, port_num, file_escaped); + g_free (host_escaped); g_free (file_escaped); } else