[jgardou] 64189: [WIN32K] - Do not make callouts to user-mode while holding a lock on GDI objects. Fixes crashes in user32_winetest:win and :msg

Author: jgardou
Date: Thu Sep 18 13:26:55 2014
New Revision: 64189

URL: http://svn.reactos.org/svn/reactos?rev=64189&view=rev
Log:
[WIN32K]
 - Do not make callouts to user-mode while holding a lock on GDI objects.
Fixes crashes in user32_winetest:win and :msg

Modified:
    trunk/reactos/win32ss/user/ntuser/painting.c

Modified: trunk/reactos/win32ss/user/ntuser/painting.c
URL:
http://svn.reactos.org/svn/reactos/trunk/reactos/win32ss/user/ntuser/painti…
==============================================================================
--- trunk/reactos/win32ss/user/ntuser/painting.c	[iso-8859-1] (original)
+++ trunk/reactos/win32ss/user/ntuser/painting.c	[iso-8859-1] Thu Sep 18 13:26:55 2014
@@ -1230,13 +1230,12 @@
     REGION_SetRectRgn(Rgn, Rect.left, Rect.top, Rect.right, Rect.bottom);
     RegionType = IntGdiCombineRgn(Rgn, Rgn, UpdateRgn, RGN_AND);
     IntGdiOffsetRgn(Rgn, -Window->rcClient.left, -Window->rcClient.top);
+    RGNOBJAPI_Unlock(UpdateRgn);
 
    if (bErase && RegionType != NULLREGION && RegionType != ERROR)
    {
       co_UserRedrawWindow(Window, NULL, NULL, RDW_ERASENOW | RDW_NOCHILDREN);
    }
-
-   RGNOBJAPI_Unlock(UpdateRgn);
 
    return RegionType;
 }
@@ -1265,7 +1264,8 @@
       RETURN(ERROR);
    }
 
-   Rgn = RGNOBJAPI_Lock(hRgn, NULL);
+   /* Use a system region, we can't hold GDI locks when doing roundtrips to user mode
*/
+   Rgn = IntSysCreateRectpRgn(0, 0, 0, 0);
    if (!Rgn)
        RETURN(ERROR);
 
@@ -1276,8 +1276,21 @@
    RETURN(ret);
 
 CLEANUP:
+   if (Rgn && (_ret_ != ERROR))
+   {
+       PREGION TheRgn = RGNOBJAPI_Lock(hRgn, NULL);
+       if (!TheRgn)
+       {
+           EngSetLastError(ERROR_INVALID_HANDLE);
+           _ret_ = ERROR;
+       }
+       IntGdiCombineRgn(TheRgn, Rgn, NULL, RGN_COPY);
+       RGNOBJAPI_Unlock(TheRgn);
+   }
+
    if (Rgn)
-       RGNOBJAPI_Unlock(Rgn);
+       REGION_Delete(Rgn);
+
    TRACE("Leave NtUserGetUpdateRgn, ret=%i\n",_ret_);
    UserLeave();
    END_CLEANUP;
@@ -1427,14 +1440,27 @@
       RETURN( FALSE);
    }
 
+   /* We can't hold lock on GDI obects while doing roundtrips to user mode,
+    * so use a copy instead */
    if (hrgnUpdate)
    {
-       RgnUpdate = RGNOBJAPI_Lock(hrgnUpdate, NULL);
+       PREGION RgnTemp;
+
+       RgnUpdate = IntSysCreateRectpRgn(0, 0, 0, 0);
        if (!RgnUpdate)
+       {
+           EngSetLastError(ERROR_NOT_ENOUGH_MEMORY);
+           RETURN(FALSE);
+       }
+
+       RgnTemp = RGNOBJAPI_Lock(hrgnUpdate, NULL);
+       if (!RgnTemp)
        {
            EngSetLastError(ERROR_INVALID_HANDLE);
            RETURN(FALSE);
        }
+       IntGdiCombineRgn(RgnUpdate, RgnTemp, NULL, RGN_COPY);
+       RGNOBJAPI_Unlock(RgnTemp);
    }
 
    UserRefObjectCo(Wnd, &Ref);
@@ -1450,7 +1476,7 @@
 
 CLEANUP:
     if (RgnUpdate)
-        RGNOBJAPI_Unlock(RgnUpdate);
+        REGION_Delete(RgnUpdate);
    TRACE("Leave NtUserRedrawWindow, ret=%i\n",_ret_);
    UserLeave();
    END_CLEANUP;
@@ -1687,11 +1713,10 @@
    INT Result;
    PWND Window = NULL, CaretWnd;
    HDC hDC;
-   PREGION RgnOwn = NULL, RgnTemp, RgnWinupd = NULL;
+   PREGION RgnUpdate = NULL, RgnTemp, RgnWinupd = NULL;
    HWND hwndCaret;
    DWORD dcxflags = 0;
    int rdw_flags;
-   BOOL bOwnRgn = TRUE;
    NTSTATUS Status = STATUS_SUCCESS;
    DECLARE_RETURN(DWORD);
    USER_REFERENCE_ENTRY Ref, CaretRef;
@@ -1743,17 +1768,26 @@
       RETURN(NULLREGION);
    }
 
