github/xrdp
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

chansrv: Protect bus lifetime across threads, detect stale fast path

Browse Source 
Three correctness gaps found in code review of the private-loop
rewrite, all in how chansrv's thread and the IBus thread interact
around `bus`:

- xrdp_input_enable() read the shared `bus` pointer with no
  synchronization at all, despite the IBus thread being free to null
  it out (disconnect, teardown) at any time - a real use-after-free
  risk, not just a formality, since this function actively calls
  methods on it rather than just checking it's non-NULL. Fixed by
  taking a ref under state_mutex before touching it, and using that
  local reference for the rest of the call; bus's creation and
  teardown in xrdp_input_main_loop() now publish/clear the shared
  pointer under the same lock so the ref-under-lock pattern actually
  synchronizes against something.

- xrdp_input_unicode_init()'s fast path trusted ibus_ready without
  re-checking the connection. ibus_ready and connection liveness
  normally change together, but there's a window between the
  underlying socket actually dying and the "disconnected" signal being
  dispatched on the IBus thread. A client that reconnects into that
  window would get a false "unicode input is supported"
  advertisement it could never actually use. Now double-checks
  ibus_bus_is_connected() before taking the fast path.

- That staleness check can't just flip ibus_ready and fall through to
  spawning a new thread - the old one may still be alive and about to
  touch bus/g_engine itself, racing the new thread's own setup. Now
  asks the stale thread to shut down (g_main_loop_quit, safe to call
  cross-thread) and waits on the exit condvar before proceeding, so
  there's never more than one IBus thread alive at a time.

Verified: 20 back-to-back xrdp_input_unicode_init() calls on a healthy
connection all take the fast path correctly (no spurious thread
restarts); 5 kill/restart cycles still show no thread leak; an 18-char
rapid-fire send against a real focused GTK entry still lands every
character in order with zero drops after these changes.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
Liyi Meng
2026-08-18 06:25:59 +00:00
parent 77388dab8a
commit a3934cde9a
1 changed files with 85 additions and 17 deletions
Download Patch File Download Diff File Expand all files Collapse all files
sesman/chansrv/input_ibus.c
+85 -17
View File
@@ -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;