[PATCH 0/4] MR7183: gdi32: Fix 64 bit EMREXTCREATEPEN.
From: Stefan Dösinger <stefan(a)codeweavers.com> The application in question doesn't care since it is at the end of a struct, but the behavior is consistent and adding it is easier than making our tests check for it. --- dlls/gdi32/emfdc.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/dlls/gdi32/emfdc.c b/dlls/gdi32/emfdc.c index e48da848a96..2fd4f72f9cc 100644 --- a/dlls/gdi32/emfdc.c +++ b/dlls/gdi32/emfdc.c @@ -666,8 +666,11 @@ static DWORD emfdc_ext_create_pen( struct emf *emf, HPEN pen ) if (!(size = GetObjectW( pen, 0, NULL ))) return 0; - emr_size = sizeof(*emr) - sizeof(emr->elp) + size; - emr = HeapAlloc( GetProcessHeap(), 0, emr_size ); + + /* Native adds an extra 4 bytes, presumably because someone wasn't careful about the + * dynamic array [1] at the end of EXTLOGPEN. */ + emr_size = sizeof(*emr) - sizeof(emr->elp) + sizeof(emr->elp.elpStyleEntry) + size; + emr = calloc( 1, emr_size ); if (!emr) return 0; GetObjectW( pen, size, &emr->elp ); @@ -677,16 +680,15 @@ static DWORD emfdc_ext_create_pen( struct emf *emf, HPEN pen ) emr->elp.elpBrushStyle == BS_PATTERN) { FIXME( "elpBrushStyle = %d\n", emr->elp.elpBrushStyle ); - HeapFree( GetProcessHeap(), 0, emr ); + free( emr ); return 0; } - emr->offBmi = emr->cbBmi = emr->offBits = emr->cbBits = 0; emr->emr.iType = EMR_EXTCREATEPEN; emr->emr.nSize = emr_size; emr->ihPen = ret = emfdc_add_handle( emf, pen ); ret = emfdc_record( emf, &emr->emr ) ? ret : 0; - HeapFree( GetProcessHeap(), 0, emr ); + free( emr ); return ret; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7183
From: Stefan Dösinger <stefan(a)codeweavers.com> Again to match what the tests get on native. --- dlls/gdi32/emfdc.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/dlls/gdi32/emfdc.c b/dlls/gdi32/emfdc.c index 2fd4f72f9cc..66a9ef7ed0f 100644 --- a/dlls/gdi32/emfdc.c +++ b/dlls/gdi32/emfdc.c @@ -683,6 +683,8 @@ static DWORD emfdc_ext_create_pen( struct emf *emf, HPEN pen ) free( emr ); return 0; } + /* Native sets these even if there is no bitmap or brush BITMAPINFO. */ + emr->offBmi = emr->offBits = emr_size; emr->emr.iType = EMR_EXTCREATEPEN; emr->emr.nSize = emr_size; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7183
From: Stefan Dösinger <stefan(a)codeweavers.com> --- This is what actually mattered. The extra offset moved the style count and broke creation of the stored pens. --- dlls/gdi32/emfdc.c | 43 +++++++++++++++++++++++++++++++------------ dlls/wow64win/gdi.c | 11 ----------- include/wingdi.h | 13 ++++++++++++- 3 files changed, 43 insertions(+), 24 deletions(-) diff --git a/dlls/gdi32/emfdc.c b/dlls/gdi32/emfdc.c index 66a9ef7ed0f..098caff0c0b 100644 --- a/dlls/gdi32/emfdc.c +++ b/dlls/gdi32/emfdc.c @@ -660,37 +660,56 @@ static BOOL emfdc_select_font( DC_ATTR *dc_attr, HFONT font ) static DWORD emfdc_ext_create_pen( struct emf *emf, HPEN pen ) { - EMREXTCREATEPEN *emr; + EMREXTCREATEPEN *emr = NULL; + EXTLOGPEN *elp = NULL; int size, emr_size; + unsigned int i; DWORD ret = 0; if (!(size = GetObjectW( pen, 0, NULL ))) return 0; + elp = malloc( size ); + if (!elp) + return 0; + /* Native adds an extra 4 bytes, presumably because someone wasn't careful about the - * dynamic array [1] at the end of EXTLOGPEN. */ - emr_size = sizeof(*emr) - sizeof(emr->elp) + sizeof(emr->elp.elpStyleEntry) + size; + * dynamic array [1] at the end of EXTLOGPEN. Also note that GetObject returns an + * EXTLOGPEN with a pointer-sized elpHatch, whereas a metafile always has a 32 bit + * sized emr->elp.elpHatch .*/ + emr_size = sizeof(*emr) - sizeof(*elp) + sizeof(emr->elp.elpStyleEntry) + size; emr = calloc( 1, emr_size ); if (!emr) - return 0; - GetObjectW( pen, size, &emr->elp ); + goto out; + GetObjectW( pen, size, elp ); - if (emr->elp.elpBrushStyle == BS_DIBPATTERN || - emr->elp.elpBrushStyle == BS_DIBPATTERNPT || - emr->elp.elpBrushStyle == BS_PATTERN) + if (elp->elpBrushStyle == BS_DIBPATTERN || + elp->elpBrushStyle == BS_DIBPATTERNPT || + elp->elpBrushStyle == BS_PATTERN) { - FIXME( "elpBrushStyle = %d\n", emr->elp.elpBrushStyle ); - free( emr ); - return 0; + FIXME( "elpBrushStyle = %d\n", elp->elpBrushStyle ); + goto out; } - /* Native sets these even if there is no bitmap or brush BITMAPINFO. */ emr->offBmi = emr->offBits = emr_size; + emr->elp.elpPenStyle = elp->elpPenStyle; + emr->elp.elpWidth = elp->elpWidth; + emr->elp.elpBrushStyle = elp->elpBrushStyle; + emr->elp.elpColor = elp->elpColor; + emr->elp.elpHatch = elp->elpHatch; + emr->elp.elpNumEntries = elp->elpNumEntries; + + for (i = 0; i < elp->elpNumEntries; ++i) + emr->elp.elpStyleEntry[i] = elp->elpStyleEntry[i]; + emr->emr.iType = EMR_EXTCREATEPEN; emr->emr.nSize = emr_size; emr->ihPen = ret = emfdc_add_handle( emf, pen ); ret = emfdc_record( emf, &emr->emr ) ? ret : 0; + +out: free( emr ); + free( elp ); return ret; } diff --git a/dlls/wow64win/gdi.c b/dlls/wow64win/gdi.c index bcd430be4c9..61e719fba5d 100644 --- a/dlls/wow64win/gdi.c +++ b/dlls/wow64win/gdi.c @@ -39,17 +39,6 @@ typedef struct ULONG bmBits; } BITMAP32; -typedef struct -{ - DWORD elpPenStyle; - DWORD elpWidth; - UINT elpBrushStyle; - COLORREF elpColor; - ULONG elpHatch; - DWORD elpNumEntries; - DWORD elpStyleEntry[1]; -} EXTLOGPEN32; - typedef struct { UINT otmSize; diff --git a/include/wingdi.h b/include/wingdi.h index 039436fbbf1..f724cc3132a 100644 --- a/include/wingdi.h +++ b/include/wingdi.h @@ -1568,6 +1568,17 @@ typedef struct tagEXTLOGPEN DWORD elpStyleEntry[1]; } EXTLOGPEN, *PEXTLOGPEN, *NPEXTLOGPEN, *LPEXTLOGPEN; +typedef struct tagEXTLOGPEN32 +{ + DWORD elpPenStyle; + DWORD elpWidth; + UINT elpBrushStyle; + COLORREF elpColor; + ULONG elpHatch; + DWORD elpNumEntries; + DWORD elpStyleEntry[1]; +} EXTLOGPEN32, *PEXTLOGPEN32, *NPEXTLOGPEN32, *LPEXTLOGPEN32; + #define PS_SOLID 0x00000000 #define PS_DASH 0x00000001 #define PS_DOT 0x00000002 @@ -2338,7 +2349,7 @@ typedef struct { DWORD cbBmi; DWORD offBits; DWORD cbBits; - EXTLOGPEN elp; + EXTLOGPEN32 elp; } EMREXTCREATEPEN, *PEMREXTCREATEPEN; typedef struct tagEMREXTESCAPE -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7183
From: Stefan Dösinger <stefan(a)codeweavers.com> --- dlls/gdi32/tests/metafile.c | 106 ++++++++++++++++++++++++++++++++++++ 1 file changed, 106 insertions(+) diff --git a/dlls/gdi32/tests/metafile.c b/dlls/gdi32/tests/metafile.c index ab7882f9641..a1c8ede04dc 100644 --- a/dlls/gdi32/tests/metafile.c +++ b/dlls/gdi32/tests/metafile.c @@ -4556,6 +4556,111 @@ static void test_mf_select(void) ok(ret, "DeleteMetaFile(%p) error %ld\n", hmf, GetLastError()); } +static void test_emf_extpen(void) +{ + static const DWORD user_style[3] = { 0xabc, 0xdef, 0x888 }; + HENHMETAFILE hemf; + HPEN pen, pen2; + LOGBRUSH brush; + HGDIOBJ obj; + DWORD size; + BOOL ret; + HDC hdc; + + /* EMREXTCREATEPEN has two pitfalls: + * + * 1) EXTLOGPEN contains a pointer-sized integer. Metafiles always use the + * 32 bit version of this structure. + * + * 2) Native adds an extra 4 bytes to the EXTLOGPEN32 structure, presumably + * because someone forgot to account for the elpStyleEntry[1] dummy array. + * This is filled with zeroes and at the end of the struct, so it only + * matters when you make assumptions about the metafile or entry size. */ + static const unsigned char extpen_bits[] = + { + 0x01, 0x00, 0x00, 0x00, 0x6c, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0xe9, 0xff, 0xff, 0xff, 0xe9, 0xff, 0xff, 0xff, + 0x20, 0x45, 0x4d, 0x46, 0x00, 0x00, 0x01, 0x00, + 0x38, 0x01, 0x00, 0x00, 0x09, 0x00, 0x00, 0x00, + 0x03, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x78, 0x05, 0x00, 0x00, 0x1a, 0x04, 0x00, 0x00, + 0x40, 0x01, 0x00, 0x00, 0xf0, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0xe2, 0x04, 0x00, + 0x80, 0xa9, 0x03, 0x00, 0x5f, 0x00, 0x00, 0x00, + 0x38, 0x00, 0x00, 0x00, 0x01, 0x00, 0x00, 0x00, + 0x38, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x38, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x01, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x45, 0x67, 0x89, 0x00, + 0x04, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x25, 0x00, 0x00, 0x00, + 0x0c, 0x00, 0x00, 0x00, 0x01, 0x00, 0x00, 0x00, + 0x5f, 0x00, 0x00, 0x00, 0x44, 0x00, 0x00, 0x00, + 0x02, 0x00, 0x00, 0x00, 0x44, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x44, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x07, 0x00, 0x00, 0x00, + 0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x12, 0x34, 0x56, 0x00, 0x04, 0x00, 0x00, 0x00, + 0x03, 0x00, 0x00, 0x00, 0xbc, 0x0a, 0x00, 0x00, + 0xef, 0x0d, 0x00, 0x00, 0x88, 0x08, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x25, 0x00, 0x00, 0x00, + 0x0c, 0x00, 0x00, 0x00, 0x02, 0x00, 0x00, 0x00, + 0x25, 0x00, 0x00, 0x00, 0x0c, 0x00, 0x00, 0x00, + 0x07, 0x00, 0x00, 0x80, 0x28, 0x00, 0x00, 0x00, + 0x0c, 0x00, 0x00, 0x00, 0x02, 0x00, 0x00, 0x00, + 0x28, 0x00, 0x00, 0x00, 0x0c, 0x00, 0x00, 0x00, + 0x01, 0x00, 0x00, 0x00, 0x0e, 0x00, 0x00, 0x00, + 0x14, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x10, 0x00, 0x00, 0x00, 0x14, 0x00, 0x00, 0x00 + }; + + hdc = CreateEnhMetaFileA(NULL, NULL, NULL, NULL); + ok(hdc != 0, "CreateEnhMetaFileA failed\n"); + + brush.lbStyle = BS_SOLID; + brush.lbColor = RGB(0x45,0x67,0x89); + brush.lbHatch = HS_CROSS; + pen = ExtCreatePen(PS_SOLID, 1, &brush, 0, NULL); + ok(!!pen, "ExtCreatePen failed: %lu\n", GetLastError()); + size = GetObjectW(pen, 0, NULL); + ok(size == offsetof(EXTLOGPEN, elpStyleEntry[0] ), "Got unexpected size %#lx\n", size); + + obj = SelectObject(hdc, pen); + ok(obj == GetStockObject(BLACK_PEN), "pen is not a stock BLACK_PEN: %p\n", obj); + + brush.lbColor = RGB(0x12,0x34,0x56); + pen2 = ExtCreatePen(PS_USERSTYLE, 1, &brush, 3, user_style); + ok(!!pen2, "ExtCreatePen failed: %lu\n", GetLastError()); + size = GetObjectW(pen2, 0, NULL); /* cf pen.c, test_logpen() */ + ok(size == offsetof(EXTLOGPEN, elpStyleEntry[3]), "Got unexpected size %#lx\n", size); + + obj = SelectObject(hdc, pen2); + ok(obj == pen, "Expected pen %p, got %p.\n", obj, pen); + + obj = SelectObject(hdc, GetStockObject(BLACK_PEN)); + ok(obj == pen2, "Expected pen %p, got %p.\n", obj, pen2); + + DeleteObject(pen2); + DeleteObject(pen); + hemf = CloseEnhMetaFile(hdc); + ok(hemf != 0, "CloseEnhMetaFile failed\n"); + + if (compare_emf_bits(hemf, extpen_bits, sizeof(extpen_bits), "emf_extpen", FALSE)) + { + dump_emf_bits(hemf, "emf_extpen"); + dump_emf_records(hemf, "emf_extpen"); + } + + ret = DeleteEnhMetaFile(hemf); + ok(ret, "DeleteEnhMetaFile(%p) error %ld\n", hemf, GetLastError()); +} + + static void test_emf_select(void) { HENHMETAFILE hemf; @@ -10831,6 +10936,7 @@ START_TEST(metafile) test_emf_StretchDIBits(); test_emf_SetDIBitsToDevice(); test_emf_palette(); + test_emf_extpen(); /* For win-format metafiles (mfdrv) */ test_mf_SaveDC(); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7183
William Horvath (@whrvth) commented about include/wingdi.h:
DWORD elpStyleEntry[1]; } EXTLOGPEN, *PEXTLOGPEN, *NPEXTLOGPEN, *LPEXTLOGPEN;
+typedef struct tagEXTLOGPEN32 +{ + DWORD elpPenStyle; + DWORD elpWidth; + UINT elpBrushStyle; + COLORREF elpColor; + ULONG elpHatch;
```suggestion:-0+0 ULONG_PTR elpHatch; ``` But then it would be identical to the struct above it. That's what's breaking the compilation, anyways. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7183#note_92619
On Thu Jan 23 21:36:09 2025 +0000, William Horvath wrote:
```suggestion:-0+0 ULONG_PTR elpHatch; ``` But then it would be identical to the struct above it. That's what's breaking the compilation, anyways.
It's a public type, using ULONG. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7183#note_92636
On Thu Jan 23 21:44:43 2025 +0000, Nikolay Sivov wrote:
It's a public type, using ULONG. ~~I don't understand? https://learn.microsoft.com/en-us/windows/win32/api/wingdi/ns-wingdi-extlogp... shows it as ULONG_PTR as well.~~ Oh, you specifically meant with the 32 suffix. Is there a header for that?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7183#note_92652
On Thu Jan 23 21:44:43 2025 +0000, William Horvath wrote:
~~I don't understand? https://learn.microsoft.com/en-us/windows/win32/api/wingdi/ns-wingdi-extlogp... shows it as ULONG_PTR as well.~~ Oh, you specifically meant with the 32 suffix. Is there a header for that? There are two types EXTLOGPEN and EXTLOGPEN32.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7183#note_92653
On Thu Jan 23 21:44:43 2025 +0000, Nikolay Sivov wrote:
There are two types EXTLOGPEN and EXTLOGPEN32. For headers it's always necessary to check what's in actual SDK, online pages are occasionally broken. These types are in wingdi.h from Windows SDK.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7183#note_92654
On Thu Jan 23 21:44:43 2025 +0000, Nikolay Sivov wrote:
For headers it's always necessary to check what's in actual SDK, online pages are occasionally broken. These types are in wingdi.h from Windows SDK. I see now. Then there are outdated static asserts which need to be updated, instead of this.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7183#note_92655
On Thu Jan 23 21:44:43 2025 +0000, William Horvath wrote:
I see now. Then there are outdated static asserts which need to be updated, instead of this. The compilation is breaking because I did not include a regenerated dlls/gdi32/tests/generated.c in the commit. This file is built by --enable-maintainer-mode, so Alexandre will update it if/when he accepts the MR
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7183#note_92656
On Thu Jan 23 21:45:45 2025 +0000, Stefan Dösinger wrote:
The compilation is breaking because I did not include a regenerated dlls/gdi32/tests/generated.c in the commit. This file is built by --enable-maintainer-mode, so Alexandre will update it if/when he accepts the MR What I don't understand is what the generated.c asserts are good for, if they are generated from the same files they test. We'd have to generate them from the Windows SDK headers rather than ours; Doing so, or building our generated.c with Visual Studio and the Windows SDK headers, would have caught this bug.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7183#note_92658
On Thu Jan 23 21:45:45 2025 +0000, Stefan Dösinger wrote:
What I don't understand is what the generated.c asserts are good for, if they are generated from the same files they test. We'd have to generate them from the Windows SDK headers rather than ours; Doing so, or building our generated.c with Visual Studio and the Windows SDK headers, would have caught this bug. Surely you're meant to generate those with reference headers, not against the same thing you're testing.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7183#note_92659
On Thu Jan 23 21:46:51 2025 +0000, Nikolay Sivov wrote:
Surely you're meant to generate those with reference headers, not against the same thing you're testing. Sorry, I didn't know about this gitlab CI edge case, I assumed most of the "missing autogenerated files" issues were fixed. I see the discussion about this on the IRC now, as well.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7183#note_92660
The compilation is breaking because I did not include a regenerated dlls/gdi32/tests/generated.c in the commit. This file is built by --enable-maintainer-mode, so Alexandre will update it if/when he accepts the MR
No, these files need to be regenerated by hand, and checked against a Visual Studio build. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7183#note_92662
participants (4)
-
Alexandre Julliard (＠julliard) -
Nikolay Sivov (＠nsivov)
-
Stefan Dösinger
-
William Horvath (＠whrvth)