[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: [e-mail]
        ReportedBy: [e-mail]

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).

[Bug 22514] lstrlen is incorrectly implemented in include/winbase.h

http://bugs.winehq.org/show_bug.cgi?id=22514

--- Comment #1 from Timur Iskhodzhanov   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

[Bug 22514] lstrlen is incorrectly implemented in include/winbase.h

http://bugs.winehq.org/show_bug.cgi?id=22514

Dmitry Timoshkov  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |RESOLVED
            Version|unspecified                 |1.1.43
         Resolution|                            |INVALID

--- Comment #2 from Dmitry Timoshkov   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

[Bug 22514] lstrlen is incorrectly implemented in include/winbase.h

http://bugs.winehq.org/show_bug.cgi?id=22514

Dmitry Timoshkov  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |CLOSED

--- Comment #3 from Dmitry Timoshkov   2010-04-28 08:28:22 ---
Closing invalid.

[Bug 22514] lstrlen is incorrectly implemented in include/winbase.h

http://bugs.winehq.org/show_bug.cgi?id=22514

Timur Iskhodzhanov  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|CLOSED                      |UNCONFIRMED
         Resolution|INVALID                     |

--- Comment #4 from Timur Iskhodzhanov   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.

[Bug 22514] lstrlen is incorrectly implemented in include/winbase.h

http://bugs.winehq.org/show_bug.cgi?id=22514

--- Comment #5 from Timur Iskhodzhanov   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

[Bug 22514] lstrlen is incorrectly implemented in include/winbase.h

http://bugs.winehq.org/show_bug.cgi?id=22514

--- Comment #6 from Timur Iskhodzhanov   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

[Bug 22514] lstrlen is implemented incorrectly (dlls/kernel32/string.c)

http://bugs.winehq.org/show_bug.cgi?id=22514

Timur Iskhodzhanov  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|lstrlen is incorrectly      |lstrlen is implemented
                   |implemented in              |incorrectly
                   |include/winbase.h           |(dlls/kernel32/string.c)

[Bug 22514] lstrlen is implemented incorrectly (dlls/kernel32/string.c)

http://bugs.winehq.org/show_bug.cgi?id=22514

Dmitry Timoshkov  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |RESOLVED
         Resolution|                            |INVALID

--- Comment #7 from Dmitry Timoshkov   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.

[Bug 22514] lstrlen is implemented incorrectly (dlls/kernel32/string.c)

http://bugs.winehq.org/show_bug.cgi?id=22514

Dmitry Timoshkov  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |CLOSED

--- Comment #8 from Dmitry Timoshkov   2010-04-28 09:04:47 ---
Closing.

[Bug 22514] lstrlen is implemented incorrectly (dlls/kernel32/string.c)

http://bugs.winehq.org/show_bug.cgi?id=22514

--- Comment #9 from Dmitry Timoshkov   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.

[Bug 22514] lstrlen is implemented incorrectly (dlls/kernel32/string.c)

http://bugs.winehq.org/show_bug.cgi?id=22514

--- Comment #10 from Timur Iskhodzhanov   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

[Bug 22514] lstrlen is implemented incorrectly (dlls/kernel32/string.c)

http://bugs.winehq.org/show_bug.cgi?id=22514

--- Comment #11 from Dmitry Timoshkov   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.

[Bug 22514] lstrlen is implemented incorrectly (dlls/kernel32/string.c)

http://bugs.winehq.org/show_bug.cgi?id=22514

--- Comment #12 from Timur Iskhodzhanov   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

[Bug 22514] lstrlen is implemented incorrectly (dlls/kernel32/string.c)

http://bugs.winehq.org/show_bug.cgi?id=22514

--- Comment #13 from Dmitry Timoshkov   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.

[Bug 22514] lstrlen is implemented incorrectly (dlls/kernel32/string.c)

http://bugs.winehq.org/show_bug.cgi?id=22514

--- Comment #14 from Lei Zhang   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

