[jimtabor] 65815: [NtUser] - Crash while in recursion through hook calls. Move fast calls to api. Placed safe guards for removing message queue structures. Lock threads while calling hooks. Just ad...
Author: jimtabor Date: Tue Dec 23 19:05:44 2014 New Revision: 65815
URL: http://svn.reactos.org/svn/reactos?rev=65815&view=rev Log: [NtUser] - Crash while in recursion through hook calls. Move fast calls to api. Placed safe guards for removing message queue structures. Lock threads while calling hooks. Just add paranoid code to prevent recursion freeing that will crash the kernel at some point. Fixes OpenMPT crashes, see CORE-8819. This might improve the same issues with CORE-6734.
Modified: trunk/reactos/win32ss/user/ntuser/event.c trunk/reactos/win32ss/user/ntuser/hook.c trunk/reactos/win32ss/user/ntuser/hook.h trunk/reactos/win32ss/user/ntuser/message.c trunk/reactos/win32ss/user/ntuser/msgqueue.c trunk/reactos/win32ss/user/ntuser/msgqueue.h
Modified: trunk/reactos/win32ss/user/ntuser/event.c URL: http://svn.reactos.org/svn/reactos/trunk/reactos/win32ss/user/ntuser/event.c... ============================================================================== --- trunk/reactos/win32ss/user/ntuser/event.c [iso-8859-1] (original) +++ trunk/reactos/win32ss/user/ntuser/event.c [iso-8859-1] Tue Dec 23 19:05:44 2014 @@ -2,7 +2,7 @@ * COPYRIGHT: See COPYING in the top level directory * PROJECT: ReactOS kernel * PURPOSE: Window event handlers - * FILE: subsystems/win32/win32k/ntuser/event.c + * FILE: win32ss/user/ntuser/event.c * PROGRAMER: James Tabor (james.tabor@rectos.org) */
@@ -153,7 +153,7 @@ // Dispatch MsgQueue Event Call processor! // LRESULT -FASTCALL +APIENTRY co_EVENT_CallEvents( DWORD event, HWND hwnd, UINT_PTR idObject,
Modified: trunk/reactos/win32ss/user/ntuser/hook.c URL: http://svn.reactos.org/svn/reactos/trunk/reactos/win32ss/user/ntuser/hook.c?... ============================================================================== --- trunk/reactos/win32ss/user/ntuser/hook.c [iso-8859-1] (original) +++ trunk/reactos/win32ss/user/ntuser/hook.c [iso-8859-1] Tue Dec 23 19:05:44 2014 @@ -2,7 +2,7 @@ * COPYRIGHT: See COPYING in the top level directory * PROJECT: ReactOS kernel * PURPOSE: Window hooks - * FILE: subsystems/win32/win32k/ntuser/hook.c + * FILE: win32ss/user/ntuser/hook.c * PROGRAMER: Casper S. Hornstrup (chorns@users.sourceforge.net) * James Tabor (james.tabor@rectos.org) * Rafal Harabien (rafalh@reactos.org) @@ -92,7 +92,7 @@ PLIST_ENTRY ListEntry; PPROCESSINFO ppiCsr;
- ERR("IntHookModuleUnloaded: iHookID=%d\n", iHookID); + TRACE("IntHookModuleUnloaded: iHookID=%d\n", iHookID);
ppiCsr = PsGetProcessWin32Process(gpepCSRSS);
@@ -222,7 +222,7 @@ return FALSE; }
- ERR("UserUnregisterUserApiHook. Server PID: %p\n", PsGetProcessId(pti->ppi->peProcess)); + TRACE("UserUnregisterUserApiHook. Server PID: %p\n", PsGetProcessId(pti->ppi->peProcess));
/* Unregister the api hook */ gpsi->dwSRVIFlags &= ~SRVINFO_APIHOOK; @@ -318,7 +318,7 @@ // Dispatch MsgQueue Hook Call processor! // LRESULT -FASTCALL +APIENTRY co_CallHook( INT HookId, INT Code, WPARAM wParam, @@ -335,14 +335,13 @@ { case WH_JOURNALPLAYBACK: case WH_JOURNALRECORD: - case WH_KEYBOARD: case WH_KEYBOARD_LL: case WH_MOUSE_LL: case WH_MOUSE: lParam = (LPARAM)pHP->pHookStructs; + case WH_KEYBOARD: break; } - /* The odds are high for this to be a Global call. */ Result = co_IntCallHookProc( HookId, Code, @@ -362,7 +361,7 @@
static LRESULT -FASTCALL +APIENTRY co_HOOK_CallHookNext( PHOOK Hook, INT Code, WPARAM wParam, @@ -516,7 +515,7 @@
static LRESULT -FASTCALL +APIENTRY co_UserCallNextHookEx(PHOOK Hook, int Code, WPARAM wParam, @@ -1080,7 +1079,7 @@ Win32k Kernel Space Hook Caller. */ LRESULT -FASTCALL +APIENTRY co_HOOK_CallHooks( INT HookId, INT Code, WPARAM wParam, @@ -1150,6 +1149,8 @@ }
Hook = CONTAINING_RECORD(pLastHead->Flink, HOOK, Chain); + ObReferenceObject(pti->pEThread); + IntReferenceThreadInfo(pti); UserRefObjectCo(Hook, &Ref);
ClientInfo = pti->pClientInfo; @@ -1199,6 +1200,8 @@ pti->sphkCurrent = SaveHook; Hook->phkNext = NULL; UserDerefObjectCo(Hook); + IntDereferenceThreadInfo(pti); + ObDereferenceObject(pti->pEThread); }
if ( Global ) @@ -1224,7 +1227,6 @@ ERR("Invalid hook!\n"); continue; } - UserRefObjectCo(Hook, &Ref);
/* Hook->Thread is null, we hax around this with Hook->head.pti. */ ptiHook = Hook->head.pti; @@ -1234,6 +1236,7 @@ TRACE("Next Hook %p, %p\n", ptiHook->rpdesk, pdo); continue; } + UserRefObjectCo(Hook, &Ref);
if (ptiHook != pti ) { @@ -1251,7 +1254,9 @@ } else { /* Make the direct call. */ - TRACE("Local Hook calling to Thread! %d\n",HookId ); + TRACE("Global going Local Hook calling to Thread! %d\n",HookId ); + ObReferenceObject(pti->pEThread); + IntReferenceThreadInfo(pti); Result = co_IntCallHookProc( HookId, Code, wParam, @@ -1261,6 +1266,8 @@ Hook->offPfn, Hook->Ansi, &Hook->ModuleName); + IntDereferenceThreadInfo(pti); + ObDereferenceObject(pti->pEThread); } UserDerefObjectCo(Hook); } @@ -1434,7 +1441,7 @@ HookId == WH_MOUSE_LL || HookId == WH_SYSMSGFILTER) { - ERR("Local hook installing Global HookId: %d\n",HookId); + TRACE("Local hook installing Global HookId: %d\n",HookId); /* these can only be global */ EngSetLastError(ERROR_GLOBAL_ONLY_HOOK); RETURN( NULL);
Modified: trunk/reactos/win32ss/user/ntuser/hook.h URL: http://svn.reactos.org/svn/reactos/trunk/reactos/win32ss/user/ntuser/hook.h?... ============================================================================== --- trunk/reactos/win32ss/user/ntuser/hook.h [iso-8859-1] (original) +++ trunk/reactos/win32ss/user/ntuser/hook.h [iso-8859-1] Tue Dec 23 19:05:44 2014 @@ -40,12 +40,12 @@ DWORD flags; } NOTIFYEVENT, *PNOTIFYEVENT;
-LRESULT FASTCALL co_CallHook(INT HookId, INT Code, WPARAM wParam, LPARAM lParam); -LRESULT FASTCALL co_HOOK_CallHooks(INT HookId, INT Code, WPARAM wParam, LPARAM lParam); -LRESULT FASTCALL co_EVENT_CallEvents(DWORD, HWND, UINT_PTR, LONG_PTR); +LRESULT APIENTRY co_CallHook(INT HookId, INT Code, WPARAM wParam, LPARAM lParam); +LRESULT APIENTRY co_HOOK_CallHooks(INT HookId, INT Code, WPARAM wParam, LPARAM lParam); +LRESULT APIENTRY co_EVENT_CallEvents(DWORD, HWND, UINT_PTR, LONG_PTR); PHOOK FASTCALL IntGetHookObject(HHOOK); PHOOK FASTCALL IntGetNextHook(PHOOK Hook); -LRESULT FASTCALL UserCallNextHookEx( PHOOK pHook, int Code, WPARAM wParam, LPARAM lParam, BOOL Ansi); +LRESULT APIENTRY UserCallNextHookEx( PHOOK pHook, int Code, WPARAM wParam, LPARAM lParam, BOOL Ansi); BOOL FASTCALL IntUnhookWindowsHook(int,HOOKPROC); BOOLEAN IntRemoveHook(PVOID Object); BOOLEAN IntRemoveEvent(PVOID Object);
Modified: trunk/reactos/win32ss/user/ntuser/message.c URL: http://svn.reactos.org/svn/reactos/trunk/reactos/win32ss/user/ntuser/message... ============================================================================== --- trunk/reactos/win32ss/user/ntuser/message.c [iso-8859-1] (original) +++ trunk/reactos/win32ss/user/ntuser/message.c [iso-8859-1] Tue Dec 23 19:05:44 2014 @@ -766,7 +766,7 @@ * WM_PAINT messages * WM_TIMER messages */ -BOOL FASTCALL +BOOL APIENTRY co_IntPeekMessage( PMSG Msg, PWND Window, UINT MsgFilterMin, @@ -952,7 +952,7 @@ return FALSE; }
-BOOL FASTCALL +BOOL APIENTRY co_IntGetPeekMessage( PMSG pMsg, HWND hWnd, UINT MsgFilterMin,
Modified: trunk/reactos/win32ss/user/ntuser/msgqueue.c URL: http://svn.reactos.org/svn/reactos/trunk/reactos/win32ss/user/ntuser/msgqueu... ============================================================================== --- trunk/reactos/win32ss/user/ntuser/msgqueue.c [iso-8859-1] (original) +++ trunk/reactos/win32ss/user/ntuser/msgqueue.c [iso-8859-1] Tue Dec 23 19:05:44 2014 @@ -26,9 +26,9 @@ NTAPI MsqInitializeImpl(VOID) { - pgMessageLookasideList = ExAllocatePoolWithTag(NonPagedPool, sizeof(PAGED_LOOKASIDE_LIST), TAG_USRMSG); - if(!pgMessageLookasideList) - return STATUS_NO_MEMORY; + pgMessageLookasideList = ExAllocatePoolWithTag(NonPagedPool, sizeof(PAGED_LOOKASIDE_LIST), TAG_USRMSG); + if (!pgMessageLookasideList) + return STATUS_NO_MEMORY; ExInitializePagedLookasideList(pgMessageLookasideList, NULL, NULL, @@ -732,6 +732,7 @@ return NULL; }
+ RtlZeroMemory(Message, sizeof(Message)); RtlMoveMemory(&Message->Msg, Msg, sizeof(MSG));
return Message; @@ -740,6 +741,12 @@ VOID FASTCALL MsqDestroyMessage(PUSER_MESSAGE Message) { + if (Message->pti == NULL) + { + ERR("Double Free Message\n"); + return; + } + Message->pti = NULL; ExFreeToPagedLookasideList(pgMessageLookasideList, Message); }
@@ -1141,7 +1148,6 @@
UserLeaveCo();
- /* Don't process messages sent to the thread */ WaitStatus = KeWaitForMultipleObjects(2, WaitObjects, WaitAny, UserRequest, UserMode, FALSE, (uTimeout ? &Timeout : NULL), NULL);
@@ -1306,7 +1312,7 @@ ERR("User APC Returned\n"); // Should not see this message. }
- if (WaitStatus != STATUS_TIMEOUT) + if ( WaitStatus != STATUS_TIMEOUT ) { if (uResult) { @@ -1344,18 +1350,15 @@ if (dwQEvent) { ERR("Post Msg; System Qeued Event Message!\n"); - InsertHeadList(&pti->PostedMessagesListHead, - &Message->ListEntry); + InsertHeadList(&pti->PostedMessagesListHead, &Message->ListEntry); } else if (!HardwareMessage) { - InsertTailList(&pti->PostedMessagesListHead, - &Message->ListEntry); + InsertTailList(&pti->PostedMessagesListHead, &Message->ListEntry); } else { - InsertTailList(&MessageQueue->HardwareMessagesListHead, - &Message->ListEntry); + InsertTailList(&MessageQueue->HardwareMessagesListHead, &Message->ListEntry); }
if (Msg->message == WM_HOTKEY) MessageBits |= QS_HOTKEY; // Justin Case, just set it. @@ -1669,6 +1672,7 @@ { /* Remove and ignore the message */ *RemoveMessages = TRUE; + TRACE("Remove and ignore the message\n"); RETURN(FALSE); } } @@ -1784,6 +1788,10 @@ BOOL co_IntProcessKeyboardMessage(MSG* Msg, BOOL* RemoveMessages) { EVENTMSG Event; + USER_REFERENCE_ENTRY Ref; + PWND pWnd; + BOOL Ret = TRUE; + PTHREADINFO pti = PsGetCurrentThreadWin32Thread();
if (Msg->message == WM_KEYDOWN || Msg->message == WM_SYSKEYDOWN || Msg->message == WM_KEYUP || Msg->message == WM_SYSKEYUP) @@ -1802,6 +1810,9 @@ } }
+ pWnd = ValidateHwndNoErr(Msg->hwnd); + if (pWnd) UserRefObjectCo(pWnd, &Ref); + Event.message = Msg->message; Event.hwnd = Msg->hwnd; Event.time = Msg->time; @@ -1810,8 +1821,33 @@ if (HIWORD(Msg->lParam) & 0x0100) Event.paramH |= 0x8000; co_HOOK_CallHooks( WH_JOURNALRECORD, HC_ACTION, 0, (LPARAM)&Event);
+ if (*RemoveMessages) + { + if ((Msg->message == WM_KEYDOWN) && + (Msg->hwnd != IntGetDesktopWindow())) + { + /* Handle F1 key by sending out WM_HELP message */ + if (Msg->wParam == VK_F1) + { + UserPostMessage( Msg->hwnd, WM_KEYF1, 0, 0 ); + } + else if (Msg->wParam >= VK_BROWSER_BACK && + Msg->wParam <= VK_LAUNCH_APP2) + { + /* FIXME: Process keystate */ + co_IntSendMessage(Msg->hwnd, WM_APPCOMMAND, (WPARAM)Msg->hwnd, MAKELPARAM(0, (FAPPCOMMAND_KEY | (Msg->wParam - VK_BROWSER_BACK + 1)))); + } + } + else if (Msg->message == WM_KEYUP) + { + /* Handle VK_APPS key by posting a WM_CONTEXTMENU message */ + if (Msg->wParam == VK_APPS && pti->MessageQueue->MenuOwner == NULL) + UserPostMessage( Msg->hwnd, WM_CONTEXTMENU, (WPARAM)Msg->hwnd, -1 ); + } + } + if (co_HOOK_CallHooks( WH_KEYBOARD, - *RemoveMessages ? HC_ACTION : HC_NOREMOVE, + *RemoveMessages ? HC_ACTION : HC_NOREMOVE, LOWORD(Msg->wParam), Msg->lParam)) { @@ -1820,10 +1856,15 @@ HCBT_KEYSKIPPED, LOWORD(Msg->wParam), Msg->lParam ); - ERR("KeyboardMessage WH_CBT Call Hook return!\n"); - return FALSE; - } - return TRUE; + + ERR("KeyboardMessage WH_KEYBOARD Call Hook return!\n"); + + *RemoveMessages = TRUE; + + Ret = FALSE; + } + if (pWnd) UserDerefObjectCo(pWnd); + return Ret; }
BOOL co_IntProcessHardwareMessage(MSG* Msg, BOOL* RemoveMessages, UINT first, UINT last) @@ -1866,41 +1907,43 @@ IN UINT QSflags, OUT MSG* pMsg) { - - BOOL AcceptMessage; - PUSER_MESSAGE CurrentMessage; - PLIST_ENTRY ListHead, CurrentEntry = NULL; - MSG msg; - BOOL Ret = FALSE; - PUSER_MESSAGE_QUEUE MessageQueue = pti->MessageQueue; - - if (!filter_contains_hw_range( MsgFilterLow, MsgFilterHigh )) return FALSE; - - ListHead = &MessageQueue->HardwareMessagesListHead; - CurrentEntry = ListHead->Flink; - - if (IsListEmpty(CurrentEntry)) return FALSE; - - if (!MessageQueue->ptiSysLock) - { - MessageQueue->ptiSysLock = pti; - pti->pcti->CTI_flags |= CTI_THREADSYSLOCK; - } - - if (MessageQueue->ptiSysLock != pti) - { - ERR("MsqPeekHardwareMessage: Thread Q is locked to another pti!\n"); - return FALSE; - } - - CurrentMessage = CONTAINING_RECORD(CurrentEntry, USER_MESSAGE, - ListEntry); - do - { - if (IsListEmpty(CurrentEntry)) break; - if (!CurrentMessage) break; - CurrentEntry = CurrentMessage->ListEntry.Flink; - if (!CurrentEntry) break; //// Fix CORE-6734 reported crash. + BOOL AcceptMessage; + PUSER_MESSAGE CurrentMessage; + PLIST_ENTRY ListHead; + MSG msg; + ULONG_PTR idSave; + DWORD QS_Flags; + BOOL Ret = FALSE; + PUSER_MESSAGE_QUEUE MessageQueue = pti->MessageQueue; + + if (!filter_contains_hw_range( MsgFilterLow, MsgFilterHigh )) return FALSE; + + ListHead = MessageQueue->HardwareMessagesListHead.Flink; + + if (IsListEmpty(ListHead)) return FALSE; + + if (!MessageQueue->ptiSysLock) + { + MessageQueue->ptiSysLock = pti; + pti->pcti->CTI_flags |= CTI_THREADSYSLOCK; + } + + if (MessageQueue->ptiSysLock != pti) + { + ERR("MsqPeekHardwareMessage: Thread Q is locked to another pti!\n"); + return FALSE; + } + + while (ListHead != &MessageQueue->HardwareMessagesListHead) + { + CurrentMessage = CONTAINING_RECORD(ListHead, USER_MESSAGE, ListEntry); + ListHead = ListHead->Flink; + + if (MessageQueue->idSysPeek == (ULONG_PTR)CurrentMessage) + { + TRACE("Skip this message due to it is in play!\n"); + continue; + } /* MSDN: 1: any window that belongs to the current thread, and any messages on the current thread's message queue whose hwnd value is NULL. @@ -1913,33 +1956,40 @@ ( CurrentMessage->Msg.message == WM_MOUSEMOVE ) ) && // Null window for mouse moves. ( ( ( MsgFilterLow == 0 && MsgFilterHigh == 0 ) && CurrentMessage->QS_Flags & QSflags ) || ( MsgFilterLow <= CurrentMessage->Msg.message && MsgFilterHigh >= CurrentMessage->Msg.message ) ) ) - { - msg = CurrentMessage->Msg; - - UpdateKeyStateFromMsg(MessageQueue, &msg); - AcceptMessage = co_IntProcessHardwareMessage(&msg, &Remove, MsgFilterLow, MsgFilterHigh); - - if (Remove) - { - RemoveEntryList(&CurrentMessage->ListEntry); - ClearMsgBitsMask(pti, CurrentMessage->QS_Flags); - MsqDestroyMessage(CurrentMessage); - } - - if (AcceptMessage) - { - *pMsg = msg; - Ret = TRUE; - break; - } - } - CurrentMessage = CONTAINING_RECORD(CurrentEntry, USER_MESSAGE, ListEntry); - } - while(CurrentEntry != ListHead); - - MessageQueue->ptiSysLock = NULL; - pti->pcti->CTI_flags &= ~CTI_THREADSYSLOCK; - return Ret; + { + idSave = MessageQueue->idSysPeek; + MessageQueue->idSysPeek = (ULONG_PTR)CurrentMessage; + + msg = CurrentMessage->Msg; + QS_Flags = CurrentMessage->QS_Flags; + + UpdateKeyStateFromMsg(MessageQueue, &msg); + AcceptMessage = co_IntProcessHardwareMessage(&msg, &Remove, MsgFilterLow, MsgFilterHigh); + + if (Remove) + { + if (CurrentMessage->pti != NULL) + { + RemoveEntryList(&CurrentMessage->ListEntry); + MsqDestroyMessage(CurrentMessage); + } + ClearMsgBitsMask(pti, QS_Flags); + } + + MessageQueue->idSysPeek = idSave; + + if (AcceptMessage) + { + *pMsg = msg; + Ret = TRUE; + break; + } + } + } + + MessageQueue->ptiSysLock = NULL; + pti->pcti->CTI_flags &= ~CTI_THREADSYSLOCK; + return Ret; }
BOOLEAN APIENTRY @@ -1951,23 +2001,19 @@ IN UINT QSflags, OUT PMSG Message) { - PLIST_ENTRY CurrentEntry; PUSER_MESSAGE CurrentMessage; PLIST_ENTRY ListHead; + DWORD QS_Flags; BOOL Ret = FALSE;
- CurrentEntry = pti->PostedMessagesListHead.Flink; - ListHead = &pti->PostedMessagesListHead; - - if (IsListEmpty(CurrentEntry)) return FALSE; - - CurrentMessage = CONTAINING_RECORD(CurrentEntry, USER_MESSAGE, - ListEntry); - do - { - if (IsListEmpty(CurrentEntry)) break; - if (!CurrentMessage) break; - CurrentEntry = CurrentEntry->Flink; + ListHead = pti->PostedMessagesListHead.Flink; + + if (IsListEmpty(ListHead)) return FALSE; + + while(ListHead != &pti->PostedMessagesListHead) + { + CurrentMessage = CONTAINING_RECORD(ListHead, USER_MESSAGE, ListEntry); + ListHead = ListHead->Flink; /* MSDN: 1: any window that belongs to the current thread, and any messages on the current thread's message queue whose hwnd value is NULL. @@ -1981,20 +2027,21 @@ ( MsgFilterLow <= CurrentMessage->Msg.message && MsgFilterHigh >= CurrentMessage->Msg.message ) ) ) { *Message = CurrentMessage->Msg; + QS_Flags = CurrentMessage->QS_Flags;
if (Remove) { - RemoveEntryList(&CurrentMessage->ListEntry); - ClearMsgBitsMask(pti, CurrentMessage->QS_Flags); - MsqDestroyMessage(CurrentMessage); + if (CurrentMessage->pti != NULL) + { + RemoveEntryList(&CurrentMessage->ListEntry); + MsqDestroyMessage(CurrentMessage); + } + ClearMsgBitsMask(pti, QS_Flags); } Ret = TRUE; break; } - CurrentMessage = CONTAINING_RECORD(CurrentEntry, USER_MESSAGE, - ListEntry); - } - while (CurrentEntry != ListHead); + }
return Ret; }
Modified: trunk/reactos/win32ss/user/ntuser/msgqueue.h URL: http://svn.reactos.org/svn/reactos/trunk/reactos/win32ss/user/ntuser/msgqueu... ============================================================================== --- trunk/reactos/win32ss/user/ntuser/msgqueue.h [iso-8859-1] (original) +++ trunk/reactos/win32ss/user/ntuser/msgqueue.h [iso-8859-1] Tue Dec 23 19:05:44 2014 @@ -46,6 +46,8 @@ struct _DESKTOP *Desktop;
PTHREADINFO ptiSysLock; + ULONG_PTR idSysLock; + ULONG_PTR idSysPeek; PTHREADINFO ptiMouse; PTHREADINFO ptiKeyboard;
@@ -257,7 +259,7 @@
UINT lParamMemorySize(UINT Msg, WPARAM wParam, LPARAM lParam);
-BOOL FASTCALL +BOOL APIENTRY co_IntGetPeekMessage( PMSG pMsg, HWND hWnd, UINT MsgFilterMin,
-
jimtabor＠svn.reactos.org