Fix race when signaling waitable objects in managed implementation

[uweigand]

• TrySignalToSatisfyWait may invalidate multiple elements of the
  WaitedListNode linked list, if the same object occurs multiple
  times in a single WaitForMultipleObjects call.

This race showed up in sporading failures in System.Threading.Tasks.Tests, indicated by an assertion failure in TrySignalToSatisfyWait when called from SignalManualResetEvent. The problem is that in this loop:

                for (ThreadWaitInfo.WaitedListNode? waiterNode = _waitersHead, nextWaiterNode;
                    waiterNode != null;
                    waiterNode = nextWaiterNode)
                {
                    // Signaling a waiter will unregister the waiter node, so keep the next node before trying
                    nextWaiterNode = waiterNode.Next;

                    waiterNode.WaitInfo.TrySignalToSatisfyWait(waiterNode, isAbandonedMutex: false);
                }

the TrySignalToSatisfyWait call may modify the linked list we're traversing. The code attempts to address that issue by keeping the next node before the call -- but that next node may itself be modified by the call. This happens only if that next node has the same WaitInfo as the current node, which is normally not the case, but may occur if the same waitable object is used multiple times as argument to a single WaitAny / WaitForMultipleObjects call.

[ghost]

Tagging subscribers to this area: @mangod9
See info in area-owners.md if you want to be subscribed.

Issue Details

---

• TrySignalToSatisfyWait may invalidate multiple elements of the
  WaitedListNode linked list, if the same object occurs multiple
  times in a single WaitForMultipleObjects call.

This race showed up in sporading failures in System.Threading.Tasks.Tests, indicated by an assertion failure in TrySignalToSatisfyWait when called from SignalManualResetEvent. The problem is that in this loop:

                for (ThreadWaitInfo.WaitedListNode? waiterNode = _waitersHead, nextWaiterNode;
                    waiterNode != null;
                    waiterNode = nextWaiterNode)
                {
                    // Signaling a waiter will unregister the waiter node, so keep the next node before trying
                    nextWaiterNode = waiterNode.Next;

                    waiterNode.WaitInfo.TrySignalToSatisfyWait(waiterNode, isAbandonedMutex: false);
                }

the TrySignalToSatisfyWait call may modify the linked list we're traversing. The code attempts to address that issue by keeping the next node before the call -- but that next node may itself be modified by the call. This happens only if that next node has the same WaitInfo as the current node, which is normally not the case, but may occur if the same waitable object is used multiple times as argument to a single WaitAny / WaitForMultipleObjects call.

Author:	uweigand
Assignees:	-
Labels:	area-System.Threading
Milestone:	-

[jkotas]

Is this fixing #52614 ? Can we re-enable the test disabled against this issue?

[uweigand]

Is this fixing #52614 ? Can we re-enable the test disabled against this issue?

From the call chain, it does look like this issue. I can't really test on iOS or tvOS myself, but I guess I could enable the test as part of this PR and see if the CI passes?

[jkotas]

I could enable the test as part of this PR and see if the CI passes?

Yep

[uweigand]

I could enable the test as part of this PR and see if the CI passes?

Yep

OK, let's give it a try.

[jkotas]

Failures are unrelated