Resize state machine: A fix and a question (#2929)

* Store EGFX state before entering resize state machine

At present the EGFX state is destroyed by states WMRZ_EGFX_DELETE_SURFACE
through WRMZ_EGFX_DELETE. This means that at WMRZ_EGFX_INITIALIZE we
cannot distinguish between EGFX not being ever used, and EGFX
having been torn down. Consequently, when running non-GFX, we don't
correctly recover the session.

* Allow multiple reasons for suppress_output

Replaces the single boolean for suppress_output with
a bitmask, to allow output to be suppressed for
more than one reason

* Disable output during resize

* Add states to dynamic resize

Adds states to the dynamic resize state machine so we wait for a
Deactivation-Reactivation sequence to finish before sending pointer
updates, etc.

* suppress module output during the dynamic resize

* Add support for dynamic resize to VNC backend

xrdp_mm needs to be informed when a resize has been performed so that
the resize stte machine can be updsate.

common/xrdp_client_info.h

@@ -199,7 +199,9 @@ struct xrdp_client_info
     int no_orders_supported;
     int use_cache_glyph_v2;
     int rail_enable;
-    int suppress_output;
+    // Mask of reasons why output may be suppressed
+    // (see enum suppress_output_reason)
+    unsigned int suppress_output_mask;
 
     int enable_token_login;
     char domain_user_separator[16];
@@ -227,6 +229,12 @@ enum xrdp_encoder_flags
     KEY_FRAME_REQUESTED                    = 1 << 3
 };
 
+/*
+ * Return true if output is suppressed for a particular reason
+ */
+#define OUTPUT_SUPPRESSED_FOR_REASON(ci,reason) \
+    (((ci)->suppress_output_mask & (unsigned int)reason) != 0)
+
 /* yyyymmdd of last incompatible change to xrdp_client_info */
 /* also used for changes to all the xrdp installed headers */
 #define CLIENT_INFO_CURRENT_VERSION 20230425

libxrdp/libxrdp.c

@@ -1181,6 +1181,12 @@ libxrdp_reset(struct xrdp_session *session,
         return 1;
     }
 
+    /*
+     * Stop output from the client during the deactivation-reactivation
+     * sequence [MS-RDPBCGR] 1.3.1.3 */
+    xrdp_rdp_suppress_output((struct xrdp_rdp *)session->rdp, 1,
+                             XSO_REASON_DEACTIVATE_REACTIVATE, 0, 0, 0, 0);
+
     /* shut down the rdp client
      *
      * When resetting the lib, disable application input checks, as

libxrdp/libxrdp.h

@@ -403,6 +403,20 @@ int
 xrdp_sec_process_mcs_data_monitors(struct xrdp_sec *self, struct stream *s);
 
 /* xrdp_rdp.c */
+
+/**
+ * Reasons why output is being suppressed or restarted
+ */
+enum suppress_output_reason
+{
+    /// Client has requested suppress via TS_SUPPRESS_OUTPUT_PDU
+    XSO_REASON_CLIENT_REQUEST = (1 << 0),
+    /// Deactivation-Reactivation Sequence [MS-RDPBCGR] 1.3.1.3
+    XSO_REASON_DEACTIVATE_REACTIVATE = (1 << 1),
+    /// Dynamic resize in progress
+    XSO_REASON_DYNAMIC_RESIZE = (1 << 2)
+};
+
 struct xrdp_rdp *
 xrdp_rdp_create(struct xrdp_session *session, struct trans *trans);
 void
@@ -438,6 +452,21 @@ xrdp_rdp_send_deactivate(struct xrdp_rdp *self);
 int
 xrdp_rdp_send_session_info(struct xrdp_rdp *self, const char *data,
                            int data_bytes);
+/**
+ * Request output suppress or resume
+ *
+ * @param self RDP struct
+ * @param suppress (!= 0 for suppress, 0 for resume)
+ * @param reason Why the output is being suppressed or resumed
+ * @param left Left pixel of repaint area (ignored for suppress)
+ * @param top Top pixel of repaint area (ignored for suppress)
+ * @param right Right pixel of inclusive repaint area (ignored for suppress)
+ * @param bottom Bottom pixel of inclusive repaint area (ignored for suppress)
+ */
+void
+xrdp_rdp_suppress_output(struct xrdp_rdp *self, int suppress,
+                         enum suppress_output_reason reason,
+                         int left, int top, int right, int bottom);
 
 /* xrdp_orders.c */
 struct xrdp_orders *

