[PATCH v3 0/1] 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 -- v3: msvfw32: Use string comparison to determine class name equality. 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, 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 --- dlls/msvfw32/mciwnd.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/dlls/msvfw32/mciwnd.c b/dlls/msvfw32/mciwnd.c index 8799293f7c9..3dc8d6f3542 100644 --- a/dlls/msvfw32/mciwnd.c +++ b/dlls/msvfw32/mciwnd.c @@ -314,8 +314,8 @@ 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 class name, file name is a unicode string */ + if (!lstrcmpiW(cs->lpszClass, mciWndClassW)) SendMessageW(hWnd, MCIWNDM_OPENW, 0, lParam); else { -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/726
Nikolay Sivov (@nsivov) commented about dlls/msvfw32/mciwnd.c:
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 class name, file name is a unicode string */ + if (!lstrcmpiW(cs->lpszClass, mciWndClassW))
It seems to me that such comparison will always match now, because you're checking class name from within WM_CREATE of the same class. I don't know exactly what was the point of original condition, maybe it's an equivalent of IsWindowUnicode() check instead? What you could probably test is which message is sent when mci window is created with CreateWindowA vs CreateWindowW, with or without a parent. And when with a parent, with both A and W parents. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/726#note_7213
On Tue Aug 30 11:17:43 2022 +0000, Nikolay Sivov wrote:
It seems to me that such comparison will always match now, because you're checking class name from within WM_CREATE of the same class. I don't know exactly what was the point of original condition, maybe it's an equivalent of IsWindowUnicode() check instead? What you could probably test is which message is sent when mci window is created with CreateWindowA vs CreateWindowW, with or without a parent. And when with a parent, with both A and W parents. Hi @nsivov, thanks for feedback!
I went down the rabbit hole, trying get a deeper understanding of this (18 year old!) code. Below are my findings (and a bit of background). There are two ways to create an MCI window: 1. Call `MCIWndRegisterClass()` and then `CreateWindowEx(MCIWND_WINDOW_CLASS, ..., optional_path_to_open)` manually. 2. `MCIWndCreateA/W(optional_path_to_open)` which will register the class and call `CreateWindowExW()` internally When an app calls `CreateWindowEx(MCIWND_WINDOW_CLASS, path)` we cannot tell whether the data is ansi or unicode, although I guess the implicit expectation is that it should match the CreateWindowExA/W function variant used. `IsWindowUnicode()` on the mci window itself is not useful since it relies on information from the registered class, which is always unicode in our case (see `MCIWndRegisterClass()`) rather on which function is used to create the window. To work around this in the general case, the current code uses the unicode status of the *parent* window as a hint about whether the passed parameter is ansi or or unicode. When using `MCIWndCreateA/W` in particular, the code knows that it always passes a unicode argument, and wants to communicate that to `MCIWND_Create`. This was previously achieved by checking for pointer equality between the internal class string memory used by `MCIWndCreateA/W` when calling `CreateWindowEx` and the class string received in the window process/`MCIWND_Create`. If these two matched we knew we were being called from `MCIWndCreateA/W` and thus that any passed string was unicode. The changes in e41c255b broke this string pointer comparison, and so the MCIWndCreateW special path was invalidated. It's my understanding that the new behavior in that commit is correct, i.e., there is no guarantee that the string memory pointed to by the CREATESTRUCT lpszClass is the same as what was passed in `CreateWindowEx`. However, if that's not the case, and the new behavior is incorrect, it will of course void this whole MR and discussion. My proposed fix which checks for (case-insensitive) class name equality fixes the `MCIWndCreateA/W` path, but, as you note, the class comparison is now always true, so it effectively disables the fallback heuristic, so now all strings are treated as unicode. The bottom line is that we need to find a different mechanism for `MCIWndCreateA/W` to communicate at window creation that what it passed is in fact a unicode value. Here are two (hacky and fragile, but not totally unreasonable) ideas: 1. Since class names are case-insensitive, we can use a string with an uncommon capitalization when we call `CreateWindowEx` from `MCIWndCreateA/W`, and check for that capitilization instead of performing a pointer comparison (e.g., "mcIwNDcLasS"). 2. `MCIWndCreateA` could pass the path prefixed by some bytes that we wouldn't expect to find in a normal ansi or unicode path string. For example, use 0x02 0x00 as the first bytes to communicate that this is a unicode string. But I would very much be interested in some cleaner solution. Any ideas? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/726#note_7327
On Wed Aug 31 13:16:08 2022 +0000, Alexandros Frantzis wrote:
Hi @nsivov, thanks for feedback! I went down the rabbit hole, trying get a deeper understanding of this (18 year old!) code. Below are my findings (and a bit of background). There are two ways to create an MCI window: 1. Call `MCIWndRegisterClass()` and then `CreateWindowEx(MCIWND_WINDOW_CLASS, ..., optional_path_to_open)` manually. 2. `MCIWndCreateA/W(optional_path_to_open)` which will register the class and call `CreateWindowExW()` internally When an app calls `CreateWindowEx(MCIWND_WINDOW_CLASS, path)` we cannot tell whether the data is ansi or unicode, although I guess the implicit expectation is that it should match the CreateWindowExA/W function variant used. `IsWindowUnicode()` on the mci window itself is not useful since it relies on information from the registered class, which is always unicode in our case (see `MCIWndRegisterClass()`) rather on which function is used to create the window. To work around this in the general case, the current code uses the unicode status of the *parent* window as a hint about whether the passed parameter is ansi or or unicode. When using `MCIWndCreateA/W` in particular, the code knows that it always passes a unicode argument, and wants to communicate that to `MCIWND_Create`. This was previously achieved by checking for pointer equality between the internal class string memory used by `MCIWndCreateA/W` when calling `CreateWindowEx` and the class string received in the window process/`MCIWND_Create`. If these two matched we knew we were being called from `MCIWndCreateA/W` and thus that any passed string was unicode. The changes in e41c255b broke this string pointer comparison, and so the MCIWndCreateW special path was invalidated. It's my understanding that the new behavior in that commit is correct, i.e., there is no guarantee that the string memory pointed to by the CREATESTRUCT lpszClass is the same as what was passed in `CreateWindowEx`. However, if that's not the case, and the new behavior is incorrect, it will of course void this whole MR and discussion. My proposed fix which checks for (case-insensitive) class name equality fixes the `MCIWndCreateA/W` path, but, as you note, the class comparison is now always true, so it effectively disables the fallback heuristic, so now all strings are treated as unicode. The bottom line is that we need to find a different mechanism for `MCIWndCreateA/W` to communicate at window creation that what it passed is in fact a unicode value. Here are two (hacky and fragile, but not totally unreasonable) ideas: 1. Since class names are case-insensitive, we can use a string with an uncommon capitalization when we call `CreateWindowEx` from `MCIWndCreateA/W`, and check for that capitilization instead of performing a pointer comparison (e.g., "mcIwNDcLasS").
2. `MCIWndCreateA` could pass the path prefixed by some bytes that we wouldn't expect to find in a normal ansi or unicode path string. For example, use 0x02 0x00 as the first bytes to communicate that this is a unicode string. But I would very much be interested in some cleaner solution. Any ideas? What matters is to replicate what happens on Windows. MCIWndCreateA/MCIWndCreateW will work as is I guess, because we control CreateWindow argument there. Regarding CreateWindowExA/CreateWindowExW, I don't think there is a way to tell from WM_CREATE which one was called, so maybe it should use some heuristic like it does now. This should be testable though. Basically you can register the class, then replace class procedure, and see what's coming in.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/726#note_7477
According to https://testbot.winehq.org/JobDetails.pl?Key=122456 it looks like: - MCIWndCreateA maps the lParam parameters from ANSI to Unicode and calls MCIWndCreateW (that what we currently do) - (*) MCIWndCreate(A|W) generate a MCIWNDM_OPENW message (in WM_CREATE handling (*)) - both CreateWindowEx(A|W) succeed with an ANSI string as lParam (and fail with Unicode string - that's not included in the testbot, but generate a MCIWNDM_OPENA message (*)) - so there's a trigger to pickup the unicode version + it's likely not linked to heuristics on lParam (as it would succeed above) + checked for all windows creation parameters => didn't find meaningful differences in styles/ext styles between MCIWndCreate and CreateWindowEx so no clue yet on what the trigger is. could be global variables (but need more checks for thread safeness and reentrancy (creating a second mci window while creating a first one) (*) from an extended version of the test (not pushed on testbot) that subclasses the wnd proc -- https://gitlab.winehq.org/wine/wine/-/merge_requests/726#note_7547
Maybe it's using a pointer check on a window text field, so see if it came from MCIWndCreate(). How did you check generated MCIWNDM_OPEN messages? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/726#note_7630
@nsivov: tried it, but the lpszName field of CREATESTRUCT is a copy of a the passed string (as lpszClass is). (it seems to be the direct pointer in hooks though, but I don't think it's a good idea to use hooks for msvfw32 implementation) The only other solution I see is to set a global variable when entering MCIWndCreate and test for it in WM_CREATE handling Yes, that's ugly. If you have better ideas... I extended the tests for windows from MCIWndClass (see MR!776) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/726#note_7631
My point is that MCIWndCreate() does not have window text argument, so internally it could use something else, and check against it in WM_CREATE. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/726#note_7632
@nsivov: sorry, I thought you meant pointer comparison. Yes, that would work. @afrantzis: since you started working on this and analyzed the issue, do you want to redo your patch along those lines? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/726#note_7648
I did mean pointer comparison. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/726#note_7649
@epo Thanks for the additional investigation, and, yes, I am happy to iterate on the fix. To ensure I have understood this correctly, the proposed plan is: use the contents of CREATESTRUCT.lpszName to differentiate whether the creation is coming from MCIWndCreateA/W, and hence whether we should expect unicode data. Since pointer comparison won't work, we would set the name to some arbitrary string value, unlikely to be used by any application. One aspect that is not clear to me, is whether the current fallback heuristic of checking the unicode state of the parent should be kept. From your investigation:
both CreateWindowEx(A|W) succeed with an ANSI string as lParam (and fail with Unicode string - that's not included in the testbot, but generate a MCIWNDM_OPENA message (*))
Were you able to check if this behavior holds regardless of the unicode state of the parent? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/726#note_7660
On Mon Sep 5 10:20:59 2022 +0000, Nikolay Sivov wrote:
I did mean pointer comparison. pointer comparison doesn't work: string is (now) copied by win32u befire being set in CREATESTRUCT string comparison does work
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/726#note_7661
On Mon Sep 5 10:26:03 2022 +0000, Alexandros Frantzis wrote:
@epo Thanks for the additional investigation, and, yes, I am happy to iterate on the fix. To ensure I have understood this correctly, the proposed plan is: use the contents of CREATESTRUCT.lpszName to differentiate whether the creation is coming from MCIWndCreateA/W, and hence whether we should expect unicode data. Since pointer comparison won't work, we would set the name to some arbitrary string value, unlikely to be used by any application. One aspect that is not clear to me, is whether the current fallback heuristic of checking the unicode state of the parent should be kept. From your investigation:
both CreateWindowEx(A|W) succeed with an ANSI string as lParam (and fail with Unicode string - that's not included in the testbot, but generate a MCIWNDM_OPENA message (*)) Were you able to check if this behavior holds regardless of the unicode state of the parent? @afrantzis: yes, I've extended the tests (see merge request !776 for the details), and the fallback is ok according to the tests: when parent (if any) is Unicode, lParam is expected to be Unicode.
perhaps the best way to move forward would be to integrate those tests in your serie -- https://gitlab.winehq.org/wine/wine/-/merge_requests/726#note_7663
perhaps the best way to move forward would be to integrate those tests in your serie
Will do, thanks! -- https://gitlab.winehq.org/wine/wine/-/merge_requests/726#note_7672
participants (4)
-
Alexandros Frantzis -
Alexandros Frantzis (＠afrantzis)
-
eric pouech (＠epo)
-
Nikolay Sivov (＠nsivov)