Fix HttpConnectionPool timers race on disposal (#72615)

Access _authorityExpireTimer and _altSvcBlocklistTimerCancellation under lock and check for disposed. Fixes #66782.
dotnet · Jul 22, 2022 · d5d9d52 · d5d9d52
1 parent 20dd6ea
commit d5d9d52
Show file tree

Hide file tree

Showing 2 changed files with 38 additions and 13 deletions.
diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPool.cs b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPool.cs
@@ -1136,10 +1136,13 @@ internal void HandleAltSvc(IEnumerable<string> altSvcHeaderValues, TimeSpan? res
                     // 'clear' should be the only value present.
                     if (value == AltSvcHeaderValue.Clear)
                     {
-                        ExpireAltSvcAuthority();
-                        Debug.Assert(_authorityExpireTimer != null);
-                        _authorityExpireTimer.Change(Timeout.Infinite, Timeout.Infinite);
-                        break;
+                        lock (SyncObj)
+                        {
+                            ExpireAltSvcAuthority();
+                            Debug.Assert(_authorityExpireTimer != null || _disposed);
+                            _authorityExpireTimer?.Change(Timeout.Infinite, Timeout.Infinite);
+                            break;
+                        }
                     }
 
                     if (nextAuthority == null && value != null && value.AlpnProtocolName == "h3")
@@ -1181,6 +1184,12 @@ internal void HandleAltSvc(IEnumerable<string> altSvcHeaderValues, TimeSpan? res
 
                 lock (SyncObj)
                 {
+                    if (_disposed)
+                    {
+                        // avoid creating or touching _authorityExpireTimer after disposal
+                        return;
+                    }
+
                     if (_authorityExpireTimer == null)
                     {
                         var thisRef = new WeakReference<HttpConnectionPool>(this);
@@ -1274,6 +1283,12 @@ internal void BlocklistAuthority(HttpAuthority badAuthority)
             {
                 lock (SyncObj)
                 {
+                    if (_disposed)
+                    {
+                        // avoid creating _altSvcBlocklistTimerCancellation after disposal
+                        return;
+                    }
+
                     altSvcBlocklist = _altSvcBlocklist;
                     if (altSvcBlocklist == null)
                     {
@@ -1297,36 +1312,46 @@ internal void BlocklistAuthority(HttpAuthority badAuthority)
                 }
             }
 
+            CancellationToken altSvcBlocklistTimerCt;
+
             lock (SyncObj)
             {
+                if (_disposed)
+                {
+                    // avoid touching _authorityExpireTimer and _altSvcBlocklistTimerCancellation after disposal
+                    return;
+                }
+
                 if (_http3Authority == badAuthority)
                 {
                     ExpireAltSvcAuthority();
                     Debug.Assert(_authorityExpireTimer != null);
                     _authorityExpireTimer.Change(Timeout.Infinite, Timeout.Infinite);
                 }
+
+                Debug.Assert(_altSvcBlocklistTimerCancellation != null);
+                altSvcBlocklistTimerCt = _altSvcBlocklistTimerCancellation.Token;
             }
 
-            Debug.Assert(_altSvcBlocklistTimerCancellation != null);
             if (added)
             {
-                _ = Task.Delay(AltSvcBlocklistTimeoutInMilliseconds)
+                _ = Task.Delay(AltSvcBlocklistTimeoutInMilliseconds, altSvcBlocklistTimerCt)
                     .ContinueWith(t =>
                     {
                         lock (altSvcBlocklist)
                         {
                             altSvcBlocklist.Remove(badAuthority);
                         }
-                    }, _altSvcBlocklistTimerCancellation.Token, TaskContinuationOptions.ExecuteSynchronously, TaskScheduler.Default);
+                    }, altSvcBlocklistTimerCt, TaskContinuationOptions.ExecuteSynchronously, TaskScheduler.Default);
             }
 
             if (disabled)
             {
-                _ = Task.Delay(AltSvcBlocklistTimeoutInMilliseconds)
+                _ = Task.Delay(AltSvcBlocklistTimeoutInMilliseconds, altSvcBlocklistTimerCt)
                     .ContinueWith(t =>
                     {
                         _altSvcEnabled = true;
-                    }, _altSvcBlocklistTimerCancellation.Token, TaskContinuationOptions.ExecuteSynchronously, TaskScheduler.Default);
+                    }, altSvcBlocklistTimerCt, TaskContinuationOptions.ExecuteSynchronously, TaskScheduler.Default);
             }
         }
 
@@ -1337,8 +1362,8 @@ public void OnNetworkChanged()
                 if (_http3Authority != null && _persistAuthority == false)
                 {
                     ExpireAltSvcAuthority();
-                    Debug.Assert(_authorityExpireTimer != null);
-                    _authorityExpireTimer.Change(Timeout.Infinite, Timeout.Infinite);
+                    Debug.Assert(_authorityExpireTimer != null || _disposed);
+                    _authorityExpireTimer?.Change(Timeout.Infinite, Timeout.Infinite);
                 }
             }
         }

diff --git a/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientTest.cs b/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientTest.cs
@@ -820,7 +820,7 @@ public async Task ConnectTimeout_NotWrappedInMultipleTimeoutExceptions()
             Assert.Contains("ConnectTimeout", connectTimeoutException.Message);
 
             Assert.Null(connectTimeoutException.InnerException);
-            Assert.DoesNotContain("42", e.ToString());
+            Assert.DoesNotContain("HttpClient.Timeout", e.ToString());
         }
 
         [Fact]
@@ -836,7 +836,7 @@ public async Task UnknownOperationCanceledException_NotWrappedInATimeoutExceptio
 
             Assert.Null(e.InnerException);
             Assert.Equal("Foo", e.Message);
-            Assert.DoesNotContain("42", e.ToString());
+            Assert.DoesNotContain("HttpClient.Timeout", e.ToString());
         }
 
         [Fact]