From 2cb75e8618965b8e902dcb768b9531f41e198561 Mon Sep 17 00:00:00 2001 From: James Muehlner Date: Fri, 11 Aug 2023 04:03:56 +0000 Subject: [PATCH 1/6] GUACAMOLE-1846: Synchronize new users with the connection state in batches. --- src/libguac/Makefile.am | 16 +- src/libguac/client.c | 266 ++++++++++++++++++++++--- src/libguac/guacamole/client-fntypes.h | 10 + src/libguac/guacamole/client.h | 80 +++++++- src/libguac/reentrant-rwlock.c | 254 +++++++++++++++++++++++ src/libguac/reentrant-rwlock.h | 144 +++++++++++++ src/protocols/kubernetes/client.c | 43 ++++ src/protocols/kubernetes/user.c | 7 - src/protocols/rdp/client.c | 49 +++++ src/protocols/rdp/user.c | 16 -- src/protocols/ssh/client.c | 42 ++++ src/protocols/ssh/user.c | 7 - src/protocols/telnet/client.c | 42 ++++ src/protocols/vnc/client.c | 46 +++++ 14 files changed, 955 insertions(+), 67 deletions(-) create mode 100644 src/libguac/reentrant-rwlock.c create mode 100644 src/libguac/reentrant-rwlock.h diff --git a/src/libguac/Makefile.am b/src/libguac/Makefile.am index 0c8680561..2097fe7ca 100644 --- a/src/libguac/Makefile.am +++ b/src/libguac/Makefile.am @@ -78,13 +78,14 @@ libguacinc_HEADERS = \ guacamole/wol.h \ guacamole/wol-constants.h -noinst_HEADERS = \ - id.h \ - encode-jpeg.h \ - encode-png.h \ - palette.h \ - user-handlers.h \ - raw_encoder.h \ +noinst_HEADERS = \ + id.h \ + encode-jpeg.h \ + encode-png.h \ + reentrant-rwlock.h \ + palette.h \ + user-handlers.h \ + raw_encoder.h \ wait-fd.h libguac_la_SOURCES = \ @@ -97,6 +98,7 @@ libguac_la_SOURCES = \ fips.c \ hash.c \ id.c \ + reentrant-rwlock.c \ palette.c \ parser.c \ pool.c \ diff --git a/src/libguac/client.c b/src/libguac/client.c index 952ffcc3e..1c74722c9 100644 --- a/src/libguac/client.c +++ b/src/libguac/client.c @@ -34,15 +34,25 @@ #include "guacamole/timestamp.h" #include "guacamole/user.h" #include "id.h" +#include "reentrant-rwlock.h" #include +#include #include #include +#include #include +#include #include #include #include +/** + * The number of nanoseconds between times that the pending users list will be + * synchronized and emptied (250 milliseconds aka 1/4 second). + */ +#define GUAC_CLIENT_PENDING_USERS_REFRESH_INTERVAL 250000000 + /** * Empty NULL-terminated array of argument names. */ @@ -128,10 +138,73 @@ void guac_client_free_stream(guac_client* client, guac_stream* stream) { } +/** + * Promote all pending users to full users, calling the join pending handler + * before, if any. + * + * @param data + * The client for which all pending users should be promoted. + */ +static void guac_client_promote_pending_users(union sigval data) { + + guac_client* client = (guac_client*) data.sival_ptr; + + /* Do not start if the previous promotion event is still running */ + if (atomic_flag_test_and_set(&(client->__pending_timer_event_active))) + return; + + /* Acquire the lock for reading and modifying the list of pending users */ + guac_acquire_write_lock(&(client->__pending_users_lock)); + + /* Run the pending join handler, if one is defined */ + if (client->join_pending_handler) + client->join_pending_handler(client); + + /* The first pending user in the list, if any */ + guac_user* first_user = client->__pending_users; + + /* The final user in the list, if any */ + guac_user* last_user = first_user; + + /* Iterate through the pending users to find the final user */ + guac_user* user = first_user; + while (user != NULL) { + last_user = user; + user = user->__next; + } + + /* Mark the list as empty */ + client->__pending_users = NULL; + + /* Acquire the lock for reading and modifying the list of full users. */ + guac_acquire_write_lock(&(client->__users_lock)); + + /* If any users were removed from the pending list, promote them now */ + if (last_user != NULL) { + + /* Add all formerly-pending users to the start of the user list */ + if (client->__users != NULL) + client->__users->__prev = last_user; + + last_user->__next = client->__users; + client->__users = first_user; + + } + + guac_release_lock(&(client->__users_lock)); + + /* Release the lock (this is done AFTER updating the non-pending user list + * to ensure that all users are always on exactly one of these lists) */ + guac_release_lock(&(client->__pending_users_lock)); + + /* Mark the timer event as complete so the next instance can run */ + atomic_flag_clear(&(client->__pending_timer_event_active)); + +} + guac_client* guac_client_alloc() { int i; - pthread_rwlockattr_t lock_attributes; /* Allocate new client */ guac_client* client = malloc(sizeof(guac_client)); @@ -169,22 +242,33 @@ guac_client* guac_client_alloc() { client->__output_streams[i].index = GUAC_CLIENT_CLOSED_STREAM_INDEX; } - /* Init locks */ - pthread_rwlockattr_init(&lock_attributes); - pthread_rwlockattr_setpshared(&lock_attributes, PTHREAD_PROCESS_SHARED); + guac_init_reentrant_rwlock(&(client->__users_lock)); + guac_init_reentrant_rwlock(&(client->__pending_users_lock)); + + /* Initialize the write lock flags to 0, as threads won't have yet */ + pthread_key_create(&(client->__users_lock.key), (void *) 0); + pthread_key_create(&(client->__pending_users_lock.key), (void *) 0); - pthread_rwlock_init(&(client->__users_lock), &lock_attributes); + /* Ensure the timer is constructed only once */ + pthread_mutex_init(&(client->__pending_users_timer_mutex), NULL); /* Set up socket to broadcast to all users */ client->socket = guac_socket_broadcast(client); + /* Set the timer event thread as initially inactive, since it hasn't run */ + atomic_flag_clear(&(client->__pending_timer_event_active)); + return client; } void guac_client_free(guac_client* client) { + /* Remove all pending users */ + while (client->__pending_users != NULL) + guac_client_remove_user(client, client->__pending_users); + /* Remove all users */ while (client->__users != NULL) guac_client_remove_user(client, client->__users); @@ -215,7 +299,15 @@ void guac_client_free(guac_client* client) { guac_client_log(client, GUAC_LOG_ERROR, "Unable to close plugin: %s", dlerror()); } - pthread_rwlock_destroy(&(client->__users_lock)); + /* Destroy the pending users timer */ + pthread_mutex_destroy(&(client->__pending_users_timer_mutex)); + if (client->__pending_users_timer_running != 0) + timer_delete(client->__pending_users_timer); + + /* Destroy the reenrant read-write locks */ + guac_destroy_reentrant_rwlock(&(client->__users_lock)); + guac_destroy_reentrant_rwlock(&(client->__pending_users_lock)); + free(client->connection_id); free(client); } @@ -277,27 +369,125 @@ void guac_client_abort(guac_client* client, guac_protocol_status status, } +/** + * Add the provided user to the list of pending users who have yet to have + * their connection state synchronized after joining, for the connection + * associated with the given guac client. + * + * @param client + * The client associated with the connection for which the provided user + * is pending a connection state synchronization after joining. + * + * @param user + * The user to add to the pending list. + */ +static void guac_client_add_pending_user( + guac_client* client, guac_user* user) { + + /* Acquire the lock for modifying the list of pending users */ + guac_acquire_write_lock(&(client->__pending_users_lock)); + + user->__prev = NULL; + user->__next = client->__pending_users; + + if (client->__pending_users != NULL) + client->__pending_users->__prev = user; + + client->__pending_users = user; + + /* Increment the user count */ + client->connected_users++; + + /* Release the lock */ + guac_release_lock(&(client->__pending_users_lock)); + +} + +/** + * Periodically promote pending users to full users. Returns zero if the timer + * is already running, or successfully created, or a non-zero value if the + * timer could not be created and started. + * + * @param client + * The guac client for which the new timer should be started, if not + * already running. + * + * @return + * Zero if the timer was successfully created and started, or a negative + * value otherwise. + */ +static int guac_client_start_pending_users_timer(guac_client* client) { + + pthread_mutex_lock(&(client->__pending_users_timer_mutex)); + + /* Return success if the timer is already created and running */ + if (client->__pending_users_timer_running != 0) { + pthread_mutex_unlock(&(client->__pending_users_timer_mutex)); + return 0; + } + + /* Configure the timer to synchronize and clear the pending users */ + struct sigevent signal_config = { 0 }; + signal_config.sigev_notify = SIGEV_THREAD; + signal_config.sigev_notify_function = guac_client_promote_pending_users; + signal_config.sigev_value.sival_ptr = client; + + /* Create a timer to synchronize any pending users periodically */ + if (timer_create( + CLOCK_MONOTONIC, + &signal_config, + &(client->__pending_users_timer))) { + pthread_mutex_unlock(&(client->__pending_users_timer_mutex)); + return 1; + } + + /* Configure the pending users timer to run on the defined interval */ + struct itimerspec time_config = { 0 }; + time_config.it_interval.tv_nsec = GUAC_CLIENT_PENDING_USERS_REFRESH_INTERVAL; + time_config.it_value.tv_nsec = GUAC_CLIENT_PENDING_USERS_REFRESH_INTERVAL; + + /* Start the timer */ + if (timer_settime( + client->__pending_users_timer, 0, &time_config, NULL) < 0) { + timer_delete(client->__pending_users_timer); + pthread_mutex_unlock(&(client->__pending_users_timer_mutex)); + return 1; + } + + client->__pending_users_timer_running = 1; + pthread_mutex_unlock(&(client->__pending_users_timer_mutex)); + return 0; + +} + int guac_client_add_user(guac_client* client, guac_user* user, int argc, char** argv) { + /* Create and start the timer if it hasn't already been initialized */ + if (guac_client_start_pending_users_timer(client)) { + + /** + * + * If the timer could not be created, do not add the user - they cannot + * be synchronized without the timer. + */ + guac_client_log(client, GUAC_LOG_ERROR, + "Could not start pending user timer: %s.", strerror(errno)); + return 1; + } + int retval = 0; /* Call handler, if defined */ if (client->join_handler) retval = client->join_handler(user, argc, argv); - pthread_rwlock_wrlock(&(client->__users_lock)); - - /* Add to list if join was successful */ if (retval == 0) { - user->__prev = NULL; - user->__next = client->__users; - - if (client->__users != NULL) - client->__users->__prev = user; - - client->__users = user; - client->connected_users++; + /* + * Add the user to the list of pending users, to have their connection + * state synchronized asynchronously. + */ + guac_client_add_pending_user(client, user); /* Update owner pointer if user is owner */ if (user->owner) @@ -305,8 +495,6 @@ int guac_client_add_user(guac_client* client, guac_user* user, int argc, char** } - pthread_rwlock_unlock(&(client->__users_lock)); - /* Notify owner of user joining connection. */ if (retval == 0 && !user->owner) guac_client_owner_notify_join(client, user); @@ -317,13 +505,16 @@ int guac_client_add_user(guac_client* client, guac_user* user, int argc, char** void guac_client_remove_user(guac_client* client, guac_user* user) { - pthread_rwlock_wrlock(&(client->__users_lock)); + guac_acquire_write_lock(&(client->__users_lock)); + guac_acquire_write_lock(&(client->__pending_users_lock)); /* Update prev / head */ if (user->__prev != NULL) user->__prev->__next = user->__next; - else + else if (client->__users == user) client->__users = user->__next; + else if (client->__pending_users == user) + client->__pending_users = user->__next; /* Update next */ if (user->__next != NULL) @@ -335,7 +526,8 @@ void guac_client_remove_user(guac_client* client, guac_user* user) { if (user->owner) client->__owner = NULL; - pthread_rwlock_unlock(&(client->__users_lock)); + guac_release_lock(&(client->__pending_users_lock)); + guac_release_lock(&(client->__users_lock)); /* Update owner of user having left the connection. */ if (!user->owner) @@ -353,7 +545,7 @@ void guac_client_foreach_user(guac_client* client, guac_user_callback* callback, guac_user* current; - pthread_rwlock_rdlock(&(client->__users_lock)); + guac_acquire_read_lock(&(client->__users_lock)); /* Call function on each user */ current = client->__users; @@ -362,7 +554,25 @@ void guac_client_foreach_user(guac_client* client, guac_user_callback* callback, current = current->__next; } - pthread_rwlock_unlock(&(client->__users_lock)); + guac_release_lock(&(client->__users_lock)); + +} + +void guac_client_foreach_pending_user( + guac_client* client, guac_user_callback* callback, void* data) { + + guac_user* current; + + guac_acquire_read_lock(&(client->__pending_users_lock)); + + /* Call function on each pending user */ + current = client->__pending_users; + while (current != NULL) { + callback(current, data); + current = current->__next; + } + + guac_release_lock(&(client->__pending_users_lock)); } @@ -371,12 +581,12 @@ void* guac_client_for_owner(guac_client* client, guac_user_callback* callback, void* retval; - pthread_rwlock_rdlock(&(client->__users_lock)); + guac_acquire_read_lock(&(client->__users_lock)); /* Invoke callback with current owner */ retval = callback(client->__owner, data); - pthread_rwlock_unlock(&(client->__users_lock)); + guac_release_lock(&(client->__users_lock)); /* Return value from callback */ return retval; @@ -391,7 +601,7 @@ void* guac_client_for_user(guac_client* client, guac_user* user, int user_valid = 0; void* retval; - pthread_rwlock_rdlock(&(client->__users_lock)); + guac_acquire_read_lock(&(client->__users_lock)); /* Loop through all users, searching for a pointer to the given user */ current = client->__users; @@ -413,7 +623,7 @@ void* guac_client_for_user(guac_client* client, guac_user* user, /* Invoke callback with requested user (if they exist) */ retval = callback(user, data); - pthread_rwlock_unlock(&(client->__users_lock)); + guac_release_lock(&(client->__users_lock)); /* Return value from callback */ return retval; diff --git a/src/libguac/guacamole/client-fntypes.h b/src/libguac/guacamole/client-fntypes.h index 381ead038..3cf76f938 100644 --- a/src/libguac/guacamole/client-fntypes.h +++ b/src/libguac/guacamole/client-fntypes.h @@ -48,6 +48,16 @@ */ typedef int guac_client_free_handler(guac_client* client); +/** + * Handler that will run before pending users are promoted to full users. + * Any required operations for pending users should be applied using + * guac_client_foreach_pending_user(). + * + * @param client + * The client whose handler was invoked. + */ +typedef void guac_client_join_pending_handler(guac_client* client); + /** * Handler for logging messages related to a given guac_client instance. * diff --git a/src/libguac/guacamole/client.h b/src/libguac/guacamole/client.h index 5712476f1..d5329a013 100644 --- a/src/libguac/guacamole/client.h +++ b/src/libguac/guacamole/client.h @@ -30,6 +30,7 @@ #include "client-types.h" #include "client-constants.h" #include "layer-types.h" +#include "reentrant-rwlock.h" #include "object-types.h" #include "pool-types.h" #include "socket-types.h" @@ -40,8 +41,10 @@ #include -#include +#include #include +#include +#include struct guac_client { @@ -162,7 +165,7 @@ struct guac_client { * Lock which is acquired when the users list is being manipulated, or when * the users list is being iterated. */ - pthread_rwlock_t __users_lock; + guac_reentrant_rwlock __users_lock; /** * The first user within the list of all connected users, or NULL if no @@ -170,6 +173,43 @@ struct guac_client { */ guac_user* __users; + /** + * Lock which is acquired when the pending users list is being manipulated, + * or when the pending users list is being iterated. + */ + guac_reentrant_rwlock __pending_users_lock; + + /** + * A timer that will periodically synchronize the list of pending users, + * emptying the list once synchronization is complete. Only for internal + * use within the client. This will be NULL until the first user joins + * the connection, as it is lazily instantiated at that time. + */ + timer_t __pending_users_timer; + + /** + * Non-zero if the pending users timer is configured and running, or zero + * otherwise. + */ + int __pending_users_timer_running; + + /** + * A mutex that must be acquired before modifying the pending users timer. + */ + pthread_mutex_t __pending_users_timer_mutex; + + /** + * A flag that indicates whether the pending users timer event thread is + * currently running. + */ + volatile atomic_flag __pending_timer_event_active; + + /** + * The first user within the list of connected users who have not yet had + * their connection states synchronized after joining. + */ + guac_user* __pending_users; + /** * The user that first created this connection. This user will also have * their "owner" flag set to a non-zero value. If the owner has left the @@ -206,6 +246,22 @@ struct guac_client { */ guac_user_join_handler* join_handler; + /** + * A handler that will be run prior to pending users being promoted to full + * users. Any required pending user operations should be applied + * guac_client_foreach_pending_user(). + * + * Example: + * @code + * void join_pending_handler(guac_client* client); + * + * int guac_client_init(guac_client* client) { + * client->join_pending_handler = join_pending_handler; + * } + * @endcode + */ + guac_client_join_pending_handler* join_pending_handler; + /** * Handler for leave events, called whenever a new user is leaving an * active connection. @@ -446,6 +502,26 @@ void guac_client_remove_user(guac_client* client, guac_user* user); void guac_client_foreach_user(guac_client* client, guac_user_callback* callback, void* data); +/** + * Calls the given function on all pending users of the given client. The + * function will be given a reference to a guac_user and the specified + * arbitrary data. The value returned by the callback will be ignored. + * + * This function is reentrant, but the pending user list MUST NOT be manipulated + * within the same thread as a callback to this function. + * + * @param client + * The client whose users should be iterated. + * + * @param callback + * The function to call for each pending user. + * + * @param data + * Arbitrary data to pass to the callback each time it is invoked. + */ +void guac_client_foreach_pending_user(guac_client* client, + guac_user_callback* callback, void* data); + /** * Calls the given function with the currently-connected user that is marked as * the owner. The owner of a connection is the user that established the diff --git a/src/libguac/reentrant-rwlock.c b/src/libguac/reentrant-rwlock.c new file mode 100644 index 000000000..1ac550128 --- /dev/null +++ b/src/libguac/reentrant-rwlock.c @@ -0,0 +1,254 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +#include +#include +#include "reentrant-rwlock.h" + +/** + * The value indicating that the current thread holds neither the read or write + * locks. + */ +#define GUAC_REENTRANT_LOCK_NO_LOCK 0 + +/** + * The value indicating that the current thread holds the read lock. + */ +#define GUAC_REENTRANT_LOCK_READ_LOCK 1 + +/** + * The value indicating that the current thread holds the write lock. + */ +#define GUAC_REENTRANT_LOCK_WRITE_LOCK 2 + +void guac_init_reentrant_rwlock(guac_reentrant_rwlock* lock) { + + /* Configure to allow sharing this lock with child processes */ + pthread_rwlockattr_t lock_attributes; + pthread_rwlockattr_init(&lock_attributes); + pthread_rwlockattr_setpshared(&lock_attributes, PTHREAD_PROCESS_SHARED); + + /* Initialize the rwlock */ + pthread_rwlock_init(&(lock->lock), &lock_attributes); + + /* Initialize the flags to 0, as threads won't have acquired it yet */ + pthread_key_create(&(lock->key), (void *) 0); + +} + +void guac_destroy_reentrant_rwlock(guac_reentrant_rwlock* lock) { + + /* Destroy the rwlock */ + pthread_rwlock_destroy(&(lock->lock)); + + /* Destroy the thread-local key */ + pthread_key_delete(lock->key); + +} + +/** + * Clean up and destroy the provided guac reentrant rwlock. + * + * @param lock + * The guac reentrant rwlock to be destroyed. + */ +void guac_destroy_reentrant_rwlock(guac_reentrant_rwlock* lock); + +/** + * Extract and return the flag indicating which lock is held, if any, from the + * provided key value. The flag is always stored in the least-significant + * nibble of the value. + * + * @param value + * The key value containing the flag. + * + * @return + * The flag indicating which lock is held, if any. + */ +static uintptr_t get_lock_flag(uintptr_t value) { + return value & 0xF; +} + +/** + * Extract and return the lock count from the provided key. This returned value + * is the difference between the number of lock and unlock requests made by the + * current thread. This count is always stored in the remaining value after the + * least-significant nibble where the flag is stored. + * + * @param value + * The key value containing the count. + * + * @return + * The difference between the number of lock and unlock requests made by + * the current thread. + */ +static uintptr_t get_lock_count(uintptr_t value) { + return value >> 4; +} + +/** + * Given a flag indicating if and how the current thread controls a lock, and + * a count of the depth of lock requests, return a value containing the flag + * in the least-significant nibble, and the count in the rest. + * + * @param flag + * A flag indiciating which lock, if any, is held by the current thread. + * + * @param count + * The depth of the lock attempt by the current thread, i.e. the number of + * lock requests minus unlock requests. + * + * @return + * A value containing the flag in the least-significant nibble, and the + * count in the rest, cast to a void* for thread-local storage. + */ +static void* get_value_from_flag_and_count( + uintptr_t flag, uintptr_t count) { + return (void*) ((flag & 0xF) | count << 4); +} + +/** + * Return zero if adding one to the current count would overflow the storage + * allocated to the count, or a non-zero value otherwise. + * + * @param current_count + * The current count for a lock that the current thread is trying to + * reentrantly acquire. + * + * @return + * Zero if adding one to the current count would overflow the storage + * allocated to the count, or a non-zero value otherwise. + */ +static int would_overflow_count(uintptr_t current_count) { + + /** + * The count will overflow if it's already equal or greated to the maximum + * possible value that can be stored in a uintptr_t excluding the first nibble. + */ + return current_count >= (UINTPTR_MAX >> 4); + +} + +int guac_acquire_write_lock(guac_reentrant_rwlock* reentrant_rwlock) { + + uintptr_t key_value = (uintptr_t) pthread_getspecific(reentrant_rwlock->key); + uintptr_t flag = get_lock_flag(key_value); + uintptr_t count = get_lock_count(key_value); + + /* If acquiring this lock again would overflow the counter storage */ + if (would_overflow_count(count)) + return GUAC_REEANTRANT_LOCK_ERROR_TOO_MANY; + + /* If the current thread already holds the write lock, increment the count */ + if (flag == GUAC_REENTRANT_LOCK_WRITE_LOCK) { + pthread_setspecific(reentrant_rwlock->key, get_value_from_flag_and_count( + flag, count + 1)); + + /* This thread already has the lock */ + return 0; + } + + /* + * The read lock must be released before the write lock can be acquired. + * This is a little odd because it may mean that a function further down + * the stack may have requested a read lock, which will get upgraded to a + * write lock by another function without the caller knowing about it. This + * shouldn't cause any issues, however. + */ + if (key_value == GUAC_REENTRANT_LOCK_READ_LOCK) + pthread_rwlock_unlock(&(reentrant_rwlock->lock)); + + /* Acquire the write lock */ + pthread_rwlock_wrlock(&(reentrant_rwlock->lock)); + + /* Mark that the current thread has the lock, and increment the count */ + pthread_setspecific(reentrant_rwlock->key, get_value_from_flag_and_count( + GUAC_REENTRANT_LOCK_WRITE_LOCK, count + 1)); + + return 0; + +} + +int guac_acquire_read_lock(guac_reentrant_rwlock* reentrant_rwlock) { + + uintptr_t key_value = (uintptr_t) pthread_getspecific(reentrant_rwlock->key); + uintptr_t flag = get_lock_flag(key_value); + uintptr_t count = get_lock_count(key_value); + + /* If acquiring this lock again would overflow the counter storage */ + if (would_overflow_count(count)) + return GUAC_REEANTRANT_LOCK_ERROR_TOO_MANY; + + /* The current thread may read if either the read or write lock is held */ + if ( + flag == GUAC_REENTRANT_LOCK_READ_LOCK || + flag == GUAC_REENTRANT_LOCK_WRITE_LOCK + ) { + + /* Increment the depth counter */ + pthread_setspecific(reentrant_rwlock->key, get_value_from_flag_and_count( + flag, count + 1)); + + /* This thread already has the lock */ + return 0; + } + + /* Acquire the lock */ + pthread_rwlock_rdlock(&(reentrant_rwlock->lock)); + + /* Set the flag that the current thread has the read lock */ + pthread_setspecific(reentrant_rwlock->key, get_value_from_flag_and_count( + GUAC_REENTRANT_LOCK_READ_LOCK, 1)); + + return 0; + +} + +int guac_release_lock(guac_reentrant_rwlock* reentrant_rwlock) { + + uintptr_t key_value = (uintptr_t) pthread_getspecific(reentrant_rwlock->key); + uintptr_t flag = get_lock_flag(key_value); + uintptr_t count = get_lock_count(key_value); + + /* + * Return an error if an attempt is made to release a lock that the current + * thread does not control. + */ + if (count <= 0) + return GUAC_REEANTRANT_LOCK_ERROR_DOUBLE_RELEASE; + + /* Release the lock if this is the last locked level */ + if (count == 1) { + + pthread_rwlock_unlock(&(reentrant_rwlock->lock)); + + /* Set the flag that the current thread holds no locks */ + pthread_setspecific(reentrant_rwlock->key, get_value_from_flag_and_count( + GUAC_REENTRANT_LOCK_NO_LOCK, 0)); + + return 0; + } + + /* Do not release the lock since it's still in use - just decrement */ + pthread_setspecific(reentrant_rwlock->key, get_value_from_flag_and_count( + flag, count - 1)); + + return 0; + +} diff --git a/src/libguac/reentrant-rwlock.h b/src/libguac/reentrant-rwlock.h new file mode 100644 index 000000000..92bc5b6a0 --- /dev/null +++ b/src/libguac/reentrant-rwlock.h @@ -0,0 +1,144 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +#ifndef __GUAC_REENTRANT_LOCK_H +#define __GUAC_REENTRANT_LOCK_H + +#include + +/** + * This file implements reentrant read-write locks using thread-local storage + * to keep track of how locks are held and released by the current thread, + * since the pthread locks do not support reentrant behavior. + * + * A thread will attempt to acquire the requested lock on the first acquire + * function call, and will release it once the number of unlock requests + * matches the number of lock requests. Therefore, it is safe to aquire a lock + * and then call a function that also acquires the same lock, provided that + * the caller and the callee request to unlock the lock when done with it. + * + * Any lock that's locked using one of the functions defined in this file + * must _only_ be unlocked using the unlock function defined here to avoid + * unexpected behavior. + */ + +/** + * An error code indicating that the calling thread is attempting to release a + * lock that it does not control. + */ +#define GUAC_REEANTRANT_LOCK_ERROR_DOUBLE_RELEASE 1 + +/** + * The lock cannot be acquired because the lock has been already been + * reentrantly acquired too many times, exhausting the capacity of this library + * to track this lock. The lock must be released using guac_release_lock() + * before it can be reacquired. + */ +#define GUAC_REEANTRANT_LOCK_ERROR_TOO_MANY 2 + +/** + * A structure packaging together a pthread rwlock along with a key to a + * thread-local property to keep track of the current status of the lock, + * allowing the functions defined in this header to provide reentrant behavior. + * Note that both the lock and key must be initialized before being provided + * to any of these functions. + */ +typedef struct guac_reentrant_rwlock { + + /** + * A non-reentrant pthread rwlock to be wrapped by the local lock, + * functions providing reentrant behavior. + */ + pthread_rwlock_t lock; + + /** + * A key to access a thread-local property tracking any ownership of the + * lock by the current thread. + */ + pthread_key_t key; + +} guac_reentrant_rwlock; + +/** + * Initialize the provided guac reentrant rwlock. The lock will be configured to be + * visible to child processes. + * + * @param lock + * The guac reentrant rwlock to be initialized. + */ +void guac_init_reentrant_rwlock(guac_reentrant_rwlock* lock); + +/** + * Clean up and destroy the provided guac reentrant rwlock. + * + * @param lock + * The guac reentrant rwlock to be destroyed. + */ +void guac_destroy_reentrant_rwlock(guac_reentrant_rwlock* lock); + +/** + * Aquire the write lock for the provided guac reentrant rwlock, if the key does not + * indicate that the write lock is already acquired. If the key indicates that + * the read lock is already acquired, the read lock will be dropped before the + * write lock is acquired. The thread local property associated with the key + * will be updated as necessary to track the thread's ownership of the lock. + * + * @param reentrant_rwlock + * The guac reentrant rwlock for which the write lock should be acquired + * reentrantly. + * + * @return + * Zero if the lock is succesfully acquired, or an error code defined above + * by a GUAC_REEANTRANT_LOCK_ERROR_* constant if the lock cannot be acquired. + */ +int guac_acquire_write_lock(guac_reentrant_rwlock* reentrant_rwlock); + +/** + * Aquire the read lock for the provided guac reentrant rwlock, if the key does not + * indicate that the read or write lock is already acquired. The thread local + * property associated with the key will be updated as necessary to track the + * thread's ownership of the lock. + * + * @param reentrant_rwlock + * The guac reentrant rwlock for which the read lock should be acquired + * reentrantly. + * + * @return + * Zero if the lock is succesfully acquired, or an error code defined above + * by a GUAC_REEANTRANT_LOCK_ERROR_* constant if the lock cannot be acquired. + */ +int guac_acquire_read_lock(guac_reentrant_rwlock* reentrant_rwlock); + +/** + * Release the the rwlock associated with the provided guac reentrant rwlock if this + * is the last level of the lock held by this thread. Otherwise, the thread + * local property associated with the key will be updated as needed to ensure + * that the correct number of release requests will finally release the lock. + * + * @param reentrant_rwlock + * The guac reentrant rwlock that should be released. + * + * @return + * Zero if the lock is succesfully released, or an error code defined above + * by a GUAC_REEANTRANT_LOCK_ERROR_* constant if the lock cannot be released. + */ +int guac_release_lock(guac_reentrant_rwlock* reentrant_rwlock); + +#endif + diff --git a/src/protocols/kubernetes/client.c b/src/protocols/kubernetes/client.c index 42a18286e..ef0a502e5 100644 --- a/src/protocols/kubernetes/client.c +++ b/src/protocols/kubernetes/client.c @@ -25,6 +25,7 @@ #include #include +#include #include #include @@ -77,6 +78,47 @@ static void guac_kubernetes_log(int level, const char* line) { } +/** + * Synchronize the connection state for the given pending user. + * + * @param user + * The pending user whose connection state should be synced. + * + * @param data + * Unused. + * + * @return + * Always NULL. + */ +static void* guac_kubernetes_sync_pending_user(guac_user* user, void* data) { + + guac_client* client = user->client; + guac_kubernetes_client* kubernetes_client = + (guac_kubernetes_client*) client->data; + + guac_terminal_dup(kubernetes_client->term, user, user->socket); + guac_kubernetes_send_current_argv(user, kubernetes_client); + guac_socket_flush(user->socket); + + return NULL; + +} + +/** + * A pending join handler implementation that will synchronize the connection + * state for all pending users prior to them being promoted to full user. + * + * @param client + * The client whose pending users are about to be promoted. + */ +static void guac_kubernetes_join_pending_handler(guac_client* client) { + + /* Synchronize each user one at a time */ + guac_client_foreach_pending_user( + client, guac_kubernetes_sync_pending_user, NULL); + +} + int guac_client_init(guac_client* client) { /* Ensure reference to main guac_client remains available in all @@ -96,6 +138,7 @@ int guac_client_init(guac_client* client) { /* Set handlers */ client->join_handler = guac_kubernetes_user_join_handler; + client->join_pending_handler = guac_kubernetes_join_pending_handler; client->free_handler = guac_kubernetes_client_free_handler; client->leave_handler = guac_kubernetes_user_leave_handler; diff --git a/src/protocols/kubernetes/user.c b/src/protocols/kubernetes/user.c index a1c067df9..369923de2 100644 --- a/src/protocols/kubernetes/user.c +++ b/src/protocols/kubernetes/user.c @@ -71,13 +71,6 @@ int guac_kubernetes_user_join_handler(guac_user* user, int argc, char** argv) { } - /* If not owner, synchronize with current display */ - else { - guac_terminal_dup(kubernetes_client->term, user, user->socket); - guac_kubernetes_send_current_argv(user, kubernetes_client); - guac_socket_flush(user->socket); - } - /* Only handle events if not read-only */ if (!settings->read_only) { diff --git a/src/protocols/rdp/client.c b/src/protocols/rdp/client.c index aed5ea68e..5071a3a97 100644 --- a/src/protocols/rdp/client.c +++ b/src/protocols/rdp/client.c @@ -21,6 +21,7 @@ #include "channels/audio-input/audio-buffer.h" #include "channels/cliprdr.h" #include "channels/disp.h" +#include "channels/pipe-svc.h" #include "config.h" #include "fs.h" #include "log.h" @@ -78,6 +79,53 @@ static int is_writable_directory(const char* path) { } +/** + * Synchronize the connection state for the given pending user. + * + * @param user + * The pending user whose connection state should be synced. + * + * @param data + * Unused. + * + * @return + * Always NULL. + */ +static void* guac_rdp_sync_pending_user(guac_user* user, void* data) { + + guac_rdp_client* rdp_client = (guac_rdp_client*) user->client->data; + + /* Synchronize any audio stream */ + if (rdp_client->audio) + guac_audio_stream_add_user(rdp_client->audio, user); + + /* Bring user up to date with any registered static channels */ + guac_rdp_pipe_svc_send_pipes(user); + + /* Synchronize with current display */ + guac_common_display_dup(rdp_client->display, user, user->socket); + + guac_socket_flush(user->socket); + + return NULL; + +} + +/** + * A pending join handler implementation that will synchronize the connection + * state for all pending users prior to them being promoted to full user. + * + * @param client + * The client whose pending users are about to be promoted. + */ +static void guac_rdp_join_pending_handler(guac_client* client) { + + /* Synchronize each user one at a time */ + guac_client_foreach_pending_user( + client, guac_rdp_sync_pending_user, NULL); + +} + int guac_client_init(guac_client* client, int argc, char** argv) { /* Automatically set HOME environment variable if unset (FreeRDP's @@ -164,6 +212,7 @@ int guac_client_init(guac_client* client, int argc, char** argv) { /* Set handlers */ client->join_handler = guac_rdp_user_join_handler; + client->join_pending_handler = guac_rdp_join_pending_handler; client->free_handler = guac_rdp_client_free_handler; client->leave_handler = guac_rdp_user_leave_handler; diff --git a/src/protocols/rdp/user.c b/src/protocols/rdp/user.c index 60b6f07a7..5381a9822 100644 --- a/src/protocols/rdp/user.c +++ b/src/protocols/rdp/user.c @@ -82,22 +82,6 @@ int guac_rdp_user_join_handler(guac_user* user, int argc, char** argv) { } - /* If not owner, synchronize with current state */ - else { - - /* Synchronize any audio stream */ - if (rdp_client->audio) - guac_audio_stream_add_user(rdp_client->audio, user); - - /* Bring user up to date with any registered static channels */ - guac_rdp_pipe_svc_send_pipes(user); - - /* Synchronize with current display */ - guac_common_display_dup(rdp_client->display, user, user->socket); - guac_socket_flush(user->socket); - - } - /* Only handle events if not read-only */ if (!settings->read_only) { diff --git a/src/protocols/ssh/client.c b/src/protocols/ssh/client.c index 4a314f061..4b1c5393d 100644 --- a/src/protocols/ssh/client.c +++ b/src/protocols/ssh/client.c @@ -34,6 +34,47 @@ #include #include #include +#include + +/** + * Synchronize the connection state for the given pending user. + * + * @param user + * The pending user whose connection state should be synced. + * + * @param data + * Unused. + * + * @return + * Always NULL. + */ +static void* guac_ssh_sync_pending_user(guac_user* user, void* data) { + + guac_client* client = user->client; + guac_ssh_client* ssh_client = (guac_ssh_client*) client->data; + + guac_terminal_dup(ssh_client->term, user, user->socket); + guac_ssh_send_current_argv(user, ssh_client); + guac_socket_flush(user->socket); + + return NULL; + +} + +/** + * A pending join handler implementation that will synchronize the connection + * state for all pending users prior to them being promoted to full user. + * + * @param client + * The client whose pending users are about to be promoted. + */ +static void guac_ssh_join_pending_handler(guac_client* client) { + + /* Synchronize each user one at a time */ + guac_client_foreach_pending_user( + client, guac_ssh_sync_pending_user, NULL); + +} int guac_client_init(guac_client* client) { @@ -46,6 +87,7 @@ int guac_client_init(guac_client* client) { /* Set handlers */ client->join_handler = guac_ssh_user_join_handler; + client->join_pending_handler = guac_ssh_join_pending_handler; client->free_handler = guac_ssh_client_free_handler; client->leave_handler = guac_ssh_user_leave_handler; diff --git a/src/protocols/ssh/user.c b/src/protocols/ssh/user.c index 124884028..eff0d8b5d 100644 --- a/src/protocols/ssh/user.c +++ b/src/protocols/ssh/user.c @@ -73,13 +73,6 @@ int guac_ssh_user_join_handler(guac_user* user, int argc, char** argv) { } - /* If not owner, synchronize with current display */ - else { - guac_terminal_dup(ssh_client->term, user, user->socket); - guac_ssh_send_current_argv(user, ssh_client); - guac_socket_flush(user->socket); - } - /* Only handle events if not read-only */ if (!settings->read_only) { diff --git a/src/protocols/telnet/client.c b/src/protocols/telnet/client.c index 27edfcdac..71bf1b0a5 100644 --- a/src/protocols/telnet/client.c +++ b/src/protocols/telnet/client.c @@ -34,6 +34,47 @@ #include #include #include +#include + +/** + * Synchronize the connection state for the given pending user. + * + * @param user + * The pending user whose connection state should be synced. + * + * @param data + * Unused. + * + * @return + * Always NULL. + */ +static void* guac_telnet_sync_pending_user(guac_user* user, void* data) { + + guac_client* client = user->client; + guac_telnet_client* telnet_client = (guac_telnet_client*) client->data; + + guac_terminal_dup(telnet_client->term, user, user->socket); + guac_telnet_send_current_argv(user, telnet_client); + guac_socket_flush(user->socket); + + return NULL; + +} + +/** + * A pending join handler implementation that will synchronize the connection + * state for all pending users prior to them being promoted to full user. + * + * @param client + * The client whose pending users are about to be promoted. + */ +static void guac_telnet_join_pending_handler(guac_client* client) { + + /* Synchronize each user one at a time */ + guac_client_foreach_pending_user( + client, guac_telnet_sync_pending_user, NULL); + +} int guac_client_init(guac_client* client) { @@ -51,6 +92,7 @@ int guac_client_init(guac_client* client) { /* Set handlers */ client->join_handler = guac_telnet_user_join_handler; + client->join_pending_handler = guac_telnet_join_pending_handler; client->free_handler = guac_telnet_client_free_handler; client->leave_handler = guac_telnet_user_leave_handler; diff --git a/src/protocols/vnc/client.c b/src/protocols/vnc/client.c index 638c14de8..14dbe0803 100644 --- a/src/protocols/vnc/client.c +++ b/src/protocols/vnc/client.c @@ -40,6 +40,51 @@ #include #include +/** + * Synchronize the connection state for the given pending user. + * + * @param user + * The pending user whose connection state should be synced. + * + * @param data + * Unused. + * + * @return + * Always NULL. + */ +static void* guac_vnc_sync_pending_user(guac_user* user, void* data) { + + guac_vnc_client* vnc_client = (guac_vnc_client*) user->client->data; + +#ifdef ENABLE_PULSE + /* Synchronize an audio stream */ + if (vnc_client->audio) + guac_pa_stream_add_user(vnc_client->audio, user); +#endif + + /* Synchronize with current display */ + guac_common_display_dup(vnc_client->display, user, user->socket); + guac_socket_flush(user->socket); + + return NULL; + +} + +/** + * A pending join handler implementation that will synchronize the connection + * state for all pending users prior to them being promoted to full user. + * + * @param client + * The client whose pending users are about to be promoted. + */ +static void guac_vnc_join_pending_handler(guac_client* client) { + + /* Synchronize each user one at a time */ + guac_client_foreach_pending_user( + client, guac_vnc_sync_pending_user, NULL); + +} + int guac_client_init(guac_client* client) { /* Set client args */ @@ -59,6 +104,7 @@ int guac_client_init(guac_client* client) { /* Set handlers */ client->join_handler = guac_vnc_user_join_handler; + client->join_pending_handler = guac_vnc_join_pending_handler; client->leave_handler = guac_vnc_user_leave_handler; client->free_handler = guac_vnc_client_free_handler; From a7443a521c3c19e2b1aeec6ee181b7a32fe86674 Mon Sep 17 00:00:00 2001 From: James Muehlner Date: Tue, 22 Aug 2023 17:46:53 +0000 Subject: [PATCH 2/6] GUACAMOLE-1846: Fix __write_all() to return length as documented. --- src/guacd/connection.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/guacd/connection.c b/src/guacd/connection.c index a5b98adfa..1bb99dc08 100644 --- a/src/guacd/connection.c +++ b/src/guacd/connection.c @@ -67,13 +67,14 @@ static int __write_all(int fd, char* buffer, int length) { /* Repeatedly write() until all data is written */ - while (length > 0) { + int remaining_length = length; + while (remaining_length > 0) { - int written = write(fd, buffer, length); + int written = write(fd, buffer, remaining_length); if (written < 0) return -1; - length -= written; + remaining_length -= written; buffer += written; } From 317e733463f40f86859d5304b58ba4a248158e62 Mon Sep 17 00:00:00 2001 From: James Muehlner Date: Tue, 22 Aug 2023 19:11:53 +0000 Subject: [PATCH 3/6] GUACAMOLE-1846: Sync data to all pending users using broadcast socket. --- src/common/common/cursor.h | 6 +- src/common/common/display.h | 7 +- src/common/common/surface.h | 8 +-- src/common/cursor.c | 6 +- src/common/display.c | 19 +++--- src/common/surface.c | 7 +- src/libguac/client.c | 8 ++- src/libguac/guacamole/client-fntypes.h | 8 ++- src/libguac/guacamole/client.h | 26 +++++--- src/libguac/guacamole/socket.h | 36 +++++++++-- src/libguac/socket-broadcast.c | 90 ++++++++++++++++++++------ src/protocols/kubernetes/argv.c | 16 +++-- src/protocols/kubernetes/argv.h | 21 +++++- src/protocols/kubernetes/client.c | 40 +++--------- src/protocols/rdp/channels/pipe-svc.c | 8 +-- src/protocols/rdp/channels/pipe-svc.h | 16 +++-- src/protocols/rdp/client.c | 42 ++++++------ src/protocols/ssh/argv.c | 19 ++++-- src/protocols/ssh/argv.h | 18 ++++++ src/protocols/ssh/client.c | 34 ++-------- src/protocols/telnet/argv.c | 20 ++++-- src/protocols/telnet/argv.h | 19 ++++++ src/protocols/telnet/client.c | 35 ++-------- src/protocols/telnet/user.c | 7 -- src/protocols/vnc/client.c | 40 ++++++------ src/protocols/vnc/user.c | 15 ----- src/terminal/display.c | 7 +- src/terminal/scrollbar.c | 3 +- src/terminal/terminal.c | 41 ++++++++++-- src/terminal/terminal/display.h | 18 +++--- src/terminal/terminal/scrollbar.h | 12 ++-- src/terminal/terminal/terminal.h | 21 ++++++ 32 files changed, 412 insertions(+), 261 deletions(-) diff --git a/src/common/common/cursor.h b/src/common/common/cursor.h index 52d93c2ae..208f61f3a 100644 --- a/src/common/common/cursor.h +++ b/src/common/common/cursor.h @@ -153,14 +153,14 @@ void guac_common_cursor_free(guac_common_cursor* cursor); * @param cursor * The cursor to send. * - * @param user + * @param client * The user receiving the updated cursor. * * @param socket * The socket over which the updated cursor should be sent. */ -void guac_common_cursor_dup(guac_common_cursor* cursor, guac_user* user, - guac_socket* socket); +void guac_common_cursor_dup( + guac_common_cursor* cursor, guac_client* client, guac_socket* socket); /** * Updates the current position and button state of the mouse cursor, marking diff --git a/src/common/common/display.h b/src/common/common/display.h index c54289021..33950a177 100644 --- a/src/common/common/display.h +++ b/src/common/common/display.h @@ -151,13 +151,14 @@ void guac_common_display_free(guac_common_display* display); * @param display * The display whose state should be sent along the given socket. * - * @param user - * The user receiving the display state. + * @param client + * The client associated with the users receiving the display state. * * @param socket * The socket over which the display state should be sent. */ -void guac_common_display_dup(guac_common_display* display, guac_user* user, +void guac_common_display_dup( + guac_common_display* display, guac_client* client, guac_socket* socket); /** diff --git a/src/common/common/surface.h b/src/common/common/surface.h index b43dcaaf5..09e5b0d5b 100644 --- a/src/common/common/surface.h +++ b/src/common/common/surface.h @@ -490,14 +490,14 @@ void guac_common_surface_flush(guac_common_surface* surface); * @param surface * The surface to duplicate. * - * @param user - * The user receiving the surface. + * @param client + * The client whos users are receiving the surface. * * @param socket * The socket over which the surface contents should be sent. */ -void guac_common_surface_dup(guac_common_surface* surface, guac_user* user, - guac_socket* socket); +void guac_common_surface_dup(guac_common_surface* surface, + guac_client* client, guac_socket* socket); /** * Declares that the given surface should receive touch events. By default, diff --git a/src/common/cursor.c b/src/common/cursor.c index b70134d81..78c4dcf1e 100644 --- a/src/common/cursor.c +++ b/src/common/cursor.c @@ -99,8 +99,8 @@ void guac_common_cursor_free(guac_common_cursor* cursor) { } -void guac_common_cursor_dup(guac_common_cursor* cursor, guac_user* user, - guac_socket* socket) { +void guac_common_cursor_dup( + guac_common_cursor* cursor, guac_client* client, guac_socket* socket) { /* Synchronize location */ guac_protocol_send_mouse(socket, cursor->x, cursor->y, cursor->button_mask, @@ -111,7 +111,7 @@ void guac_common_cursor_dup(guac_common_cursor* cursor, guac_user* user, guac_protocol_send_size(socket, cursor->buffer, cursor->width, cursor->height); - guac_user_stream_png(user, socket, GUAC_COMP_SRC, + guac_client_stream_png(client, socket, GUAC_COMP_SRC, cursor->buffer, 0, 0, cursor->surface); guac_protocol_send_cursor(socket, diff --git a/src/common/display.c b/src/common/display.c index 9b4b87f0e..0fddb9b06 100644 --- a/src/common/display.c +++ b/src/common/display.c @@ -37,20 +37,20 @@ * The head element of the linked list of layers to synchronize, which may * be NULL if the list is currently empty. * - * @param user - * The user receiving the layers. + * @param client + * The client associated with the users receiving the layers. * * @param socket * The socket over which each layer should be sent. */ static void guac_common_display_dup_layers(guac_common_display_layer* layers, - guac_user* user, guac_socket* socket) { + guac_client* client, guac_socket* socket) { guac_common_display_layer* current = layers; /* Synchronize all surfaces in given list */ while (current != NULL) { - guac_common_surface_dup(current->surface, user, socket); + guac_common_surface_dup(current->surface, client, socket); current = current->next; } @@ -163,20 +163,21 @@ void guac_common_display_free(guac_common_display* display) { } -void guac_common_display_dup(guac_common_display* display, guac_user* user, +void guac_common_display_dup( + guac_common_display* display, guac_client* client, guac_socket* socket) { pthread_mutex_lock(&display->_lock); /* Sunchronize shared cursor */ - guac_common_cursor_dup(display->cursor, user, socket); + guac_common_cursor_dup(display->cursor, client, socket); /* Synchronize default surface */ - guac_common_surface_dup(display->default_surface, user, socket); + guac_common_surface_dup(display->default_surface, client, socket); /* Synchronize all layers and buffers */ - guac_common_display_dup_layers(display->layers, user, socket); - guac_common_display_dup_layers(display->buffers, user, socket); + guac_common_display_dup_layers(display->layers, client, socket); + guac_common_display_dup_layers(display->buffers, client, socket); pthread_mutex_unlock(&display->_lock); diff --git a/src/common/surface.c b/src/common/surface.c index 61b77c7d2..e5d1ca9f3 100644 --- a/src/common/surface.c +++ b/src/common/surface.c @@ -1989,8 +1989,8 @@ void guac_common_surface_flush(guac_common_surface* surface) { } -void guac_common_surface_dup(guac_common_surface* surface, guac_user* user, - guac_socket* socket) { +void guac_common_surface_dup(guac_common_surface* surface, + guac_client* client, guac_socket* socket) { pthread_mutex_lock(&surface->_lock); @@ -2028,7 +2028,7 @@ void guac_common_surface_dup(guac_common_surface* surface, guac_user* user, surface->width, surface->height, surface->stride); /* Send PNG for rect */ - guac_user_stream_png(user, socket, GUAC_COMP_OVER, surface->layer, + guac_client_stream_png(client, socket, GUAC_COMP_OVER, surface->layer, 0, 0, rect); cairo_surface_destroy(rect); @@ -2038,4 +2038,3 @@ void guac_common_surface_dup(guac_common_surface* surface, guac_user* user, pthread_mutex_unlock(&surface->_lock); } - diff --git a/src/libguac/client.c b/src/libguac/client.c index 1c74722c9..995fc5623 100644 --- a/src/libguac/client.c +++ b/src/libguac/client.c @@ -193,7 +193,7 @@ static void guac_client_promote_pending_users(union sigval data) { guac_release_lock(&(client->__users_lock)); - /* Release the lock (this is done AFTER updating the non-pending user list + /* Release the lock (this is done AFTER updating the connected user list * to ensure that all users are always on exactly one of these lists) */ guac_release_lock(&(client->__pending_users_lock)); @@ -253,8 +253,9 @@ guac_client* guac_client_alloc() { /* Ensure the timer is constructed only once */ pthread_mutex_init(&(client->__pending_users_timer_mutex), NULL); - /* Set up socket to broadcast to all users */ + /* Set up broadcast sockets */ client->socket = guac_socket_broadcast(client); + client->pending_socket = guac_socket_broadcast_pending(client); /* Set the timer event thread as initially inactive, since it hasn't run */ atomic_flag_clear(&(client->__pending_timer_event_active)); @@ -280,8 +281,9 @@ void guac_client_free(guac_client* client) { } - /* Free socket */ + /* Free sockets */ guac_socket_free(client->socket); + guac_socket_free(client->pending_socket); /* Free layer pools */ guac_pool_free(client->__buffer_pool); diff --git a/src/libguac/guacamole/client-fntypes.h b/src/libguac/guacamole/client-fntypes.h index 3cf76f938..26cddb6f8 100644 --- a/src/libguac/guacamole/client-fntypes.h +++ b/src/libguac/guacamole/client-fntypes.h @@ -30,7 +30,9 @@ #include "client-types.h" #include "object-types.h" #include "protocol-types.h" +#include "socket.h" #include "stream-types.h" +#include "user-fntypes.h" #include "user-types.h" #include @@ -49,9 +51,9 @@ typedef int guac_client_free_handler(guac_client* client); /** - * Handler that will run before pending users are promoted to full users. - * Any required operations for pending users should be applied using - * guac_client_foreach_pending_user(). + * Handler that will run before immediately before pending users are promoted + * to full users. The pending user socket should be used to communicate to the + * pending users. * * @param client * The client whose handler was invoked. diff --git a/src/libguac/guacamole/client.h b/src/libguac/guacamole/client.h index d5329a013..b7d1b4845 100644 --- a/src/libguac/guacamole/client.h +++ b/src/libguac/guacamole/client.h @@ -49,16 +49,24 @@ struct guac_client { /** - * The guac_socket structure to be used to communicate with all connected - * web-clients (users). Unlike the user-level guac_socket, this guac_socket - * will broadcast instructions to all connected users simultaneously. It - * is expected that the implementor of any Guacamole proxy client will - * provide their own mechanism of I/O for their protocol. The guac_socket - * structure is used only to communicate conveniently with the Guacamole - * web-client. + * The guac_socket structure to be used to communicate with all non-pending + * connected web-clients (users). Unlike the user-level guac_socket, this + * guac_socket will broadcast instructions to all non-pending connected users + * simultaneously. It is expected that the implementor of any Guacamole proxy + * client will provide their own mechanism of I/O for their protocol. The + * guac_socket structure is used only to communicate conveniently with the + * Guacamole web-client. */ guac_socket* socket; + /** + * The guac_socket structure to be used to communicate with all pending + * connected web-clients (users). Aside from operating on a different + * subset of users, this socket has all the same behavior and semantics as + * the non-pending socket. + */ + guac_socket* pending_socket; + /** * The current state of the client. When the client is first allocated, * this will be initialized to GUAC_CLIENT_RUNNING. It will remain at @@ -248,8 +256,8 @@ struct guac_client { /** * A handler that will be run prior to pending users being promoted to full - * users. Any required pending user operations should be applied - * guac_client_foreach_pending_user(). + * users. Any required pending user operations should be performed using + * the client's pending user socket. * * Example: * @code diff --git a/src/libguac/guacamole/socket.h b/src/libguac/guacamole/socket.h index 2da697fed..bb5711c04 100644 --- a/src/libguac/guacamole/socket.h +++ b/src/libguac/guacamole/socket.h @@ -230,10 +230,10 @@ guac_socket* guac_socket_tee(guac_socket* primary, guac_socket* secondary); /** * Allocates and initializes a new guac_socket which duplicates all - * instructions written across the sockets of each connected user of the given - * guac_client. The returned socket is a write-only socket. Attempts to read - * from the socket will fail. If a write occurs while no users are connected, - * that write will simply be dropped. + * instructions written across the sockets of each connected user of the + * given guac_client. The returned socket is a write-only socket. Attempts + * to read from the socket will fail. If a write occurs while no users are + * connected, that write will simply be dropped. * * Return values (error codes) from each user's socket will not affect the * in-progress write, but each failing user will be forcibly stopped with @@ -248,12 +248,38 @@ guac_socket* guac_socket_tee(guac_socket* primary, guac_socket* secondary); * * @return * A write-only guac_socket object which broadcasts copies of all - * instructions written across all connected users of the given + * instructions written across all non-pending connected users of the given * guac_client, or NULL if an error occurs while allocating the guac_socket * object. */ guac_socket* guac_socket_broadcast(guac_client* client); +/** + * Allocates and initializes a new guac_socket which duplicates all + * instructions written across the sockets of each pending connected + * user of the given guac_client. The returned socket is a write-only socket. + * Attempts to read from the socket will fail. If a write occurs while no + * users are connected, that write will simply be dropped. + * + * Return values (error codes) from each user's socket will not affect the + * in-progress write, but each failing user will be forcibly stopped with + * guac_user_stop(). + * + * If an error occurs while allocating the guac_socket object, NULL is returned, + * and guac_error is set appropriately. + * + * @param client + * The client associated with the group of pending users across which + * duplicates of all instructions should be written. + * + * @return + * A write-only guac_socket object which broadcasts copies of all + * instructions written across all pending connected users of the given + * guac_client, or NULL if an error occurs while allocating the guac_socket + * object. + */ +guac_socket* guac_socket_broadcast_pending(guac_client* client); + /** * Writes the given unsigned int to the given guac_socket object. The data * written may be buffered until the buffer is flushed automatically or diff --git a/src/libguac/socket-broadcast.c b/src/libguac/socket-broadcast.c index f551e8172..49f6672ed 100644 --- a/src/libguac/socket-broadcast.c +++ b/src/libguac/socket-broadcast.c @@ -28,8 +28,25 @@ #include /** - * Data associated with an open socket which writes to all connected users of - * a particular guac_client. + * A function that will broadcast arbitrary data to a subset of users for + * the provided client, using the provided user callback for any user-specific + * operations. + * + * @param client + * The guac_client associated with the users to broadcast to. + * + * @param callback + * A callback that should be invoked with each broadcasted user. + * + * @param data + * Arbitrary data that may be used to broadcast to the subset of users. + */ +typedef void guac_socket_broadcast_handler( + guac_client* client, guac_user_callback* callback, void* data); + +/** + * Data associated with an open socket which writes to a subset of connected + * users of a particular guac_client. */ typedef struct guac_socket_broadcast_data { @@ -45,6 +62,11 @@ typedef struct guac_socket_broadcast_data { */ pthread_mutex_t socket_lock; + /** + * The function to broadcast + */ + guac_socket_broadcast_handler* broadcast_handler; + } guac_socket_broadcast_data; /** @@ -91,7 +113,7 @@ static ssize_t __guac_socket_broadcast_read_handler(guac_socket* socket, } /** - * Callback invoked by guac_client_foreach_user() which write a given chunk of + * Callback invoked by the broadcast handler which write a given chunk of * data to that user's socket. If the write attempt fails, the user is * signalled to stop with guac_user_stop(). * @@ -146,15 +168,15 @@ static ssize_t __guac_socket_broadcast_write_handler(guac_socket* socket, chunk.buffer = buf; chunk.length = count; - /* Broadcast chunk to all users */ - guac_client_foreach_user(data->client, __write_chunk_callback, &chunk); + /* Broadcast chunk to the users */ + data->broadcast_handler(data->client, __write_chunk_callback, &chunk); return count; } /** - * Callback which is invoked by guac_client_foreach_user() to flush all + * Callback which is invoked by the broadcast handler to flush all * pending data on the given user's socket. If an error occurs while flushing * a user's socket, that user is signalled to stop with guac_user_stop(). * @@ -162,7 +184,7 @@ static ssize_t __guac_socket_broadcast_write_handler(guac_socket* socket, * The user whose socket should be flushed. * * @param data - * Arbitrary data passed to guac_client_foreach_user(). This is not needed + * Arbitrary data passed to the broadcast handler. This is not needed * by this callback, and should be left as NULL. * * @return @@ -195,15 +217,15 @@ static ssize_t __guac_socket_broadcast_flush_handler(guac_socket* socket) { guac_socket_broadcast_data* data = (guac_socket_broadcast_data*) socket->data; - /* Flush all users */ - guac_client_foreach_user(data->client, __flush_callback, NULL); + /* Flush the users */ + data->broadcast_handler(data->client, __flush_callback, NULL); return 0; } /** - * Callback which is invoked by guac_client_foreach_user() to lock the given + * Callback which is invoked by the broadcast handler to lock the given * user's socket in preparation for the beginning of a Guacamole protocol * instruction. * @@ -211,7 +233,7 @@ static ssize_t __guac_socket_broadcast_flush_handler(guac_socket* socket) { * The user whose socket should be locked. * * @param data - * Arbitrary data passed to guac_client_foreach_user(). This is not needed + * Arbitrary data passed to the broadcast handler. This is not needed * by this callback, and should be left as NULL. * * @return @@ -243,20 +265,20 @@ static void __guac_socket_broadcast_lock_handler(guac_socket* socket) { /* Acquire exclusive access to socket */ pthread_mutex_lock(&(data->socket_lock)); - /* Lock sockets of all users */ - guac_client_foreach_user(data->client, __lock_callback, NULL); + /* Lock sockets of the users */ + data->broadcast_handler(data->client, __lock_callback, NULL); } /** - * Callback which is invoked by guac_client_foreach_user() to unlock the given + * Callback which is invoked by the broadcast handler to unlock the given * user's socket at the end of a Guacamole protocol instruction. * * @param user * The user whose socket should be unlocked. * * @param data - * Arbitrary data passed to guac_client_foreach_user(). This is not needed + * Arbitrary data passed to the broadcast handler. This is not needed * by this callback, and should be left as NULL. * * @return @@ -285,7 +307,7 @@ static void __guac_socket_broadcast_unlock_handler(guac_socket* socket) { (guac_socket_broadcast_data*) socket->data; /* Unlock sockets of all users */ - guac_client_foreach_user(data->client, __unlock_callback, NULL); + data->broadcast_handler(data->client, __unlock_callback, NULL); /* Relinquish exclusive access to socket */ pthread_mutex_unlock(&(data->socket_lock)); @@ -343,7 +365,22 @@ static int __guac_socket_broadcast_free_handler(guac_socket* socket) { } -guac_socket* guac_socket_broadcast(guac_client* client) { +/** + * Construct and return a socket that will broadcast to the users given by + * by the provided broadcast handler. + * + * @param client + * The client who's users are being broadcast to. + * + * @param broadcast_handler + * The handler that will peform the broadcast against a subset of users + * of the provided client. + * + * @return + * The newly constructed broadcast socket + */ +static guac_socket* __guac_socket_init( + guac_client* client, guac_socket_broadcast_handler* broadcast_handler) { pthread_mutexattr_t lock_attributes; @@ -352,6 +389,9 @@ guac_socket* guac_socket_broadcast(guac_client* client) { guac_socket_broadcast_data* data = malloc(sizeof(guac_socket_broadcast_data)); + /* Set the provided broadcast handler */ + data->broadcast_handler = broadcast_handler; + /* Store client as socket data */ data->client = client; socket->data = data; @@ -361,7 +401,7 @@ guac_socket* guac_socket_broadcast(guac_client* client) { /* Init lock */ pthread_mutex_init(&(data->socket_lock), &lock_attributes); - + /* Set read/write handlers */ socket->read_handler = __guac_socket_broadcast_read_handler; socket->write_handler = __guac_socket_broadcast_write_handler; @@ -375,3 +415,17 @@ guac_socket* guac_socket_broadcast(guac_client* client) { } +guac_socket* guac_socket_broadcast(guac_client* client) { + + /* Broadcast to all connected non-pending users*/ + return __guac_socket_init(client, guac_client_foreach_user); + +} + +guac_socket* guac_socket_broadcast_pending(guac_client* client) { + + /* Broadcast to all connected pending users*/ + return __guac_socket_init(client, guac_client_foreach_pending_user); + +} + diff --git a/src/protocols/kubernetes/argv.c b/src/protocols/kubernetes/argv.c index 0cbc71601..9b2bfcfcd 100644 --- a/src/protocols/kubernetes/argv.c +++ b/src/protocols/kubernetes/argv.c @@ -63,23 +63,31 @@ int guac_kubernetes_argv_callback(guac_user* user, const char* mimetype, void* guac_kubernetes_send_current_argv(guac_user* user, void* data) { - guac_kubernetes_client* kubernetes_client = (guac_kubernetes_client*) data; + /* Defer to the batch handler, using the user socket */ + return guac_kubernetes_send_current_argv_batch(user->client, user->socket); + +} + +void* guac_kubernetes_send_current_argv_batch( + guac_client* client, guac_socket* socket) { + + guac_kubernetes_client* kubernetes_client = (guac_kubernetes_client*) client->data; guac_terminal* terminal = kubernetes_client->term; /* Send current color scheme */ - guac_user_stream_argv(user, user->socket, "text/plain", + guac_client_stream_argv(client, socket, "text/plain", GUAC_KUBERNETES_ARGV_COLOR_SCHEME, guac_terminal_get_color_scheme(terminal)); /* Send current font name */ - guac_user_stream_argv(user, user->socket, "text/plain", + guac_client_stream_argv(client, socket, "text/plain", GUAC_KUBERNETES_ARGV_FONT_NAME, guac_terminal_get_font_name(terminal)); /* Send current font size */ char font_size[64]; sprintf(font_size, "%i", guac_terminal_get_font_size(terminal)); - guac_user_stream_argv(user, user->socket, "text/plain", + guac_client_stream_argv(client, socket, "text/plain", GUAC_KUBERNETES_ARGV_FONT_SIZE, font_size); return NULL; diff --git a/src/protocols/kubernetes/argv.h b/src/protocols/kubernetes/argv.h index 307ebc71b..dd73d4239 100644 --- a/src/protocols/kubernetes/argv.h +++ b/src/protocols/kubernetes/argv.h @@ -22,6 +22,7 @@ #define GUAC_KUBERNETES_ARGV_H #include "config.h" +#include "kubernetes.h" #include #include @@ -55,7 +56,7 @@ guac_argv_callback guac_kubernetes_argv_callback; * while the connection is running to the given user. Note that the user * receiving these values will not necessarily be able to set new values * themselves if their connection is read-only. This function can be used as - * the callback for guac_client_foreach_user() and guac_client_for_owner() + * the callback for guac_client_foreach_user() and guac_client_for_owner(). * * @param user * The user that should receive the values of all non-sensitive parameters @@ -70,5 +71,23 @@ guac_argv_callback guac_kubernetes_argv_callback; */ void* guac_kubernetes_send_current_argv(guac_user* user, void* data); +/** + * Sends the current values of all non-sensitive parameters which may be set + * while the connection is running to the all users associated with the + * provided socket. Note that the users receiving these values will not + * necessarily be able to set new values themselves if their connection is + * read-only. + * + * @param client + * The client associated with the users who should receive the values of + * all non-sensitive parameters which may be set while the connection is + * running. + * + * @return + * Always NULL. + */ +void* guac_kubernetes_send_current_argv_batch( + guac_client* client, guac_socket* socket); + #endif diff --git a/src/protocols/kubernetes/client.c b/src/protocols/kubernetes/client.c index ef0a502e5..b3a44e110 100644 --- a/src/protocols/kubernetes/client.c +++ b/src/protocols/kubernetes/client.c @@ -78,44 +78,24 @@ static void guac_kubernetes_log(int level, const char* line) { } -/** - * Synchronize the connection state for the given pending user. - * - * @param user - * The pending user whose connection state should be synced. - * - * @param data - * Unused. - * - * @return - * Always NULL. - */ -static void* guac_kubernetes_sync_pending_user(guac_user* user, void* data) { - - guac_client* client = user->client; - guac_kubernetes_client* kubernetes_client = - (guac_kubernetes_client*) client->data; - - guac_terminal_dup(kubernetes_client->term, user, user->socket); - guac_kubernetes_send_current_argv(user, kubernetes_client); - guac_socket_flush(user->socket); - - return NULL; - -} - /** * A pending join handler implementation that will synchronize the connection * state for all pending users prior to them being promoted to full user. * * @param client - * The client whose pending users are about to be promoted. + * The client whose pending users are about to be promoted to full users, + * and therefore need their connection state synchronized. */ static void guac_kubernetes_join_pending_handler(guac_client* client) { - /* Synchronize each user one at a time */ - guac_client_foreach_pending_user( - client, guac_kubernetes_sync_pending_user, NULL); + guac_kubernetes_client* kubernetes_client = + (guac_kubernetes_client*) client->data; + + /* Synchronize the terminal state to all pending users */ + guac_socket* broadcast_socket = client->pending_socket; + guac_terminal_sync_users(kubernetes_client->term, client, broadcast_socket); + guac_kubernetes_send_current_argv_batch(client, broadcast_socket); + guac_socket_flush(broadcast_socket); } diff --git a/src/protocols/rdp/channels/pipe-svc.c b/src/protocols/rdp/channels/pipe-svc.c index 2db42d688..261155089 100644 --- a/src/protocols/rdp/channels/pipe-svc.c +++ b/src/protocols/rdp/channels/pipe-svc.c @@ -41,9 +41,10 @@ void guac_rdp_pipe_svc_send_pipe(guac_socket* socket, guac_rdp_pipe_svc* pipe_sv } -void guac_rdp_pipe_svc_send_pipes(guac_user* user) { - guac_client* client = user->client; +void guac_rdp_pipe_svc_send_pipes( + guac_client* client, guac_socket* socket) { + guac_rdp_client* rdp_client = (guac_rdp_client*) client->data; guac_common_list_lock(rdp_client->available_svc); @@ -51,12 +52,11 @@ void guac_rdp_pipe_svc_send_pipes(guac_user* user) { /* Send pipe for each allocated SVC's output stream */ guac_common_list_element* current = rdp_client->available_svc->head; while (current != NULL) { - guac_rdp_pipe_svc_send_pipe(user->socket, (guac_rdp_pipe_svc*) current->data); + guac_rdp_pipe_svc_send_pipe(socket, (guac_rdp_pipe_svc*) current->data); current = current->next; } guac_common_list_unlock(rdp_client->available_svc); - } void guac_rdp_pipe_svc_add(guac_client* client, guac_rdp_pipe_svc* pipe_svc) { diff --git a/src/protocols/rdp/channels/pipe-svc.h b/src/protocols/rdp/channels/pipe-svc.h index 242d4e50a..f4575d4ec 100644 --- a/src/protocols/rdp/channels/pipe-svc.h +++ b/src/protocols/rdp/channels/pipe-svc.h @@ -95,14 +95,18 @@ void guac_rdp_pipe_svc_send_pipe(guac_socket* socket, guac_rdp_pipe_svc* svc); /** * Sends the "pipe" instructions describing all static virtual channels - * available to the given user along that user's socket. Each pipe instruction - * will relate the associated SVC's underlying output stream with the SVC's - * name and the mimetype "application/octet-stream". + * available to the all users associated with the provided socket. Each pipe + * instruction will relate the associated SVC's underlying output stream with + * the SVC's name and the mimetype "application/octet-stream". * - * @param user - * The user to send the "pipe" instructions to. + * @param client + * The client associated with the users being sent the pipe instruction. + * + * @param socket + * The socket to send the pipe instruction accross. */ -void guac_rdp_pipe_svc_send_pipes(guac_user* user); +void guac_rdp_pipe_svc_send_pipes( + guac_client* client, guac_socket* socket); /** * Add the given SVC to the list of all available SVCs. This function must be diff --git a/src/protocols/rdp/client.c b/src/protocols/rdp/client.c index 5071a3a97..0228b7e39 100644 --- a/src/protocols/rdp/client.c +++ b/src/protocols/rdp/client.c @@ -80,32 +80,22 @@ static int is_writable_directory(const char* path) { } /** - * Synchronize the connection state for the given pending user. + * Add the provided user to the provided audio stream. * * @param user - * The pending user whose connection state should be synced. + * The pending user who should be added to the audio stream. * * @param data - * Unused. + * The audio stream that the user should be added to. * * @return * Always NULL. */ -static void* guac_rdp_sync_pending_user(guac_user* user, void* data) { +static void* guac_rdp_sync_pending_user_audio(guac_user* user, void* data) { - guac_rdp_client* rdp_client = (guac_rdp_client*) user->client->data; - - /* Synchronize any audio stream */ - if (rdp_client->audio) - guac_audio_stream_add_user(rdp_client->audio, user); - - /* Bring user up to date with any registered static channels */ - guac_rdp_pipe_svc_send_pipes(user); - - /* Synchronize with current display */ - guac_common_display_dup(rdp_client->display, user, user->socket); - - guac_socket_flush(user->socket); + /* Add the user to the stream */ + guac_audio_stream* audio = (guac_audio_stream*) data; + guac_audio_stream_add_user(audio, user); return NULL; @@ -120,9 +110,21 @@ static void* guac_rdp_sync_pending_user(guac_user* user, void* data) { */ static void guac_rdp_join_pending_handler(guac_client* client) { - /* Synchronize each user one at a time */ - guac_client_foreach_pending_user( - client, guac_rdp_sync_pending_user, NULL); + guac_rdp_client* rdp_client = (guac_rdp_client*) client->data; + guac_socket* broadcast_socket = client->pending_socket; + + /* Synchronize any audio stream for each pending user */ + if (rdp_client->audio) + guac_client_foreach_pending_user( + client, guac_rdp_sync_pending_user_audio, rdp_client->audio); + + /* Bring user up to date with any registered static channels */ + guac_rdp_pipe_svc_send_pipes(client, broadcast_socket); + + /* Synchronize with current display */ + guac_common_display_dup(rdp_client->display, client, broadcast_socket); + + guac_socket_flush(broadcast_socket); } diff --git a/src/protocols/ssh/argv.c b/src/protocols/ssh/argv.c index a8f31f2ec..6082b02ff 100644 --- a/src/protocols/ssh/argv.c +++ b/src/protocols/ssh/argv.c @@ -70,26 +70,33 @@ int guac_ssh_argv_callback(guac_user* user, const char* mimetype, void* guac_ssh_send_current_argv(guac_user* user, void* data) { - guac_ssh_client* ssh_client = (guac_ssh_client*) data; + /* Defer to the batch handler, using the user's socket to send the data */ + guac_ssh_send_current_argv_batch(user->client, user->socket); + + return NULL; + +} + +void guac_ssh_send_current_argv_batch(guac_client* client, guac_socket* socket) { + + guac_ssh_client* ssh_client = (guac_ssh_client*) client->data; guac_terminal* terminal = ssh_client->term; /* Send current color scheme */ - guac_user_stream_argv(user, user->socket, "text/plain", + guac_client_stream_argv(client, socket, "text/plain", GUAC_SSH_ARGV_COLOR_SCHEME, guac_terminal_get_color_scheme(terminal)); /* Send current font name */ - guac_user_stream_argv(user, user->socket, "text/plain", + guac_client_stream_argv(client, socket, "text/plain", GUAC_SSH_ARGV_FONT_NAME, guac_terminal_get_font_name(terminal)); /* Send current font size */ char font_size[64]; sprintf(font_size, "%i", guac_terminal_get_font_size(terminal)); - guac_user_stream_argv(user, user->socket, "text/plain", + guac_client_stream_argv(client, socket, "text/plain", GUAC_SSH_ARGV_FONT_SIZE, font_size); - return NULL; - } diff --git a/src/protocols/ssh/argv.h b/src/protocols/ssh/argv.h index 4cbdb4c84..0b32d0908 100644 --- a/src/protocols/ssh/argv.h +++ b/src/protocols/ssh/argv.h @@ -69,5 +69,23 @@ guac_argv_callback guac_ssh_argv_callback; */ void* guac_ssh_send_current_argv(guac_user* user, void* data); +/** + * Sends the current values of all non-sensitive parameters which may be set + * while the connection is running to the users associated with the provided + * socket. Note that the users receiving these values will not necessarily be + * able to set new values themselves if their connection is read-only. + * + * @param client + * The client associated with the users that should receive the values of + * all non-sensitive parameters which may be set while the connection is running. + * + * @param socket + * The socket to the arguments to the batch of users along. + * + * @return + * Always NULL. + */ +void guac_ssh_send_current_argv_batch(guac_client* client, guac_socket* socket); + #endif diff --git a/src/protocols/ssh/client.c b/src/protocols/ssh/client.c index 4b1c5393d..c813808b8 100644 --- a/src/protocols/ssh/client.c +++ b/src/protocols/ssh/client.c @@ -36,30 +36,6 @@ #include #include -/** - * Synchronize the connection state for the given pending user. - * - * @param user - * The pending user whose connection state should be synced. - * - * @param data - * Unused. - * - * @return - * Always NULL. - */ -static void* guac_ssh_sync_pending_user(guac_user* user, void* data) { - - guac_client* client = user->client; - guac_ssh_client* ssh_client = (guac_ssh_client*) client->data; - - guac_terminal_dup(ssh_client->term, user, user->socket); - guac_ssh_send_current_argv(user, ssh_client); - guac_socket_flush(user->socket); - - return NULL; - -} /** * A pending join handler implementation that will synchronize the connection @@ -70,9 +46,13 @@ static void* guac_ssh_sync_pending_user(guac_user* user, void* data) { */ static void guac_ssh_join_pending_handler(guac_client* client) { - /* Synchronize each user one at a time */ - guac_client_foreach_pending_user( - client, guac_ssh_sync_pending_user, NULL); + guac_ssh_client* ssh_client = (guac_ssh_client*) client->data; + + /* Synchronize the terminal state to all pending users */ + guac_socket* broadcast_socket = client->pending_socket; + guac_terminal_sync_users(ssh_client->term, client, broadcast_socket); + guac_ssh_send_current_argv_batch(client, broadcast_socket); + guac_socket_flush(broadcast_socket); } diff --git a/src/protocols/telnet/argv.c b/src/protocols/telnet/argv.c index 3ea3095ef..0424e323d 100644 --- a/src/protocols/telnet/argv.c +++ b/src/protocols/telnet/argv.c @@ -65,26 +65,34 @@ int guac_telnet_argv_callback(guac_user* user, const char* mimetype, void* guac_telnet_send_current_argv(guac_user* user, void* data) { - guac_telnet_client* telnet_client = (guac_telnet_client*) data; + /* Defer to the batch handler, using the user's socket to send the data */ + guac_telnet_send_current_argv_batch(user->client, user->socket); + + return NULL; + +} + +void guac_telnet_send_current_argv_batch( + guac_client* client, guac_socket* socket) { + + guac_telnet_client* telnet_client = (guac_telnet_client*) client->data; guac_terminal* terminal = telnet_client->term; /* Send current color scheme */ - guac_user_stream_argv(user, user->socket, "text/plain", + guac_client_stream_argv(client, socket, "text/plain", GUAC_TELNET_ARGV_COLOR_SCHEME, guac_terminal_get_color_scheme(terminal)); /* Send current font name */ - guac_user_stream_argv(user, user->socket, "text/plain", + guac_client_stream_argv(client, socket, "text/plain", GUAC_TELNET_ARGV_FONT_NAME, guac_terminal_get_font_name(terminal)); /* Send current font size */ char font_size[64]; sprintf(font_size, "%i", guac_terminal_get_font_size(terminal)); - guac_user_stream_argv(user, user->socket, "text/plain", + guac_client_stream_argv(client, socket, "text/plain", GUAC_TELNET_ARGV_FONT_SIZE, font_size); - return NULL; - } diff --git a/src/protocols/telnet/argv.h b/src/protocols/telnet/argv.h index 60c1534f7..2c48c141b 100644 --- a/src/protocols/telnet/argv.h +++ b/src/protocols/telnet/argv.h @@ -69,5 +69,24 @@ guac_argv_callback guac_telnet_argv_callback; */ void* guac_telnet_send_current_argv(guac_user* user, void* data); +/** + * Sends the current values of all non-sensitive parameters which may be set + * while the connection is running to the users associated with the provided + * socket. Note that the users receiving these values will not necessarily be + * able to set new values themselves if their connection is read-only. + * + * @param client + * The client associated with the users that should receive the values of + * all non-sensitive parameters which may be set while the connection is running. + * + * @param socket + * The socket to the arguments to the batch of users along. + * + * @return + * Always NULL. + */ +void guac_telnet_send_current_argv_batch( + guac_client* client, guac_socket* socket); + #endif diff --git a/src/protocols/telnet/client.c b/src/protocols/telnet/client.c index 71bf1b0a5..6f2546324 100644 --- a/src/protocols/telnet/client.c +++ b/src/protocols/telnet/client.c @@ -36,31 +36,6 @@ #include #include -/** - * Synchronize the connection state for the given pending user. - * - * @param user - * The pending user whose connection state should be synced. - * - * @param data - * Unused. - * - * @return - * Always NULL. - */ -static void* guac_telnet_sync_pending_user(guac_user* user, void* data) { - - guac_client* client = user->client; - guac_telnet_client* telnet_client = (guac_telnet_client*) client->data; - - guac_terminal_dup(telnet_client->term, user, user->socket); - guac_telnet_send_current_argv(user, telnet_client); - guac_socket_flush(user->socket); - - return NULL; - -} - /** * A pending join handler implementation that will synchronize the connection * state for all pending users prior to them being promoted to full user. @@ -70,9 +45,13 @@ static void* guac_telnet_sync_pending_user(guac_user* user, void* data) { */ static void guac_telnet_join_pending_handler(guac_client* client) { - /* Synchronize each user one at a time */ - guac_client_foreach_pending_user( - client, guac_telnet_sync_pending_user, NULL); + guac_telnet_client* telnet_client = (guac_telnet_client*) client->data; + + /* Synchronize the terminal state to all pending users */ + guac_socket* broadcast_socket = client->pending_socket; + guac_terminal_sync_users(telnet_client->term, client, broadcast_socket); + guac_telnet_send_current_argv_batch(client, broadcast_socket); + guac_socket_flush(broadcast_socket); } diff --git a/src/protocols/telnet/user.c b/src/protocols/telnet/user.c index 2c0e1c613..e9d9df463 100644 --- a/src/protocols/telnet/user.c +++ b/src/protocols/telnet/user.c @@ -70,13 +70,6 @@ int guac_telnet_user_join_handler(guac_user* user, int argc, char** argv) { } - /* If not owner, synchronize with current display */ - else { - guac_terminal_dup(telnet_client->term, user, user->socket); - guac_telnet_send_current_argv(user, telnet_client); - guac_socket_flush(user->socket); - } - /* Only handle events if not read-only */ if (!settings->read_only) { diff --git a/src/protocols/vnc/client.c b/src/protocols/vnc/client.c index 14dbe0803..9d7aa099d 100644 --- a/src/protocols/vnc/client.c +++ b/src/protocols/vnc/client.c @@ -40,35 +40,29 @@ #include #include +#ifdef ENABLE_PULSE /** - * Synchronize the connection state for the given pending user. + * Add the provided user to the provided audio stream. * * @param user - * The pending user whose connection state should be synced. + * The pending user who should be added to the audio stream. * * @param data - * Unused. + * The audio stream that the user should be added to. * * @return * Always NULL. */ -static void* guac_vnc_sync_pending_user(guac_user* user, void* data) { - - guac_vnc_client* vnc_client = (guac_vnc_client*) user->client->data; - -#ifdef ENABLE_PULSE - /* Synchronize an audio stream */ - if (vnc_client->audio) - guac_pa_stream_add_user(vnc_client->audio, user); -#endif +static void* guac_vnc_sync_pending_user_audio(guac_user* user, void* data) { - /* Synchronize with current display */ - guac_common_display_dup(vnc_client->display, user, user->socket); - guac_socket_flush(user->socket); + /* Add the user to the stream */ + guac_pa_stream* audio = (guac_pa_stream*) data; + guac_pa_stream_add_user(audio, user); return NULL; } +#endif /** * A pending join handler implementation that will synchronize the connection @@ -79,9 +73,19 @@ static void* guac_vnc_sync_pending_user(guac_user* user, void* data) { */ static void guac_vnc_join_pending_handler(guac_client* client) { - /* Synchronize each user one at a time */ - guac_client_foreach_pending_user( - client, guac_vnc_sync_pending_user, NULL); + guac_vnc_client* vnc_client = (guac_vnc_client*) client->data; + guac_socket* broadcast_socket = client->pending_socket; + +#ifdef ENABLE_PULSE + /* Synchronize any audio stream for each pending user */ + if (vnc_client->audio) + guac_client_foreach_pending_user( + client, guac_vnc_sync_pending_user_audio, vnc_client->audio); +#endif + + /* Synchronize with current display */ + guac_common_display_dup(vnc_client->display, client, broadcast_socket); + guac_socket_flush(broadcast_socket); } diff --git a/src/protocols/vnc/user.c b/src/protocols/vnc/user.c index 4e6f473c8..74204f624 100644 --- a/src/protocols/vnc/user.c +++ b/src/protocols/vnc/user.c @@ -74,21 +74,6 @@ int guac_vnc_user_join_handler(guac_user* user, int argc, char** argv) { } - /* If not owner, synchronize with current state */ - else { - -#ifdef ENABLE_PULSE - /* Synchronize an audio stream */ - if (vnc_client->audio) - guac_pa_stream_add_user(vnc_client->audio, user); -#endif - - /* Synchronize with current display */ - guac_common_display_dup(vnc_client->display, user, user->socket); - guac_socket_flush(user->socket); - - } - /* Only handle events if not read-only */ if (!settings->read_only) { diff --git a/src/terminal/display.c b/src/terminal/display.c index a9d671f67..92d866771 100644 --- a/src/terminal/display.c +++ b/src/terminal/display.c @@ -818,11 +818,11 @@ void guac_terminal_display_flush(guac_terminal_display* display) { } -void guac_terminal_display_dup(guac_terminal_display* display, guac_user* user, - guac_socket* socket) { +void guac_terminal_display_dup( + guac_terminal_display* display, guac_client* client, guac_socket* socket) { /* Create default surface */ - guac_common_surface_dup(display->display_surface, user, socket); + guac_common_surface_dup(display->display_surface, client, socket); /* Select layer is a child of the display layer */ guac_protocol_send_move(socket, display->select_layer, @@ -1023,4 +1023,3 @@ int guac_terminal_display_set_font(guac_terminal_display* display, return 0; } - diff --git a/src/terminal/scrollbar.c b/src/terminal/scrollbar.c index a0eec8ec0..6b5ac7ab3 100644 --- a/src/terminal/scrollbar.c +++ b/src/terminal/scrollbar.c @@ -23,6 +23,7 @@ #include #include #include +#include #include @@ -332,7 +333,7 @@ static void calculate_state(guac_terminal_scrollbar* scrollbar, } void guac_terminal_scrollbar_dup(guac_terminal_scrollbar* scrollbar, - guac_user* user, guac_socket* socket) { + guac_client* client, guac_socket* socket) { /* Get old state */ guac_terminal_scrollbar_render_state* state = &scrollbar->render_state; diff --git a/src/terminal/terminal.c b/src/terminal/terminal.c index 6e6600640..ad539eafb 100644 --- a/src/terminal/terminal.c +++ b/src/terminal/terminal.c @@ -1976,18 +1976,47 @@ int guac_terminal_create_typescript(guac_terminal* term, const char* path, } -void guac_terminal_dup(guac_terminal* term, guac_user* user, - guac_socket* socket) { +/** + * Synchronize the state of the provided terminal to a subset of users of + * the provided guac_client using the provided socket. + * + * @param client + * The client whose users should be synchronized. + * + * @param term + * The terminal state that should be synchronized to the users. + * + * @param socket + * The socket that should be used to communicate with the users. + */ +static void __guac_terminal_sync_socket( + guac_client* client, guac_terminal* term, guac_socket* socket) { /* Synchronize display state with new user */ guac_terminal_repaint_default_layer(term, socket); - guac_terminal_display_dup(term->display, user, socket); + guac_terminal_display_dup(term->display, client, socket); /* Synchronize mouse cursor */ - guac_common_cursor_dup(term->cursor, user, socket); + guac_common_cursor_dup(term->cursor, client, socket); + + /* Paint scrollbar for joining users */ + guac_terminal_scrollbar_dup(term->scrollbar, client, socket); + +} + +void guac_terminal_dup(guac_terminal* term, guac_user* user, + guac_socket* socket) { + + /* Ignore the user and just use the provided socket directly */ + __guac_terminal_sync_socket(user->client, term, socket); + +} + +void guac_terminal_sync_users( + guac_terminal* term, guac_client* client, guac_socket* socket) { - /* Paint scrollbar for joining user */ - guac_terminal_scrollbar_dup(term->scrollbar, user, socket); + /* Use the provided socket to synchronize state to the users */ + __guac_terminal_sync_socket(client, term, socket); } diff --git a/src/terminal/terminal/display.h b/src/terminal/terminal/display.h index db85b2ac9..4c84dc56f 100644 --- a/src/terminal/terminal/display.h +++ b/src/terminal/terminal/display.h @@ -308,21 +308,23 @@ void guac_terminal_display_resize(guac_terminal_display* display, int width, int void guac_terminal_display_flush(guac_terminal_display* display); /** - * Initializes and syncs the current terminal display state for the given user - * that has just joined the connection, sending the necessary instructions to - * completely recreate and redraw the terminal rendering over the given socket. + * Initializes and syncs the current terminal display state for all joining + * users associated with the provided socket, sending the necessary instructions + * to completely recreate and redraw the terminal rendering over the given + * socket. * * @param display - * The terminal display to sync to the given user. + * The terminal display to sync to the users associated with the provided + * socket. * - * @param user - * The user that has just joined the connection. + * @param client + * The client whose users are joining. * * @param socket * The socket over which any necessary instructions should be sent. */ -void guac_terminal_display_dup(guac_terminal_display* display, guac_user* user, - guac_socket* socket); +void guac_terminal_display_dup( + guac_terminal_display* display, guac_client* client, guac_socket* socket); /** * Draws the text selection rectangle from the given coordinates to the given end coordinates. diff --git a/src/terminal/terminal/scrollbar.h b/src/terminal/terminal/scrollbar.h index c30833779..b33f57d43 100644 --- a/src/terminal/terminal/scrollbar.h +++ b/src/terminal/terminal/scrollbar.h @@ -255,22 +255,22 @@ void guac_terminal_scrollbar_free(guac_terminal_scrollbar* scrollbar); void guac_terminal_scrollbar_flush(guac_terminal_scrollbar* scrollbar); /** - * Forces a complete redraw / resync of scrollbar state for the given user that - * has just joined the connection, sending the necessary instructions to + * Forces a complete redraw / resync of scrollbar state for all joinging users + * associated with the provided socket, sending the necessary instructions to * completely recreate and redraw the scrollbar rendering over the given * socket. * * @param scrollbar - * The scrollbar to sync to the given user. + * The scrollbar to sync to the given users. * - * @param user - * The user that has just joined the connection. + * @param client + * The client associated with the joining users. * * @param socket * The socket over which any necessary instructions should be sent. */ void guac_terminal_scrollbar_dup(guac_terminal_scrollbar* scrollbar, - guac_user* user, guac_socket* socket); + guac_client* client, guac_socket* socket); /** * Sets the minimum and maximum allowed scroll values of the given scrollbar diff --git a/src/terminal/terminal/terminal.h b/src/terminal/terminal/terminal.h index 082a3d23b..221937f80 100644 --- a/src/terminal/terminal/terminal.h +++ b/src/terminal/terminal/terminal.h @@ -619,6 +619,9 @@ int guac_terminal_sendf(guac_terminal* term, const char* format, ...); * connection. All instructions necessary to replicate state are sent over the * given socket. * + * @deprecated The guac_terminal_sync_users method should be used when + * duplicating display state to a set of users. + * * @param term * The terminal emulator associated with the connection being joined. * @@ -632,6 +635,24 @@ int guac_terminal_sendf(guac_terminal* term, const char* format, ...); void guac_terminal_dup(guac_terminal* term, guac_user* user, guac_socket* socket); +/** + * Replicates the current display state to one or more users that are joining + * the connection. All instructions necessary to replicate state are sent over + * the given socket. The set of users receiving these instructions is + * determined solely by the socket chosen. + * + * @param term + * The terminal whose state should be synchronized to the users. + * + * @param client + * The client associated with the users to be synchronized. + * + * @param socket + * The socket to which the terminal state will be broadcast. + */ +void guac_terminal_sync_users( + guac_terminal* term, guac_client* client, guac_socket* socket); + /** * Resize the client display and terminal to the given pixel dimensions. * From 8824f2c7d783a9474c439fbeeb188cbac108248d Mon Sep 17 00:00:00 2001 From: James Muehlner Date: Wed, 23 Aug 2023 23:54:01 +0000 Subject: [PATCH 4/6] GUACAMOLE-1846: Migrate away from unsupported atomic state for pending user promotion. --- src/libguac/client.c | 72 +++++++++++++++++++++++++++------- src/libguac/guacamole/client.h | 14 ++----- 2 files changed, 61 insertions(+), 25 deletions(-) diff --git a/src/libguac/client.c b/src/libguac/client.c index 995fc5623..21906077e 100644 --- a/src/libguac/client.c +++ b/src/libguac/client.c @@ -42,7 +42,6 @@ #include #include #include -#include #include #include #include @@ -53,6 +52,24 @@ */ #define GUAC_CLIENT_PENDING_USERS_REFRESH_INTERVAL 250000000 +/** + * A value that indicates that the pending users timer has yet to be + * initialized and started. + */ +#define GUAC_CLIENT_PENDING_TIMER_UNREGISTERED 0 + +/** + * A value that indicates that the pending users timer has been initialized + * and started, but that the timer handler is not currently running. + */ +#define GUAC_CLIENT_PENDING_TIMER_REGISTERED 1 + +/** + * A value that indicates that the pending users timer has been initialized + * and started, and that the timer handler is currently running. + */ +#define GUAC_CLIENT_PENDING_TIMER_TRIGGERED 2 + /** * Empty NULL-terminated array of argument names. */ @@ -149,8 +166,20 @@ static void guac_client_promote_pending_users(union sigval data) { guac_client* client = (guac_client*) data.sival_ptr; - /* Do not start if the previous promotion event is still running */ - if (atomic_flag_test_and_set(&(client->__pending_timer_event_active))) + pthread_mutex_lock(&(client->__pending_users_timer_mutex)); + + /* Check if the previous instance of this handler is still running */ + int already_running = ( + client->__pending_users_timer_state + == GUAC_CLIENT_PENDING_TIMER_TRIGGERED); + + /* Mark the handler as running if it isn't already */ + client->__pending_users_timer_state = GUAC_CLIENT_PENDING_TIMER_TRIGGERED; + + pthread_mutex_unlock(&(client->__pending_users_timer_mutex)); + + /* Do not start the handler if the previous instance is still running */ + if (already_running) return; /* Acquire the lock for reading and modifying the list of pending users */ @@ -197,8 +226,10 @@ static void guac_client_promote_pending_users(union sigval data) { * to ensure that all users are always on exactly one of these lists) */ guac_release_lock(&(client->__pending_users_lock)); - /* Mark the timer event as complete so the next instance can run */ - atomic_flag_clear(&(client->__pending_timer_event_active)); + /* Mark the handler as complete so the next instance can run */ + pthread_mutex_lock(&(client->__pending_users_timer_mutex)); + client->__pending_users_timer_state = GUAC_CLIENT_PENDING_TIMER_REGISTERED; + pthread_mutex_unlock(&(client->__pending_users_timer_mutex)); } @@ -250,16 +281,16 @@ guac_client* guac_client_alloc() { pthread_key_create(&(client->__users_lock.key), (void *) 0); pthread_key_create(&(client->__pending_users_lock.key), (void *) 0); - /* Ensure the timer is constructed only once */ + /* The timer will be lazily created in the child process */ + client->__pending_users_timer_state = GUAC_CLIENT_PENDING_TIMER_UNREGISTERED; + + /* Set up the pending user promotion mutex */ pthread_mutex_init(&(client->__pending_users_timer_mutex), NULL); /* Set up broadcast sockets */ client->socket = guac_socket_broadcast(client); client->pending_socket = guac_socket_broadcast_pending(client); - /* Set the timer event thread as initially inactive, since it hasn't run */ - atomic_flag_clear(&(client->__pending_timer_event_active)); - return client; } @@ -301,12 +332,20 @@ void guac_client_free(guac_client* client) { guac_client_log(client, GUAC_LOG_ERROR, "Unable to close plugin: %s", dlerror()); } - /* Destroy the pending users timer */ - pthread_mutex_destroy(&(client->__pending_users_timer_mutex)); - if (client->__pending_users_timer_running != 0) + /* Find out if the pending user promotion timer was ever started */ + pthread_mutex_lock(&(client->__pending_users_timer_mutex)); + int was_started = ( + client->__pending_users_timer_state + != GUAC_CLIENT_PENDING_TIMER_UNREGISTERED); + pthread_mutex_unlock(&(client->__pending_users_timer_mutex)); + + /* If the timer was registered, stop it before destroying the lock */ + if (was_started) timer_delete(client->__pending_users_timer); - /* Destroy the reenrant read-write locks */ + pthread_mutex_destroy(&(client->__pending_users_timer_mutex)); + + /* Destroy the reentrant read-write locks */ guac_destroy_reentrant_rwlock(&(client->__users_lock)); guac_destroy_reentrant_rwlock(&(client->__pending_users_lock)); @@ -423,7 +462,8 @@ static int guac_client_start_pending_users_timer(guac_client* client) { pthread_mutex_lock(&(client->__pending_users_timer_mutex)); /* Return success if the timer is already created and running */ - if (client->__pending_users_timer_running != 0) { + if (client->__pending_users_timer_state + != GUAC_CLIENT_PENDING_TIMER_UNREGISTERED) { pthread_mutex_unlock(&(client->__pending_users_timer_mutex)); return 0; } @@ -456,7 +496,9 @@ static int guac_client_start_pending_users_timer(guac_client* client) { return 1; } - client->__pending_users_timer_running = 1; + /* Mark the timer as registered but not yet running */ + client->__pending_users_timer_state = GUAC_CLIENT_PENDING_TIMER_REGISTERED; + pthread_mutex_unlock(&(client->__pending_users_timer_mutex)); return 0; diff --git a/src/libguac/guacamole/client.h b/src/libguac/guacamole/client.h index b7d1b4845..c98c43b09 100644 --- a/src/libguac/guacamole/client.h +++ b/src/libguac/guacamole/client.h @@ -196,22 +196,16 @@ struct guac_client { timer_t __pending_users_timer; /** - * Non-zero if the pending users timer is configured and running, or zero - * otherwise. + * A flag storing the current state of the pending users timer. */ - int __pending_users_timer_running; + int __pending_users_timer_state; /** - * A mutex that must be acquired before modifying the pending users timer. + * A mutex that must be acquired before modifying or checking the value of + * the timer state. */ pthread_mutex_t __pending_users_timer_mutex; - /** - * A flag that indicates whether the pending users timer event thread is - * currently running. - */ - volatile atomic_flag __pending_timer_event_active; - /** * The first user within the list of connected users who have not yet had * their connection states synchronized after joining. From b02abfd9fa253248a46d9c9838b83f8fb77d05d1 Mon Sep 17 00:00:00 2001 From: James Muehlner Date: Thu, 24 Aug 2023 00:23:32 +0000 Subject: [PATCH 5/6] GUACAMOLE-1846: Add error handling support to join pending handler. --- src/libguac/client.c | 21 +++++++++++++++++++-- src/libguac/guacamole/client-fntypes.h | 6 +++++- src/libguac/guacamole/client.h | 2 +- src/protocols/kubernetes/client.c | 7 ++++++- src/protocols/rdp/client.c | 7 ++++++- src/protocols/ssh/client.c | 7 ++++++- src/protocols/telnet/client.c | 7 ++++++- src/protocols/vnc/client.c | 7 ++++++- 8 files changed, 55 insertions(+), 9 deletions(-) diff --git a/src/libguac/client.c b/src/libguac/client.c index 21906077e..43d61106e 100644 --- a/src/libguac/client.c +++ b/src/libguac/client.c @@ -186,8 +186,25 @@ static void guac_client_promote_pending_users(union sigval data) { guac_acquire_write_lock(&(client->__pending_users_lock)); /* Run the pending join handler, if one is defined */ - if (client->join_pending_handler) - client->join_pending_handler(client); + if (client->join_pending_handler) { + + /* If an error occurs in the pending handler */ + if(client->join_pending_handler(client)) { + + guac_release_lock(&(client->__pending_users_lock)); + + /* Mark the handler as not running */ + pthread_mutex_lock(&(client->__pending_users_timer_mutex)); + client->__pending_users_timer_state = GUAC_CLIENT_PENDING_TIMER_REGISTERED; + pthread_mutex_unlock(&(client->__pending_users_timer_mutex)); + + /* Log a warning and abort the promotion of the pending users */ + guac_client_log(client, GUAC_LOG_WARNING, + "join_pending_handler did not successfully complete;" + " any pending users have not been promoted.\n"); + return; + } + } /* The first pending user in the list, if any */ guac_user* first_user = client->__pending_users; diff --git a/src/libguac/guacamole/client-fntypes.h b/src/libguac/guacamole/client-fntypes.h index 26cddb6f8..367a6f1e2 100644 --- a/src/libguac/guacamole/client-fntypes.h +++ b/src/libguac/guacamole/client-fntypes.h @@ -57,8 +57,12 @@ typedef int guac_client_free_handler(guac_client* client); * * @param client * The client whose handler was invoked. + * + * @return + * Zero if the pending handler ran successfuly, or a non-zero value if an + * error occured. */ -typedef void guac_client_join_pending_handler(guac_client* client); +typedef int guac_client_join_pending_handler(guac_client* client); /** * Handler for logging messages related to a given guac_client instance. diff --git a/src/libguac/guacamole/client.h b/src/libguac/guacamole/client.h index c98c43b09..4bfdc0704 100644 --- a/src/libguac/guacamole/client.h +++ b/src/libguac/guacamole/client.h @@ -255,7 +255,7 @@ struct guac_client { * * Example: * @code - * void join_pending_handler(guac_client* client); + * int join_pending_handler(guac_client* client); * * int guac_client_init(guac_client* client) { * client->join_pending_handler = join_pending_handler; diff --git a/src/protocols/kubernetes/client.c b/src/protocols/kubernetes/client.c index b3a44e110..f90358fbc 100644 --- a/src/protocols/kubernetes/client.c +++ b/src/protocols/kubernetes/client.c @@ -85,8 +85,11 @@ static void guac_kubernetes_log(int level, const char* line) { * @param client * The client whose pending users are about to be promoted to full users, * and therefore need their connection state synchronized. + * + * @return + * Always zero. */ -static void guac_kubernetes_join_pending_handler(guac_client* client) { +static int guac_kubernetes_join_pending_handler(guac_client* client) { guac_kubernetes_client* kubernetes_client = (guac_kubernetes_client*) client->data; @@ -97,6 +100,8 @@ static void guac_kubernetes_join_pending_handler(guac_client* client) { guac_kubernetes_send_current_argv_batch(client, broadcast_socket); guac_socket_flush(broadcast_socket); + return 0; + } int guac_client_init(guac_client* client) { diff --git a/src/protocols/rdp/client.c b/src/protocols/rdp/client.c index 0228b7e39..5130f1944 100644 --- a/src/protocols/rdp/client.c +++ b/src/protocols/rdp/client.c @@ -107,8 +107,11 @@ static void* guac_rdp_sync_pending_user_audio(guac_user* user, void* data) { * * @param client * The client whose pending users are about to be promoted. + * + * @return + * Always zero. */ -static void guac_rdp_join_pending_handler(guac_client* client) { +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; @@ -126,6 +129,8 @@ static void guac_rdp_join_pending_handler(guac_client* client) { guac_socket_flush(broadcast_socket); + return 0; + } int guac_client_init(guac_client* client, int argc, char** argv) { diff --git a/src/protocols/ssh/client.c b/src/protocols/ssh/client.c index c813808b8..74a62ba07 100644 --- a/src/protocols/ssh/client.c +++ b/src/protocols/ssh/client.c @@ -43,8 +43,11 @@ * * @param client * The client whose pending users are about to be promoted. + * + * @return + * Always zero. */ -static void guac_ssh_join_pending_handler(guac_client* client) { +static int guac_ssh_join_pending_handler(guac_client* client) { guac_ssh_client* ssh_client = (guac_ssh_client*) client->data; @@ -54,6 +57,8 @@ static void guac_ssh_join_pending_handler(guac_client* client) { guac_ssh_send_current_argv_batch(client, broadcast_socket); guac_socket_flush(broadcast_socket); + return 0; + } int guac_client_init(guac_client* client) { diff --git a/src/protocols/telnet/client.c b/src/protocols/telnet/client.c index 6f2546324..3b4184023 100644 --- a/src/protocols/telnet/client.c +++ b/src/protocols/telnet/client.c @@ -42,8 +42,11 @@ * * @param client * The client whose pending users are about to be promoted. + * + * @return + * Always zero. */ -static void guac_telnet_join_pending_handler(guac_client* client) { +static int guac_telnet_join_pending_handler(guac_client* client) { guac_telnet_client* telnet_client = (guac_telnet_client*) client->data; @@ -53,6 +56,8 @@ static void guac_telnet_join_pending_handler(guac_client* client) { guac_telnet_send_current_argv_batch(client, broadcast_socket); guac_socket_flush(broadcast_socket); + return 0; + } int guac_client_init(guac_client* client) { diff --git a/src/protocols/vnc/client.c b/src/protocols/vnc/client.c index 9d7aa099d..47cafa721 100644 --- a/src/protocols/vnc/client.c +++ b/src/protocols/vnc/client.c @@ -70,8 +70,11 @@ static void* guac_vnc_sync_pending_user_audio(guac_user* user, void* data) { * * @param client * The client whose pending users are about to be promoted. + * + * @return + * Always zero. */ -static void guac_vnc_join_pending_handler(guac_client* client) { +static int guac_vnc_join_pending_handler(guac_client* client) { guac_vnc_client* vnc_client = (guac_vnc_client*) client->data; guac_socket* broadcast_socket = client->pending_socket; @@ -87,6 +90,8 @@ static void guac_vnc_join_pending_handler(guac_client* client) { guac_common_display_dup(vnc_client->display, client, broadcast_socket); guac_socket_flush(broadcast_socket); + return 0; + } int guac_client_init(guac_client* client) { From 826cb784d483e7d17e584658e48fc7931ed1be31 Mon Sep 17 00:00:00 2001 From: James Muehlner Date: Thu, 24 Aug 2023 23:04:40 +0000 Subject: [PATCH 6/6] GUACAMOLE-1846: Ensure that stuck child processes are nonetheless cleaned up. --- src/common/common/list.h | 16 ++++++- src/common/list.c | 20 ++++++++- src/guacd/daemon.c | 77 +++++++++++++++++++++++++++++++++- src/guacd/proc-map.c | 76 +++++++++++++++++++++++++++++++-- src/guacd/proc-map.h | 47 +++++++++++++++++++++ src/guacd/proc.c | 64 +++++++++++++++++++++++++++- src/libguac/client.c | 18 +++++--- src/libguac/guacamole/client.h | 3 +- src/protocols/rdp/rdp.c | 2 +- 9 files changed, 306 insertions(+), 17 deletions(-) diff --git a/src/common/common/list.h b/src/common/common/list.h index 5f6be1b76..f1a16a1b1 100644 --- a/src/common/common/list.h +++ b/src/common/common/list.h @@ -75,12 +75,26 @@ typedef struct guac_common_list { */ guac_common_list* guac_common_list_alloc(); +/** + * A handler that will be invoked with the data pointer of each element of + * the list when guac_common_list_free() is invoked. + * + * @param data + * The arbitrary data pointed to by the list element. + */ +typedef void guac_common_list_element_free_handler(void* data); + /** * Frees the given list. * * @param list The list to free. + * + * @param free_element_handler + * A handler that will be invoked with each arbitrary data pointer in the + * list, if not NULL. */ -void guac_common_list_free(guac_common_list* list); +void guac_common_list_free(guac_common_list* list, + guac_common_list_element_free_handler* free_element_handler); /** * Adds the given data to the list as a new element, returning the created diff --git a/src/common/list.c b/src/common/list.c index 072ef89fa..4138d3cc9 100644 --- a/src/common/list.c +++ b/src/common/list.c @@ -34,8 +34,26 @@ guac_common_list* guac_common_list_alloc() { } -void guac_common_list_free(guac_common_list* list) { +void guac_common_list_free( + guac_common_list* list, + guac_common_list_element_free_handler* free_element_handler) { + + /* Free every element of the list */ + guac_common_list_element* element = list->head; + while(element != NULL) { + + guac_common_list_element* next = element->next; + + if (free_element_handler != NULL) + free_element_handler(element->data); + + free(element); + element = next; + } + + /* Free the list itself */ free(list); + } guac_common_list_element* guac_common_list_add(guac_common_list* list, diff --git a/src/guacd/daemon.c b/src/guacd/daemon.c index 2861cffee..adfaa3c17 100644 --- a/src/guacd/daemon.c +++ b/src/guacd/daemon.c @@ -43,6 +43,7 @@ #include #include #include +#include #include #define GUACD_DEV_NULL "/dev/null" @@ -245,6 +246,49 @@ static void guacd_openssl_free_locks(int count) { #endif #endif +/** + * A flag that, if non-zero, indicates that the daemon should immediately stop + * accepting new connections. + */ +int stop_everything = 0; + +/** + * A signal handler that will set a flag telling the daemon to immediately stop + * accepting new connections. Note that the signal itself will cause any pending + * accept() calls to be interrupted, causing the daemon to unlock and begin + * cleaning up. + * + * @param signal + * The signal that was received. Unused in this function since only + * signals that should result in stopping the daemon should invoke this. + */ +static void signal_stop_handler(int signal) { + + /* Instruct the daemon to stop accepting new connections */ + stop_everything = 1; + +} + +/** + * A callback for guacd_proc_map_foreach which will stop every process in the + * map. + * + * @param proc + * The guacd process to stop. + * + * @param data + * Unused. + */ +static void stop_process_callback(guacd_proc* proc, void* data) { + + guacd_log(GUAC_LOG_DEBUG, + "Killing connection %s (%i)\n", + proc->client->connection_id, (int) proc->pid); + + guacd_proc_stop(proc); + +} + int main(int argc, char* argv[]) { /* Server */ @@ -452,6 +496,12 @@ int main(int argc, char* argv[]) { "Child processes may pile up in the process table."); } + /* Clean up and exit if SIGINT or SIGTERM signals are caught */ + struct sigaction signal_stop_action = { 0 }; + signal_stop_action.sa_handler = signal_stop_handler; + sigaction(SIGINT, &signal_stop_action, NULL); + sigaction(SIGTERM, &signal_stop_action, NULL); + /* Log listening status */ guacd_log(GUAC_LOG_INFO, "Listening on host %s, port %s", bound_address, bound_port); @@ -465,7 +515,7 @@ int main(int argc, char* argv[]) { } /* Daemon loop */ - for (;;) { + while (!stop_everything) { pthread_t child_thread; @@ -475,7 +525,10 @@ int main(int argc, char* argv[]) { (struct sockaddr*) &client_addr, &client_addr_len); if (connected_socket_fd < 0) { - guacd_log(GUAC_LOG_ERROR, "Could not accept client connection: %s", strerror(errno)); + if (errno == EINTR) + guacd_log(GUAC_LOG_DEBUG, "Accepting of further client connection(s) interrupted by signal."); + else + guacd_log(GUAC_LOG_ERROR, "Could not accept client connection: %s", strerror(errno)); continue; } @@ -499,6 +552,26 @@ int main(int argc, char* argv[]) { } + /* Stop all connections */ + if (map != NULL) { + + guacd_proc_map_foreach(map, stop_process_callback, NULL); + + /* + * FIXME: Clean up the proc map. This is not as straightforward as it + * might seem, since the detached connection threads will attempt to + * remove the connection proccesses from the map when they complete, + * which will also happen upon shutdown. So there's a good chance that + * this map cleanup will happen at the same time as the thread cleanup. + * The map _does_ have locking mechanisms in place for ensuring thread + * safety, but cleaning up the map also requires destroying those locks, + * making them unusable for this case. One potential fix could be to + * join every one of the connection threads instead of detaching them, + * but that does complicate the cleanup of thread resources. + */ + + } + /* Close socket */ if (close(socket_fd) < 0) { guacd_log(GUAC_LOG_ERROR, "Could not close socket: %s", strerror(errno)); diff --git a/src/guacd/proc-map.c b/src/guacd/proc-map.c index ac87196ad..73a9daeb9 100644 --- a/src/guacd/proc-map.c +++ b/src/guacd/proc-map.c @@ -27,6 +27,24 @@ #include #include +/** + * A value to be stored in the buckets, containing the guacd proc itself, + * as well as a link to the element in the list of all guacd processes. + */ +typedef struct guacd_proc_map_entry { + + /** + * The guacd process itself. + */ + guacd_proc* proc; + + /** + * A pointer to the corresponding entry in the list of all processes. + */ + guac_common_list_element* element; + +} guacd_proc_map_entry; + /** * Returns a hash code based on the given connection ID. * @@ -98,7 +116,7 @@ static guac_common_list_element* __guacd_proc_find(guac_common_list* bucket, while (current != NULL) { /* Check connection ID */ - guacd_proc* proc = (guacd_proc*) current->data; + guacd_proc* proc = ((guacd_proc_map_entry*) current->data)->proc; if (strcmp(proc->client->connection_id, id) == 0) break; @@ -112,6 +130,7 @@ static guac_common_list_element* __guacd_proc_find(guac_common_list* bucket, guacd_proc_map* guacd_proc_map_alloc() { guacd_proc_map* map = malloc(sizeof(guacd_proc_map)); + map->processes = guac_common_list_alloc(); guac_common_list** current; int i; @@ -140,8 +159,18 @@ int guacd_proc_map_add(guacd_proc_map* map, guacd_proc* proc) { /* If no such element, we can add the new client successfully */ if (found == NULL) { - guac_common_list_add(bucket, proc); + + guacd_proc_map_entry* entry = malloc(sizeof(guacd_proc_map_entry)); + + guac_common_list_lock(map->processes); + entry->element = guac_common_list_add(map->processes, proc); + guac_common_list_unlock(map->processes); + + entry->proc = proc; + + guac_common_list_add(bucket, entry); guac_common_list_unlock(bucket); + return 0; } @@ -168,7 +197,7 @@ guacd_proc* guacd_proc_map_retrieve(guacd_proc_map* map, const char* id) { return NULL; } - proc = (guacd_proc*) found->data; + proc = ((guacd_proc_map_entry*) found->data)->proc; guac_common_list_unlock(bucket); return proc; @@ -192,11 +221,50 @@ guacd_proc* guacd_proc_map_remove(guacd_proc_map* map, const char* id) { return NULL; } - proc = (guacd_proc*) found->data; + guacd_proc_map_entry* entry = (guacd_proc_map_entry*) found->data; + + /* Find and remove the key from the process list */ + guac_common_list_lock(map->processes); + guac_common_list_remove(map->processes, entry->element); + guac_common_list_unlock(map->processes); + + proc = entry->proc; guac_common_list_remove(bucket, found); + free (entry); + guac_common_list_unlock(bucket); return proc; } +void guacd_proc_map_foreach(guacd_proc_map* map, + guacd_proc_map_foreach_callback* callback, void* data) { + + guac_common_list* list = map->processes; + + guac_common_list_lock(list); + + /* Invoke the callback for every element in the list */ + guac_common_list_element* element; + for (element = list->head; element != NULL; element = element->next) + callback((guacd_proc*) element->data, data); + + guac_common_list_unlock(list); + +} + +void guacd_proc_map_free(guacd_proc_map* map) { + + /* Free the list of all processes */ + guac_common_list_free(map->processes, NULL); + + /* Free each bucket */ + guac_common_list** buckets = map->__buckets; + int i; + for (i = 0; i < GUACD_PROC_MAP_BUCKETS; i++) { + guac_common_list_free(*(buckets + i), free); + } + +} + diff --git a/src/guacd/proc-map.h b/src/guacd/proc-map.h index a24218855..07ebbb97c 100644 --- a/src/guacd/proc-map.h +++ b/src/guacd/proc-map.h @@ -49,6 +49,12 @@ typedef struct guacd_proc_map { */ guac_common_list* __buckets[GUACD_PROC_MAP_BUCKETS]; + /** + * All processes present in the map. For internal use only. To operate on these + * keys, use guacd_proc_map_foreach(). + */ + guac_common_list* processes; + } guacd_proc_map; /** @@ -60,6 +66,16 @@ typedef struct guacd_proc_map { */ guacd_proc_map* guacd_proc_map_alloc(); +/** + * Free all resources allocated for the provided map. Note that this function + * will _not_ clean up the processes contained within the map, only the map + * itself. + * + * @param map + * The guacd proc map to free. + */ +void guacd_proc_map_free(guacd_proc_map* map); + /** * Adds the given process to the client process map. On success, zero is * returned. If adding the client fails (due to lack of space, or duplicate @@ -112,5 +128,36 @@ guacd_proc* guacd_proc_map_retrieve(guacd_proc_map* map, const char* id); */ guacd_proc* guacd_proc_map_remove(guacd_proc_map* map, const char* id); +/** + * A callback function that will be invoked with every guacd_proc stored + * in the provided map, when provided to guacd_proc_map_foreach(), along with + * any provided arbitrary data. + * + * @param proc + * The current guacd process. + * + * @param data + * The arbitrary data provided to guacd_proc_map_foreach(). + */ +typedef void guacd_proc_map_foreach_callback(guacd_proc* proc, void* data); + +/** + * Invoke the provided callback with any provided arbitrary data and each guacd + * proc contained in the provided map, once each and in no particular order. + * + * @param map + * The map from which all guacd processes should be extracted and provided + * to the callback. + * + * @param callback + * The callback function to be invoked once with each guacd process + * contained in the provided map. + * + * @param data + * Arbitrary data to be provided to the callback function. + */ +void guacd_proc_map_foreach(guacd_proc_map* map, + guacd_proc_map_foreach_callback* callback, void* data); + #endif diff --git a/src/guacd/proc.c b/src/guacd/proc.c index 9641ac05a..936ad9085 100644 --- a/src/guacd/proc.c +++ b/src/guacd/proc.c @@ -287,6 +287,26 @@ static int guacd_timed_client_free(guac_client* client, int timeout) { return !free_operation.completed; } +/** + * A reference to the current guacd process. + */ +guacd_proc* guacd_proc_self = NULL; + +/** + * A signal handler that will be invoked when a signal is caught telling this + * guacd process to immediately exit. + * + * @param signal + * The signal that was received. Unused in this function since only + * signals that should result in stopping the proc should invoke this. + */ +static void signal_stop_handler(int signal) { + + /* Stop the current guacd proc */ + guacd_proc_stop(guacd_proc_self); + +} + /** * Starts protocol-specific handling on the given process by loading the client * plugin for that protocol. This function does NOT return. It initializes the @@ -333,6 +353,14 @@ static void guacd_exec_proc(guacd_proc* proc, const char* protocol) { /* Enable keep alive on the broadcast socket */ guac_socket_require_keep_alive(client->socket); + guacd_proc_self = proc; + + /* Clean up and exit if SIGINT or SIGTERM signals are caught */ + struct sigaction signal_stop_action = { 0 }; + signal_stop_action.sa_handler = signal_stop_handler; + sigaction(SIGINT, &signal_stop_action, NULL); + sigaction(SIGTERM, &signal_stop_action, NULL); + /* Add each received file descriptor as a new user */ int received_fd; while ((received_fd = guacd_recv_fd(proc->fd_socket)) != -1) { @@ -458,8 +486,43 @@ guacd_proc* guacd_create_proc(const char* protocol) { } +/** + * Kill the provided child guacd process. This function must be called by the + * parent process, and will block until all processes associated with the + * child process have terminated. + * + * @param proc + * The child guacd process to kill. + */ +static void guacd_proc_kill(guacd_proc* proc) { + + /* Request orderly termination of process */ + if (kill(proc->pid, SIGTERM)) + guacd_log(GUAC_LOG_DEBUG, "Unable to request termination of " + "client process: %s ", strerror(errno)); + + /* Wait for all processes within process group to terminate */ + pid_t child_pid; + while ((child_pid = waitpid(-proc->pid, NULL, 0)) > 0 || errno == EINTR) { + guacd_log(GUAC_LOG_DEBUG, "Child process %i of connection \"%s\" has terminated", + child_pid, proc->client->connection_id); + } + + guacd_log(GUAC_LOG_DEBUG, "All child processes for connection \"%s\" have been terminated.", + proc->client->connection_id); + +} + void guacd_proc_stop(guacd_proc* proc) { + /* A non-zero PID means that this is the parent process */ + if (proc->pid != 0) { + guacd_proc_kill(proc); + return; + } + + /* Otherwise, this is the child process */ + /* Signal client to stop */ guac_client_stop(proc->client); @@ -473,4 +536,3 @@ void guacd_proc_stop(guacd_proc* proc) { close(proc->fd_socket); } - diff --git a/src/libguac/client.c b/src/libguac/client.c index 43d61106e..ceb556ddc 100644 --- a/src/libguac/client.c +++ b/src/libguac/client.c @@ -314,6 +314,10 @@ guac_client* guac_client_alloc() { void guac_client_free(guac_client* client) { + /* Acquire write locks before referencing user pointers */ + guac_acquire_write_lock(&(client->__pending_users_lock)); + guac_acquire_write_lock(&(client->__users_lock)); + /* Remove all pending users */ while (client->__pending_users != NULL) guac_client_remove_user(client, client->__pending_users); @@ -322,6 +326,10 @@ void guac_client_free(guac_client* client) { while (client->__users != NULL) guac_client_remove_user(client, client->__users); + /* Release the locks */ + guac_release_lock(&(client->__users_lock)); + guac_release_lock(&(client->__pending_users_lock)); + if (client->free_handler) { /* FIXME: Errors currently ignored... */ @@ -545,9 +553,9 @@ int guac_client_add_user(guac_client* client, guac_user* user, int argc, char** if (retval == 0) { /* - * Add the user to the list of pending users, to have their connection - * state synchronized asynchronously. - */ + * Add the user to the list of pending users, to have their connection + * state synchronized asynchronously. + */ guac_client_add_pending_user(client, user); /* Update owner pointer if user is owner */ @@ -566,8 +574,8 @@ int guac_client_add_user(guac_client* client, guac_user* user, int argc, char** void guac_client_remove_user(guac_client* client, guac_user* user) { - guac_acquire_write_lock(&(client->__users_lock)); guac_acquire_write_lock(&(client->__pending_users_lock)); + guac_acquire_write_lock(&(client->__users_lock)); /* Update prev / head */ if (user->__prev != NULL) @@ -587,8 +595,8 @@ void guac_client_remove_user(guac_client* client, guac_user* user) { if (user->owner) client->__owner = NULL; - guac_release_lock(&(client->__pending_users_lock)); guac_release_lock(&(client->__users_lock)); + guac_release_lock(&(client->__pending_users_lock)); /* Update owner of user having left the connection. */ if (!user->owner) diff --git a/src/libguac/guacamole/client.h b/src/libguac/guacamole/client.h index 4bfdc0704..9b7868704 100644 --- a/src/libguac/guacamole/client.h +++ b/src/libguac/guacamole/client.h @@ -41,9 +41,8 @@ #include -#include +#include #include -#include #include struct guac_client { diff --git a/src/protocols/rdp/rdp.c b/src/protocols/rdp/rdp.c index 5905f8e6d..7b90f683c 100644 --- a/src/protocols/rdp/rdp.c +++ b/src/protocols/rdp/rdp.c @@ -640,7 +640,7 @@ static int guac_rdp_handle_connection(guac_client* client) { rdp_client->rdp_inst = NULL; /* Free SVC list */ - guac_common_list_free(rdp_client->available_svc); + guac_common_list_free(rdp_client->available_svc, NULL); rdp_client->available_svc = NULL; /* Free RDP keyboard state */