From: Armin Novak <armin.novak@thincast.com>
Date: Thu, 11 Apr 2019 13:30:05 +0000 (+0200)
Subject: Unified update->BeginPaint and update->EndPaint
X-Git-Tag: 2.0.0~479^2
X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=2a26aa0d87ec15f84fcd0176c3f3a016b8deacaf;p=platform%2Fupstream%2Ffreerdp.git

Unified update->BeginPaint and update->EndPaint

Since these functions were called from 2 different threads
(main thread or dynamic channel) depending on fastpath or
gfx channel use lock between these.
If not locked the invalid region may be accessed from both
threads and lead to crashes as experienced with nightly df280a7ff
---

diff --git a/include/freerdp/update.h b/include/freerdp/update.h
index b6d5283..b2e1a4c 100644
--- a/include/freerdp/update.h
+++ b/include/freerdp/update.h
@@ -252,6 +252,7 @@ struct rdp_update
 	BOOL combineUpdates;
 	rdpBounds currentBounds;
 	rdpBounds previousBounds;
+	CRITICAL_SECTION mux;
 };
 
 #endif /* FREERDP_UPDATE_H */
diff --git a/libfreerdp/core/fastpath.c b/libfreerdp/core/fastpath.c
index c7899fd..f915371 100644
--- a/libfreerdp/core/fastpath.c
+++ b/libfreerdp/core/fastpath.c
@@ -633,6 +633,7 @@ out_fail:
 
 int fastpath_recv_updates(rdpFastPath* fastpath, wStream* s)
 {
+	int rc = -2;
 	rdpUpdate* update;
 
 	if (!fastpath || !fastpath->rdp || !fastpath->rdp->update || !s)
@@ -640,22 +641,26 @@ int fastpath_recv_updates(rdpFastPath* fastpath, wStream* s)
 
 	update = fastpath->rdp->update;
 
-	if (!IFCALLRESULT(TRUE, update->BeginPaint, update->context))
-		return -2;
+	if (!update_begin_paint(update))
+		goto fail;
 
 	while (Stream_GetRemainingLength(s) >= 3)
 	{
 		if (fastpath_recv_update_data(fastpath, s) < 0)
 		{
 			WLog_ERR(TAG, "fastpath_recv_update_data() fail");
-			return -3;
+			rc = -3;
+			goto fail;
 		}
 	}
 
-	if (!IFCALLRESULT(FALSE, update->EndPaint, update->context))
+	rc = 0;
+fail:
+
+	if (!update_end_paint(update))
 		return -4;
 
-	return 0;
+	return rc;
 }
 
 static BOOL fastpath_read_input_event_header(wStream* s, BYTE* eventFlags, BYTE* eventCode)
diff --git a/libfreerdp/core/freerdp.c b/libfreerdp/core/freerdp.c
index 917347c..cf86548 100644
--- a/libfreerdp/core/freerdp.c
+++ b/libfreerdp/core/freerdp.c
@@ -270,12 +270,16 @@ BOOL freerdp_connect(freerdp* instance)
 			Stream_SetLength(s, record.length);
 			Stream_SetPosition(s, 0);
 
-			if (!update->BeginPaint(update->context))
-				status = FALSE;
-			else if (update_recv_surfcmds(update, s) < 0)
-				status = FALSE;
-			else if (!update->EndPaint(update->context))
+			if (!update_begin_paint(update))
 				status = FALSE;
+			else
+			{
+				if (update_recv_surfcmds(update, s) < 0)
+					status = FALSE;
+
+				if (!update_end_paint(update))
+					status = FALSE;
+			}
 
 			Stream_Release(s);
 		}
diff --git a/libfreerdp/core/update.c b/libfreerdp/core/update.c
index 8936742..f6f059c 100644
--- a/libfreerdp/core/update.c
+++ b/libfreerdp/core/update.c
@@ -636,8 +636,8 @@ BOOL update_recv(rdpUpdate* update, wStream* s)
 	Stream_Read_UINT16(s, updateType); /* updateType (2 bytes) */
 	WLog_Print(update->log, WLOG_TRACE, "%s Update Data PDU", UPDATE_TYPE_STRINGS[updateType]);
 
