From caf6e7f2ecd2d71cd70719956f7d60bcacb1701b Mon Sep 17 00:00:00 2001 From: Armin Novak Date: Tue, 17 Feb 2026 08:17:33 +0100 Subject: [PATCH 1/4] [codec,nsc] log decoder function parameter issues --- libfreerdp/codec/nsc.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/libfreerdp/codec/nsc.c b/libfreerdp/codec/nsc.c index 4d677b5b6..ec05db866 100644 --- a/libfreerdp/codec/nsc.c +++ b/libfreerdp/codec/nsc.c @@ -439,12 +439,24 @@ BOOL nsc_process_message(NSC_CONTEXT* WINPR_RESTRICT context, UINT16 bpp, UINT32 wStream sbuffer = { 0 }; BOOL ret = 0; if (!context || !data || !pDstData) + { + WLog_ERR(TAG, "Invalid argument: context=%p, data=%p, pDstData=%p", (void*)context, + (const void*)data, (void*)pDstData); return FALSE; + } if (nXDst > nWidth) + { + WLog_Print(context->priv->log, WLOG_ERROR, "nXDst %" PRIu32 " > nWidth %" PRIu32, nXDst, + nWidth); return FALSE; + } if (nYDst > nHeight) + { + WLog_Print(context->priv->log, WLOG_ERROR, "nYDst %" PRIu32 " > nHeight %" PRIu32, nYDst, + nHeight); return FALSE; + } wStream* s = Stream_StaticConstInit(&sbuffer, data, length); if (!s) @@ -454,7 +466,11 @@ BOOL nsc_process_message(NSC_CONTEXT* WINPR_RESTRICT context, UINT16 bpp, UINT32 if (nDstStride == 0) nDstStride = minStride; if (nDstStride < minStride) + { + WLog_Print(context->priv->log, WLOG_ERROR, + "nDstStride %" PRIu32 " < minimum stride %" PRIu32, nDstStride, minStride); return FALSE; + } switch (bpp) { From 24a23e3028d61e192d5fc0866bee7d42147b5612 Mon Sep 17 00:00:00 2001 From: Armin Novak Date: Tue, 17 Feb 2026 08:30:24 +0100 Subject: [PATCH 2/4] [codec,nsc] deprecate nsc_decompose_message this function is just a wrapper around nsc_process_message with some wStream handling around it. Since proper length checks are missing deprecate it completely. --- include/freerdp/codec/nsc.h | 15 +++++++++++---- libfreerdp/codec/nsc_encode.c | 2 ++ 2 files changed, 13 insertions(+), 4 deletions(-) diff --git a/include/freerdp/codec/nsc.h b/include/freerdp/codec/nsc.h index b100ae305..d7b9854ef 100644 --- a/include/freerdp/codec/nsc.h +++ b/include/freerdp/codec/nsc.h @@ -84,10 +84,17 @@ extern "C" wStream* WINPR_RESTRICT s, const BYTE* WINPR_RESTRICT bmpdata, UINT32 width, UINT32 height, UINT32 rowstride); - FREERDP_API BOOL nsc_decompose_message(NSC_CONTEXT* WINPR_RESTRICT context, - wStream* WINPR_RESTRICT s, BYTE* WINPR_RESTRICT bmpdata, - UINT32 x, UINT32 y, UINT32 width, UINT32 height, - UINT32 rowstride, UINT32 format, UINT32 flip); + +#if !defined(WITHOUT_FREERDP_3x_DEPRECATED) + + WINPR_DEPRECATED_VAR( + "[since 3.23.0] deprecated, insecure! missing length checks. use nsc_process_message", + FREERDP_API BOOL nsc_decompose_message(NSC_CONTEXT* WINPR_RESTRICT context, + wStream* WINPR_RESTRICT s, + BYTE* WINPR_RESTRICT bmpdata, UINT32 x, UINT32 y, + UINT32 width, UINT32 height, UINT32 rowstride, + UINT32 format, UINT32 flip)); +#endif FREERDP_API BOOL nsc_context_reset(NSC_CONTEXT* WINPR_RESTRICT context, UINT32 width, UINT32 height); diff --git a/libfreerdp/codec/nsc_encode.c b/libfreerdp/codec/nsc_encode.c index b0fa8f4f2..ec6f282e1 100644 --- a/libfreerdp/codec/nsc_encode.c +++ b/libfreerdp/codec/nsc_encode.c @@ -495,6 +495,7 @@ BOOL nsc_compose_message(NSC_CONTEXT* WINPR_RESTRICT context, wStream* WINPR_RES return nsc_write_message(context, s, &message); } +#if !defined(WITHOUT_FREERDP_3x_DEPRECATED) BOOL nsc_decompose_message(NSC_CONTEXT* WINPR_RESTRICT context, wStream* WINPR_RESTRICT s, BYTE* WINPR_RESTRICT bmpdata, UINT32 x, UINT32 y, UINT32 width, UINT32 height, UINT32 rowstride, UINT32 format, UINT32 flip) @@ -511,3 +512,4 @@ BOOL nsc_decompose_message(NSC_CONTEXT* WINPR_RESTRICT context, wStream* WINPR_R Stream_Seek(s, size); return TRUE; } +#endif From 169971607cece48384cb94632b829bd57336af0f Mon Sep 17 00:00:00 2001 From: Armin Novak Date: Tue, 17 Feb 2026 08:38:04 +0100 Subject: [PATCH 3/4] [codec,nsc] fix use of nsc_process_message the second width/height argument should reflect the destination buffer pixel size --- libfreerdp/codec/clear.c | 10 ++++++---- libfreerdp/codec/nsc.c | 12 +++++++----- libfreerdp/gdi/gdi.c | 5 +++-- 3 files changed, 16 insertions(+), 11 deletions(-) diff --git a/libfreerdp/codec/clear.c b/libfreerdp/codec/clear.c index 0158bacfb..65b0dfb1c 100644 --- a/libfreerdp/codec/clear.c +++ b/libfreerdp/codec/clear.c @@ -133,7 +133,8 @@ static BOOL convert_color(BYTE* WINPR_RESTRICT dst, UINT32 nDstStep, UINT32 DstF static BOOL clear_decompress_nscodec(NSC_CONTEXT* WINPR_RESTRICT nsc, UINT32 width, UINT32 height, wStream* WINPR_RESTRICT s, UINT32 bitmapDataByteCount, BYTE* WINPR_RESTRICT pDstData, UINT32 DstFormat, - UINT32 nDstStep, UINT32 nXDstRel, UINT32 nYDstRel) + UINT32 nDstStep, UINT32 nXDstRel, UINT32 nYDstRel, + UINT32 nDstWidth, UINT32 nDstHeight) { BOOL rc = 0; @@ -141,8 +142,8 @@ static BOOL clear_decompress_nscodec(NSC_CONTEXT* WINPR_RESTRICT nsc, UINT32 wid return FALSE; rc = nsc_process_message(nsc, 32, width, height, Stream_Pointer(s), bitmapDataByteCount, - pDstData, DstFormat, nDstStep, nXDstRel, nYDstRel, width, height, - FREERDP_FLIP_NONE); + pDstData, DstFormat, nDstStep, nXDstRel, nYDstRel, nDstWidth, + nDstHeight, FREERDP_FLIP_NONE); Stream_Seek(s, bitmapDataByteCount); return rc; } @@ -532,7 +533,8 @@ static BOOL clear_decompress_subcodecs_data(CLEAR_CONTEXT* WINPR_RESTRICT clear, case 1: /* NSCodec */ if (!clear_decompress_nscodec(clear->nsc, width, height, s, bitmapDataByteCount, - pDstData, DstFormat, nDstStep, nXDstRel, nYDstRel)) + pDstData, DstFormat, nDstStep, nXDstRel, nYDstRel, + nDstWidth, nDstHeight)) return FALSE; break; diff --git a/libfreerdp/codec/nsc.c b/libfreerdp/codec/nsc.c index ec05db866..3bcd0a480 100644 --- a/libfreerdp/codec/nsc.c +++ b/libfreerdp/codec/nsc.c @@ -433,15 +433,17 @@ BOOL nsc_context_set_parameters(NSC_CONTEXT* WINPR_RESTRICT context, NSC_PARAMET BOOL nsc_process_message(NSC_CONTEXT* WINPR_RESTRICT context, UINT16 bpp, UINT32 width, UINT32 height, const BYTE* data, UINT32 length, BYTE* WINPR_RESTRICT pDstData, UINT32 DstFormat, UINT32 nDstStride, - UINT32 nXDst, UINT32 nYDst, UINT32 nWidth, - WINPR_ATTR_UNUSED UINT32 nHeight, UINT32 flip) + UINT32 nXDst, UINT32 nYDst, UINT32 nWidth, UINT32 nHeight, UINT32 flip) { + WINPR_ASSERT(context); + WINPR_ASSERT(context->priv); + wStream sbuffer = { 0 }; BOOL ret = 0; - if (!context || !data || !pDstData) + if (!data || !pDstData) { - WLog_ERR(TAG, "Invalid argument: context=%p, data=%p, pDstData=%p", (void*)context, - (const void*)data, (void*)pDstData); + WLog_Print(context->priv->log, WLOG_ERROR, "Invalid argument: data=%p, pDstData=%p", + (const void*)data, (void*)pDstData); return FALSE; } diff --git a/libfreerdp/gdi/gdi.c b/libfreerdp/gdi/gdi.c index b2e3c0472..d5e70a230 100644 --- a/libfreerdp/gdi/gdi.c +++ b/libfreerdp/gdi/gdi.c @@ -1144,8 +1144,9 @@ static BOOL gdi_surface_bits(rdpContext* context, const SURFACE_BITS_COMMAND* cm if (!nsc_process_message( context->codecs->nsc, cmd->bmp.bpp, cmd->bmp.width, cmd->bmp.height, cmd->bmp.bitmapData, cmd->bmp.bitmapDataLength, gdi->primary_buffer, format, - gdi->stride, cmdRect.left, cmdRect.top, cmdRect.right - cmdRect.left, - cmdRect.bottom - cmdRect.top, FREERDP_FLIP_VERTICAL)) + gdi->stride, cmdRect.left, cmdRect.top, + WINPR_ASSERTING_INT_CAST(UINT32, gdi->width), + WINPR_ASSERTING_INT_CAST(UINT32, gdi->height), FREERDP_FLIP_VERTICAL)) { WLog_ERR(TAG, "Failed to process NSCodec message"); goto out; From f3d23da0ae47cd04f5a8932b0bdce2b040b57abb Mon Sep 17 00:00:00 2001 From: Armin Novak Date: Tue, 17 Feb 2026 09:15:03 +0100 Subject: [PATCH 4/4] [codec,nsc] update function docs * add doxygen for functions * add support for scanline value 0 --- include/freerdp/codec/nsc.h | 42 ++++++++++++++++++++++++++++++----- libfreerdp/codec/nsc_encode.c | 3 +++ 2 files changed, 39 insertions(+), 6 deletions(-) diff --git a/include/freerdp/codec/nsc.h b/include/freerdp/codec/nsc.h index d7b9854ef..c28d79f6d 100644 --- a/include/freerdp/codec/nsc.h +++ b/include/freerdp/codec/nsc.h @@ -34,12 +34,14 @@ extern "C" { #endif + /// parameter types available to change in a \ref NSC_CONTEXT See [MS-RDPNSC] 2.2.1 NSCodec + /// Capability Set (TS_NSCODEC_CAPABILITYSET) for details typedef enum { - NSC_COLOR_LOSS_LEVEL, - NSC_ALLOW_SUBSAMPLING, - NSC_DYNAMIC_COLOR_FIDELITY, - NSC_COLOR_FORMAT + NSC_COLOR_LOSS_LEVEL, /**< \b colorLossLevel */ + NSC_ALLOW_SUBSAMPLING, /**< \b fAllowSubsampling */ + NSC_DYNAMIC_COLOR_FIDELITY, /**< \b fAllowDynamicFidelity */ + NSC_COLOR_FORMAT /**< \ref PIXEL_FORMAT color format used for internal bitmap buffer */ } NSC_PARAMETER; typedef struct S_NSC_CONTEXT NSC_CONTEXT; @@ -50,12 +52,20 @@ extern "C" UINT32 pixel_format)); #endif + /** @brief Set a \ref NSC_PARAMETER for a \ref NSC_CONTEXT + * + * @param context The \ref NSC_CONTEXT context to work on. Must not be \b NULL + * @param what A \ref NSC_PARAMETER to identify what to change + * @param value The value to set + * + * @return \b TRUE if successful, \b FALSE otherwise + */ FREERDP_API BOOL nsc_context_set_parameters(NSC_CONTEXT* WINPR_RESTRICT context, NSC_PARAMETER what, UINT32 value); /** @brief decode a NSC message * - * @param context The context to work on + * @param context The \ref NSC_CONTEXT context to work on. Must not be \b NULL * @param bpp The bit depth of the data * @param width The width in pixels of the NSC surface * @param height The height in pixels of the NSC surface @@ -80,10 +90,22 @@ extern "C" UINT32 DstFormat, UINT32 nDstStride, UINT32 nXDst, UINT32 nYDst, UINT32 nWidth, UINT32 nHeight, UINT32 flip); + /** @brief Encode a bitmap with \b NSC + * + * @param context The \ref NSC_CONTEXT context to work on. Must not be \b NULL + * @param s a \ref wStream used to write \b NSC encoded data to. Must not be \b NULL + * @param bmpdata A pointer to the bitmap to encode. Format must match \b NSC_COLOR_FORMAT + * @param width The width of the bitmap in pixels + * @param height The height of the bitmap in pixels + * @param scanline The stride of a bitmap line in bytes. If \b 0 the value of \b width * bytes + * \b NSC_COLOR_FORMAT is used. + * + * @bug Versions < 3.23.0 do not support \b 0 for scanlines and abort. + */ FREERDP_API BOOL nsc_compose_message(NSC_CONTEXT* WINPR_RESTRICT context, wStream* WINPR_RESTRICT s, const BYTE* WINPR_RESTRICT bmpdata, UINT32 width, - UINT32 height, UINT32 rowstride); + UINT32 height, UINT32 scanline); #if !defined(WITHOUT_FREERDP_3x_DEPRECATED) @@ -96,6 +118,14 @@ extern "C" UINT32 format, UINT32 flip)); #endif + /** @brief This function resets a \ref NSC_CONTEXT to a new resolution + * + * @param context The \ref NSC_CONTEXT context to work on. Must not be \b NULL + * @param width The width of the context in pixels + * @param height The height of the context in pixels + * + * @return \b TRUE if successful, \b FALSE otherwise + */ FREERDP_API BOOL nsc_context_reset(NSC_CONTEXT* WINPR_RESTRICT context, UINT32 width, UINT32 height); diff --git a/libfreerdp/codec/nsc_encode.c b/libfreerdp/codec/nsc_encode.c index ec6f282e1..a0a82139a 100644 --- a/libfreerdp/codec/nsc_encode.c +++ b/libfreerdp/codec/nsc_encode.c @@ -463,6 +463,9 @@ BOOL nsc_compose_message(NSC_CONTEXT* WINPR_RESTRICT context, wStream* WINPR_RES if (!context || !s || !data) return FALSE; + if (scanline == 0) + scanline = width * FreeRDPGetBytesPerPixel(context->format); + context->width = WINPR_ASSERTING_INT_CAST(UINT16, width); context->height = WINPR_ASSERTING_INT_CAST(UINT16, height);