libxrdp/xrdp_rdp.c

@@ -1292,6 +1292,20 @@ xrdp_rdp_process_data_font(struct xrdp_rdp *self, struct stream *s)
         self->session->up_and_running = 1;
         LOG_DEVEL(LOG_LEVEL_INFO, "yeah, up_and_running");
         xrdp_rdp_send_data_update_sync(self);
+
+        /* This is also the end of an Deactivation-reactivation
+         * sequence [MS-RDPBCGR] 1.3.1.3 */
+        xrdp_rdp_suppress_output(self, 0, XSO_REASON_DEACTIVATE_REACTIVATE,
+                                 0, 0,
+                                 self->client_info.display_sizes.session_width,
+                                 self->client_info.display_sizes.session_height);
+
+        if (self->session->callback != 0)
+        {
+            /* call to xrdp_wm.c : callback */
+            self->session->callback(self->session->id, 0x555a, 0, 0,
+                                    0, 0);
+        }
         xrdp_channel_drdynvc_start(self->sec_layer->chan_layer);
     }
     else
@@ -1401,69 +1415,83 @@ xrdp_rdp_process_frame_ack(struct xrdp_rdp *self, struct stream *s)
     return 0;
 }
 
+/*****************************************************************************/
+void
+xrdp_rdp_suppress_output(struct xrdp_rdp *self, int suppress,
+                         enum suppress_output_reason reason,
+                         int left, int top, int right, int bottom)
+{
+    int old_suppress = self->client_info.suppress_output_mask != 0;
+    if (suppress)
+    {
+        self->client_info.suppress_output_mask |= (unsigned int)reason;
+    }
+    else
+    {
+        self->client_info.suppress_output_mask &= ~(unsigned int)reason;
+    }
+
+    int current_suppress =  self->client_info.suppress_output_mask != 0;
+    if (current_suppress != old_suppress && self->session->callback != 0)
+    {
+        self->session->callback(self->session->id, 0x5559, suppress,
+                                MAKELONG(left, top),
+                                MAKELONG(right, bottom), 0);
+    }
+}
+
 /*****************************************************************************/
 /* Process a [MS-RDPBCGR] TS_SUPPRESS_OUTPUT_PDU message */
 static int
 xrdp_rdp_process_suppress(struct xrdp_rdp *self, struct stream *s)
 {
+    int rv = 1;
     int allowDisplayUpdates;
     int left;
     int top;
     int right;
     int bottom;
 
-    if (!s_check_rem_and_log(s, 1, "Parsing [MS-RDPBCGR] TS_SUPPRESS_OUTPUT_PDU"))
+    if (s_check_rem_and_log(s, 1, "Parsing [MS-RDPBCGR] TS_SUPPRESS_OUTPUT_PDU"))
     {
-        return 1;
+        in_uint8(s, allowDisplayUpdates);
+        LOG_DEVEL(LOG_LEVEL_TRACE,
+                  "Received [MS-RDPBCGR] TS_SUPPRESS_OUTPUT_PDU "
+                  "allowDisplayUpdates %d", allowDisplayUpdates);
+        switch (allowDisplayUpdates)
+        {
+            case 0: /* SUPPRESS_DISPLAY_UPDATES */
+                LOG_DEVEL(LOG_LEVEL_DEBUG,
+                          "Client requested display output to be suppressed");
+                xrdp_rdp_suppress_output(self, 1,
+                                         XSO_REASON_CLIENT_REQUEST,
+                                         0, 0, 0, 0);
+                rv = 0;
+                break;
+            case 1: /* ALLOW_DISPLAY_UPDATES */
+                LOG_DEVEL(LOG_LEVEL_DEBUG,
+                          "Client requested display output to be enabled");
+                if (s_check_rem_and_log(s, 11,
+                                        "Parsing [MS-RDPBCGR] Padding and TS_RECTANGLE16"))
+                {
+                    in_uint8s(s, 3); /* pad */
+                    in_uint16_le(s, left);
+                    in_uint16_le(s, top);
+                    in_uint16_le(s, right);
+                    in_uint16_le(s, bottom);
+                    LOG_DEVEL(LOG_LEVEL_TRACE,
+                              "Received [MS-RDPBCGR] TS_RECTANGLE16 "
+                              "left %d, top %d, right %d, bottom %d",
+                              left, top, right, bottom);
+                    xrdp_rdp_suppress_output(self, 0,
+                                             XSO_REASON_CLIENT_REQUEST,
+                                             left, top, right, bottom);
+                    rv = 0;
+                }
+                break;
+        }
     }
-    in_uint8(s, allowDisplayUpdates);
+    return rv;
-    LOG_DEVEL(LOG_LEVEL_TRACE, "Received [MS-RDPBCGR] TS_SUPPRESS_OUTPUT_PDU "
-              "allowDisplayUpdates %d", allowDisplayUpdates);
-    switch (allowDisplayUpdates)
-    {
-        case 0: /* SUPPRESS_DISPLAY_UPDATES */
-            self->client_info.suppress_output = 1;
-            LOG_DEVEL(LOG_LEVEL_DEBUG, "Client requested display output to be suppressed");
-            if (self->session->callback != 0)
-            {
-                self->session->callback(self->session->id, 0x5559, 1,
-                                        0, 0, 0);
-            }
-            else
-            {
-                LOG_DEVEL(LOG_LEVEL_WARNING,
-                          "Bug: no callback registered for xrdp_rdp_process_suppress");
-            }
-            break;
-        case 1: /* ALLOW_DISPLAY_UPDATES */
-            self->client_info.suppress_output = 0;
-            LOG_DEVEL(LOG_LEVEL_DEBUG, "Client requested display output to be enabled");
-            if (!s_check_rem_and_log(s, 11, "Parsing [MS-RDPBCGR] Padding and TS_RECTANGLE16"))
-            {
-                return 1;
-            }
-            in_uint8s(s, 3); /* pad */
-            in_uint16_le(s, left);
-            in_uint16_le(s, top);
-            in_uint16_le(s, right);
-            in_uint16_le(s, bottom);
-            LOG_DEVEL(LOG_LEVEL_TRACE, "Received [MS-RDPBCGR] TS_RECTANGLE16 "
-                      "left %d, top %d, right %d, bottom %d",
-                      left, top, right, bottom);
-            if (self->session->callback != 0)
-            {
-                self->session->callback(self->session->id, 0x5559, 0,
-                                        MAKELONG(left, top),
-                                        MAKELONG(right, bottom), 0);
-            }
-            else
-            {
-                LOG_DEVEL(LOG_LEVEL_WARNING,
-                          "Bug: no callback registered for xrdp_rdp_process_suppress");
-            }
-            break;
-    }
-    return 0;
 }
 
 /*****************************************************************************/

