GUACAMOLE-377: Wrap all message handling within drawing context.

It is otherwise difficult to guarantee that all operations touching the
pending frame buffer will occur while holding an open raw context,
resulting in unstable behavior.

src/protocols/rdp/gdi.c

@@ -81,18 +81,10 @@ BOOL guac_rdp_gdi_begin_paint(rdpContext* context) {
     guac_rdp_client* rdp_client = (guac_rdp_client*) client->data;
     rdpGdi* gdi = context->gdi;
 
-    GUAC_ASSERT(rdp_client->current_paint == NULL);
-
-    guac_display_layer* default_layer =
-        guac_display_default_layer(rdp_client->display);
-
-    guac_display_layer_raw_context* current_paint =
-        guac_display_layer_open_raw(default_layer);
-    current_paint->buffer = gdi->primary_buffer;
-    current_paint->stride = gdi->stride;
-    guac_rect_init(&current_paint->bounds, 0, 0, gdi->width, gdi->height);
-
-    rdp_client->current_paint = current_paint;
+    guac_display_layer_raw_context* current_context = rdp_client->current_context;
+    current_context->buffer = gdi->primary_buffer;
+    current_context->stride = gdi->stride;
+    guac_rect_init(&current_context->bounds, 0, 0, gdi->width, gdi->height);
 
     return TRUE;
 
@@ -104,12 +96,7 @@ BOOL guac_rdp_gdi_end_paint(rdpContext* context) {
     guac_rdp_client* rdp_client = (guac_rdp_client*) client->data;
     rdpGdi* gdi = context->gdi;
 
-    GUAC_ASSERT(rdp_client->current_paint != NULL);
-    guac_display_layer_raw_context* current_paint = rdp_client->current_paint;
-    rdp_client->current_paint = NULL;
-
-    guac_display_layer* default_layer =
-        guac_display_default_layer(rdp_client->display);
+    guac_display_layer_raw_context* current_context = rdp_client->current_context;
 
     /* Ignore paint if GDI output is suppressed */
     if (gdi->suppressOutput)
@@ -126,12 +113,11 @@ BOOL guac_rdp_gdi_end_paint(rdpContext* context) {
 
     guac_rect dst_rect;
     guac_rect_init(&dst_rect, x, y, w, h);
-    guac_rect_constrain(&dst_rect, &current_paint->bounds);
+    guac_rect_constrain(&dst_rect, &current_context->bounds);
 
-    guac_rect_extend(&current_paint->dirty, &dst_rect);
+    guac_rect_extend(&current_context->dirty, &dst_rect);
 
 paint_complete:
-    guac_display_layer_close_raw(default_layer, current_paint);
     return TRUE;
 
 }
@@ -146,27 +132,19 @@ BOOL guac_rdp_gdi_desktop_resize(rdpContext* context) {
     int width = guac_rdp_get_width(context->instance);
     int height = guac_rdp_get_height(context->instance);
 
-    /* It's not expected that a resize will be requested by FreeRDP in the
-     * middle of a paint operation, but if this does happen we incorporate the
-     * resize into that operation */
-    guac_display_layer_raw_context* current_paint = rdp_client->current_paint;
-    if (current_paint == NULL)
-        current_paint = guac_display_layer_open_raw(default_layer);
+    guac_display_layer_raw_context* current_context = rdp_client->current_context;
 
+    /* Resize FreeRDP's GDI buffer */
     BOOL retval = gdi_resize(context->gdi, width, height);
     GUAC_ASSERT(gdi->primary_buffer != NULL);
 
     /* Update our reference to the GDI buffer, as well as any structural
      * details, which may now all be different */
-    current_paint->buffer = gdi->primary_buffer;
-    current_paint->stride = gdi->stride;
-    guac_rect_init(&current_paint->bounds, 0, 0, gdi->width, gdi->height);
-
-    /* There's no need to close the context if we are just adding to an
-     * in-progress paint operation */
-    if (current_paint != rdp_client->current_paint)
-        guac_display_layer_close_raw(default_layer, current_paint);
+    current_context->buffer = gdi->primary_buffer;
+    current_context->stride = gdi->stride;
+    guac_rect_init(&current_context->bounds, 0, 0, gdi->width, gdi->height);
 
+    /* Resize layer to match new display dimensions and underlying buffer */
     guac_display_layer_resize(default_layer, gdi->width, gdi->height);
     guac_client_log(client, GUAC_LOG_DEBUG, "Server resized display to %ix%i",
             gdi->width, gdi->height);

src/protocols/rdp/rdp.c

@@ -465,6 +465,41 @@ static int rdp_guac_client_wait_for_messages(guac_client* client,
 
 }
 
+/**
+ * Handles any queued RDP-related events, including inbound RDP messages that
+ * have been received, updating the Guacamole display accordingly.
+ *
+ * @param rdp_client
+ *     The guac_rdp_client of the RDP connection whose current messages should
+ *     be handled.
+ *
+ * @return
+ *     True (non-zero) if messages were handled successfully, false (zero)
+ *     otherwise.
+ */
+static int guac_rdp_handle_events(guac_rdp_client* rdp_client) {
+
+    freerdp* rdp_inst = rdp_client->rdp_inst;
+    guac_display_layer* default_layer = guac_display_default_layer(rdp_client->display);
+
+    /* All potential drawing operations must occur while holding an open context */
+    guac_display_layer_raw_context* context = guac_display_layer_open_raw(default_layer);
+    rdp_client->current_context = context;
+
+    /* Actually handle messages (this may result in drawing to the
+     * guac_display, resizing the display buffer, etc.) */
+    pthread_mutex_lock(&(rdp_client->message_lock));
+    int retval = freerdp_check_event_handles(GUAC_RDP_CONTEXT(rdp_inst));
+    pthread_mutex_unlock(&(rdp_client->message_lock));
+
+    /* There will be no further drawing operations */
+    guac_display_layer_close_raw(default_layer, context);
+    rdp_client->current_context = NULL;
+
+    return retval;
+
+}
+
 /**
  * Connects to an RDP server as described by the guac_rdp_settings structure
  * associated with the given client, allocating and freeing all objects
@@ -590,13 +625,9 @@ static int guac_rdp_handle_connection(guac_client* client) {
             do {
 
                 /* Handle any queued FreeRDP events (this may result in RDP
-                 * messages being sent) */
-                pthread_mutex_lock(&(rdp_client->message_lock));
-                int event_result = freerdp_check_event_handles(GUAC_RDP_CONTEXT(rdp_inst));
-                pthread_mutex_unlock(&(rdp_client->message_lock));
-
-                /* Abort if FreeRDP event handling fails */
-                if (!event_result) {
+                 * messages being sent), aborting if FreeRDP event handling
+                 * fails */
+                if (!guac_rdp_handle_events(rdp_client)) {
                     wait_result = -1;
                     break;
                 }
@@ -668,14 +699,6 @@ static int guac_rdp_handle_connection(guac_client* client) {
     freerdp_disconnect(rdp_inst);
     pthread_mutex_unlock(&(rdp_client->message_lock));
 
-    /* Any outstanding paint operation must have completed by now (we must do
-     * this before freeing FreeRDP's GDI as the guac_display will have a
-     * reference to the GDI's primary_buffer) */
-    if (rdp_client->current_paint) {
-        guac_display_layer_close_raw(guac_display_default_layer(rdp_client->display), rdp_client->current_paint);
-        rdp_client->current_paint = NULL;
-    }
-
     /* Free display */
     guac_display_free(rdp_client->display);
     rdp_client->display = NULL;

src/protocols/rdp/rdp.h

@@ -107,11 +107,13 @@ typedef struct guac_rdp_client {
     guac_display_layer* current_surface;
 
     /**
-     * The current, in-progress paint operation, as signalled by FreeRDP's
-     * "BeginPaint" callback. Once the paint operation is complete (signalled
-     * by "EndPaint"), this will be NULL.
+     * The current raw context that can be used to draw to Guacamole's default
+     * layer. This context is obtained prior to handling any events/messages
+     * within FreeRDP and closed when FreeRDP is done handling those
+     * events/messages. If event handling is not currently underway, this will
+     * be NULL.
      */
-    guac_display_layer_raw_context* current_paint;
+    guac_display_layer_raw_context* current_context;
 
     /**
      * Whether the RDP server has reported that a new frame is in progress, and

src/protocols/vnc/display.c

@@ -51,48 +51,25 @@ void guac_vnc_update(rfbClient* client, int x, int y, int w, int h) {
     guac_vnc_client* vnc_client = (guac_vnc_client*) gc->data;
     guac_display_layer* default_layer = guac_display_default_layer(vnc_client->display);
 
-    int rfb_height = client->height;
-    int rfb_width  = client->width;
-
-    guac_display_layer_raw_context* context;
+    guac_display_layer_raw_context* context = vnc_client->current_context;
     unsigned int vnc_bpp = client->format.bitsPerPixel / 8;
-    size_t vnc_stride = guac_mem_ckd_mul_or_die(vnc_bpp, rfb_width);
+    size_t vnc_stride = guac_mem_ckd_mul_or_die(vnc_bpp, client->width);
 
     /* Convert operation coordinates to guac_rect for easier manipulation */
     guac_rect op_bounds;
     guac_rect_init(&op_bounds, x, y, w, h);
 
-    /* Point directly at framebuffer if the pixel format used is identical to
-     * that expected by guac_display. Resize of the layer is implicit in this
-     * case. */
-    if (vnc_bpp == GUAC_DISPLAY_LAYER_RAW_BPP && !vnc_client->settings->swap_red_blue) {
-
-        context = guac_display_layer_open_raw(default_layer);
-        context->buffer = client->frameBuffer;
-        context->stride = vnc_stride;
-
-        /* Update bounds of pending frame to match those of RFB framebuffer */
-        guac_rect_init(&context->bounds, 0, 0, rfb_width, rfb_height);
-
-        /* Ensure operation bounds are within possibly updated bounds of the
-         * pending frame (now the RFB client framebuffer) */
-        guac_rect_constrain(&op_bounds, &context->bounds);
-
-    }
-
-    /* All other framebuffer formats must be manually converted */
-    else {
+    /* Ensure operation bounds are within possibly updated bounds of the
+     * pending frame (now the RFB client framebuffer) */
+    guac_rect_constrain(&op_bounds, &context->bounds);
 
-        /* Resize the surface if VNC screen size has changed (this call
-         * automatically deals with invalid dimensions and is a no-op if the size
-         * has not changed) */
-        guac_display_layer_resize(default_layer, rfb_width, rfb_height);
+    /* NOTE: The guac_display will be pointed directly at the libvncclient
+     * framebuffer if the pixel format used is identical to that expected by
+     * guac_display. No need to manually copy anything around in that case. */
 
-        /* Begin drawing operation directly to default layer. NOTE: This is
-         * intentionally after the call to guac_display_layer_resize() to
-         * ensure the bounds of the resulting context are representative of the
-         * resize operation. */
-        context = guac_display_layer_open_raw(default_layer);
+    /* All framebuffer formats must be manually converted if not identical to
+     * the format used by guac_display */
+    if (vnc_bpp != GUAC_DISPLAY_LAYER_RAW_BPP || vnc_client->settings->swap_red_blue) {
 
         /* Ensure draw is within current bounds of the pending frame */
         guac_rect_constrain(&op_bounds, &context->bounds);
@@ -152,9 +129,6 @@ void guac_vnc_update(rfbClient* client, int x, int y, int w, int h) {
         vnc_client->copy_rect_used = 0;
     }
 
-    /* Draw operation is now complete */
-    guac_display_layer_close_raw(default_layer, context);
-
 }
 
 void guac_vnc_copyrect(rfbClient* client, int src_x, int src_y, int w, int h, int dest_x, int dest_y) {
@@ -393,26 +367,8 @@ rfbBool guac_vnc_malloc_framebuffer(rfbClient* rfb_client) {
     guac_client* gc = rfbClientGetClientData(rfb_client, GUAC_VNC_CLIENT_KEY);
     guac_vnc_client* vnc_client = (guac_vnc_client*) gc->data;
 
-    /* Resize of underlying buffer must be performed while holding an open raw
-     * context if the guac_display is active (replacing the underlying
-     * framebuffer while guac_display may still attempt to flush a pending
-     * frame is bad news, as that flush may still reference the freed buffer) */
-    if (vnc_client->display != NULL) {
-
-        guac_display_layer* default_layer = guac_display_default_layer(vnc_client->display);
-        guac_display_layer_resize(default_layer, rfb_client->width, rfb_client->height);
-
-        /* Use original, wrapped proc to resize the buffer maintained by libvncclient */
-        guac_display_layer_raw_context* context = guac_display_layer_open_raw(default_layer);
-        rfbBool result = vnc_client->rfb_MallocFrameBuffer(rfb_client);
-        guac_display_layer_close_raw(default_layer, context);
-
-        return result;
-
-    }
-
-    /* No need to bracket the buffer allocation in a raw context if there's no
-     * guac_display yet */
+    /* Use original, wrapped proc to resize the buffer maintained by
+     * libvncclient */
     return vnc_client->rfb_MallocFrameBuffer(rfb_client);
 
 }

src/protocols/vnc/vnc.c

@@ -269,6 +269,57 @@ static int guac_vnc_wait_for_messages(rfbClient* rfb_client, int msec_timeout) {
 
 }
 
+/**
+ * Handles any inbound VNC messages that have been received, updating the
+ * Guacamole display accordingly.
+ *
+ * @param vnc_client
+ *     The guac_vnc_client of the VNC connection whose current messages should
+ *     be handled.
+ *
+ * @return
+ *     True (non-zero) if messages were handled successfully, false (zero)
+ *     otherwise.
+ */
+static rfbBool guac_vnc_handle_messages(guac_vnc_client* vnc_client) {
+
+    rfbClient* rfb_client = vnc_client->rfb_client;
+    guac_display_layer* default_layer = guac_display_default_layer(vnc_client->display);
+
+    /* All potential drawing operations must occur while holding an open context */
+    guac_display_layer_raw_context* context = guac_display_layer_open_raw(default_layer);
+    vnc_client->current_context = context;
+
+    /* Actually handle messages (this may result in drawing to the
+     * guac_display, resizing the display buffer, etc.) */
+    rfbBool retval = HandleRFBServerMessage(rfb_client);
+
+    /* Use the buffer of libvncclient directly if it matches the guac_display
+     * format */
+    unsigned int vnc_bpp = rfb_client->format.bitsPerPixel / 8;
+    if (vnc_bpp == GUAC_DISPLAY_LAYER_RAW_BPP && !vnc_client->settings->swap_red_blue) {
+
+        context->buffer = rfb_client->frameBuffer;
+        context->stride = guac_mem_ckd_mul_or_die(vnc_bpp, rfb_client->width);
+
+        /* Update bounds of pending frame to match those of RFB framebuffer */
+        guac_rect_init(&context->bounds, 0, 0, rfb_client->width, rfb_client->height);
+
+    }
+
+    /* There will be no further drawing operations */
+    guac_display_layer_close_raw(default_layer, context);
+    vnc_client->current_context = NULL;
+
+    /* Resize the surface if VNC screen size has changed (this call
+     * automatically deals with invalid dimensions and is a no-op
+     * if the size has not changed) */
+    guac_display_layer_resize(default_layer, rfb_client->width, rfb_client->height);
+
+    return retval;
+
+}
+
 void* guac_vnc_client_thread(void* data) {
 
     guac_client* client = (guac_client*) data;
@@ -557,7 +608,7 @@ void* guac_vnc_client_thread(void* data) {
             do {
 
                 /* Handle any message received */
-                if (!HandleRFBServerMessage(rfb_client)) {
+                if (!guac_vnc_handle_messages(vnc_client)) {
                     guac_client_abort(client,
                             GUAC_PROTOCOL_STATUS_UPSTREAM_ERROR,
                             "Error handling message from VNC server.");

src/protocols/vnc/vnc.h

@@ -107,6 +107,12 @@ typedef struct guac_vnc_client {
      */
     guac_display* display;
 
+    /**
+     * The context of the current drawing (update) operation, if any. If no
+     * operation is in progress, this will be NULL.
+     */
+    guac_display_layer_raw_context* current_context;
+
     /**
      * Internal clipboard.
      */