From ef3eec8a2876691a71574d871668283ae8d379b7 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Mon, 28 Oct 2019 19:13:50 +0000 Subject: [PATCH 1/4] gdbusauthmechanismsha1: Remove unnecessary g_warning() calls These can be hit in the tests (if multiple tests run in parallel are racing for `~/.dbus-keyrings/org_gtk_gdbus_general.lock` for a prolonged period) and will cause spurious test failures due to the use of `G_DEBUG=fatal-warnings`. Instead, allow the error messages to be inspected programmatically. Signed-off-by: Philip Withnall Helps: #1912 --- gio/gdbusauthmechanismsha1.c | 24 +++++++++++++++--------- 1 file changed, 15 insertions(+), 9 deletions(-) diff --git a/gio/gdbusauthmechanismsha1.c b/gio/gdbusauthmechanismsha1.c index 553bcdc32..d29048ad7 100644 --- a/gio/gdbusauthmechanismsha1.c +++ b/gio/gdbusauthmechanismsha1.c @@ -48,6 +48,7 @@ struct _GDBusAuthMechanismSha1Private gboolean is_client; gboolean is_server; GDBusAuthMechanismState state; + gchar *reject_reason; /* non-NULL iff (state == G_DBUS_AUTH_MECHANISM_STATE_REJECTED) */ /* used on the client side */ gchar *to_send; @@ -101,6 +102,7 @@ _g_dbus_auth_mechanism_sha1_finalize (GObject *object) { GDBusAuthMechanismSha1 *mechanism = G_DBUS_AUTH_MECHANISM_SHA1 (object); + g_free (mechanism->priv->reject_reason); g_free (mechanism->priv->to_send); g_free (mechanism->priv->cookie); @@ -963,7 +965,8 @@ mechanism_server_data_receive (GDBusAuthMechanism *mechanism, tokens = g_strsplit (data, " ", 0); if (g_strv_length (tokens) != 2) { - g_warning ("Malformed data '%s'", data); + g_free (m->priv->reject_reason); + m->priv->reject_reason = g_strdup_printf ("Malformed data '%s'", data); m->priv->state = G_DBUS_AUTH_MECHANISM_STATE_REJECTED; goto out; } @@ -979,6 +982,8 @@ mechanism_server_data_receive (GDBusAuthMechanism *mechanism, } else { + g_free (m->priv->reject_reason); + m->priv->reject_reason = g_strdup_printf ("SHA-1 mismatch"); m->priv->state = G_DBUS_AUTH_MECHANISM_STATE_REJECTED; } @@ -1014,7 +1019,8 @@ mechanism_server_data_send (GDBusAuthMechanism *mechanism, &m->priv->cookie, &error)) { - g_warning ("Error adding entry to keyring: %s", error->message); + g_free (m->priv->reject_reason); + m->priv->reject_reason = g_strdup_printf ("Error adding entry to keyring: %s", error->message); g_error_free (error); m->priv->state = G_DBUS_AUTH_MECHANISM_STATE_REJECTED; goto out; @@ -1042,10 +1048,7 @@ mechanism_server_get_reject_reason (GDBusAuthMechanism *mechanism) g_return_val_if_fail (m->priv->is_server && !m->priv->is_client, NULL); g_return_val_if_fail (m->priv->state == G_DBUS_AUTH_MECHANISM_STATE_REJECTED, NULL); - /* can never end up here because we are never in the REJECTED state */ - g_assert_not_reached (); - - return NULL; + return g_strdup (m->priv->reject_reason); } static void @@ -1128,7 +1131,8 @@ mechanism_client_data_receive (GDBusAuthMechanism *mechanism, tokens = g_strsplit (data, " ", 0); if (g_strv_length (tokens) != 3) { - g_warning ("Malformed data '%s'", data); + g_free (m->priv->reject_reason); + m->priv->reject_reason = g_strdup_printf ("Malformed data '%s'", data); m->priv->state = G_DBUS_AUTH_MECHANISM_STATE_REJECTED; goto out; } @@ -1137,7 +1141,8 @@ mechanism_client_data_receive (GDBusAuthMechanism *mechanism, cookie_id = g_ascii_strtoll (tokens[1], &endp, 10); if (*endp != '\0') { - g_warning ("Malformed cookie_id '%s'", tokens[1]); + g_free (m->priv->reject_reason); + m->priv->reject_reason = g_strdup_printf ("Malformed cookie_id '%s'", tokens[1]); m->priv->state = G_DBUS_AUTH_MECHANISM_STATE_REJECTED; goto out; } @@ -1147,7 +1152,8 @@ mechanism_client_data_receive (GDBusAuthMechanism *mechanism, cookie = keyring_lookup_entry (cookie_context, cookie_id, &error); if (cookie == NULL) { - g_warning ("Problems looking up entry in keyring: %s", error->message); + g_free (m->priv->reject_reason); + m->priv->reject_reason = g_strdup_printf ("Problems looking up entry in keyring: %s", error->message); g_error_free (error); m->priv->state = G_DBUS_AUTH_MECHANISM_STATE_REJECTED; goto out; From 9df8d76c972de7cad6e62f7fbdda043ce86b8cc7 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Mon, 28 Oct 2019 19:40:26 +0000 Subject: [PATCH 2/4] gdbusauthmechanismsha1: Create .dbus-keyrings directory recursively If the directory is overridden, for example when running tests, the parent directory of `.dbus-keyrings` (i.e. the fake `$HOME` directory) might not exist. Create it automatically. This should realistically not have an effect on non-test code. Signed-off-by: Philip Withnall Helps: #1912 --- gio/gdbusauthmechanismsha1.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gio/gdbusauthmechanismsha1.c b/gio/gdbusauthmechanismsha1.c index d29048ad7..2754d3c2b 100644 --- a/gio/gdbusauthmechanismsha1.c +++ b/gio/gdbusauthmechanismsha1.c @@ -292,7 +292,7 @@ ensure_keyring_directory (GError **error) goto out; } - if (g_mkdir (path, 0700) != 0) + if (g_mkdir_with_parents (path, 0700) != 0) { int errsv = errno; g_set_error (error, From 833579d982e23d440210e82580fe920cacd24be9 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Mon, 28 Oct 2019 19:41:45 +0000 Subject: [PATCH 3/4] tests: Move main loop and test GUID into test functions in gdbus-peer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit There’s actually no need for them to be global or reused between unit tests, so move them inside the test functions. This is one step towards eliminating shared state between the unit tests. Signed-off-by: Philip Withnall Helps: #1912 --- gio/tests/gdbus-peer.c | 45 ++++++++++++++++++++++++++++++++++-------- 1 file changed, 37 insertions(+), 8 deletions(-) diff --git a/gio/tests/gdbus-peer.c b/gio/tests/gdbus-peer.c index a3260a6e0..b410e80d1 100644 --- a/gio/tests/gdbus-peer.c +++ b/gio/tests/gdbus-peer.c @@ -1033,6 +1033,9 @@ do_test_peer (void) static void test_peer (void) { + test_guid = g_dbus_generate_guid (); + loop = g_main_loop_new (NULL, FALSE); + /* Run this test multiple times using different address formats to ensure * they all work. */ @@ -1049,6 +1052,9 @@ test_peer (void) do_test_peer (); teardown_test_address (); #endif + + g_main_loop_unref (loop); + g_free (test_guid); } /* ---------------------------------------------------------------------------------------------------- */ @@ -1064,6 +1070,9 @@ test_peer_signals (void) g_test_bug ("https://gitlab.gnome.org/GNOME/glib/issues/1620"); + test_guid = g_dbus_generate_guid (); + loop = g_main_loop_new (NULL, FALSE); + setup_test_address (); memset (&data, '\0', sizeof (PeerData)); data.current_connections = g_ptr_array_new_with_free_func (g_object_unref); @@ -1119,6 +1128,9 @@ test_peer_signals (void) g_thread_join (service_thread); teardown_test_address (); + + g_main_loop_unref (loop); + g_free (test_guid); } /* ---------------------------------------------------------------------------------------------------- */ @@ -1260,6 +1272,9 @@ delayed_message_processing (void) GThread *service_thread; guint n; + test_guid = g_dbus_generate_guid (); + loop = g_main_loop_new (NULL, FALSE); + setup_test_address (); data = g_new0 (DmpData, 1); @@ -1307,6 +1322,9 @@ delayed_message_processing (void) g_thread_join (service_thread); dmp_data_free (data); teardown_test_address (); + + g_main_loop_unref (loop); + g_free (test_guid); } /* ---------------------------------------------------------------------------------------------------- */ @@ -1405,6 +1423,9 @@ test_nonce_tcp (void) gboolean res; const gchar *address; + test_guid = g_dbus_generate_guid (); + loop = g_main_loop_new (NULL, FALSE); + memset (&data, '\0', sizeof (PeerData)); data.current_connections = g_ptr_array_new_with_free_func (g_object_unref); @@ -1512,6 +1533,9 @@ test_nonce_tcp (void) g_thread_join (service_thread); g_ptr_array_unref (data.current_connections); + + g_main_loop_unref (loop); + g_free (test_guid); } static void @@ -1596,6 +1620,9 @@ test_tcp_anonymous (void) GDBusConnection *connection; GError *error; + test_guid = g_dbus_generate_guid (); + loop = g_main_loop_new (NULL, FALSE); + seen_connection = FALSE; service_thread = g_thread_new ("tcp-anon-service", tcp_anonymous_service_thread_func, @@ -1623,6 +1650,9 @@ test_tcp_anonymous (void) server = NULL; g_thread_join (service_thread); + + g_main_loop_unref (loop); + g_free (test_guid); } /* ---------------------------------------------------------------------------------------------------- */ @@ -1767,6 +1797,9 @@ codegen_test_peer (void) GVariant *value; const gchar *s; + test_guid = g_dbus_generate_guid (); + loop = g_main_loop_new (NULL, FALSE); + setup_test_address (); /* bring up a server - we run the server in a different thread to avoid deadlocks */ @@ -1874,6 +1907,9 @@ codegen_test_peer (void) g_thread_join (service_thread); teardown_test_address (); + + g_main_loop_unref (loop); + g_free (test_guid); } /* ---------------------------------------------------------------------------------------------------- */ @@ -1892,11 +1928,6 @@ main (int argc, g_assert (introspection_data != NULL); test_interface_introspection_data = introspection_data->interfaces[0]; - test_guid = g_dbus_generate_guid (); - - /* all the tests rely on a shared main loop */ - loop = g_main_loop_new (NULL, FALSE); - g_test_add_func ("/gdbus/peer-to-peer", test_peer); g_test_add_func ("/gdbus/peer-to-peer/signals", test_peer_signals); g_test_add_func ("/gdbus/delayed-message-processing", delayed_message_processing); @@ -1906,10 +1937,8 @@ main (int argc, g_test_add_func ("/gdbus/credentials", test_credentials); g_test_add_func ("/gdbus/codegen-peer-to-peer", codegen_test_peer); - ret = g_test_run(); + ret = g_test_run (); - g_main_loop_unref (loop); - g_free (test_guid); g_dbus_node_info_unref (introspection_data); return ret; From 6fb38c3f25e8e859812e25fe88fad752a68133b3 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Mon, 28 Oct 2019 19:42:41 +0000 Subject: [PATCH 4/4] tests: Isolate directories in gdbus-peer test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit So that the tests all end up using separate `.dbus-keyring` directories, and hence not racing to create and acquire lock files, use `G_TEST_OPTION_ISOLATE_DIRS` to ensure they all run in separate disposable directories. This has the added benefit of meaning they don’t touch the developer’s actual `$HOME` directory. This reduces the false-failure rate of `gdbus-peer` by a factor of 9 for me on my local machine. Signed-off-by: Philip Withnall Fixes: #1912 --- gio/tests/gdbus-peer.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gio/tests/gdbus-peer.c b/gio/tests/gdbus-peer.c index b410e80d1..dc4176cf2 100644 --- a/gio/tests/gdbus-peer.c +++ b/gio/tests/gdbus-peer.c @@ -1922,7 +1922,7 @@ main (int argc, gint ret; GDBusNodeInfo *introspection_data = NULL; - g_test_init (&argc, &argv, NULL); + g_test_init (&argc, &argv, G_TEST_OPTION_ISOLATE_DIRS, NULL); introspection_data = g_dbus_node_info_new_for_xml (test_interface_introspection_xml, NULL); g_assert (introspection_data != NULL);