Merge pull request #118 from glyptodon/user-validity-race

GUAC-1389: Fix user validity race.

src/common/guac_cursor.c

@@ -104,6 +104,36 @@ void guac_common_cursor_dup(guac_common_cursor* cursor, guac_user* user,
 
 }
 
+/**
+ * Callback for guac_client_for_user() which shows the cursor layer for the
+ * given user (if they exist). The cursor layer is normally hidden when a user
+ * is moving the mouse, and will only be shown if a DIFFERENT user is moving
+ * the mouse.
+ *
+ * @param user
+ *     The user to show the cursor to, or NULL if that user does not exist.
+ *
+ * @param data
+ *     A pointer to the guac_common_cursor structure describing the cursor to
+ *     be shown.
+ *
+ * @return
+ *     Always NULL.
+ */
+static void* guac_common_cursor_show(guac_user* user, void* data) {
+
+    guac_common_cursor* cursor = (guac_common_cursor*) data;
+
+    /* Make cursor layer visible to given user */
+    if (user != NULL) {
+        guac_protocol_send_shade(user->socket, cursor->layer, 255);
+        guac_socket_flush(user->socket);
+    }
+
+    return NULL;
+
+}
+
 void guac_common_cursor_move(guac_common_cursor* cursor, guac_user* user,
         int x, int y) {
 
@@ -115,10 +145,8 @@ void guac_common_cursor_move(guac_common_cursor* cursor, guac_user* user,
         cursor->user = user;
 
         /* Make cursor layer visible to previous user */
-        if (last_user != NULL) {
+        guac_client_for_user(cursor->client, last_user,
-            guac_protocol_send_shade(last_user->socket, cursor->layer, 255);
+                guac_common_cursor_show, cursor);
-            guac_socket_flush(last_user->socket);
-        }
 
         /* Show hardware cursor */
         guac_protocol_send_cursor(user->socket,
@@ -208,6 +236,40 @@ static void* __send_user_cursor_image(guac_user* user, void* data) {
 
 }
 
+/**
+ * Callback for guac_client_for_user() which updates the hardware cursor and
+ * hotspot for the given user (if they exist). The hardware cursor image is
+ * normally hidden when a user is not moving the mouse, and will only be shown
+ * if that user begins moving the mouse.
+ *
+ * @param user
+ *     The user whose hardware cursor should be updated, or NULL if that user
+ *     does not exist.
+ *
+ * @param data
+ *     A pointer to the guac_common_cursor structure describing the cursor to
+ *     be sent as the hardware cursor.
+ *
+ * @return
+ *     Always NULL.
+ */
+static void* guac_common_cursor_update(guac_user* user, void* data) {
+
+    guac_common_cursor* cursor = (guac_common_cursor*) data;
+
+    /* Update hardware cursor of current user */
+    if (user != NULL) {
+        guac_protocol_send_cursor(user->socket,
+                cursor->hotspot_x, cursor->hotspot_y,
+                cursor->layer, 0, 0, cursor->width, cursor->height);
+
+        guac_socket_flush(user->socket);
+    }
+
+    return NULL;
+
+}
+
 void guac_common_cursor_set_argb(guac_common_cursor* cursor, int hx, int hy,
     unsigned const char* data, int width, int height, int stride) {
 
@@ -242,13 +304,10 @@ void guac_common_cursor_set_argb(guac_common_cursor* cursor, int hx, int hy,
 
     guac_socket_flush(cursor->client->socket);
 
-    /* Update hardware cursor of current user */
+    /* Update hardware cursor of current user (if they are indeed valid) */
-    if (cursor->user != NULL) {
+    if (cursor->user != NULL)
-        guac_protocol_send_cursor(cursor->user->socket, hx, hy,
+        guac_client_for_user(cursor->client, cursor->user,
-                cursor->layer, 0, 0, width, height);
+                guac_common_cursor_update, cursor);
-
-        guac_socket_flush(cursor->user->socket);
-    }
 
 }
 
@@ -303,12 +362,3 @@ void guac_common_cursor_set_blank(guac_common_cursor* cursor) {
 
 }
 
-void guac_common_cursor_remove_user(guac_common_cursor* cursor,
-        guac_user* user) {
-
-    /* Disassociate from given user */
-    if (cursor->user == user)
-        cursor->user = NULL;
-
-}
-

src/common/guac_cursor.h

@@ -252,19 +252,4 @@ void guac_common_cursor_set_ibar(guac_common_cursor* cursor);
  */
 void guac_common_cursor_set_blank(guac_common_cursor* cursor);
 
-/**
- * Removes the given user, such that future synchronization will not occur.
- * This is necessary when a user leaves the connection. If a user leaves the
- * connection and this is not called, the corresponding guac_user and socket
- * may cease to be valid, and future synchronization attempts will segfault.
- *
- * @param cursor
- *     The cursor to remove the user from.
- *
- * @param user
- *     The user to remove.
- */
-void guac_common_cursor_remove_user(guac_common_cursor* cursor,
-        guac_user* user);
-
 #endif

src/libguac/client.c

@@ -637,6 +637,43 @@ void* guac_client_for_owner(guac_client* client, guac_user_callback* callback,
 
 }
 
+void* guac_client_for_user(guac_client* client, guac_user* user,
+        guac_user_callback* callback, void* data) {
+
+    guac_user* current;
+
+    int user_valid = 0;
+    void* retval;
+
+    pthread_rwlock_rdlock(&(client->__users_lock));
+
+    /* Loop through all users, searching for a pointer to the given user */
+    current = client->__users;
+    while (current != NULL) {
+
+        /* If the user's pointer exists in the list, they are indeed valid */
+        if (current == user) {
+            user_valid = 1;
+            break;
+        }
+
+        current = current->__next;
+    }
+
+    /* Use NULL if user does not actually exist */
+    if (!user_valid)
+        user = NULL;
+
+    /* Invoke callback with requested user (if they exist) */
+    retval = callback(user, data);
+
+    pthread_rwlock_unlock(&(client->__users_lock));
+
+    /* Return value from callback */
+    return retval;
+
+}
+
 int guac_client_end_frame(guac_client* client) {
 
     /* Update and send timestamp */

src/libguac/guacamole/client.h

@@ -457,14 +457,10 @@ void guac_client_foreach_user(guac_client* client,
         guac_user_callback* callback, void* data);
 
 /**
- * Retrieves the connected user that is marked as the owner. The owner of a
+ * Calls the given function with the currently-connected user that is marked as
- * connection is the user that established the initial connection that created
+ * the owner. The owner of a connection is the user that established the
- * the connection (the first user to connect and join).
- *
- * Calls the given function on with the currently-connected user that is marked
- * as the owner. The owner of a connection is the user that established the
  * initial connection that created the connection (the first user to connect
- * and join). The function will be given a reference to a guac_user and the
+ * and join). The function will be given a reference to the guac_user and the
  * specified arbitrary data. If the owner has since left the connection, the
  * function will instead be invoked with NULL as the guac_user. The value
  * returned by the callback will be returned by this function.
@@ -480,12 +476,58 @@ void guac_client_foreach_user(guac_client* client,
  * @param client
  *     The client to retrieve the owner from.
  *
+ * @param callback
+ *     The callback to invoke on the user marked as the owner of the
+ *     connection. NULL will be passed to this callback instead if there is no
+ *     owner.
+ *
+ * @param data
+ *     Arbitrary data to pass to the given callback.
+ *
  * @return
  *     The value returned by the callback.
  */
 void* guac_client_for_owner(guac_client* client, guac_user_callback* callback,
         void* data);
 
+/**
+ * Calls the given function with the given user ONLY if they are currently
+ * connected. The function will be given a reference to the guac_user and the
+ * specified arbitrary data. If the provided user doesn't exist or has since
+ * left the connection, the function will instead be invoked with NULL as the
+ * guac_user. The value returned by the callback will be returned by this
+ * function.
+ *
+ * This function is reentrant, but the user list MUST NOT be manipulated
+ * within the same thread as a callback to this function.
+ *
+ * Because this function depends on a consistent list of connected users, this
+ * function MUST NOT be invoked within the same thread as a "leave" or "join"
+ * handler. Doing so results in undefined behavior, including possible
+ * segfaults.
+ *
+ * @param client
+ *     The client that the given user is expected to be associated with.
+ *
+ * @param user
+ *     The user to provide to the given callback if valid. The pointer need not
+ *     even point to properly allocated memory; the user will only be passed to
+ *     the callback function if they are valid, and the provided user pointer
+ *     will not be dereferenced during this process.
+ *
+ * @param callback
+ *     The callback to invoke on the given user if they are valid. NULL will be
+ *     passed to this callback instead if the user is not valid.
+ *
+ * @param data
+ *     Arbitrary data to pass to the given callback.
+ *
+ * @return
+ *     The value returned by the callback.
+ */
+void* guac_client_for_user(guac_client* client, guac_user* user,
+        guac_user_callback* callback, void* data);
+
 /**
  * Marks the end of the current frame by sending a "sync" instruction to
  * all connected users. This instruction will contain the current timestamp.

src/protocols/vnc/user.c

@@ -100,12 +100,3 @@ int guac_vnc_user_join_handler(guac_user* user, int argc, char** argv) {
 
 }
 
-int guac_vnc_user_leave_handler(guac_user* user) {
-
-    guac_vnc_client* vnc_client = (guac_vnc_client*) user->client->data;
-
-    guac_common_cursor_remove_user(vnc_client->display->cursor, user);
-
-    return 0;
-}
-

src/protocols/vnc/user.h

@@ -32,10 +32,5 @@
  */
 guac_user_join_handler guac_vnc_user_join_handler;
 
-/**
- * Handler for leaving users.
- */
-guac_user_leave_handler guac_vnc_user_leave_handler;
-
 #endif