[PATCH] gdiplus: Handle some degenerate cases with GdipCreatePath2.
Signed-off-by: Jeff Smith <whydoubt(a)gmail.com> --- dlls/gdiplus/graphicspath.c | 22 ++++++++ dlls/gdiplus/tests/graphicspath.c | 87 +++++++++++++++++++++++++++++++ 2 files changed, 109 insertions(+) diff --git a/dlls/gdiplus/graphicspath.c b/dlls/gdiplus/graphicspath.c index 55545a449a..db011607e8 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) 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..2f6ed41339 100644 --- a/dlls/gdiplus/tests/graphicspath.c +++ b/dlls/gdiplus/tests/graphicspath.c @@ -25,6 +25,7 @@ #define expect(expected, got) ok(got == expected, "Expected %.8x, got %.8x\n", expected, got) #define expectf(expected, got) ok(fabs(expected - got) < 2.0, "Expected %.2f, got %.2f\n", expected, got) +#define expectp(expected, got) ok(got == expected, "Expected %p, got %p\n", expected, got) #define POINT_TYPE_MAX_LEN (75) static void stringify_point_type(PathPointType type, char * name) @@ -174,6 +175,91 @@ 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); + expectp(NULL, 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); + expectp(NULL, path); + if(path && path != (void *)0xdeadbeef) + GdipDeletePath(path); + + /* 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 +1749,7 @@ START_TEST(graphicspath) test_constructor_destructor(); test_getpathdata(); + test_createpath2(); test_line2(); test_arc(); test_worldbounds(); -- 2.23.0
On 4/9/20 10:52 PM, Jeff Smith wrote:
+ if(count <= 0) { + *path = NULL; + return OutOfMemory; + } Could this be a side effect of treating count as unsigned? By the way, actual allocations of point or types arrays are not checked.
#define expect(expected, got) ok(got == expected, "Expected %.8x, got %.8x\n", expected, got) #define expectf(expected, got) ok(fabs(expected - got) < 2.0, "Expected %.2f, got %.2f\n", expected, got) +#define expectp(expected, got) ok(got == expected, "Expected %p, got %p\n", expected, got) #define POINT_TYPE_MAX_LEN (75) In my opinion we should get rid of such macros, not add more. Especially for floating point tests.
On Thu, Apr 9, 2020 at 3:22 PM Nikolay Sivov <nsivov(a)codeweavers.com> wrote:
In my opinion we should get rid of such macros, not add more. Especially for floating point tests.
I think you're right in this case. There's no point in tracing the pointer when we already know it's non-NULL. But what's the problem with the existing macros?
On 4/10/20 4:33 AM, Esme (they/them) wrote:
On Thu, Apr 9, 2020 at 3:22 PM Nikolay Sivov <nsivov(a)codeweavers.com> wrote:
In my opinion we should get rid of such macros, not add more. Especially for floating point tests. I think you're right in this case. There's no point in tracing the pointer when we already know it's non-NULL.
But what's the problem with the existing macros? Not a problem that requires immediate attention, but something to consider. For example expectf() uses arbitrary absolute difference threshold, which isn't great in general, because it should depend on magnitude of values being compared. And expect() does not offer anything more than straight ok() call does.
On Thu, Apr 9, 2020 at 3:22 PM Nikolay Sivov <nsivov(a)codeweavers.com> wrote:
On 4/9/20 10:52 PM, Jeff Smith wrote:
+ if(count <= 0) { + *path = NULL; + return OutOfMemory; + } Could this be a side effect of treating count as unsigned? By the way, actual allocations of point or types arrays are not checked.
First, 0 results in the same behaviour as -1. Second, some hacky tests I did further suggest that it is being treated as signed. I've found no evidence to suggest it is being handled as unsigned.
#define expect(expected, got) ok(got == expected, "Expected %.8x, got %.8x\n", expected, got) #define expectf(expected, got) ok(fabs(expected - got) < 2.0, "Expected %.2f, got %.2f\n", expected, got) +#define expectp(expected, got) ok(got == expected, "Expected %p, got %p\n", expected, got) #define POINT_TYPE_MAX_LEN (75) In my opinion we should get rid of such macros, not add more. Especially for floating point tests.
I don't mind dropping this one. I only use it a few times anyway.
participants (3)
-
Esme (they/them) -
Jeff Smith -
Nikolay Sivov