From 0c7e6c3c2dae3a3c75edfa56bd2e288a9222c727 Mon Sep 17 00:00:00 2001 From: Armin Novak Date: Tue, 22 Feb 2022 10:54:42 +0100 Subject: [PATCH] Refactored disp channel * Assert all input parameters * Use DISP_CHANNEL_NAME define for channel name --- channels/disp/client/disp_main.c | 51 ++++++++++++++++--- channels/disp/server/disp_main.c | 87 +++++++++++++++++++++++--------- client/common/cmdline.c | 3 +- include/freerdp/channels/disp.h | 2 + 4 files changed, 111 insertions(+), 32 deletions(-) diff --git a/channels/disp/client/disp_main.c b/channels/disp/client/disp_main.c index 41b2d0e55..66b80f261 100644 --- a/channels/disp/client/disp_main.c +++ b/channels/disp/client/disp_main.c @@ -29,6 +29,7 @@ #include #include +#include #include #include #include @@ -87,8 +88,14 @@ disp_send_display_control_monitor_layout_pdu(DISP_CHANNEL_CALLBACK* callback, UI UINT32 index; DISP_PLUGIN* disp; UINT32 MonitorLayoutSize; - DISPLAY_CONTROL_HEADER header; + DISPLAY_CONTROL_HEADER header = { 0 }; + + WINPR_ASSERT(callback); + WINPR_ASSERT(Monitors || (NumMonitors == 0)); + disp = (DISP_PLUGIN*)callback->plugin; + WINPR_ASSERT(disp); + MonitorLayoutSize = DISPLAY_CONTROL_MONITOR_LAYOUT_SIZE; header.length = 8 + 8 + (NumMonitors * MonitorLayoutSize); header.type = DISPLAY_CONTROL_PDU_TYPE_MONITOR_LAYOUT; @@ -172,8 +179,15 @@ static UINT disp_recv_display_control_caps_pdu(DISP_CHANNEL_CALLBACK* callback, DISP_PLUGIN* disp; DispClientContext* context; UINT ret = CHANNEL_RC_OK; + + WINPR_ASSERT(callback); + WINPR_ASSERT(s); + disp = (DISP_PLUGIN*)callback->plugin; + WINPR_ASSERT(disp); + context = (DispClientContext*)disp->iface.pInterface; + WINPR_ASSERT(context); if (Stream_GetRemainingLength(s) < 12) { @@ -200,7 +214,10 @@ static UINT disp_recv_display_control_caps_pdu(DISP_CHANNEL_CALLBACK* callback, static UINT disp_recv_pdu(DISP_CHANNEL_CALLBACK* callback, wStream* s) { UINT32 error; - DISPLAY_CONTROL_HEADER header; + DISPLAY_CONTROL_HEADER header = { 0 }; + + WINPR_ASSERT(callback); + WINPR_ASSERT(s); if (Stream_GetRemainingLength(s) < 8) { @@ -264,6 +281,13 @@ static UINT disp_on_new_channel_connection(IWTSListenerCallback* pListenerCallba { DISP_CHANNEL_CALLBACK* callback; DISP_LISTENER_CALLBACK* listener_callback = (DISP_LISTENER_CALLBACK*)pListenerCallback; + + WINPR_ASSERT(listener_callback); + WINPR_ASSERT(pChannel); + WINPR_ASSERT(Data); + WINPR_ASSERT(pbAccept); + WINPR_ASSERT(ppCallback); + callback = (DISP_CHANNEL_CALLBACK*)calloc(1, sizeof(DISP_CHANNEL_CALLBACK)); if (!callback) @@ -291,6 +315,10 @@ static UINT disp_plugin_initialize(IWTSPlugin* pPlugin, IWTSVirtualChannelManage { UINT status; DISP_PLUGIN* disp = (DISP_PLUGIN*)pPlugin; + + WINPR_ASSERT(disp); + WINPR_ASSERT(pChannelMgr); + if (disp->initialized) { WLog_ERR(TAG, "[%s] channel initialized twice, aborting", DISP_DVC_CHANNEL_NAME); @@ -349,8 +377,16 @@ static UINT disp_plugin_terminated(IWTSPlugin* pPlugin) static UINT disp_send_monitor_layout(DispClientContext* context, UINT32 NumMonitors, DISPLAY_CONTROL_MONITOR_LAYOUT* Monitors) { - DISP_PLUGIN* disp = (DISP_PLUGIN*)context->handle; - DISP_CHANNEL_CALLBACK* callback = disp->listener_callback->channel_callback; + DISP_PLUGIN* disp; + DISP_CHANNEL_CALLBACK* callback; + + WINPR_ASSERT(context); + + disp = (DISP_PLUGIN*)context->handle; + WINPR_ASSERT(disp); + + callback = disp->listener_callback->channel_callback; + return disp_send_display_control_monitor_layout_pdu(callback, NumMonitors, Monitors); } @@ -370,7 +406,10 @@ UINT DVCPluginEntry(IDRDYNVC_ENTRY_POINTS* pEntryPoints) UINT error = CHANNEL_RC_OK; DISP_PLUGIN* disp; DispClientContext* context; - disp = (DISP_PLUGIN*)pEntryPoints->GetPlugin(pEntryPoints, "disp"); + + WINPR_ASSERT(pEntryPoints); + + disp = (DISP_PLUGIN*)pEntryPoints->GetPlugin(pEntryPoints, DISP_CHANNEL_NAME); if (!disp) { @@ -401,7 +440,7 @@ UINT DVCPluginEntry(IDRDYNVC_ENTRY_POINTS* pEntryPoints) context->handle = (void*)disp; context->SendMonitorLayout = disp_send_monitor_layout; disp->iface.pInterface = (void*)context; - error = pEntryPoints->RegisterPlugin(pEntryPoints, "disp", &disp->iface); + error = pEntryPoints->RegisterPlugin(pEntryPoints, DISP_CHANNEL_NAME, &disp->iface); } else { diff --git a/channels/disp/server/disp_main.c b/channels/disp/server/disp_main.c index 11ab155aa..3766e1fc3 100644 --- a/channels/disp/server/disp_main.c +++ b/channels/disp/server/disp_main.c @@ -27,6 +27,7 @@ #include #include +#include #include #include #include @@ -90,8 +91,10 @@ static void disp_server_sanitize_monitor_layout(DISPLAY_CONTROL_MONITOR_LAYOUT* } } -static BOOL disp_server_is_monitor_layout_valid(DISPLAY_CONTROL_MONITOR_LAYOUT* monitor) +static BOOL disp_server_is_monitor_layout_valid(const DISPLAY_CONTROL_MONITOR_LAYOUT* monitor) { + WINPR_ASSERT(monitor); + if (monitor->Width < DISPLAY_CONTROL_MIN_MONITOR_WIDTH || monitor->Width > DISPLAY_CONTROL_MAX_MONITOR_WIDTH) { @@ -127,8 +130,10 @@ static UINT disp_recv_display_control_monitor_layout_pdu(wStream* s, DispServerC { UINT32 error = CHANNEL_RC_OK; UINT32 index; - DISPLAY_CONTROL_MONITOR_LAYOUT_PDU pdu; - DISPLAY_CONTROL_MONITOR_LAYOUT* monitor; + DISPLAY_CONTROL_MONITOR_LAYOUT_PDU pdu = { 0 }; + + WINPR_ASSERT(s); + WINPR_ASSERT(context); if (Stream_GetRemainingLength(s) < 8) { @@ -174,7 +179,8 @@ static UINT disp_recv_display_control_monitor_layout_pdu(wStream* s, DispServerC for (index = 0; index < pdu.NumMonitors; index++) { - monitor = &(pdu.Monitors[index]); + DISPLAY_CONTROL_MONITOR_LAYOUT* monitor = &(pdu.Monitors[index]); + Stream_Read_UINT32(s, monitor->Flags); /* Flags (4 bytes) */ Stream_Read_UINT32(s, monitor->Left); /* Left (4 bytes) */ Stream_Read_UINT32(s, monitor->Top); /* Top (4 bytes) */ @@ -216,7 +222,11 @@ static UINT disp_server_receive_pdu(DispServerContext* context, wStream* s) { UINT error = CHANNEL_RC_OK; size_t beg, end; - DISPLAY_CONTROL_HEADER header; + DISPLAY_CONTROL_HEADER header = { 0 }; + + WINPR_ASSERT(s); + WINPR_ASSERT(context); + beg = Stream_GetPosition(s); if ((error = disp_read_header(s, &header))) @@ -259,8 +269,16 @@ static UINT disp_server_handle_messages(DispServerContext* context) DWORD BytesReturned; void* buffer; UINT ret = CHANNEL_RC_OK; - DispServerPrivate* priv = context->priv; - wStream* s = priv->input_stream; + DispServerPrivate* priv; + wStream* s; + + WINPR_ASSERT(context); + + priv = context->priv; + WINPR_ASSERT(priv); + + s = priv->input_stream; + WINPR_ASSERT(s); /* Check whether the dynamic channel is ready */ if (!priv->isReady) @@ -328,12 +346,17 @@ static UINT disp_server_handle_messages(DispServerContext* context) static DWORD WINAPI disp_server_thread_func(LPVOID arg) { DispServerContext* context = (DispServerContext*)arg; - DispServerPrivate* priv = context->priv; + DispServerPrivate* priv; DWORD status; - DWORD nCount; - HANDLE events[8]; + DWORD nCount = 0; + HANDLE events[8] = { 0 }; UINT error = CHANNEL_RC_OK; - nCount = 0; + + WINPR_ASSERT(context); + + priv = context->priv; + WINPR_ASSERT(priv); + events[nCount++] = priv->stopEvent; events[nCount++] = priv->channelEvent; @@ -372,11 +395,16 @@ static DWORD WINAPI disp_server_thread_func(LPVOID arg) static UINT disp_server_open(DispServerContext* context) { UINT rc = ERROR_INTERNAL_ERROR; - DispServerPrivate* priv = context->priv; + DispServerPrivate* priv; DWORD BytesReturned = 0; PULONG pSessionId = NULL; - void* buffer; - buffer = NULL; + void* buffer = NULL; + + WINPR_ASSERT(context); + + priv = context->priv; + WINPR_ASSERT(priv); + priv->SessionId = WTS_CURRENT_SESSION; if (WTSQuerySessionInformationA(context->vcm, WTS_CURRENT_SESSION, WTSSessionId, @@ -450,6 +478,9 @@ static UINT disp_server_packet_send(DispServerContext* context, wStream* s) UINT ret; ULONG written; + WINPR_ASSERT(context); + WINPR_ASSERT(s); + if (!WTSVirtualChannelWrite(context->priv->disp_channel, (PCHAR)Stream_Buffer(s), Stream_GetPosition(s), &written)) { @@ -477,7 +508,11 @@ out: */ static UINT disp_server_send_caps_pdu(DispServerContext* context) { - wStream* s = disp_server_single_packet_new(DISPLAY_CONTROL_PDU_TYPE_CAPS, 12); + wStream* s; + + WINPR_ASSERT(context); + + s = disp_server_single_packet_new(DISPLAY_CONTROL_PDU_TYPE_CAPS, 12); if (!s) { @@ -499,7 +534,12 @@ static UINT disp_server_send_caps_pdu(DispServerContext* context) static UINT disp_server_close(DispServerContext* context) { UINT error = CHANNEL_RC_OK; - DispServerPrivate* priv = context->priv; + DispServerPrivate* priv; + + WINPR_ASSERT(context); + + priv = context->priv; + WINPR_ASSERT(priv); if (priv->thread) { @@ -536,7 +576,7 @@ DispServerContext* disp_server_context_new(HANDLE vcm) if (!context) { WLog_ERR(TAG, "disp_server_context_new(): calloc DispServerContext failed!"); - goto out_free; + goto fail; } priv = context->priv = (DispServerPrivate*)calloc(1, sizeof(DispServerPrivate)); @@ -544,7 +584,7 @@ DispServerContext* disp_server_context_new(HANDLE vcm) if (!context->priv) { WLog_ERR(TAG, "disp_server_context_new(): calloc DispServerPrivate failed!"); - goto out_free; + goto fail; } priv->input_stream = Stream_New(NULL, 4); @@ -552,7 +592,7 @@ DispServerContext* disp_server_context_new(HANDLE vcm) if (!priv->input_stream) { WLog_ERR(TAG, "Stream_New failed!"); - goto out_free_priv; + goto fail; } context->vcm = vcm; @@ -561,10 +601,8 @@ DispServerContext* disp_server_context_new(HANDLE vcm) context->DisplayControlCaps = disp_server_send_caps_pdu; priv->isReady = FALSE; return context; -out_free_priv: - free(context->priv); -out_free: - free(context); +fail: + disp_server_context_free(context); return NULL; } @@ -573,10 +611,9 @@ void disp_server_context_free(DispServerContext* context) if (!context) return; - disp_server_close(context); - if (context->priv) { + disp_server_close(context); Stream_Free(context->priv->input_stream, TRUE); free(context->priv); } diff --git a/client/common/cmdline.c b/client/common/cmdline.c index a61be5bfe..5a9a111c8 100644 --- a/client/common/cmdline.c +++ b/client/common/cmdline.c @@ -40,6 +40,7 @@ #include #include #include +#include #include #include #include @@ -3841,7 +3842,7 @@ BOOL freerdp_client_load_addins(rdpChannels* channels, rdpSettings* settings) if (settings->SupportDisplayControl) { - const char* p[] = { "disp" }; + const char* p[] = { DISP_CHANNEL_NAME }; if (!freerdp_client_add_dynamic_channel(settings, ARRAYSIZE(p), p)) return FALSE; diff --git a/include/freerdp/channels/disp.h b/include/freerdp/channels/disp.h index 61d85ea47..f95c4fceb 100644 --- a/include/freerdp/channels/disp.h +++ b/include/freerdp/channels/disp.h @@ -27,6 +27,8 @@ #define DISPLAY_CONTROL_PDU_TYPE_MONITOR_LAYOUT 0x00000002 #define DISPLAY_CONTROL_MONITOR_LAYOUT_SIZE 40 +#define DISP_CHANNEL_NAME "disp" + #define DISP_DVC_CHANNEL_NAME "Microsoft::Windows::RDS::DisplayControl" #define ORIENTATION_LANDSCAPE 0 #define ORIENTATION_PORTRAIT 90