[PATCH] ntdll: Avoid more race conditions in RtlDeregisterWaitEx.

[Stefan Dösinger]

af35aada9b078f8dc71dcc85e505da8eee4571da left some issues unfixed.
Instead of running the callback and returning a retval that indicates
the callback is not running, we sometimes wait for it when we are not
supposed to.

Signed-off-by: Stefan Dösinger

---

WGC calls RtlDeregisterWaitEx(wait, NULL) while holding a critical
section that the wait callback will try to acquire. We must not wait for
the callback to finish with a NULL wait event under any circumstances,
otherwise WGC will deadlock.

To make matters worse, WGC manages to fairly reliably hit the racy case
where the wait is signalled at the same time it is cancelled. If
RtlDeregisterWaitEx thinks the callback is not running it will wait for
the worker thread to finish before returning STATUS_SUCCESS. We can't
return STATUS_PENDING if the callback is not running (see tests) and we
must not return STATUS_SUCCESS and execute the callback. So make sure
that after we decide the callback is not running the callback will not
start to run.
---
 dlls/ntdll/threadpool.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/dlls/ntdll/threadpool.c b/dlls/ntdll/threadpool.c
index 1b0546a7816..bf7449cdfa6 100644
--- a/dlls/ntdll/threadpool.c
+++ b/dlls/ntdll/threadpool.c
@@ -81,7 +81,7 @@ struct wait_work_item
     ULONG Flags;
     HANDLE CompletionEvent;
     LONG DeleteCount;
-    BOOLEAN CallbackInProgress;
+    int CallbackInProgress;
 };

struct timer_queue;
@@ -530,9 +530,14 @@ static DWORD CALLBACK wait_thread_proc(LPVOID Arg)
                     wait_work_item->Context );
                 TimerOrWaitFired = TRUE;
             }
-            wait_work_item->CallbackInProgress = TRUE;
+            interlocked_xchg( &wait_work_item->CallbackInProgress, TRUE );
+            if (wait_work_item->CompletionEvent)
+            {
+                TRACE( "Work has been canceled.\n" );
+                break;
+            }
             wait_work_item->Callback( wait_work_item->Context, TimerOrWaitFired );
-            wait_work_item->CallbackInProgress = FALSE;
+            interlocked_xchg( &wait_work_item->CallbackInProgress, FALSE );

if (wait_work_item->Flags & WT_EXECUTEONLYONCE)
                 break;
@@ -546,7 +551,8 @@ static DWORD CALLBACK wait_thread_proc(LPVOID Arg)
     {
         completion_event = wait_work_item->CompletionEvent;
         delete_wait_work_item( wait_work_item );
-        if (completion_event) NtSetEvent( completion_event, NULL );
+        if (completion_event && completion_event != INVALID_HANDLE_VALUE)
+            NtSetEvent( completion_event, NULL );
     }

return 0;
@@ -637,14 +643,16 @@ NTSTATUS WINAPI RtlDeregisterWaitEx(HANDLE WaitHandle, HANDLE CompletionEvent)
     struct wait_work_item *wait_work_item = WaitHandle;
     NTSTATUS status;
     HANDLE LocalEvent = NULL;
-    BOOLEAN CallbackInProgress;
+    int CallbackInProgress;

TRACE( "(%p %p)\n", WaitHandle, CompletionEvent );

if (WaitHandle == NULL)
         return STATUS_INVALID_HANDLE;

+    interlocked_xchg_ptr( &wait_work_item->CompletionEvent, INVALID_HANDLE_VALUE );
     CallbackInProgress = wait_work_item->CallbackInProgress;
+    TRACE( "callback in progress %u\n", CallbackInProgress );
     if (CompletionEvent == INVALID_HANDLE_VALUE || !CallbackInProgress)
     {
         status = NtCreateEvent( &LocalEvent, EVENT_ALL_ACCESS, NULL, NotificationEvent, FALSE );
@@ -666,6 +674,7 @@ NTSTATUS WINAPI RtlDeregisterWaitEx(HANDLE WaitHandle, HANDLE CompletionEvent)
     }
     else if (LocalEvent)
     {
+        TRACE( "Waiting for completion event\n" );
         NtWaitForSingleObject( LocalEvent, FALSE, NULL );
         status = STATUS_SUCCESS;
     }