From ad180bed9ccec0ae53cfab97daac7c44900ee2bc Mon Sep 17 00:00:00 2001 From: James Muehlner Date: Thu, 24 Aug 2023 23:04:40 +0000 Subject: [PATCH] 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 8bf303518..10f64159a 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 */ @@ -457,6 +501,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); @@ -470,7 +520,7 @@ int main(int argc, char* argv[]) { } /* Daemon loop */ - for (;;) { + while (!stop_everything) { pthread_t child_thread; @@ -480,7 +530,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; } @@ -504,6 +557,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 f899cd29c..82e3e952a 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 19e2f7856..dd6a2730a 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 b60db48aa..7022c7fc4 100644 --- a/src/protocols/rdp/rdp.c +++ b/src/protocols/rdp/rdp.c @@ -650,7 +650,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 */