[Bug 37556] New: String compare functions with only one length argument can lead to page faults.
https://bugs.winehq.org/show_bug.cgi?id=37556 Bug ID: 37556 Summary: String compare functions with only one length argument can lead to page faults. Product: Wine Version: 1.7.31 Hardware: x86 OS: Linux Status: UNCONFIRMED Severity: normal Priority: P2 Component: -unknown Assignee: wine-bugs(a)winehq.org Reporter: sebastian(a)fds-team.de Distribution: --- This issue was discovered with Adobe Flash, but is not limited to this single application. Moreover there are much more ways to trigger this issue than just the way described below. Important parts of the stacktrace: --- snip --- Unhandled exception: page fault on read access to 0x077ca000 in 32-bit code (0xf7563773). [...] Backtrace: =>0 0xf7563773 real_length+0x1f(str=".SOL", len=0xf) [libs/wine/sortkey.c:329] in libwine.so.1 (0x0093cf98) 1 0xf7563791 wine_compare_string+0x10(flags=0x1, str1=".SOL", len1=0xf, str2="\\?\GLOBALROOT\", len2=0xf) [libs/wine/sortkey.c:338] in libwine.so.1 (0x0093cfb8) 2 0x7b84f65f CompareStringEx+0x1f0(locale=..., flags=..., str1=..., len1=..., str2=..., len2=..., version=..., reserved=..., lParam=...) [dlls/kernel32/locale.c:3312] in kernel32 (0x0093d028) 3 0x7b84f45f CompareStringW+0x3a(lcid=..., flags=..., str1=..., len1=..., str2=..., len2=...) [dlls/kernel32/locale.c:3271] in kernel32 (0x0093d078) 4 0x7dd949a3 StrCmpNIW+0x9f(lpszStr=..., lpszComp=..., iLen=...) [dlls/shlwapi/string.c:418] in shlwapi (0x0093d0d8) [...] 0xf7563773 real_length+0x1f [libs/wine/sortkey.c:329] in libwine.so.1: movzwl 0x0(%eax),%eax 329 while (len && !str[len - 1]) len--; --- snip --- What happens here is that StrCmpNIW is called with two strings. --- snip --- INT WINAPI StrCmpNIW(LPCWSTR lpszStr, LPCWSTR lpszComp, int iLen) { TRACE("(%s,%s,%i)\n", debugstr_w(lpszStr), debugstr_w(lpszComp), iLen); return CompareStringW(GetThreadLocale(), NORM_IGNORECASE, lpszStr, iLen, lpszComp, iLen) - CSTR_EQUAL; } --- snip -- The exact length of both strings is unknown in this function, so wine passes "iLen" for both strings to CompareStringW. CompareStringW assumes that memory regions are valid, and then tries to access them in real_length(..). In this case however unfortunately one of the strings is very close to a page boundary, so accessing the whole memory block leads to a page fault. To fix this issue it is either necessary to: * Change all places where CompareStringW(..) is called with 'invalid' length values. * Add exception handlers to real_length(..). * Move real_length(..) from the beginning of CompareStringW(..) somewhere to the end, so that the function aborts on the first non-matching byte. I am opening this issue as a bug report since it needs further investigation, and I am not sure yet whats the right way to fix it. -- 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=37556 --- Comment #1 from Sebastian Lackner <sebastian(a)fds-team.de> --- Created attachment 49987 --> https://bugs.winehq.org/attachment.cgi?id=49987 Testcase In Wine it crashes on the first test (CompareStringW), in Windows on the second (CompareStringA). -- 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=37556 --- Comment #2 from Dmitry Timoshkov <dmitry(a)baikal.ru> --- Created attachment 50084 --> https://bugs.winehq.org/attachment.cgi?id=50084 patch Hi Sebastian, something like the attached patch should fix the bug. -- 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=37556 Sebastian Lackner <sebastian(a)fds-team.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Summary|String compare functions |CompareStringW should abort |with only one length |on the first nonmatching |argument can lead to page |character to avoid invalid |faults. |memory access. Severity|normal |minor --- Comment #3 from Sebastian Lackner <sebastian(a)fds-team.de> --- Hi Dmitry, sorry for the delayed reply. The main issue with Flash is already fixed by some of the patches I sent last week. In this case it was possible to avoid the invalid memory access by changing the way how CompareStringW is called from Wine code. Nevertheless I would like to keep this bug report open for the more general issue, which can also be reproduced using the attached test. Changing priority to 'minor' because I am not aware of any real world application affected by this. The patch fixes the issue, but has some problems: * First of all, *str1 and *str2 should only be accessed when len1/len2 is > 0, so I would suggest to change the order here. * The old code in real_length(...) used a loop, the new code uses just an if statement. Not sure if there are already tests for that, if not it probably makes sense to check what exactly is right. This would make a difference for strings with multiple null termination characters. * The following code could also behaves a bit different when the strings still contain null termination: --- snip --- ... if (!(flags & SORT_STRINGSORT)) { if (*str1 == '-' || *str1 == '\'') { if (*str2 != '-' && *str2 != '\'') { str1++; len1--; continue; } ... --- snip --- At the moment '-' and '\'' at the end of the string are not skipped. Regards, Sebastian -- 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=37556 Dmitry Timoshkov <dmitry(a)baikal.ru> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #50084|0 |1 is obsolete| | --- Comment #4 from Dmitry Timoshkov <dmitry(a)baikal.ru> --- Created attachment 50119 --> https://bugs.winehq.org/attachment.cgi?id=50119 patch (In reply to Sebastian Lackner from comment #3) Thanks for the comments Sebastian.
The patch fixes the issue, but has some problems:
* First of all, *str1 and *str2 should only be accessed when len1/len2 is > 0, so I would suggest to change the order here.
Good catch, attached a fixed version of the patch.
* The old code in real_length(...) used a loop, the new code uses just an if statement. Not sure if there are already tests for that, if not it probably makes sense to check what exactly is right. This would make a difference for strings with multiple null termination characters.
There are already some tests that fail without real_length(), that's why I added this call in the first place long time ago.
* The following code could also behaves a bit different when the strings still contain null termination:
--- snip --- ... if (!(flags & SORT_STRINGSORT)) { if (*str1 == '-' || *str1 == '\'') { if (*str2 != '-' && *str2 != '\'') { str1++; len1--; continue; } ... --- snip ---
At the moment '-' and '\'' at the end of the string are not skipped.
This would need some additional tests. Still there are some tests already which check strings with embedded '\0'. -- 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=37556 Sebastian Lackner <sebastian(a)fds-team.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|UNCONFIRMED |STAGED CC| |erich.e.hoover(a)wine-staging | |.com, michael(a)fds-team.de, | |sebastian(a)fds-team.de Ever confirmed|0 |1 Staged patchset| |https://github.com/wine-com | |pholio/wine-staging/tree/ma | |ster/patches/kernel32-Compa | |reString_Length -- 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=37556 Austin English <austinenglish(a)gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords| |patch -- 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=37556 Nikolay Sivov <bunglehead(a)gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Component|-unknown |kernel32 -- 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=37556 Sebastian Lackner <sebastian(a)fds-team.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Fixed by SHA1| |dfbbd55af7e625968473d233671 | |d9aa63ec33f83 Status|STAGED |RESOLVED Resolution|--- |FIXED --- Comment #5 from Sebastian Lackner <sebastian(a)fds-team.de> --- Fixed with http://source.winehq.org/git/wine.git/patch/dfbbd55af7e625968473d233671d9aa6.... To ensure that the testcase doesn't get lost, I've sent it to the mailing list: http://source.winehq.org/patches/data/119175 -- 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=37556 Michael Stefaniuc <mstefani(a)redhat.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |mstefani(a)redhat.com Target Milestone|--- |1.8.x -- 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=37556 --- Comment #6 from Sebastian Lackner <sebastian(a)fds-team.de> --- @Michael Stefaniuc: Please note that the priority for backporting this patch is low because there is currently no known real-world. -- 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=37556 Alexandre Julliard <julliard(a)winehq.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|RESOLVED |CLOSED --- Comment #7 from Alexandre Julliard <julliard(a)winehq.org> --- Closing bugs fixed in 1.9.4. -- 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=37556 Michael Stefaniuc <mstefani(a)redhat.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Target Milestone|1.8.x |--- --- Comment #8 from Michael Stefaniuc <mstefani(a)redhat.com> --- Removing 1.8.x milestone from bugs included in 1.8.2. -- 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)
-
wine-bugs＠winehq.org