[PATCH v3 0/3] MR4186: win32u: Return ERROR_ACCESS_DENIED for WH_JOURNALRECORD and WH_JOURNALPLAYBACK...
win32u: Return ERROR_ACCESS_DENIED for WH_JOURNALRECORD and WH_JOURNALPLAYBACK on newer OS versions. On > XP, settings WH_JOURNALRECORD or WH_JOURNALPLAYBACK hooks report ERROR_ACCESS_DENIED according to tests, even with administrator rights. PCSE_TERM depends on this to not crash. MSDN also says that journaling hooks APIs are unsupported starting in Windows 11. So I hope this justifies the version check in NtUserSetWindowsHookEx(). -- v3: win32u: Return ERROR_ACCESS_DENIED for WH_JOURNALRECORD and WH_JOURNALPLAYBACK. win32u: Return ERROR_GLOBAL_ONLY_HOOK when a thread ID is specified for global hooks. user32/tests: Add more parameter checks for SetWindowsHookExA(). https://gitlab.winehq.org/wine/wine/-/merge_requests/4186
From: Zhiyi Zhang <zzhang(a)codeweavers.com> --- dlls/user32/tests/Makefile.in | 9 ++-- dlls/user32/tests/msg.c | 82 ++++++++++++++++++++++++++++++++++ dlls/user32/tests/testdll.c | 32 +++++++++++++ dlls/user32/tests/testdll.spec | 1 + 4 files changed, 121 insertions(+), 3 deletions(-) create mode 100644 dlls/user32/tests/testdll.c create mode 100644 dlls/user32/tests/testdll.spec diff --git a/dlls/user32/tests/Makefile.in b/dlls/user32/tests/Makefile.in index 33c7f13f12d..bff53de7713 100644 --- a/dlls/user32/tests/Makefile.in +++ b/dlls/user32/tests/Makefile.in @@ -1,7 +1,9 @@ TESTDLL = user32.dll IMPORTS = user32 gdi32 advapi32 hid imm32 setupapi -C_SRCS = \ +testdll_IMPORTS = user32 + +SOURCES = \ broadcast.c \ class.c \ clipboard.c \ @@ -18,13 +20,14 @@ C_SRCS = \ monitor.c \ msg.c \ resource.c \ + resource.rc \ scroll.c \ static.c \ sysparams.c \ + testdll.c \ + testdll.spec \ text.c \ uitools.c \ win.c \ winstation.c \ wsprintf.c - -RC_SRCS = resource.rc diff --git a/dlls/user32/tests/msg.c b/dlls/user32/tests/msg.c index 07df7d5cb86..1ebea15dce0 100644 --- a/dlls/user32/tests/msg.c +++ b/dlls/user32/tests/msg.c @@ -12223,11 +12223,41 @@ skip_mouse_ll_hook_test: ok(DestroyWindow(hwnd), "failed to destroy window\n"); } +static char *get_test_dll_path(void) +{ + static const char *dll_name = "testdll.dll"; + static char path[MAX_PATH]; + DWORD written; + HANDLE file; + HRSRC res; + void *ptr; + + GetTempPathA(ARRAY_SIZE(path), path); + strcat(path, dll_name); + + file = CreateFileA(path, GENERIC_READ|GENERIC_WRITE, 0, NULL, CREATE_ALWAYS, 0, 0); + ok(file != INVALID_HANDLE_VALUE, "Failed to create file %s: %lu.\n", debugstr_a(path), GetLastError()); + + res = FindResourceA(NULL, dll_name, "TESTDLL"); + ok(!!res, "Failed to load resource: %lu\n", GetLastError()); + ptr = LockResource(LoadResource(GetModuleHandleA(NULL), res)); + WriteFile(file, ptr, SizeofResource(GetModuleHandleA(NULL), res), &written, NULL); + ok(written == SizeofResource(GetModuleHandleA(NULL), res), "Failed to write resource\n"); + CloseHandle(file); + + return path; +} + static void test_set_hook(void) { + LRESULT (CALLBACK *p_dummy_hook_proc)(int code, WPARAM wp, LPARAM lp); + HMODULE test_dll_module; + char *test_dll_path; + DWORD error; BOOL ret; HHOOK hhook; HWINEVENTHOOK hwinevent_hook; + int i; hhook = SetWindowsHookExA(WH_CBT, cbt_hook_proc, GetModuleHandleA(0), GetCurrentThreadId()); ok(hhook != 0, "local hook does not require hModule set to 0\n"); @@ -12255,6 +12285,58 @@ static void test_set_hook(void) GetLastError() == 0xdeadbeef, /* Win9x */ "unexpected error %ld\n", GetLastError()); + test_dll_path = get_test_dll_path(); + test_dll_module = LoadLibraryA(test_dll_path); + p_dummy_hook_proc = (void *)GetProcAddress(test_dll_module, "dummy_hook_proc"); + for (i = WH_MIN; i <= WH_MAX; i++) + { + winetest_push_context("ID %d", i); + + /* Test that setting hooks should succeed for hook procs in a library. But for WH_JOURNALRECORD + * and WH_JOURNALPLAYBACK, ERROR_ACCESS_DENIED is returned, even with administrator rights */ + SetLastError(0xdeadbeef); + hhook = SetWindowsHookExA(i, p_dummy_hook_proc, test_dll_module, 0); + error = GetLastError(); + if (i == WH_JOURNALRECORD || i == WH_JOURNALPLAYBACK) + { + todo_wine + ok(!hhook, "SetWinEventHook succeeded.\n"); + todo_wine + ok(error == ERROR_ACCESS_DENIED, "Got unexpected error %ld.\n", GetLastError()); + } + else + { + ok(!!hhook, "SetWinEventHook failed.\n"); + ok(error == NO_ERROR, "Got unexpected error %ld.\n", GetLastError()); + } + if (hhook) + UnhookWindowsHookEx(hhook); + + /* Test settings global hooks with a thread ID */ + SetLastError(0xdeadbeef); + hhook = SetWindowsHookExA(i, p_dummy_hook_proc, test_dll_module, GetCurrentThreadId()); + error = GetLastError(); + if (i == WH_JOURNALRECORD || i == WH_JOURNALPLAYBACK || i == WH_SYSMSGFILTER + || i == WH_KEYBOARD_LL || i == WH_MOUSE_LL) + { + ok(!hhook, "SetWinEventHook succeeded.\n"); + todo_wine + ok(error == ERROR_GLOBAL_ONLY_HOOK, "Got unexpected error %ld.\n", GetLastError()); + } + else + { + ok(!!hhook, "SetWinEventHook failed.\n"); + ok(error == NO_ERROR, "Got unexpected error %ld.\n", GetLastError()); + } + if (hhook) + UnhookWindowsHookEx(hhook); + + winetest_pop_context(); + } + FreeLibrary(test_dll_module); + ret = DeleteFileA(test_dll_path); + ok(ret, "Failed to remove the test dll, error %ld.\n", GetLastError()); + if (!pSetWinEventHook || !pUnhookWinEvent) return; /* even process local incontext hooks require hmodule */ diff --git a/dlls/user32/tests/testdll.c b/dlls/user32/tests/testdll.c new file mode 100644 index 00000000000..52ca93fc340 --- /dev/null +++ b/dlls/user32/tests/testdll.c @@ -0,0 +1,32 @@ +/* + * Copyright 2023 Zhiyi Zhang for CodeWeavers + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA + */ + +#if 0 +#pragma makedep testdll +#endif + +#include <stdarg.h> + +#include <windef.h> +#include <winbase.h> +#include <winuser.h> + +LRESULT CALLBACK dummy_hook_proc(int code, WPARAM wp, LPARAM lp) +{ + return CallNextHookEx(0, code, wp, lp); +} diff --git a/dlls/user32/tests/testdll.spec b/dlls/user32/tests/testdll.spec new file mode 100644 index 00000000000..80834cbc21a --- /dev/null +++ b/dlls/user32/tests/testdll.spec @@ -0,0 +1 @@ +@ stdcall dummy_hook_proc(long ptr ptr) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4186
From: Zhiyi Zhang <zzhang(a)codeweavers.com> --- dlls/user32/tests/msg.c | 1 - dlls/win32u/hook.c | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/dlls/user32/tests/msg.c b/dlls/user32/tests/msg.c index 1ebea15dce0..b1be1f9ee87 100644 --- a/dlls/user32/tests/msg.c +++ b/dlls/user32/tests/msg.c @@ -12320,7 +12320,6 @@ static void test_set_hook(void) || i == WH_KEYBOARD_LL || i == WH_MOUSE_LL) { ok(!hhook, "SetWinEventHook succeeded.\n"); - todo_wine ok(error == ERROR_GLOBAL_ONLY_HOOK, "Got unexpected error %ld.\n", GetLastError()); } else diff --git a/dlls/win32u/hook.c b/dlls/win32u/hook.c index 062f10c4702..3cd6b1ee1a2 100644 --- a/dlls/win32u/hook.c +++ b/dlls/win32u/hook.c @@ -91,7 +91,7 @@ HHOOK WINAPI NtUserSetWindowsHookEx( HINSTANCE inst, UNICODE_STRING *module, DWO id == WH_SYSMSGFILTER) { /* these can only be global */ - RtlSetLastWin32Error( ERROR_INVALID_PARAMETER ); + RtlSetLastWin32Error( ERROR_GLOBAL_ONLY_HOOK ); return 0; } } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4186
From: Zhiyi Zhang <zzhang(a)codeweavers.com> On > XP, settings WH_JOURNALRECORD or WH_JOURNALPLAYBACK hooks report ERROR_ACCESS_DENIED according to tests, even with administrator rights. PCSE_TERM depends on this to not crash. MSDN also says that journaling hooks APIs are unsupported starting in Windows 11. Reject journalling hooks unconditionally here because they have been broken for a long time. --- dlls/user32/tests/msg.c | 2 -- dlls/win32u/hook.c | 10 ++++++++++ 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/dlls/user32/tests/msg.c b/dlls/user32/tests/msg.c index b1be1f9ee87..6eadf324e6f 100644 --- a/dlls/user32/tests/msg.c +++ b/dlls/user32/tests/msg.c @@ -12299,9 +12299,7 @@ static void test_set_hook(void) error = GetLastError(); if (i == WH_JOURNALRECORD || i == WH_JOURNALPLAYBACK) { - todo_wine ok(!hhook, "SetWinEventHook succeeded.\n"); - todo_wine ok(error == ERROR_ACCESS_DENIED, "Got unexpected error %ld.\n", GetLastError()); } else diff --git a/dlls/win32u/hook.c b/dlls/win32u/hook.c index 3cd6b1ee1a2..b9990298e0e 100644 --- a/dlls/win32u/hook.c +++ b/dlls/win32u/hook.c @@ -105,6 +105,16 @@ HHOOK WINAPI NtUserSetWindowsHookEx( HINSTANCE inst, UNICODE_STRING *module, DWO } } + /* On > XP, settings WH_JOURNALRECORD or WH_JOURNALPLAYBACK hooks report ERROR_ACCESS_DENIED + * according to tests, even with administrator rights. PCSE_TERM depends on this to not crash. + * MSDN also says that journaling hooks APIs are unsupported starting in Windows 11. Reject + * journalling hooks unconditionally here because they have been broken for a long time */ + if (id == WH_JOURNALRECORD || id == WH_JOURNALPLAYBACK) + { + RtlSetLastWin32Error( ERROR_ACCESS_DENIED ); + return 0; + } + SERVER_START_REQ( set_hook ) { req->id = id; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4186
Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details: The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=139349 Your paranoid android. === w11pro64 (testbot log) === Use of uninitialized value $Started in numeric lt (<) at /home/testbot/lib/WineTestBot/LogUtils.pm line 1626. Use of uninitialized value $Started in numeric lt (<) at /home/testbot/lib/WineTestBot/LogUtils.pm line 1626. Use of uninitialized value $Started in numeric lt (<) at /home/testbot/lib/WineTestBot/LogUtils.pm line 1626. Use of uninitialized value $Started in numeric lt (<) at /home/testbot/lib/WineTestBot/LogUtils.pm line 1626. === w10pro64 (32 bit report) === user32: win.c:3817: Test failed: GetForegroundWindow returned 000300FA win.c:3749: Test failed: SetForegroundWindow failed, error 0 win.c:3752: Test failed: GetForegroundWindow returned 000300FA win.c:3789: Test failed: GetForegroundWindow returned 000300FA win.c:3874: Test failed: GetActiveWindow() = 00020292 win.c:3878: Test failed: GetFocus() = 00000000 win.c:3881: Test failed: GetFocus() = 00000000 === w10pro64 (64 bit report) === user32: win.c:3817: Test failed: GetForegroundWindow returned 000000000003017E win.c:3749: Test failed: SetForegroundWindow failed, error 0 win.c:3752: Test failed: GetForegroundWindow returned 000000000003017E win.c:3789: Test failed: GetForegroundWindow returned 000000000003017E win.c:3874: Test failed: GetActiveWindow() = 00000000000102B4 win.c:3878: Test failed: GetFocus() = 0000000000000000 win.c:3881: Test failed: GetFocus() = 0000000000000000
v3: Reject journalling hooks unconditionally and remove XP tests. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4186#note_50142
participants (3)
-
Marvin -
Zhiyi Zhang -
Zhiyi Zhang (＠zhiyi)