[PATCH v5 0/1] MR3288: gdiplus: Clip polygons before drawing them

[David Kahurani (＠kahurani)]

Validate user data before passing it to PolyDraw.

The program in the bug requests to draw figures outrageously outside the DC's region after presumably, some uninitialized values happen as a result of a missing font. Native gdiplus seems to handle this gracefully so we probably also should.

Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=41617

Signed-off-by: David Kahurani

--
  v5: gdiplus: Clip polygons before drawing them

https://gitlab.winehq.org/wine/wine/-/merge_requests/3288

[PATCH v5 1/1] gdiplus: Clip polygons before drawing them

[David Kahurani]

From: David Kahurani

This should reduce the load on GDI when an application is trying to
draw to regions outside of selected DC's region

Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=41617
Signed-off-by: David Kahurani 
---
 dlls/gdiplus/graphics.c | 234 ++++++++++++++++++++++++++++++++--------
 1 file changed, 187 insertions(+), 47 deletions(-)

diff --git a/dlls/gdiplus/graphics.c b/dlls/gdiplus/graphics.c
index 6dc34707bbf..f9f54026eec 100644
--- a/dlls/gdiplus/graphics.c
+++ b/dlls/gdiplus/graphics.c
@@ -1920,6 +1920,192 @@ static void shorten_bezier_amt(GpPointF * pt, REAL amt, BOOL rev)
     }
 }

