From: Stephen Toub <stoub@microsoft.com>
Date: Mon, 2 Dec 2019 21:45:40 +0000 (-0500)
Subject: Fix CancellationTokenRegistration.Unregister race condition (#309)
X-Git-Tag: submit/tizen/20210909.063632~10844
X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=d4ef0d16186bbfba2fd078ba235f8759fd1128d3;p=platform%2Fupstream%2Fdotnet%2Fruntime.git

Fix CancellationTokenRegistration.Unregister race condition (#309)

There's a race condition that exists if multiple threads are accessing the same CancellationTokenRegistration field, with one Unregistering and zero'ing out the field while another Unregisters.  We shouldn't ever be in a situation where we have a non-null node and a 0 id, but due to struct tearing we can end up in that exact situation inside of Unregister (if the thread zero'ing out the struct succeeded in zero'ing out the id but we still read a valid node).  If the node was then already unregistered, it will contain 0 for its id, in which case we'll see that the ids match and assume that means the node is still in the list.  At that point we proceed to dereference nodes in the list and null ref.

The fix is simple: rather than just asserting that we'll never get 0, explicitly check for 0 and return if it is.
---

diff --git a/src/libraries/System.Private.CoreLib/src/System/Threading/CancellationTokenSource.cs b/src/libraries/System.Private.CoreLib/src/System/Threading/CancellationTokenSource.cs
index 4577591..dd31213 100644
--- a/src/libraries/System.Private.CoreLib/src/System/Threading/CancellationTokenSource.cs
+++ b/src/libraries/System.Private.CoreLib/src/System/Threading/CancellationTokenSource.cs
@@ -524,6 +524,7 @@ namespace System.Threading
                 {
                     // Assign the next available unique ID.
                     id = partition.NextAvailableId++;
+                    Debug.Assert(id != 0, "IDs should never be the reserved value 0.");
 
                     // Get a node, from the free list if possible or else a new one.
                     node = partition.FreeNodeList;
@@ -939,9 +940,16 @@ namespace System.Threading
 
             internal bool Unregister(long id, CallbackNode node)
             {
-                Debug.Assert(id != 0, "Expected non-zero id");
                 Debug.Assert(node != null, "Expected non-null node");
 
+                if (id == 0)
+                {
+                    // In general, we won't get 0 passed in here.  However, race conditions between threads
+                    // Unregistering and also zero'ing out the CancellationTokenRegistration could cause 0
+                    // to be passed in here, in which case there's nothing to do. 0 is never a valid id.
+                    return false;
+                }
+
                 bool lockTaken = false;
                 Lock.Enter(ref lockTaken);
                 try