From b3b2cce36ea4247ef73b232d59dd4f5667d8ce35 Mon Sep 17 00:00:00 2001 From: pletch <17855860+pletch@users.noreply.github.com> Date: Wed, 2 Sep 2026 23:32:39 -0400 Subject: [PATCH] fix(rdp): reinstall the RDPGFX wrappers after a channel reconnect The previous commit stopped guac_rdp_rdpgfx_channel_connected() from wrapping its own wrapper, which fixed the recursion crash, but it guarded on orig_surface_command being NULL: if (rdp_client->settings->enable_h264 && rdp_client->orig_surface_command == NULL) { orig_surface_command lives on guac_rdp_client, so it is per-connection, while the handler it protects is per-channel-instance. FreeRDP's gdi_graphics_pipeline_uninit() sets SurfaceCommand to NULL and gdi_graphics_pipeline_init() assigns gdi_SurfaceCommand back (libfreerdp/gdi/gfx.c:1864, :1911), so a reconnect restores FreeRDP's own handler -- and the once-only guard, seeing a non-NULL orig_surface_command from the first install, declines to wrap it again. Passthrough is then dead for the rest of the session. xrdp reconnects the channel at login, for the display resize the previous commit already identified, so this fired on essentially every xrdp session. It was silent: the surface commands still reached FreeRDP's GDI decoder and guacd re-encoded them as JPEG/WebP, so the desktop rendered correctly at ~100% of a core while no H.264 reached the browser. A trace on xrdp 0.10.6 showed one keyframe forwarded, the channel unloading 52ms later, and no "H.264 passthrough enabled" line after the reconnect that followed. Guard on what is actually installed, per callback. The two are restored on different schedules -- gdi_graphics_pipeline_init() assigns SurfaceCommand but never touches CapsConfirm -- so after a reconnect CapsConfirm is still ours while SurfaceCommand is FreeRDP's again. A single guard keyed on SurfaceCommand would pass and then point orig_caps_confirm at guac_rdp_gfx_caps_confirm itself, reaching the same stack exhaustion by the other callback. With this, the same host forwards 807 AVC420 deltas and 3 keyframes over 60s of desktop use, with guacd at 3.3% of a core. --- patches/004-h264-passthrough.patch | 55 +++++++++++++++++++++--------- 1 file changed, 39 insertions(+), 16 deletions(-) diff --git a/patches/004-h264-passthrough.patch b/patches/004-h264-passthrough.patch index aa10297..94a579c 100644 --- a/patches/004-h264-passthrough.patch +++ b/patches/004-h264-passthrough.patch @@ -992,7 +992,7 @@ index 327e7c2b..c49737d7 100644 /** * Callback which associates handlers specific to Guacamole with the * RdpgfxClientContext instance allocated by FreeRDP to deal with received -@@ -63,13 +438,45 @@ static void guac_rdp_rdpgfx_channel_connected(rdpContext* context, +@@ -63,13 +438,68 @@ static void guac_rdp_rdpgfx_channel_connected(rdpContext* context, RdpgfxClientContext* rdpgfx = (RdpgfxClientContext*) args->pInterface; rdpGdi* gdi = context->gdi; @@ -1009,27 +1009,50 @@ index 327e7c2b..c49737d7 100644 + guac_rdp_client* rdp_client = (guac_rdp_client*) client->data; + + /* Installed only for a connection that asked for H.264, and only -+ * once. This handler runs again whenever the RDPGFX channel -+ * reconnects -- the display resize xrdp performs at login triggers -+ * exactly that -- and a second install reads back the wrapper the -+ * first one left behind, so orig_surface_command ends up pointing at -+ * guac_rdp_gfx_surface_command itself. Every surface command that is -+ * not H.264 then calls the wrapper from inside the wrapper and -+ * recurses until the stack is gone. H.264 commands take the ++ * onto a handler that is not already ours. This runs again every ++ * time the RDPGFX channel reconnects -- the display resize xrdp ++ * performs at login triggers exactly that -- and the two ways of ++ * getting it wrong fail in opposite directions. ++ * ++ * Reinstalling unconditionally is fatal: the second pass reads back ++ * the wrapper the first one left, orig_surface_command ends up ++ * pointing at guac_rdp_gfx_surface_command itself, and every surface ++ * command that is not H.264 calls the wrapper from inside the ++ * wrapper until the stack is gone. H.264 commands take the + * skip_decode path and never make that call, so a server sending -+ * nothing but H.264 survives it, while one mixing in planar or -+ * progressive surfaces dies on the first of them. */ -+ if (rdp_client->settings->enable_h264 -+ && rdp_client->orig_surface_command == NULL) { ++ * nothing but H.264 survives it while one mixing in planar or ++ * progressive surfaces dies on the first of them. ++ * ++ * Installing only once is quieter and just as broken. ++ * gdi_graphics_pipeline_uninit() sets SurfaceCommand to NULL and ++ * gdi_graphics_pipeline_init() puts gdi_SurfaceCommand back, so a ++ * reconnect leaves FreeRDP's own handler installed while a ++ * once-only guard declines to wrap it again. Passthrough then stops ++ * for the rest of the session -- silently, since the frames still ++ * render via GDI and guacd simply re-encodes them. ++ * ++ * Testing what is actually installed, per callback, covers both. */ ++ if (rdp_client->settings->enable_h264) { + + /* Wrap SurfaceCommand to intercept H.264 data after GDI decode. + * Store the original callback per-connection to avoid clobbering + * when multiple RDP sessions are active concurrently. */ -+ rdp_client->orig_surface_command = (void*) rdpgfx->SurfaceCommand; -+ rdpgfx->SurfaceCommand = guac_rdp_gfx_surface_command; ++ if (rdpgfx->SurfaceCommand != guac_rdp_gfx_surface_command) { ++ rdp_client->orig_surface_command = (void*) rdpgfx->SurfaceCommand; ++ rdpgfx->SurfaceCommand = guac_rdp_gfx_surface_command; ++ } + -+ rdp_client->orig_caps_confirm = (void*) rdpgfx->CapsConfirm; -+ rdpgfx->CapsConfirm = guac_rdp_gfx_caps_confirm; ++ /* Guarded separately, because the two callbacks are restored on ++ * different schedules: gdi_graphics_pipeline_init() assigns ++ * SurfaceCommand but never touches CapsConfirm, so after a ++ * reconnect CapsConfirm is still ours while SurfaceCommand is ++ * FreeRDP's again. One shared guard therefore re-reads a wrapper ++ * we installed ourselves and points orig_caps_confirm at ++ * guac_rdp_gfx_caps_confirm. */ ++ if (rdpgfx->CapsConfirm != guac_rdp_gfx_caps_confirm) { ++ rdp_client->orig_caps_confirm = (void*) rdpgfx->CapsConfirm; ++ rdpgfx->CapsConfirm = guac_rdp_gfx_caps_confirm; ++ } + + guac_client_log(client, GUAC_LOG_INFO, + "H.264 passthrough enabled for RDPGFX channel.");