[ros-diffs] [jimtabor] 66309: [NtUser] - This fixes use after free linking in the message system. See CORE-9173. Dedicated to Thomas Faber.

Author: jimtabor
Date: Mon Feb 16 03:16:01 2015
New Revision: 66309

URL: http://svn.reactos.org/svn/reactos?rev=66309&view=rev
Log:
[NtUser]
- This fixes use after free linking in the message system. See CORE-9173. Dedicated to
Thomas Faber.

Modified:
    trunk/reactos/win32ss/user/ntuser/msgqueue.c

Modified: trunk/reactos/win32ss/user/ntuser/msgqueue.c
URL:
http://svn.reactos.org/svn/reactos/trunk/reactos/win32ss/user/ntuser/msgque…
==============================================================================
--- trunk/reactos/win32ss/user/ntuser/msgqueue.c	[iso-8859-1] (original)
+++ trunk/reactos/win32ss/user/ntuser/msgqueue.c	[iso-8859-1] Mon Feb 16 03:16:01 2015
@@ -789,8 +789,7 @@
 
    /* 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 @@
    }
 
    /* 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 @@
    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 @@
                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 @@
             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,29 +1250,9 @@
                   Message->CompletionEvent = NULL;
                   Message->Result = NULL;
                   RemoveEntryList(&Message->ListEntry);
+                  RemoveEntryList(&Message->DispatchingListEntry);
                   ClearMsgBitsMask(ptirec, Message->QS_Flags);
                   ExFreePoolWithTag(Message, TAG_USRMSG);
-                  break;
-               }
-               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;
@@ -1375,12 +1333,7 @@
 
    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 @@
    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 @@
    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 @@
    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 @@
    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;