Merge branch '2400-dbus-race' into 'main'

gdbusconnection: Fix race between method calls and object unregistration

Closes #2400

See merge request GNOME/glib!2265

gio/gdbusconnection.c

@@ -466,7 +466,8 @@ typedef struct ExportedObject ExportedObject;
 static void exported_object_free (ExportedObject *eo);
 
 typedef struct ExportedSubtree ExportedSubtree;
-static void exported_subtree_free (ExportedSubtree *es);
+static ExportedSubtree *exported_subtree_ref (ExportedSubtree *es);
+static void exported_subtree_unref (ExportedSubtree *es);
 
 enum
 {
@@ -1096,7 +1097,7 @@ g_dbus_connection_init (GDBusConnection *connection)
   connection->map_object_path_to_es = g_hash_table_new_full (g_str_hash,
                                                              g_str_equal,
                                                              NULL,
-                                                             (GDestroyNotify) exported_subtree_free);
+                                                             (GDestroyNotify) exported_subtree_unref);
 
   connection->map_id_to_es = g_hash_table_new (g_direct_hash,
                                                g_direct_equal);
@@ -4085,23 +4086,39 @@ typedef struct
 {
   ExportedObject *eo;
 
-  guint                       id;
+  gint                        refcount;  /* (atomic) */
-  gchar                      *interface_name;
-  GDBusInterfaceVTable       *vtable;
-  GDBusInterfaceInfo         *interface_info;
 
-  GMainContext               *context;
+  guint                       id;
+  gchar                      *interface_name;  /* (owned) */
+  GDBusInterfaceVTable       *vtable;  /* (owned) */
+  GDBusInterfaceInfo         *interface_info;  /* (owned) */
+
+  GMainContext               *context;  /* (owned) */
   gpointer                    user_data;
   GDestroyNotify              user_data_free_func;
 } ExportedInterface;
 
-/* called with lock held */
+static ExportedInterface *
-static void
+exported_interface_ref (ExportedInterface *ei)
-exported_interface_free (ExportedInterface *ei)
 {
+  g_atomic_int_inc (&ei->refcount);
+
+  return ei;
+}
+
+/* May be called with lock held */
+static void
+exported_interface_unref (ExportedInterface *ei)
+{
+  if (!g_atomic_int_dec_and_test (&ei->refcount))
+    return;
+
   g_dbus_interface_info_cache_release (ei->interface_info);
   g_dbus_interface_info_unref ((GDBusInterfaceInfo *) ei->interface_info);
 
+  /* All uses of ei->vtable from callbacks scheduled in idle functions must
+   * have completed by this call_destroy_notify() call, as language bindings
+   * may destroy function closures in this callback. */
   call_destroy_notify (ei->context,
                        ei->user_data_free_func,
                        ei->user_data);
@@ -4113,36 +4130,95 @@ exported_interface_free (ExportedInterface *ei)
   g_free (ei);
 }
 
+struct ExportedSubtree
+{
+  gint                      refcount;  /* (atomic) */
+
+  guint                     id;
+  gchar                    *object_path;  /* (owned) */
+  GDBusConnection          *connection;  /* (unowned) */
+  GDBusSubtreeVTable       *vtable;  /* (owned) */
+  GDBusSubtreeFlags         flags;
+
+  GMainContext             *context;  /* (owned) */
+  gpointer                  user_data;
+  GDestroyNotify            user_data_free_func;
+};
+
+static ExportedSubtree *
+exported_subtree_ref (ExportedSubtree *es)
+{
+  g_atomic_int_inc (&es->refcount);
+
+  return es;
+}
+
+/* May be called with lock held */
+static void
+exported_subtree_unref (ExportedSubtree *es)
+{
+  if (!g_atomic_int_dec_and_test (&es->refcount))
+    return;
+
+  /* All uses of es->vtable from callbacks scheduled in idle functions must
+   * have completed by this call_destroy_notify() call, as language bindings
+   * may destroy function closures in this callback. */
+  call_destroy_notify (es->context,
+                       es->user_data_free_func,
+                       es->user_data);
+
+  g_main_context_unref (es->context);
+
+  _g_dbus_subtree_vtable_free (es->vtable);
+  g_free (es->object_path);
+  g_free (es);
+}
+
 /* ---------------------------------------------------------------------------------------------------- */
 
 /* Convenience function to check if @registration_id (if not zero) or
  * @subtree_registration_id (if not zero) has been unregistered. If
  * so, returns %TRUE.
  *
+ * If not, sets @out_ei and/or @out_es to a strong reference to the relevant
+ * #ExportedInterface/#ExportedSubtree and returns %FALSE.
+ *
  * May be called by any thread. Caller must *not* hold lock.
  */
 static gboolean
