[Bug 22514] New: lstrlen is incorrectly implemented in include/winbase.h
http://bugs.winehq.org/show_bug.cgi?id=22514 Summary: lstrlen is incorrectly implemented in include/winbase.h Product: Wine Version: unspecified Platform: x86 OS/Version: Linux Status: UNCONFIRMED Severity: normal Priority: P2 Component: -unknown AssignedTo: wine-bugs(a)winehq.org ReportedBy: timurrrr(a)google.com Hi. I've been running Chromium tests under wine under Valgrind (see http://wiki.winehq.org/Wine_and_Valgrind ) and found an incorrect implementation of lstrlen in Wine. According to MSDN, lstrlen should return 0 if the argument is NULL See http://msdn.microsoft.com/en-us/library/ms647492(VS.85).aspx In include/winbase.h I found this:
>>> extern inline INT WINAPI lstrlenW( LPCWSTR str ) { const WCHAR *s = str; while (*s) s++; return s - str; }
extern inline INT WINAPI lstrlenA( LPCSTR str ) { return strlen( str ); } <<<<<<<<< Both implementations handle the "str=NULL" case incorrectly. strlen segfaults on Linux if you simply call strlen(NULL). -- Configure bugmail: http://bugs.winehq.org/userprefs.cgi?tab=email 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.
http://bugs.winehq.org/show_bug.cgi?id=22514 --- Comment #1 from Timur Iskhodzhanov <timurrrr(a)google.com> 2010-04-28 08:22:08 --- Created an attachment (id=27595) --> (http://bugs.winehq.org/attachment.cgi?id=27595) A simple patch to work around the issue -- Configure bugmail: http://bugs.winehq.org/userprefs.cgi?tab=email 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.
http://bugs.winehq.org/show_bug.cgi?id=22514 Dmitry Timoshkov <dmitry(a)codeweavers.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|UNCONFIRMED |RESOLVED Version|unspecified |1.1.43 Resolution| |INVALID --- Comment #2 from Dmitry Timoshkov <dmitry(a)codeweavers.com> 2010-04-28 08:28:09 --- Implementations in include/winbase.h are used exclusively inside Wine code where NULL input is not possible and should lead to a crash (as desired). Public (exported) API implementations are in dlls/kernel32/string.c -- Configure bugmail: http://bugs.winehq.org/userprefs.cgi?tab=email 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.
http://bugs.winehq.org/show_bug.cgi?id=22514 Dmitry Timoshkov <dmitry(a)codeweavers.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|RESOLVED |CLOSED --- Comment #3 from Dmitry Timoshkov <dmitry(a)codeweavers.com> 2010-04-28 08:28:22 --- Closing invalid. -- Configure bugmail: http://bugs.winehq.org/userprefs.cgi?tab=email 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.
http://bugs.winehq.org/show_bug.cgi?id=22514 Timur Iskhodzhanov <timurrrr(a)google.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|CLOSED |UNCONFIRMED Resolution|INVALID | --- Comment #4 from Timur Iskhodzhanov <timurrrr(a)google.com> 2010-04-28 08:37:39 --- Ah, sorry. However, I think the implementation in dlls/kernel32/string.c is wrong as well:
>>>>>>>>>> INT WINAPI lstrlenA( LPCSTR str ) { INT ret; __TRY { ret = strlen(str); } __EXCEPT_PAGE_FAULT { SetLastError( ERROR_INVALID_PARAMETER ); return 0; } __ENDTRY return ret; }
/*********************************************************************** * lstrlenW (KERNEL32.@) */ INT WINAPI lstrlenW( LPCWSTR str ) { INT ret; __TRY { ret = strlenW(str); } __EXCEPT_PAGE_FAULT { SetLastError( ERROR_INVALID_PARAMETER ); return 0; } __ENDTRY return ret; } <<<<<<<<<<<<<<<<< As you can see, it calls SetLastError if we pass NULL as a parameter. Also, it reads *(NULL) which is why Valgrind is reporting "uninitialized reads". I don't think it's a good idea to read *(NULL) if we can easily do without. -- Configure bugmail: http://bugs.winehq.org/userprefs.cgi?tab=email 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.
http://bugs.winehq.org/show_bug.cgi?id=22514 --- Comment #5 from Timur Iskhodzhanov <timurrrr(a)google.com> 2010-04-28 08:39:03 --- Created an attachment (id=27596) --> (http://bugs.winehq.org/attachment.cgi?id=27596) A short snippet to test whether lstrlen calls SetLastError Here's an output: $ cl wine.c ... $ wine.exe GetLastError() = 0 lstrlenA(NULL) = 0 GetLastError() = 0 lstrlenW(NULL) = 0 GetLastError() = 0 -- Configure bugmail: http://bugs.winehq.org/userprefs.cgi?tab=email 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.
http://bugs.winehq.org/show_bug.cgi?id=22514 --- Comment #6 from Timur Iskhodzhanov <timurrrr(a)google.com> 2010-04-28 08:43:55 --- I get a different output under Wine $ wine wine.exe GetLastError() = 0 lstrlenA(NULL) = 0 GetLastError() = 5 lstrlenW(NULL) = 0 GetLastError() = 5 -- Configure bugmail: http://bugs.winehq.org/userprefs.cgi?tab=email 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.
http://bugs.winehq.org/show_bug.cgi?id=22514 Timur Iskhodzhanov <timurrrr(a)google.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Summary|lstrlen is incorrectly |lstrlen is implemented |implemented in |incorrectly |include/winbase.h |(dlls/kernel32/string.c) -- Configure bugmail: http://bugs.winehq.org/userprefs.cgi?tab=email 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.
http://bugs.winehq.org/show_bug.cgi?id=22514 Dmitry Timoshkov <dmitry(a)codeweavers.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|UNCONFIRMED |RESOLVED Resolution| |INVALID --- Comment #7 from Dmitry Timoshkov <dmitry(a)codeweavers.com> 2010-04-28 09:04:35 --- That's a pure speculation. If you have an app that depend on that feel free to reopen the bug. -- Configure bugmail: http://bugs.winehq.org/userprefs.cgi?tab=email 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.
http://bugs.winehq.org/show_bug.cgi?id=22514 Dmitry Timoshkov <dmitry(a)codeweavers.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|RESOLVED |CLOSED --- Comment #8 from Dmitry Timoshkov <dmitry(a)codeweavers.com> 2010-04-28 09:04:47 --- Closing. -- Configure bugmail: http://bugs.winehq.org/userprefs.cgi?tab=email 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.
http://bugs.winehq.org/show_bug.cgi?id=22514 --- Comment #9 from Dmitry Timoshkov <dmitry(a)codeweavers.com> 2010-04-28 09:06:43 --- And yes, some applications expect an exception when they call lstrlen(NULL), and do handle that on their own. -- Configure bugmail: http://bugs.winehq.org/userprefs.cgi?tab=email 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.
http://bugs.winehq.org/show_bug.cgi?id=22514 --- Comment #10 from Timur Iskhodzhanov <timurrrr(a)google.com> 2010-04-28 09:10:31 --- Chromium code calls lstrlen with NULL and it produces false warnings when we run Chromium tests under Wine under Valgrind -- Configure bugmail: http://bugs.winehq.org/userprefs.cgi?tab=email 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.
http://bugs.winehq.org/show_bug.cgi?id=22514 --- Comment #11 from Dmitry Timoshkov <dmitry(a)codeweavers.com> 2010-04-28 10:06:09 --- (In reply to comment #10)
Chromium code calls lstrlen with NULL
Is there any reason Chromium code does that? Is that a bug in Chromium?
and it produces false warnings when we run Chromium tests under Wine under Valgrind
How is that a Wine bug? Sounds more like a Valgrind one. -- Configure bugmail: http://bugs.winehq.org/userprefs.cgi?tab=email 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.
http://bugs.winehq.org/show_bug.cgi?id=22514 --- Comment #12 from Timur Iskhodzhanov <timurrrr(a)google.com> 2010-04-28 10:14:59 --- For me it's clearly a wine bug since the native Windows run of an .exe file gives different results from a run under wine. === Chromium uses the documented "If lpString is NULL, the return value is 0" feature. What Valgrind is complaining about is reading *(NULL). I know you do it in the try-statement but a) I don't think it's a good idea to handle this case using exceptions b) the native implementation doesn't call SetLastError while Wine does -- Configure bugmail: http://bugs.winehq.org/userprefs.cgi?tab=email 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.
http://bugs.winehq.org/show_bug.cgi?id=22514 --- Comment #13 from Dmitry Timoshkov <dmitry(a)codeweavers.com> 2010-04-28 10:20:15 --- (In reply to comment #12)
Chromium uses the documented "If lpString is NULL, the return value is 0" feature.
For what? Wine perfectly does that as well.
What Valgrind is complaining about is reading *(NULL). I know you do it in the try-statement but a) I don't think it's a good idea to handle this case using exceptions
Windows does that in an exception handler, Wine must follow that.
b) the native implementation doesn't call SetLastError while Wine does
Please feel free to send a patch with a test case to wine-patches. -- Configure bugmail: http://bugs.winehq.org/userprefs.cgi?tab=email 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.
http://bugs.winehq.org/show_bug.cgi?id=22514 --- Comment #14 from Lei Zhang <thestig(a)google.com> 2010-04-28 19:31:53 --- (In reply to comment #13)
(In reply to comment #12)
b) the native implementation doesn't call SetLastError while Wine does
Please feel free to send a patch with a test case to wine-patches.
http://www.winehq.org/pipermail/wine-patches/2010-April/087881.html -- Configure bugmail: http://bugs.winehq.org/userprefs.cgi?tab=email 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.
http://bugs.winehq.org/show_bug.cgi?id=22514 --- Comment #15 from Timur Iskhodzhanov <timurrrr(a)google.com> 2010-04-29 04:47:43 --- Created an attachment (id=27610) --> (http://bugs.winehq.org/attachment.cgi?id=27610) A simple test that shows lstrlen(0) is handled differently to lstrlen(1) Lei, the test you've proposed is perfectly fine for me. However, I still disagree about the lstrlen implementation :-) What I'm trying to say is NULL argument is documented as "OK" in MSDN http://msdn.microsoft.com/en-us/library/ms647492(VS.85).aspx -> "lstrlen assumes that lpString is a null-terminated string, or NULL. If it is not, this could lead to a buffer overrun or a denial of service attack against your application." Looks like it doesn't really read from *NULL when run natively: I've added a call to lstrlenW(1) and ran the program under DrMemory (it's a win tool similar to Valgrind)
>>>>>>>>>>>>>>>> $ cl wine.c && wine.exe 5: GetLastError() = 0 7: lstrlenW(NULL) = 0 8: GetLastError() = 0 10: lstrlenW(1) = 0 11: GetLastError() = 0 13: lstrlenA(NULL) = 0 14: GetLastError() = 0
$ drmemory.exe wine.exe 5: GetLastError() = 0 7: lstrlenW(NULL) = 0 8: GetLastError() = 0 10: lstrlenW(1) = 0 11: GetLastError() = 0 13: lstrlenA(NULL) = 0 14: GetLastError() = 0 (from logdir/global.XXX.log) Error #1: UNADDRESSABLE ACCESS: reading 0x00000001-0x00000003 2 byte(s) within 0x00000001-0x00000003 @0:00:00.609 in thread 472 0x7c90fe60 <ntdll.dll+0xfe60> 0x7c809acc <KERNEL32.dll+0x9acc> 0x0040106f <wine.exe+0x106f> 0x0040175c <wine.exe+0x175c> 0x7c817077 <KERNEL32.dll+0x17077> <<<<<<<<<<<<<<<<<<<< Also, DrMemory barks if I do strlen(NULL) inside __try/__except(GetExceptionCode() == EXCEPTION_ACCESS_VIOLATION) section. It doesn't bark on lstrlenW(0). That's why I want to add "if (str == NULL) return 0;" BEFORE the __try statement. ===================
And yes, some applications expect an exception when they call lstrlen(NULL), and do handle that on their own. Dmitry, can you please give an example?
-- Configure bugmail: http://bugs.winehq.org/userprefs.cgi?tab=email 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.
http://bugs.winehq.org/show_bug.cgi?id=22514 Timur Iskhodzhanov <timurrrr(a)google.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #27595|0 |1 is obsolete| | --- Comment #16 from Timur Iskhodzhanov <timurrrr(a)google.com> 2010-04-29 04:53:02 --- Created an attachment (id=27611) --> (http://bugs.winehq.org/attachment.cgi?id=27611) Proposed patch for dlls/kernel32/string.c -- Configure bugmail: http://bugs.winehq.org/userprefs.cgi?tab=email 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.
http://bugs.winehq.org/show_bug.cgi?id=22514 --- Comment #17 from Timur Iskhodzhanov <timurrrr(a)google.com> 2010-04-29 05:11:14 --- Created an attachment (id=27612) --> (http://bugs.winehq.org/attachment.cgi?id=27612) lstrlenW disassembly (got it using MS VS debugger) I clearly see a fast-exit for lpString==NULL and I don't see any exception handling in this case. -- Configure bugmail: http://bugs.winehq.org/userprefs.cgi?tab=email 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.
http://bugs.winehq.org/show_bug.cgi?id=22514 Timur Iskhodzhanov <timurrrr(a)google.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|CLOSED |UNCONFIRMED Resolution|INVALID | --- Comment #18 from Timur Iskhodzhanov <timurrrr(a)google.com> 2010-04-29 05:12:16 --- Re-opening the bug once again at least because wine is failing on the test Lei proposed: http://www.winehq.org/pipermail/wine-patches/2010-April/087881.html -- Configure bugmail: http://bugs.winehq.org/userprefs.cgi?tab=email 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.
http://bugs.winehq.org/show_bug.cgi?id=22514 Timur Iskhodzhanov <timurrrr(a)google.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #27611|0 |1 is obsolete| | --- Comment #19 from Timur Iskhodzhanov <timurrrr(a)google.com> 2010-04-29 05:42:00 --- Created an attachment (id=27613) --> (http://bugs.winehq.org/attachment.cgi?id=27613) Proposed patch for dlls/kernel32/string.c (3) -- Configure bugmail: http://bugs.winehq.org/userprefs.cgi?tab=email 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.
http://bugs.winehq.org/show_bug.cgi?id=22514 --- Comment #20 from Dmitry Timoshkov <dmitry(a)codeweavers.com> 2010-04-29 05:49:53 --- (In reply to comment #15)
And yes, some applications expect an exception when they call lstrlen(NULL), and do handle that on their own. Dmitry, can you please give an example?
I don't remember which one relies on that.
Created an attachment (id=27612) --> (http://bugs.winehq.org/attachment.cgi?id=27612) [details] lstrlenW disassembly (got it using MS VS debugger)
Don't do that, that's illegal, especially in the Wine context. -- Configure bugmail: http://bugs.winehq.org/userprefs.cgi?tab=email 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.
http://bugs.winehq.org/show_bug.cgi?id=22514 Dmitry Timoshkov <dmitry(a)codeweavers.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #27612|lstrlenW disassembly (got |ADMIN PLEASE REMOVE description|it using MS VS debugger) | Attachment #27612|lstrlenW disassembly.txt |illegal.txt filename| | Attachment #27612|0 |1 is obsolete| | -- Configure bugmail: http://bugs.winehq.org/userprefs.cgi?tab=email 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.
http://bugs.winehq.org/show_bug.cgi?id=22514 Dmitry Timoshkov <dmitry(a)codeweavers.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|UNCONFIRMED |RESOLVED Resolution| |INVALID --- Comment #21 from Dmitry Timoshkov <dmitry(a)codeweavers.com> 2010-04-29 05:53:26 --- Invalid. -- Configure bugmail: http://bugs.winehq.org/userprefs.cgi?tab=email 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.
http://bugs.winehq.org/show_bug.cgi?id=22514 Dmitry Timoshkov <dmitry(a)codeweavers.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|RESOLVED |CLOSED --- Comment #22 from Dmitry Timoshkov <dmitry(a)codeweavers.com> 2010-04-29 05:53:41 --- Do not reopen. -- Configure bugmail: http://bugs.winehq.org/userprefs.cgi?tab=email 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.
http://bugs.winehq.org/show_bug.cgi?id=22514 --- Comment #23 from Alexandre Julliard <julliard(a)winehq.org> 2010-04-29 09:04:49 --- The content of attachment 27612 has been deleted by Alexandre Julliard <julliard(a)winehq.org> who provided the following reason: Disassembling Windows code is not acceptable The token used to delete this attachment was generated at 2010-04-29 09:04:28. -- Configure bugmail: http://bugs.winehq.org/userprefs.cgi?tab=email 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.
http://bugs.winehq.org/show_bug.cgi?id=22514 --- Comment #24 from Timur Iskhodzhanov <timurrrr(a)google.com> 2010-04-29 09:19:10 --- Sorry for that, didn't know the restriction... You may have noticed that the initial suggestions/patches were sent before I went to disassembler and I was talking about externally visible differences between Wine and native Windows.
Do not reopen. Well, I just wanted to make wine better and more usable for my tasks. Probably I can do without pushing this upstream but I'm really surprised you don't want to fix this bug with a tiny patch.
-- Configure bugmail: http://bugs.winehq.org/userprefs.cgi?tab=email 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.
http://bugs.winehq.org/show_bug.cgi?id=22514 --- Comment #25 from Dmitry Timoshkov <dmitry(a)codeweavers.com> 2010-04-29 09:59:55 --- (In reply to comment #24)
Well, I just wanted to make wine better and more usable for my tasks. Probably I can do without pushing this upstream but I'm really surprised you don't want to fix this bug with a tiny patch.
You may want to fix Chromium instead. Passing NULL to strlen() in Linux crashes, Microsoft has added the exception handler to lstrlen() for broken application. -- Configure bugmail: http://bugs.winehq.org/userprefs.cgi?tab=email 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.
http://bugs.winehq.org/show_bug.cgi?id=22514 --- Comment #26 from Timur Iskhodzhanov <timurrrr(a)google.com> 2010-04-29 10:13:56 --- They don't pass NULL to strlen in Chromium. They pass NULL only to lstrlen in windows-specific code and expect 0 in this case. They don't try to pass invalid non-NULL pointers. I doubt they would accept additional "if()" branches to make Wine+Valgrind happy. After all, they use the documented features of lstrlen and they do it correctly. -- Configure bugmail: http://bugs.winehq.org/userprefs.cgi?tab=email 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.
http://bugs.winehq.org/show_bug.cgi?id=22514 --- Comment #27 from Alexandre Julliard <julliard(a)winehq.org> 2010-04-29 10:28:21 --- (In reply to comment #24)
Well, I just wanted to make wine better and more usable for my tasks. Probably I can do without pushing this upstream but I'm really surprised you don't want to fix this bug with a tiny patch.
A patch can be accepted, but it has to come with a test case demonstrating that it's correct, and it can't be done by you since you looked at the disassembly. -- Configure bugmail: http://bugs.winehq.org/userprefs.cgi?tab=email 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.
http://bugs.winehq.org/show_bug.cgi?id=22514 --- Comment #28 from Dmitry Timoshkov <dmitry(a)codeweavers.com> 2010-04-29 10:32:16 --- (In reply to comment #26)
They don't pass NULL to strlen in Chromium.
strlen() functionally is equivalent of lstrlen(), with only exception of internal handling of invalid pointers.
They pass NULL only to lstrlen in windows-specific code and expect 0 in this case. They don't try to pass invalid non-NULL pointers. I doubt they would accept additional "if()" branches to make Wine+Valgrind happy. After all, they use the documented features of lstrlen and they do it correctly.
NULL pointer is invalid by definition. Passing it to lstrlen() should crash, but Microsoft did a favour to broken application and added an exception handler. Claiming that an app uses documented way of passing NULL to lstrlen() and expects it to return 0 is confusing. -- Configure bugmail: http://bugs.winehq.org/userprefs.cgi?tab=email 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