[PATCH v2 0/2] MR3461: gdiplus: Reject zero-width/zero-height rectangles
Works around the issue manifesting here : https://bugs.winehq.org/show_bug.cgi?id=55351 The MR also contains a patch that refactors the related tests a bit. Signed-off-by: David Kahurani k.kahurani(a)gmail.com -- v2: gdiplus: Move Flatten() tests into the same function gdiplus: Reject zero-width/zero-height rectangles https://gitlab.winehq.org/wine/wine/-/merge_requests/3461
From: David Kahurani <k.kahurani(a)gmail.com> Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=55351 Signed-off-by: David Kahurani <k.kahurani(a)gmail.com> --- dlls/gdiplus/graphicspath.c | 20 ++++++++++---------- dlls/gdiplus/tests/graphicspath.c | 4 +++- 2 files changed, 13 insertions(+), 11 deletions(-) diff --git a/dlls/gdiplus/graphicspath.c b/dlls/gdiplus/graphicspath.c index 545aaa8441e..bae5d858add 100644 --- a/dlls/gdiplus/graphicspath.c +++ b/dlls/gdiplus/graphicspath.c @@ -283,10 +283,10 @@ static GpStatus extend_current_figure(GpPath *path, GDIPCONST PointF *points, IN * * PARAMS * path [I/O] Path that the arc is appended to - * x1 [I] X coordinate of the boundary box - * y1 [I] Y coordinate of the boundary box - * x2 [I] Width of the boundary box - * y2 [I] Height of the boundary box + * x [I] X coordinate of the boundary rectangle + * y [I] Y coordinate of the boundary rectangle + * width [I] Width of the boundary rectangle + * height [I] Height of the boundary rectangle * startAngle [I] Starting angle of the arc, clockwise * sweepAngle [I] Angle of the arc, clockwise * @@ -302,20 +302,20 @@ static GpStatus extend_current_figure(GpPath *path, GDIPCONST PointF *points, IN * In both cases, the value of newfigure of the given path is FALSE * afterwards. */ -GpStatus WINGDIPAPI GdipAddPathArc(GpPath *path, REAL x1, REAL y1, REAL x2, - REAL y2, REAL startAngle, REAL sweepAngle) +GpStatus WINGDIPAPI GdipAddPathArc(GpPath *path, REAL x, REAL y, REAL width, + REAL height, REAL startAngle, REAL sweepAngle) { GpPointF *points; GpStatus status; INT count; TRACE("(%p, %.2f, %.2f, %.2f, %.2f, %.2f, %.2f)\n", - path, x1, y1, x2, y2, startAngle, sweepAngle); + path, x, y, width, height, startAngle, sweepAngle); - if(!path) + if(!path || !width || !height) return InvalidParameter; - count = arc2polybezier(NULL, x1, y1, x2, y2, startAngle, sweepAngle); + count = arc2polybezier(NULL, x, y, width, height, startAngle, sweepAngle); if(count == 0) return Ok; @@ -323,7 +323,7 @@ GpStatus WINGDIPAPI GdipAddPathArc(GpPath *path, REAL x1, REAL y1, REAL x2, if(!points) return OutOfMemory; - arc2polybezier(points, x1, y1, x2, y2, startAngle, sweepAngle); + arc2polybezier(points, x, y, width, height, startAngle, sweepAngle); status = extend_current_figure(path, points, count, PathPointTypeBezier); diff --git a/dlls/gdiplus/tests/graphicspath.c b/dlls/gdiplus/tests/graphicspath.c index 558eedc147f..aa9efc8e788 100644 --- a/dlls/gdiplus/tests/graphicspath.c +++ b/dlls/gdiplus/tests/graphicspath.c @@ -451,6 +451,8 @@ static void test_arc(void) GpPath* path; GdipCreatePath(FillModeAlternate, &path); + status = GdipAddPathArc(path, 100.0, 100.0, 0.0, 0.0, 0.0, 90.0); + expect(InvalidParameter, status); /* Exactly 90 degrees */ status = GdipAddPathArc(path, 100.0, 100.0, 500.0, 700.0, 0.0, 90.0); expect(Ok, status); @@ -1266,7 +1268,7 @@ static void test_flatten2(void) /* path seen in the wild that caused a stack overflow */ status = GdipAddPathArc(path, -136.33, 20.00, 786.00, 786.00, -105.00, 30.00); expect(Ok, status); - status = GdipAddPathArc(path, 256.67, 413.00, 0.00, 0.00, -75.00, -30.00); + status = GdipAddPathArc(path, 256.67, 413.00, 1.00, 1.00, -75.00, -30.00); expect(Ok, status); status = GdipClosePathFigure(path); expect(Ok, status); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3461
From: David Kahurani <k.kahurani(a)gmail.com> Signed-off-by: David Kahurani <k.kahurani(a)gmail.com> --- dlls/gdiplus/tests/graphicspath.c | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-) diff --git a/dlls/gdiplus/tests/graphicspath.c b/dlls/gdiplus/tests/graphicspath.c index aa9efc8e788..8600723f404 100644 --- a/dlls/gdiplus/tests/graphicspath.c +++ b/dlls/gdiplus/tests/graphicspath.c @@ -1251,16 +1251,7 @@ static void test_flatten(void) expect(Ok, status); ok_path(path, flattenquater_path, ARRAY_SIZE(flattenquater_path), FALSE); - GdipDeleteMatrix(m); - GdipDeletePath(path); -} - -static void test_flatten2(void) -{ - GpStatus status; - GpPath *path; - - status = GdipCreatePath(0, &path); + status = GdipResetPath(path); expect(Ok, status); status = GdipStartPathFigure(path); expect(Ok, status); @@ -1287,7 +1278,9 @@ static void test_flatten2(void) status = GdipClosePathFigure(path); expect(Ok, status); status = GdipFlattenPath(path, NULL, 1.0); + expect(Ok, status); + GdipDeleteMatrix(m); GdipDeletePath(path); } @@ -1973,7 +1966,6 @@ START_TEST(graphicspath) test_widen_cap(); test_isvisible(); test_empty_rect(); - test_flatten2(); GdiplusShutdown(gdiplusToken); } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3461
Jeffrey Smith (@whydoubt) commented about dlls/gdiplus/tests/graphicspath.c:
GpPath* path;
GdipCreatePath(FillModeAlternate, &path); + status = GdipAddPathArc(path, 100.0, 100.0, 0.0, 0.0, 0.0, 90.0); + expect(InvalidParameter, status);
A couple of improvements I would suggest: 1. Separate into zero-width vs zero-height tests. 2. Test negative width/height as well, to see whether 0 is a special case (error on value != 0) vs a boundary (error on value <= 0). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3461#note_40831
Jeffrey Smith (@whydoubt) commented about dlls/gdiplus/tests/graphicspath.c:
/* path seen in the wild that caused a stack overflow */ status = GdipAddPathArc(path, -136.33, 20.00, 786.00, 786.00, -105.00, 30.00); expect(Ok, status); - status = GdipAddPathArc(path, 256.67, 413.00, 0.00, 0.00, -75.00, -30.00); + status = GdipAddPathArc(path, 256.67, 413.00, 1.00, 1.00, -75.00, -30.00);
Since this was a recent addition, I wonder if it is known whether the original values here are critical to exercising the special case this was written for. @DarkShadow44 -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3461#note_40832
On Mon Jul 31 15:32:25 2023 +0000, Jeffrey Smith wrote:
Since this was a recent addition, I wonder if it is known whether the original values here are critical to exercising the special case this was written for. @DarkShadow44 Thanks for the ping, I wasn't even aware of the issue.
I did some testing, and the real critical part is the second part of the test: ```c /* path seen in the wild that caused a stack overflow */ /* same path but redo with the manual points that caused a crash */ status = GdipResetPath(path); expect(Ok, status); status = GdipAddPathBezier(path, 154.950806, 33.391144, 221.586075, 15.536285, 291.747314, 15.536285, 358.382568, 33.391144); expect(Ok, status); status = GdipAddPathBezier(path, 256.666809, 412.999512, 256.666718, 412.999481, 256.666656, 412.999481, 256.666565, 412.999512); expect(Ok, status); status = GdipClosePathFigure(path); expect(Ok, status); status = GdipFlattenPath(path, NULL, 1.0); ``` The first part of the flatten2 test was just a duplicate test for the same issue, it could be removed as well. Since it doesn't work on windows (sorry for that btw) I think that would be the better idea. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3461#note_40837
On Mon Jul 31 15:24:34 2023 +0000, Jeffrey Smith wrote:
A couple of improvements I would suggest: 1. Separate into zero-width vs zero-height tests. 2. Test negative width/height as well, to see whether 0 is a special case (error on value != 0) vs a boundary (error on value <= 0). From what I know about gdiplus, negative width/height would just mean that the rectangle is to the left or right, respectively of it's origin(x,y in this particular case) so I didn't really bother testing that.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3461#note_40839
On Mon Jul 31 15:59:36 2023 +0000, Fabian Maurer wrote:
Thanks for the ping, I wasn't even aware of the issue. I did some testing, and the real critical part is the second part of the test: ```c /* path seen in the wild that caused a stack overflow */ /* same path but redo with the manual points that caused a crash */ status = GdipResetPath(path); expect(Ok, status); status = GdipAddPathBezier(path, 154.950806, 33.391144, 221.586075, 15.536285, 291.747314, 15.536285, 358.382568, 33.391144); expect(Ok, status); status = GdipAddPathBezier(path, 256.666809, 412.999512, 256.666718, 412.999481, 256.666656, 412.999481, 256.666565, 412.999512); expect(Ok, status); status = GdipClosePathFigure(path); expect(Ok, status); status = GdipFlattenPath(path, NULL, 1.0); ``` The first part of the flatten2 test was just a duplicate test for the same issue, it could be removed as well. Since it doesn't work on windows (sorry for that btw) I think that would be the better idea. It looks to me like the original tests are just trying to pass in the smallest rectangle possible. I used `1.0` for the width and height since that is the default pen width.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3461#note_40840
On Mon Jul 31 16:20:00 2023 +0000, David Kahurani wrote:
It looks to me like the original tests are just trying to pass in the smallest rectangle possible. I used `1.0` for the width and height since that is the default pen width. Removing a test because it doesn't work doesn't sound the like the best idea to me... Neither does changing it. But since I added the test, I do think I know it can be removed. There is two tests testing the same thing, but one is broken, so we can remove the broken one. If you just change the values, it doesn't have the same effect anyways.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3461#note_40842
On Mon Jul 31 16:27:28 2023 +0000, Fabian Maurer wrote:
Neither does changing it. But since I added the test, I do think I know it can be removed. There is two tests testing the same thing, but one is broken, so we can remove the broken one. If you just change the values, it doesn't have the same effect anyways. I clearly explained the reasoning behind my changing my tests but if you insist, I will remove it.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3461#note_40843
On Mon Jul 31 16:32:08 2023 +0000, David Kahurani wrote:
I clearly explained the reasoning behind my changing my tests but if you insist, I will remove it. I mean, we could also keep it around, although I personally don't see the point in a test that doesn't test what was made to test.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3461#note_40844
On Mon Jul 31 16:34:03 2023 +0000, Fabian Maurer wrote:
I mean, we could also keep it around, although I personally don't see the point in a test that doesn't test what was made to test. Nothing wrong the reasoning, but there is more information now. I agree that the AddPathArc version of the test may be removed. It appears to have no value given the more precise test. And as for the cause of the broken test this MR is addressing, that is better handled by the new test for zero-width/zero-height.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3461#note_40845
participants (4)
-
David Kahurani -
David Kahurani (＠kahurani)
-
Fabian Maurer (＠DarkShadow44)
-
Jeffrey Smith (＠whydoubt)