From 9753559726dde2affe4719eece4ffc771676d293 Mon Sep 17 00:00:00 2001 From: matt335672 <30179339+matt335672@users.noreply.github.com> Date: Wed, 31 Jan 2024 22:14:56 +0000 Subject: [PATCH] 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 | 10 ++- libxrdp/libxrdp.c | 6 ++ libxrdp/libxrdp.h | 29 +++++++++ libxrdp/xrdp_rdp.c | 128 +++++++++++++++++++++++--------------- vnc/vnc.c | 6 ++ xrdp/xrdp.h | 5 ++ xrdp/xrdp_mm.c | 57 +++++++++++++---- xrdp/xrdp_types.h | 10 ++- xrdp/xrdp_wm.c | 3 + 9 files changed, 189 insertions(+), 65 deletions(-) diff --git a/common/xrdp_client_info.h b/common/xrdp_client_info.h index 3acb13bfd6..03dd5e3c70 100644 --- a/common/xrdp_client_info.h +++ b/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 diff --git a/libxrdp/libxrdp.c b/libxrdp/libxrdp.c index 358a441868..af1714de87 100644 --- a/libxrdp/libxrdp.c +++ b/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 diff --git a/libxrdp/libxrdp.h b/libxrdp/libxrdp.h index 4b7c906cb7..b9d1b8f94b 100644 --- a/libxrdp/libxrdp.h +++ b/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 * diff --git a/libxrdp/xrdp_rdp.c b/libxrdp/xrdp_rdp.c index f9196d4b18..29901db610 100644 --- a/libxrdp/xrdp_rdp.c +++ b/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")) - { - return 1; - } - in_uint8(s, allowDisplayUpdates); - LOG_DEVEL(LOG_LEVEL_TRACE, "Received [MS-RDPBCGR] TS_SUPPRESS_OUTPUT_PDU " - "allowDisplayUpdates %d", allowDisplayUpdates); - switch (allowDisplayUpdates) + if (s_check_rem_and_log(s, 1, "Parsing [MS-RDPBCGR] TS_SUPPRESS_OUTPUT_PDU")) { - 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; + 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; + } } - return 0; + return rv; } /*****************************************************************************/ diff --git a/vnc/vnc.c b/vnc/vnc.c index 6a72834a6d..77845e2f4d 100644 --- a/vnc/vnc.c +++ b/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 { diff --git a/xrdp/xrdp.h b/xrdp/xrdp.h index 00843fa99f..4fc8a808df 100644 --- a/xrdp/xrdp.h +++ b/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 diff --git a/xrdp/xrdp_mm.c b/xrdp/xrdp_mm.c index 6d6279f1ef..0203b0bf60 100644 --- a/xrdp/xrdp_mm.c +++ b/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 diff --git a/xrdp/xrdp_types.h b/xrdp/xrdp_types.h index 06ccf4469b..a066980fdd 100644 --- a/xrdp/xrdp_types.h +++ b/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" : \ diff --git a/xrdp/xrdp_wm.c b/xrdp/xrdp_wm.c index 8d0a48c317..fcf59e5550 100644 --- a/xrdp/xrdp_wm.c +++ b/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;