[PATCH] user32: Add an exception handler to GetWindowText.

[Dmitry Timoshkov]

Signed-off-by: Dmitry Timoshkov 
---
 dlls/user32/tests/win.c | 29 ++++++++++++++++----
 dlls/user32/win.c       | 59 ++++++++++++++++++++++++++++++-----------
 2 files changed, 67 insertions(+), 21 deletions(-)

diff --git a/dlls/user32/tests/win.c b/dlls/user32/tests/win.c
index 6b13d93be14..2679ea0813f 100644
--- a/dlls/user32/tests/win.c
+++ b/dlls/user32/tests/win.c
@@ -7923,17 +7923,36 @@ static void test_gettext(void)
     if (0)
     {
     r = SendMessageA( hwnd, WM_GETTEXT, 0x10, 0x1000);
-    ok( r == 0, "settext should return zero\n");
+    ok(r == 0, "WM_GETTEXT should return zero (%ld)\n", r);

r = SendMessageA( hwnd, WM_GETTEXT, 0x10000, 0);
-    ok( r == 0, "settext should return zero (%ld)\n", r);
+    ok(r == 0, "WM_GETTEXT should return zero (%ld)\n", r);

r = SendMessageA( hwnd, WM_GETTEXT, 0xff000000, 0x1000);
-    ok( r == 0, "settext should return zero (%ld)\n", r);
+    ok(r == 0, "WM_GETTEXT should return zero (%ld)\n", r);

r = SendMessageA( hwnd, WM_GETTEXT, 0x1000, 0xff000000);
-    ok( r == 0, "settext should return zero (%ld)\n", r);
-    }
+    ok(r == 0, "WM_GETTEXT should return zero (%ld)\n", r);
+    }
+
+    /* GetWindowText doesn't crash */
+    r = GetWindowTextA(hwnd, (LPSTR)0x10, 0x1000);
+    ok(r == 0, "GetWindowText should return zero (%ld)\n", r);
+    r = GetWindowTextA(hwnd, (LPSTR)0x10000, 0);
+    ok(r == 0, "GetWindowText should return zero (%ld)\n", r);
+    r = GetWindowTextA(hwnd, (LPSTR)0xff000000, 0x1000);
+    ok(r == 0, "GetWindowText should return zero (%ld)\n", r);
+    r = GetWindowTextA(hwnd, (LPSTR)0x1000, 0xff000000);
+    ok(r == 0, "GetWindowText should return zero (%ld)\n", r);
+
+    r = GetWindowTextW(hwnd, (LPWSTR)0x10, 0x1000);
+    ok(r == 0, "GetWindowText should return zero (%ld)\n", r);
+    r = GetWindowTextW(hwnd, (LPWSTR)0x10000, 0);
+    ok(r == 0, "GetWindowText should return zero (%ld)\n", r);
+    r = GetWindowTextW(hwnd, (LPWSTR)0xff000000, 0x1000);
+    ok(r == 0, "GetWindowText should return zero (%ld)\n", r);
+    r = GetWindowTextW(hwnd, (LPWSTR)0x1000, 0xff000000);
+    ok(r == 0, "GetWindowText should return zero (%ld)\n", r);

DestroyWindow(hwnd);
 }
diff --git a/dlls/user32/win.c b/dlls/user32/win.c
index 5e89f4c2c97..e57f291be35 100644
--- a/dlls/user32/win.c
+++ b/dlls/user32/win.c
@@ -35,6 +35,7 @@
 #include "controls.h"
 #include "winerror.h"
 #include "wine/gdi_driver.h"
+#include "wine/exception.h"
 #include "wine/debug.h"