+static GpStatus get_graphics_device_bounds(GpGraphics* graphics, GpRectF* rect)
+{
+    RECT wnd_rect;
+    GpStatus stat=Ok;
+    GpUnit unit;
+
+    if(graphics->hwnd) {
+        if(!GetClientRect(graphics->hwnd, &wnd_rect))
+            return GenericError;
+
+        rect->X = wnd_rect.left;
+        rect->Y = wnd_rect.top;
+        rect->Width = wnd_rect.right - wnd_rect.left;
+        rect->Height = wnd_rect.bottom - wnd_rect.top;
+    }else if (graphics->image){
+        stat = GdipGetImageBounds(graphics->image, rect, &unit);
+        if (stat == Ok && unit != UnitPixel)
+            FIXME("need to convert from unit %i\n", unit);
+    }else if (GetObjectType(graphics->hdc) == OBJ_MEMDC){
+        HBITMAP hbmp;
+        BITMAP bmp;
+
+        rect->X = 0;
+        rect->Y = 0;
+
+        hbmp = GetCurrentObject(graphics->hdc, OBJ_BITMAP);
+        if (hbmp && GetObjectW(hbmp, sizeof(bmp), &bmp))
+        {
+            rect->Width = bmp.bmWidth;
+            rect->Height = bmp.bmHeight;
+        }
+        else
+        {
+            /* FIXME: ??? */
+            rect->Width = 1;
+            rect->Height = 1;
+        }
+    }else{
+        rect->X = 0;
+        rect->Y = 0;
+        rect->Width = GetDeviceCaps(graphics->hdc, HORZRES);
+        rect->Height = GetDeviceCaps(graphics->hdc, VERTRES);
+    }
+
+    return stat;
+}
+
+static void poly_clip(GpGraphics *graphics, POINT *pti, BYTE *tp, int count)
+{
+    RectF rect;
+    BYTE *tpcopy = heap_alloc_zero(count * sizeof(BYTE));
+
+    memcpy(tpcopy, tp, count);
+    get_graphics_device_bounds(graphics, &rect);
+
+    /* Ignore data that is not meant for drawing */
+    if (rect.Width == 1 && rect.Height == 1)
+        return;
+
+    for (int i = 0; i < count; i++)
+    {
+         RectF bounds;
+
+         switch(tp[i] &~ PT_CLOSEFIGURE)
+         {
+         case PT_BEZIERTO:
+         {
+             int j, left_most = pti[i - 1].x, right_most = pti[i - 1].x,
+                 top = pti[i - 1].y, bottom = pti[i - 1].y;
+
+             for (j = 0; j < 3; j++)
+             {
+                  if (pti[i + j].x < left_most)
+                      left_most = pti[i + j].x;
+
+                  if (pti[i + j].x > right_most)
+                      right_most = pti[i + j].x;
+
+                  if (pti[i + j].y < top)
+                      top = pti[i + j].y;
+
+                  if (pti[i + j].y > bottom)
+                      bottom = pti[i + j].y;
+             }
+
+             set_rect(&bounds, left_most, top, right_most - left_most, bottom - top);
+
+             /* properties of intersect rectangle */
+             left_most = (rect.X < bounds.X) ? bounds.X : rect.X;
+             top = (rect.Y < bounds.Y) ? bounds.Y : rect.Y;
+
+             right_most = (rect.X + rect.Width < bounds.X + bounds.Width)
+                           ? rect.X + rect.Width : bounds.X + bounds.Width;
+             bottom = (rect.Y + rect.Height < bounds.Y + bounds.Height)
+                      ? rect.Y + rect.Height : bounds.Y + bounds.Height;
+
+             if (left_most < right_most && top < bottom)
+             {
+                 i += 2;
+                 break;
+             }
+             else
+                 for(j = 0; j < 3; j++)
+                     tp[i + j] = PT_MOVETO;
+
+             i += 2;
+             break;
+         }
+         case PT_LINETO:
+         {
+             POINT tmp[4];
+
+             int j, left_most = pti[i - 1].x, right_most = pti[i - 1].x,
+                 top = pti[i - 1].y, bottom = pti[i - 1].y;
+
+             /* widen the path by a width of 10 */
+             if (pti[i - 1].y != pti[i].y)
+             {
+                 tmp[0].x = pti[i - 1].x + 5;
+                 tmp[0].y = pti[i - 1].y;
+                 tmp[1].x = pti[i - 1].x - 5;
+                 tmp[1].y = pti[i - 1].y;
+
+                 tmp[2].x = pti[i].x + 5;
+                 tmp[2].y = pti[i].y;
+                 tmp[3].x = pti[i].x - 5;
+                 tmp[3].y = pti[i].y;
+             }
+             else
+             {
+                 tmp[0].x = pti[i - 1].x;
+                 tmp[0].y = pti[i - 1].y + 5;
+                 tmp[1].x = pti[i - 1].x;
+                 tmp[1].y = pti[i - 1].y - 5;
+
+                 tmp[2].x = pti[i].x;
+                 tmp[2].y = pti[i].y + 5;
+                 tmp[3].x = pti[i].x;
+                 tmp[3].y = pti[i].y - 5;
+             }
+
+             for (j = 0; j < 4; j++)
+             {
+                  if (tmp[j].x < left_most)
+                      left_most = tmp[j].x;
+
+                  if (tmp[j].x > right_most)
+                      right_most = tmp[j].x;
+
+                  if (tmp[j].y < top)
+                      top = tmp[j].y;
+
+                  if (tmp[j].y > bottom)
+                      bottom = tmp[j].y;
+             }
+
+             set_rect(&bounds, left_most, top, right_most - left_most, bottom - top);
+
+             left_most = (rect.X < bounds.X) ? bounds.X : rect.X;
+             top = (rect.Y < bounds.Y) ? bounds.Y : rect.Y;
+
+             right_most = (rect.X + rect.Width < bounds.X + bounds.Width)
+                           ? rect.X + rect.Width : bounds.X + bounds.Width;
+             bottom = (rect.Y + rect.Height < bounds.Y + bounds.Height)
+                      ? rect.Y + rect.Height : bounds.Y + bounds.Height;
+
+             if (left_most < right_most && top < bottom)
+             {
+                 break;
+             }
+
+             tp[i] = PT_MOVETO;
+
+             break;
+         }
+         case PT_MOVETO:
+             break;
+
+         default:
+             ERR("Bad point type \n");
+         }
+    }
+
+    free(tpcopy);
+}
+
 /* Draws a combination of bezier curves and lines between points. */
 static GpStatus draw_poly(GpGraphics *graphics, GpPen *pen, GDIPCONST GpPointF * pt,
     GDIPCONST BYTE * types, INT count, BOOL caps)
@@ -2036,6 +2222,7 @@ static GpStatus draw_poly(GpGraphics *graphics, GpPen *pen, GDIPCONST GpPointF *
         tp[i] = convert_path_point_type(types[i]);
     }

+    poly_clip(graphics, pti, tp, count);
     PolyDraw(graphics->hdc, pti, tp, count);

status = Ok;
@@ -2153,53 +2340,6 @@ static GpStatus restore_container(GpGraphics* graphics,
     return Ok;
 }

