[PATCH v4 0/2] MR726: msvfw32: Use string comparison to determine class name equality.
The current version of the code incorrectly assumes that the lpszClass member of CREATESTRUCT passed with WM_CREATE will point to the same memory used for the CreateWindowEx class name parameter, and uses a pointer comparison to check for class name equality. As a side effect of commit e41c255be6ba66d1eec7affe674b8cc7699226b8 "win32u: Use send_message_timeout for WM_CREATE and WM_NCCREATE" the CREATESTRUCT lpszClass member started pointing to different memory, breaking the current implementation of MCIWND_Create(). This commit fixes the problem by performing a proper, case-insensitive string comparison to determine class name equality. Wine-bug: https://bugs.winehq.org/show_bug.cgi?id=53578 -- v4: msvfw32: Use window name to determine if window is created using MCIWndCreate. msvfw32: Test window creation with filename. https://gitlab.winehq.org/wine/wine/-/merge_requests/726
From: Eric Pouech <eric.pouech(a)gmail.com> Signed-off-by: Eric Pouech <eric.pouech(a)gmail.com> --- dlls/msvfw32/tests/mciwnd.c | 146 +++++++++++++++++++++++++++++++++--- 1 file changed, 134 insertions(+), 12 deletions(-) diff --git a/dlls/msvfw32/tests/mciwnd.c b/dlls/msvfw32/tests/mciwnd.c index 85006ca95bf..915624b2eeb 100644 --- a/dlls/msvfw32/tests/mciwnd.c +++ b/dlls/msvfw32/tests/mciwnd.c @@ -153,6 +153,114 @@ static BOOL create_avi_file(char *fname) return ret; } +/* expected information for window creation */ +static struct { + /* input */ + const char* expectedA; + const WCHAR* expectedW; + /* output */ + unsigned open_msg; + enum {NO_MATCH, PTR_ANSI_MATCH, PTR_UNICODE_MATCH, ANSI_MATCH, UNICODE_MATCH, TODO_WINE = 0x40} match; +} wnd_creation; + +static WNDPROC old_MCIWndProc; +static LRESULT WINAPI proxy_MCIWndProc(HWND hWnd, UINT wMsg, WPARAM wParam, LPARAM lParam) +{ + switch (wMsg) + { + case WM_CREATE: + { + CREATESTRUCTW* cs = (CREATESTRUCTW*)lParam; + if (cs->lpCreateParams) + { + if (cs->lpCreateParams == wnd_creation.expectedA) + wnd_creation.match = PTR_ANSI_MATCH; + else if (cs->lpCreateParams == wnd_creation.expectedW) + wnd_creation.match = PTR_UNICODE_MATCH; + else if (!strcmp(cs->lpCreateParams, wnd_creation.expectedA)) + wnd_creation.match = ANSI_MATCH; + else if (!wcscmp(cs->lpCreateParams, wnd_creation.expectedW)) + wnd_creation.match = UNICODE_MATCH; + } + } + break; + case MCIWNDM_OPENA: + case MCIWNDM_OPENW: + wnd_creation.open_msg = wMsg; + break; + default: + break; + } + return old_MCIWndProc(hWnd, wMsg, wParam, lParam); +} + +static LRESULT CALLBACK hook_proc( int code, WPARAM wp, LPARAM lp ) +{ + if (code == HCBT_CREATEWND && lp) + { + CREATESTRUCTW* cs = ((CBT_CREATEWNDW *)lp)->lpcs; + if (cs && ((ULONG_PTR)cs->lpszClass >> 16) && !wcsicmp(cs->lpszClass, L"MCIWndClass")) + { + old_MCIWndProc = (WNDPROC)SetWindowLongPtrW((HWND)wp, GWLP_WNDPROC, (LPARAM)proxy_MCIWndProc); + ok(old_MCIWndProc != NULL, "No wnd proc\n"); + } + } + + return CallNextHookEx( NULL, code, wp, lp ); +} + +static void test_window_create(unsigned line, const char* fname, HWND parent, + BOOL with_mci, BOOL call_ansi, unsigned match) +{ + HMODULE hinst = GetModuleHandleA(NULL); + WCHAR fnameW[MAX_PATH]; + HWND window; + char error[200]; + LRESULT ret; + BOOL expect_ansi = (match & ~TODO_WINE) == PTR_ANSI_MATCH || (match & ~TODO_WINE) == ANSI_MATCH; + + MultiByteToWideChar(CP_ACP, 0, fname, -1, fnameW, ARRAY_SIZE(fnameW)); + + wnd_creation.expectedA = fname; + wnd_creation.expectedW = fnameW; + wnd_creation.open_msg = 0; + wnd_creation.match = NO_MATCH; + + if (call_ansi) + { + if (with_mci) + window = MCIWndCreateA(parent, hinst, MCIWNDF_NOERRORDLG, fname); + else + window = CreateWindowExA(0, "MCIWndClass", NULL, + WS_CLIPSIBLINGS | WS_CLIPCHILDREN | MCIWNDF_NOERRORDLG, + 0, 0, 300, 0, + parent, 0, hinst, expect_ansi ? (LPVOID)fname : (LPVOID)fnameW); + } + else + { + if (with_mci) + window = MCIWndCreateW(parent, hinst, MCIWNDF_NOERRORDLG, fnameW); + else + window = CreateWindowExW(0, L"MCIWndClass", NULL, + WS_CLIPSIBLINGS | WS_CLIPCHILDREN | MCIWNDF_NOERRORDLG, + 0, 0, 300, 0, + parent, 0, hinst, expect_ansi ? (LPVOID)fname : (LPVOID)fnameW); + } + ok_(__FILE__, line)(window != NULL, "Failed to create an MCIWnd window\n"); + ok_(__FILE__, line)(wnd_creation.match == (match & ~TODO_WINE), "unexpected match %u\n", wnd_creation.match); + todo_wine_if(match & TODO_WINE) + ok_(__FILE__, line)((expect_ansi && wnd_creation.open_msg == MCIWNDM_OPENA) || + (!expect_ansi && wnd_creation.open_msg == MCIWNDM_OPENW), + "bad open message %u %s%u\n", match, + wnd_creation.open_msg >= WM_USER ? "WM_USER+" : "", + wnd_creation.open_msg >= WM_USER ? wnd_creation.open_msg - WM_USER : wnd_creation.open_msg); + ret = SendMessageA(window, MCIWNDM_GETERRORA, sizeof(error), (LPARAM)error); + todo_wine_if(match & TODO_WINE) + ok_(__FILE__, line)(!ret || broken(ret == ERROR_INVALID_HANDLE) /* w2003std, w2008s64 */, + "Unexpected error %Id\n", ret); + DestroyWindow(window); +} + static void test_MCIWndCreate(void) { HWND parent, window; @@ -160,32 +268,46 @@ static void test_MCIWndCreate(void) char fname[MAX_PATH]; char invalid_fname[] = "invalid.avi"; char error[200]; + HHOOK hook; LRESULT ret; create_avi_file(fname); - window = MCIWndCreateA(NULL, hinst, MCIWNDF_NOERRORDLG, fname); - ok(window != NULL, "Failed to create an MCIWnd window without parent\n"); + hook = SetWindowsHookExW( WH_CBT, hook_proc, NULL, GetCurrentThreadId() ); - ret = SendMessageA(window, MCIWNDM_GETERRORA, sizeof(error), (LPARAM)error); - ok(!ret || broken(ret == ERROR_INVALID_HANDLE) /* w2003std, w2008s64 */, - "Unexpected error %Id\n", ret); - - DestroyWindow(window); + test_window_create( __LINE__, fname, NULL, TRUE, TRUE, UNICODE_MATCH | TODO_WINE ); + test_window_create( __LINE__, fname, NULL, TRUE, FALSE, PTR_UNICODE_MATCH | TODO_WINE ); + test_window_create( __LINE__, fname, NULL, FALSE, TRUE, PTR_ANSI_MATCH ); + test_window_create( __LINE__, fname, NULL, FALSE, FALSE, PTR_ANSI_MATCH ); parent = CreateWindowExA(0, "static", "msvfw32 test", WS_POPUP, 0, 0, 100, 100, 0, 0, 0, NULL); ok(parent != NULL, "Failed to create a window\n"); - window = MCIWndCreateA(parent, hinst, MCIWNDF_NOERRORDLG, fname); - ok(window != NULL, "Failed to create an MCIWnd window\n"); + ok(!IsWindowUnicode(parent), "Expecting ansi parent window\n"); - ret = SendMessageA(window, MCIWNDM_GETERRORA, sizeof(error), (LPARAM)error); - ok(!ret || broken(ret == ERROR_INVALID_HANDLE) /* w2003std, w2008s64 */, - "Unexpected error %Id\n", ret); + test_window_create( __LINE__, fname, parent, TRUE, TRUE, UNICODE_MATCH | TODO_WINE ); + test_window_create( __LINE__, fname, parent, TRUE, FALSE, PTR_UNICODE_MATCH | TODO_WINE ); + test_window_create( __LINE__, fname, parent, FALSE, TRUE, PTR_ANSI_MATCH ); + test_window_create( __LINE__, fname, parent, FALSE, FALSE, PTR_ANSI_MATCH ); DestroyWindow(parent); + parent = CreateWindowExW(0, L"static", L"msvfw32 test", + WS_POPUP, 0, 0, 100, 100, + 0, 0, 0, NULL); + ok(parent != NULL, "Failed to create a window\n"); + ok(IsWindowUnicode(parent), "Expecting unicode parent window\n"); + + test_window_create( __LINE__, fname, parent, TRUE, TRUE, UNICODE_MATCH ); + test_window_create( __LINE__, fname, parent, TRUE, FALSE, PTR_UNICODE_MATCH ); + test_window_create( __LINE__, fname, parent, FALSE, TRUE, PTR_UNICODE_MATCH ); + test_window_create( __LINE__, fname, parent, FALSE, FALSE, PTR_UNICODE_MATCH ); + + DestroyWindow(parent); + + UnhookWindowsHookEx( hook ); + window = MCIWndCreateA(NULL, hinst, MCIWNDF_NOERRORDLG, invalid_fname); ok(window != NULL, "Failed to create an MCIWnd window\n"); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/726
From: Alexandros Frantzis <alexandros.frantzis(a)collabora.com> The current version of the code incorrectly assumes that the lpszClass member of CREATESTRUCT passed with WM_CREATE will point to the same memory used for the CreateWindowEx class name parameter. MCIWND_Create uses this assumption to perform a pointer comparison on the class name to determine whether the MCI window is being created using MCIWndCreateA/W and should therefore expect a unicode path parameter. As a side effect of commit e41c255be6ba66d1eec7affe674b8cc7699226b8 "win32u: Use send_message_timeout for WM_CREATE and WM_NCCREATE" the CREATESTRUCT lpszClass member started pointing to different memory, breaking the current implementation of MCIWND_Create(). This commit fixes the problem by changing MCIWndCreateA/W to use an internal window name, unlikely to be used by normal applications, which can then be checked in MCIWND_Create to determine if the MCI window is being created using MCIWndCreateA/W. Wine-bug: https://bugs.winehq.org/show_bug.cgi?id=53578 --- dlls/msvfw32/mciwnd.c | 9 ++++++--- dlls/msvfw32/tests/mciwnd.c | 16 +++++++--------- 2 files changed, 13 insertions(+), 12 deletions(-) diff --git a/dlls/msvfw32/mciwnd.c b/dlls/msvfw32/mciwnd.c index 8799293f7c9..e5f387a0bab 100644 --- a/dlls/msvfw32/mciwnd.c +++ b/dlls/msvfw32/mciwnd.c @@ -38,6 +38,8 @@ WINE_DEFAULT_DEBUG_CHANNEL(mci); extern HMODULE MSVFW32_hModule; static const WCHAR mciWndClassW[] = {'M','C','I','W','n','d','C','l','a','s','s',0}; +static const WCHAR mciWndNameW[] = {'M','C','I','W','n','d','C','r','e','a','t','e', + 'W','i','n','e','I','n','t','e','r','n','a','l', 0}; typedef struct { @@ -113,7 +115,7 @@ HWND VFWAPIV MCIWndCreateW(HWND hwndParent, HINSTANCE hInstance, else dwStyle |= WS_VISIBLE | WS_CAPTION | WS_SYSMENU | WS_THICKFRAME | WS_MINIMIZEBOX | WS_MAXIMIZEBOX; - return CreateWindowExW(0, mciWndClassW, NULL, + return CreateWindowExW(0, mciWndClassW, mciWndNameW, dwStyle | WS_CLIPSIBLINGS | WS_CLIPCHILDREN, 0, 0, 300, 0, hwndParent, 0, hInstance, (LPVOID)szFile); @@ -314,8 +316,9 @@ static LRESULT MCIWND_Create(HWND hWnd, LPCREATESTRUCTW cs) else lParam = (LPARAM)cs->lpCreateParams; - /* If it's our internal class pointer, file name is a unicode string */ - if (cs->lpszClass == mciWndClassW) + /* If it's our internal window name, we are being called from MCIWndCreateA/W, + * so file name is a unicode string */ + if (!lstrcmpW(cs->lpszName, mciWndNameW)) SendMessageW(hWnd, MCIWNDM_OPENW, 0, lParam); else { diff --git a/dlls/msvfw32/tests/mciwnd.c b/dlls/msvfw32/tests/mciwnd.c index 915624b2eeb..8bf4c103a6c 100644 --- a/dlls/msvfw32/tests/mciwnd.c +++ b/dlls/msvfw32/tests/mciwnd.c @@ -160,7 +160,7 @@ static struct { const WCHAR* expectedW; /* output */ unsigned open_msg; - enum {NO_MATCH, PTR_ANSI_MATCH, PTR_UNICODE_MATCH, ANSI_MATCH, UNICODE_MATCH, TODO_WINE = 0x40} match; + enum {NO_MATCH, PTR_ANSI_MATCH, PTR_UNICODE_MATCH, ANSI_MATCH, UNICODE_MATCH } match; } wnd_creation; static WNDPROC old_MCIWndProc; @@ -217,7 +217,7 @@ static void test_window_create(unsigned line, const char* fname, HWND parent, HWND window; char error[200]; LRESULT ret; - BOOL expect_ansi = (match & ~TODO_WINE) == PTR_ANSI_MATCH || (match & ~TODO_WINE) == ANSI_MATCH; + BOOL expect_ansi = match == PTR_ANSI_MATCH || match == ANSI_MATCH; MultiByteToWideChar(CP_ACP, 0, fname, -1, fnameW, ARRAY_SIZE(fnameW)); @@ -247,15 +247,13 @@ static void test_window_create(unsigned line, const char* fname, HWND parent, parent, 0, hinst, expect_ansi ? (LPVOID)fname : (LPVOID)fnameW); } ok_(__FILE__, line)(window != NULL, "Failed to create an MCIWnd window\n"); - ok_(__FILE__, line)(wnd_creation.match == (match & ~TODO_WINE), "unexpected match %u\n", wnd_creation.match); - todo_wine_if(match & TODO_WINE) + ok_(__FILE__, line)(wnd_creation.match == match, "unexpected match %u\n", wnd_creation.match); ok_(__FILE__, line)((expect_ansi && wnd_creation.open_msg == MCIWNDM_OPENA) || (!expect_ansi && wnd_creation.open_msg == MCIWNDM_OPENW), "bad open message %u %s%u\n", match, wnd_creation.open_msg >= WM_USER ? "WM_USER+" : "", wnd_creation.open_msg >= WM_USER ? wnd_creation.open_msg - WM_USER : wnd_creation.open_msg); ret = SendMessageA(window, MCIWNDM_GETERRORA, sizeof(error), (LPARAM)error); - todo_wine_if(match & TODO_WINE) ok_(__FILE__, line)(!ret || broken(ret == ERROR_INVALID_HANDLE) /* w2003std, w2008s64 */, "Unexpected error %Id\n", ret); DestroyWindow(window); @@ -275,8 +273,8 @@ static void test_MCIWndCreate(void) hook = SetWindowsHookExW( WH_CBT, hook_proc, NULL, GetCurrentThreadId() ); - test_window_create( __LINE__, fname, NULL, TRUE, TRUE, UNICODE_MATCH | TODO_WINE ); - test_window_create( __LINE__, fname, NULL, TRUE, FALSE, PTR_UNICODE_MATCH | TODO_WINE ); + test_window_create( __LINE__, fname, NULL, TRUE, TRUE, UNICODE_MATCH ); + test_window_create( __LINE__, fname, NULL, TRUE, FALSE, PTR_UNICODE_MATCH ); test_window_create( __LINE__, fname, NULL, FALSE, TRUE, PTR_ANSI_MATCH ); test_window_create( __LINE__, fname, NULL, FALSE, FALSE, PTR_ANSI_MATCH ); @@ -286,8 +284,8 @@ static void test_MCIWndCreate(void) ok(parent != NULL, "Failed to create a window\n"); ok(!IsWindowUnicode(parent), "Expecting ansi parent window\n"); - test_window_create( __LINE__, fname, parent, TRUE, TRUE, UNICODE_MATCH | TODO_WINE ); - test_window_create( __LINE__, fname, parent, TRUE, FALSE, PTR_UNICODE_MATCH | TODO_WINE ); + test_window_create( __LINE__, fname, parent, TRUE, TRUE, UNICODE_MATCH ); + test_window_create( __LINE__, fname, parent, TRUE, FALSE, PTR_UNICODE_MATCH ); test_window_create( __LINE__, fname, parent, FALSE, TRUE, PTR_ANSI_MATCH ); test_window_create( __LINE__, fname, parent, FALSE, FALSE, PTR_ANSI_MATCH ); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/726
On Mon Sep 5 11:45:49 2022 +0000, Alexandros Frantzis wrote:
perhaps the best way to move forward would be to integrate those tests in your serie Will do, thanks! I have updated the MR as follows:
1. Incorporated tests from !776 as the first commit in the series. 2. Used window name to check whether window is created using MCIWndCreateA/W. The window name is currently the legible "MCIWndCreateWineInternal", but we can go with something much more random/arbitrary if we fear that apps may end up with this name. 3. Updated test code to remove TODO_WINE flag support, since it's now completely unused there and in danger of code rot. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/726#note_7684
Used window name to check whether window is created using MCIWndCreateA/W. The window name is currently the legible "MCIWndCreateWineInternal", but we can go with something much more random/arbitrary if we fear that apps may end up with this name.
Except that we know that it does not work like that on Windows. Not that I have a better solution, except for using some global mutex like @epo suggested earlier. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/726#note_7690
participants (4)
-
Alexandros Frantzis -
Alexandros Frantzis (＠afrantzis)
-
Eric Pouech
-
Nikolay Sivov (＠nsivov)