Re: [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  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.