-has_object_been_unregistered (GDBusConnection  *connection,
+has_object_been_unregistered (GDBusConnection    *connection,
-                              guint             registration_id,
+                              guint               registration_id,
-                              guint             subtree_registration_id)
+                              ExportedInterface **out_ei,
+                              guint               subtree_registration_id,
+                              ExportedSubtree   **out_es)
 {
   gboolean ret;
+  ExportedInterface *ei = NULL;
+  gpointer es = NULL;
 
   g_return_val_if_fail (G_IS_DBUS_CONNECTION (connection), FALSE);
 
   ret = FALSE;
 
   CONNECTION_LOCK (connection);
-  if (registration_id != 0 && g_hash_table_lookup (connection->map_id_to_ei,
+
-                                                   GUINT_TO_POINTER (registration_id)) == NULL)
+  if (registration_id != 0)
     {
-      ret = TRUE;
+      ei = g_hash_table_lookup (connection->map_id_to_ei, GUINT_TO_POINTER (registration_id));
+      if (ei == NULL)
+        ret = TRUE;
+      else if (out_ei != NULL)
+        *out_ei = exported_interface_ref (ei);
     }
-  else if (subtree_registration_id != 0 && g_hash_table_lookup (connection->map_id_to_es,
+  if (subtree_registration_id != 0)
-                                                                GUINT_TO_POINTER (subtree_registration_id)) == NULL)
     {
-      ret = TRUE;
+      es = g_hash_table_lookup (connection->map_id_to_es, GUINT_TO_POINTER (subtree_registration_id));
+      if (es == NULL)
+        ret = TRUE;
+      else if (out_es != NULL)
+        *out_es = exported_subtree_ref (es);
     }
+
   CONNECTION_UNLOCK (connection);
 
   return ret;
@@ -4179,10 +4255,14 @@ invoke_get_property_in_idle_cb (gpointer _data)
   GVariant *value;
   GError *error;
   GDBusMessage *reply;
+  ExportedInterface *ei = NULL;
+  ExportedSubtree *es = NULL;
 
   if (has_object_been_unregistered (data->connection,
                                     data->registration_id,
-                                    data->subtree_registration_id))
+                                    &ei,
+                                    data->subtree_registration_id,
+                                    &es))
     {
       reply = g_dbus_message_new_method_error (data->message,
                                                "org.freedesktop.DBus.Error.UnknownMethod",
@@ -4229,6 +4309,9 @@ invoke_get_property_in_idle_cb (gpointer _data)
     }
 
  out:
+  g_clear_pointer (&ei, exported_interface_unref);
+  g_clear_pointer (&es, exported_subtree_unref);
+
   return FALSE;
 }
 
@@ -4526,10 +4609,14 @@ invoke_get_all_properties_in_idle_cb (gpointer _data)
   GVariantBuilder builder;
   GDBusMessage *reply;
   guint n;
