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@gmail.com
-- v2: gdiplus: Move Flatten() tests into the same function gdiplus: Reject zero-width/zero-height rectangles
From: David Kahurani k.kahurani@gmail.com
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=55351 Signed-off-by: David Kahurani k.kahurani@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);
From: David Kahurani k.kahurani@gmail.com
Signed-off-by: David Kahurani k.kahurani@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); }
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).
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
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.
On Mon Jul 31 15:24:34 2023 +0000, Jeffrey Smith wrote:
A couple of improvements I would suggest:
- Separate into zero-width vs zero-height tests.
- 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.
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:
/* 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.
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.
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.
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.
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.
-
David Kahurani -
David Kahurani (＠kahurani)
-
Fabian Maurer (＠DarkShadow44)
-
Jeffrey Smith (＠whydoubt)