WINE_DEFAULT_DEBUG_CHANNEL(win);
@@ -3001,22 +3002,34 @@ LONG WINAPI DECLSPEC_HOTPATCH SetWindowLongW(
 INT WINAPI GetWindowTextA( HWND hwnd, LPSTR lpString, INT nMaxCount )
 {
     WCHAR *buffer;
+    int ret = 0;

if (!lpString || nMaxCount <= 0) return 0;

-    if (WIN_IsCurrentProcess( hwnd ))
+    __TRY
     {
-        lpString[0] = 0;
-        return (INT)SendMessageA( hwnd, WM_GETTEXT, nMaxCount, (LPARAM)lpString );
+        if (WIN_IsCurrentProcess( hwnd ))
+        {
+            lpString[0] = 0;
+            ret = (INT)SendMessageA( hwnd, WM_GETTEXT, nMaxCount, (LPARAM)lpString );
+        }
+        else if ((buffer = HeapAlloc( GetProcessHeap(), 0, nMaxCount * sizeof(WCHAR) )))
+        {
+            /* when window belongs to other process, don't send a message */
+            get_server_window_text( hwnd, buffer, nMaxCount );
+            if (!WideCharToMultiByte( CP_ACP, 0, buffer, -1, lpString, nMaxCount, NULL, NULL ))
+                lpString[nMaxCount-1] = 0;
+            HeapFree( GetProcessHeap(), 0, buffer );
+            ret = strlen(lpString);
+        }
+    }
+    __EXCEPT_ALL
+    {
+        ret = 0;
     }
+    __ENDTRY

-    /* when window belongs to other process, don't send a message */
-    if (!(buffer = HeapAlloc( GetProcessHeap(), 0, nMaxCount * sizeof(WCHAR) ))) return 0;
-    get_server_window_text( hwnd, buffer, nMaxCount );
-    if (!WideCharToMultiByte( CP_ACP, 0, buffer, -1, lpString, nMaxCount, NULL, NULL ))
-        lpString[nMaxCount-1] = 0;
-    HeapFree( GetProcessHeap(), 0, buffer );
-    return strlen(lpString);
+    return ret;
 }

@@ -3049,17 +3062,31 @@ INT WINAPI InternalGetWindowText(HWND hwnd,LPWSTR lpString,INT nMaxCount )
  */
 INT WINAPI GetWindowTextW( HWND hwnd, LPWSTR lpString, INT nMaxCount )
 {
+    int ret;
+
     if (!lpString || nMaxCount <= 0) return 0;

-    if (WIN_IsCurrentProcess( hwnd ))
+    __TRY
     {
-        lpString[0] = 0;
-        return (INT)SendMessageW( hwnd, WM_GETTEXT, nMaxCount, (LPARAM)lpString );
+        if (WIN_IsCurrentProcess( hwnd ))
+        {
+            lpString[0] = 0;
+            ret = (INT)SendMessageW( hwnd, WM_GETTEXT, nMaxCount, (LPARAM)lpString );
+        }
+        else
+        {
+            /* when window belongs to other process, don't send a message */
+            get_server_window_text( hwnd, lpString, nMaxCount );
+            ret = lstrlenW(lpString);
+        }
     }
+    __EXCEPT_ALL
+    {
+        ret = 0;
+    }
+    __ENDTRY

-    /* when window belongs to other process, don't send a message */
-    get_server_window_text( hwnd, lpString, nMaxCount );
-    return lstrlenW(lpString);
+    return ret;
 }

[Rémi Bernon]

On 10/13/21 11:41 AM, Dmitry Timoshkov wrote:
> Signed-off-by: Dmitry Timoshkov
>
> dlls/user32/tests/win.c | 29 ++++++++++++++++----
>   dlls/user32/win.c       | 59 ++++++++++++++++++++++++++++++-----------
>   2 files changed, 67 insertions(+), 21 deletions(-)
>
> diff --git a/dlls/user32/tests/win.c b/dlls/user32/tests/win.c
> index 6b13d93be14..2679ea0813f 100644
> --- a/dlls/user32/tests/win.c
> +++ b/dlls/user32/tests/win.c
> @@ -7923,17 +7923,36 @@ static void test_gettext(void)
>       if (0)
>       {
>       r = SendMessageA( hwnd, WM_GETTEXT, 0x10, 0x1000);
> - ok( r == 0, "settext should return zero\n");
> -
>
> ok(r == 0, "WM_GETTEXT should return zero (%ld)\n", r);
>
> r = SendMessageA( hwnd, WM_GETTEXT, 0x10000, 0);
> - ok( r == 0, "settext should return zero (%ld)\n", r);
> -
>
> ok(r == 0, "WM_GETTEXT should return zero (%ld)\n", r);
>
> r = SendMessageA( hwnd, WM_GETTEXT, 0xff000000, 0x1000);
> - ok( r == 0, "settext should return zero (%ld)\n", r);
> -
>
> ok(r == 0, "WM_GETTEXT should return zero (%ld)\n", r);
>
> r = SendMessageA( hwnd, WM_GETTEXT, 0x1000, 0xff000000);
> - ok( r == 0, "settext should return zero (%ld)\n", r);
> - }
> - ok(r == 0, "WM_GETTEXT should return zero (%ld)\n", r);
> - }