-	if (!IFCALLRESULT(TRUE, update->BeginPaint, context))
-		return FALSE;
+	if (!update_begin_paint(update))
+		goto fail;
 
 	switch (updateType)
 	{
@@ -652,7 +652,7 @@ BOOL update_recv(rdpUpdate* update, wStream* s)
 				if (!bitmap_update)
 				{
 					WLog_ERR(TAG, "UPDATE_TYPE_BITMAP - update_read_bitmap_update() failed");
-					return FALSE;
+					goto fail;
 				}
 
 				rc = IFCALLRESULT(FALSE, update->BitmapUpdate, context, bitmap_update);
@@ -667,7 +667,7 @@ BOOL update_recv(rdpUpdate* update, wStream* s)
 				if (!palette_update)
 				{
 					WLog_ERR(TAG, "UPDATE_TYPE_PALETTE - update_read_palette() failed");
-					return FALSE;
+					goto fail;
 				}
 
 				rc = IFCALLRESULT(FALSE, update->Palette, context, palette_update);
@@ -684,15 +684,17 @@ BOOL update_recv(rdpUpdate* update, wStream* s)
 			break;
 	}
 
+fail:
+
+	if (!update_end_paint(update))
+		rc = FALSE;
+
 	if (!rc)
 	{
 		WLog_ERR(TAG, "UPDATE_TYPE %s [%"PRIu16"] failed", update_type_to_string(updateType), updateType);
 		return FALSE;
 	}
 
-	if (!IFCALLRESULT(FALSE, update->EndPaint, context))
-		return FALSE;
-
 	return TRUE;
 }
 
@@ -764,13 +766,16 @@ void update_post_disconnect(rdpUpdate* update)
 	update->initialState = TRUE;
 }
 
-static BOOL update_begin_paint(rdpContext* context)
+static BOOL _update_begin_paint(rdpContext* context)
 {
 	wStream* s;
 	rdpUpdate* update = context->update;
 
 	if (update->us)
-		update->EndPaint(context);
+	{
+		if (!update_end_paint(update))
+			return FALSE;
+	}
 
 	s = fastpath_update_pdu_init_new(context->rdp->fastpath);
 
@@ -785,7 +790,7 @@ static BOOL update_begin_paint(rdpContext* context)
 	return TRUE;
 }
 
-static BOOL update_end_paint(rdpContext* context)
+static BOOL _update_end_paint(rdpContext* context)
 {
 	wStream* s;
 	int headerLength;
@@ -821,20 +826,14 @@ static void update_flush(rdpContext* context)
 
 	if (update->numberOrders > 0)
 	{
-		update->EndPaint(context);
-		update->BeginPaint(context);
+		update_end_paint(update);
+		update_begin_paint(update);
 	}
 }
 
 static void update_force_flush(rdpContext* context)
 {
-	rdpUpdate* update = context->update;
-
-	if (update->numberOrders > 0)
-	{
-		update->EndPaint(context);
-		update->BeginPaint(context);
-	}
+	update_flush(context);
 }
 
 static BOOL update_check_flush(rdpContext* context, int size)
@@ -845,7 +844,7 @@ static BOOL update_check_flush(rdpContext* context, int size)
 
 	if (!update->us)
 	{
-		update->BeginPaint(context);
+		update_begin_paint(update);
 		return FALSE;
 	}
 
