[PATCH v2] gdiplus: Handle some degenerate cases with GdipCreatePath2.

[Jeff Smith]

Signed-off-by: Jeff Smith <whydoubt(a)gmail.com>
---
v2:
- Check if points and types parameters are valid,
    and add some tests to exercise these.
- Got rid of expectp macro.

 dlls/gdiplus/graphicspath.c       |  24 ++++++-
 dlls/gdiplus/tests/graphicspath.c | 103 ++++++++++++++++++++++++++++++
 2 files changed, 126 insertions(+), 1 deletion(-)

diff --git a/dlls/gdiplus/graphicspath.c b/dlls/gdiplus/graphicspath.c
index 55545a449a..df6a32d22d 100644
--- a/dlls/gdiplus/graphicspath.c
+++ b/dlls/gdiplus/graphicspath.c
@@ -1213,14 +1213,34 @@ GpStatus WINGDIPAPI GdipCreatePath(GpFillMode fill, GpPath **path)
 GpStatus WINGDIPAPI GdipCreatePath2(GDIPCONST GpPointF* points,
     GDIPCONST BYTE* types, INT count, GpFillMode fill, GpPath **path)
 {
+    int i;
+
     TRACE("(%p, %p, %d, %d, %p)\n", points, types, count, fill, path);
 
-    if(!path)
+    if(!points || !types || !path)
         return InvalidParameter;
 
+    if(count <= 0) {
+        *path = NULL;
+        return OutOfMemory;
+    }
+
     *path = heap_alloc_zero(sizeof(GpPath));
     if(!*path)  return OutOfMemory;
 
+    for(i = 1; i < count; i++) {
+        if((types[i] & PathPointTypePathTypeMask) == PathPointTypeBezier) {
+            if(i+2 < count &&
+                    (types[i+1] & PathPointTypePathTypeMask) == PathPointTypeBezier &&
+                    (types[i+2] & PathPointTypePathTypeMask) == PathPointTypeBezier)
+                i += 2;
+            else {
+                count = 0;
+                break;
+            }
+        }
+    }
+
     (*path)->pathdata.Points = heap_alloc_zero(count * sizeof(PointF));
     (*path)->pathdata.Types = heap_alloc_zero(count);
 
@@ -1233,6 +1253,8 @@ GpStatus WINGDIPAPI GdipCreatePath2(GDIPCONST GpPointF* points,
 
     memcpy((*path)->pathdata.Points, points, count * sizeof(PointF));
     memcpy((*path)->pathdata.Types, types, count);
+    if(count > 0)
+        (*path)->pathdata.Types[0] = PathPointTypeStart;
     (*path)->pathdata.Count = count;
     (*path)->datalen = count;
 
diff --git a/dlls/gdiplus/tests/graphicspath.c b/dlls/gdiplus/tests/graphicspath.c
index 17e6a7cafb..1b3a9a161f 100644
--- a/dlls/gdiplus/tests/graphicspath.c
+++ b/dlls/gdiplus/tests/graphicspath.c
@@ -174,6 +174,108 @@ static void test_getpathdata(void)
     GdipDeletePath(path);
 }
 
+static void test_createpath2(void)
+{
+    GpStatus status;
+    GpPath* path = NULL;
+    GpPathData data;
+    INT i, count, expect_count;
+
+    PointF test_line_points[] = {{1.0,1.0}, {2.0,1.0}};
+    BYTE test_line_types[] = {PathPointTypeStart, PathPointTypeLine};
+
+    PointF test_bez_points[] = {{1.0,1.0}, {2.0,1.0}, {3.0,1.0}, {4.0,1.0},
+            {5.0,1.0}, {6.0,1.0}, {7.0,1.0}};
+    BYTE test_bez_types[] = {PathPointTypeStart, PathPointTypeBezier,
+            PathPointTypeBezier, PathPointTypeBezier, PathPointTypeBezier,
+            PathPointTypeBezier, PathPointTypeBezier};
+
+    status = GdipCreatePath2(test_line_points, test_line_types, 2, FillModeAlternate, &path);
+    expect(Ok, status);
+    status = GdipGetPointCount(path, &count);
+    expect(Ok, status);
+    expect(2, count);
+    GdipDeletePath(path);
+
+    status = GdipCreatePath2(test_line_points, test_line_types, 1, FillModeAlternate, &path);
+    expect(Ok, status);
+    status = GdipGetPointCount(path, &count);
+    expect(Ok, status);
+    expect(1, count);
+    GdipDeletePath(path);
+
+    path = (void *)0xdeadbeef;
+    status = GdipCreatePath2(test_line_points, test_line_types, 0, FillModeAlternate, &path);
+    expect(OutOfMemory, status);
+    ok(!path, "Expected NULL, got %p\n", path);
+    if(path && path != (void *)0xdeadbeef)
+        GdipDeletePath(path);
+
+    path = (void *)0xdeadbeef;
+    status = GdipCreatePath2(test_line_points, test_line_types, -1, FillModeAlternate, &path);
+    expect(OutOfMemory, status);
+    ok(!path, "Expected NULL, got %p\n", path);
+    if(path && path != (void *)0xdeadbeef)
+        GdipDeletePath(path);
+
+    path = (void *)0xdeadbeef;
+    status = GdipCreatePath2(NULL, test_line_types, 2, FillModeAlternate, &path);
+    expect(InvalidParameter, status);
+    ok(path == (void *)0xdeadbeef, "Expected %p, got %p\n", (void *)0xdeadbeef, path);
+    if(path && path != (void *)0xdeadbeef)
+        GdipDeletePath(path);
+
+    path = (void *)0xdeadbeef;
+    status = GdipCreatePath2(test_line_points, NULL, 2, FillModeAlternate, &path);
+    expect(InvalidParameter, status);
+    ok(path == (void *)0xdeadbeef, "Expected %p, got %p\n", (void *)0xdeadbeef, path);
+    if(path && path != (void *)0xdeadbeef)
+        GdipDeletePath(path);
+
+    status = GdipCreatePath2(test_line_points, test_line_types, 2, FillModeAlternate, NULL);
+    expect(InvalidParameter, status);
+
+    /* Zero-length line points do not get altered */
+    path = NULL;
+    test_line_points[1].X = test_line_points[0].X;
+    test_line_points[1].Y = test_line_points[0].Y;
+    status = GdipCreatePath2(test_line_points, test_line_types, 2, FillModeAlternate, &path);
+    expect(Ok, status);
+    status = GdipGetPointCount(path, &count);
+    expect(Ok, status);
+    expect(2, count);
+    GdipDeletePath(path);
+
+    /* The type of the first point is always converted to PathPointTypeStart */
+    test_line_types[0] = PathPointTypeLine;
+    status = GdipCreatePath2(test_line_points, test_line_types, 1, FillModeAlternate, &path);
+    expect(Ok, status);
+    status = GdipGetPointCount(path, &count);
+    expect(Ok, status);
+    expect(1, count);
+    data.Count  = count;
+    data.Types  = GdipAlloc(sizeof(BYTE) * count);
+    data.Points = GdipAlloc(sizeof(PointF) * count);
+    status = GdipGetPathData(path, &data);
+    expect(Ok, status);
+    expect((data.Points[0].X == 1.0) && (data.Points[0].Y == 1.0), TRUE);
+    expect(data.Types[0], PathPointTypeStart);
+    GdipFree(data.Points);
+    GdipFree(data.Types);
+    GdipDeletePath(path);
+
+    /* Bezier points must come in groups of three */
+    for(i = 2; i <= 7; i++) {
+        expect_count = (i % 3 == 1) ? i : 0;
+        status = GdipCreatePath2(test_bez_points, test_bez_types, i, FillModeAlternate, &path);
+        expect(Ok, status);
+        status = GdipGetPointCount(path, &count);
+        expect(Ok, status);
+        expect(expect_count, count);
+        GdipDeletePath(path);
+    }
+}
+
 static path_test_t line2_path[] = {
     {0.0, 50.0, PathPointTypeStart, 0, 0}, /*0*/
     {5.0, 45.0, PathPointTypeLine, 0, 0}, /*1*/
@@ -1663,6 +1765,7 @@ START_TEST(graphicspath)
 
     test_constructor_destructor();
     test_getpathdata();
+    test_createpath2();
     test_line2();
     test_arc();
     test_worldbounds();
-- 
2.23.0

[Esme (they/them)]

Signed-off-by: Vincent Povirk <vincent(a)codeweavers.com>

It seems like you're going out of your way to make the tests robust. I
don't think that's necessary. It's ok if the tests crash when things
fail in ways that aren't known to happen on any existing
implementation.

[Jeff Smith]

On Fri, Apr 10, 2020 at 10:20 AM Esme (they/them)
<vincent(a)codeweavers.com> wrote:
> Signed-off-by: Vincent Povirk <vincent(a)codeweavers.com>
>
> It seems like you're going out of your way to make the tests robust. I
> don't think that's necessary. It's ok if the tests crash when things
> fail in ways that aren't known to happen on any existing
> implementation.

Yes, perhaps I am.  Sometimes it's hard to strike the right balance.

Though if I was REALLY going out of my way to be robust, I would
be setting path before EVERY time I called GdipCreatePath2, instead
of just around the times I am interested in the expected failure.