From bd5b4d8c4894420f9ed3bdd15e01a1d5ac73c557 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Sat, 21 May 2016 02:46:02 +0200 Subject: [PATCH] Fix a race condition when terminating runaway_killer_thread MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The code used to call g_main_loop_quit() from the main thread, without having any guarantee that runaway_killer_thread_func() has even entered its g_main_loop_run(). If a main loop is not running, g_main_loop_quit() has no effect. This could occasionally be reproduced in test-polkitbackendjsauthority.c, which is creating several very short-lived PolkitBackendJSAuthority instances. Real polkitd should not generally be affected, because it is using a single instance running for the life of the process ~ for the uptime of the system, enough time to enter the runaway_killer_thread main loop. To fix this, use g_idle_source_new () to make sure g_main_loop_quit () is called from within the running main loop. Also, simplify the initialization of runaway_killer_thread by moving the creation of rkt_context and rkt_loop into the main thread; this makes the condition variable and its associated mutex completely unnecessary. Finally, only destroy rkt_timeout_pending_mutex _after_ the thread terminates; before, we were certain that rkt_source was destroyed by that time, but AFAICS that does not ensure that the rkt_on_timeout () callback has already terminated. https://bugs.freedesktop.org/show_bug.cgi?id=95513 Signed-off-by: Miloslav Trmač --- src/polkitbackend/polkitbackendjsauthority.c | 73 +++++++++++++++------------- 1 file changed, 39 insertions(+), 34 deletions(-) diff --git a/src/polkitbackend/polkitbackendjsauthority.c b/src/polkitbackend/polkitbackendjsauthority.c index 2112868..3356290 100644 --- a/src/polkitbackend/polkitbackendjsauthority.c +++ b/src/polkitbackend/polkitbackendjsauthority.c @@ -79,8 +79,6 @@ struct _PolkitBackendJsAuthorityPrivate JSObject *js_polkit; GThread *runaway_killer_thread; - GMutex rkt_init_mutex; - GCond rkt_init_cond; GMainContext *rkt_context; GMainLoop *rkt_loop; GSource *rkt_source; @@ -128,6 +126,7 @@ enum /* ---------------------------------------------------------------------------------------------------- */ static gpointer runaway_killer_thread_func (gpointer user_data); +static void runaway_killer_terminate (PolkitBackendJsAuthority *authority); static GList *polkit_backend_js_authority_get_admin_auth_identities (PolkitBackendInteractiveAuthority *authority, PolkitSubject *caller, @@ -532,20 +531,14 @@ polkit_backend_js_authority_constructed (GObject *object) authority->priv->rules_dirs[1] = g_strdup (PACKAGE_DATA_DIR "/polkit-1/rules.d"); } - g_mutex_init (&authority->priv->rkt_init_mutex); - g_cond_init (&authority->priv->rkt_init_cond); + authority->priv->rkt_context = g_main_context_new (); + authority->priv->rkt_loop = g_main_loop_new (authority->priv->rkt_context, FALSE); g_mutex_init (&authority->priv->rkt_timeout_pending_mutex); authority->priv->runaway_killer_thread = g_thread_new ("runaway-killer-thread", runaway_killer_thread_func, authority); - /* wait for runaway_killer_thread to set up its GMainContext */ - g_mutex_lock (&authority->priv->rkt_init_mutex); - while (authority->priv->rkt_context == NULL) - g_cond_wait (&authority->priv->rkt_init_cond, &authority->priv->rkt_init_mutex); - g_mutex_unlock (&authority->priv->rkt_init_mutex); - setup_file_monitors (authority); load_scripts (authority); @@ -568,15 +561,11 @@ polkit_backend_js_authority_finalize (GObject *object) PolkitBackendJsAuthority *authority = POLKIT_BACKEND_JS_AUTHORITY (object); guint n; - g_mutex_clear (&authority->priv->rkt_init_mutex); - g_cond_clear (&authority->priv->rkt_init_cond); - g_mutex_clear (&authority->priv->rkt_timeout_pending_mutex); + runaway_killer_terminate (authority); - /* shut down the killer thread */ - g_assert (authority->priv->rkt_loop != NULL); - g_main_loop_quit (authority->priv->rkt_loop); - g_thread_join (authority->priv->runaway_killer_thread); - g_assert (authority->priv->rkt_loop == NULL); + g_mutex_clear (&authority->priv->rkt_timeout_pending_mutex); + g_main_loop_unref (authority->priv->rkt_loop); + g_main_context_unref (authority->priv->rkt_context); for (n = 0; authority->priv->dir_monitors != NULL && authority->priv->dir_monitors[n] != NULL; n++) { @@ -934,25 +923,9 @@ runaway_killer_thread_func (gpointer user_data) { PolkitBackendJsAuthority *authority = POLKIT_BACKEND_JS_AUTHORITY (user_data); - g_mutex_lock (&authority->priv->rkt_init_mutex); - - authority->priv->rkt_context = g_main_context_new (); - authority->priv->rkt_loop = g_main_loop_new (authority->priv->rkt_context, FALSE); g_main_context_push_thread_default (authority->priv->rkt_context); - - /* Signal the main thread that we're done constructing */ - g_cond_signal (&authority->priv->rkt_init_cond); - g_mutex_unlock (&authority->priv->rkt_init_mutex); - g_main_loop_run (authority->priv->rkt_loop); - g_main_context_pop_thread_default (authority->priv->rkt_context); - - g_main_loop_unref (authority->priv->rkt_loop); - authority->priv->rkt_loop = NULL; - g_main_context_unref (authority->priv->rkt_context); - authority->priv->rkt_context = NULL; - return NULL; } @@ -1039,6 +1012,38 @@ runaway_killer_teardown (PolkitBackendJsAuthority *authority) authority->priv->rkt_source = NULL; } +static gboolean +runaway_killer_call_g_main_quit (gpointer user_data) +{ + PolkitBackendJsAuthority *authority = user_data; + g_main_loop_quit (authority->priv->rkt_loop); + return FALSE; +} + +static void +runaway_killer_terminate (PolkitBackendJsAuthority *authority) +{ + GSource *source; + + /* Use a g_idle_source_new () to ensure g_main_loop_quit () is called from + * inside a running rkt_loop. This prevents a possible race condition, where + * we could be calling g_main_loop_quit () on the main thread before + * runaway_killer_thread_func () starts its g_main_loop_run () call; + * g_main_loop_quit () before g_main_loop_run () does nothing, so in such + * a case we would not terminate the thread and become blocked in + * g_thread_join () below. + */ + g_assert (authority->priv->rkt_loop != NULL); + + source = g_idle_source_new (); + g_source_set_callback (source, runaway_killer_call_g_main_quit, authority, + NULL); + g_source_attach (source, authority->priv->rkt_context); + g_source_unref (source); + + g_thread_join (authority->priv->runaway_killer_thread); +} + static JSBool execute_script_with_runaway_killer (PolkitBackendJsAuthority *authority, #if JS_VERSION == 186 -- 2.4.11