Re: comctl32: tooltips: avoid buffer overrun (spotted by [e-mail], bug #8361), make sure some strings are NUL-terminated

[Alexandre Julliard]

Mikołaj Zalewski  writes:
> @@ -389,10 +390,10 @@ static void TOOLTIPS_GetDispInfoW(HWND hwnd, TOOLTIPS_INFO *infoPtr, TTTOOL_INFO
>                  sizeof(ttnmdi.szText)/sizeof(ttnmdi.szText[0]) : INFOTIPSIZE-1;
>          lstrcpynW(infoPtr->szTipText, ttnmdi.lpszText, max_len);
>          if (ttnmdi.uFlags & TTF_DI_SETITEM) {
> -
>        INT len = max(strlenW(ttnmdi.lpszText), max_len);
> -
>        INT len = min(strlenW(ttnmdi.lpszText), max_len);

It the text is really allowed to not be null-terminated then calling
strlenW on it is wrong in any case. It does seem surprising though,
are you sure we really need to support this?

comctl32: tooltips: avoid buffer overrun (spotted by [e-mail], bug #8361), make sure some strings are NUL-terminated

[Mikołaj Zalewski]

Alexandre Julliard wrote:
> Mikołaj Zalewski  writes:
> > @@ -389,10 +390,10 @@ static void TOOLTIPS_GetDispInfoW(HWND hwnd, TOOLTIPS_INFO *infoPtr, TTTOOL_INFO
> >                 sizeof(ttnmdi.szText)/sizeof(ttnmdi.szText[0]) : INFOTIPSIZE-1;
> >         lstrcpynW(infoPtr->szTipText, ttnmdi.lpszText, max_len);
> >         if (ttnmdi.uFlags & TTF_DI_SETITEM) {
> > -
> >        INT len = max(strlenW(ttnmdi.lpszText), max_len);
> > -
> >        INT len = min(strlenW(ttnmdi.lpszText), max_len);
>
> It the text is really allowed to not be null-terminated then calling
> strlenW on it is wrong in any case. It does seem surprising though,
> are you sure we really need to support this?

I forgot that only lstrlenW has an exception handler. There is  a 
(somewhat broken) support for non-NULL-terminated string it in the 
current code and I don't know why it was added so I didn't want to 
remove it. But the main part of the patch is the min instead of max. 
Should I change strlenW to lstrlenW or should I remove the 
non-NULL-terminated support?

Mikolaj Zalewski

comctl32: tooltips: avoid buffer overrun (spotted by [e-mail], bug #8361), make sure some strings are NUL-terminated

[Alexandre Julliard]

Mikołaj Zalewski  writes:
> I forgot that only lstrlenW has an exception handler. There is  a
> (somewhat broken) support for non-NULL-terminated string it in the
> current code and I don't know why it was added so I didn't want to
> remove it. But the main part of the patch is the min instead of
> max. Should I change strlenW to lstrlenW or should I remove the
> non-NULL-terminated support?

lstrlenW doesn't have an exception handler inside Wine code so it
wouldn't help, and even if it did it would still only be hiding the
bug. I think the non-null-terminated support should simply be killed,
unless there is a test case that shows that it is needed.