GUACAMOLE-1846: Ensure that stuck child processes are nonetheless cle…

…aned up.

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

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,

src/guacd/daemon.c

@@ -43,6 +43,7 @@
 #include <sys/socket.h>
 #include <sys/stat.h>
 #include <sys/types.h>
+#include <sys/wait.h>
 #include <unistd.h>
 
 #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));

src/guacd/proc-map.c

@@ -27,6 +27,24 @@
 #include <stdlib.h>
 #include <string.h>
 
+/**
+ * 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);
+    }
+
+}
+

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