Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

GUACAMOLE-1732: Prevent deadlock and enhance cleanup in RDP client. #496

Merged
merged 1 commit into from
Mar 13, 2024
Merged

GUACAMOLE-1732: Prevent deadlock and enhance cleanup in RDP client. #496

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 14 additions & 5 deletions src/protocols/rdp/client.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,10 +35,12 @@
#include "common-ssh/user.h"
#endif

#include <guacamole/argv.h>
#include <guacamole/audio.h>
#include <guacamole/client.h>
#include <guacamole/mem.h>
#include <guacamole/recording.h>
#include <guacamole/rwlock.h>

#include <dirent.h>
#include <errno.h>
Expand Down Expand Up @@ -117,7 +119,7 @@ static int guac_rdp_join_pending_handler(guac_client* client) {
guac_rdp_client* rdp_client = (guac_rdp_client*) client->data;
guac_socket* broadcast_socket = client->pending_socket;

pthread_rwlock_rdlock(&(rdp_client->lock));
guac_rwlock_acquire_read_lock(&(rdp_client->lock));

/* Synchronize any audio stream for each pending user */
if (rdp_client->audio)
Expand All @@ -133,7 +135,7 @@ static int guac_rdp_join_pending_handler(guac_client* client) {
guac_socket_flush(broadcast_socket);
}

pthread_rwlock_unlock(&(rdp_client->lock));
guac_rwlock_release_lock(&(rdp_client->lock));

return 0;

Expand Down Expand Up @@ -220,7 +222,7 @@ int guac_client_init(guac_client* client, int argc, char** argv) {
PTHREAD_MUTEX_RECURSIVE);

/* Init required locks */
pthread_rwlock_init(&(rdp_client->lock), NULL);
guac_rwlock_init(&(rdp_client->lock));
pthread_mutex_init(&(rdp_client->message_lock), &(rdp_client->attributes));

/* Set handlers */
Expand All @@ -241,6 +243,14 @@ int guac_rdp_client_free_handler(guac_client* client) {

guac_rdp_client* rdp_client = (guac_rdp_client*) client->data;

/*
* Signals any threads that are blocked awaiting user input for authentication
* (e.g., username or password) to terminate their wait. By broadcasting a
* condition signal, the authentication process is interrupted, allowing for
* premature termination and cleanup during client disconnection.
*/
guac_argv_stop();

/* Wait for client thread */
pthread_join(rdp_client->client_thread, NULL);

Expand Down Expand Up @@ -297,7 +307,7 @@ int guac_rdp_client_free_handler(guac_client* client) {
if (rdp_client->audio_input != NULL)
guac_rdp_audio_buffer_free(rdp_client->audio_input);

pthread_rwlock_destroy(&(rdp_client->lock));
guac_rwlock_destroy(&(rdp_client->lock));
pthread_mutex_destroy(&(rdp_client->message_lock));

/* Free client data */
Expand All @@ -306,4 +316,3 @@ int guac_rdp_client_free_handler(guac_client* client) {
return 0;

}

14 changes: 7 additions & 7 deletions src/protocols/rdp/input.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
#include <freerdp/input.h>
#include <guacamole/client.h>
#include <guacamole/recording.h>
#include <guacamole/rwlock.h>
#include <guacamole/user.h>

#include <stdlib.h>
Expand All @@ -39,7 +40,7 @@ int guac_rdp_user_mouse_handler(guac_user* user, int x, int y, int mask) {
guac_client* client = user->client;
guac_rdp_client* rdp_client = (guac_rdp_client*) client->data;

pthread_rwlock_rdlock(&(rdp_client->lock));
guac_rwlock_acquire_read_lock(&(rdp_client->lock));

/* Skip if not yet connected */
freerdp* rdp_inst = rdp_client->rdp_inst;
Expand Down Expand Up @@ -125,7 +126,7 @@ int guac_rdp_user_mouse_handler(guac_user* user, int x, int y, int mask) {
}

complete:
pthread_rwlock_unlock(&(rdp_client->lock));
guac_rwlock_release_lock(&(rdp_client->lock));

return 0;
}
Expand All @@ -136,7 +137,7 @@ int guac_rdp_user_touch_handler(guac_user* user, int id, int x, int y,
guac_client* client = user->client;
guac_rdp_client* rdp_client = (guac_rdp_client*) client->data;

pthread_rwlock_rdlock(&(rdp_client->lock));
guac_rwlock_acquire_read_lock(&(rdp_client->lock));

/* Skip if not yet connected */
freerdp* rdp_inst = rdp_client->rdp_inst;
Expand All @@ -152,7 +153,7 @@ int guac_rdp_user_touch_handler(guac_user* user, int id, int x, int y,
guac_rdp_rdpei_touch_update(rdp_client->rdpei, id, x, y, force);

complete:
pthread_rwlock_unlock(&(rdp_client->lock));
guac_rwlock_release_lock(&(rdp_client->lock));

return 0;
}
Expand All @@ -163,7 +164,7 @@ int guac_rdp_user_key_handler(guac_user* user, int keysym, int pressed) {
guac_rdp_client* rdp_client = (guac_rdp_client*) client->data;
int retval = 0;

pthread_rwlock_rdlock(&(rdp_client->lock));
guac_rwlock_acquire_read_lock(&(rdp_client->lock));

/* Report key state within recording */
if (rdp_client->recording != NULL)
Expand All @@ -179,7 +180,7 @@ int guac_rdp_user_key_handler(guac_user* user, int keysym, int pressed) {
keysym, pressed, GUAC_RDP_KEY_SOURCE_CLIENT);

complete:
pthread_rwlock_unlock(&(rdp_client->lock));
guac_rwlock_release_lock(&(rdp_client->lock));

return retval;

Expand All @@ -202,4 +203,3 @@ int guac_rdp_user_size_handler(guac_user* user, int width, int height) {
return 0;

}

5 changes: 3 additions & 2 deletions src/protocols/rdp/keyboard.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
#include <freerdp/input.h>
#include <guacamole/client.h>
#include <guacamole/mem.h>
#include <guacamole/rwlock.h>

#include <stdlib.h>

Expand Down Expand Up @@ -718,7 +719,7 @@ BOOL guac_rdp_keyboard_set_indicators(rdpContext* context, UINT16 flags) {
guac_client* client = ((rdp_freerdp_context*) context)->client;
guac_rdp_client* rdp_client = (guac_rdp_client*) client->data;

pthread_rwlock_rdlock(&(rdp_client->lock));
guac_rwlock_acquire_read_lock(&(rdp_client->lock));

/* Skip if keyboard not yet ready */
guac_rdp_keyboard* keyboard = rdp_client->keyboard;
Expand All @@ -730,7 +731,7 @@ BOOL guac_rdp_keyboard_set_indicators(rdpContext* context, UINT16 flags) {
keyboard->lock_flags = flags;

complete:
pthread_rwlock_unlock(&(rdp_client->lock));
guac_rwlock_release_lock(&(rdp_client->lock));
return TRUE;

}
27 changes: 22 additions & 5 deletions src/protocols/rdp/rdp.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,8 +108,14 @@ BOOL rdp_freerdp_pre_connect(freerdp* instance) {

/* Load "AUDIO_INPUT" plugin for audio input*/
if (settings->enable_audio_input) {
/* Upgrade the lock to write temporarily for setting the newly allocated audio buffer */
guac_rwlock_acquire_write_lock(&(rdp_client->lock));
rdp_client->audio_input = guac_rdp_audio_buffer_alloc(client);
guac_rdp_audio_load_plugin(instance->context);

/* Downgrade the lock to allow for concurrent read access */
guac_rwlock_release_lock(&(rdp_client->lock));
guac_rwlock_acquire_read_lock(&(rdp_client->lock));
}

/* Load "cliprdr" service if not disabled */
Expand Down Expand Up @@ -470,7 +476,7 @@ static int guac_rdp_handle_connection(guac_client* client) {
/* Init random number generator */
srandom(time(NULL));

pthread_rwlock_wrlock(&(rdp_client->lock));
guac_rwlock_acquire_write_lock(&(rdp_client->lock));

/* Create display */
rdp_client->display = guac_common_display_alloc(client,
Expand Down Expand Up @@ -516,12 +522,23 @@ static int guac_rdp_handle_connection(guac_client* client) {
/* Set default pointer */
guac_common_cursor_set_pointer(rdp_client->display->cursor);

/*
* Downgrade the lock to allow for concurrent read access.
* Access to read locks needs to be made available for other processes such
* as the join_pending_handler to use while we await credentials from the user.
*/
guac_rwlock_release_lock(&(rdp_client->lock));
guac_rwlock_acquire_read_lock(&(rdp_client->lock));

/* Connect to RDP server */
if (!freerdp_connect(rdp_inst)) {
guac_rdp_client_abort(client, rdp_inst);
goto fail;
}

/* Upgrade to write lock again for further exclusive operations */
guac_rwlock_acquire_write_lock(&(rdp_client->lock));

/* Connection complete */
rdp_client->rdp_inst = rdp_inst;

Expand All @@ -530,7 +547,7 @@ static int guac_rdp_handle_connection(guac_client* client) {
/* Signal that reconnect has been completed */
guac_rdp_disp_reconnect_complete(rdp_client->disp);

pthread_rwlock_unlock(&(rdp_client->lock));
guac_rwlock_release_lock(&(rdp_client->lock));

/* Handle messages from RDP server while client is running */
while (client->state == GUAC_CLIENT_RUNNING
Expand Down Expand Up @@ -617,7 +634,7 @@ static int guac_rdp_handle_connection(guac_client* client) {

}

pthread_rwlock_wrlock(&(rdp_client->lock));
guac_rwlock_acquire_write_lock(&(rdp_client->lock));

/* Clean up print job, if active */
if (rdp_client->active_job != NULL) {
Expand Down Expand Up @@ -652,15 +669,15 @@ static int guac_rdp_handle_connection(guac_client* client) {
guac_common_display_free(rdp_client->display);
rdp_client->display = NULL;

pthread_rwlock_unlock(&(rdp_client->lock));
guac_rwlock_release_lock(&(rdp_client->lock));

/* Client is now disconnected */
guac_client_log(client, GUAC_LOG_INFO, "Internal RDP client disconnected");

return 0;

fail:
pthread_rwlock_unlock(&(rdp_client->lock));
guac_rwlock_release_lock(&(rdp_client->lock));
return 1;

}
Expand Down
4 changes: 2 additions & 2 deletions src/protocols/rdp/rdp.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@
#include <freerdp/freerdp.h>
#include <guacamole/audio.h>
#include <guacamole/client.h>
#include <guacamole/rwlock.h>
#include <guacamole/recording.h>
#include <winpr/wtypes.h>

Expand Down Expand Up @@ -170,7 +171,7 @@ typedef struct guac_rdp_client {
* from running when RDP data structures are allocated or freed
* by the client thread.
*/
pthread_rwlock_t lock;
guac_rwlock lock;

/**
* Lock which synchronizes the sending of each RDP message, ensuring
Expand Down Expand Up @@ -219,4 +220,3 @@ typedef struct rdp_freerdp_context {
void* guac_rdp_client_thread(void* data);

#endif