-static GpStatus get_graphics_device_bounds(GpGraphics* graphics, GpRectF* rect)
-{
-    RECT wnd_rect;
-    GpStatus stat=Ok;
-    GpUnit unit;
-
-    if(graphics->hwnd) {
-        if(!GetClientRect(graphics->hwnd, &wnd_rect))
-            return GenericError;
-
-        rect->X = wnd_rect.left;
-        rect->Y = wnd_rect.top;
-        rect->Width = wnd_rect.right - wnd_rect.left;
-        rect->Height = wnd_rect.bottom - wnd_rect.top;
-    }else if (graphics->image){
-        stat = GdipGetImageBounds(graphics->image, rect, &unit);
-        if (stat == Ok && unit != UnitPixel)
-            FIXME("need to convert from unit %i\n", unit);
-    }else if (GetObjectType(graphics->hdc) == OBJ_MEMDC){
-        HBITMAP hbmp;
-        BITMAP bmp;
-
-        rect->X = 0;
-        rect->Y = 0;
-
-        hbmp = GetCurrentObject(graphics->hdc, OBJ_BITMAP);
-        if (hbmp && GetObjectW(hbmp, sizeof(bmp), &bmp))
-        {
-            rect->Width = bmp.bmWidth;
-            rect->Height = bmp.bmHeight;
-        }
-        else
-        {
-            /* FIXME: ??? */
-            rect->Width = 1;
-            rect->Height = 1;
-        }
-    }else{
-        rect->X = 0;
-        rect->Y = 0;
-        rect->Width = GetDeviceCaps(graphics->hdc, HORZRES);
-        rect->Height = GetDeviceCaps(graphics->hdc, VERTRES);
-    }
-
-    return stat;
-}
-
 static GpStatus get_graphics_bounds(GpGraphics* graphics, GpRectF* rect)
 {
     GpStatus stat = get_graphics_device_bounds(graphics, rect);

[Esme Povirk (＠madewokherd)]

Esme Povirk (@madewokherd) commented about dlls/gdiplus/graphics.c:
> -
>    rect->Height = GetDeviceCaps(graphics->hdc, VERTRES);
> - }
> -
> - return stat;
>
> +}
> -
>
> +static void poly_clip(GpGraphics *graphics, POINT *pti, BYTE *tp, int count)
> +{
> - RectF rect;
> - BYTE *tpcopy = heap_alloc_zero(count * sizeof(BYTE));
> -
> - memcpy(tpcopy, tp, count);
> - get_graphics_device_bounds(graphics, &rect);
> -
> - /* Ignore data that is not meant for drawing */
> - if (rect.Width == 1 && rect.Height == 1)

This doesn't seem right to me. A 1x1 image or window could still be valid.

[Esme Povirk (＠madewokherd)]

I don't think this will interact correctly with PT_CLOSEFIGURE. According to MSDN, PT_CLOSEFIGURE draws a line or bezier to the most recent PT_MOVETO points, so by changing points to PT_MOVETO, the behavior of PT_CLOSEFIGURE can be changed.

[Esme Povirk (＠madewokherd)]

Given some other similar bugs in other drawing operations, I think a better approach may be to optimize the conversion from GpRegion to HRGN to only convert the parts within a known bounding rectangle. We could then use SOFTWARE_GdipFillPath which would benefit from that. Unfortunately, that requires rewriting the conversion so it doesn't rely on gdi32.

[PATCH v5 0/6] MR3288: gdiplus: Clip polygons before drawing them

[David Kahurani (＠kahurani)]

On Fri Dec 15 14:18:24 2023 +0000, Esme Povirk wrote:
> Given some other similar bugs in other drawing operations, I think a
> better approach may be to optimize the conversion from GpRegion to HRGN
> to only convert the parts within a known bounding rectangle. We could
> then use SOFTWARE_GdipFillPath which would benefit from that.
> Unfortunately, that requires rewriting the conversion so it doesn't rely
> on gdi32.

Hello,

Sorry, got carried away with other things so yeah, haven't managed to continue working on this until recently.

It looks like it would be a great idea to convert this so as not to rely on gdi32 but it doesn't look like `SOFTWARE_GdipFillPath` is the right way to go. *FillPath methods will fill the path while `DrawPoly` only outlines.