[Bug 55446] New: Using C run-time for unicode strings comparison breaks case insensitive LB_FINDSTRING
https://bugs.winehq.org/show_bug.cgi?id=55446
Bug ID: 55446 Summary: Using C run-time for unicode strings comparison breaks case insensitive LB_FINDSTRING Product: Wine Version: 8.13 Hardware: x86-64 OS: Linux Status: NEW Keywords: regression Severity: normal Priority: P2 Component: user32 Assignee: wine-bugs@winehq.org Reporter: dmitry@baikal.ru CC: julliard@winehq.org Regression SHA1: 9cc92365560f19c2fd2b9796f79aa75e02381bb1 Distribution: ---
If an application uses LB_FINDSTRING and provides a string that doesn't match exactly then LB_FINDSTRING fails. This is a regression, and it's caused by
commit 9cc92365560f19c2fd2b9796f79aa75e02381bb1 Author: Alexandre Julliard julliard@winehq.org Date: Mon Sep 21 13:53:21 2020 +0200
user32: Build with msvcrt.
Signed-off-by: Alexandre Julliard julliard@winehq.org
wcsnicmp() used by LB_FINDSTRING (in both user32 and comctl32) doesn't work properly because it depends on LC_TYPE locale being correctly set, otherwise _towlower_l() correctly handles only ASCII character range.
Relying on CRT locale being correctly initialized in a win32 core DLL seems undesirable because an application may not use or care about C run-time at all.
Under Windows SendMessageW(LB_FINDSTRING) works just fine regardless of system locale.
P.S. This is a much more serious problem, and is not limited to user32/comctl32 because wcsnicmp()/wcsicmp() is used everywhere in the Wine codebase.
https://bugs.winehq.org/show_bug.cgi?id=55446
--- Comment #1 from Alexandre Julliard julliard@winehq.org --- (In reply to Dmitry Timoshkov from comment #0)
Under Windows SendMessageW(LB_FINDSTRING) works just fine regardless of system locale.
Of course that depends how we define "works just fine" ;-)
I agree we don't want to use the C locale, but should it use the user locale instead? Or the thread locale? Or the default Unicode mapping table? Some tests would be interesting.
https://bugs.winehq.org/show_bug.cgi?id=55446
Jactry Zeng jactry92@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |jactry92@gmail.com
https://bugs.winehq.org/show_bug.cgi?id=55446
--- Comment #2 from Dmitry Timoshkov dmitry@baikal.ru --- (In reply to Alexandre Julliard from comment #1)
(In reply to Dmitry Timoshkov from comment #0)
Under Windows SendMessageW(LB_FINDSTRING) works just fine regardless of system locale.
Of course that depends how we define "works just fine" ;-)
It means that LB_FINDSTRING returns index of the correct listbox item.
I agree we don't want to use the C locale, but should it use the user locale instead? Or the thread locale? Or the default Unicode mapping table? Some tests would be interesting.
I'd guess that listbox is supposed to use the locale set by LB_SETLOCALE like it already does in other places of the code, that way it used to work before the problematic commit.
https://bugs.winehq.org/show_bug.cgi?id=55446
--- Comment #3 from Alexandre Julliard julliard@winehq.org --- (In reply to Dmitry Timoshkov from comment #2)
(In reply to Alexandre Julliard from comment #1)
(In reply to Dmitry Timoshkov from comment #0)
Under Windows SendMessageW(LB_FINDSTRING) works just fine regardless of system locale.
Of course that depends how we define "works just fine" ;-)
It means that LB_FINDSTRING returns index of the correct listbox item.
There's no single "correct" answer for case-insensitive string matching.
I agree we don't want to use the C locale, but should it use the user locale instead? Or the thread locale? Or the default Unicode mapping table? Some tests would be interesting.
I'd guess that listbox is supposed to use the locale set by LB_SETLOCALE like it already does in other places of the code, that way it used to work before the problematic commit.
It never used the locale for partial matches, it was using the default Unicode mapping table.
It uses the locale for exact matching, but it's not clear that this is correct either, because there are no tests, and because we didn't support locale-specific matching when the locale support was implemented. This really needs proper tests.
https://bugs.winehq.org/show_bug.cgi?id=55446
--- Comment #4 from Dmitry Timoshkov dmitry@baikal.ru --- (In reply to Alexandre Julliard from comment #3)
(In reply to Dmitry Timoshkov from comment #2)
(In reply to Alexandre Julliard from comment #1)
(In reply to Dmitry Timoshkov from comment #0)
Under Windows SendMessageW(LB_FINDSTRING) works just fine regardless of system locale.
Of course that depends how we define "works just fine" ;-)
It means that LB_FINDSTRING returns index of the correct listbox item.
There's no single "correct" answer for case-insensitive string matching.
In which case returning an expected item index could not be correct?
I agree we don't want to use the C locale, but should it use the user locale instead? Or the thread locale? Or the default Unicode mapping table? Some tests would be interesting.
I'd guess that listbox is supposed to use the locale set by LB_SETLOCALE like it already does in other places of the code, that way it used to work before the problematic commit.
It never used the locale for partial matches, it was using the default Unicode mapping table.
It uses the locale for exact matching, but it's not clear that this is correct either, because there are no tests, and because we didn't support locale-specific matching when the locale support was implemented. This really needs proper tests.
Probably that kind of tests should have been added during the code changes similar to the commit that introduced the regression. It doesn't look fair to request adding the tests from the reporter of the bug. Are you planning to ask the commit author to add the tests to make sure that the proposed changes don't inroduce the regression, or revert the commit otherwise? :)
https://bugs.winehq.org/show_bug.cgi?id=55446
--- Comment #5 from Alexandre Julliard julliard@winehq.org --- (In reply to Dmitry Timoshkov from comment #4)
Probably that kind of tests should have been added during the code changes similar to the commit that introduced the regression. It doesn't look fair to request adding the tests from the reporter of the bug. Are you planning to ask the commit author to add the tests to make sure that the proposed changes don't inroduce the regression, or revert the commit otherwise? :)
I didn't ask you to write the tests, all I'm saying is that we are going to need tests to fix the bug. Anybody can volunteer to write them, and if no one does, I'll do it myself eventually.
https://bugs.winehq.org/show_bug.cgi?id=55446
--- Comment #6 from Dmitry Timoshkov dmitry@baikal.ru --- (In reply to Alexandre Julliard from comment #5)
I didn't ask you to write the tests, all I'm saying is that we are going to need tests to fix the bug. Anybody can volunteer to write them, and if no one does, I'll do it myself eventually.
I didn't want to sound rude, sorry about that. However I don't see anyone else here to whom all that questions about possible use of different locales were directed. On the other hand reverting to previous behaviour by introducing simple wrapper around CompareStringW() would already fix most of the problems regardless of the user/system locale. ​Moreover, it sounds like any other place in the code that currently uses wcsicmp()/wcsnicmp() would also need writing some exhaustive tests to answer same questions.
https://bugs.winehq.org/show_bug.cgi?id=55446
--- Comment #7 from Alexandre Julliard julliard@winehq.org --- (In reply to Dmitry Timoshkov from comment #6)
(In reply to Alexandre Julliard from comment #5)
I didn't ask you to write the tests, all I'm saying is that we are going to need tests to fix the bug. Anybody can volunteer to write them, and if no one does, I'll do it myself eventually.
I didn't want to sound rude, sorry about that. However I don't see anyone else here to whom all that questions about possible use of different locales were directed. On the other hand reverting to previous behaviour by introducing simple wrapper around CompareStringW() would already fix most of the problems regardless of the user/system locale. ​Moreover, it sounds like any other place in the code that currently uses wcsicmp()/wcsnicmp() would also need writing some exhaustive tests to answer same questions.
Most places use wcsicmp to compare against constant strings, so that's easier. There are only a few places that compare arbitrary user-provided strings, but yes, such places would need tests.
https://bugs.winehq.org/show_bug.cgi?id=55446
Fabian Maurer dark.shadow4@web.de changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |dark.shadow4@web.de
--- Comment #8 from Fabian Maurer dark.shadow4@web.de --- Is this fixed by https://gitlab.winehq.org/wine/wine/-/commit/343398d23e8d10cc3931ff3fc98ec43... ?
https://bugs.winehq.org/show_bug.cgi?id=55446
Vijay Kamuju infyquest@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Fixed by SHA1| |343398d23e8d10cc3931ff3fc98 | |ec438c54fac74 Resolution|--- |FIXED Status|NEW |RESOLVED CC| |infyquest@gmail.com
--- Comment #9 from Vijay Kamuju infyquest@gmail.com --- Fix commited - 343398d23e8d10cc3931ff3fc98ec438c54fac74
https://bugs.winehq.org/show_bug.cgi?id=55446
Dmitry Timoshkov dmitry@baikal.ru changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|RESOLVED |REOPENED Resolution|FIXED |---
--- Comment #10 from Dmitry Timoshkov dmitry@baikal.ru --- Thanks Alexandre for working on this!
It's still not fixed for comctl32 which incudes a copy of the same code.
https://bugs.winehq.org/show_bug.cgi?id=55446
Alexandre Julliard julliard@winehq.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|REOPENED |RESOLVED Resolution|--- |FIXED
--- Comment #11 from Alexandre Julliard julliard@winehq.org --- The component and regression SHA1 are about user32, so this can be considered fixed. I'll update the corresponding comctl32 code.
https://bugs.winehq.org/show_bug.cgi?id=55446
Alexandre Julliard julliard@winehq.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|RESOLVED |CLOSED
--- Comment #12 from Alexandre Julliard julliard@winehq.org --- Closing bugs fixed in 9.0-rc3.
-
WineHQ Bugzilla