[PATCH] gdiplus: Check return value of SelectClipPath in brush_fill_path().
For Crossover bug 16126. When GraphicPath is empty, filling path with gdi32 will result in a DC with empty path. When SelectClipPath() is called with such a DC, it will fail because it requires a closed path in DC. Thus further operation should be canceled. Signed-off-by: Zhiyi Zhang <zzhang(a)codeweavers.com> --- dlls/gdiplus/graphics.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/dlls/gdiplus/graphics.c b/dlls/gdiplus/graphics.c index 76aabe74bf..2a95d686fa 100644 --- a/dlls/gdiplus/graphics.c +++ b/dlls/gdiplus/graphics.c @@ -1052,6 +1052,7 @@ static BOOL brush_can_fill_path(GpBrush *brush, BOOL is_fill) static void brush_fill_path(GpGraphics *graphics, GpBrush* brush) { + BOOL success; switch (brush->bt) { case BrushTypeSolidColor: @@ -1064,8 +1065,8 @@ static void brush_fill_path(GpGraphics *graphics, GpBrush* brush) RECT rc; /* partially transparent fill */ - SelectClipPath(graphics->hdc, RGN_AND); - if (GetClipBox(graphics->hdc, &rc) != NULLREGION) + success = SelectClipPath(graphics->hdc, RGN_AND); + if (success && GetClipBox(graphics->hdc, &rc) != NULLREGION) { HDC hdc = CreateCompatibleDC(NULL); -- 2.17.1
It looks like we also encountered this case in get_path_hrgn. Maybe we shouldn't call brush_fill_pixels if the path is empty? If we're going to ignore errors, I think we should at least check GetLastError() to make sure it's one we expect. On Fri, Jun 15, 2018 at 4:04 AM, Zhiyi Zhang <zzhang(a)codeweavers.com> wrote:
For Crossover bug 16126.
When GraphicPath is empty, filling path with gdi32 will result in a DC with empty path. When SelectClipPath() is called with such a DC, it will fail because it requires a closed path in DC. Thus further operation should be canceled.
Signed-off-by: Zhiyi Zhang <zzhang(a)codeweavers.com> --- dlls/gdiplus/graphics.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/dlls/gdiplus/graphics.c b/dlls/gdiplus/graphics.c index 76aabe74bf..2a95d686fa 100644 --- a/dlls/gdiplus/graphics.c +++ b/dlls/gdiplus/graphics.c @@ -1052,6 +1052,7 @@ static BOOL brush_can_fill_path(GpBrush *brush, BOOL is_fill)
static void brush_fill_path(GpGraphics *graphics, GpBrush* brush) { + BOOL success; switch (brush->bt) { case BrushTypeSolidColor: @@ -1064,8 +1065,8 @@ static void brush_fill_path(GpGraphics *graphics, GpBrush* brush) RECT rc; /* partially transparent fill */
- SelectClipPath(graphics->hdc, RGN_AND); - if (GetClipBox(graphics->hdc, &rc) != NULLREGION) + success = SelectClipPath(graphics->hdc, RGN_AND); + if (success && GetClipBox(graphics->hdc, &rc) != NULLREGION) { HDC hdc = CreateCompatibleDC(NULL);
-- 2.17.1
Hi Vincent, Yes, we could check if a path is empty just like get_path_hrgn did. I also worried that a not empty but open path will also make SelectClipPath() fail. Is there any function to check if a path is closed? I couldn't found any. As for SelectClipPath() last error, I don't think any further operation should be continued if any error occurs. So we add a check for empty path and also skip further operation if SelectClipPath() returns any error. Is that enough? Thanks, Zhiyi On Fri 6 15 23:55, Vincent Povirk wrote:
It looks like we also encountered this case in get_path_hrgn. Maybe we shouldn't call brush_fill_pixels if the path is empty? If we're going to ignore errors, I think we should at least check GetLastError() to make sure it's one we expect.
On Fri, Jun 15, 2018 at 4:04 AM, Zhiyi Zhang <zzhang(a)codeweavers.com> wrote:
For Crossover bug 16126.
When GraphicPath is empty, filling path with gdi32 will result in a DC with empty path. When SelectClipPath() is called with such a DC, it will fail because it requires a closed path in DC. Thus further operation should be canceled.
Signed-off-by: Zhiyi Zhang <zzhang(a)codeweavers.com> --- dlls/gdiplus/graphics.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/dlls/gdiplus/graphics.c b/dlls/gdiplus/graphics.c index 76aabe74bf..2a95d686fa 100644 --- a/dlls/gdiplus/graphics.c +++ b/dlls/gdiplus/graphics.c @@ -1052,6 +1052,7 @@ static BOOL brush_can_fill_path(GpBrush *brush, BOOL is_fill)
static void brush_fill_path(GpGraphics *graphics, GpBrush* brush) { + BOOL success; switch (brush->bt) { case BrushTypeSolidColor: @@ -1064,8 +1065,8 @@ static void brush_fill_path(GpGraphics *graphics, GpBrush* brush) RECT rc; /* partially transparent fill */
- SelectClipPath(graphics->hdc, RGN_AND); - if (GetClipBox(graphics->hdc, &rc) != NULLREGION) + success = SelectClipPath(graphics->hdc, RGN_AND); + if (success && GetClipBox(graphics->hdc, &rc) != NULLREGION) { HDC hdc = CreateCompatibleDC(NULL);
-- 2.17.1
If converting a path to a region fails for an open path, then our code in get_path_hrgn is also wrong. From reading gdi32 code, think it will work as long as there are points in the path, but the only way to be sure is to add a test. If we check for failure in SelectClipPath, I agree that we should not continue the operation for any error. I think we should report failure to the caller if it's an error we don't expect. That would also help explain why it's correct to silently ignore the failure. On Sat, Jun 16, 2018 at 9:31 AM, Zhiyi Zhang <zzhang(a)codeweavers.com> wrote:
Hi Vincent,
Yes, we could check if a path is empty just like get_path_hrgn did. I also worried that a not empty but open path will also make SelectClipPath() fail. Is there any function to check if a path is closed? I couldn't found any. As for SelectClipPath() last error, I don't think any further operation should be continued if any error occurs. So we add a check for empty path and also skip further operation if SelectClipPath() returns any error. Is that enough?
Thanks, Zhiyi
On Fri 6 15 23:55, Vincent Povirk wrote:
It looks like we also encountered this case in get_path_hrgn. Maybe we shouldn't call brush_fill_pixels if the path is empty? If we're going to ignore errors, I think we should at least check GetLastError() to make sure it's one we expect.
On Fri, Jun 15, 2018 at 4:04 AM, Zhiyi Zhang <zzhang(a)codeweavers.com> wrote:
For Crossover bug 16126.
When GraphicPath is empty, filling path with gdi32 will result in a DC with empty path. When SelectClipPath() is called with such a DC, it will fail because it requires a closed path in DC. Thus further operation should be canceled.
Signed-off-by: Zhiyi Zhang <zzhang(a)codeweavers.com> --- dlls/gdiplus/graphics.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/dlls/gdiplus/graphics.c b/dlls/gdiplus/graphics.c index 76aabe74bf..2a95d686fa 100644 --- a/dlls/gdiplus/graphics.c +++ b/dlls/gdiplus/graphics.c @@ -1052,6 +1052,7 @@ static BOOL brush_can_fill_path(GpBrush *brush, BOOL is_fill)
static void brush_fill_path(GpGraphics *graphics, GpBrush* brush) { + BOOL success; switch (brush->bt) { case BrushTypeSolidColor: @@ -1064,8 +1065,8 @@ static void brush_fill_path(GpGraphics *graphics, GpBrush* brush) RECT rc; /* partially transparent fill */
- SelectClipPath(graphics->hdc, RGN_AND); - if (GetClipBox(graphics->hdc, &rc) != NULLREGION) + success = SelectClipPath(graphics->hdc, RGN_AND); + if (success && GetClipBox(graphics->hdc, &rc) != NULLREGION) { HDC hdc = CreateCompatibleDC(NULL);
-- 2.17.1
Thanks. I'll work on it as you suggest. On Sat 6 16 23:26, Vincent Povirk wrote:
If converting a path to a region fails for an open path, then our code in get_path_hrgn is also wrong. From reading gdi32 code, think it will work as long as there are points in the path, but the only way to be sure is to add a test.
If we check for failure in SelectClipPath, I agree that we should not continue the operation for any error. I think we should report failure to the caller if it's an error we don't expect. That would also help explain why it's correct to silently ignore the failure.
On Sat, Jun 16, 2018 at 9:31 AM, Zhiyi Zhang <zzhang(a)codeweavers.com> wrote:
Hi Vincent,
Yes, we could check if a path is empty just like get_path_hrgn did. I also worried that a not empty but open path will also make SelectClipPath() fail. Is there any function to check if a path is closed? I couldn't found any. As for SelectClipPath() last error, I don't think any further operation should be continued if any error occurs. So we add a check for empty path and also skip further operation if SelectClipPath() returns any error. Is that enough?
Thanks, Zhiyi
On Fri 6 15 23:55, Vincent Povirk wrote:
It looks like we also encountered this case in get_path_hrgn. Maybe we shouldn't call brush_fill_pixels if the path is empty? If we're going to ignore errors, I think we should at least check GetLastError() to make sure it's one we expect.
On Fri, Jun 15, 2018 at 4:04 AM, Zhiyi Zhang <zzhang(a)codeweavers.com> wrote:
For Crossover bug 16126.
When GraphicPath is empty, filling path with gdi32 will result in a DC with empty path. When SelectClipPath() is called with such a DC, it will fail because it requires a closed path in DC. Thus further operation should be canceled.
Signed-off-by: Zhiyi Zhang <zzhang(a)codeweavers.com> --- dlls/gdiplus/graphics.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/dlls/gdiplus/graphics.c b/dlls/gdiplus/graphics.c index 76aabe74bf..2a95d686fa 100644 --- a/dlls/gdiplus/graphics.c +++ b/dlls/gdiplus/graphics.c @@ -1052,6 +1052,7 @@ static BOOL brush_can_fill_path(GpBrush *brush, BOOL is_fill)
static void brush_fill_path(GpGraphics *graphics, GpBrush* brush) { + BOOL success; switch (brush->bt) { case BrushTypeSolidColor: @@ -1064,8 +1065,8 @@ static void brush_fill_path(GpGraphics *graphics, GpBrush* brush) RECT rc; /* partially transparent fill */
- SelectClipPath(graphics->hdc, RGN_AND); - if (GetClipBox(graphics->hdc, &rc) != NULLREGION) + success = SelectClipPath(graphics->hdc, RGN_AND); + if (success && GetClipBox(graphics->hdc, &rc) != NULLREGION) { HDC hdc = CreateCompatibleDC(NULL);
-- 2.17.1
Hi Vincent, I tested open paths for path to region function manually. It works on Windows and Wine. So we have to consider empty path on Wine only. In case any error happens in SelectClipPath(), further operation is canceled and error is reported to the caller. V2 is sent. Thanks, Zhiyi On Sat 6 16 23:26, Vincent Povirk wrote:
If converting a path to a region fails for an open path, then our code in get_path_hrgn is also wrong. From reading gdi32 code, think it will work as long as there are points in the path, but the only way to be sure is to add a test.
If we check for failure in SelectClipPath, I agree that we should not continue the operation for any error. I think we should report failure to the caller if it's an error we don't expect. That would also help explain why it's correct to silently ignore the failure.
On Sat, Jun 16, 2018 at 9:31 AM, Zhiyi Zhang <zzhang(a)codeweavers.com> wrote:
Hi Vincent,
Yes, we could check if a path is empty just like get_path_hrgn did. I also worried that a not empty but open path will also make SelectClipPath() fail. Is there any function to check if a path is closed? I couldn't found any. As for SelectClipPath() last error, I don't think any further operation should be continued if any error occurs. So we add a check for empty path and also skip further operation if SelectClipPath() returns any error. Is that enough?
Thanks, Zhiyi
On Fri 6 15 23:55, Vincent Povirk wrote:
It looks like we also encountered this case in get_path_hrgn. Maybe we shouldn't call brush_fill_pixels if the path is empty? If we're going to ignore errors, I think we should at least check GetLastError() to make sure it's one we expect.
On Fri, Jun 15, 2018 at 4:04 AM, Zhiyi Zhang <zzhang(a)codeweavers.com> wrote:
For Crossover bug 16126.
When GraphicPath is empty, filling path with gdi32 will result in a DC with empty path. When SelectClipPath() is called with such a DC, it will fail because it requires a closed path in DC. Thus further operation should be canceled.
Signed-off-by: Zhiyi Zhang <zzhang(a)codeweavers.com> --- dlls/gdiplus/graphics.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/dlls/gdiplus/graphics.c b/dlls/gdiplus/graphics.c index 76aabe74bf..2a95d686fa 100644 --- a/dlls/gdiplus/graphics.c +++ b/dlls/gdiplus/graphics.c @@ -1052,6 +1052,7 @@ static BOOL brush_can_fill_path(GpBrush *brush, BOOL is_fill)
static void brush_fill_path(GpGraphics *graphics, GpBrush* brush) { + BOOL success; switch (brush->bt) { case BrushTypeSolidColor: @@ -1064,8 +1065,8 @@ static void brush_fill_path(GpGraphics *graphics, GpBrush* brush) RECT rc; /* partially transparent fill */
- SelectClipPath(graphics->hdc, RGN_AND); - if (GetClipBox(graphics->hdc, &rc) != NULLREGION) + success = SelectClipPath(graphics->hdc, RGN_AND); + if (success && GetClipBox(graphics->hdc, &rc) != NULLREGION) { HDC hdc = CreateCompatibleDC(NULL);
-- 2.17.1
participants (3)
-
Vincent Povirk -
Zhiyi Zhang -
Zhiyi Zhang