From febc4b3073bbcbe423e39fa09732b90ce09efa39 Mon Sep 17 00:00:00 2001 From: akallabeth Date: Fri, 18 Nov 2022 11:32:33 +0100 Subject: [PATCH] [gdi,gfx] Fixed possible memory leaks * WINPR_ASSERT all callbacks required to be set * Unify cache slot creation/destruction * Destroy cache slot before setting it --- channels/rdpgfx/client/rdpgfx_main.c | 2 +- libfreerdp/gdi/gfx.c | 174 +++++++++++++++++++-------- 2 files changed, 122 insertions(+), 54 deletions(-) diff --git a/channels/rdpgfx/client/rdpgfx_main.c b/channels/rdpgfx/client/rdpgfx_main.c index 392831027..ebfbacb94 100644 --- a/channels/rdpgfx/client/rdpgfx_main.c +++ b/channels/rdpgfx/client/rdpgfx_main.c @@ -82,7 +82,7 @@ static void evict_cache_slots(RdpgfxClientContext* context, UINT16 MaxCacheSlots { if (CacheSlots[index]) { - RDPGFX_EVICT_CACHE_ENTRY_PDU pdu; + RDPGFX_EVICT_CACHE_ENTRY_PDU pdu = { 0 }; pdu.cacheSlot = (UINT16)index + 1; if (context && context->EvictCacheEntry) diff --git a/libfreerdp/gdi/gfx.c b/libfreerdp/gdi/gfx.c index 0c8b4f6fd..95d18c290 100644 --- a/libfreerdp/gdi/gfx.c +++ b/libfreerdp/gdi/gfx.c @@ -88,7 +88,6 @@ static UINT gdi_ResetGraphics(RdpgfxClientContext* context, UINT16 count; UINT32 DesktopWidth; UINT32 DesktopHeight; - gdiGfxSurface* surface; UINT16* pSurfaceIds = NULL; rdpGdi* gdi; rdpUpdate* update; @@ -120,11 +119,14 @@ static UINT gdi_ResetGraphics(RdpgfxClientContext* context, update->DesktopResize(gdi->context); } + WINPR_ASSERT(context->GetSurfaceIds); context->GetSurfaceIds(context, &pSurfaceIds, &count); for (index = 0; index < count; index++) { - surface = (gdiGfxSurface*)context->GetSurfaceData(context, pSurfaceIds[index]); + WINPR_ASSERT(context->GetSurfaceData); + gdiGfxSurface* surface = + (gdiGfxSurface*)context->GetSurfaceData(context, pSurfaceIds[index]); if (!surface) continue; @@ -239,7 +241,6 @@ static UINT gdi_UpdateSurfaces(RdpgfxClientContext* context) UINT16 count; UINT16 index; UINT status = ERROR_INTERNAL_ERROR; - gdiGfxSurface* surface; UINT16* pSurfaceIds = NULL; rdpGdi* gdi; @@ -249,12 +250,16 @@ static UINT gdi_UpdateSurfaces(RdpgfxClientContext* context) WINPR_ASSERT(gdi); EnterCriticalSection(&context->mux); + + WINPR_ASSERT(context->GetSurfaceIds); context->GetSurfaceIds(context, &pSurfaceIds, &count); status = CHANNEL_RC_OK; for (index = 0; index < count; index++) { - surface = (gdiGfxSurface*)context->GetSurfaceData(context, pSurfaceIds[index]); + WINPR_ASSERT(context->GetSurfaceData); + gdiGfxSurface* surface = + (gdiGfxSurface*)context->GetSurfaceData(context, pSurfaceIds[index]); if (!surface) continue; @@ -335,6 +340,8 @@ static UINT gdi_SurfaceCommand_Uncompressed(rdpGdi* gdi, RdpgfxClientContext* co WINPR_ASSERT(gdi); WINPR_ASSERT(context); WINPR_ASSERT(cmd); + + WINPR_ASSERT(context->GetSurfaceData); surface = (gdiGfxSurface*)context->GetSurfaceData(context, (UINT16)MIN(UINT16_MAX, cmd->surfaceId)); @@ -399,6 +406,8 @@ static UINT gdi_SurfaceCommand_RemoteFX(rdpGdi* gdi, RdpgfxClientContext* contex WINPR_ASSERT(gdi); WINPR_ASSERT(context); WINPR_ASSERT(cmd); + + WINPR_ASSERT(context->GetSurfaceData); surface = (gdiGfxSurface*)context->GetSurfaceData(context, (UINT16)MIN(UINT16_MAX, cmd->surfaceId)); @@ -457,6 +466,8 @@ static UINT gdi_SurfaceCommand_ClearCodec(rdpGdi* gdi, RdpgfxClientContext* cont WINPR_ASSERT(gdi); WINPR_ASSERT(context); WINPR_ASSERT(cmd); + + WINPR_ASSERT(context->GetSurfaceData); surface = (gdiGfxSurface*)context->GetSurfaceData(context, (UINT16)MIN(UINT16_MAX, cmd->surfaceId)); @@ -514,6 +525,8 @@ static UINT gdi_SurfaceCommand_Planar(rdpGdi* gdi, RdpgfxClientContext* context, WINPR_ASSERT(gdi); WINPR_ASSERT(context); WINPR_ASSERT(cmd); + + WINPR_ASSERT(context->GetSurfaceData); surface = (gdiGfxSurface*)context->GetSurfaceData(context, (UINT16)MIN(UINT16_MAX, cmd->surfaceId)); @@ -574,6 +587,7 @@ static UINT gdi_SurfaceCommand_AVC420(rdpGdi* gdi, RdpgfxClientContext* context, WINPR_ASSERT(context); WINPR_ASSERT(cmd); + WINPR_ASSERT(context->GetSurfaceData); surface = (gdiGfxSurface*)context->GetSurfaceData(context, (UINT16)MIN(UINT16_MAX, cmd->surfaceId)); @@ -663,6 +677,8 @@ static UINT gdi_SurfaceCommand_AVC444(rdpGdi* gdi, RdpgfxClientContext* context, WINPR_ASSERT(gdi); WINPR_ASSERT(context); WINPR_ASSERT(cmd); + + WINPR_ASSERT(context->GetSurfaceData); surface = (gdiGfxSurface*)context->GetSurfaceData(context, (UINT16)MIN(UINT16_MAX, cmd->surfaceId)); @@ -802,6 +818,7 @@ static UINT gdi_SurfaceCommand_Alpha(rdpGdi* gdi, RdpgfxClientContext* context, if (!Stream_CheckAndLogRequiredLength(TAG, s, 4)) return ERROR_INVALID_DATA; + WINPR_ASSERT(context->GetSurfaceData); surface = (gdiGfxSurface*)context->GetSurfaceData(context, (UINT16)MIN(UINT16_MAX, cmd->surfaceId)); @@ -937,6 +954,8 @@ static UINT gdi_SurfaceCommand_Progressive(rdpGdi* gdi, RdpgfxClientContext* con WINPR_ASSERT(context); WINPR_ASSERT(cmd); const UINT16 surfaceId = (UINT16)MIN(UINT16_MAX, cmd->surfaceId); + + WINPR_ASSERT(context->GetSurfaceData); surface = (gdiGfxSurface*)context->GetSurfaceData(context, surfaceId); if (!surface) @@ -1151,6 +1170,8 @@ static UINT gdi_CreateSurface(RdpgfxClientContext* context, memset(surface->data, 0xFF, (size_t)surface->scanline * surface->height); region16_init(&surface->invalidRegion); + + WINPR_ASSERT(context->SetSurfaceData); rc = context->SetSurfaceData(context, surface->surfaceId, (void*)surface); fail: LeaveCriticalSection(&context->mux); @@ -1170,6 +1191,8 @@ static UINT gdi_DeleteSurface(RdpgfxClientContext* context, rdpCodecs* codecs = NULL; gdiGfxSurface* surface = NULL; EnterCriticalSection(&context->mux); + + WINPR_ASSERT(context->GetSurfaceData); surface = (gdiGfxSurface*)context->GetSurfaceData(context, deleteSurface->surfaceId); if (surface) @@ -1187,6 +1210,7 @@ static UINT gdi_DeleteSurface(RdpgfxClientContext* context, free(surface); } + WINPR_ASSERT(context->SetSurfaceData); res = context->SetSurfaceData(context, deleteSurface->surfaceId, NULL); if (res) rc = res; @@ -1215,6 +1239,8 @@ static UINT gdi_SolidFill(RdpgfxClientContext* context, const RDPGFX_SOLID_FILL_ RECTANGLE_16 invalidRect; rdpGdi* gdi = (rdpGdi*)context->custom; EnterCriticalSection(&context->mux); + + WINPR_ASSERT(context->GetSurfaceData); surface = (gdiGfxSurface*)context->GetSurfaceData(context, solidFill->surfaceId); if (!surface) @@ -1284,6 +1310,8 @@ static UINT gdi_SurfaceToSurface(RdpgfxClientContext* context, rdpGdi* gdi = (rdpGdi*)context->custom; EnterCriticalSection(&context->mux); rectSrc = &(surfaceToSurface->rectSrc); + + WINPR_ASSERT(context->GetSurfaceData); surfaceSrc = (gdiGfxSurface*)context->GetSurfaceData(context, surfaceToSurface->surfaceIdSrc); sameSurface = (surfaceToSurface->surfaceIdSrc == surfaceToSurface->surfaceIdDest) ? TRUE : FALSE; @@ -1341,6 +1369,40 @@ fail: return status; } +static void gdi_GfxCacheEntryFree(gdiGfxCacheEntry* entry) +{ + if (!entry) + return; + free(entry->data); + free(entry); +} + +static gdiGfxCacheEntry* gdi_GfxCacheEntryNew(UINT64 cacheKey, UINT32 width, UINT32 height, + UINT32 format) +{ + gdiGfxCacheEntry* cacheEntry = (gdiGfxCacheEntry*)calloc(1, sizeof(gdiGfxCacheEntry)); + if (!cacheEntry) + goto fail; + + cacheEntry->cacheKey = cacheKey; + cacheEntry->width = width; + cacheEntry->height = height; + cacheEntry->format = format; + cacheEntry->scanline = gfx_align_scanline(cacheEntry->width * 4, 16); + + if ((cacheEntry->width > 0) && (cacheEntry->height > 0)) + { + cacheEntry->data = (BYTE*)calloc(cacheEntry->height, cacheEntry->scanline); + + if (!cacheEntry->data) + goto fail; + } + return cacheEntry; +fail: + gdi_GfxCacheEntryFree(cacheEntry); + return NULL; +} + /** * Function description * @@ -1351,10 +1413,12 @@ static UINT gdi_SurfaceToCache(RdpgfxClientContext* context, { const RECTANGLE_16* rect; gdiGfxSurface* surface; - gdiGfxCacheEntry* cacheEntry; + gdiGfxCacheEntry* cacheEntry = NULL; UINT rc = ERROR_INTERNAL_ERROR; EnterCriticalSection(&context->mux); rect = &(surfaceToCache->rectSrc); + + WINPR_ASSERT(context->GetSurfaceData); surface = (gdiGfxSurface*)context->GetSurfaceData(context, surfaceToCache->surfaceId); if (!surface) @@ -1363,35 +1427,29 @@ static UINT gdi_SurfaceToCache(RdpgfxClientContext* context, if (!is_rect_valid(rect, surface->width, surface->height)) goto fail; - cacheEntry = (gdiGfxCacheEntry*)calloc(1, sizeof(gdiGfxCacheEntry)); + cacheEntry = gdi_GfxCacheEntryNew(surfaceToCache->cacheKey, (UINT32)(rect->right - rect->left), + (UINT32)(rect->bottom - rect->top), surface->format); if (!cacheEntry) goto fail; - cacheEntry->cacheKey = surfaceToCache->cacheKey; - cacheEntry->width = (UINT32)(rect->right - rect->left); - cacheEntry->height = (UINT32)(rect->bottom - rect->top); - cacheEntry->format = surface->format; - cacheEntry->scanline = gfx_align_scanline(cacheEntry->width * 4, 16); - cacheEntry->data = (BYTE*)calloc(cacheEntry->height, cacheEntry->scanline); - if (!cacheEntry->data) - { - free(cacheEntry); goto fail; - } if (!freerdp_image_copy(cacheEntry->data, cacheEntry->format, cacheEntry->scanline, 0, 0, cacheEntry->width, cacheEntry->height, surface->data, surface->format, surface->scanline, rect->left, rect->top, NULL, FREERDP_FLIP_NONE)) - { - free(cacheEntry->data); - free(cacheEntry); goto fail; - } + RDPGFX_EVICT_CACHE_ENTRY_PDU evict = { surfaceToCache->cacheSlot }; + WINPR_ASSERT(context->EvictCacheEntry); + context->EvictCacheEntry(context, &evict); + + WINPR_ASSERT(context->SetCacheSlotData); rc = context->SetCacheSlotData(context, surfaceToCache->cacheSlot, (void*)cacheEntry); fail: + if (rc != CHANNEL_RC_OK) + gdi_GfxCacheEntryFree(cacheEntry); LeaveCriticalSection(&context->mux); return rc; } @@ -1412,7 +1470,11 @@ static UINT gdi_CacheToSurface(RdpgfxClientContext* context, rdpGdi* gdi = (rdpGdi*)context->custom; EnterCriticalSection(&context->mux); + + WINPR_ASSERT(context->GetSurfaceData); surface = (gdiGfxSurface*)context->GetSurfaceData(context, cacheToSurface->surfaceId); + + WINPR_ASSERT(context->GetCacheSlotData); cacheEntry = (gdiGfxCacheEntry*)context->GetCacheSlotData(context, cacheToSurface->cacheSlot); if (!surface || !cacheEntry) @@ -1471,7 +1533,6 @@ static UINT gdi_CacheImportReply(RdpgfxClientContext* context, UINT16 index; UINT16 count; const UINT16* slots; - gdiGfxCacheEntry* cacheEntry; UINT error = CHANNEL_RC_OK; slots = cacheImportReply->cacheSlots; @@ -1484,28 +1545,26 @@ static UINT gdi_CacheImportReply(RdpgfxClientContext* context, if (cacheSlot == 0) continue; - cacheEntry = (gdiGfxCacheEntry*)context->GetCacheSlotData(context, cacheSlot); + WINPR_ASSERT(context->GetCacheSlotData); + gdiGfxCacheEntry* cacheEntry = + (gdiGfxCacheEntry*)context->GetCacheSlotData(context, cacheSlot); if (cacheEntry) continue; - cacheEntry = (gdiGfxCacheEntry*)calloc(1, sizeof(gdiGfxCacheEntry)); + cacheEntry = gdi_GfxCacheEntryNew(cacheSlot, 0, 0, PIXEL_FORMAT_BGRX32); if (!cacheEntry) return ERROR_INTERNAL_ERROR; - cacheEntry->width = 0; - cacheEntry->height = 0; - cacheEntry->format = PIXEL_FORMAT_BGRX32; - cacheEntry->scanline = (cacheEntry->width + (cacheEntry->width % 4)) * 4; - cacheEntry->data = NULL; - + WINPR_ASSERT(context->SetCacheSlotData); error = context->SetCacheSlotData(context, cacheSlot, (void*)cacheEntry); if (error) { WLog_ERR(TAG, "CacheImportReply: SetCacheSlotData failed with error %" PRIu32 "", error); + gdi_GfxCacheEntryFree(cacheEntry); break; } } @@ -1516,41 +1575,38 @@ static UINT gdi_CacheImportReply(RdpgfxClientContext* context, static UINT gdi_ImportCacheEntry(RdpgfxClientContext* context, UINT16 cacheSlot, PERSISTENT_CACHE_ENTRY* importCacheEntry) { - UINT error; + UINT error = ERROR_INTERNAL_ERROR; gdiGfxCacheEntry* cacheEntry; if (cacheSlot == 0) return CHANNEL_RC_OK; - cacheEntry = (gdiGfxCacheEntry*)calloc(1, sizeof(gdiGfxCacheEntry)); + cacheEntry = gdi_GfxCacheEntryNew(importCacheEntry->key64, importCacheEntry->width, + importCacheEntry->height, PIXEL_FORMAT_BGRX32); if (!cacheEntry) - return ERROR_INTERNAL_ERROR; - - cacheEntry->cacheKey = importCacheEntry->key64; - cacheEntry->width = (UINT32)importCacheEntry->width; - cacheEntry->height = (UINT32)importCacheEntry->height; - cacheEntry->format = PIXEL_FORMAT_BGRX32; - cacheEntry->scanline = (cacheEntry->width + (cacheEntry->width % 4)) * 4; - cacheEntry->data = (BYTE*)calloc(cacheEntry->height, cacheEntry->scanline); - - if (!cacheEntry->data) - { - free(cacheEntry); - return ERROR_INTERNAL_ERROR; - } + goto fail; if (!freerdp_image_copy(cacheEntry->data, cacheEntry->format, cacheEntry->scanline, 0, 0, cacheEntry->width, cacheEntry->height, importCacheEntry->data, PIXEL_FORMAT_BGRX32, 0, 0, 0, NULL, FREERDP_FLIP_NONE)) - { - return ERROR_INTERNAL_ERROR; - } + goto fail; + RDPGFX_EVICT_CACHE_ENTRY_PDU evict = { cacheSlot }; + WINPR_ASSERT(context->EvictCacheEntry); + error = context->EvictCacheEntry(context, &evict); + if (error != CHANNEL_RC_OK) + goto fail; + + WINPR_ASSERT(context->SetCacheSlotData); error = context->SetCacheSlotData(context, cacheSlot, (void*)cacheEntry); +fail: if (error) + { + gdi_GfxCacheEntryFree(cacheEntry); WLog_ERR(TAG, "ImportCacheEntry: SetCacheSlotData failed with error %" PRIu32 "", error); + } return error; } @@ -1560,6 +1616,7 @@ static UINT gdi_ExportCacheEntry(RdpgfxClientContext* context, UINT16 cacheSlot, { gdiGfxCacheEntry* cacheEntry; + WINPR_ASSERT(context->GetCacheSlotData); cacheEntry = (gdiGfxCacheEntry*)context->GetCacheSlotData(context, cacheSlot); if (cacheEntry) @@ -1586,15 +1643,18 @@ static UINT gdi_EvictCacheEntry(RdpgfxClientContext* context, { gdiGfxCacheEntry* cacheEntry; UINT rc = ERROR_NOT_FOUND; + + WINPR_ASSERT(context); + WINPR_ASSERT(evictCacheEntry); + EnterCriticalSection(&context->mux); + + WINPR_ASSERT(context->GetCacheSlotData); cacheEntry = (gdiGfxCacheEntry*)context->GetCacheSlotData(context, evictCacheEntry->cacheSlot); - if (cacheEntry) - { - free(cacheEntry->data); - free(cacheEntry); - } + gdi_GfxCacheEntryFree(cacheEntry); + WINPR_ASSERT(context->SetCacheSlotData); rc = context->SetCacheSlotData(context, evictCacheEntry->cacheSlot, NULL); LeaveCriticalSection(&context->mux); return rc; @@ -1611,6 +1671,8 @@ static UINT gdi_MapSurfaceToOutput(RdpgfxClientContext* context, UINT rc = ERROR_INTERNAL_ERROR; gdiGfxSurface* surface = NULL; EnterCriticalSection(&context->mux); + + WINPR_ASSERT(context->GetSurfaceData); surface = (gdiGfxSurface*)context->GetSurfaceData(context, surfaceToOutput->surfaceId); if (!surface) @@ -1642,6 +1704,8 @@ gdi_MapSurfaceToScaledOutput(RdpgfxClientContext* context, UINT rc = ERROR_INTERNAL_ERROR; gdiGfxSurface* surface = NULL; EnterCriticalSection(&context->mux); + + WINPR_ASSERT(context->GetSurfaceData); surface = (gdiGfxSurface*)context->GetSurfaceData(context, surfaceToOutput->surfaceId); if (!surface) @@ -1677,6 +1741,8 @@ static UINT gdi_MapSurfaceToWindow(RdpgfxClientContext* context, UINT rc = ERROR_INTERNAL_ERROR; gdiGfxSurface* surface; EnterCriticalSection(&context->mux); + + WINPR_ASSERT(context->GetSurfaceData); surface = (gdiGfxSurface*)context->GetSurfaceData(context, surfaceToWindow->surfaceId); if (!surface) @@ -1719,6 +1785,8 @@ gdi_MapSurfaceToScaledWindow(RdpgfxClientContext* context, UINT rc = ERROR_INTERNAL_ERROR; gdiGfxSurface* surface; EnterCriticalSection(&context->mux); + + WINPR_ASSERT(context->GetSurfaceData); surface = (gdiGfxSurface*)context->GetSurfaceData(context, surfaceToWindow->surfaceId); if (!surface)