[PATCH 0/2] MR3929: ntdll: Remove entries from queue in RtlWakeAddressAll().

[Paul Gofman (＠gofman)]

This avoids one case of spurious wakeup of WaitOnAddress (which could be triggered by prior race on consequent WakeAddressAll and a thread trying to wake on that, leaving the waking thread in an alerted state). And fixes Resident Evil games randomly crashing due to unhandled spurious SleepConditionVariableCS wakeups.

[PATCH 1/2] ntdll: Remove entries from queue in RtlWakeAddressAll().

[Paul Gofman]

From: Paul Gofman

---
 dlls/ntdll/sync.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/dlls/ntdll/sync.c b/dlls/ntdll/sync.c
index fa64917029a..1e2019e52c4 100644
--- a/dlls/ntdll/sync.c
+++ b/dlls/ntdll/sync.c
@@ -925,8 +925,8 @@ NTSTATUS WINAPI RtlWaitOnAddress( const void *addr, const void *cmp, SIZE_T size
 void WINAPI RtlWakeAddressAll( const void *addr )
 {
     struct futex_queue *queue = get_futex_queue( addr );
+    struct futex_entry *entry, *next;
     unsigned int count = 0, i;
-    struct futex_entry *entry;
     DWORD tids[256];

TRACE("%p\n", addr);
@@ -938,10 +938,12 @@ void WINAPI RtlWakeAddressAll( const void *addr )
     if (!queue->queue.next)
         list_init(&queue->queue);

-    LIST_FOR_EACH_ENTRY( entry, &queue->queue, struct futex_entry, entry )
+    LIST_FOR_EACH_ENTRY_SAFE( entry, next, &queue->queue, struct futex_entry, entry )
     {
         if (entry->addr == addr)
         {
+            entry->addr = NULL;
+            list_remove( &entry->entry );
             /* Try to buffer wakes, so that we don't make a system call while
              * holding a spinlock. */
             if (count < ARRAY_SIZE(tids))

[PATCH 2/2] ntdll: Pre-check entry->addr before taking a spin lock in RtlWaitOnAddress().

[Paul Gofman]

From: Paul Gofman

---
 dlls/ntdll/sync.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/dlls/ntdll/sync.c b/dlls/ntdll/sync.c
index 1e2019e52c4..3b1e4a721e2 100644
--- a/dlls/ntdll/sync.c
+++ b/dlls/ntdll/sync.c
@@ -907,11 +907,14 @@ NTSTATUS WINAPI RtlWaitOnAddress( const void *addr, const void *cmp, SIZE_T size

ret = NtWaitForAlertByThreadId( NULL, timeout );

-    spin_lock( &queue->lock );
-    /* We may have already been removed by a call to RtlWakeAddressSingle(). */
+    /* We may have already been removed by a call to RtlWakeAddressSingle() or RtlWakeAddressAll(). */
     if (entry.addr)
-        list_remove( &entry.entry );
-    spin_unlock( &queue->lock );
+    {
+        spin_lock( &queue->lock );
+        if (entry.addr)
+            list_remove( &entry.entry );
+        spin_unlock( &queue->lock );
+    }

TRACE("returning %#lx\n", ret);

[Paul Gofman (＠gofman)]

I made a small perf text just in case (attached), which of course can't catch the difference of RtlWakeAddressAll() time due to pre-existing much greater variance in the execution time. The change is very subtle perf wise.

Unrelated here, but the test shows some things interesting by itsef. E. g., the time of RtlWakeAddressAll with 6 threads is more than 2 times bigger than on Windows, and as far as my further profiling went, the vast majority of time is spent in Linux futex_wake() (not even in multiple NtAlerthThreadById calls through dispacther, hacking those in one call instead of 6 improves things aon average a bit but not significantly).

[waitonaddr.c](/uploads/f4ac4129f4fb5ccfb0d364313936c90c/waitonaddr.c)

[Paul Gofman (＠gofman)]

As an unrelated note (that issue is the same before and after these patches), the fact we add a stack variable to the queue list looks not great: if a thread gets force terminated the queue will be left with bad data in the list. As unfortunate as it sounds, we probably have to allocate queue entry elsewise using some cache?

[Zebediah Figura (＠zfigura)]

> As an unrelated note (that issue is the same before and after these patches), the fact we add a stack variable to the queue list looks not great: if a thread gets force terminated the queue will be left with bad data in the list. As unfortunate as it sounds, we probably have to allocate queue entry elsewise using some cache?

Anyone using TerminateThread() is asking for trouble. Unless we specifically run into an application that expects to be able to terminate a thread mid-wait, I don't think there's anything that needs to be done here.

[Paul Gofman (＠gofman)]

> Unless we specifically run into an application that expects to be able to terminate a thread mid-wait, I don't think there's anything that needs to be done here.

Yeah, at least such an issue will be relatively easy to spot. Unlike the one fixed by this MR.

[PATCH 0/2] MR3929: ntdll: Remove entries from queue in RtlWakeAddressAll(). - approved

[Elizabeth Figura (＠zfigura)]

This merge request was approved by Elizabeth Figura.

[Elizabeth Figura (＠zfigura)]

Sorry this never got approved... not sure why I let it fall on the floor.