From f35530b52348063ff2b98b451a30c22848d920fd Mon Sep 17 00:00:00 2001 From: Liyi Meng Date: Tue, 18 Aug 2026 13:07:55 +0000 Subject: [PATCH] chansrv: Stop XrdpIme's commit racing the client's own IME cleanup Microsoft Remote Desktop for Mac was observed to garble Chinese text typed through its local Pinyin IME: the composed characters would land with pieces missing or extra characters deleted from the document. Root cause, confirmed with debug logging plus dbus-monitor on the live ibus session bus: the client forwards the raw pre-composition keystrokes over the normal RDP keyboard channel while composing (these reach XrdpIme's process-key-event handler and, since it never consumes anything, get typed into the document as literal ASCII), then once composition finishes sends a compensating burst of Backspace keystrokes sized to exactly undo them, interleaved with the final composed text arriving over the separate TS_UNICODE_KEYBOARD_EVENT side channel. The two channels have no ordering guarantee, so the side-channel commit could land in the middle of the client's own backspace cleanup and get partially deleted. Measured the backspace burst against the leaked raw-character count across several phrases (23-for-23, 29-for-29) - it's always exactly right, so this is a pure interleaving race, not a client miscount. That makes it fixable server-side: xrdp_input_queue_source_prepare/ _check now withhold a queued commit until raw key traffic on the engine has been quiet for XRDP_INPUT_COMMIT_QUIET_US (100ms), so the client's own cleanup has time to finish first, capped by XRDP_INPUT_COMMIT_MAX_WAIT_US (500ms) so back-to-back composition can't delay a commit indefinitely. This is an empirical mitigation tuned to observed client behaviour, not a protocol guarantee. Also fixes two latent data races found while reviewing this code against the cross-thread invariants already documented at the top of the file: g_engine and last_input_name are written by the IBus thread (engine enable/disable, an unexpected daemon disconnect) but were read and, for last_input_name, also written directly by xrdp_input_enable() on chansrv's own thread with no synchronization - unlike `bus`, which already gets this treatment. Both now follow the same publish/clear-under-state_mutex pattern as `bus`. Added IBUS-KEY/IBUS-COMMIT debug logging used to diagnose this. Verified live against a real Microsoft Remote Desktop for Mac client connected to an xrdp/ibus session: multiple Chinese phrases of varying length composed and committed cleanly with the debounce in place, after reliably reproducing the interleaved garbling without it. Co-Authored-By: Claude Sonnet 5 --- sesman/chansrv/input_ibus.c | 207 +++++++++++++++++++++++++++++++----- 1 file changed, 183 insertions(+), 24 deletions(-) diff --git a/sesman/chansrv/input_ibus.c b/sesman/chansrv/input_ibus.c index 5b736e91..745176c0 100644 --- a/sesman/chansrv/input_ibus.c +++ b/sesman/chansrv/input_ibus.c @@ -77,16 +77,45 @@ static gboolean ibus_thread_exited = TRUE; static gboolean ibus_shutting_down = FALSE; static int engine_id = 0; +/* Debounce state for committing queued unicode text - IBus-thread only, + * same as g_engine et al. above. RDP clients that compose IME text + * locally (e.g. Microsoft Remote Desktop for Mac) have been observed to + * *also* forward the raw pre-composition keystrokes over the normal + * keyboard channel, then send a compensating burst of Backspace key + * events sized to exactly undo them once composition finishes - see the + * commit that introduced this debounce for the measurements (backspace + * count matched leaked-character count exactly, every time: 23-for-23, + * 29-for-29). Those backspaces and this engine's queued commit arrive + * over two independent channels with no ordering guarantee, so without + * this delay the commit can land in the middle of the client's own + * cleanup and get partially deleted. Waiting for raw key traffic on this + * engine to go quiet before committing sidesteps that race entirely, + * since it's the same fix as "wait for the client's known-correct + * cleanup to finish" without needing to trust the client to order things + * itself. This is an empirical mitigation, not a protocol guarantee - + * a client that pauses mid-burst for longer than the quiet window could + * still race it, hence the absolute cap below. */ +static gint64 last_raw_key_time = 0; +static gint64 queue_wait_since = 0; +#define XRDP_INPUT_COMMIT_QUIET_US ((gint64)100 * 1000) +#define XRDP_INPUT_COMMIT_MAX_WAIT_US ((gint64)500 * 1000) + /*****************************************************************************/ static void xrdp_input_ibus_engine_enable(IBusEngine *engine) { + IBusEngine *old_engine = NULL; + LOG(LOG_LEVEL_INFO, "xrdp_ibus_engine_enable: IM enabled, engine=%p", engine); + /* g_engine is read cross-thread (unlocked) by xrdp_input_enable() on + * chansrv's own thread - see the comment there. Publish it under + * state_mutex, same pattern as `bus`. */ + g_mutex_lock(&state_mutex); if (g_engine != NULL && g_engine != engine) { - g_object_unref(g_engine); + old_engine = g_engine; g_engine = NULL; } @@ -94,6 +123,12 @@ xrdp_input_ibus_engine_enable(IBusEngine *engine) { g_engine = g_object_ref(engine); } + g_mutex_unlock(&state_mutex); + + if (old_engine != NULL) + { + g_object_unref(old_engine); + } if (ibus_context != NULL) { @@ -105,14 +140,23 @@ xrdp_input_ibus_engine_enable(IBusEngine *engine) static void xrdp_input_ibus_engine_disable(IBusEngine *engine) { + IBusEngine *old_engine = NULL; + LOG(LOG_LEVEL_INFO, "xrdp_ibus_engine_disable: IM disabled, engine=%p", engine); + g_mutex_lock(&state_mutex); if (g_engine == engine) { - g_object_unref(g_engine); + old_engine = g_engine; g_engine = NULL; } + g_mutex_unlock(&state_mutex); + + if (old_engine != NULL) + { + g_object_unref(old_engine); + } } /*****************************************************************************/ @@ -122,10 +166,21 @@ engine_process_key_event_cb(IBusEngine *engine, guint keycode, guint state) { - /* XrdpIme is a Unicode commit bridge, not a keyboard-event IME. */ + last_raw_key_time = g_get_monotonic_time(); + + LOG(LOG_LEVEL_DEBUG, + "IBUS-KEY: engine=%p keyval=0x%08X keycode=%u state=0x%08X", + engine, + keyval, + keycode, + state); + + /* XrdpIme is a Unicode commit bridge, not a keyboard-event IME - it + * never consumes keys itself. last_raw_key_time above is what keeps + * this from racing the client's own compensating keystrokes; see the + * comment on it. */ return FALSE; } - /*****************************************************************************/ static IBusEngine * xrdp_input_ibus_create_engine(IBusFactory *factory, @@ -180,6 +235,8 @@ xrdp_input_enable(void) const gchar *name; IBusBus *local_bus; gboolean result; + gboolean engine_ready; + gchar *last_input_name_snapshot; g_mutex_lock(&state_mutex); local_bus = (bus != NULL) ? g_object_ref(bus) : NULL; @@ -193,13 +250,27 @@ xrdp_input_enable(void) return FALSE; } + /* g_engine and last_input_name are otherwise IBus-thread-only (see + * the comment at their declarations) - snapshot both under + * state_mutex here rather than reading them directly, since this + * function runs on chansrv's own thread and the IBus thread can + * write either of them at any time (engine enable/disable, an + * unexpected daemon disconnect tearing both down). Working from a + * private copy of the name avoids racing a concurrent free of it. */ + g_mutex_lock(&state_mutex); + engine_ready = (g_engine != NULL); + last_input_name_snapshot = + (last_input_name != NULL) ? g_strdup(last_input_name) : NULL; + g_mutex_unlock(&state_mutex); + 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) { - if (g_engine != NULL) + if (engine_ready) { + g_free(last_input_name_snapshot); g_object_unref(desc); g_object_unref(local_bus); return TRUE; @@ -217,8 +288,8 @@ xrdp_input_enable(void) * succession, was most of them). Fall through and let the * queue/prepare-check mechanism wait for g_engine as usual. */ } - else if (last_input_name != NULL && name != NULL && - g_ascii_strcasecmp(name, last_input_name) != 0) + else if (last_input_name_snapshot != NULL && name != NULL && + g_ascii_strcasecmp(name, last_input_name_snapshot) != 0) { /* Only reclaim XrdpIme if the current engine is either unset, * or is the original one we remembered before ever switching @@ -237,21 +308,28 @@ xrdp_input_enable(void) "xrdp_input_enable: global engine is \"%s\", not XrdpIme or " "the original \"%s\" - leaving it alone rather than " "reclaiming it, dropping this commit", - name, last_input_name); + name, last_input_name_snapshot); + g_free(last_input_name_snapshot); g_object_unref(desc); g_object_unref(local_bus); return FALSE; } /* Remember the user's engine only the first time we replace it. */ - if (last_input_name == NULL && name != NULL) + if (last_input_name_snapshot == NULL && name != NULL) { - last_input_name = g_strdup(name); + g_mutex_lock(&state_mutex); + if (last_input_name == NULL) + { + last_input_name = g_strdup(name); + } + g_mutex_unlock(&state_mutex); LOG(LOG_LEVEL_INFO, - "xrdp_input_enable: saving original IBus engine: %s", - last_input_name); + "xrdp_input_enable: saving original IBus engine: %s", name); } + g_free(last_input_name_snapshot); + if (desc != NULL) { g_object_unref(desc); @@ -282,6 +360,10 @@ xrdp_input_process_unicode_queue(gpointer data) if (text != NULL) { + LOG(LOG_LEVEL_DEBUG, + "IBUS-COMMIT: engine=%p committing U+%04X", + g_engine, event->unicode); + /* ibus_engine_commit_text() sinks and releases `text` itself * since it's still floating (IBusText derives from * GInitiallyUnowned) - do not unref it again here, that @@ -295,19 +377,70 @@ xrdp_input_process_unicode_queue(gpointer data) return G_SOURCE_CONTINUE; } +/*****************************************************************************/ +/* TRUE once queued unicode text is safe to commit: either raw key traffic + * on this engine has been quiet for XRDP_INPUT_COMMIT_QUIET_US (the + * client's own compensating keystrokes, if any, have had time to land), + * or XRDP_INPUT_COMMIT_MAX_WAIT_US has elapsed since the queue first had + * something pending, whichever comes first. If not yet ready, *wait_us + * (when non-NULL) is set to how much longer until it will be. */ +static gboolean +xrdp_input_queue_ready(gint64 *wait_us) +{ + gint64 now; + gint64 deadline; + + if (g_engine == NULL || g_async_queue_length(unicode_queue) <= 0) + { + queue_wait_since = 0; + return FALSE; + } + + now = g_get_monotonic_time(); + if (queue_wait_since == 0) + { + queue_wait_since = now; + } + + deadline = MIN(last_raw_key_time + XRDP_INPUT_COMMIT_QUIET_US, + queue_wait_since + XRDP_INPUT_COMMIT_MAX_WAIT_US); + + if (now >= deadline) + { + return TRUE; + } + + if (wait_us != NULL) + { + *wait_us = deadline - now; + } + return FALSE; +} + /*****************************************************************************/ static gboolean xrdp_input_queue_source_prepare(GSource *source, gint *timeout) { - *timeout = -1; - return g_engine != NULL && g_async_queue_length(unicode_queue) > 0; + gint64 wait_us = 0; + + if (xrdp_input_queue_ready(&wait_us)) + { + *timeout = 0; + return TRUE; + } + + /* Either nothing pending (wait_us left at 0 - block until the next + * g_main_context_wakeup(), same as the old *timeout = -1 behaviour), + * or pending but not quiet yet (wake us up once it should be). */ + *timeout = (wait_us > 0) ? (gint)((wait_us / 1000) + 1) : -1; + return FALSE; } /*****************************************************************************/ static gboolean xrdp_input_queue_source_check(GSource *source) { - return g_engine != NULL && g_async_queue_length(unicode_queue) > 0; + return xrdp_input_queue_ready(NULL); } /*****************************************************************************/ @@ -347,25 +480,35 @@ static gboolean xrdp_input_shutdown_cb(gpointer data) { XrdpUnicodeEvent *event; + gchar *original_engine_name; LOG(LOG_LEVEL_INFO, "xrdp_input: shutting down IBus"); ibus_shutting_down = TRUE; - if (last_input_name != NULL && bus != NULL && + /* Take ownership of last_input_name under state_mutex rather than + * reading then freeing it directly - xrdp_input_enable() on + * chansrv's own thread can be reading or (re)writing it concurrently + * (see the comment there), and this runs on the IBus thread. */ + g_mutex_lock(&state_mutex); + original_engine_name = last_input_name; + last_input_name = NULL; + g_mutex_unlock(&state_mutex); + + if (original_engine_name != NULL && bus != NULL && ibus_bus_is_connected(bus)) { LOG(LOG_LEVEL_INFO, "xrdp_input: restoring original IBus engine: %s", - last_input_name); - if (!ibus_bus_set_global_engine(bus, last_input_name)) + original_engine_name); + if (!ibus_bus_set_global_engine(bus, original_engine_name)) { LOG(LOG_LEVEL_WARNING, "xrdp_input: failed to restore original IBus engine"); } } - g_clear_pointer(&last_input_name, g_free); + g_free(original_engine_name); while ((event = g_async_queue_try_pop(unicode_queue)) != NULL) { @@ -556,13 +699,29 @@ thread_cleanup: thread_default_pushed = FALSE; } - if (g_engine != NULL) { - g_object_unref(g_engine); - g_engine = NULL; - } + /* Same reasoning as the `bus` clear below: g_engine and + * last_input_name are read cross-thread (unlocked) by + * xrdp_input_enable() on chansrv's own thread, and this can run + * on an unexpected daemon disconnect with no coordination from + * that thread at all - clear the shared pointers under lock + * before releasing what they pointed to. */ + IBusEngine *old_engine; + gchar *old_last_input_name; - g_clear_pointer(&last_input_name, g_free); + g_mutex_lock(&state_mutex); + old_engine = g_engine; + g_engine = NULL; + old_last_input_name = last_input_name; + last_input_name = NULL; + g_mutex_unlock(&state_mutex); + + if (old_engine != NULL) + { + g_object_unref(old_engine); + } + g_free(old_last_input_name); + } if (desc != NULL) {