+   /* We must use a copy of the region, as we can't hold an exclusive lock
+    * on it while doing callouts to user-mode */
+   RgnUpdate = IntSysCreateRectpRgn(0, 0, 0, 0);
+   if(!RgnUpdate)
+   {
+       EngSetLastError(ERROR_NOT_ENOUGH_MEMORY);
+       RETURN(ERROR);
+   }
+
    if (hrgnUpdate)
    {
-      RgnOwn = RGNOBJAPI_Lock(hrgnUpdate, NULL);
-      if (!RgnOwn)
-      {
-          RETURN(ERROR);
-      }
-      bOwnRgn = FALSE;
-   }
-   else
-      RgnOwn = IntSysCreateRectpRgn(0, 0, 0, 0);
+       RgnTemp = RGNOBJAPI_Lock(hrgnUpdate, NULL);
+       if (!RgnTemp)
+       {
+           EngSetLastError(ERROR_INVALID_HANDLE);
+           RETURN(ERROR);
+       }
+       IntGdiCombineRgn(RgnUpdate, RgnTemp, NULL, RGN_COPY);
+       RGNOBJAPI_Unlock(RgnTemp);
+   }
 
    /* ScrollWindow uses the window DC, ScrollWindowEx doesn't */
    if (flags & SW_SCROLLWNDDCE)
@@ -1788,9 +1822,9 @@
    Result = UserScrollDC( hDC,
                           dx,
                           dy,
-                         &rcScroll,
-                         &rcClip,
-                          RgnOwn,
+                          &rcScroll,
+                          &rcClip,
+                          RgnUpdate,
                           prcUnsafeUpdate? &rcUpdate : NULL);
 
    UserReleaseDC(Window, hDC, FALSE);
@@ -1801,28 +1835,31 @@
     */
 
    RgnTemp = IntSysCreateRectpRgn(0, 0, 0, 0);
-   if (RgnTemp)
-   {
-       if (co_UserGetUpdateRgn(Window, RgnTemp, FALSE) != NULLREGION)
-       {
-          PREGION RgnClip = IntSysCreateRectpRgnIndirect(&rcClip);
-          if (RgnClip)
+   if (!RgnTemp)
+   {
+       EngSetLastError(ERROR_NOT_ENOUGH_MEMORY);
+       RETURN(ERROR);
+   }
+
+   if (co_UserGetUpdateRgn(Window, RgnTemp, FALSE) != NULLREGION)
+   {
+      PREGION RgnClip = IntSysCreateRectpRgnIndirect(&rcClip);
+      if (RgnClip)
+      {
+          if (hrgnUpdate)
           {
-              if (!bOwnRgn)
-              {
-                 RgnWinupd = IntSysCreateRectpRgn( 0, 0, 0, 0);
-                 IntGdiCombineRgn( RgnWinupd, RgnTemp, 0, RGN_COPY);
-              }
-              IntGdiOffsetRgn(RgnTemp, dx, dy);
-              IntGdiCombineRgn(RgnTemp, RgnTemp, RgnClip, RGN_AND);
-              if (!bOwnRgn)
-                  IntGdiCombineRgn( RgnWinupd, RgnWinupd, RgnTemp, RGN_OR );
-              co_UserRedrawWindow(Window, NULL, RgnTemp, rdw_flags );
-              REGION_Delete(RgnClip);
+             RgnWinupd = IntSysCreateRectpRgn( 0, 0, 0, 0);
+             IntGdiCombineRgn( RgnWinupd, RgnTemp, 0, RGN_COPY);
           }
-       }
-       REGION_Delete(RgnTemp);
-   }
+          IntGdiOffsetRgn(RgnTemp, dx, dy);
+          IntGdiCombineRgn(RgnTemp, RgnTemp, RgnClip, RGN_AND);
+          if (hrgnUpdate)
+              IntGdiCombineRgn( RgnWinupd, RgnWinupd, RgnTemp, RGN_OR );
+          co_UserRedrawWindow(Window, NULL, RgnTemp, rdw_flags );
+          REGION_Delete(RgnClip);
+      }
+   }
+   REGION_Delete(RgnTemp);
 
    if (flags & SW_SCROLLCHILDREN)
    {
@@ -1854,7 +1891,7 @@
 
    if (flags & (SW_INVALIDATE | SW_ERASE))
    {
-      co_UserRedrawWindow(Window, NULL, RgnOwn, rdw_flags |
+      co_UserRedrawWindow(Window, NULL, RgnUpdate, rdw_flags |
                           ((flags & SW_ERASE) ? RDW_ERASENOW : 0) |
                           ((flags & SW_SCROLLCHILDREN) ? RDW_ALLCHILDREN : 0));
    }
@@ -1893,19 +1930,27 @@
    RETURN(Result);
 
 CLEANUP:
-   if (RgnWinupd && !bOwnRgn)
-   {
-      IntGdiCombineRgn( RgnOwn, RgnOwn, RgnWinupd, RGN_OR);
-      REGION_Delete(RgnWinupd);
-   }
-
-   if (RgnOwn && !hrgnUpdate)
-   {
-      REGION_Delete(RgnOwn);
-   }
-   else if (RgnOwn)
-   {
-       RGNOBJAPI_Unlock(RgnOwn);
+   if (hrgnUpdate && (Result != ERROR))
+   {
+       /* Give everything back to the caller */
+       RgnTemp = RGNOBJAPI_Lock(hrgnUpdate, NULL);
+       /* The handle should still be valid */
+       ASSERT(RgnTemp);
+       if (RgnWinupd)
+           IntGdiCombineRgn(RgnTemp, RgnUpdate, RgnWinupd, RGN_OR);
+       else
+           IntGdiCombineRgn(RgnTemp, RgnUpdate, NULL, RGN_COPY);
+       RGNOBJAPI_Unlock(RgnTemp);
+   }
+
+   if (RgnWinupd)
+   {
+       REGION_Delete(RgnWinupd);
+   }
+
+   if (RgnUpdate)
+   {
+      REGION_Delete(RgnUpdate);
    }
 
    if (Window)