From 1c84690c2f5ded39a8667fd05e66894f9f287b7b Mon Sep 17 00:00:00 2001 From: akallabeth Date: Tue, 21 Sep 2021 09:56:56 +0200 Subject: [PATCH] Fixes from tests (#7308) * Fixed memory leak in tls_verify_certificate * Fixed missing NULL checks * Fixed missing checks for FreeRDP_DeactivateClientDecoding * Added WINPR_ASSERT for client common new/free * Added /disable-output switch to deactivate client decoding Allows low resource remote connections that do not require visual feedback. (e.g. load testing/...) --- client/common/client.c | 16 +++++++-- client/common/cmdline.c | 4 +++ client/common/cmdline.h | 3 ++ libfreerdp/crypto/tls.c | 8 +++-- libfreerdp/gdi/gfx.c | 36 +++++++++++++++------ winpr/libwinpr/path/include/PathCchAppend.c | 12 ++++--- winpr/libwinpr/path/shell.c | 4 +-- 7 files changed, 61 insertions(+), 22 deletions(-) diff --git a/client/common/client.c b/client/common/client.c index 4dae6bd53..60a9973ea 100644 --- a/client/common/client.c +++ b/client/common/client.c @@ -38,13 +38,25 @@ static BOOL freerdp_client_common_new(freerdp* instance, rdpContext* context) { - RDP_CLIENT_ENTRY_POINTS* pEntryPoints = instance->pClientEntryPoints; + RDP_CLIENT_ENTRY_POINTS* pEntryPoints; + + WINPR_ASSERT(instance); + WINPR_ASSERT(context); + + pEntryPoints = instance->pClientEntryPoints; + WINPR_ASSERT(pEntryPoints); return IFCALLRESULT(TRUE, pEntryPoints->ClientNew, instance, context); } static void freerdp_client_common_free(freerdp* instance, rdpContext* context) { - RDP_CLIENT_ENTRY_POINTS* pEntryPoints = instance->pClientEntryPoints; + RDP_CLIENT_ENTRY_POINTS* pEntryPoints; + + WINPR_ASSERT(instance); + WINPR_ASSERT(context); + + pEntryPoints = instance->pClientEntryPoints; + WINPR_ASSERT(pEntryPoints); IFCALL(pEntryPoints->ClientFree, instance, context); } diff --git a/client/common/cmdline.c b/client/common/cmdline.c index 2eaa94e4e..2bf41d231 100644 --- a/client/common/cmdline.c +++ b/client/common/cmdline.c @@ -2400,6 +2400,10 @@ int freerdp_client_settings_parse_command_line_arguments(rdpSettings* settings, { settings->RedirectDrives = enable; } + CommandLineSwitchCase(arg, "disable-output") + { + freerdp_settings_set_bool(settings, FreeRDP_DeactivateClientDecoding, enable); + } CommandLineSwitchCase(arg, "home-drive") { settings->RedirectHomeDrive = enable; diff --git a/client/common/cmdline.h b/client/common/cmdline.h index 18825dd49..b79c8f9a7 100644 --- a/client/common/cmdline.h +++ b/client/common/cmdline.h @@ -348,6 +348,9 @@ static const COMMAND_LINE_ARGUMENT_A args[] = { "SPN authentication service class" }, { "ssh-agent", COMMAND_LINE_VALUE_FLAG, NULL, NULL, NULL, -1, "ssh-agent", "SSH Agent forwarding channel" }, + { "disable-output", COMMAND_LINE_VALUE_FLAG, NULL, NULL, NULL, -1, NULL, + "Deactivate all graphics decoding in the client session. Useful for load tests with many " + "simultaneous connections" }, { "t", COMMAND_LINE_VALUE_REQUIRED, "", NULL, NULL, -1, "title", "Window title" }, { "themes", COMMAND_LINE_VALUE_BOOL, NULL, BoolValueTrue, NULL, -1, NULL, "themes" }, { "timeout", COMMAND_LINE_VALUE_REQUIRED, "<time in ms>", "9000", NULL, -1, "timeout", diff --git a/libfreerdp/crypto/tls.c b/libfreerdp/crypto/tls.c index 53eb9c280..b9a3cdde4 100644 --- a/libfreerdp/crypto/tls.c +++ b/libfreerdp/crypto/tls.c @@ -1504,9 +1504,11 @@ int tls_verify_certificate(rdpTls* tls, CryptoCert cert, const char* hostname, U const char* old_issuer = certificate_data_get_issuer(stored_data); const char* old_fp = certificate_data_get_fingerprint(stored_data); const char* old_pem = certificate_data_get_pem(stored_data); + const BOOL fpIsAllocated = + !old_pem || !freerdp_settings_get_bool( + instance->settings, FreeRDP_CertificateCallbackPreferPEM); char* fp; - if (old_pem && freerdp_settings_get_bool(instance->settings, - FreeRDP_CertificateCallbackPreferPEM)) + if (!fpIsAllocated) { cflags |= VERIFY_CERT_FLAG_FP_IS_PEM; fp = pem; @@ -1519,7 +1521,7 @@ int tls_verify_certificate(rdpTls* tls, CryptoCert cert, const char* hostname, U accept_certificate = instance->VerifyChangedCertificateEx( instance, hostname, port, common_name, subject, issuer, pem, old_subject, old_issuer, old_fp, cflags); - if (!old_pem) + if (fpIsAllocated) free(fp); } #if defined(WITH_FREERDP_DEPRECATED) diff --git a/libfreerdp/gdi/gfx.c b/libfreerdp/gdi/gfx.c index b28e3337f..7dd3a792a 100644 --- a/libfreerdp/gdi/gfx.c +++ b/libfreerdp/gdi/gfx.c @@ -137,11 +137,14 @@ static UINT gdi_ResetGraphics(RdpgfxClientContext* context, free(pSurfaceIds); - if (!freerdp_client_codecs_reset(gdi->context->codecs, - freerdp_settings_get_codecs_flags(settings), gdi->width, - gdi->height)) + if (!freerdp_settings_get_bool(gdi->context->settings, FreeRDP_DeactivateClientDecoding)) { - goto fail; + if (!freerdp_client_codecs_reset(gdi->context->codecs, + freerdp_settings_get_codecs_flags(settings), gdi->width, + gdi->height)) + { + goto fail; + } } rc = CHANNEL_RC_OK; @@ -1070,21 +1073,28 @@ static UINT gdi_CreateSurface(RdpgfxClientContext* context, { UINT rc = ERROR_INTERNAL_ERROR; gdiGfxSurface* surface; + rdpGdi* gdi; WINPR_ASSERT(context); WINPR_ASSERT(createSurface); + gdi = (rdpGdi*)context->custom; + WINPR_ASSERT(gdi); + WINPR_ASSERT(gdi->context); EnterCriticalSection(&context->mux); surface = (gdiGfxSurface*)calloc(1, sizeof(gdiGfxSurface)); if (!surface) goto fail; - WINPR_ASSERT(context->codecs); - surface->codecs = context->codecs; - - if (!surface->codecs) + if (!freerdp_settings_get_bool(gdi->context->settings, FreeRDP_DeactivateClientDecoding)) { - free(surface); - goto fail; + WINPR_ASSERT(context->codecs); + surface->codecs = context->codecs; + + if (!surface->codecs) + { + free(surface); + goto fail; + } } surface->surfaceId = createSurface->surfaceId; @@ -1641,6 +1651,12 @@ BOOL gdi_graphics_pipeline_init_ex(rdpGdi* gdi, RdpgfxClientContext* gfx, gfx->UpdateSurfaces = NULL; gfx->SurfaceCommand = NULL; } + if (freerdp_settings_get_bool(settings, FreeRDP_DeactivateClientDecoding)) + { + gfx->UpdateSurfaceArea = NULL; + gfx->UpdateSurfaces = NULL; + gfx->SurfaceCommand = NULL; + } return TRUE; } diff --git a/winpr/libwinpr/path/include/PathCchAppend.c b/winpr/libwinpr/path/include/PathCchAppend.c index e54f093aa..2e7507b6d 100644 --- a/winpr/libwinpr/path/include/PathCchAppend.c +++ b/winpr/libwinpr/path/include/PathCchAppend.c @@ -65,8 +65,8 @@ HRESULT PATH_CCH_APPEND(PWSTR pszPath, size_t cchPath, PCWSTR pszMore) HRESULT PATH_CCH_APPEND(PSTR pszPath, size_t cchPath, PCSTR pszMore) { - BOOL pathBackslash; - BOOL moreBackslash; + BOOL pathBackslash = FALSE; + BOOL moreBackslash = FALSE; size_t pszMoreLength; size_t pszPathLength; @@ -79,11 +79,13 @@ HRESULT PATH_CCH_APPEND(PSTR pszPath, size_t cchPath, PCSTR pszMore) if (cchPath == 0 || cchPath > PATHCCH_MAX_CCH) return E_INVALIDARG; - pszMoreLength = lstrlenA(pszMore); pszPathLength = lstrlenA(pszPath); + if (pszPathLength > 0) + pathBackslash = (pszPath[pszPathLength - 1] == CUR_PATH_SEPARATOR_CHR) ? TRUE : FALSE; - pathBackslash = (pszPath[pszPathLength - 1] == CUR_PATH_SEPARATOR_CHR) ? TRUE : FALSE; - moreBackslash = (pszMore[0] == CUR_PATH_SEPARATOR_CHR) ? TRUE : FALSE; + pszMoreLength = lstrlenA(pszMore); + if (pszMoreLength > 0) + moreBackslash = (pszMore[0] == CUR_PATH_SEPARATOR_CHR) ? TRUE : FALSE; if (pathBackslash && moreBackslash) { diff --git a/winpr/libwinpr/path/shell.c b/winpr/libwinpr/path/shell.c index 6ba16d4cc..6e0cc00d6 100644 --- a/winpr/libwinpr/path/shell.c +++ b/winpr/libwinpr/path/shell.c @@ -416,8 +416,8 @@ char* GetCombinedPath(const char* basePath, const char* subPath) HRESULT status; char* path = NULL; char* subPathCpy = NULL; - int basePathLength = 0; - int subPathLength = 0; + size_t basePathLength = 0; + size_t subPathLength = 0; if (basePath) basePathLength = strlen(basePath);