diff --git a/CMakeLists.txt b/CMakeLists.txt index 8624c31..cbb35a5 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -15,7 +15,7 @@ find_program(MINGW32_C_COMPILER i686-w64-mingw32-gcc REQUIRED) # GLib dependencies. Keep it alongside the Winelib helper. set(menu_hook "${CMAKE_CURRENT_BINARY_DIR}/wine-access-menu.dll") add_custom_command(OUTPUT "${menu_hook}" - COMMAND "${MINGW_C_COMPILER}" -std=c11 -O2 -Wall -Wextra -shared + COMMAND "${MINGW_C_COMPILER}" -std=c11 -O2 -g -fno-omit-frame-pointer -Wall -Wextra -shared -static-libgcc -o "${menu_hook}" "${CMAKE_CURRENT_SOURCE_DIR}/menu_hook.c" "${CMAKE_CURRENT_SOURCE_DIR}/edit_hook.c" "${CMAKE_CURRENT_SOURCE_DIR}/scintilla_hook.c" -luser32 -lcomctl32 -lgdi32 @@ -26,14 +26,14 @@ add_custom_target(wine-access-menu ALL DEPENDS "${menu_hook}") set(menu_hook32 "${CMAKE_CURRENT_BINARY_DIR}/wine-access-menu32.dll") set(menu_host32 "${CMAKE_CURRENT_BINARY_DIR}/wine-access-menu-host32.exe") add_custom_command(OUTPUT "${menu_hook32}" "${menu_host32}" - COMMAND "${MINGW32_C_COMPILER}" -std=c11 -O2 -Wall -Wextra -shared + COMMAND "${MINGW32_C_COMPILER}" -std=c11 -O2 -g -fno-omit-frame-pointer -Wall -Wextra -shared -static-libgcc -Wl,--kill-at -o "${menu_hook32}" "${CMAKE_CURRENT_SOURCE_DIR}/menu_hook.c" "${CMAKE_CURRENT_SOURCE_DIR}/edit_hook.c" -luser32 -lcomctl32 COMMAND "${MINGW32_C_COMPILER}" -std=c11 -O2 -Wall -Wextra -mwindows -static-libgcc -o "${menu_host32}" "${CMAKE_CURRENT_SOURCE_DIR}/menu_host32.c" -luser32 - DEPENDS menu_hook.c menu_bridge.h menu_host32.c edit_hook.c edit_hook.h control_class.h + DEPENDS menu_hook.c menu_bridge.h menu_host32.c foreground_hooks.h edit_hook.c edit_hook.h control_class.h VERBATIM) add_custom_target(wine-access-menu32 ALL DEPENDS "${menu_hook32}" "${menu_host32}") @@ -95,6 +95,12 @@ if(BUILD_TESTING) else() set(fixture_compiler "${MINGW_C_COMPILER}") endif() + set(hook_fixture "${CMAKE_CURRENT_BINARY_DIR}/hook-safety-fixture${arch}.exe") + add_custom_command(OUTPUT "${hook_fixture}" + COMMAND "${fixture_compiler}" -std=c11 -O2 -Wall -Wextra + -o "${hook_fixture}" "${CMAKE_CURRENT_SOURCE_DIR}/tests/hook_safety_fixture.c" -luser32 -lcomctl32 + DEPENDS tests/hook_safety_fixture.c edit_hook.h VERBATIM) + add_custom_target(hook-safety-fixture${arch} ALL DEPENDS "${hook_fixture}") set(review_fixture "${CMAKE_CURRENT_BINARY_DIR}/review-fixture${arch}.exe") add_custom_command(OUTPUT "${review_fixture}" COMMAND "${fixture_compiler}" -std=c11 -Wall -Wextra -mwindows @@ -107,6 +113,9 @@ if(BUILD_TESTING) set_tests_properties(review-capture${arch} PROPERTIES TIMEOUT 35 ENVIRONMENT "WINEPREFIX=${CMAKE_CURRENT_BINARY_DIR}/review-prefix${arch};CTHULHU_WINE_ACCESS_MANAGED=1;WINEDEBUG=-all;WINEDLLOVERRIDES=mscoree,mshtml=") endforeach() + add_test(NAME hook-safety COMMAND python3 + "${CMAKE_CURRENT_SOURCE_DIR}/tests/hook_safety_test.py" "${CMAKE_CURRENT_BINARY_DIR}") + set_tests_properties(hook-safety PROPERTIES TIMEOUT 100) endif() # Winegcc emits a launcher and a Winelib sidecar. CMake's normal target diff --git a/README.md b/README.md index 13ba6d9..bbb26d8 100644 --- a/README.md +++ b/README.md @@ -177,10 +177,11 @@ shortcuts, submenu roles, and checked/unavailable states to the helper. The helper queues these messages before forwarding them over D-Bus, suppresses repeated selections, and restores standard-control focus reporting when menus close. The hooks apply to the helper's Wine desktop and prefix and are removed -on normal shutdown. The 32-bit host attaches to the foreground 32-bit UI thread -so Wine's global hook chain cannot interfere with 64-bit menus. It exits when the helper dies or its bridge -window closes. Menu and editor hooks are disabled by `--no-dialog-reader`. The -32-bit companion attaches both hooks to the same foreground UI thread. +on normal shutdown. Each host attaches both hooks only to the foreground UI +thread of the matching architecture, releasing them when focus moves away. +This avoids synchronous cross-architecture traffic through Wine's global hook +chain. The 32-bit host exits when the helper dies or its bridge window closes. +Menu and editor hooks are disabled by `--no-dialog-reader`. Focused combo boxes are checked every 100 ms to recover value changes that Wine does not report through WinEvents. The combo's internal list events use diff --git a/edit_hook.c b/edit_hook.c index fad3858..45f4448 100644 --- a/edit_hook.c +++ b/edit_hook.c @@ -79,17 +79,10 @@ static void notify_text(HWND window, UINT kind, const WCHAR *text, DWORD start, wine_access_notify_text(window, kind, text, start, end, 0); } -static LRESULT CALLBACK edit_proc(HWND window, UINT message, WPARAM wp, LPARAM lp, - UINT_PTR id, DWORD_PTR reference) { - EditState *state = (EditState *)reference; - if (message == WM_NCDESTROY) { - RemoveWindowSubclass(window, edit_proc, id); - RemovePropW(window, EDIT_STATE_PROPERTY); - state->destroyed = TRUE; - if (!state->busy) { free(state->text); free(state); } - return DefSubclassProc(window, message, wp, lp); - } - if (state->busy) return DefSubclassProc(window, message, wp, lp); +// Keep the text buffers off the stack when forwarding ignored or reentrant +// messages. Inlining this handler would put them back in every subclass call. +static __attribute__((noinline)) LRESULT handle_edit(HWND window, UINT message, + WPARAM wp, LPARAM lp, EditState *state) { BOOL navigation = message == WM_KEYDOWN && (wp == VK_LEFT || wp == VK_RIGHT || wp == VK_UP || wp == VK_DOWN || wp == VK_HOME || wp == VK_END || wp == VK_PRIOR || wp == VK_NEXT); @@ -106,8 +99,6 @@ static LRESULT CALLBACK edit_proc(HWND window, UINT message, WPARAM wp, LPARAM l // such as MUSHclient consume history arrows before DispatchMessage instead. if (navigation || focus) state->history_key = state->history_replaced = FALSE; BOOL history_replacement = message == WM_SETTEXT && state->history_key; - if (!navigation && !changing && !focus && !selected && message != EM_SETPASSWORDCHAR) - return DefSubclassProc(window, message, wp, lp); state->busy = TRUE; WCHAR pending_high = state->pending_high; @@ -242,6 +233,37 @@ static LRESULT CALLBACK edit_proc(HWND window, UINT message, WPARAM wp, LPARAM l return result; } +static LRESULT CALLBACK edit_proc(HWND window, UINT message, WPARAM wp, LPARAM lp, + UINT_PTR id, DWORD_PTR reference) { + EditState *state = (EditState *)reference; + if (message == WM_NCDESTROY) { + RemoveWindowSubclass(window, edit_proc, id); + RemovePropW(window, EDIT_STATE_PROPERTY); + state->destroyed = TRUE; + if (!state->busy) { free(state->text); free(state); } + return DefSubclassProc(window, message, wp, lp); + } + if (state->busy) return DefSubclassProc(window, message, wp, lp); + BOOL relevant = FALSE; + switch (message) { + case WM_KEYDOWN: + relevant = wp == VK_LEFT || wp == VK_RIGHT || wp == VK_UP || wp == VK_DOWN || + wp == VK_HOME || wp == VK_END || wp == VK_PRIOR || wp == VK_NEXT || wp == VK_DELETE; + break; + case WM_CHAR: case WM_PASTE: case WM_CUT: case WM_CLEAR: + case WM_UNDO: case EM_UNDO: case EM_REPLACESEL: case WM_SETTEXT: + case EM_SETHANDLE: case EM_FMTLINES: case WM_SETFOCUS: case EM_SETSEL: + case EM_SETPASSWORDCHAR: + relevant = TRUE; + break; + default: + relevant = message == RegisterWindowMessageW(EDIT_FOCUS_MESSAGE); + break; + } + if (!relevant) return DefSubclassProc(window, message, wp, lp); + return handle_edit(window, message, wp, lp, state); +} + BOOL wine_access_watch_edit(HWND window) { WCHAR class_name[64]; if (!window || !GetClassNameW(window, class_name, 64)) return FALSE; diff --git a/foreground_hooks.h b/foreground_hooks.h new file mode 100644 index 0000000..a8fdd69 --- /dev/null +++ b/foreground_hooks.h @@ -0,0 +1,37 @@ +// SPDX-License-Identifier: LGPL-2.1-or-later +#pragma once +#include + +typedef struct WineAccessForegroundHooks { + HMODULE module; + HOOKPROC menu_callback, input_callback; + HHOOK menu, input; + DWORD thread; +} WineAccessForegroundHooks; + +static inline void wine_access_release_hooks(WineAccessForegroundHooks *hooks) { + if (hooks->menu) UnhookWindowsHookEx(hooks->menu); + if (hooks->input) UnhookWindowsHookEx(hooks->input); + hooks->menu = hooks->input = NULL; + hooks->thread = 0; +} + +// Global hooks cross the 32/64-bit boundary through the installing thread. +// Avoid that synchronous traffic and hook only the matching foreground UI +// thread. Both hosts use the same detach/retry rules. +static inline void wine_access_update_hooks(WineAccessForegroundHooks *hooks, BOOL target_wow64) { + DWORD pid = 0; + DWORD thread = GetWindowThreadProcessId(GetForegroundWindow(), &pid); + HANDLE process = pid ? OpenProcess(PROCESS_QUERY_LIMITED_INFORMATION, FALSE, pid) : NULL; + BOOL wow64 = FALSE; + BOOL known = process && IsWow64Process(process, &wow64); + if (process) CloseHandle(process); + if (!known || wow64 != target_wow64) thread = 0; + if (thread == hooks->thread) return; + wine_access_release_hooks(hooks); + if (!thread || !hooks->menu_callback || !hooks->input_callback) return; + hooks->menu = SetWindowsHookExW(WH_CALLWNDPROC, hooks->menu_callback, hooks->module, thread); + hooks->input = SetWindowsHookExW(WH_GETMESSAGE, hooks->input_callback, hooks->module, thread); + if (hooks->menu && hooks->input) hooks->thread = thread; + else wine_access_release_hooks(hooks); // Retry both on the next foreground poll. +} diff --git a/main.cpp b/main.cpp index 3dea3b2..94daeef 100644 --- a/main.cpp +++ b/main.cpp @@ -9,6 +9,7 @@ #include "native_items.h" #include "review_service.h" #include "foreground_focus.h" +#include "foreground_hooks.h" #include "reader_endpoint.h" #include "msaa_events.h" #include "reader_events.h" @@ -1065,8 +1066,7 @@ int WINAPI WinMain(HINSTANCE instance, HINSTANCE, char *, int) { } HWINEVENTHOOK hook = nullptr; - HHOOK menuHook = nullptr; - HHOOK inputHook = nullptr; + WineAccessForegroundHooks foregroundHooks{}; HMODULE menuModule = nullptr; HWND menuBridge = nullptr; HANDLE menuHost32 = nullptr; @@ -1082,15 +1082,13 @@ int WINAPI WinMain(HINSTANCE instance, HINSTANCE, char *, int) { WINE_ACCESS_MENU_CLASS, L"", 0, 0, 0, 0, 0, nullptr, nullptr, instance, nullptr); if (menuBridge != nullptr && (menuModule = load_menu_hook()) != nullptr) { - const auto callback = std::bit_cast(GetProcAddress(menuModule, "WineAccessMenuHook")); - if (callback != nullptr) - menuHook = SetWindowsHookExW(WH_CALLWNDPROC, callback, menuModule, 0); - const auto inputCallback = std::bit_cast(GetProcAddress(menuModule, "WineAccessInputHook")); - if (inputCallback != nullptr) - inputHook = SetWindowsHookExW(WH_GETMESSAGE, inputCallback, menuModule, 0); - editReaderActive = menuHook != nullptr && inputHook != nullptr; + foregroundHooks.module = menuModule; + foregroundHooks.menu_callback = std::bit_cast(GetProcAddress(menuModule, "WineAccessMenuHook")); + foregroundHooks.input_callback = std::bit_cast(GetProcAddress(menuModule, "WineAccessInputHook")); + editReaderActive = foregroundHooks.menu_callback && foregroundHooks.input_callback; + wine_access_update_hooks(&foregroundHooks, FALSE); } - if (menuHook == nullptr) + if (!editReaderActive) std::fprintf(stderr, "wine-access: native menu hook unavailable (error %lu)\n", static_cast(GetLastError())); if (menuBridge != nullptr) { @@ -1098,6 +1096,9 @@ int WINAPI WinMain(HINSTANCE instance, HINSTANCE, char *, int) { if (menuHost32 == nullptr) std::fprintf(stderr, "wine-access: 32-bit menu host unavailable (error %lu)\n", static_cast(GetLastError())); + // Existing subclasses keep working after foreground hooks move away, + // including those attached by the 32-bit host. + editReaderActive = editReaderActive || menuHost32 != nullptr; } } MSG message{}; @@ -1116,6 +1117,7 @@ int WINAPI WinMain(HINSTANCE instance, HINSTANCE, char *, int) { if (current_msaa_source(source)) present_standard_focus(source.window); } restore_virtual_desktop_focus(); + wine_access_update_hooks(&foregroundHooks, FALSE); poll_control_focus(); reviewService.focus_changed(reviewIdentities.observe_foreground()); continue; @@ -1129,15 +1131,12 @@ int WINAPI WinMain(HINSTANCE instance, HINSTANCE, char *, int) { reviewService.stop(); if (hook != nullptr) UnhookWinEvent(hook); - if (menuHook != nullptr) - UnhookWindowsHookEx(menuHook); - if (inputHook != nullptr) - UnhookWindowsHookEx(inputHook); + wine_access_release_hooks(&foregroundHooks); if (menuBridge != nullptr) DestroyWindow(menuBridge); if (menuHost32 != nullptr) { // Destroying the bridge ends the host's loop. Parent-process death also - // stops it, so a reader crash cannot leave an orphaned global menu hook. + // stops it, so a reader crash cannot leave an orphaned foreground hook. if (WaitForSingleObject(menuHost32, 1000) == WAIT_TIMEOUT) { TerminateProcess(menuHost32, 0); WaitForSingleObject(menuHost32, 1000); diff --git a/menu_hook.c b/menu_hook.c index 7967e6c..e37fdab 100644 --- a/menu_hook.c +++ b/menu_hook.c @@ -14,6 +14,53 @@ static void menu_text(WCHAR *output, UINT capacity, const WCHAR *input) { output[used] = 0; } +// Keep menu payloads off the forwarding callback's stack, including in +// optimized builds. Most application messages do not need menu inspection. +static __attribute__((noinline)) void handle_menu(const CWPSTRUCT *message) { + HWND bridge = FindWindowW(WINE_ACCESS_MENU_CLASS, NULL); + HWND owner = GetAncestor(message->hwnd, GA_ROOT); + BOOL closed = message->message == WM_EXITMENULOOP || + (!message->lParam && HIWORD(message->wParam) == 0xffff); + if (bridge && (closed || owner == GetAncestor(GetForegroundWindow(), GA_ROOT))) { + WineAccessMenuEvent event = {0}; + UINT flags = HIWORD(message->wParam); + HMENU menu = (HMENU)message->lParam; + event.owner = (ULONGLONG)(UINT_PTR)owner; + event.menu = (ULONGLONG)(UINT_PTR)menu; + event.item = LOWORD(message->wParam); + if (closed) { + event.flags = WINE_ACCESS_MENU_CLOSED; + } else { + WCHAR text[640] = {0}; + MENUITEMINFOW info = {0}; + info.cbSize = sizeof(info); + info.fMask = MIIM_STRING | MIIM_STATE | MIIM_FTYPE | MIIM_SUBMENU; + info.dwTypeData = text; + info.cch = sizeof(text) / sizeof(text[0]); + if (!GetMenuItemInfoW(menu, event.item, (flags & MF_POPUP) != 0, &info) || + (info.fType & MFT_SEPARATOR)) + return; + menu_text(event.name, 512, text); + for (UINT i = 0; text[i]; ++i) { + if (text[i] == '\t') { + lstrcpynW(event.shortcut, text + i + 1, 128); + break; + } + } + if (!event.name[0]) return; + if (info.hSubMenu) event.flags |= WINE_ACCESS_MENU_SUBMENU; + if (info.fState & MFS_CHECKED) event.flags |= WINE_ACCESS_MENU_CHECKED; + if (info.fState & MFS_DISABLED) event.flags |= WINE_ACCESS_MENU_DISABLED; + GetWindowTextW(owner, event.title, 256); + } + COPYDATASTRUCT data = {WINE_ACCESS_MENU_TAG, sizeof(event), &event}; + DWORD_PTR result; + // The bridge only queues the payload; speech never runs in this hook. + SendMessageTimeoutW(bridge, WM_COPYDATA, (WPARAM)message->hwnd, + (LPARAM)&data, SMTO_ABORTIFHUNG | SMTO_BLOCK, 50, &result); + } +} + // This runs in the application's process: Wine does not expose another // process's HMENU contents to the standalone reader. __declspec(dllexport) LRESULT CALLBACK WineAccessMenuHook(int code, WPARAM wp, @@ -21,50 +68,8 @@ __declspec(dllexport) LRESULT CALLBACK WineAccessMenuHook(int code, WPARAM wp, if (code >= 0) { const CWPSTRUCT *message = (const CWPSTRUCT *)lp; if (message->message == WM_SETFOCUS) wine_access_watch_edit(message->hwnd); - if (message->message == WM_MENUSELECT || message->message == WM_EXITMENULOOP) { - HWND bridge = FindWindowW(WINE_ACCESS_MENU_CLASS, NULL); - HWND owner = GetAncestor(message->hwnd, GA_ROOT); - BOOL closed = message->message == WM_EXITMENULOOP || - (!message->lParam && HIWORD(message->wParam) == 0xffff); - if (bridge && (closed || owner == GetAncestor(GetForegroundWindow(), GA_ROOT))) { - WineAccessMenuEvent event = {0}; - UINT flags = HIWORD(message->wParam); - HMENU menu = (HMENU)message->lParam; - event.owner = (ULONGLONG)(UINT_PTR)owner; - event.menu = (ULONGLONG)(UINT_PTR)menu; - event.item = LOWORD(message->wParam); - if (closed) { - event.flags = WINE_ACCESS_MENU_CLOSED; - } else { - WCHAR text[640] = {0}; - MENUITEMINFOW info = {0}; - info.cbSize = sizeof(info); - info.fMask = MIIM_STRING | MIIM_STATE | MIIM_FTYPE | MIIM_SUBMENU; - info.dwTypeData = text; - info.cch = sizeof(text) / sizeof(text[0]); - if (!GetMenuItemInfoW(menu, event.item, (flags & MF_POPUP) != 0, &info) || - (info.fType & MFT_SEPARATOR)) - return CallNextHookEx(NULL, code, wp, lp); - menu_text(event.name, 512, text); - for (UINT i = 0; text[i]; ++i) { - if (text[i] == '\t') { - lstrcpynW(event.shortcut, text + i + 1, 128); - break; - } - } - if (!event.name[0]) return CallNextHookEx(NULL, code, wp, lp); - if (info.hSubMenu) event.flags |= WINE_ACCESS_MENU_SUBMENU; - if (info.fState & MFS_CHECKED) event.flags |= WINE_ACCESS_MENU_CHECKED; - if (info.fState & MFS_DISABLED) event.flags |= WINE_ACCESS_MENU_DISABLED; - GetWindowTextW(owner, event.title, 256); - } - COPYDATASTRUCT data = {WINE_ACCESS_MENU_TAG, sizeof(event), &event}; - DWORD_PTR result; - // The bridge only queues the payload; speech never runs in this hook. - SendMessageTimeoutW(bridge, WM_COPYDATA, (WPARAM)message->hwnd, - (LPARAM)&data, SMTO_ABORTIFHUNG | SMTO_BLOCK, 50, &result); - } - } + if (message->message == WM_MENUSELECT || message->message == WM_EXITMENULOOP) + handle_menu(message); } return CallNextHookEx(NULL, code, wp, lp); } diff --git a/menu_host32.c b/menu_host32.c index 07564a0..92be293 100644 --- a/menu_host32.c +++ b/menu_host32.c @@ -1,18 +1,9 @@ // SPDX-License-Identifier: LGPL-2.1-or-later #include "menu_bridge.h" +#include "foreground_hooks.h" #include #include -static DWORD foreground_thread32(void) { - DWORD pid = 0; - DWORD thread = GetWindowThreadProcessId(GetForegroundWindow(), &pid); - HANDLE process = pid ? OpenProcess(PROCESS_QUERY_LIMITED_INFORMATION, FALSE, pid) : NULL; - BOOL wow64 = FALSE; - BOOL known = process && IsWow64Process(process, &wow64); - if (process) CloseHandle(process); - return known && wow64 ? thread : 0; -} - // Hook DLLs must match the application's architecture. This small PE host // supplies 32-bit menu/editor hooks without requiring 32-bit Linux GLib/Winelib. int WINAPI WinMain(HINSTANCE instance, HINSTANCE previous, LPSTR args, int show) { @@ -40,29 +31,12 @@ int WINAPI WinMain(HINSTANCE instance, HINSTANCE previous, LPSTR args, int show) HMODULE module = LoadLibraryW(path); HOOKPROC callback = module ? (HOOKPROC)GetProcAddress(module, "WineAccessMenuHook") : NULL; HOOKPROC inputCallback = module ? (HOOKPROC)GetProcAddress(module, "WineAccessInputHook") : NULL; - HHOOK hook = NULL; - HHOOK inputHook = NULL; - DWORD hookedThread = 0; + WineAccessForegroundHooks hooks = {module, callback, inputCallback, NULL, NULL, 0}; if (!callback || !inputCallback) fprintf(stderr, "wine-access: 32-bit menu/editor hooks unavailable (error %lu)\n", GetLastError()); if (callback && inputCallback) { while (IsWindow(bridge)) { - // Wine's mixed-architecture global hook chain can suppress the 64-bit - // hook. Attach only to the foreground 32-bit UI thread, and release it - // when focus moves elsewhere. The 64-bit helper keeps its global hook. - DWORD thread = foreground_thread32(); - if (thread != hookedThread) { - if (hook) UnhookWindowsHookEx(hook); - if (inputHook) UnhookWindowsHookEx(inputHook); - hook = thread ? SetWindowsHookExW(WH_CALLWNDPROC, callback, module, thread) : NULL; - inputHook = thread ? SetWindowsHookExW(WH_GETMESSAGE, inputCallback, module, thread) : NULL; - hookedThread = hook && inputHook ? thread : 0; - if (!hookedThread) { - if (hook) UnhookWindowsHookEx(hook); - if (inputHook) UnhookWindowsHookEx(inputHook); - hook = inputHook = NULL; - } - } + wine_access_update_hooks(&hooks, TRUE); DWORD result = MsgWaitForMultipleObjects(1, &parent, FALSE, 100, QS_ALLINPUT); if (result == WAIT_OBJECT_0 || result == WAIT_FAILED) break; MSG message; @@ -71,8 +45,7 @@ int WINAPI WinMain(HINSTANCE instance, HINSTANCE previous, LPSTR args, int show) DispatchMessageW(&message); } } - if (hook) UnhookWindowsHookEx(hook); - if (inputHook) UnhookWindowsHookEx(inputHook); + wine_access_release_hooks(&hooks); } if (module) FreeLibrary(module); CloseHandle(parent); diff --git a/tests/README.md b/tests/README.md index 1d4ec69..2080d04 100644 --- a/tests/README.md +++ b/tests/README.md @@ -1,5 +1,35 @@ # Wine control integration +## Native hook scope and stack use + +Run sequentially after building: + +```sh +xvfb-run -a ctest --test-dir build -R hook-safety --output-on-failure +xvfb-run -a python3 tests/menu_integration.py --build-dir build --fixture hooks +xvfb-run -a python3 tests/menu_integration.py --build-dir build --fixture hooks --32-bit +``` + +The hook-safety test loads both actual DLLs in a disposable Wine prefix. It +compares nested ANSI Edit calls with a minimal comctl32 subclass, checks the +busy reentrant path and menu-hook forwarding stack, and exercises application +subclasses installed before and after attachment, A/W text queries, typing and +destruction during a text change. The scope fixture switches between UI threads +and checks that only the foreground thread acquires new Edit subclasses. + +The original 32-bit callbacks fail the stack budget; the original global 64-bit +hooks fail the background-thread check. These reproduce specific defects, +without proving they caused the live MUSHclient stack overflow. Hook DLLs keep +debug symbols and frame pointers to improve subsequent native stack captures. + +Also run the existing menu and editor tests for both architectures, including +`--late-helper --restart-helper`, the 32-bit list fixture and the reader timeout +test. For live acceptance, check MUSHclient typing, command history, incoming +plugin speech, menu return and switching between Wine and Linux applications. +Check Notepad, password exclusion and speech/braille separately. Loaded Edit +subclasses pin the DLL, so testing an installed replacement requires restarting +the affected Wine application as well as the helper. + ## Reader reply stalls Run sequentially on a private X display: @@ -170,7 +200,7 @@ MinGW's x86_64 and i686 C compilers, PyGObject, and `dbus-run-session`. The test creates a temporary Wine prefix and a private D-Bus service. Its 64-bit PE application exercises the separately running Winelib helper's -global menu hook, rather than calling helper functions inside the fixture. +foreground menu hook, rather than calling helper functions inside the fixture. The test processes carry the managed-helper marker so the running Cthulhu supervisor does not start an additional helper in the test prefix. diff --git a/tests/hook_safety_fixture.c b/tests/hook_safety_fixture.c new file mode 100644 index 0000000..831ddfc --- /dev/null +++ b/tests/hook_safety_fixture.c @@ -0,0 +1,125 @@ +// SPDX-License-Identifier: LGPL-2.1-or-later +#include +#include +#include +#include +#include +#include "../edit_hook.h" + +#define PROBE_MESSAGE (WM_APP + 10) +#define PROBE_DEPTH 16 +static WNDPROC original, outer_original; +static uintptr_t deepest, hook_stack; +static BOOL nested, destroy_on_set; +static unsigned calls; + +static LRESULT CALLBACK passthrough(HWND window, UINT message, WPARAM wp, LPARAM lp, + UINT_PTR id, DWORD_PTR reference) { + (void)id; (void)reference; + return DefSubclassProc(window, message, wp, lp); +} + +static LRESULT CALLBACK application_proc(HWND window, UINT message, WPARAM wp, LPARAM lp) { + volatile char marker = 0; + ++calls; + if (message == PROBE_MESSAGE) { + if (wp) return SendMessageA(window, PROBE_MESSAGE, wp - 1, 0); + deepest = (uintptr_t)▮ + return 123; + } + if (message == WM_SETTEXT && nested) { + char text[64]; + GetWindowTextA(window, text, sizeof(text)); + SendMessageA(window, PROBE_MESSAGE, PROBE_DEPTH, 0); + } + if (message == WM_SETTEXT && destroy_on_set) { + DestroyWindow(window); + return FALSE; + } + return CallWindowProcA(original, window, message, wp, lp); +} + +static LRESULT CALLBACK outer_proc(HWND window, UINT message, WPARAM wp, LPARAM lp) { + return CallWindowProcA(outer_original, window, message, wp, lp); +} + +static LRESULT CALLBACK observer(int code, WPARAM wp, LPARAM lp) { + volatile char marker = 0; + if (code >= 0 && ((CWPSTRUCT *)lp)->message == PROBE_MESSAGE) + hook_stack = (uintptr_t)▮ + return CallNextHookEx(NULL, code, wp, lp); +} + +static uintptr_t probe(HWND window, BOOL busy) { + volatile char marker = 0; + nested = busy; + if (busy) SetWindowTextA(window, "replacement"); + else if (SendMessageA(window, PROBE_MESSAGE, PROBE_DEPTH, 0) != 123) ExitProcess(2); + nested = FALSE; + return (uintptr_t)&marker - deepest; +} + +int main(int argc, char **argv) { + if (argc != 2) return 2; + HMODULE module = LoadLibraryA(argv[1]); + HOOKPROC callback = module ? (HOOKPROC)GetProcAddress(module, "WineAccessMenuHook") : NULL; + if (!callback) return 3; + HWND window = CreateWindowA("Edit", "initial", WS_POPUP | ES_MULTILINE, + 0, 0, 200, 100, NULL, NULL, GetModuleHandleW(NULL), NULL); + if (!window || IsWindowUnicode(window)) return 4; + original = (WNDPROC)SetWindowLongPtrA(window, GWLP_WNDPROC, (LONG_PTR)application_proc); + // Include comctl32's subclass dispatch in both measurements so the budget + // measures our callback rather than Wine's architecture-specific thunks. + if (!SetWindowSubclass(window, passthrough, 1, 0)) return 4; + uintptr_t baseline = probe(window, FALSE); + uintptr_t busy_baseline = probe(window, TRUE); + RemoveWindowSubclass(window, passthrough, 1); + // Attach through the real DLL's focus path, after the application's ANSI + // subclass. This also exercises Wine's A/W procedure conversion. + CWPSTRUCT focus = {0, 0, WM_SETFOCUS, window}; + callback(HC_ACTION, 0, (LPARAM)&focus); + if (!GetPropW(window, EDIT_STATE_PROPERTY)) return 5; + uintptr_t hooked = probe(window, FALSE); + uintptr_t busy_hooked = probe(window, TRUE); + printf("edit stack: baseline=%llu hooked=%llu busy-baseline=%llu busy-hooked=%llu\n", + (unsigned long long)baseline, (unsigned long long)hooked, + (unsigned long long)busy_baseline, (unsigned long long)busy_hooked); + // Allow comctl32 and A/W thunk overhead, but not a 4 KiB buffer frame on + // every ignored or reentrant message. This compares the same call chain. + BOOL stack_ok = hooked < baseline + PROBE_DEPTH * 2048 && + busy_hooked < busy_baseline + PROBE_DEPTH * 2048 + 8192; + + outer_original = (WNDPROC)SetWindowLongPtrA(window, GWLP_WNDPROC, (LONG_PTR)outer_proc); + SetWindowTextA(window, "ANSI replacement"); + SendMessageA(window, EM_SETSEL, 16, 16); + SendMessageA(window, WM_CHAR, '!', 1); + char text[64]; + WCHAR wide[64]; + GetWindowTextA(window, text, sizeof(text)); + GetWindowTextW(window, wide, 64); + if (strcmp(text, "ANSI replacement!") || lstrcmpW(wide, L"ANSI replacement!") || !calls) + return 6; + // Destroy while the accessibility callback is busy; it must defer freeing + // its state until the application's callback has returned. + destroy_on_set = TRUE; + SetWindowTextA(window, "destroy"); + if (IsWindow(window)) return 7; + + window = CreateWindowA("Static", "", WS_POPUP, 0, 0, 10, 10, + NULL, NULL, GetModuleHandleW(NULL), NULL); + HHOOK measure = SetWindowsHookExW(WH_CALLWNDPROC, observer, NULL, GetCurrentThreadId()); + if (!window || !measure) return 8; + SendMessageW(window, PROBE_MESSAGE, 0, 0); + uintptr_t unhooked_stack = hook_stack; + HHOOK hook = SetWindowsHookExW(WH_CALLWNDPROC, callback, module, GetCurrentThreadId()); + if (!hook) return 9; + SendMessageW(window, PROBE_MESSAGE, 0, 0); + uintptr_t overhead = unhooked_stack - hook_stack; + printf("menu forwarding stack overhead: %llu\n", (unsigned long long)overhead); + UnhookWindowsHookEx(hook); + UnhookWindowsHookEx(measure); + DestroyWindow(window); + if (!stack_ok || overhead >= 2048) return 10; + puts("PASS: bounded forwarding stack, ANSI subclass chaining, nested queries and destruction"); + return 0; +} diff --git a/tests/hook_safety_test.py b/tests/hook_safety_test.py new file mode 100644 index 0000000..66c755b --- /dev/null +++ b/tests/hook_safety_test.py @@ -0,0 +1,26 @@ +"""Exercise real PE callbacks in an isolated Wine prefix (both architectures).""" +import os +from pathlib import Path +import subprocess +import sys +import tempfile + +build = Path(sys.argv[1]).resolve() +with tempfile.TemporaryDirectory(prefix="wine-access-hook-safety-") as temporary: + env = os.environ.copy() + env.update(WINEPREFIX=str(Path(temporary) / "prefix"), WINEDEBUG="-all", + WINEDLLOVERRIDES="mscoree,mshtml=", CTHULHU_WINE_ACCESS_MANAGED="1") + try: + subprocess.run(["wineboot", "-u"], env=env, check=True, timeout=45) + failed = False + for arch in (32, 64): + dll = build / ("wine-access-menu32.dll" if arch == 32 else "wine-access-menu.dll") + result = subprocess.run(["wine", str(build / f"hook-safety-fixture{arch}.exe"), + "Z:" + str(dll).replace("/", "\\")], + env=env, timeout=20) + print(f"{arch}-bit hook safety: exit {result.returncode}", flush=True) + failed |= result.returncode != 0 + sys.exit(1 if failed else 0) + finally: + subprocess.run(["wineserver", "-k"], env=env, timeout=5) + subprocess.run(["wineserver", "-w"], env=env, timeout=5) diff --git a/tests/hooks_fixture.c b/tests/hooks_fixture.c new file mode 100644 index 0000000..e4419bb --- /dev/null +++ b/tests/hooks_fixture.c @@ -0,0 +1,83 @@ +// SPDX-License-Identifier: LGPL-2.1-or-later +#include +#include +#include "../edit_hook.h" + +static HWND worker_window; +static HANDLE ready; + +static void pump(DWORD milliseconds) { + DWORD end = GetTickCount() + milliseconds; + do { + MSG message; + while (PeekMessageW(&message, NULL, 0, 0, PM_REMOVE)) { + TranslateMessage(&message); + DispatchMessageW(&message); + } + Sleep(10); + } while ((LONG)(end - GetTickCount()) > 0); +} + +static BOOL attached_on_focus(HWND parent) { + HWND edit = CreateWindowA("Edit", "ANSI background probe", WS_CHILD, + 0, 0, 100, 30, parent, NULL, GetModuleHandleW(NULL), NULL); + if (!edit) ExitProcess(2); + // A background application can send focus messages of its own. It must + // not acquire new accessibility subclasses through a desktop-wide hook. + SendMessageW(edit, WM_SETFOCUS, 0, 0); + BOOL attached = GetPropW(edit, EDIT_STATE_PROPERTY) != NULL; + DestroyWindow(edit); + return attached; +} + +static LRESULT CALLBACK window_proc(HWND window, UINT message, WPARAM wp, LPARAM lp) { + if (message == WM_APP) return attached_on_focus(window); + if (message == WM_CLOSE) { DestroyWindow(window); PostQuitMessage(0); return 0; } + return DefWindowProcW(window, message, wp, lp); +} + +static DWORD WINAPI worker(void *unused) { + (void)unused; + worker_window = CreateWindowW(L"WineAccessHookScope", L"Other UI thread", WS_OVERLAPPEDWINDOW, + 300, 0, 200, 120, NULL, NULL, GetModuleHandleW(NULL), NULL); + SetEvent(ready); + MSG message; + while (GetMessageW(&message, NULL, 0, 0) > 0) DispatchMessageW(&message); + return 0; +} + +int main(void) { + WNDCLASSW cls = {0}; + cls.lpfnWndProc = window_proc; + cls.hInstance = GetModuleHandleW(NULL); + cls.lpszClassName = L"WineAccessHookScope"; + if (!RegisterClassW(&cls)) return 2; + HWND window = CreateWindowW(cls.lpszClassName, L"Hook scope fixture", WS_OVERLAPPEDWINDOW, + 0, 0, 200, 120, NULL, NULL, cls.hInstance, NULL); + ready = CreateEventW(NULL, TRUE, FALSE, NULL); + HANDLE thread = CreateThread(NULL, 0, worker, NULL, 0, NULL); + if (!window || !ready || !thread || WaitForSingleObject(ready, 3000) != WAIT_OBJECT_0 || + !worker_window) return 3; + ShowWindow(window, SW_SHOW); + SetForegroundWindow(window); + pump(600); + BOOL first = GetForegroundWindow() == window && attached_on_focus(window); + BOOL background = SendMessageW(worker_window, WM_APP, 0, 0); + ShowWindow(worker_window, SW_SHOW); + SetForegroundWindow(worker_window); + pump(600); + BOOL switched = GetForegroundWindow() == worker_window && SendMessageW(worker_window, WM_APP, 0, 0); + BOOL old_thread = attached_on_focus(window); + SetForegroundWindow(window); + pump(600); + BOOL restored = GetForegroundWindow() == window && attached_on_focus(window); + BOOL detached = !SendMessageW(worker_window, WM_APP, 0, 0); + printf("hook scope: initial=%d background=%d switched=%d old-thread=%d restored=%d detached=%d\n", + first, background, switched, old_thread, restored, detached); + PostMessageW(worker_window, WM_CLOSE, 0, 0); + WaitForSingleObject(thread, 3000); + CloseHandle(thread); + CloseHandle(ready); + DestroyWindow(window); + return first && !background && switched && !old_thread && restored && detached ? 0 : 4; +} diff --git a/tests/menu_integration.py b/tests/menu_integration.py index 799bc3b..fc2cd84 100644 --- a/tests/menu_integration.py +++ b/tests/menu_integration.py @@ -18,15 +18,15 @@ parser = argparse.ArgumentParser() parser.add_argument("--build-dir", default="build") parser.add_argument("--session", action="store_true") parser.add_argument("--reader", choices=("cthulhu", "orca")) -parser.add_argument("--fixture", choices=("menu", "edit", "notepad", "list", "listbox", "delphi", "winforms", "controls", "scintilla", "goldwave"), default="menu") +parser.add_argument("--fixture", choices=("menu", "edit", "hooks", "notepad", "list", "listbox", "delphi", "winforms", "controls", "scintilla", "goldwave"), default="menu") parser.add_argument("--notepadpp-exe", type=Path) parser.add_argument("--goldwave-exe", type=Path) parser.add_argument("--32-bit", dest="is32Bit", action="store_true") parser.add_argument("--late-helper", action="store_true") parser.add_argument("--restart-helper", action="store_true") args = parser.parse_args() -if args.is32Bit and args.fixture not in ("menu", "edit", "winforms", "list", "listbox", "delphi", "controls"): - parser.error("32-bit checks currently require --fixture menu, edit, winforms, list, listbox, delphi or controls") +if args.is32Bit and args.fixture not in ("menu", "edit", "hooks", "winforms", "list", "listbox", "delphi", "controls"): + parser.error("32-bit checks currently require --fixture menu, edit, hooks, winforms, list, listbox, delphi or controls") if (args.late_helper or args.restart_helper) and args.fixture not in ("edit", "winforms", "scintilla"): parser.error("helper lifecycle checks require --fixture edit, winforms or scintilla") if args.fixture == "scintilla" and (not args.notepadpp_exe or not args.notepadpp_exe.is_file()): @@ -139,6 +139,9 @@ with tempfile.TemporaryDirectory(prefix="wine-access-menus-") as temporary: while GLib.MainContext.default().iteration(False): pass time.sleep(0.01) + if args.fixture == "hooks": + print("PASS: hooks follow only the foreground UI thread, detach and reattach") + sys.exit(0) focused = None for event in events: if event[1] == "focus":