[PATCH 0/3] MR1292: uxtheme: Protect CloseThemeData() from invalid input.
The first patch is from Dmitry. I asked him before sending it. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1292
From: Dmitry Timoshkov <dmitry(a)baikal.ru> From: Dmitry Timoshkov <dmitry(a)baikal.ru> With test case by Michael Müller <michael(a)fds-team.de>. Zhiyi Zhang's comments: Some applications close the same HTHEME handle more than once, causing use-after-free. HTHEME is a handle rather than a pointer. Some testing shows that it's a handle starting from 0x10000 or 0x20000. Each new handle increments from the first handle and closing handles decrements it. I prefer not to implement this handle to data map for now because it will likely hurt performance. Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=29974 --- dlls/uxtheme/msstyles.c | 22 ++++++++++++++++++++++ dlls/uxtheme/msstyles.h | 1 + dlls/uxtheme/tests/system.c | 2 ++ 3 files changed, 25 insertions(+) diff --git a/dlls/uxtheme/msstyles.c b/dlls/uxtheme/msstyles.c index 9e5a79a1c69..d871f213b05 100644 --- a/dlls/uxtheme/msstyles.c +++ b/dlls/uxtheme/msstyles.c @@ -32,6 +32,7 @@ #include "msstyles.h" +#include "wine/exception.h" #include "wine/debug.h" #include "wine/heap.h" @@ -49,6 +50,8 @@ static HRESULT MSSTYLES_GetFont (LPCWSTR lpStringStart, LPCWSTR lpStringEnd, LPC #define MSSTYLES_VERSION 0x0003 +#define THEME_CLASS_SIGNATURE 0x12bc6d83 + static PTHEME_FILE tfActiveTheme; /***********************************************************************/ @@ -204,6 +207,7 @@ void MSSTYLES_CloseThemeFile(PTHEME_FILE tf) pcls->partstate = ps->next; heap_free(ps); } + pcls->signature = 0; heap_free(pcls); } } @@ -442,6 +446,7 @@ static PTHEME_CLASS MSSTYLES_AddClass(PTHEME_FILE tf, LPCWSTR pszAppName, LPCWST if(cur) return cur; cur = heap_alloc(sizeof(*cur)); + cur->signature = THEME_CLASS_SIGNATURE; cur->hTheme = tf->hTheme; lstrcpyW(cur->szAppName, pszAppName); lstrcpyW(cur->szClassName, pszClassName); @@ -1075,6 +1080,23 @@ PTHEME_CLASS MSSTYLES_OpenThemeClass(LPCWSTR pszAppName, LPCWSTR pszClassList, U */ HRESULT MSSTYLES_CloseThemeClass(PTHEME_CLASS tc) { + __TRY + { + if (tc->signature != THEME_CLASS_SIGNATURE) + tc = NULL; + } + __EXCEPT_PAGE_FAULT + { + tc = NULL; + } + __ENDTRY + + if (!tc) + { + WARN("Invalid theme class handle\n"); + return E_HANDLE; + } + MSSTYLES_CloseThemeFile (tc->tf); return S_OK; } diff --git a/dlls/uxtheme/msstyles.h b/dlls/uxtheme/msstyles.h index 67f81315d7a..a1918314341 100644 --- a/dlls/uxtheme/msstyles.h +++ b/dlls/uxtheme/msstyles.h @@ -49,6 +49,7 @@ typedef struct _THEME_PARTSTATE { struct _THEME_FILE; typedef struct _THEME_CLASS { + DWORD signature; HMODULE hTheme; struct _THEME_FILE* tf; WCHAR szAppName[MAX_THEME_APP_NAME]; diff --git a/dlls/uxtheme/tests/system.c b/dlls/uxtheme/tests/system.c index a90c512091f..f8253c6a983 100644 --- a/dlls/uxtheme/tests/system.c +++ b/dlls/uxtheme/tests/system.c @@ -835,6 +835,8 @@ static void test_CloseThemeData(void) ok( hRes == E_HANDLE, "Expected E_HANDLE, got 0x%08lx\n", hRes); hRes = CloseThemeData(INVALID_HANDLE_VALUE); ok( hRes == E_HANDLE, "Expected E_HANDLE, got 0x%08lx\n", hRes); + hRes = CloseThemeData((HTHEME)0xdeadbeef); + ok(hRes == E_HANDLE, "Expected E_HANDLE, got 0x%08lx\n", hRes); } static void test_buffer_dc_props(HDC hdc, const RECT *rect) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/1292
From: Zhiyi Zhang <zzhang(a)codeweavers.com> Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=29974 Signed-off-by: Zhiyi Zhang <zzhang(a)codeweavers.com> --- dlls/uxtheme/msstyles.c | 9 ++++++++- dlls/uxtheme/msstyles.h | 1 + 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/dlls/uxtheme/msstyles.c b/dlls/uxtheme/msstyles.c index d871f213b05..529a280b2e5 100644 --- a/dlls/uxtheme/msstyles.c +++ b/dlls/uxtheme/msstyles.c @@ -447,6 +447,7 @@ static PTHEME_CLASS MSSTYLES_AddClass(PTHEME_FILE tf, LPCWSTR pszAppName, LPCWST cur = heap_alloc(sizeof(*cur)); cur->signature = THEME_CLASS_SIGNATURE; + cur->refcount = 0; cur->hTheme = tf->hTheme; lstrcpyW(cur->szAppName, pszAppName); lstrcpyW(cur->szClassName, pszClassName); @@ -1061,6 +1062,7 @@ PTHEME_CLASS MSSTYLES_OpenThemeClass(LPCWSTR pszAppName, LPCWSTR pszClassList, U TRACE("Opened app %s, class %s from list %s\n", debugstr_w(cls->szAppName), debugstr_w(cls->szClassName), debugstr_w(pszClassList)); cls->tf = tfActiveTheme; cls->tf->dwRefCount++; + InterlockedIncrement(&cls->refcount); cls->dpi = dpi; } return cls; @@ -1080,6 +1082,8 @@ PTHEME_CLASS MSSTYLES_OpenThemeClass(LPCWSTR pszAppName, LPCWSTR pszClassList, U */ HRESULT MSSTYLES_CloseThemeClass(PTHEME_CLASS tc) { + LONG refcount; + __TRY { if (tc->signature != THEME_CLASS_SIGNATURE) @@ -1097,7 +1101,10 @@ HRESULT MSSTYLES_CloseThemeClass(PTHEME_CLASS tc) return E_HANDLE; } - MSSTYLES_CloseThemeFile (tc->tf); + refcount = InterlockedDecrement(&tc->refcount); + /* Some buggy apps may double free HTHEME handles */ + if (refcount >= 0) + MSSTYLES_CloseThemeFile(tc->tf); return S_OK; } diff --git a/dlls/uxtheme/msstyles.h b/dlls/uxtheme/msstyles.h index a1918314341..9fe99784838 100644 --- a/dlls/uxtheme/msstyles.h +++ b/dlls/uxtheme/msstyles.h @@ -50,6 +50,7 @@ struct _THEME_FILE; typedef struct _THEME_CLASS { DWORD signature; + LONG refcount; HMODULE hTheme; struct _THEME_FILE* tf; WCHAR szAppName[MAX_THEME_APP_NAME]; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/1292
From: Zhiyi Zhang <zzhang(a)codeweavers.com> Signed-off-by: Zhiyi Zhang <zzhang(a)codeweavers.com> --- dlls/uxtheme/msstyles.c | 13 ++++++++----- dlls/uxtheme/msstyles.h | 2 +- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/dlls/uxtheme/msstyles.c b/dlls/uxtheme/msstyles.c index 529a280b2e5..cfd21a48989 100644 --- a/dlls/uxtheme/msstyles.c +++ b/dlls/uxtheme/msstyles.c @@ -171,7 +171,7 @@ HRESULT MSSTYLES_OpenThemeFile(LPCWSTR lpThemeFile, LPCWSTR pszColorName, LPCWST (*tf)->pszAvailSizes = pszSizes; (*tf)->pszSelectedColor = pszSelectedColor; (*tf)->pszSelectedSize = pszSelectedSize; - (*tf)->dwRefCount = 1; + (*tf)->refcount = 1; return S_OK; invalid_theme: @@ -187,9 +187,12 @@ invalid_theme: */ void MSSTYLES_CloseThemeFile(PTHEME_FILE tf) { + LONG refcount; + if(tf) { - tf->dwRefCount--; - if(!tf->dwRefCount) { + refcount = InterlockedDecrement(&tf->refcount); + if (!refcount) + { if(tf->hTheme) FreeLibrary(tf->hTheme); if(tf->classes) { while(tf->classes) { @@ -235,7 +238,7 @@ HRESULT MSSTYLES_SetActiveTheme(PTHEME_FILE tf, BOOL setMetrics) tfActiveTheme = tf; if (tfActiveTheme) { - tfActiveTheme->dwRefCount++; + InterlockedIncrement(&tfActiveTheme->refcount); if(!tfActiveTheme->classes) MSSTYLES_ParseThemeIni(tfActiveTheme, setMetrics); } @@ -1061,7 +1064,7 @@ PTHEME_CLASS MSSTYLES_OpenThemeClass(LPCWSTR pszAppName, LPCWSTR pszClassList, U if(cls) { TRACE("Opened app %s, class %s from list %s\n", debugstr_w(cls->szAppName), debugstr_w(cls->szClassName), debugstr_w(pszClassList)); cls->tf = tfActiveTheme; - cls->tf->dwRefCount++; + InterlockedIncrement(&cls->tf->refcount); InterlockedIncrement(&cls->refcount); cls->dpi = dpi; } diff --git a/dlls/uxtheme/msstyles.h b/dlls/uxtheme/msstyles.h index 9fe99784838..98e1aa045d6 100644 --- a/dlls/uxtheme/msstyles.h +++ b/dlls/uxtheme/msstyles.h @@ -71,7 +71,7 @@ typedef struct _THEME_IMAGE { } THEME_IMAGE, *PTHEME_IMAGE; typedef struct _THEME_FILE { - DWORD dwRefCount; + LONG refcount; HMODULE hTheme; WCHAR szThemeFile[MAX_PATH]; LPWSTR pszAvailColors; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/1292
This merge request was approved by Zhiyi Zhang. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1292
participants (3)
-
Dmitry Timoshkov -
Zhiyi Zhang
-
Zhiyi Zhang (＠zhiyi)