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@winehq.org Reporter: sebastian@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.
https://bugs.winehq.org/show_bug.cgi?id=37556
--- Comment #1 from Sebastian Lackner sebastian@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).
https://bugs.winehq.org/show_bug.cgi?id=37556
--- Comment #2 from Dmitry Timoshkov dmitry@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.
https://bugs.winehq.org/show_bug.cgi?id=37556
Sebastian Lackner sebastian@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@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
https://bugs.winehq.org/show_bug.cgi?id=37556
Dmitry Timoshkov dmitry@baikal.ru changed:
What |Removed |Added ---------------------------------------------------------------------------- Attachment #50084|0 |1 is obsolete| |
--- Comment #4 from Dmitry Timoshkov dmitry@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'.
https://bugs.winehq.org/show_bug.cgi?id=37556
Sebastian Lackner sebastian@fds-team.de changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|UNCONFIRMED |STAGED CC| |erich.e.hoover@wine-staging | |.com, michael@fds-team.de, | |sebastian@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
https://bugs.winehq.org/show_bug.cgi?id=37556
Austin English austinenglish@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Keywords| |patch
https://bugs.winehq.org/show_bug.cgi?id=37556
Nikolay Sivov bunglehead@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Component|-unknown |kernel32
https://bugs.winehq.org/show_bug.cgi?id=37556
Sebastian Lackner sebastian@fds-team.de changed:
What |Removed |Added ---------------------------------------------------------------------------- Fixed by SHA1| |dfbbd55af7e625968473d233671 | |d9aa63ec33f83 Status|STAGED |RESOLVED Resolution|--- |FIXED
--- Comment #5 from Sebastian Lackner sebastian@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
https://bugs.winehq.org/show_bug.cgi?id=37556
Michael Stefaniuc mstefani@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |mstefani@redhat.com Target Milestone|--- |1.8.x
https://bugs.winehq.org/show_bug.cgi?id=37556
--- Comment #6 from Sebastian Lackner sebastian@fds-team.de --- @Michael Stefaniuc: Please note that the priority for backporting this patch is low because there is currently no known real-world.
https://bugs.winehq.org/show_bug.cgi?id=37556
Alexandre Julliard julliard@winehq.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|RESOLVED |CLOSED
--- Comment #7 from Alexandre Julliard julliard@winehq.org --- Closing bugs fixed in 1.9.4.
https://bugs.winehq.org/show_bug.cgi?id=37556
Michael Stefaniuc mstefani@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Target Milestone|1.8.x |---
--- Comment #8 from Michael Stefaniuc mstefani@redhat.com --- Removing 1.8.x milestone from bugs included in 1.8.2.