Merge pull request #133 from glyptodon/rdp-disconnect-leaks

GUAC-1164: Ensure RDP-related memory is freed between connect/disconnect cycles.

src/common-ssh/guac_ssh.c

@@ -128,6 +128,9 @@ static void guac_common_ssh_openssl_free_locks(int count) {
     for (i=0; i < count; i++)
         pthread_mutex_destroy(&(guac_common_ssh_openssl_locks[i]));
 
+    /* Free lock array */
+    free(guac_common_ssh_openssl_locks);
+
 }
 
 int guac_common_ssh_init(guac_client* client) {

src/common-ssh/guac_ssh_key.c

@@ -150,6 +150,7 @@ void guac_common_ssh_key_free(guac_common_ssh_key* key) {
     else if (key->type == SSH_KEY_DSA)
         DSA_free(key->dsa);
 
+    free(key->private_key);
     free(key->public_key);
     free(key);
 }

src/libguac/client.c

@@ -482,6 +482,7 @@ void guac_client_free(guac_client* client) {
     }
 
     pthread_rwlock_destroy(&(client->__users_lock));
+    free(client->connection_id);
     free(client);
 }
 

src/protocols/rdp/input.c

@@ -165,9 +165,6 @@ int guac_rdp_user_mouse_handler(guac_user* user, int x, int y, int mask) {
 
     pthread_mutex_lock(&(rdp_client->rdp_lock));
 
-    /* Store current mouse location */
-    guac_common_cursor_move(rdp_client->display->cursor, user, x, y);
-
     /* Skip if not yet connected */
     freerdp* rdp_inst = rdp_client->rdp_inst;
     if (rdp_inst == NULL) {
@@ -175,6 +172,9 @@ int guac_rdp_user_mouse_handler(guac_user* user, int x, int y, int mask) {
         return 0;
     }
 
+    /* Store current mouse location */
+    guac_common_cursor_move(rdp_client->display->cursor, user, x, y);
+
     /* If button mask unchanged, just send move event */
     if (mask == rdp_client->mouse_button_mask)
         rdp_inst->input->MouseEvent(rdp_inst->input, PTR_FLAGS_MOVE, x, y);

src/protocols/rdp/rdp.c

@@ -764,8 +764,6 @@ static int guac_rdp_handle_connection(guac_client* client) {
     __guac_rdp_client_load_keymap(client, settings->server_layout);
 
 #ifdef ENABLE_COMMON_SSH
-    guac_common_ssh_init(client);
-
     /* Connect via SSH if SFTP is enabled */
     if (settings->enable_sftp) {
 
@@ -981,18 +979,28 @@ static int guac_rdp_handle_connection(guac_client* client) {
 
     pthread_mutex_lock(&(rdp_client->rdp_lock));
 
-    /* Clean up RDP client */
+    /* Disconnect client and channels */
     freerdp_channels_close(channels, rdp_inst);
     freerdp_channels_free(channels);
     freerdp_disconnect(rdp_inst);
+
+    /* Clean up RDP client context */
     freerdp_clrconv_free(((rdp_freerdp_context*) rdp_inst->context)->clrconv);
     cache_free(rdp_inst->context->cache);
+    freerdp_context_free(rdp_inst);
+
+    /* Clean up RDP client */
     freerdp_free(rdp_inst);
+    rdp_client->rdp_inst = NULL;
 
     /* Clean up filesystem, if allocated */
     if (rdp_client->filesystem != NULL)
         guac_rdp_fs_free(rdp_client->filesystem);
 
+    /* Clean up audio stream, if allocated */
+    if (rdp_client->audio != NULL)
+        guac_audio_stream_free(rdp_client->audio);
+
 #ifdef ENABLE_COMMON_SSH
     /* Free SFTP filesystem, if loaded */
     if (rdp_client->sftp_filesystem)
@@ -1005,8 +1013,6 @@ static int guac_rdp_handle_connection(guac_client* client) {
     /* Free SFTP user */
     if (rdp_client->sftp_user)
         guac_common_ssh_destroy_user(rdp_client->sftp_user);
-
-    guac_common_ssh_uninit();
 #endif
 
     /* Free SVC list */
@@ -1015,9 +1021,6 @@ static int guac_rdp_handle_connection(guac_client* client) {
     /* Free display */
     guac_common_display_free(rdp_client->display);
 
-    /* Mark FreeRDP instance as freed */
-    rdp_client->rdp_inst = NULL;
-
     pthread_mutex_unlock(&(rdp_client->rdp_lock));
     return 0;
 
@@ -1027,13 +1030,20 @@ void* guac_rdp_client_thread(void* data) {
 
     guac_client* client = (guac_client*) data;
 
+#ifdef ENABLE_COMMON_SSH
+    guac_common_ssh_init(client);
+#endif
+
+    /* Continue handling connections until error or client disconnect */
     while (client->state == GUAC_CLIENT_RUNNING) {
-
         if (guac_rdp_handle_connection(client))
-            return NULL;
+            break;
-
     }
 
+#ifdef ENABLE_COMMON_SSH
+    guac_common_ssh_uninit();
+#endif
+
     return NULL;
 
 }

src/protocols/rdp/rdp_settings.c

@@ -765,8 +765,20 @@ void guac_rdp_settings_free(guac_rdp_settings* settings) {
     free(settings->username);
 
     /* Free channel name array */
+    if (settings->svc_names != NULL) {
+
+        /* Free all elements of array */
+        char** current = &(settings->svc_names[0]);
+        while (*current != NULL) {
+            free(*current);
+            current++;
+        }
+
+        /* Free array itself */
         free(settings->svc_names);
 
+    }
+
 #ifdef ENABLE_COMMON_SSH
     /* Free SFTP settings */
     free(settings->sftp_directory);
@@ -852,6 +864,28 @@ static int guac_rdp_get_performance_flags(guac_rdp_settings* guac_settings) {
 
 }
 
+/**
+ * Simple wrapper for strdup() which behaves identically to standard strdup(),
+ * execpt that NULL will be returned if the provided string is NULL.
+ *
+ * @param str
+ *     The string to duplicate as a newly-allocated string.
+ *
+ * @return
+ *     A newly-allocated string containing identically the same content as the
+ *     given string, or NULL if the given string was NULL.
+ */
+static char* guac_rdp_strdup(const char* str) {
+
+    /* Return NULL if no string provided */
+    if (str == NULL)
+        return NULL;
+
+    /* Otherwise just invoke strdup() */
+    return strdup(str);
+
+}
+
 void guac_rdp_push_settings(guac_rdp_settings* guac_settings, freerdp* rdp) {
 
     BOOL bitmap_cache;
@@ -859,21 +893,21 @@ void guac_rdp_push_settings(guac_rdp_settings* guac_settings, freerdp* rdp) {
 
     /* Authentication */
 #ifdef LEGACY_RDPSETTINGS
-    rdp_settings->domain = guac_settings->domain;
+    rdp_settings->domain = guac_rdp_strdup(guac_settings->domain);
-    rdp_settings->username = guac_settings->username;
+    rdp_settings->username = guac_rdp_strdup(guac_settings->username);
-    rdp_settings->password = guac_settings->password;
+    rdp_settings->password = guac_rdp_strdup(guac_settings->password);
 #else
-    rdp_settings->Domain = guac_settings->domain;
+    rdp_settings->Domain = guac_rdp_strdup(guac_settings->domain);
-    rdp_settings->Username = guac_settings->username;
+    rdp_settings->Username = guac_rdp_strdup(guac_settings->username);
-    rdp_settings->Password = guac_settings->password;
+    rdp_settings->Password = guac_rdp_strdup(guac_settings->password);
 #endif
 
     /* Connection */
 #ifdef LEGACY_RDPSETTINGS
-    rdp_settings->hostname = guac_settings->hostname;
+    rdp_settings->hostname = guac_rdp_strdup(guac_settings->hostname);
     rdp_settings->port = guac_settings->port;
 #else
-    rdp_settings->ServerHostname = guac_settings->hostname;
+    rdp_settings->ServerHostname = guac_rdp_strdup(guac_settings->hostname);
     rdp_settings->ServerPort = guac_settings->port;
 #endif
 
@@ -882,13 +916,13 @@ void guac_rdp_push_settings(guac_rdp_settings* guac_settings, freerdp* rdp) {
     rdp_settings->color_depth = guac_settings->color_depth;
     rdp_settings->width = guac_settings->width;
     rdp_settings->height = guac_settings->height;
-    rdp_settings->shell = guac_settings->initial_program;
+    rdp_settings->shell = guac_rdp_strdup(guac_settings->initial_program);
     rdp_settings->kbd_layout = guac_settings->server_layout->freerdp_keyboard_layout;
 #else
     rdp_settings->ColorDepth = guac_settings->color_depth;
     rdp_settings->DesktopWidth = guac_settings->width;
     rdp_settings->DesktopHeight = guac_settings->height;
-    rdp_settings->AlternateShell = guac_settings->initial_program;
+    rdp_settings->AlternateShell = guac_rdp_strdup(guac_settings->initial_program);
     rdp_settings->KeyboardLayout = guac_settings->server_layout->freerdp_keyboard_layout;
 #endif
 
@@ -1034,7 +1068,7 @@ void guac_rdp_push_settings(guac_rdp_settings* guac_settings, freerdp* rdp) {
         rdp_settings->RemoteApplicationMode = TRUE;
         rdp_settings->RemoteAppLanguageBarSupported = TRUE;
         rdp_settings->RemoteApplicationProgram = guac_settings->remote_app;
-        rdp_settings->ShellWorkingDirectory = guac_settings->remote_app_dir;
+        rdp_settings->ShellWorkingDirectory = guac_rdp_strdup(guac_settings->remote_app_dir);
         rdp_settings->RemoteApplicationCmdLine = guac_settings->remote_app_args;
 #endif
     }

src/protocols/rdp/rdp_stream.c

@@ -286,7 +286,15 @@ int guac_rdp_clipboard_end_handler(guac_user* user, guac_stream* stream) {
 
     guac_client* client = user->client;
     guac_rdp_client* rdp_client = (guac_rdp_client*) client->data;
-    rdpChannels* channels = rdp_client->rdp_inst->context->channels;
+
+    /* Terminate clipboard data with NULL */
+    guac_common_clipboard_append(rdp_client->clipboard, "", 1);
+
+    /* Notify RDP server of new data, if connected */
+    freerdp* rdp_inst = rdp_client->rdp_inst;
+    if (rdp_inst != NULL) {
+
+        rdpChannels* channels = rdp_inst->context->channels;
 
         RDP_CB_FORMAT_LIST_EVENT* format_list =
             (RDP_CB_FORMAT_LIST_EVENT*) freerdp_event_new(
@@ -294,9 +302,6 @@ int guac_rdp_clipboard_end_handler(guac_user* user, guac_stream* stream) {
                 CliprdrChannel_FormatList,
                 NULL, NULL);
 
-    /* Terminate clipboard data with NULL */
-    guac_common_clipboard_append(rdp_client->clipboard, "", 1);
-
         /* Notify server that text data is now available */
         format_list->formats = (UINT32*) malloc(sizeof(UINT32) * 2);
         format_list->formats[0] = CB_FORMAT_TEXT;
@@ -305,6 +310,8 @@ int guac_rdp_clipboard_end_handler(guac_user* user, guac_stream* stream) {
 
         freerdp_channels_send_event(channels, (wMessage*) format_list);
 
+    }
+
     return 0;
 }