[PATCH] comctl32/tooltip: Protect TTM_ADDTOOLW from invalid text pointers (resend)

[Nikolay Sivov]

On 17.02.2016 18:39, Nikolay Sivov wrote:
> On 17.02.2016 18:28, Alexandre Julliard wrote:
>> Alistair Leslie-Hughes <leslie_alistair at hotmail.com> writes:
>>
>>> @@ -1076,10 +1077,19 @@ TOOLTIPS_AddToolT (TOOLTIPS_INFO *infoPtr, const TTTOOLINFOW *ti, BOOL isW)
>>>                  toolPtr->lpszText = LPSTR_TEXTCALLBACKW;
>>>              }
>>>              else if (isW) {
>>> -                INT len = lstrlenW (ti->lpszText);
>>> -                TRACE("add text %s!\n", debugstr_w(ti->lpszText));
>>> -                toolPtr->lpszText =	Alloc ((len + 1)*sizeof(WCHAR));
>>> -                strcpyW (toolPtr->lpszText, ti->lpszText);
>>> +                __TRY
>>> +                {
>>> +                    INT len = lstrlenW (ti->lpszText);
>>> +                    TRACE("add text %s!\n", debugstr_w(ti->lpszText));
>>> +                    toolPtr->lpszText =	Alloc ((len + 1)*sizeof(WCHAR));
>>> +                    strcpyW (toolPtr->lpszText, ti->lpszText);
>>> +                }
>>> +                __EXCEPT_PAGE_FAULT
>>> +                {
>>> +                    WARN("Invalid lpszText.\n");
>>> +                    return FALSE;
>>
> 
> If we're going to fix it like that please remove exclamation mark from
> trace message, and a tab in front of Alloc. Regarding tests, why are
> they commented out, does it sometimes crash on windows? Also it's not
> clear for me what happens on wine exactly, lstrlenW() already handles
> page fault exception.
> 

Actually, since it will crash in lstrlenW, you can wrap only that in
__TRY {} section, leaving allocation and everything else out of it.
Cleaner way in my opinion would be to ban lstr* stuff from wine source
(except tests), and use actual lstrlenW in this case, checking for error
on return to see if it caught an exception; but that's a lot to clean up
and clearly is a side task, if we're doing it.