[Bug 22514] lstrlen is implemented incorrectly (dlls/kernel32/string.c)

http://bugs.winehq.org/show_bug.cgi?id=22514

--- Comment #15 from Timur Iskhodzhanov   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?

[Bug 22514] lstrlen is implemented incorrectly (dlls/kernel32/string.c)

http://bugs.winehq.org/show_bug.cgi?id=22514

Timur Iskhodzhanov  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #27595|0                           |1
        is obsolete|                            |

--- Comment #16 from Timur Iskhodzhanov   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

[Bug 22514] lstrlen is implemented incorrectly (dlls/kernel32/string.c)

http://bugs.winehq.org/show_bug.cgi?id=22514

--- Comment #17 from Timur Iskhodzhanov   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.

[Bug 22514] lstrlen is implemented incorrectly (dlls/kernel32/string.c)

http://bugs.winehq.org/show_bug.cgi?id=22514

Timur Iskhodzhanov  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|CLOSED                      |UNCONFIRMED
         Resolution|INVALID                     |

--- Comment #18 from Timur Iskhodzhanov   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

[Bug 22514] lstrlen is implemented incorrectly (dlls/kernel32/string.c)

http://bugs.winehq.org/show_bug.cgi?id=22514

Timur Iskhodzhanov  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #27611|0                           |1
        is obsolete|                            |

--- Comment #19 from Timur Iskhodzhanov   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)

[Bug 22514] lstrlen is implemented incorrectly (dlls/kernel32/string.c)

http://bugs.winehq.org/show_bug.cgi?id=22514

--- Comment #20 from Dmitry Timoshkov   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.

[Bug 22514] lstrlen is implemented incorrectly (dlls/kernel32/string.c)

http://bugs.winehq.org/show_bug.cgi?id=22514

Dmitry Timoshkov  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|                            |

[Bug 22514] lstrlen is implemented incorrectly (dlls/kernel32/string.c)

http://bugs.winehq.org/show_bug.cgi?id=22514

Dmitry Timoshkov  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |RESOLVED
         Resolution|                            |INVALID

--- Comment #21 from Dmitry Timoshkov   2010-04-29 05:53:26 ---
Invalid.

[Bug 22514] lstrlen is implemented incorrectly (dlls/kernel32/string.c)

http://bugs.winehq.org/show_bug.cgi?id=22514

Dmitry Timoshkov  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |CLOSED

--- Comment #22 from Dmitry Timoshkov   2010-04-29 05:53:41 ---
Do not reopen.

[Bug 22514] lstrlen is implemented incorrectly (dlls/kernel32/string.c)

http://bugs.winehq.org/show_bug.cgi?id=22514

--- Comment #23 from Alexandre Julliard   2010-04-29 09:04:49 ---
The content of attachment 27612 has been deleted by
    Alexandre Julliard 
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.

[Bug 22514] lstrlen is implemented incorrectly (dlls/kernel32/string.c)

http://bugs.winehq.org/show_bug.cgi?id=22514

--- Comment #24 from Timur Iskhodzhanov   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.

[Bug 22514] lstrlen is implemented incorrectly (dlls/kernel32/string.c)

http://bugs.winehq.org/show_bug.cgi?id=22514

--- Comment #25 from Dmitry Timoshkov   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.

[Bug 22514] lstrlen is implemented incorrectly (dlls/kernel32/string.c)

http://bugs.winehq.org/show_bug.cgi?id=22514

--- Comment #26 from Timur Iskhodzhanov   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.

[Bug 22514] lstrlen is implemented incorrectly (dlls/kernel32/string.c)

http://bugs.winehq.org/show_bug.cgi?id=22514

--- Comment #27 from Alexandre Julliard   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.

[Bug 22514] lstrlen is implemented incorrectly (dlls/kernel32/string.c)

http://bugs.winehq.org/show_bug.cgi?id=22514

--- Comment #28 from Dmitry Timoshkov   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.