From 809df89c0869e6c595c19ef6b4b1d2cbfab802ff Mon Sep 17 00:00:00 2001
From: matt335672 <30179339+matt335672@users.noreply.github.com>
Date: Mon, 3 Jun 2024 15:50:16 +0100
Subject: [PATCH 1/2] Prevent SEGV when resizing with GFX

The xrdp_enc_data contains a union for handling surface commands
and gfx commands. Memory processing is different for these two
options.

The default destructor for the encoder FIFO only knows about surface
commands. Consequently, if the encoder has queued GFX data when the
encoder is closed, the destructor processes the queued data as if
it contained surface commands rather than GFX commands. This typically
causes a SEGV as the drects field of the overlaid surface command
structure is not pointing at anything valid when it is freed.
---
 xrdp/xrdp_encoder.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/xrdp/xrdp_encoder.c b/xrdp/xrdp_encoder.c
index 2f077b5c8..ad4aaeb57 100644
--- a/xrdp/xrdp_encoder.c
+++ b/xrdp/xrdp_encoder.c
@@ -88,8 +88,15 @@ static void
 xrdp_enc_data_destructor(void *item, void *closure)
 {
     XRDP_ENC_DATA *enc = (XRDP_ENC_DATA *)item;
-    g_free(enc->u.sc.drects);
-    g_free(enc->u.sc.crects);
+    if (ENC_IS_BIT_SET(enc->flags, ENC_FLAGS_GFX_BIT))
+    {
+        g_free(enc->u.gfx.cmd);
+    }
+    else
+    {
+        g_free(enc->u.sc.drects);
+        g_free(enc->u.sc.crects);
+    }
     g_free(enc);
 }
 

From 985b0de35e84ae5e7e46bd1a2b2e9fa1e31e53c0 Mon Sep 17 00:00:00 2001
From: matt335672 <30179339+matt335672@users.noreply.github.com>
Date: Mon, 3 Jun 2024 15:56:05 +0100
Subject: [PATCH 2/2] Add explicit object for the encoder finishing

On a resize, the encoder is deleted. At present this is done by asking
the encoder to exit, and then waiting a second.

- On slower systems, a second may not be enough, and so the encoder
  data structures are freed while they are still being used by the
  encoder.
- On quicker systems, resizes are delayed by hundreds of milliseconds
  longer than they need to be.

This commit adds a wait object which the encoder can use to signal it
has actually finished.
---
 xrdp/xrdp_encoder.c | 17 ++++++++++++-----
 xrdp/xrdp_encoder.h |  3 ++-
 2 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/xrdp/xrdp_encoder.c b/xrdp/xrdp_encoder.c
index ad4aaeb57..125c0d694 100644
--- a/xrdp/xrdp_encoder.c
+++ b/xrdp/xrdp_encoder.c
@@ -220,7 +220,8 @@ xrdp_encoder_create(struct xrdp_mm *mm)
     g_snprintf(buf, 1024, "xrdp_%8.8x_encoder_event_processed", pid);
     self->xrdp_encoder_event_processed = g_create_wait_obj(buf);
     g_snprintf(buf, 1024, "xrdp_%8.8x_encoder_term", pid);
-    self->xrdp_encoder_term = g_create_wait_obj(buf);
+    self->xrdp_encoder_term_request = g_create_wait_obj(buf);
+    self->xrdp_encoder_term_done = g_create_wait_obj(buf);
     if (client_info->gfx)
     {
         const char *env_var = g_getenv("XRDP_GFX_FRAMES_IN_FLIGHT");
@@ -296,8 +297,12 @@ xrdp_encoder_delete(struct xrdp_encoder *self)
         return;
     }
     /* tell worker thread to shut down */
-    g_set_wait_obj(self->xrdp_encoder_term);
-    g_sleep(1000);
+    g_set_wait_obj(self->xrdp_encoder_term_request);
+    g_obj_wait(&self->xrdp_encoder_term_done, 1, NULL, 0, 5000);
+    if (!g_is_wait_obj_set(self->xrdp_encoder_term_done))
+    {
+        LOG(LOG_LEVEL_WARNING, "Encoder failed to shut down cleanly");
+    }
 
 #ifdef XRDP_RFXCODEC
     for (index = 0; index < 16; index++)
@@ -330,7 +335,8 @@ xrdp_encoder_delete(struct xrdp_encoder *self)
     /* destroy wait objects used for signalling */
     g_delete_wait_obj(self->xrdp_encoder_event_to_proc);
     g_delete_wait_obj(self->xrdp_encoder_event_processed);
-    g_delete_wait_obj(self->xrdp_encoder_term);
+    g_delete_wait_obj(self->xrdp_encoder_term_request);
+    g_delete_wait_obj(self->xrdp_encoder_term_done);
 
     /* cleanup fifos */
     fifo_delete(self->fifo_to_proc, NULL);
@@ -1413,7 +1419,7 @@ proc_enc_msg(void *arg)
     event_to_proc = self->xrdp_encoder_event_to_proc;
 
     term_obj = g_get_term();
-    lterm_obj = self->xrdp_encoder_term;
+    lterm_obj = self->xrdp_encoder_term_request;
 
     cont = 1;
     while (cont)
@@ -1464,6 +1470,7 @@ proc_enc_msg(void *arg)
         }
 
     } /* end while (cont) */
+    g_set_wait_obj(self->xrdp_encoder_term_done);
     LOG_DEVEL(LOG_LEVEL_DEBUG, "proc_enc_msg: thread exit");
     return 0;
 }
diff --git a/xrdp/xrdp_encoder.h b/xrdp/xrdp_encoder.h
index 8dc1311ba..3996fac54 100644
--- a/xrdp/xrdp_encoder.h
+++ b/xrdp/xrdp_encoder.h
@@ -24,7 +24,8 @@ struct xrdp_encoder
     int max_compressed_bytes;
     tbus xrdp_encoder_event_to_proc;
     tbus xrdp_encoder_event_processed;
-    tbus xrdp_encoder_term;
+    tbus xrdp_encoder_term_request;
+    tbus xrdp_encoder_term_done;
     struct fifo *fifo_to_proc;
     struct fifo *fifo_processed;
     tbus mutex;