[PATCH v2 0/1] MR3974: winemac.drv: Ensure user-land callback can access app_icon results.
Wine-Bugs: https://bugs.winehq.org/show_bug.cgi?id=55658
Signed-off-by: Eric Pouech epouech@codeweavers.com
-- v2: winemac.drv: Ensure user-land callback can access app_icon results.
From: Eric Pouech epouech@codeweavers.com
Wine-Bugs: https://bugs.winehq.org/show_bug.cgi?id=55658
Signed-off-by: Eric Pouech epouech@codeweavers.com --- dlls/winemac.drv/dllmain.c | 24 +++++++++++++----------- dlls/winemac.drv/image.c | 20 +++++++++++++------- dlls/winemac.drv/unixlib.h | 6 ------ 3 files changed, 26 insertions(+), 24 deletions(-)
diff --git a/dlls/winemac.drv/dllmain.c b/dlls/winemac.drv/dllmain.c index b20f8d71dc9..c075a174808 100644 --- a/dlls/winemac.drv/dllmain.c +++ b/dlls/winemac.drv/dllmain.c @@ -22,6 +22,7 @@ #include "macdrv_res.h" #include "shellapi.h" #include "wine/debug.h" +#include "ntstatus.h"
WINE_DEFAULT_DEBUG_CHANNEL(macdrv);
@@ -243,29 +244,30 @@ static BOOL CALLBACK get_first_resource(HMODULE module, LPCWSTR type, LPWSTR nam */ static NTSTATUS WINAPI macdrv_app_icon(void *arg, ULONG size) { - struct app_icon_params *params = arg; - struct app_icon_result *result = param_ptr(params->result); + struct app_icon_result result; HRSRC res_info; - HGLOBAL res_data; + HGLOBAL res_data = NULL; GRPICONDIR *icon_dir; int i;
TRACE("()\n");
- result->count = 0; + if (size) + return NtCallbackReturn(NULL, 0, STATUS_INVALID_PARAMETER);
+ result.count = 0; res_info = NULL; EnumResourceNamesW(NULL, (LPCWSTR)RT_GROUP_ICON, get_first_resource, (LONG_PTR)&res_info); if (!res_info) { WARN("found no RT_GROUP_ICON resource\n"); - return 0; + goto cleanup; }
if (!(res_data = LoadResource(NULL, res_info))) { WARN("failed to load RT_GROUP_ICON resource\n"); - return 0; + goto cleanup; }
if (!(icon_dir = LockResource(res_data))) @@ -274,9 +276,9 @@ static NTSTATUS WINAPI macdrv_app_icon(void *arg, ULONG size) goto cleanup; }
- for (i = 0; i < icon_dir->idCount && result->count < ARRAYSIZE(result->entries); i++) + for (i = 0; i < icon_dir->idCount && result.count < ARRAYSIZE(result.entries); i++) { - struct app_icon_entry *entry = &result->entries[result->count]; + struct app_icon_entry *entry = &result.entries[result.count]; int width = icon_dir->idEntries[i].bWidth; int height = icon_dir->idEntries[i].bHeight; BOOL found_better_bpp = FALSE; @@ -338,7 +340,7 @@ static NTSTATUS WINAPI macdrv_app_icon(void *arg, ULONG size) { entry->png = (UINT_PTR)icon_bits; entry->icon = 0; - result->count++; + result.count++; } else { @@ -348,7 +350,7 @@ static NTSTATUS WINAPI macdrv_app_icon(void *arg, ULONG size) { entry->icon = HandleToUlong(icon); entry->png = 0; - result->count++; + result.count++; } else WARN("failed to create icon %d from resource with ID %hd\n", i, icon_dir->idEntries[i].nID); @@ -363,7 +365,7 @@ static NTSTATUS WINAPI macdrv_app_icon(void *arg, ULONG size) cleanup: FreeResource(res_data);
- return 0; + return NtCallbackReturn(&result, sizeof(result), STATUS_SUCCESS); }
typedef NTSTATUS (WINAPI *kernel_callback)(void *params, ULONG size); diff --git a/dlls/winemac.drv/image.c b/dlls/winemac.drv/image.c index 857684db9c2..7ce27fedd09 100644 --- a/dlls/winemac.drv/image.c +++ b/dlls/winemac.drv/image.c @@ -249,27 +249,33 @@ cleanup: */ CFArrayRef create_app_icon_images(void) { - struct app_icon_result icons; - struct app_icon_params params = { .result = (UINT_PTR)&icons }; CFMutableArrayRef images = NULL; + NTSTATUS status; + struct app_icon_result *icons; + ULONG ret_len; int i;
TRACE("()\n");
- macdrv_client_func(client_func_app_icon, ¶ms, sizeof(params)); + status = KeUserModeCallback(client_func_app_icon, NULL, 0, (void**)&icons, &ret_len);
- if (!icons.count) return NULL; + if (status || !icons || ret_len != sizeof(*icons)) + { + WARN("incorrect callback result\n"); + return NULL; + } + if (!icons->count) return NULL;
- images = CFArrayCreateMutable(NULL, icons.count, &kCFTypeArrayCallBacks); + images = CFArrayCreateMutable(NULL, icons->count, &kCFTypeArrayCallBacks); if (!images) { WARN("failed to create images array\n"); return NULL; }
- for (i = 0; i < icons.count; i++) + for (i = 0; i < icons->count; i++) { - struct app_icon_entry *icon = &icons.entries[i]; + struct app_icon_entry *icon = &icons->entries[i]; CGImageRef cgimage = NULL;
if (icon->png) diff --git a/dlls/winemac.drv/unixlib.h b/dlls/winemac.drv/unixlib.h index 61f4f44fb75..9f02f3a2efa 100644 --- a/dlls/winemac.drv/unixlib.h +++ b/dlls/winemac.drv/unixlib.h @@ -110,12 +110,6 @@ struct app_icon_result struct app_icon_entry entries[64]; };
-/* macdrv_app_icon params */ -struct app_icon_params -{ - UINT64 result; /* FIXME: Use NtCallbackReturn instead */ -}; - /* macdrv_app_quit_request params */ struct app_quit_request_params {
On Thu Sep 28 10:30:16 2023 +0000, Jacek Caban wrote:
We shouldn't need to pass result pointer from unixlib to PE. It was needed during the transition (when the driver couldn't assume that it's executed on Unix stack), but at this point it's just a leftover. We can just have the result on PE stack now and just pass it to `NtCallbackReturn` when returning from the callback.
V2 pushed using NtCallbackReturn instead
pipeline failed in mci driver (with a nice crash), on 32bit linux... so not related to winemac.drv change AFAICT
Jacek Caban (@jacek) commented about dlls/winemac.drv/dllmain.c:
static NTSTATUS WINAPI macdrv_app_icon(void *arg, ULONG size) {
- struct app_icon_params *params = arg;
- struct app_icon_result *result = param_ptr(params->result);
- struct app_icon_result result; HRSRC res_info;
- HGLOBAL res_data;
HGLOBAL res_data = NULL; GRPICONDIR *icon_dir; int i;
TRACE("()\n");
- result->count = 0;
- if (size)
return NtCallbackReturn(NULL, 0, STATUS_INVALID_PARAMETER);
You don't need to validate arguments from Unix side.
Jacek Caban (@jacek) commented about dlls/winemac.drv/dllmain.c:
int i; TRACE("()\n");
- result->count = 0;
if (size)
return NtCallbackReturn(NULL, 0, STATUS_INVALID_PARAMETER);result.count = 0; res_info = NULL; EnumResourceNamesW(NULL, (LPCWSTR)RT_GROUP_ICON, get_first_resource, (LONG_PTR)&res_info); if (!res_info) { WARN("found no RT_GROUP_ICON resource\n");
return 0;
goto cleanup;
This change is not needed, Unix side may deduce that from 0 result size. In fact, I'd be tempted to get rid of `app_icon_result`, pass an array of `app_icon_entry`, use the size of actually used entries and calculate count from that size on Unix side.
-
Eric Pouech -
eric pouech (＠epo)
-
Jacek Caban (＠jacek)