From 2524af80a95a334606e7001cd4990e03c92e81fe Mon Sep 17 00:00:00 2001 From: Michael Jumper Date: Wed, 28 Jul 2021 15:50:13 -0700 Subject: [PATCH] GUACAMOLE-1388: Ensure RDP-specific resources are cleaned up after channel disconnect. Without these changes, RDP-specific resources like the CLIPRDR and RDPEI channels may remain from past connections if the RDP connection is dynamically reconnected via the "Reconnect" display resize method, resulting in assertion failures or memory errors if those stale resources are reused after reconnect is completed. --- src/protocols/rdp/channels/cliprdr.c | 46 ++++++++++++++++++++++++++++ src/protocols/rdp/channels/disp.c | 40 ++++++++++++++++++++++++ src/protocols/rdp/channels/rdpei.c | 39 +++++++++++++++++++++++ src/protocols/rdp/rdp.c | 24 +++++++-------- 4 files changed, 137 insertions(+), 12 deletions(-) diff --git a/src/protocols/rdp/channels/cliprdr.c b/src/protocols/rdp/channels/cliprdr.c index b0e3345e..261f0c98 100644 --- a/src/protocols/rdp/channels/cliprdr.c +++ b/src/protocols/rdp/channels/cliprdr.c @@ -546,6 +546,48 @@ static void guac_rdp_cliprdr_channel_connected(rdpContext* context, } +/** + * Callback which disassociates Guacamole from the CliprdrClientContext + * instance that was originally allocated by FreeRDP and is about to be + * deallocated. + * + * This function is called whenever a channel disconnects via the PubSub event + * system within FreeRDP, but only has any effect if the disconnected channel + * is the CLIPRDR channel. This specific callback is registered with the PubSub + * system of the relevant rdpContext when guac_rdp_clipboard_load_plugin() is + * called. + * + * @param context + * The rdpContext associated with the active RDP session. + * + * @param e + * Event-specific arguments, mainly the name of the channel, and a + * reference to the associated plugin loaded for that channel by FreeRDP. + */ +static void guac_rdp_cliprdr_channel_disconnected(rdpContext* context, + ChannelDisconnectedEventArgs* e) { + + guac_client* client = ((rdp_freerdp_context*) context)->client; + guac_rdp_client* rdp_client = (guac_rdp_client*) client->data; + guac_rdp_clipboard* clipboard = rdp_client->clipboard; + + /* FreeRDP-specific handlers for CLIPRDR are not assigned, and thus not + * callable, until after the relevant guac_rdp_clipboard structure is + * allocated and associated with the guac_rdp_client */ + assert(clipboard != NULL); + + /* Ignore disconnection event if it's not for the CLIPRDR channel */ + if (strcmp(e->name, CLIPRDR_SVC_CHANNEL_NAME) != 0) + return; + + /* Channel is no longer connected */ + clipboard->cliprdr = NULL; + + guac_client_log(client, GUAC_LOG_DEBUG, "CLIPRDR (clipboard redirection) " + "channel disconnected."); + +} + guac_rdp_clipboard* guac_rdp_clipboard_alloc(guac_client* client) { /* Allocate clipboard and underlying storage */ @@ -575,6 +617,10 @@ void guac_rdp_clipboard_load_plugin(guac_rdp_clipboard* clipboard, PubSub_SubscribeChannelConnected(context->pubSub, (pChannelConnectedEventHandler) guac_rdp_cliprdr_channel_connected); + /* Clean up any RDP-specific resources when channel is disconnected */ + PubSub_SubscribeChannelDisconnected(context->pubSub, + (pChannelDisconnectedEventHandler) guac_rdp_cliprdr_channel_disconnected); + guac_client_log(clipboard->client, GUAC_LOG_DEBUG, "Support for CLIPRDR " "(clipboard redirection) registered. Awaiting channel " "connection."); diff --git a/src/protocols/rdp/channels/disp.c b/src/protocols/rdp/channels/disp.c index 95dc6d99..9bfbc4e2 100644 --- a/src/protocols/rdp/channels/disp.c +++ b/src/protocols/rdp/channels/disp.c @@ -96,12 +96,52 @@ static void guac_rdp_disp_channel_connected(rdpContext* context, } +/** + * Callback which disassociates Guacamole from the DispClientContext instance + * that was originally allocated by FreeRDP and is about to be deallocated. + * + * This function is called whenever a channel disconnects via the PubSub event + * system within FreeRDP, but only has any effect if the disconnected channel + * is the Display Update channel. This specific callback is registered with the + * PubSub system of the relevant rdpContext when guac_rdp_disp_load_plugin() is + * called. + * + * @param context + * The rdpContext associated with the active RDP session. + * + * @param e + * Event-specific arguments, mainly the name of the channel, and a + * reference to the associated plugin loaded for that channel by FreeRDP. + */ +static void guac_rdp_disp_channel_disconnected(rdpContext* context, + ChannelDisconnectedEventArgs* e) { + + guac_client* client = ((rdp_freerdp_context*) context)->client; + guac_rdp_client* rdp_client = (guac_rdp_client*) client->data; + guac_rdp_disp* guac_disp = rdp_client->disp; + + /* Ignore disconnection event if it's not for the Display Update channel */ + if (strcmp(e->name, DISP_DVC_CHANNEL_NAME) != 0) + return; + + /* Channel is no longer connected */ + guac_disp->disp = NULL; + + guac_client_log(client, GUAC_LOG_DEBUG, "Display update channel " + "disconnected."); + +} + void guac_rdp_disp_load_plugin(rdpContext* context) { /* Subscribe to and handle channel connected events */ PubSub_SubscribeChannelConnected(context->pubSub, (pChannelConnectedEventHandler) guac_rdp_disp_channel_connected); + /* Subscribe to and handle channel disconnected events */ + PubSub_SubscribeChannelDisconnected(context->pubSub, + (pChannelDisconnectedEventHandler) guac_rdp_disp_channel_disconnected); + /* Add "disp" channel */ guac_freerdp_dynamic_channel_collection_add(context->settings, "disp", NULL); diff --git a/src/protocols/rdp/channels/rdpei.c b/src/protocols/rdp/channels/rdpei.c index af1771d4..06d2ba4c 100644 --- a/src/protocols/rdp/channels/rdpei.c +++ b/src/protocols/rdp/channels/rdpei.c @@ -94,12 +94,51 @@ static void guac_rdp_rdpei_channel_connected(rdpContext* context, } +/** + * Callback which disassociates Guacamole from the RdpeiClientContext instance + * that was originally allocated by FreeRDP and is about to be deallocated. + * + * This function is called whenever a channel disconnects via the PubSub event + * system within FreeRDP, but only has any effect if the disconnected channel + * is the RDPEI channel. This specific callback is registered with the PubSub + * system of the relevant rdpContext when guac_rdp_rdpei_load_plugin() is + * called. + * + * @param context + * The rdpContext associated with the active RDP session. + * + * @param e + * Event-specific arguments, mainly the name of the channel, and a + * reference to the associated plugin loaded for that channel by FreeRDP. + */ +static void guac_rdp_rdpei_channel_disconnected(rdpContext* context, + ChannelDisconnectedEventArgs* e) { + + guac_client* client = ((rdp_freerdp_context*) context)->client; + guac_rdp_client* rdp_client = (guac_rdp_client*) client->data; + guac_rdp_rdpei* guac_rdpei = rdp_client->rdpei; + + /* Ignore disconnection event if it's not for the RDPEI channel */ + if (strcmp(e->name, RDPEI_DVC_CHANNEL_NAME) != 0) + return; + + /* Channel is no longer connected */ + guac_rdpei->rdpei = NULL; + + guac_client_log(client, GUAC_LOG_DEBUG, "RDPDI channel disconnected."); + +} + void guac_rdp_rdpei_load_plugin(rdpContext* context) { /* Subscribe to and handle channel connected events */ PubSub_SubscribeChannelConnected(context->pubSub, (pChannelConnectedEventHandler) guac_rdp_rdpei_channel_connected); + /* Subscribe to and handle channel disconnected events */ + PubSub_SubscribeChannelDisconnected(context->pubSub, + (pChannelDisconnectedEventHandler) guac_rdp_rdpei_channel_disconnected); + /* Add "rdpei" channel */ guac_freerdp_dynamic_channel_collection_add(context->settings, "rdpei", NULL); diff --git a/src/protocols/rdp/rdp.c b/src/protocols/rdp/rdp.c index 1c528e66..2c6e8029 100644 --- a/src/protocols/rdp/rdp.c +++ b/src/protocols/rdp/rdp.c @@ -471,18 +471,6 @@ static int guac_rdp_handle_connection(guac_client* client) { pthread_rwlock_wrlock(&(rdp_client->lock)); - /* Set up screen recording, if requested */ - if (settings->recording_path != NULL) { - rdp_client->recording = guac_common_recording_create(client, - settings->recording_path, - settings->recording_name, - settings->create_recording_path, - !settings->recording_exclude_output, - !settings->recording_exclude_mouse, - !settings->recording_exclude_touch, - settings->recording_include_keys); - } - /* Create display */ rdp_client->display = guac_common_display_alloc(client, rdp_client->settings->width, @@ -816,6 +804,18 @@ void* guac_rdp_client_thread(void* data) { } #endif + /* Set up screen recording, if requested */ + if (settings->recording_path != NULL) { + rdp_client->recording = guac_common_recording_create(client, + settings->recording_path, + settings->recording_name, + settings->create_recording_path, + !settings->recording_exclude_output, + !settings->recording_exclude_mouse, + !settings->recording_exclude_touch, + settings->recording_include_keys); + } + /* Continue handling connections until error or client disconnect */ while (client->state == GUAC_CLIENT_RUNNING) { if (guac_rdp_handle_connection(client))