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

chansrv: Stop XrdpIme's commit racing the client's own IME cleanup

Browse Source 
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 <noreply@anthropic.com>
This commit is contained in:
Liyi Meng
2026-08-18 13:07:55 +00:00
parent 55a4f81083
commit f35530b523
1 changed files with 183 additions and 24 deletions
Download Patch File Download Diff File Expand all files Collapse all files
sesman/chansrv/input_ibus.c
+183 -24
View File
@@ -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)
{