[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(a)winehq.org Reporter: dmitry(a)baikal.ru CC: julliard(a)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(a)winehq.org> Date: Mon Sep 21 13:53:21 2020 +0200 user32: Build with msvcrt. Signed-off-by: Alexandre Julliard <julliard(a)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. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=55446 --- Comment #1 from Alexandre Julliard <julliard(a)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. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=55446 Jactry Zeng <jactry92(a)gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jactry92(a)gmail.com -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=55446 --- Comment #2 from Dmitry Timoshkov <dmitry(a)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. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=55446 --- Comment #3 from Alexandre Julliard <julliard(a)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. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=55446 --- Comment #4 from Dmitry Timoshkov <dmitry(a)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? :) -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=55446 --- Comment #5 from Alexandre Julliard <julliard(a)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. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=55446 --- Comment #6 from Dmitry Timoshkov <dmitry(a)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. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=55446 --- Comment #7 from Alexandre Julliard <julliard(a)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. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=55446 Fabian Maurer <dark.shadow4(a)web.de> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |dark.shadow4(a)web.de --- Comment #8 from Fabian Maurer <dark.shadow4(a)web.de> --- Is this fixed by https://gitlab.winehq.org/wine/wine/-/commit/343398d23e8d10cc3931ff3fc98ec43... ? -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=55446 Vijay Kamuju <infyquest(a)gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Fixed by SHA1| |343398d23e8d10cc3931ff3fc98 | |ec438c54fac74 Resolution|--- |FIXED Status|NEW |RESOLVED CC| |infyquest(a)gmail.com --- Comment #9 from Vijay Kamuju <infyquest(a)gmail.com> --- Fix commited - 343398d23e8d10cc3931ff3fc98ec438c54fac74 -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=55446 Dmitry Timoshkov <dmitry(a)baikal.ru> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|RESOLVED |REOPENED Resolution|FIXED |--- --- Comment #10 from Dmitry Timoshkov <dmitry(a)baikal.ru> --- Thanks Alexandre for working on this! It's still not fixed for comctl32 which incudes a copy of the same code. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=55446 Alexandre Julliard <julliard(a)winehq.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|REOPENED |RESOLVED Resolution|--- |FIXED --- Comment #11 from Alexandre Julliard <julliard(a)winehq.org> --- The component and regression SHA1 are about user32, so this can be considered fixed. I'll update the corresponding comctl32 code. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=55446 Alexandre Julliard <julliard(a)winehq.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|RESOLVED |CLOSED --- Comment #12 from Alexandre Julliard <julliard(a)winehq.org> --- Closing bugs fixed in 9.0-rc3. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
participants (1)
-
WineHQ Bugzilla