diff --git a/sesman/chansrv/input_ibus.c b/sesman/chansrv/input_ibus.c index 01ececfd..8df7af23 100644 --- a/sesman/chansrv/input_ibus.c +++ b/sesman/chansrv/input_ibus.c @@ -167,28 +167,40 @@ xrdp_input_ibus_create_engine(IBusFactory *factory, * with "Set global engine failed: Timeout was reached". Calling this * from chansrv's thread instead leaves the IBus thread free to answer * the nested call while this blocks. g_dbus_connection sync calls are - * documented thread-safe to issue from any thread, so this is fine - * despite `bus` otherwise being owned by the IBus thread. */ + * documented thread-safe to issue from any thread, so that part is + * fine despite `bus` otherwise being owned by the IBus thread - but + * the `bus` pointer itself is not: the IBus thread can null it out + * (disconnect, teardown) at any time. Take our own reference under + * state_mutex before touching it, so a concurrent teardown drops the + * shared pointer without pulling the object out from under us. */ static gboolean xrdp_input_enable(void) { IBusEngineDesc *desc; const gchar *name; + IBusBus *local_bus; + gboolean result; - if (bus == NULL || !ibus_bus_is_connected(bus)) + g_mutex_lock(&state_mutex); + local_bus = (bus != NULL) ? g_object_ref(bus) : NULL; + g_mutex_unlock(&state_mutex); + + if (local_bus == NULL || !ibus_bus_is_connected(local_bus)) { LOG(LOG_LEVEL_ERROR, "xrdp_input_enable: IBus is not connected"); + g_clear_object(&local_bus); return FALSE; } - desc = ibus_bus_get_global_engine(bus); + desc = ibus_bus_get_global_engine(local_bus); name = desc != NULL ? ibus_engine_desc_get_name(desc) : NULL; if (name != NULL && g_ascii_strcasecmp(name, "XrdpIme") == 0 && g_engine != NULL) { g_object_unref(desc); + g_object_unref(local_bus); return TRUE; } @@ -206,14 +218,15 @@ xrdp_input_enable(void) g_object_unref(desc); } - if (!ibus_bus_set_global_engine(bus, "XrdpIme")) + result = ibus_bus_set_global_engine(local_bus, "XrdpIme"); + if (!result) { LOG(LOG_LEVEL_ERROR, "xrdp_input_enable: failed to switch global engine to XrdpIme"); - return FALSE; } - return TRUE; + g_object_unref(local_bus); + return result; } /*****************************************************************************/ @@ -410,14 +423,23 @@ xrdp_input_main_loop(void *in_val) g_main_context_push_thread_default(ibus_context); thread_default_pushed = TRUE; - bus = ibus_bus_new(); - - if (bus == NULL) { - LOG(LOG_LEVEL_ERROR, "xrdp_input_main_loop: ibus_bus_new failed"); - goto thread_cleanup; + IBusBus *new_bus = ibus_bus_new(); + + if (new_bus == NULL) + { + LOG(LOG_LEVEL_ERROR, "xrdp_input_main_loop: ibus_bus_new failed"); + goto thread_cleanup; + } + g_object_ref_sink(new_bus); + + /* Published under lock so xrdp_input_enable(), running on + * chansrv's thread, never observes a partially-constructed + * `bus`. */ + g_mutex_lock(&state_mutex); + bus = new_bus; + g_mutex_unlock(&state_mutex); } - g_object_ref_sink(bus); if (!ibus_bus_is_connected(bus)) { @@ -518,10 +540,22 @@ thread_cleanup: g_object_unref(factory); factory = NULL; } - if (bus != NULL) { - g_object_unref(bus); + /* Clear the shared pointer under lock before dropping the + * reference, so a concurrent xrdp_input_enable() ref (taken + * under the same lock) always sees either the live object or + * NULL, never a dangling one. */ + IBusBus *old_bus; + + g_mutex_lock(&state_mutex); + old_bus = bus; bus = NULL; + g_mutex_unlock(&state_mutex); + + if (old_bus != NULL) + { + g_object_unref(old_bus); + } } if (ibus_loop != NULL) { @@ -633,8 +667,42 @@ xrdp_input_unicode_init(void) g_mutex_lock(&state_mutex); if (ibus_ready) { - g_mutex_unlock(&state_mutex); - return 0; + /* ibus_ready normally tracks connection liveness exactly, + * since the "disconnected" handler and thread_exit update it + * together - but there's a window between the socket actually + * dying and that signal being dispatched on the IBus thread. + * Double-check here rather than trust a possibly-stale flag, + * so a client that reconnects right into that window gets a + * clean failure (and a real retry next time) instead of a + * false "unicode input is supported" advertisement it can + * never actually use. */ + IBusBus *check_bus = (bus != NULL) ? g_object_ref(bus) : NULL; + gboolean connected = (check_bus != NULL) && + ibus_bus_is_connected(check_bus); + g_clear_object(&check_bus); + + if (connected) + { + g_mutex_unlock(&state_mutex); + return 0; + } + + /* Stale: the IBus thread hasn't noticed its connection is dead + * yet. Ask it to shut down and wait for it to fully exit + * before starting a new one below - starting a second thread + * while the first is still alive would race both of them over + * bus/g_engine. */ + LOG(LOG_LEVEL_WARNING, + "xrdp_input_unicode_init: stale IBus connection, " + "restarting IBus thread"); + if (ibus_loop != NULL) + { + g_main_loop_quit(ibus_loop); + } + while (!ibus_thread_exited) + { + g_cond_wait(&state_cond, &state_mutex); + } } ibus_shutting_down = FALSE; ibus_thread_exited = FALSE;