From 3349cb2eec32b6049473d95e887da76a8622421a Mon Sep 17 00:00:00 2001
From: Michael Jumper <mjumper@apache.org>
Date: Sat, 31 May 2025 13:06:27 -0700
Subject: [PATCH] GUACAMOLE-2075: Ensure guac_display threads are stopped
 before attempting to free underlying external buffers.

---
 src/libguac/display-priv.h      |  8 +++++
 src/libguac/display.c           | 58 ++++++++++++++++++++++++++++-----
 src/libguac/guacamole/display.h | 26 +++++++++++++++
 src/protocols/rdp/rdp.c         |  4 +++
 src/protocols/vnc/client.c      |  5 +++
 5 files changed, 93 insertions(+), 8 deletions(-)

diff --git a/src/libguac/display-priv.h b/src/libguac/display-priv.h
index 67eb08cd..255fb897 100644
--- a/src/libguac/display-priv.h
+++ b/src/libguac/display-priv.h
@@ -219,6 +219,14 @@
  */
 #define GUAC_DISPLAY_RENDER_STATE_FRAME_NOT_IN_PROGRESS 2
 
+/**
+ * Bitwise flag set on the render_state flag in guac_display when the
+ * guac_display has been stopped and all worker threads have terminated (no
+ * further frames will render). This flag is set when guac_display_stop() has
+ * been invoked, including as part of guac_display_free().
+ */
+#define GUAC_DISPLAY_RENDER_STATE_STOPPED 4
+
 /**
  * Bitwise flag that is set on the state of a guac_display_render_thread when
  * the thread should be stopped.
diff --git a/src/libguac/display.c b/src/libguac/display.c
index 3e080e36..f4a0f656 100644
--- a/src/libguac/display.c
+++ b/src/libguac/display.c
@@ -162,15 +162,58 @@ guac_display* guac_display_alloc(guac_client* client) {
 
 }
 
+void guac_display_stop(guac_display* display) {
+
+    /* Ensure only one of any number of concurrent calls to guac_display_stop()
+     * will actually start terminating the worker threads */
+    guac_fifo_lock(&display->ops);
+
+    /* Stop and clean up worker threads if the display is not already being
+     * stopped (we don't use the GUAC_DISPLAY_RENDER_STATE_STOPPED flag here,
+     * as we must consider the case that guac_display_stop() has already been
+     * called in a different thread but has not yet finished) */
+    if (guac_fifo_is_valid(&display->ops)) {
+
+        /* Stop further use of the operation FIFO */
+        guac_fifo_invalidate(&display->ops);
+        guac_fifo_unlock(&display->ops);
+
+        /* Wait for all worker threads to terminate (they should nearly immediately
+         * terminate following invalidation of the FIFO) */
+        for (int i = 0; i < display->worker_thread_count; i++)
+            pthread_join(display->worker_threads[i], NULL);
+
+        /* All worker threads are now terminated and may be safely cleaned up */
+        guac_mem_free(display->worker_threads);
+        display->worker_thread_count = 0;
+
+        /* NOTE: The only other reference to the worker_threads AT ALL is in
+         * guac_display_create(). Nothing outside of guac_display_create() and
+         * guac_display_stop() references worker_threads or worker_thread_count. */
+
+        /* Notify other calls to guac_display_stop() that the display is now
+         * officially stopped */
+        guac_flag_set(&display->render_state, GUAC_DISPLAY_RENDER_STATE_STOPPED);
+
+    }
+
+    /* Even if it isn't this particular call to guac_display_stop() that
+     * terminates and waits on all the worker threads, ensure that we only
+     * return after all threads are known to have been stopped */
+    else {
+
+        guac_fifo_unlock(&display->ops);
+
+        guac_flag_wait_and_lock(&display->render_state, GUAC_DISPLAY_RENDER_STATE_STOPPED);
+        guac_flag_unlock(&display->render_state);
+
+    }
+
+}
+
 void guac_display_free(guac_display* display) {
 
-    /* Stop further use of the operation FIFO */
-    guac_fifo_invalidate(&display->ops);
-
-    /* Wait for all worker threads to terminate (they should nearly immediately
-     * terminate following invalidation of the FIFO) */
-    for (int i = 0; i < display->worker_thread_count; i++)
-        pthread_join(display->worker_threads[i], NULL);
+    guac_display_stop(display);
 
     /* All locks, FIFOs, etc. are now unused and can be safely destroyed */
     guac_flag_destroy(&display->render_state);
@@ -188,7 +231,6 @@ void guac_display_free(guac_display* display) {
     while (display->last_frame.layers != NULL)
         guac_display_free_layer(display->last_frame.layers);
 
-    guac_mem_free(display->worker_threads);
     guac_mem_free(display);
 
 }
diff --git a/src/libguac/guacamole/display.h b/src/libguac/guacamole/display.h
index baf768d4..9894042a 100644
--- a/src/libguac/guacamole/display.h
+++ b/src/libguac/guacamole/display.h
@@ -212,6 +212,32 @@ struct guac_display_layer_raw_context {
  */
 guac_display* guac_display_alloc(guac_client* client);
 
+/**
+ * Stops all background processes that may be running beneath the given
+ * guac_display, ensuring nothing within guac_display will continue to access
+ * any memory unless explicitly and externally requested. After this function
+ * returns, all background threads used by guac_display have stopped. This
+ * function is threadsafe and may be safely invoked by multiple threads. All
+ * concurrent calls to this function will block until all background threads
+ * used by the guac_display have terminated.
+ *
+ * This function DOES NOT affect the state of a guac_display_render_thread,
+ * which must be similarly stopped and destroyed with a call to
+ * guac_display_render_thread_destroy() before underlying external buffers can
+ * be safely freed.
+ *
+ * NOTE: Invoking this function may be necessary to allow external objects to
+ * be safely cleaned up, particularly if external buffers have been provided to
+ * replace the buffers allocated by guac_display. Doing otherwise would mean
+ * that background worker threads used for encoding by guac_display may
+ * continue to check the contents of external buffers while those buffers are
+ * being freed.
+ *
+ * @param display
+ *     The display to stop.
+ */
+void guac_display_stop(guac_display* display);
+
 /**
  * Frees all resources associated with the given guac_display.
  *
diff --git a/src/protocols/rdp/rdp.c b/src/protocols/rdp/rdp.c
index 5f4249e0..aad1763b 100644
--- a/src/protocols/rdp/rdp.c
+++ b/src/protocols/rdp/rdp.c
@@ -680,6 +680,10 @@ static int guac_rdp_handle_connection(guac_client* client) {
     context->buffer = NULL;
     guac_display_layer_close_raw(default_layer, context);
 
+    /* Ensure all background rendering processes are stopped before freeing
+     * underlying memory */
+    guac_display_stop(rdp_client->display);
+
     /* Clean up FreeRDP internal GDI implementation (this must be done BEFORE
      * freeing the guac_display, as freeing the GDI will free objects like
      * rdpPointer that will attempt to free associated guac_display_layer
diff --git a/src/protocols/vnc/client.c b/src/protocols/vnc/client.c
index 206dad89..5573d277 100644
--- a/src/protocols/vnc/client.c
+++ b/src/protocols/vnc/client.c
@@ -132,6 +132,11 @@ int guac_vnc_client_free_handler(guac_client* client) {
     guac_vnc_client* vnc_client = (guac_vnc_client*) client->data;
     guac_vnc_settings* settings = vnc_client->settings;
 
+    /* Ensure all background rendering processes are stopped before freeing
+     * underlying memory */
+    if (vnc_client->display != NULL)
+        guac_display_stop(vnc_client->display);
+
     /* Clean up VNC client*/
     rfbClient* rfb_client = vnc_client->rfb_client;
     if (rfb_client != NULL) {