@@ -2080,8 +2079,8 @@ static BOOL update_send_set_keyboard_ime_status(rdpContext* context,
 
 void update_register_server_callbacks(rdpUpdate* update)
 {
-	update->BeginPaint = update_begin_paint;
-	update->EndPaint = update_end_paint;
+	update->BeginPaint = _update_begin_paint;
+	update->EndPaint = _update_end_paint;
 	update->SetBounds = update_set_bounds;
 	update->Synchronize = update_send_synchronize;
 	update->DesktopResize = update_send_desktop_resize;
@@ -2095,7 +2094,6 @@ void update_register_server_callbacks(rdpUpdate* update)
 	update->SetKeyboardImeStatus = update_send_set_keyboard_ime_status;
 	update->SaveSessionInfo = rdp_send_save_session_info;
 	update->ServerStatusInfo = rdp_send_server_status_info;
-
 	update->primary->DstBlt = update_send_dstblt;
 	update->primary->PatBlt = update_send_patblt;
 	update->primary->ScrBlt = update_send_scrblt;
@@ -2103,7 +2101,6 @@ void update_register_server_callbacks(rdpUpdate* update)
 	update->primary->LineTo = update_send_line_to;
 	update->primary->MemBlt = update_send_memblt;
 	update->primary->GlyphIndex = update_send_glyph_index;
-
 	update->secondary->CacheBitmap = update_send_cache_bitmap;
 	update->secondary->CacheBitmapV2 = update_send_cache_bitmap_v2;
 	update->secondary->CacheBitmapV3 = update_send_cache_bitmap_v3;
@@ -2111,10 +2108,8 @@ void update_register_server_callbacks(rdpUpdate* update)
 	update->secondary->CacheGlyph = update_send_cache_glyph;
 	update->secondary->CacheGlyphV2 = update_send_cache_glyph_v2;
 	update->secondary->CacheBrush = update_send_cache_brush;
-
 	update->altsec->CreateOffscreenBitmap = update_send_create_offscreen_bitmap_order;
 	update->altsec->SwitchSurface = update_send_switch_surface_order;
-
 	update->pointer->PointerSystem = update_send_pointer_system;
 	update->pointer->PointerPosition = update_send_pointer_position;
 	update->pointer->PointerColor = update_send_pointer_color;
@@ -2164,6 +2159,7 @@ rdpUpdate* update_new(rdpRdp* rdp)
 		return NULL;
 
 	update->log = WLog_Get("com.freerdp.core.update");
+	InitializeCriticalSection(&(update->mux));
 	update->pointer = (rdpPointerUpdate*) calloc(1, sizeof(rdpPointerUpdate));
 
 	if (!update->pointer)
@@ -2241,6 +2237,35 @@ void update_free(rdpUpdate* update)
 		}
 
 		MessageQueue_Free(update->queue);
+		DeleteCriticalSection(&update->mux);
 		free(update);
 	}
 }
+
+
+BOOL update_begin_paint(rdpUpdate* update)
+{
+	if (!update)
+		return FALSE;
+
+	EnterCriticalSection(&update->mux);
+
+	if (!update->BeginPaint)
+		return TRUE;
+
+	return update->BeginPaint(update->context);
+}
+
+BOOL update_end_paint(rdpUpdate* update)
+{
+	BOOL rc = FALSE;
+
+	if (!update)
+		return FALSE;
+
+	if (update->EndPaint)
+		rc = update->EndPaint(update->context);
+
+	LeaveCriticalSection(&update->mux);
+	return rc;
+}
diff --git a/libfreerdp/core/update.h b/libfreerdp/core/update.h
index a8b88b1..9cc24de 100644
--- a/libfreerdp/core/update.h
+++ b/libfreerdp/core/update.h
@@ -64,4 +64,7 @@ FREERDP_LOCAL void update_register_server_callbacks(rdpUpdate* update);
 FREERDP_LOCAL void update_register_client_callbacks(rdpUpdate* update);
 FREERDP_LOCAL int update_process_messages(rdpUpdate* update);
 
+FREERDP_LOCAL BOOL update_begin_paint(rdpUpdate* update);
+FREERDP_LOCAL BOOL update_end_paint(rdpUpdate* update);
+
 #endif /* FREERDP_LIB_CORE_UPDATE_H */
diff --git a/libfreerdp/gdi/gfx.c b/libfreerdp/gdi/gfx.c
index 6220346..40a0878 100644
--- a/libfreerdp/gdi/gfx.c
+++ b/libfreerdp/gdi/gfx.c
@@ -23,6 +23,8 @@
 #include "config.h"
 #endif
 
+#include "../core/update.h"
+
 #include <freerdp/log.h>
 #include <freerdp/gdi/gfx.h>
 #include <freerdp/gdi/region.h>
@@ -124,7 +126,7 @@ static UINT gdi_OutputUpdate(rdpGdi* gdi, gdiGfxSurface* surface)
 	if (!(rects = region16_rects(&surface->invalidRegion, &nbRects)) || !nbRects)
 		return CHANNEL_RC_OK;
 
