[PATCH 0/2] MR10235: gdiplus: Validate window handle in has_gdi_dc
graphics->owndc being TRUE indicates that graphics is created using GdipCreateFromHWND. In has_gdi_dc, it is necessary to check whether the window has already been destroyed. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/10235
From: YeshunYe <yeyeshun@uniontech.com> Add a test case to verify the behavior of GdipMeasureString when the window associated with the graphics object is destroyed before the operation. Signed-off-by: YeshunYe <yeyeshun@uniontech.com> --- dlls/gdiplus/tests/graphics.c | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/dlls/gdiplus/tests/graphics.c b/dlls/gdiplus/tests/graphics.c index 96ef52ce935..bba7338973f 100644 --- a/dlls/gdiplus/tests/graphics.c +++ b/dlls/gdiplus/tests/graphics.c @@ -35,6 +35,7 @@ static GpStatus (WINAPI *pGdipGraphicsSetAbort)(GpGraphics*,GdiplusAbort*); static const REAL mm_per_inch = 25.4; static const REAL point_per_inch = 72.0; static HWND hwnd; +static HWND hwnd2; static void set_rect_empty(RectF *rc) { @@ -4629,6 +4630,7 @@ static void test_measure_string(void) CharacterRange range; GpRegion *region; GpGraphics *graphics; + GpGraphics *graphics2; GpFontFamily *family; GpFont *font; GpStatus status; @@ -4649,6 +4651,7 @@ static void test_measure_string(void) hdc = CreateCompatibleDC(0); status = GdipCreateFromHDC(hdc, &graphics); + status = GdipCreateFromHWND(hwnd2, &graphics2); status = GdipCreateFont(family, 20, FontStyleRegular, UnitPixel, &font); expect(Ok, status); @@ -5088,6 +5091,21 @@ static void test_measure_string(void) expect(3, glyphs); expect(1, lines); + DestroyWindow(hwnd2); + hwnd2 = NULL; + rect.X = 5.0; + rect.Y = 5.0; + rect.Width = width_M_M; + rect.Height = 32000.0; + status = GdipMeasureString(graphics2, string2, -1, font, &rect, format_no_wrap, &bounds, &glyphs, &lines); + todo_wine expect(Ok, status); + if (status == Ok) + { + expectf_(width_M_M, bounds.Width, 0.1); + expect(3, glyphs); + expect(1, lines); + } + status = GdipDeleteFont(font); expect(Ok, status); @@ -7553,6 +7571,9 @@ START_TEST(graphics) hwnd = CreateWindowA( "gdiplus_test", "graphics test", WS_OVERLAPPEDWINDOW | WS_VISIBLE, CW_USEDEFAULT, CW_USEDEFAULT, 200, 200, 0, 0, GetModuleHandleA(0), 0 ); ok(hwnd != NULL, "Expected window to be created\n"); + hwnd2 = CreateWindowA( "gdiplus_test", "test graphics after window destroy", WS_OVERLAPPEDWINDOW | WS_VISIBLE, + CW_USEDEFAULT, CW_USEDEFAULT, 200, 200, 0, 0, GetModuleHandleA(0), 0 ); + ok(hwnd2 != NULL, "Expected window to be created\n"); gdiplusStartupInput.GdiplusVersion = 1; gdiplusStartupInput.DebugEventCallback = NULL; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/10235
From: YeshunYe <yeyeshun@uniontech.com> graphics->owndc being TRUE indicates that graphics is created using `GdipCreateFromHWND`. In `has_gdi_dc`, it is necessary to check whether the window has already been destroyed. Signed-off-by: YeshunYe <yeyeshun@uniontech.com> --- dlls/gdiplus/gdiplus_private.h | 2 +- dlls/gdiplus/tests/graphics.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/dlls/gdiplus/gdiplus_private.h b/dlls/gdiplus/gdiplus_private.h index 9d0a166fdeb..a5e3c09251e 100644 --- a/dlls/gdiplus/gdiplus_private.h +++ b/dlls/gdiplus/gdiplus_private.h @@ -699,7 +699,7 @@ static inline void image_unlock(GpImage *image) static inline BOOL has_gdi_dc(GpGraphics *graphics) { - return graphics->hdc != NULL || graphics->owndc; + return graphics->hdc != NULL || (graphics->owndc && IsWindow(graphics->hwnd)); } static inline void set_rect(GpRectF *rect, REAL x, REAL y, REAL width, REAL height) diff --git a/dlls/gdiplus/tests/graphics.c b/dlls/gdiplus/tests/graphics.c index bba7338973f..3195698deaf 100644 --- a/dlls/gdiplus/tests/graphics.c +++ b/dlls/gdiplus/tests/graphics.c @@ -5098,7 +5098,7 @@ static void test_measure_string(void) rect.Width = width_M_M; rect.Height = 32000.0; status = GdipMeasureString(graphics2, string2, -1, font, &rect, format_no_wrap, &bounds, &glyphs, &lines); - todo_wine expect(Ok, status); + expect(Ok, status); if (status == Ok) { expectf_(width_M_M, bounds.Width, 0.1); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/10235
Esme Povirk (@madewokherd) commented about dlls/gdiplus/tests/graphics.c:
hdc = CreateCompatibleDC(0); status = GdipCreateFromHDC(hdc, &graphics); + status = GdipCreateFromHWND(hwnd2, &graphics2);
`graphics2` is never deleted. Also, the `status` value is never tested (which is also an issue with the existing code, but we might as well fix that while we're changing it). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/10235#note_131304
Esme Povirk (@madewokherd) commented about dlls/gdiplus/gdiplus_private.h:
static inline BOOL has_gdi_dc(GpGraphics *graphics) { - return graphics->hdc != NULL || graphics->owndc; + return graphics->hdc != NULL || (graphics->owndc && IsWindow(graphics->hwnd));
This would add a non-obvious corner case. We generally assume that there are only a few valid Graphics object types: Metafile recording, Bitmap, HWND, and HDC, and that has_gdi_dc divides them cleanly (Metafile/Bitmap do not have a DC, and HWND/HDC do). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/10235#note_131305
Esme Povirk (@madewokherd) commented about dlls/gdiplus/tests/graphics.c:
expect(3, glyphs); expect(1, lines);
+ DestroyWindow(hwnd2); + hwnd2 = NULL; + rect.X = 5.0; + rect.Y = 5.0; + rect.Width = width_M_M; + rect.Height = 32000.0; + status = GdipMeasureString(graphics2, string2, -1, font, &rect, format_no_wrap, &bounds, &glyphs, &lines); + todo_wine expect(Ok, status);
We should also test a drawing function. It's unclear whether native succeeds here because GdipMeasureString doesn't need to draw anything (and therefore in theory doesn't need to call GetDC), or because native is ignoring failures from it. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/10235#note_131306
On Wed Mar 4 17:45:53 2026 +0000, Esme Povirk wrote:
`graphics2` is never deleted. Also, the `status` value is never tested (which is also an issue with the existing code, but we might as well fix that while we're changing it). OK, when I improve this patch, I will add test code for the status.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/10235#note_131338
On Wed Mar 4 17:45:53 2026 +0000, Esme Povirk wrote:
This would add a non-obvious corner case. We generally assume that there are only a few valid Graphics object types: Metafile recording, Bitmap, HWND, and HDC, and that has_gdi_dc divides them cleanly (Metafile/Bitmap do not have a DC, and HWND/HDC do). Based on my observations, the code, upon successfully checking `has_gdi_dc`, will proceed to call `gdi_dc_acquire` to obtain an `hdc` without any further checks. However, if the `graphics` was created using `GdipCreateFromHWND` and the corresponding `hwnd` has already been destroyed, the `hdc` cannot be obtained. This would cause Wine's behavior to differ from Windows. For this reason, I chose to add an extra check within `has_gdi_dc`. Perhaps you have a better suggestion?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/10235#note_131339
On Wed Mar 4 17:45:53 2026 +0000, Esme Povirk wrote:
We should also test a drawing function. It's unclear whether native succeeds here because GdipMeasureString doesn't need to draw anything (and therefore in theory doesn't need to call GetDC), or because native is ignoring failures from it. That's also what I guessed—`GdipMeasureString` doesn't require actual rendering, so the test case passes on Windows. Regarding the drawing functions, since the window has already been destroyed in this test scenario, the drawing result is not important, so I didn't pay much attention to it. After this patch is completed, I will focus on the drawing functions.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/10235#note_131342
On Thu Mar 5 00:55:08 2026 +0000, Yeshun Ye wrote:
Based on my observations, the code, upon successfully checking `has_gdi_dc`, will proceed to call `gdi_dc_acquire` to obtain an `hdc` without any further checks. However, if the `graphics` was created using `GdipCreateFromHWND` and the corresponding `hwnd` has already been destroyed, the `hdc` cannot be obtained. This would cause Wine's behavior to differ from Windows. For this reason, I chose to add an extra check within `has_gdi_dc`. Perhaps you have a better suggestion? That's going to depend on whether the behavior of GdipMeasureString is typical.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/10235#note_131503
On Fri Mar 6 22:31:01 2026 +0000, Esme Povirk wrote:
That's going to depend on whether the behavior of GdipMeasureString is typical. After analyzing the code again, I agree that the `has_gdi_dc` function shouldn't be modified. Currently, this issue only affects `GdipMeasureString`, so I've decided to place the check inside `GdipMeasureString` instead.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/10235#note_131514
participants (3)
-
Esme Povirk (＠madewokherd) -
Yeshun Ye (＠yeyeshun)
-
YeshunYe