From 3c2e989c61c7466bc6d061e03277dbbd8a7e54e9 Mon Sep 17 00:00:00 2001 From: Stephen Toub Date: Wed, 20 Feb 2019 06:54:11 -0500 Subject: [PATCH] Fix race condition in ManualResetValueTaskSourceCore (#22711) * Fix race condition in ManualResetValueTaskSourceCore ManualResetValueTaskSourceCore.GetStatus is used from ValueTaskAwaiter.IsCompleted. If GetStatus returns Success too early, then IsCompleted will also return true too early, which will result in GetResult being called too early. This doesn't happen today when an MRVTSC is used sequentially. But if an instance is pooled by the object its stored on getting put back into a pool as part of a call to the wrapper's GetResult, then we can end up in the following situation: - Thread 1 starts an await on an instance containing an MRVTSC. It calls IsCompleted. - Thread 2 starts to complete that instance, getting as far as calling SignalCompletion and setting _completed to true but not yet setting _continuation. - Thread 1 sees _completed == true and returns true from IsCompleted. It calls GetResult, and the wrapper extracts the result from the instance, resets it, and puts it back into the pool. - Thread 3 takes the object out of the pool and starts using it. - Thread 2 continues SignalCompletion on that instance, and sets _continuation to the sentinel. - Now Thread 3's instance's _continuation is the sentinel when it should be null. If it calls SignalCompletion, it'll erroneously find that _continuation is not null and will queue _continuation/_continuationState for execution, resulting in the sentinel getting executed. If it calls OnCompleted, it'll find that the _continuation is not null, and will queue the supplied continuation/state to execute immediately even though the operation may not yet actually be done. The fix is simply to check not just _completed but also _continuation. The operation is considered pending if either _completed is false, meaning SignalCompletion has not yet been called, or if _continuation is still null, meaning it's neither been set to the supplied delegate nor to the sentinel. We can't just rely on _completed for the above outlined reasons, and we can't just rely on _continuation because it can be non-null if OnCompleted was called to hook up a callback (if we only cared about the await pattern, then we could just check _continuation and wouldn't need _completed, but we also need to support non-await access). * Address PR feedback --- .../System/Threading/Tasks/Sources/ManualResetValueTaskSourceCore.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/System.Private.CoreLib/shared/System/Threading/Tasks/Sources/ManualResetValueTaskSourceCore.cs b/src/System.Private.CoreLib/shared/System/Threading/Tasks/Sources/ManualResetValueTaskSourceCore.cs index b9e58d9..264fe92 100644 --- a/src/System.Private.CoreLib/shared/System/Threading/Tasks/Sources/ManualResetValueTaskSourceCore.cs +++ b/src/System.Private.CoreLib/shared/System/Threading/Tasks/Sources/ManualResetValueTaskSourceCore.cs @@ -80,7 +80,7 @@ namespace System.Threading.Tasks.Sources { ValidateToken(token); return - !_completed ? ValueTaskSourceStatus.Pending : + _continuation == null || !_completed ? ValueTaskSourceStatus.Pending : _error == null ? ValueTaskSourceStatus.Succeeded : _error.SourceException is OperationCanceledException ? ValueTaskSourceStatus.Canceled : ValueTaskSourceStatus.Faulted; -- 2.7.4