[PATCH 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(). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4186
From: Zhiyi Zhang <zzhang(a)codeweavers.com> --- dlls/user32/tests/Makefile.in | 6 ++- dlls/user32/tests/msg.c | 82 ++++++++++++++++++++++++++++++++++ dlls/user32/tests/testdll.c | 32 +++++++++++++ dlls/user32/tests/testdll.spec | 1 + 4 files changed, 120 insertions(+), 1 deletion(-) 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..ca1c8667567 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 \ @@ -21,6 +23,8 @@ C_SRCS = \ scroll.c \ static.c \ sysparams.c \ + testdll.c \ + testdll.spec \ text.c \ uitools.c \ win.c \ 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
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=139119 Your paranoid android. === w11pro64_amd (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. === w10pro64_zh_CN (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.
participants (3)
-
Marvin -
Zhiyi Zhang -
Zhiyi Zhang (＠zhiyi)