[PATCH 0/3] MR4186: win32u: Return ERROR_ACCESS_DENIED for WH_JOURNALRECORD and WH_JOURNALPLAYBACK...

[Zhiyi Zhang (＠zhiyi)]

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().

[PATCH 1/3] user32/tests: Add more parameter checks for SetWindowsHookExA().

[Zhiyi Zhang]

From: Zhiyi Zhang

---
 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)

[PATCH 2/3] win32u: Return ERROR_GLOBAL_ONLY_HOOK when a thread ID is specified for global hooks.

[Zhiyi Zhang]

From: Zhiyi Zhang

---
 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;
         }
     }

[PATCH 3/3] win32u: Return ERROR_ACCESS_DENIED for WH_JOURNALRECORD and WH_JOURNALPLAYBACK on newer OS versions.

[Zhiyi Zhang]

From: Zhiyi Zhang

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;

[PATCH 3/3] win32u: Return ERROR_ACCESS_DENIED for WH_JOURNALRECORD and WH_JOURNALPLAYBACK on newer OS versions.

[Marvin]

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.