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

[Alexandre Julliard]

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;

You can't just return like that without cleaning up.

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

[Nikolay Sivov]

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.

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

[Dmitry Timoshkov]

Nikolay Sivov  wrote:
> Also it's not
> clear for me what happens on wine exactly, lstrlenW() already handles
> page fault exception.

http://source.winehq.org/git/wine.git/blob/HEAD:/include/winbase.h#l2619

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