GUACAMOLE-1846: Synchronize new users to connections in batches to fix join race condition.

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

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);
 
 /**

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/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,

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,

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);
 

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/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);
 
 }
-

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;
 
     }

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 */
@@ -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));