tizen: kdbus: revamp thread deinitializationaccepted/tizen/unified/x/asan/20241014.000007accepted/tizen/unified/x/20241008.034733accepted/tizen/unified/toolchain/20241022.122915accepted/tizen/unified/toolchain/20241022.122441accepted/tizen/unified/20241007.161028accepted/tizen_unified_x_asanaccepted/tizen_unified_xaccepted/tizen_unified_toolchainaccepted/tizen_unified

We had a bug reported where after a GLib upgrade, quickly recreating a
bus with some operations done on it would consistently result in a
SIGBUS. We believe that the root cause is that the deinitializion was
not correct and a race condition was always possible, the upgrade only
revealed it. After this commit, the bug is no longer reproducible.

To be more exact, the issue fixed by this commit is that the lifetime of
the thread launched by GKDBusWorker is not explicitly linked to the
lifetime of GKDBusWorker itself, but operations launched on the thread
attempt to access the GKDBusWorker. In our case, deliver_synthetic_reply
was scheduled asynchronously and launched after GKDBusWorker was freed,
but this function calls a callback stored inside GKDBusWorker, which
means an instant crash.

The GKDBusWorker cleanup happens when the last reference to it is
dropped, using the finalize method builtin to GLib. It does attempt to
clean up the thread inside _g_kdbus_close. This is done by telling the
thread loop to quit via _g_kdbus_quit_loop and then dropping the
reference to the thread and related resources. Unfortunately the
_g_kdbus_quit_loop call is asynchronous and neither implies that the
loop will quit quickly, nor waits for the loop to be completed. Dropping
the reference does not help either, as any running GLib thread holds a
reference to itself. The result is that the code cannot guarantee that
the thread will end before further GKDBusWorker resources or
GKDBusWorker itself are freed.

Our solution to this issue is to hold a reference to GKDBusWorker inside
a thread. This seems idiomatic, as it encodes that the thread will
access GKDBusWorker, and guarantees that the thread will already be
completed before we move into the finalize method. Additionally we
terminate the thread loop much earlier, before we drop the main
GKDBusWorker reference.

This is probably not perfect, but is a minimal fix that solves our
issue. We could use a general refactor of this code, but I think we
neither understand it well enough, nor we have enough time to do so
without a clear reason.

Change-Id: I0c4ba4dace8ba8a5a07e2caebe9ad15312d49c71

1 files changed, 6 insertions, 6 deletions

diff --git a/gio/gkdbus.c b/gio/gkdbus.c
index aeb4408a1..fd7a63bdb 100755
--- a/gio/gkdbus.c
+++ b/gio/gkdbus.c
@@ -605,8 +605,6 @@ _g_kdbus_close (GKDBusWorker *worker)
   if (worker->closed)
     return TRUE;
 
-  g_main_context_invoke (worker->context, _g_kdbus_quit_loop, worker->loop);
-
   g_main_context_unref (worker->context);
   worker->context = NULL;
 
@@ -4201,11 +4199,11 @@ g_kdbus_worker_init (GKDBusWorker *worker)
 static gpointer
 _g_kdbus_worker_thread (gpointer _data)
 {
-  GMainLoop *loop = (GMainLoop *) _data;
+  GKDBusWorker *worker = (GKDBusWorker *) _data;
 
-  g_main_loop_run (loop);
+  g_main_loop_run (worker->loop);
 
-  g_main_loop_unref (loop);
+  g_object_unref (worker);
 
   return NULL;
 }
@@ -4225,7 +4223,8 @@ _g_kdbus_worker_new (const gchar  *address,
 
   worker->context = g_main_context_new ();
   worker->loop = g_main_loop_new (worker->context, FALSE);
-  worker->thread = g_thread_new ("gkdbus", _g_kdbus_worker_thread, g_main_loop_ref(worker->loop));
+  g_object_ref (worker);
+  worker->thread = g_thread_new ("gkdbus", _g_kdbus_worker_thread, worker);
 
   return worker;
 }
@@ -4343,6 +4342,7 @@ _g_kdbus_worker_stop (GKDBusWorker *worker)
   g_source_destroy (worker->source);
   g_source_unref (worker->source);
   worker->source = 0;
+  g_main_context_invoke (worker->context, _g_kdbus_quit_loop, worker->loop);
 
   g_object_unref (worker);
 }