[PATCH v4 0/3] MR6004: comctl32: A NULL string fix.
-- v4: comctl32/tests: Add tests for NULL strings with edit control WM_GETTEXT message. user32: Correctly handle NULL string in WM_GETTEXT AtoW wrapper. https://gitlab.winehq.org/wine/wine/-/merge_requests/6004
From: Aida Jonikienė <aidas957(a)gmail.com> Zadig passes a fixed string length to WM_GETTEXT in one place regardless of the string: https://github.com/pbatard/libwdi/blob/9b23b82a2dd1cbffc16d46c212f92c6bf8c0c... (which causes a segfault with NULL strings on Wine but not on Windows). This fixes an unrelated issue in https://bugs.winehq.org/show_bug.cgi?id=54750 (but not the main one). --- dlls/comctl32/edit.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dlls/comctl32/edit.c b/dlls/comctl32/edit.c index 4b950412617..b7fd50a75e9 100644 --- a/dlls/comctl32/edit.c +++ b/dlls/comctl32/edit.c @@ -3183,7 +3183,7 @@ static void EDIT_WM_ContextMenu(EDITSTATE *es, INT x, INT y) */ static INT EDIT_WM_GetText(const EDITSTATE *es, INT count, LPWSTR dst) { - if (!count) + if (!count || !dst) return 0; lstrcpynW(dst, es->text, count); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6004
From: Aida Jonikienė <aidas957(a)gmail.com> Spotted in WM_GETTEXT NULL string tests. --- dlls/user32/winproc.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/dlls/user32/winproc.c b/dlls/user32/winproc.c index 5678e75b4e9..4cb3e007de2 100644 --- a/dlls/user32/winproc.c +++ b/dlls/user32/winproc.c @@ -275,8 +275,14 @@ LRESULT WINPROC_CallProcAtoW( winproc_callback_t callback, HWND hwnd, UINT msg, DWORD len = wParam * sizeof(WCHAR); if (!(ptr = get_buffer( buffer, sizeof(buffer), len ))) break; + if (!str) + { + free_buffer( buffer, ptr ); + ptr = NULL; + } + ret = callback( hwnd, msg, wParam, (LPARAM)ptr, result, arg ); - if (wParam) + if (wParam && str) { len = 0; if (*result) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6004
From: Aida Jonikienė <aidas957(a)gmail.com> --- dlls/comctl32/tests/edit.c | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/dlls/comctl32/tests/edit.c b/dlls/comctl32/tests/edit.c index bce0215ef19..92017c046b2 100644 --- a/dlls/comctl32/tests/edit.c +++ b/dlls/comctl32/tests/edit.c @@ -1470,6 +1470,19 @@ static void test_edit_control_6(void) ok(ret == strlen(str), "Expected %s, got len %ld\n", str, ret); ok(!strcmp(buf, str), "Expected %s, got %s\n", str, buf); + ret = SendMessageA(hWnd, WM_GETTEXT, MAXLEN, (LPARAM)NULL); + ok(ret == 0, "Expected 0, got len %ld\n", ret); + ret = SendMessageA(hWnd, WM_GETTEXT, 0, (LPARAM)NULL); + ok(ret == 0, "Expected 0, got len %ld\n", ret); + + if (0) /* Crashes on Windows. */ + { + ret = SendMessageW(hWnd, WM_GETTEXT, MAXLEN, (LPARAM)NULL); + ok(ret == 0, "Expected 0, got len %ld\n", ret); + ret = SendMessageW(hWnd, WM_GETTEXT, 0, (LPARAM)NULL); + ok(ret == 0, "Expected 0, got len %ld\n", ret); + } + buf[0] = 0; ret = SendMessageA(hWnd, WM_DESTROY, 0, 0); todo_wine -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6004
The only failure is in the windows.media.speech test now (which seems unrelated); I'll re-run the test to see if it still happens :frog: -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6004#note_75376
Zhiyi Zhang (@zhiyi) commented about dlls/user32/winproc.c:
DWORD len = wParam * sizeof(WCHAR);
if (!(ptr = get_buffer( buffer, sizeof(buffer), len ))) break; + if (!str)
If you check str for NULL before get_buffer() then you don't need to call free_buffer(). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6004#note_75706
Zhiyi Zhang (@zhiyi) commented about dlls/user32/winproc.c:
break;
case WM_GETTEXT: case WM_ASKCBFORMATNAME:
Please add tests for WM_ASKCBFORMATNAME as well. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6004#note_75707
Zhiyi Zhang (@zhiyi) commented about dlls/comctl32/tests/edit.c:
ok(ret == strlen(str), "Expected %s, got len %ld\n", str, ret); ok(!strcmp(buf, str), "Expected %s, got %s\n", str, buf);
+ ret = SendMessageA(hWnd, WM_GETTEXT, MAXLEN, (LPARAM)NULL); + ok(ret == 0, "Expected 0, got len %ld\n", ret); + ret = SendMessageA(hWnd, WM_GETTEXT, 0, (LPARAM)NULL); + ok(ret == 0, "Expected 0, got len %ld\n", ret);
Please add tests for user32/edit as well. Also, test WM_ASKCBFORMATNAME. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6004#note_75708
On Wed Jul 10 07:48:16 2024 +0000, Zhiyi Zhang wrote:
If you check str for NULL before get_buffer() then you don't need to call free_buffer(). If I move the `if` statement before the `ptr` assignment then NULL lParam won't be passed through (which doesn't match direct winproc call behavior)
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6004#note_75761
On Wed Jul 10 07:49:27 2024 +0000, Zhiyi Zhang wrote:
Please add tests for user32/edit as well. Also, test WM_ASKCBFORMATNAME. user32/edit and comctl32/edit basically look the same to me (so could those test parts be commonized?)
WM_ASKCBFORMATNAME basically has no meaningful tests though (so that would be a nice thing to work on) but probably nothing relies on that message either (so :shrug:) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6004#note_75776
user32/edit and comctl32/edit basically look the same to me (so could those test parts be commonized?)
They look the same but they are for different components, one for user32 edit and the another for the edit control of comctl32 version 6.
WM_ASKCBFORMATNAME basically has no meaningful tests though (so that would be a nice thing to work on) but probably nothing relies on that message either (so 🤷)
It's better to have a test. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6004#note_75785
On Wed Jul 10 11:44:08 2024 +0000, Aida Jonikienė wrote:
If I move the `if` statement before the `ptr` assignment then NULL lParam won't be passed through (which doesn't match direct winproc call behavior) Can't it be something like the following?
``` if (!str) ptr = NULL; else if (!(ptr = get_buffer( buffer, sizeof(buffer), len ))) break; ``` -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6004#note_75786
participants (2)
-
Aida Jonikienė -
Zhiyi Zhang (＠zhiyi)