vnc/vnc.c

@@ -1249,6 +1249,12 @@ lib_framebuffer_waiting_for_resize_confirm(struct vnc *v)
             {
                 LOG(LOG_LEVEL_DEBUG, "VNC server successfully resized");
                 log_screen_layout(LOG_LEVEL_INFO, "NewLayout", &layout);
+                // If this resize was requested by the client mid-session
+                // (dynamic resize), we need to tell xrdp_mm that
+                // it's OK to continue with the resize state machine.
+                // We do this by sending a reset with bpp == 0
+                error = v->server_reset(v, v->server_width,
+                                        v->server_height, 0);
             }
             else
             {

xrdp/xrdp.h

@@ -471,6 +471,9 @@ struct display_control_monitor_layout_data
     enum display_resize_state state;
     int last_state_update_timestamp;
     int start_time;
+    /// This flag is set if the state machine needs to
+    /// shutdown/startup EGFX
+    int using_egfx;
 };
 
 int
@@ -478,6 +481,8 @@ xrdp_mm_drdynvc_up(struct xrdp_mm *self);
 int
 xrdp_mm_suppress_output(struct xrdp_mm *self, int suppress,
                         int left, int top, int right, int bottom);
+int
+xrdp_mm_up_and_running(struct xrdp_mm *self);
 struct xrdp_mm *
 xrdp_mm_create(struct xrdp_wm *owner);
 void

xrdp/xrdp_mm.c

@@ -1541,10 +1541,11 @@ dynamic_monitor_data(intptr_t id, int chan_id, char *data, int bytes)
     pro = (struct xrdp_process *) id;
     wm = pro->wm;
 
