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@winehq.org ReportedBy: timurrrr@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).
http://bugs.winehq.org/show_bug.cgi?id=22514
--- Comment #1 from Timur Iskhodzhanov timurrrr@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
http://bugs.winehq.org/show_bug.cgi?id=22514
Dmitry Timoshkov dmitry@codeweavers.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|UNCONFIRMED |RESOLVED Version|unspecified |1.1.43 Resolution| |INVALID
--- Comment #2 from Dmitry Timoshkov dmitry@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
http://bugs.winehq.org/show_bug.cgi?id=22514
Dmitry Timoshkov dmitry@codeweavers.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|RESOLVED |CLOSED
--- Comment #3 from Dmitry Timoshkov dmitry@codeweavers.com 2010-04-28 08:28:22 --- Closing invalid.
http://bugs.winehq.org/show_bug.cgi?id=22514
Timur Iskhodzhanov timurrrr@google.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|CLOSED |UNCONFIRMED Resolution|INVALID |
--- Comment #4 from Timur Iskhodzhanov timurrrr@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.
http://bugs.winehq.org/show_bug.cgi?id=22514
--- Comment #5 from Timur Iskhodzhanov timurrrr@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
http://bugs.winehq.org/show_bug.cgi?id=22514
--- Comment #6 from Timur Iskhodzhanov timurrrr@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
http://bugs.winehq.org/show_bug.cgi?id=22514
Timur Iskhodzhanov timurrrr@google.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Summary|lstrlen is incorrectly |lstrlen is implemented |implemented in |incorrectly |include/winbase.h |(dlls/kernel32/string.c)
http://bugs.winehq.org/show_bug.cgi?id=22514
Dmitry Timoshkov dmitry@codeweavers.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|UNCONFIRMED |RESOLVED Resolution| |INVALID
--- Comment #7 from Dmitry Timoshkov dmitry@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.
http://bugs.winehq.org/show_bug.cgi?id=22514
Dmitry Timoshkov dmitry@codeweavers.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|RESOLVED |CLOSED
--- Comment #8 from Dmitry Timoshkov dmitry@codeweavers.com 2010-04-28 09:04:47 --- Closing.
http://bugs.winehq.org/show_bug.cgi?id=22514
--- Comment #9 from Dmitry Timoshkov dmitry@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.
http://bugs.winehq.org/show_bug.cgi?id=22514
--- Comment #10 from Timur Iskhodzhanov timurrrr@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
http://bugs.winehq.org/show_bug.cgi?id=22514
--- Comment #11 from Dmitry Timoshkov dmitry@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.
http://bugs.winehq.org/show_bug.cgi?id=22514
--- Comment #12 from Timur Iskhodzhanov timurrrr@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
http://bugs.winehq.org/show_bug.cgi?id=22514
--- Comment #13 from Dmitry Timoshkov dmitry@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.
http://bugs.winehq.org/show_bug.cgi?id=22514
--- Comment #14 from Lei Zhang thestig@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
http://bugs.winehq.org/show_bug.cgi?id=22514
--- Comment #15 from Timur Iskhodzhanov timurrrr@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?
http://bugs.winehq.org/show_bug.cgi?id=22514
Timur Iskhodzhanov timurrrr@google.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Attachment #27595|0 |1 is obsolete| |
--- Comment #16 from Timur Iskhodzhanov timurrrr@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
http://bugs.winehq.org/show_bug.cgi?id=22514
--- Comment #17 from Timur Iskhodzhanov timurrrr@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.
http://bugs.winehq.org/show_bug.cgi?id=22514
Timur Iskhodzhanov timurrrr@google.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|CLOSED |UNCONFIRMED Resolution|INVALID |
--- Comment #18 from Timur Iskhodzhanov timurrrr@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
http://bugs.winehq.org/show_bug.cgi?id=22514
Timur Iskhodzhanov timurrrr@google.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Attachment #27611|0 |1 is obsolete| |
--- Comment #19 from Timur Iskhodzhanov timurrrr@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)
http://bugs.winehq.org/show_bug.cgi?id=22514
--- Comment #20 from Dmitry Timoshkov dmitry@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.
http://bugs.winehq.org/show_bug.cgi?id=22514
Dmitry Timoshkov dmitry@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| |
http://bugs.winehq.org/show_bug.cgi?id=22514
Dmitry Timoshkov dmitry@codeweavers.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|UNCONFIRMED |RESOLVED Resolution| |INVALID
--- Comment #21 from Dmitry Timoshkov dmitry@codeweavers.com 2010-04-29 05:53:26 --- Invalid.
http://bugs.winehq.org/show_bug.cgi?id=22514
Dmitry Timoshkov dmitry@codeweavers.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|RESOLVED |CLOSED
--- Comment #22 from Dmitry Timoshkov dmitry@codeweavers.com 2010-04-29 05:53:41 --- Do not reopen.
http://bugs.winehq.org/show_bug.cgi?id=22514
--- Comment #23 from Alexandre Julliard julliard@winehq.org 2010-04-29 09:04:49 --- The content of attachment 27612 has been deleted by Alexandre Julliard julliard@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.
http://bugs.winehq.org/show_bug.cgi?id=22514
--- Comment #24 from Timur Iskhodzhanov timurrrr@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.
http://bugs.winehq.org/show_bug.cgi?id=22514
--- Comment #25 from Dmitry Timoshkov dmitry@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.
http://bugs.winehq.org/show_bug.cgi?id=22514
--- Comment #26 from Timur Iskhodzhanov timurrrr@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.
http://bugs.winehq.org/show_bug.cgi?id=22514
--- Comment #27 from Alexandre Julliard julliard@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.
http://bugs.winehq.org/show_bug.cgi?id=22514
--- Comment #28 from Dmitry Timoshkov dmitry@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.
-
wine-bugs＠winehq.org