+  ExportedInterface *ei = NULL;
+  ExportedSubtree *es = NULL;
 
   if (has_object_been_unregistered (data->connection,
                                     data->registration_id,
-                                    data->subtree_registration_id))
+                                    &ei,
+                                    data->subtree_registration_id,
+                                    &es))
     {
       reply = g_dbus_message_new_method_error (data->message,
                                                "org.freedesktop.DBus.Error.UnknownMethod",
@@ -4582,6 +4669,9 @@ invoke_get_all_properties_in_idle_cb (gpointer _data)
   g_object_unref (reply);
 
  out:
+  g_clear_pointer (&ei, exported_interface_unref);
+  g_clear_pointer (&es, exported_subtree_unref);
+
   return FALSE;
 }
 
@@ -4891,13 +4981,17 @@ call_in_idle_cb (gpointer user_data)
   GDBusInterfaceVTable *vtable;
   guint registration_id;
   guint subtree_registration_id;
+  ExportedInterface *ei = NULL;
+  ExportedSubtree *es = NULL;
 
   registration_id = GPOINTER_TO_UINT (g_object_get_data (G_OBJECT (invocation), "g-dbus-registration-id"));
   subtree_registration_id = GPOINTER_TO_UINT (g_object_get_data (G_OBJECT (invocation), "g-dbus-subtree-registration-id"));
 
   if (has_object_been_unregistered (g_dbus_method_invocation_get_connection (invocation),
                                     registration_id,
-                                    subtree_registration_id))
+                                    &ei,
+                                    subtree_registration_id,
+                                    &es))
     {
       GDBusMessage *reply;
       reply = g_dbus_message_new_method_error (g_dbus_method_invocation_get_message (invocation),
@@ -4923,6 +5017,9 @@ call_in_idle_cb (gpointer user_data)
                        g_dbus_method_invocation_get_user_data (invocation));
 
  out:
+  g_clear_pointer (&ei, exported_interface_unref);
+  g_clear_pointer (&es, exported_subtree_unref);
+
   return FALSE;
 }
 
@@ -5224,7 +5321,7 @@ g_dbus_connection_register_object (GDBusConnection             *connection,
       eo->map_if_name_to_ei = g_hash_table_new_full (g_str_hash,
                                                      g_str_equal,
                                                      NULL,
-                                                     (GDestroyNotify) exported_interface_free);
+                                                     (GDestroyNotify) exported_interface_unref);
       g_hash_table_insert (connection->map_object_path_to_eo, eo->object_path, eo);
     }
 
@@ -5241,6 +5338,7 @@ g_dbus_connection_register_object (GDBusConnection             *connection,
     }
 
   ei = g_new0 (ExportedInterface, 1);
+  ei->refcount = 1;
   ei->id = (guint) g_atomic_int_add (&_global_registration_id, 1); /* TODO: overflow etc. */
   ei->eo = eo;
   ei->user_data = user_data;