-    if (wm->client_info->suppress_output == 1)
+    if (OUTPUT_SUPPRESSED_FOR_REASON(wm->client_info,
+                                     XSO_REASON_CLIENT_REQUEST))
     {
         LOG(LOG_LEVEL_INFO, "dynamic_monitor_data: Not allowing resize."
-            " Suppress output is active.");
+            " Suppress output requested by client");
         return error;
     }
 
@@ -1664,13 +1665,18 @@ process_display_control_monitor_layout_data(struct xrdp_wm *wm)
     switch (description->state)
     {
         case WMRZ_ENCODER_DELETE:
+            // Stop any output from the module
+            rdp = (struct xrdp_rdp *) (wm->session->rdp);
+            xrdp_rdp_suppress_output(rdp,
+                                     1, XSO_REASON_DYNAMIC_RESIZE,
+                                     0, 0, 0, 0);
             // Disable the encoder until the resize is complete.
             if (mm->encoder != NULL)
             {
                 xrdp_encoder_delete(mm->encoder);
                 mm->encoder = NULL;
             }
-            if (mm->egfx == 0)
+            if (mm->resize_data->using_egfx == 0)
             {
                 advance_resize_state_machine(mm, WMRZ_SERVER_MONITOR_RESIZE);
             }
@@ -1695,7 +1701,7 @@ process_display_control_monitor_layout_data(struct xrdp_wm *wm)
             break;
         // Also processed in xrdp_egfx_close_response
         case WMRZ_EGFX_CONN_CLOSING:
-            rdp = (struct xrdp_rdp *) (mm->wm->session->rdp);
+            rdp = (struct xrdp_rdp *) (wm->session->rdp);
             sec = rdp->sec_layer;
             chan = sec->chan_layer;
 
@@ -1750,9 +1756,9 @@ process_display_control_monitor_layout_data(struct xrdp_wm *wm)
         // Not processed here. Processed in server_reset
         // case WMRZ_SERVER_MONITOR_MESSAGE_PROCESSING:
         case WMRZ_SERVER_MONITOR_MESSAGE_PROCESSED:
-            advance_resize_state_machine(mm, WMRZ_XRDP_CORE_RESIZE);
+            advance_resize_state_machine(mm, WMRZ_XRDP_CORE_RESET);
             break;
-        case WMRZ_XRDP_CORE_RESIZE:
+        case WMRZ_XRDP_CORE_RESET:
             // TODO: Unify this logic with server_reset
             error = libxrdp_reset(
                         wm->session, desc_width, desc_height, wm->screen->bpp);
@@ -1772,6 +1778,12 @@ process_display_control_monitor_layout_data(struct xrdp_wm *wm)
                           " xrdp_cache_reset failed %d", error);
                 return advance_error(error, mm);
             }
+            advance_resize_state_machine(mm, WMRZ_XRDP_CORE_RESET_PROCESSING);
+            break;
+
+        // Not processed here. Processed in xrdp_mm_up_and_running()
+        // case WMRZ_XRDP_CORE_RESET_PROCESSING:
+        case WMRZ_XRDP_CORE_RESET_PROCESSED:
             /* load some stuff */
             error = xrdp_wm_load_static_colors_plus(wm, 0);
             if (error != 0)
@@ -1804,7 +1816,7 @@ process_display_control_monitor_layout_data(struct xrdp_wm *wm)
             advance_resize_state_machine(mm, WMRZ_EGFX_INITIALIZE);
             break;
         case WMRZ_EGFX_INITIALIZE:
-            if (error == 0 && mm->egfx == NULL && mm->egfx_up == 0)
+            if (mm->resize_data->using_egfx)
             {
                 egfx_initialize(mm);
                 advance_resize_state_machine(mm, WMRZ_EGFX_INITALIZING);
@@ -1827,6 +1839,16 @@ process_display_control_monitor_layout_data(struct xrdp_wm *wm)
             advance_resize_state_machine(mm, WMRZ_SERVER_INVALIDATE);
             break;
         case WMRZ_SERVER_INVALIDATE:
+            if (module != 0)
+            {
+                // Ack all frames to speed up resize.
+                module->mod_frame_ack(module, 0, INT_MAX);
+            }
+            // Restart module output
+            rdp = (struct xrdp_rdp *) (wm->session->rdp);
+            xrdp_rdp_suppress_output(rdp,
+                                     0, XSO_REASON_DYNAMIC_RESIZE,
+                                     0, 0, desc_width, desc_height);
             /* redraw */
             error = xrdp_bitmap_invalidate(wm->screen, 0);
             if (error != 0)
@@ -1836,11 +1858,6 @@ process_display_control_monitor_layout_data(struct xrdp_wm *wm)
                           " xrdp_bitmap_invalidate failed %d", error);
                 return advance_error(error, mm);
             }