FWIW everything else in this test function (and as far as I could see 
most user32, or at least recently added, code) uses space-in-paren 
style, if you're fixing the spaces I think it should be the other way 
around ;)

[Dmitry Timoshkov]

Rémi Bernon  wrote:
> FWIW everything else in this test function (and as far as I could see 
> most user32, or at least recently added, code) uses space-in-paren 
> style, if you're fixing the spaces I think it should be the other way 
> around ;)

Well, the intent was actually to fix the failure message. I've got an impression
that having a space after the first parenthese while the closing one doesn't have
it is a typo. If removing the first space is a problem, I can surely resend.

[Rémi Bernon]

On 10/13/21 11:58 AM, Dmitry Timoshkov wrote:
> Rémi Bernon  wrote:
> > FWIW everything else in this test function (and as far as I could see
> > most user32, or at least recently added, code) uses space-in-paren
> > style, if you're fixing the spaces I think it should be the other way
> > around ;)
>
> Well, the intent was actually to fix the failure message. I've got an impression
> that having a space after the first parenthese while the closing one doesn't have
> it is a typo. If removing the first space is a problem, I can surely resend.

Imho inconsistent style makes code much more difficult to read, and I 
think we should aim to make things more consistent, not less, -at least 
on the module scope- and use opportunities like this to fix inconsistencies.

For some modules it's hard to tell, but doing it a local basis too is 
better than nothing.

[Marvin]

Hi,

While running your changed tests, I think I found new failures.
Being a bot and all I'm not very good at pattern recognition, so I might be
wrong, but could you please double-check?

Full results can be found at:
https://testbot.winehq.org/JobDetails.pl?Key=99950

Your paranoid android.

=== w1064_tsign (64 bit report) ===

user32:
win.c:4073: Test failed: hwnd 0000000000020178/0000000000020178 message 0200
win.c:4077: Test failed: hwnd 0000000000020178/0000000000020178 message 0201
win.c:4086: Test failed: hwnd 00000000007E0272/00000000007E0272 message 0202
win.c:4089: Test failed: hwnd 00000000007E0272/00000000007E0272 message 0200
win.c:4135: Test failed: hwnd 0000000000050054/0000000000050054 message 0200
win.c:4137: Test failed: wparam 0
win.c:4148: Test failed: hwnd 0000000000050054/0000000000050054 message 0203
win.c:4152: Test failed: message 0202 available

[Rémi Bernon]

