From 2ff860c8f129404567f6261c5ff9b031d5130470 Mon Sep 17 00:00:00 2001 From: James Tabor Date: Mon, 16 Feb 2015 03:16:01 +0000 Subject: [PATCH] [NtUser] - This fixes use after free linking in the message system. See CORE-9173. Dedicated to Thomas Faber. svn path=/trunk/; revision=66309 --- reactos/win32ss/user/ntuser/msgqueue.c | 75 +++++--------------------- 1 file changed, 12 insertions(+), 63 deletions(-) diff --git a/reactos/win32ss/user/ntuser/msgqueue.c b/reactos/win32ss/user/ntuser/msgqueue.c index ce5b3efb59e..c5b90994023 100644 --- a/reactos/win32ss/user/ntuser/msgqueue.c +++ b/reactos/win32ss/user/ntuser/msgqueue.c @@ -789,8 +789,7 @@ co_MsqDispatchOneSentMessage(PTHREADINFO pti) /* insert it to the list of messages that are currently dispatched by this message queue */ - InsertTailList(&pti->LocalDispatchingMessagesHead, - &Message->ListEntry); + InsertTailList(&pti->LocalDispatchingMessagesHead, &Message->ListEntry); ClearMsgBitsMask(pti, Message->QS_Flags); @@ -866,7 +865,7 @@ co_MsqDispatchOneSentMessage(PTHREADINFO pti) } /* remove the message from the dispatching list if needed, so lock the sender's message queue */ - if (Message->ptiSender) + if (Message->ptiSender && !(Message->ptiSender->TIF_flags & TIF_INCLEANUP)) { if (Message->DispatchingListEntry.Flink != NULL) { @@ -953,8 +952,8 @@ MsqRemoveWindowMessagesFromQueue(PWND Window) ListHead = &pti->SentMessagesListHead; while (CurrentEntry != ListHead) { - SentMessage = CONTAINING_RECORD(CurrentEntry, USER_SENT_MESSAGE, - ListEntry); + SentMessage = CONTAINING_RECORD(CurrentEntry, USER_SENT_MESSAGE, ListEntry); + if(SentMessage->Msg.hwnd == Window->head.h) { TRACE("Notify the sender and remove a message from the queue that had not been dispatched\n"); @@ -1187,6 +1186,7 @@ co_MsqSendMessage(PTHREADINFO ptirec, Message->CompletionEvent = NULL; Message->Result = NULL; RemoveEntryList(&Message->ListEntry); + RemoveEntryList(&Message->DispatchingListEntry); ClearMsgBitsMask(ptirec, Message->QS_Flags); ExFreePoolWithTag(Message, TAG_USRMSG); break; @@ -1194,30 +1194,8 @@ co_MsqSendMessage(PTHREADINFO ptirec, Entry = Entry->Flink; } - /* Remove from the local dispatching list so the other thread knows, - it can't pass a result and it must not set the completion event anymore */ - Entry = pti->DispatchingMessagesHead.Flink; - while (Entry != &pti->DispatchingMessagesHead) - { - if ((PUSER_SENT_MESSAGE) CONTAINING_RECORD(Entry, USER_SENT_MESSAGE, DispatchingListEntry) - == Message) - { - /* We can access Message here, it's secure because the sender's message is locked - and the message has definitely not yet been destroyed, otherwise it would - have been removed from this list by the dispatching routine right after - dispatching the message */ - Message->CompletionEvent = NULL; - Message->Result = NULL; - RemoveEntryList(&Message->DispatchingListEntry); - Message->DispatchingListEntry.Flink = NULL; - break; - } - Entry = Entry->Flink; - } - TRACE("MsqSendMessage (blocked) timed out 1 Status %p\n",WaitStatus); - - } + } // Receiving thread passed on and left us hanging with issues still pending. if ( WaitStatus == STATUS_WAIT_1 ) { @@ -1272,6 +1250,7 @@ co_MsqSendMessage(PTHREADINFO ptirec, Message->CompletionEvent = NULL; Message->Result = NULL; RemoveEntryList(&Message->ListEntry); + RemoveEntryList(&Message->DispatchingListEntry); ClearMsgBitsMask(ptirec, Message->QS_Flags); ExFreePoolWithTag(Message, TAG_USRMSG); break; @@ -1279,27 +1258,6 @@ co_MsqSendMessage(PTHREADINFO ptirec, Entry = Entry->Flink; } - /* Remove from the local dispatching list so the other thread knows, - it can't pass a result and it must not set the completion event anymore */ - Entry = pti->DispatchingMessagesHead.Flink; - while (Entry != &pti->DispatchingMessagesHead) - { - if ((PUSER_SENT_MESSAGE) CONTAINING_RECORD(Entry, USER_SENT_MESSAGE, DispatchingListEntry) - == Message) - { - /* We can access Message here, it's secure because the sender's message is locked - and the message has definitely not yet been destroyed, otherwise it would - have been removed from this list by the dispatching routine right after - dispatching the message */ - Message->CompletionEvent = NULL; - Message->Result = NULL; - RemoveEntryList(&Message->DispatchingListEntry); - Message->DispatchingListEntry.Flink = NULL; - break; - } - Entry = Entry->Flink; - } - TRACE("MsqSendMessage timed out 2 Status %p\n",WaitStatus); break; @@ -1375,12 +1333,7 @@ MsqPostMessage(PTHREADINFO pti, MessageQueue = pti->MessageQueue; - if (dwQEvent) - { - ERR("Post Msg; System Qeued Event Message!\n"); - InsertHeadList(&pti->PostedMessagesListHead, &Message->ListEntry); - } - else if (!HardwareMessage) + if (!HardwareMessage) { InsertTailList(&pti->PostedMessagesListHead, &Message->ListEntry); } @@ -2122,8 +2075,7 @@ MsqCleanupThreadMsgs(PTHREADINFO pti) while (!IsListEmpty(&pti->PostedMessagesListHead)) { CurrentEntry = RemoveHeadList(&pti->PostedMessagesListHead); - CurrentMessage = CONTAINING_RECORD(CurrentEntry, USER_MESSAGE, - ListEntry); + CurrentMessage = CONTAINING_RECORD(CurrentEntry, USER_MESSAGE, ListEntry); MsqDestroyMessage(CurrentMessage); } @@ -2131,8 +2083,7 @@ MsqCleanupThreadMsgs(PTHREADINFO pti) while (!IsListEmpty(&pti->SentMessagesListHead)) { CurrentEntry = RemoveHeadList(&pti->SentMessagesListHead); - CurrentSentMessage = CONTAINING_RECORD(CurrentEntry, USER_SENT_MESSAGE, - ListEntry); + CurrentSentMessage = CONTAINING_RECORD(CurrentEntry, USER_SENT_MESSAGE, ListEntry); TRACE("Notify the sender and remove a message from the queue that had not been dispatched\n"); /* Only if the message has a sender was the message in the DispatchingList */ @@ -2163,8 +2114,7 @@ MsqCleanupThreadMsgs(PTHREADINFO pti) while (!IsListEmpty(&pti->LocalDispatchingMessagesHead)) { CurrentEntry = RemoveHeadList(&pti->LocalDispatchingMessagesHead); - CurrentSentMessage = CONTAINING_RECORD(CurrentEntry, USER_SENT_MESSAGE, - ListEntry); + CurrentSentMessage = CONTAINING_RECORD(CurrentEntry, USER_SENT_MESSAGE, ListEntry); /* remove the message from the dispatching list */ if(CurrentSentMessage->DispatchingListEntry.Flink != NULL) @@ -2194,8 +2144,7 @@ MsqCleanupThreadMsgs(PTHREADINFO pti) while (! IsListEmpty(&pti->DispatchingMessagesHead)) { CurrentEntry = RemoveHeadList(&pti->DispatchingMessagesHead); - CurrentSentMessage = CONTAINING_RECORD(CurrentEntry, USER_SENT_MESSAGE, - DispatchingListEntry); + CurrentSentMessage = CONTAINING_RECORD(CurrentEntry, USER_SENT_MESSAGE, DispatchingListEntry); CurrentSentMessage->CompletionEvent = NULL; CurrentSentMessage->Result = NULL;