-            if (module != 0)
-            {
-                // Ack all frames to speed up resize.
-                module->mod_frame_ack(module, 0, INT_MAX);
-            }
             advance_resize_state_machine(mm, WMRZ_COMPLETE);
             break;
         default:
@@ -1920,6 +1937,7 @@ dynamic_monitor_process_queue(struct xrdp_mm *self)
             const int time = g_time3();
             self->resize_data->start_time = time;
             self->resize_data->last_state_update_timestamp = time;
+            self->resize_data->using_egfx = (self->egfx != NULL);
             advance_resize_state_machine(self, WMRZ_ENCODER_DELETE);
         }
         else
@@ -2067,6 +2085,21 @@ xrdp_mm_suppress_output(struct xrdp_mm *self, int suppress,
     return 0;
 }
 
+/******************************************************************************/
+int
+xrdp_mm_up_and_running(struct xrdp_mm *self)
+{
+    LOG_DEVEL(LOG_LEVEL_DEBUG, "xrdp_mm_up_and_running:");
+    if (self->resize_data != NULL &&
+            self->resize_data->state == WMRZ_XRDP_CORE_RESET_PROCESSING)
+    {
+        LOG(LOG_LEVEL_INFO,
+            "xrdp_mm_up_and_running: Core reset done.");
+        advance_resize_state_machine(self, WMRZ_XRDP_CORE_RESET_PROCESSED);
+    }
+    return 0;
+}
+
 /*****************************************************************************/
 /* open response from client going to channel server */
 static int

xrdp/xrdp_types.h

@@ -347,7 +347,9 @@ enum display_resize_state
     WMRZ_SERVER_VERSION_MESSAGE_START,
     WMRZ_SERVER_MONITOR_MESSAGE_PROCESSING,
     WMRZ_SERVER_MONITOR_MESSAGE_PROCESSED,
-    WMRZ_XRDP_CORE_RESIZE,
+    WMRZ_XRDP_CORE_RESET,
+    WMRZ_XRDP_CORE_RESET_PROCESSING,
+    WMRZ_XRDP_CORE_RESET_PROCESSED,
     WMRZ_EGFX_INITIALIZE,
     WMRZ_EGFX_INITALIZING,
     WMRZ_EGFX_INITIALIZED,
@@ -371,7 +373,11 @@ enum display_resize_state
      "WMRZ_SERVER_MONITOR_MESSAGE_PROCESSING" : \
      (status) == WMRZ_SERVER_MONITOR_MESSAGE_PROCESSED ? \
      "WMRZ_SERVER_MONITOR_MESSAGE_PROCESSED" : \
-     (status) == WMRZ_XRDP_CORE_RESIZE ? "WMRZ_XRDP_CORE_RESIZE" : \
+     (status) == WMRZ_XRDP_CORE_RESET ? "WMRZ_XRDP_CORE_RESET" : \
+     (status) == WMRZ_XRDP_CORE_RESET_PROCESSING ? \
+     "WMRZ_XRDP_CORE_RESET_PROCESSING" : \
+     (status) == WMRZ_XRDP_CORE_RESET_PROCESSED ? \
+     "WMRZ_XRDP_CORE_RESET_PROCESSED" : \
      (status) == WMRZ_EGFX_INITIALIZE ? "WMRZ_EGFX_INITIALIZE" : \
      (status) == WMRZ_EGFX_INITALIZING ? "WMRZ_EGFX_INITALIZING" : \
      (status) == WMRZ_EGFX_INITIALIZED ? "WMRZ_EGFX_INITIALIZED" : \

xrdp/xrdp_wm.c

@@ -2029,6 +2029,9 @@ callback(intptr_t id, int msg, intptr_t param1, intptr_t param2,
             xrdp_mm_suppress_output(wm->mm, param1,
                                     LOWORD(param2), HIWORD(param2),
                                     LOWORD(param3), HIWORD(param3));
+        case 0x555a:
+            // "yeah, up_and_running"
+            xrdp_mm_up_and_running(wm->mm);
             break;
     }
     return rv;