GUACAMOLE-2328: Initialize the FreeRDP GDI within PostConnect, not PreConnect. - #713
Merged
Merged
Conversation
…eConnect. gdi_init() requires that the rdpContext cache already exist. That cache is not created until the connection sequence reaches Confirm Active, which occurs after the PreConnect callback has returned. Calling gdi_init() from PreConnect therefore dereferences a NULL cache within gdi_init_ex(), aborting or crashing the connection before any session can be established. This is only observable with FreeRDP 3.31.0 and later. Earlier versions created the cache within gdi_init_ex() itself, so the call ordering went unnoticed. PostConnect is where FreeRDP's own reference clients call gdi_init(), and is also where the GDI is sized using the display geometry the server actually negotiated rather than the geometry initially requested. The pointer registration and update handler assignments move along with gdi_init(). They must follow it: gdi_init() installs FreeRDP's own BeginPaint, EndPaint, and DesktopResize handlers, silently replacing anything registered beforehand. Verified against both FreeRDP 3.30.0 and 3.31.0, connecting to an RDP server that authenticates successfully and confirming that drawing instructions continue to be produced in both cases.
mike-jumper
approved these changes
Sep 10, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes GUACAMOLE-2328.
gdi_init()is currently called from the PreConnect callback. It requires that the rdpContext cache already exist, and that cache is not created until the connection sequence reaches Confirm Active, which is after PreConnect has returned. On FreeRDP 3.31.0 and later this dereferences a NULL cache insidegdi_init_ex(), and no RDP session can be established.Earlier FreeRDP versions created the cache inside
gdi_init_ex()itself, so the ordering went unnoticed. FreeRDP were asked about this first and consider PreConnect an unsupported place to callgdi_init(), noting it isn't done in any of their reference clients.This moves
gdi_init()into a new PostConnect callback. The pointer registration and the update handler assignments move with it, because they have to followgdi_init(): it installs FreeRDP's own BeginPaint, EndPaint and DesktopResize handlers, and would silently replace anything registered earlier. What stays in PreConnect isguac_rdp_push_settings(), the add-in provider registration, and the LoadChannels fallback, all of which need to happen before the connection sequence begins.graphicsis no longer used inrdp_freerdp_pre_connect()and has been removed. The doc comments on both functions have been updated to match.Testing
Built on Ubuntu 24.04 arm64 and tested against an RDP server that authenticates successfully, counting Guacamole protocol instructions produced after
readyso that drawing is exercised rather than just the handshake.freerdp_connect()The 3.30.0 row is included to show the change does not impose a minimum FreeRDP version.
One thing that may help anyone else hitting this: Debian and Ubuntu build FreeRDP with
-DNDEBUG, which compiles out theWINPR_ASSERTthat would otherwise name the NULL cache. On those builds the failure is a bare SIGSEGV in the guacd child with nothing logged at all, rather than an assertion with a backtrace.