On 10/13/21 11:41 AM, Dmitry Timoshkov wrote:
> Signed-off-by: Dmitry Timoshkov
>
> dlls/user32/tests/win.c | 29 ++++++++++++++++----
>   dlls/user32/win.c       | 59 ++++++++++++++++++++++++++++++-----------
>   2 files changed, 67 insertions(+), 21 deletions(-)
>
> diff --git a/dlls/user32/tests/win.c b/dlls/user32/tests/win.c
> index 6b13d93be14..2679ea0813f 100644
> --- a/dlls/user32/tests/win.c
> +++ b/dlls/user32/tests/win.c
> @@ -7923,17 +7923,36 @@ static void test_gettext(void)
>       if (0)
>       {
>       r = SendMessageA( hwnd, WM_GETTEXT, 0x10, 0x1000);
> - ok( r == 0, "settext should return zero\n");
> -
>
> ok(r == 0, "WM_GETTEXT should return zero (%ld)\n", r);
>
> r = SendMessageA( hwnd, WM_GETTEXT, 0x10000, 0);
> - ok( r == 0, "settext should return zero (%ld)\n", r);
> -
>
> ok(r == 0, "WM_GETTEXT should return zero (%ld)\n", r);
>
> r = SendMessageA( hwnd, WM_GETTEXT, 0xff000000, 0x1000);
> - ok( r == 0, "settext should return zero (%ld)\n", r);
> -
>
> ok(r == 0, "WM_GETTEXT should return zero (%ld)\n", r);
>
> r = SendMessageA( hwnd, WM_GETTEXT, 0x1000, 0xff000000);
> - ok( r == 0, "settext should return zero (%ld)\n", r);
> - }
> -
>
> ok(r == 0, "WM_GETTEXT should return zero (%ld)\n", r);
> -
>
> }
> -
> -
>
> /* GetWindowText doesn't crash */
> -
>
> r = GetWindowTextA(hwnd, (LPSTR)0x10, 0x1000);
> -
>
> ok(r == 0, "GetWindowText should return zero (%ld)\n", r);
> -
>
> r = GetWindowTextA(hwnd, (LPSTR)0x10000, 0);
> -
>
> ok(r == 0, "GetWindowText should return zero (%ld)\n", r);
> -
>
> r = GetWindowTextA(hwnd, (LPSTR)0xff000000, 0x1000);
> -
>
> ok(r == 0, "GetWindowText should return zero (%ld)\n", r);
> -
>
> r = GetWindowTextA(hwnd, (LPSTR)0x1000, 0xff000000);
> -
>
> ok(r == 0, "GetWindowText should return zero (%ld)\n", r);
> -
> -
>
> r = GetWindowTextW(hwnd, (LPWSTR)0x10, 0x1000);
> -
>
> ok(r == 0, "GetWindowText should return zero (%ld)\n", r);
> -
>
> r = GetWindowTextW(hwnd, (LPWSTR)0x10000, 0);
> -
>
> ok(r == 0, "GetWindowText should return zero (%ld)\n", r);
> -
>
> r = GetWindowTextW(hwnd, (LPWSTR)0xff000000, 0x1000);
> -
>
> ok(r == 0, "GetWindowText should return zero (%ld)\n", r);
> -
>
> r = GetWindowTextW(hwnd, (LPWSTR)0x1000, 0xff000000);
> -
>
> ok(r == 0, "GetWindowText should return zero (%ld)\n", r);
>
> DestroyWindow(hwnd);
> }
>
> diff --git a/dlls/user32/win.c b/dlls/user32/win.c
> index 5e89f4c2c97..e57f291be35 100644
> --- a/dlls/user32/win.c
> +++ b/dlls/user32/win.c
> @@ -35,6 +35,7 @@
>   #include "controls.h"
>   #include "winerror.h"
>   #include "wine/gdi_driver.h"
> +#include "wine/exception.h"
>   #include "wine/debug.h"
>
> WINE_DEFAULT_DEBUG_CHANNEL(win);
> @@ -3001,22 +3002,34 @@ LONG WINAPI DECLSPEC_HOTPATCH SetWindowLongW(
>   INT WINAPI GetWindowTextA( HWND hwnd, LPSTR lpString, INT nMaxCount )
>   {
>       WCHAR *buffer;
> -
>
> int ret = 0;
>
> if (!lpString || nMaxCount <= 0) return 0;
> - if (WIN_IsCurrentProcess( hwnd ))
> - __TRY
>  {
> -
>    lpString[0] = 0;
> -
>    return (INT)SendMessageA( hwnd, WM_GETTEXT, nMaxCount, (LPARAM)lpString );
> -
>    if (WIN_IsCurrentProcess( hwnd ))
> -
>    {
> -
>        lpString[0] = 0;
> -
>        ret = (INT)SendMessageA( hwnd, WM_GETTEXT, nMaxCount, (LPARAM)lpString );
> -
>    }
> -
>    else if ((buffer = HeapAlloc( GetProcessHeap(), 0, nMaxCount * sizeof(WCHAR) )))
> -
>    {
> -
>        /* when window belongs to other process, don't send a message */
> -
>        get_server_window_text( hwnd, buffer, nMaxCount );
> -
>        if (!WideCharToMultiByte( CP_ACP, 0, buffer, -1, lpString, nMaxCount, NULL, NULL ))
> -
>            lpString[nMaxCount-1] = 0;
> -
>        HeapFree( GetProcessHeap(), 0, buffer );
> -
>        ret = strlen(lpString);
> -
>    }
> - }
> - __EXCEPT_ALL
> - {
> -
>    ret = 0;
> }
> - __ENDTRY
> - /* when window belongs to other process, don't send a message */
> - if (!(buffer = HeapAlloc( GetProcessHeap(), 0, nMaxCount * sizeof(WCHAR) ))) return 0;
> - get_server_window_text( hwnd, buffer, nMaxCount );
> - if (!WideCharToMultiByte( CP_ACP, 0, buffer, -1, lpString, nMaxCount, NULL, NULL ))
> -
>    lpString[nMaxCount-1] = 0;
> - HeapFree( GetProcessHeap(), 0, buffer );
> - return strlen(lpString);
> - return ret;
> }
>
> @@ -3049,17 +3062,31 @@ INT WINAPI InternalGetWindowText(HWND hwnd,LPWSTR lpString,INT nMaxCount )
>    */
>   INT WINAPI GetWindowTextW( HWND hwnd, LPWSTR lpString, INT nMaxCount )
>   {
> - int ret;
> -
> if (!lpString || nMaxCount <= 0) return 0;
>
> - if (WIN_IsCurrentProcess( hwnd ))
> - __TRY
>  {
> -
>    lpString[0] = 0;
> -
>    return (INT)SendMessageW( hwnd, WM_GETTEXT, nMaxCount, (LPARAM)lpString );
> -
>    if (WIN_IsCurrentProcess( hwnd ))
> -
>    {
> -
>        lpString[0] = 0;
> -
>        ret = (INT)SendMessageW( hwnd, WM_GETTEXT, nMaxCount, (LPARAM)lpString );
> -
>    }
> -
>    else
> -
>    {
> -
>        /* when window belongs to other process, don't send a message */
> -
>        get_server_window_text( hwnd, lpString, nMaxCount );
> -
>        ret = lstrlenW(lpString);
> -
>    }
> }
> - __EXCEPT_ALL
> - {
> -
>    ret = 0;
> - }
> - __ENDTRY
> - /* when window belongs to other process, don't send a message */
> - get_server_window_text( hwnd, lpString, nMaxCount );
> - return lstrlenW(lpString);
> - return ret;
> }

Then I'm not sure if it's good to catch all exceptions. It will swallow 
any error coming from the heap or send message. Is it really what it 
should do?

[Dmitry Timoshkov]

Rémi Bernon  wrote:
> Then I'm not sure if it's good to catch all exceptions. It will swallow 
> any error coming from the heap or send message. Is it really what it 
> should do?

I have an application that passes rubbish addresses to GetWindowText(),
and apparently it works fine under Windows.

[Zebediah Figura (she/her)]

On 10/13/21 05:18, Dmitry Timoshkov wrote:
> Rémi Bernon  wrote:
> > Then I'm not sure if it's good to catch all exceptions. It will swallow
> > any error coming from the heap or send message. Is it really what it
> > should do?
>
> I have an application that passes rubbish addresses to GetWindowText(),
> and apparently it works fine under Windows.

Should the exception be handled here, though? Is it swallowed for the 
cross-process case? Should it be in DefWindowProc() instead?

[Rémi Bernon]

On 10/13/21 4:42 PM, Zebediah Figura (she/her) wrote:
> On 10/13/21 05:18, Dmitry Timoshkov wrote:
> > Rémi Bernon  wrote:
> > > Then I'm not sure if it's good to catch all exceptions. It will swallow
> > > any error coming from the heap or send message. Is it really what it
> > > should do?
> >
> > I have an application that passes rubbish addresses to GetWindowText(),
> > and apparently it works fine under Windows.
>
> Should the exception be handled here, though? Is it swallowed for the 
> cross-process case? Should it be in DefWindowProc() instead?

We don't send a message for the cross process case, we call wineserver 
instead.

Also, as far as I could see, the window proc isn't even called when an 
invalid string is passed, and the function still returns 0.

Which matches what we are doing, as we write to the buffer once before 
calling SendMessageW.