[PATCH v3 0/1] MR10036: winemac.drv: Unconditionally use CreateIconFromResourceEx for app icons.
Sidesteps an issue cause by some apps having an inaccurate size for the resource in the GRPICONDIR size ; CreateIconFromResourceEx handles that case, but the old CGImage path would fault if the GRPICONDIR size was too large. --- It seems that some apps have different sizes in the GRPICONDIR and the IMAGE_RESOURCE_DATA_ENTRY. winemenubuilder has had a workaround for this since 2014 - see c205e6800a63a5df9960d8484a2e67687d53bc50. The size difference is great enough in some situations (e.g. Brotato) that the attempt read the resource may fault, but that's only because we passed the PNG data over to the Unix side of winemac.drv directly, and used CG methods to deal with it, which will attempt to copy in the entire array of bytes. If we use CreateIconFromResourceEx instead, for all cases (it can handle pngs), we do not fault, since that method does not attempt to copy the entire resource size. The original mailing list discussion about the winemenubuilder patch is [here](https://marc.info/?l=wine-patches&m=140903555724711&w=2). Huge thanks to @bshanks for spotting that patch. This has apparently always been faulting for some apps, but only became an issue after !10032. The subsequent syscall fault unwinds out of the pthread_once that !10032 added, so future threads that wind up there will hang. -- v3: winemac.drv: Unconditionally use CreateIconFromResourceEx for app icons. https://gitlab.winehq.org/wine/wine/-/merge_requests/10036
From: Tim Clem <tclem@codeweavers.com> Sidesteps an issue cause by some apps having an inaccurate size for the resource in the GRPICONDIR size ; CreateIconFromResourceEx handles that case, but the old CGImage path would fault if the GRPICONDIR size was too large. --- dlls/winemac.drv/dllmain.c | 22 ++++++---------------- dlls/winemac.drv/image.c | 25 +++---------------------- dlls/winemac.drv/unixlib.h | 1 - 3 files changed, 9 insertions(+), 39 deletions(-) diff --git a/dlls/winemac.drv/dllmain.c b/dlls/winemac.drv/dllmain.c index 7ece4dc99a3..38ded507722 100644 --- a/dlls/winemac.drv/dllmain.c +++ b/dlls/winemac.drv/dllmain.c @@ -334,31 +334,21 @@ static NTSTATUS WINAPI macdrv_app_icon(void *arg, ULONG size) icon_bits = LockResource(icon_res_data); if (icon_bits) { - static const BYTE png_magic[] = { 0x89, 0x50, 0x4e, 0x47 }; + HICON icon; entry->width = width; entry->height = height; entry->size = icon_dir->idEntries[i].dwBytesInRes; - if (!memcmp(icon_bits, png_magic, sizeof(png_magic))) + icon = CreateIconFromResourceEx(icon_bits, icon_dir->idEntries[i].dwBytesInRes, + TRUE, 0x00030000, width, height, 0); + if (icon) { - entry->png = (UINT_PTR)icon_bits; - entry->icon = 0; + entry->icon = HandleToUlong(icon); count++; } else - { - HICON icon = CreateIconFromResourceEx(icon_bits, icon_dir->idEntries[i].dwBytesInRes, - TRUE, 0x00030000, width, height, 0); - if (icon) - { - entry->icon = HandleToUlong(icon); - entry->png = 0; - count++; - } - else - WARN("failed to create icon %d from resource with ID %hd\n", i, icon_dir->idEntries[i].nID); - } + WARN("failed to create icon %d from resource with ID %hd\n", i, icon_dir->idEntries[i].nID); } else WARN("failed to lock RT_ICON resource %d with ID %hd\n", i, icon_dir->idEntries[i].nID); diff --git a/dlls/winemac.drv/image.c b/dlls/winemac.drv/image.c index eb5939ce438..cf927ce7d1a 100644 --- a/dlls/winemac.drv/image.c +++ b/dlls/winemac.drv/image.c @@ -278,28 +278,9 @@ CFArrayRef create_app_icon_images(void) { struct app_icon_entry *icon = &entries[i]; CGImageRef cgimage = NULL; - - if (icon->png) - { - CFDataRef data = CFDataCreate(NULL, param_ptr(icon->png), icon->size); - if (data) - { - CGDataProviderRef provider = CGDataProviderCreateWithCFData(data); - CFRelease(data); - if (provider) - { - cgimage = CGImageCreateWithPNGDataProvider(provider, NULL, FALSE, - kCGRenderingIntentDefault); - CGDataProviderRelease(provider); - } - } - } - else - { - HICON handle = UlongToHandle(icon->icon); - cgimage = create_cgimage_from_icon(handle, icon->width, icon->height); - NtUserDestroyCursor(handle, 0); - } + HICON handle = UlongToHandle(icon->icon); + cgimage = create_cgimage_from_icon(handle, icon->width, icon->height); + NtUserDestroyCursor(handle, 0); if (cgimage) { diff --git a/dlls/winemac.drv/unixlib.h b/dlls/winemac.drv/unixlib.h index d39b3ea5b83..d174cbb7cad 100644 --- a/dlls/winemac.drv/unixlib.h +++ b/dlls/winemac.drv/unixlib.h @@ -56,7 +56,6 @@ UINT32 height; UINT32 size; UINT32 icon; - UINT64 png; }; /* macdrv_app_quit_request params */ -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/10036
On Thu Feb 5 21:28:15 2026 +0000, Brendan Shanks wrote:
If we use `CreateIconFromResourceEx()`, doesn't that mean that Wine decompresses the PNG, then we have to draw it into a CGImage? These are typically Vista icons, so 256x256 or larger. `create_cgimage_from_icon()` and `create_cgimage_from_icon_bitmaps()` are also not simple functions. It seems preferable to just give the PNG to macOS when that's possible. Ha :) I just put up a CreateIconFromResourceEx version. I liked unifying the path, and since it's a one-time operation, the round-tripping didn't bother me. But I definitely understand skipping the HICON-ification. :shrug: Either way strikes me as acceptable.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/10036#note_128914
On Thu Feb 5 21:33:39 2026 +0000, Tim Clem wrote:
Ha :) I just put up a CreateIconFromResourceEx version. I liked unifying the path, and since it's a one-time operation, the round-tripping didn't bother me. But I definitely understand skipping the HICON-ification. :shrug: Either way strikes me as acceptable. It's the same code path we are using for other icons and cursors, I don't see any reason that the app icon needs to be treated differently, particularly when that's causing crashes...
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/10036#note_128915
On Thu Feb 5 21:34:07 2026 +0000, Alexandre Julliard wrote:
It's the same code path we are using for other icons and cursors, I don't see any reason that the app icon needs to be treated differently, particularly when that's causing crashes... (Can confirm by the way that the CreateIconFromResourceEx path does not fault when given an overlarge size.)
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/10036#note_128916
On Thu Feb 5 21:37:06 2026 +0000, Tim Clem wrote:
(Can confirm by the way that the CreateIconFromResourceEx path does not fault when given an overlarge size.) It may also be worth noting that the goal is to get rid of the PE side of the graphics drivers, so the icon processing will ultimately have to be done somewhere else and the driver won't be able to get the PNG bits.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/10036#note_128917
Brendan Shanks (@bshanks) commented about dlls/winemac.drv/dllmain.c:
icon_bits = LockResource(icon_res_data); if (icon_bits) { - static const BYTE png_magic[] = { 0x89, 0x50, 0x4e, 0x47 }; + HICON icon;
entry->width = width; entry->height = height; entry->size = icon_dir->idEntries[i].dwBytesInRes;
I don't think entry->size is used any more -- https://gitlab.winehq.org/wine/wine/-/merge_requests/10036#note_128918
Brendan Shanks (@bshanks) commented about dlls/winemac.drv/dllmain.c:
icon_bits = LockResource(icon_res_data); if (icon_bits) { - static const BYTE png_magic[] = { 0x89, 0x50, 0x4e, 0x47 }; + HICON icon;
entry->width = width; entry->height = height; entry->size = icon_dir->idEntries[i].dwBytesInRes;
- if (!memcmp(icon_bits, png_magic, sizeof(png_magic))) + icon = CreateIconFromResourceEx(icon_bits, icon_dir->idEntries[i].dwBytesInRes, + TRUE, 0x00030000, width, height, 0);
I guess I'll defer to whichever way Alexandre wants to go, but even if it works it seems like a bad idea to knowingly give an incorrect length to `CreateIconFromResourceEx` (`dwResSize` is documented as "The size, in bytes, of the set of bits pointed to by the pbIconBits parameter.") -- https://gitlab.winehq.org/wine/wine/-/merge_requests/10036#note_128919
participants (4)
-
Alexandre Julliard (＠julliard) -
Brendan Shanks (＠bshanks)
-
Tim Clem
-
Tim Clem (＠tclem)