[PATCH v2 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(). -- v2: win32u: Return ERROR_ACCESS_DENIED for WH_JOURNALRECORD and WH_JOURNALPLAYBACK on newer OS versions. 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..3a326dee628 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, it succeeds on XP but fails on higher versions, even with administrator rights */ + SetLastError(0xdeadbeef); + hhook = SetWindowsHookExA(i, p_dummy_hook_proc, test_dll_module, 0); + error = GetLastError(); + if (LOBYTE(LOWORD(GetVersion())) >= 6 && (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 || broken(error == 0xdeadbeef) /* XP */, "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 || broken(error == 0xdeadbeef) /* XP */, "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 3a326dee628..2db1e62930e 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. --- dlls/user32/tests/msg.c | 2 -- dlls/win32u/hook.c | 9 +++++++++ 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/dlls/user32/tests/msg.c b/dlls/user32/tests/msg.c index 2db1e62930e..d924f889f72 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 (LOBYTE(LOWORD(GetVersion())) >= 6 && (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..b16f76cba2a 100644 --- a/dlls/win32u/hook.c +++ b/dlls/win32u/hook.c @@ -105,6 +105,15 @@ 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 */ + if (NtCurrentTeb()->Peb->OSMajorVersion >= 6 && (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
I think this could be disabled unconditionally, journalling hooks have been broken for ages now. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4186#note_49899
participants (3)
-
Alexandre Julliard (＠julliard) -
Zhiyi Zhang
-
Zhiyi Zhang (＠zhiyi)