Re: [v6 PATCH] comctl32/tooltip: Protect TTM_ADDTOOLW from invalid text pointers

[Nikolay Sivov]

On 22.04.2016 10:55, Alistair Leslie-Hughes wrote:
> Fixes https://bugs.winehq.org/show_bug.cgi?id=10347
>
> Signed-off-by: Alistair Leslie-Hughes <leslie_alistair(a)hotmail.com>
> ---
>  dlls/comctl32/tests/tooltips.c | 39 +++++++++++++++++++++++++++++++++++++++
>  dlls/comctl32/tooltips.c       |  5 ++++-
>  2 files changed, 43 insertions(+), 1 deletion(-)
>
> diff --git a/dlls/comctl32/tests/tooltips.c b/dlls/comctl32/tests/tooltips.c
> index 3382fce..db3d3c4 100644
> --- a/dlls/comctl32/tests/tooltips.c
> +++ b/dlls/comctl32/tests/tooltips.c
> @@ -446,6 +446,45 @@ static void test_gettext(void)
>      r = SendMessageW(hwnd, TTM_ADDTOOLW, 0, (LPARAM)&toolinfoW);
>      ok(!r, "Adding the tool to the tooltip succeeded!\n");
>
> +    /* lpszText with an invalid address */
> +    toolinfoW.cbSize = sizeof(TTTOOLINFOW);
> +    toolinfoW.hwnd = notify;
> +    toolinfoW.hinst = GetModuleHandleA(NULL);
> +    toolinfoW.uFlags = 0;
> +    toolinfoW.uId = 0;
> +    toolinfoW.lpszText = (LPWSTR)0xdeadbeef;
> +    toolinfoW.lParam = 0;
> +    GetClientRect(hwnd, &toolinfoW.rect);
> +    r = SendMessageA(hwnd, TTM_ADDTOOLW, 0, (LPARAM)&toolinfoW);
> +    ok(!r, "Adding the tool to the tooltip succeeded!\n");
> +
> +    /* lpszText with an callback address */
> +    toolinfoW.cbSize = sizeof(TTTOOLINFOW);
> +    toolinfoW.hwnd = notify;
> +    toolinfoW.hinst = GetModuleHandleA(NULL);
> +    toolinfoW.uFlags = 0;
> +    toolinfoW.uId = 0;
> +    toolinfoW.lpszText = LPSTR_TEXTCALLBACKW;
> +    toolinfoW.lParam = 0;
> +    GetClientRect(hwnd, &toolinfoW.rect);
> +    r = SendMessageA(hwnd, TTM_ADDTOOLW, 0, (LPARAM)&toolinfoW);
> +    todo_wine ok(!r, "Adding the tool to the tooltip succeeded!\n");
> +

This is broken, 'notify' is invalid at this point, take a look at a
patch I sent couple days ago. Also these tests don't work for me even
with valid string pointers, and with reduce cbSize and invalid pointers
they crash on Windows. So the whole idea might be wrong.
> if (0)  /* crashes on NT4 */
>      {
>          toolinfoW.hwnd = NULL;
> diff --git a/dlls/comctl32/tooltips.c b/dlls/comctl32/tooltips.c
> index 8bf6919..804e9ed 100644
> --- a/dlls/comctl32/tooltips.c
> +++ b/dlls/comctl32/tooltips.c
> @@ -1038,7 +1038,10 @@ TOOLTIPS_AddToolT (TOOLTIPS_INFO *infoPtr, const TTTOOLINFOW *ti, BOOL isW)
>      infoPtr->hwndSelf, ti->hwnd, ti->uId,
>      (ti->uFlags & TTF_IDISHWND) ? " TTF_IDISHWND" : "");
>
> -    if (ti->cbSize >= TTTOOLINFOW_V2_SIZE && !ti->lpszText && isW)
> +    if (ti->cbSize >= TTTOOLINFOW_V2_SIZE && isW
> +         && !TOOLTIPS_IsCallbackString(ti->lpszText, isW)
> +         && !(ti->lpszText && IS_INTRESOURCE(ti->lpszText))
> +         && IsBadStringPtrW(ti->lpszText, sizeof(WCHAR)) )
>          return FALSE;

Text check is wrong.