-	if (!IFCALLRESULT(TRUE, update->BeginPaint, gdi->context))
+	if (!update_begin_paint(update))
 		goto fail;
 
 	for (i = 0; i < nbRects; i++)
@@ -146,11 +148,12 @@ static UINT gdi_OutputUpdate(rdpGdi* gdi, gdiGfxSurface* surface)
 		gdi_InvalidateRegion(gdi->primary->hdc, nXDst, nYDst, width, height);
 	}
 
-	if (!IFCALLRESULT(FALSE, update->EndPaint, gdi->context))
-		goto fail;
-
 	rc = CHANNEL_RC_OK;
 fail:
+
+	if (!update_end_paint(update))
+		rc = ERROR_INTERNAL_ERROR;
+
 	region16_clear(&(surface->invalidRegion));
 	return rc;
 }
diff --git a/libfreerdp/gdi/video.c b/libfreerdp/gdi/video.c
index 40f95f4..895693e 100644
--- a/libfreerdp/gdi/video.c
+++ b/libfreerdp/gdi/video.c
@@ -17,6 +17,7 @@
  * limitations under the License.
  */
 
+#include "../core/update.h"
 
 #include <freerdp/client/geometry.h>
 #include <freerdp/client/video.h>
@@ -82,6 +83,7 @@ static VideoSurface* gdiVideoCreateSurface(VideoClientContext* video, BYTE* data
 
 static BOOL gdiVideoShowSurface(VideoClientContext* video, VideoSurface* surface)
 {
+	BOOL rc = FALSE;
 	rdpGdi* gdi = (rdpGdi*)video->custom;
 	gdiVideoSurface* gdiSurface = (gdiVideoSurface*)surface;
 	RECTANGLE_16 surfaceRect;
@@ -90,18 +92,22 @@ static BOOL gdiVideoShowSurface(VideoClientContext* video, VideoSurface* surface
 	surfaceRect.top = surface->y;
 	surfaceRect.right = surface->x + surface->w;
 	surfaceRect.bottom = surface->y + surface->h;
-	update->BeginPaint(gdi->context);
+
+	if (!update_begin_paint(update))
+		goto fail;
 
 	if ((gdi->width < 0) || (gdi->height < 0))
-		return FALSE;
+		goto fail;
 	else
 	{
 		const UINT32 nXSrc = surface->x;
 		const UINT32 nYSrc = surface->y;
 		const UINT32 nXDst = nXSrc;
 		const UINT32 nYDst = nYSrc;
-		const UINT32 width = (surface->w + surface->x < (UINT32)gdi->width) ? surface->w : (UINT32)gdi->width - surface->x;
-		const UINT32 height = (surface->h + surface->y < (UINT32)gdi->height) ? surface->h : (UINT32)gdi->height -
+		const UINT32 width = (surface->w + surface->x < (UINT32)gdi->width) ? surface->w :
+		                     (UINT32)gdi->width - surface->x;
+		const UINT32 height = (surface->h + surface->y < (UINT32)gdi->height) ? surface->h :
+		                      (UINT32)gdi->height -
 		                      surface->y;
 
 		if (!freerdp_image_copy(gdi->primary_buffer, gdi->primary->hdc->format,
@@ -109,14 +115,21 @@ static BOOL gdiVideoShowSurface(VideoClientContext* video, VideoSurface* surface
 		                        nXDst, nYDst, width, height,
 		                        surface->data, gdi->primary->hdc->format,
 		                        gdiSurface->scanline, 0, 0, NULL, FREERDP_FLIP_NONE))
-			return FALSE;
+			goto fail;
 
 		if ((nXDst > INT32_MAX) || (nYDst > INT32_MAX) || (width > INT32_MAX) || (height > INT32_MAX))
-			return FALSE;
+			goto fail;
+
 		gdi_InvalidateRegion(gdi->primary->hdc, (INT32)nXDst, (INT32)nYDst, (INT32)width, (INT32)height);
 	}
-	update->EndPaint(gdi->context);
-	return TRUE;
+
+	rc = TRUE;
+fail:
+
+	if (!update_end_paint(update))
+		return FALSE;
+
+	return rc;
 }
 
 static BOOL gdiVideoDeleteSurface(VideoClientContext* video, VideoSurface* surface)