@@ -6401,33 +6499,6 @@ g_dbus_connection_call_with_unix_fd_list_sync (GDBusConnection     *connection,
 
 /* ---------------------------------------------------------------------------------------------------- */
 
-struct ExportedSubtree
-{
-  guint                     id;
-  gchar                    *object_path;
-  GDBusConnection          *connection;
-  GDBusSubtreeVTable       *vtable;
-  GDBusSubtreeFlags         flags;
-
-  GMainContext             *context;
-  gpointer                  user_data;
-  GDestroyNotify            user_data_free_func;
-};
-
-static void
-exported_subtree_free (ExportedSubtree *es)
-{
-  call_destroy_notify (es->context,
-                       es->user_data_free_func,
-                       es->user_data);
-
-  g_main_context_unref (es->context);
-
-  _g_dbus_subtree_vtable_free (es->vtable);
-  g_free (es->object_path);
-  g_free (es);
-}
-
 /* called without lock held in the thread where the caller registered
  * the subtree
  */
@@ -6753,14 +6824,15 @@ handle_subtree_method_invocation (GDBusConnection *connection,
 
 typedef struct
 {
-  GDBusMessage *message;
+  GDBusMessage *message;  /* (owned) */
-  ExportedSubtree *es;
+  ExportedSubtree *es;  /* (owned) */
 } SubtreeDeferredData;
 
 static void
 subtree_deferred_data_free (SubtreeDeferredData *data)
 {
   g_object_unref (data->message);
+  exported_subtree_unref (data->es);
   g_free (data);
 }
 
@@ -6819,7 +6891,7 @@ subtree_message_func (GDBusConnection *connection,
 
   data = g_new0 (SubtreeDeferredData, 1);
   data->message = g_object_ref (message);
-  data->es = es;
+  data->es = exported_subtree_ref (es);
 
   /* defer this call to an idle handler in the right thread */
   idle_source = g_idle_source_new ();
@@ -6924,6 +6996,7 @@ g_dbus_connection_register_subtree (GDBusConnection           *connection,
     }
 
   es = g_new0 (ExportedSubtree, 1);
+  es->refcount = 1;
   es->object_path = g_strdup (object_path);
   es->connection = connection;
 

gio/tests/gdbus-export.c

@@ -1792,6 +1792,184 @@ test_async_properties (void)
   g_object_unref (c);
 }
 
+typedef struct
+{
+  GDBusConnection *connection;  /* (owned) */
+  guint registration_id;
+  guint subtree_registration_id;
+} ThreadedUnregistrationData;
+
+static gpointer
+unregister_thread_cb (gpointer user_data)
+{
+  ThreadedUnregistrationData *data = user_data;
+
+  /* Sleeping here makes the race more likely to be hit, as it balances the
+   * time taken to set up the thread and unregister, with the time taken to
+   * make and handle the D-Bus call. This will likely change with future kernel
+   * versions, but there isn’t a more deterministic synchronisation point that
+   * I can think of to use instead. */
+  usleep (330);
+
+  if (data->registration_id > 0)
+    g_assert_true (g_dbus_connection_unregister_object (data->connection, data->registration_id));
+
+  if (data->subtree_registration_id > 0)
+    g_assert_true (g_dbus_connection_unregister_subtree (data->connection, data->subtree_registration_id));
+
+  return NULL;
+}
+
+static void
+async_result_cb (GObject      *source_object,
+                 GAsyncResult *result,
+                 gpointer      user_data)
+{
+  GAsyncResult **result_out = user_data;
+
+  *result_out = g_object_ref (result);
+  g_main_context_wakeup (NULL);
+}
+
+/* Returns %TRUE if this iteration resolved the race with the unregistration
+ * first, %FALSE if the call handler was invoked first. */
+static gboolean
+test_threaded_unregistration_iteration (gboolean subtree)
+{
+  ThreadedUnregistrationData data = { NULL, 0, 0 };
+  ObjectRegistrationData object_registration_data = { 0, 0, 2 };
+  GError *local_error = NULL;
+  GThread *unregister_thread = NULL;
+  const gchar *object_path;
+  GVariant *value = NULL;
+  const gchar *value_str;
+  GAsyncResult *call_result = NULL;
+  gboolean unregistration_was_first;
+
+  data.connection = g_bus_get_sync (G_BUS_TYPE_SESSION, NULL, &local_error);
+  g_assert_no_error (local_error);
+  g_assert_nonnull (data.connection);
+
+  /* Register an object or a subtree */
+  if (!subtree)
+    {
+      data.registration_id = g_dbus_connection_register_object (data.connection,
+                                                                "/foo/boss",
+                                                                (GDBusInterfaceInfo *) &foo_interface_info,
+                                                                &foo_vtable,
+                                                                &object_registration_data,
+                                                                on_object_unregistered,
+                                                                &local_error);
+      g_assert_no_error (local_error);
+      g_assert_cmpint (data.registration_id, >, 0);
+
+      object_path = "/foo/boss";
+    }
+  else
+    {
+      data.subtree_registration_id = g_dbus_connection_register_subtree (data.connection,
+                                                                         "/foo/boss/executives",
+                                                                         &subtree_vtable,
+                                                                         G_DBUS_SUBTREE_FLAGS_NONE,
+                                                                         &object_registration_data,
+                                                                         on_subtree_unregistered,
+                                                                         &local_error);
+      g_assert_no_error (local_error);
+      g_assert_cmpint (data.subtree_registration_id, >, 0);
+
+      object_path = "/foo/boss/executives/vp0";
+    }
+
+  /* Allow the registrations to go through. */
+  g_main_context_iteration (NULL, FALSE);
+
+  /* Spawn a thread to unregister the object/subtree. This will race with
+   * the call we subsequently make. */
+  unregister_thread = g_thread_new ("unregister-object",
+                                    unregister_thread_cb, &data);
+
+  /* Call a method on the object (or an object in the subtree). The callback
+   * will be invoked in this main context. */
+  g_dbus_connection_call (data.connection,
+                          g_dbus_connection_get_unique_name (data.connection),
+                          object_path,
+                          "org.example.Foo",
+                          "Method1",
+                          g_variant_new ("(s)", "winwinwin"),
+                          NULL,
+                          G_DBUS_CALL_FLAGS_NONE,
+                          -1,
+                          NULL,
+                          async_result_cb,
+                          &call_result);
+
+  while (call_result == NULL)
+    g_main_context_iteration (NULL, TRUE);
+
+  value = g_dbus_connection_call_finish (data.connection, call_result, &local_error);
+
+  /* The result of the method could either be an error (that the object doesn’t
+   * exist) or a valid result, depending on how the thread was scheduled
+   * relative to the call. */
+  unregistration_was_first = (value == NULL);
+  if (value != NULL)
+    {
+      g_assert_no_error (local_error);
+      g_assert_true (g_variant_is_of_type (value, G_VARIANT_TYPE ("(s)")));
+      g_variant_get (value, "(&s)", &value_str);
+      g_assert_cmpstr (value_str, ==, "You passed the string 'winwinwin'. Jolly good!");
+    }
+  else
+    {
+      g_assert_null (value);
+      g_assert_error (local_error, G_DBUS_ERROR, G_DBUS_ERROR_UNKNOWN_METHOD);
+    }
+
+  g_clear_pointer (&value, g_variant_unref);
+  g_clear_error (&local_error);
+
+  /* Tidy up. */
+  g_thread_join (g_steal_pointer (&unregister_thread));
+
+  g_clear_object (&call_result);
+  g_clear_object (&data.connection);
+
+  return unregistration_was_first;
+}
+
+static void
+test_threaded_unregistration (gconstpointer test_data)
+{
+  gboolean subtree = GPOINTER_TO_INT (test_data);
+  guint i;
+  guint n_iterations_unregistration_first = 0;
+  guint n_iterations_call_first = 0;
+
+  g_test_bug ("https://gitlab.gnome.org/GNOME/glib/-/issues/2400");
+  g_test_summary ("Test that object/subtree unregistration from one thread "
+                  "doesn’t cause problems when racing with method callbacks "
+                  "in another thread for that object or subtree");
+
+  /* Run iterations of the test until it’s likely we’ve hit the race. Limit the
+   * number of iterations so the test doesn’t run forever if not. The choice of
+   * 100 is arbitrary. */
+  for (i = 0; i < 1000 && (n_iterations_unregistration_first < 100 || n_iterations_call_first < 100); i++)
+    {
+      if (test_threaded_unregistration_iteration (subtree))
+        n_iterations_unregistration_first++;
+      else
+        n_iterations_call_first++;
+    }
+
+  /* If the condition below is met, we probably failed to reproduce the race.
+   * Don’t fail the test, though, as we can’t always control whether we hit the
+   * race, and spurious test failures are annoying. */
+  if (n_iterations_unregistration_first < 100 ||
+      n_iterations_call_first < 100)
+    g_test_skip_printf ("Failed to reproduce race (%u iterations with unregistration first, %u with call first); skipping test",
+                        n_iterations_unregistration_first, n_iterations_call_first);
+}
+
 /* ---------------------------------------------------------------------------------------------------- */
 
 int
@@ -1809,6 +1987,8 @@ main (int   argc,
   g_test_add_func ("/gdbus/object-registration-with-closures", test_object_registration_with_closures);
   g_test_add_func ("/gdbus/registered-interfaces", test_registered_interfaces);
   g_test_add_func ("/gdbus/async-properties", test_async_properties);
+  g_test_add_data_func ("/gdbus/threaded-unregistration/object", GINT_TO_POINTER (FALSE), test_threaded_unregistration);
+  g_test_add_data_func ("/gdbus/threaded-unregistration/subtree", GINT_TO_POINTER (TRUE), test_threaded_unregistration);
 
   /* TODO: check that we spit out correct introspection data */
   /* TODO: check that registering a whole subtree works */