From f9e3284cef3bd22b2bb9464f393ace996350784b Mon Sep 17 00:00:00 2001 From: wfurt Date: Sun, 25 Jul 2021 21:49:28 -0700 Subject: [PATCH 1/5] avoid NRE in MsQuicConnection ConnectAsync --- .../Net/Quic/Implementations/MsQuic/MsQuicConnection.cs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicConnection.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicConnection.cs index 7b0579e73c7279..270ccb9c36a728 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicConnection.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicConnection.cs @@ -234,7 +234,7 @@ private static uint HandleEventConnected(State state, ref ConnectionEvent connec private static uint HandleEventShutdownInitiatedByTransport(State state, ref ConnectionEvent connectionEvent) { - if (!state.Connected && state.ConnectTcs != null) + if (!state.Connected && state.ConnectTcs != null && !state.ConnectTcs.Task.IsCompleted) { Debug.Assert(state.Connection != null); state.Connection = null; @@ -681,10 +681,10 @@ private static uint NativeCallbackHandler( NetEventSource.Error(state, $"{state.TraceId} Exception occurred during handling {connectionEvent.Type} connection callback: {ex}"); } - if (state.ConnectTcs != null) + if (state.ConnectTcs != null && !state.ConnectTcs.Task.IsCompleted) { - state.ConnectTcs.SetException(ex); - state.ConnectTcs = null; + // This is opportunistic if we get exception and have ability to propagate it to caller. + state.ConnectTcs.TrySetException(ex); state.Connection = null; } else From 6f3901f1848ff67f45483e10b14b333c9e4e44f7 Mon Sep 17 00:00:00 2001 From: wfurt Date: Mon, 26 Jul 2021 17:36:35 -0700 Subject: [PATCH 2/5] enable disabled test --- .../System.Net.Quic/tests/FunctionalTests/MsQuicTests.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicTests.cs b/src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicTests.cs index a85132ac6c2aa7..475a5f1cff5109 100644 --- a/src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicTests.cs +++ b/src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicTests.cs @@ -157,7 +157,6 @@ public async Task CertificateCallbackThrowPropagates() } [Fact] - [ActiveIssue("https://github.com/dotnet/runtime/issues/56263")] public async Task ConnectWithCertificateCallback() { X509Certificate2 c1 = System.Net.Test.Common.Configuration.Certificates.GetServerCertificate(); From dccf730a6de2ed33118c0d44e38d66eca058e089 Mon Sep 17 00:00:00 2001 From: wfurt Date: Mon, 26 Jul 2021 21:28:49 -0700 Subject: [PATCH 3/5] one more place where we set ConnectTcs to null --- .../System/Net/Quic/Implementations/MsQuic/MsQuicConnection.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicConnection.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicConnection.cs index 270ccb9c36a728..33b68f09169b1c 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicConnection.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicConnection.cs @@ -242,7 +242,6 @@ private static uint HandleEventShutdownInitiatedByTransport(State state, ref Con uint hresult = connectionEvent.Data.ShutdownInitiatedByTransport.Status; Exception ex = QuicExceptionHelpers.CreateExceptionForHResult(hresult, "Connection has been shutdown by transport."); state.ConnectTcs!.SetException(ExceptionDispatchInfo.SetCurrentStackTrace(ex)); - state.ConnectTcs = null; } state.AcceptQueue.Writer.TryComplete(); From 4c5e6de01e65d04762746d72f17b7eb0bc3a0094 Mon Sep 17 00:00:00 2001 From: wfurt Date: Tue, 27 Jul 2021 10:04:30 -0700 Subject: [PATCH 4/5] use local variable --- .../Quic/Implementations/MsQuic/MsQuicConnection.cs | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicConnection.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicConnection.cs index 33b68f09169b1c..bf8b300b5b8e84 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicConnection.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicConnection.cs @@ -227,6 +227,7 @@ private static uint HandleEventConnected(State state, ref ConnectionEvent connec state.Connected = true; state.ConnectTcs!.SetResult(MsQuicStatusCodes.Success); + state.ConnectTcs = null; } return MsQuicStatusCodes.Success; @@ -234,7 +235,7 @@ private static uint HandleEventConnected(State state, ref ConnectionEvent connec private static uint HandleEventShutdownInitiatedByTransport(State state, ref ConnectionEvent connectionEvent) { - if (!state.Connected && state.ConnectTcs != null && !state.ConnectTcs.Task.IsCompleted) + if (!state.Connected && state.ConnectTcs != null) { Debug.Assert(state.Connection != null); state.Connection = null; @@ -242,6 +243,7 @@ private static uint HandleEventShutdownInitiatedByTransport(State state, ref Con uint hresult = connectionEvent.Data.ShutdownInitiatedByTransport.Status; Exception ex = QuicExceptionHelpers.CreateExceptionForHResult(hresult, "Connection has been shutdown by transport."); state.ConnectTcs!.SetException(ExceptionDispatchInfo.SetCurrentStackTrace(ex)); + state.ConnectTcs = null; } state.AcceptQueue.Writer.TryComplete(); @@ -575,7 +577,7 @@ internal override ValueTask ConnectAsync(CancellationToken cancellationToken = d throw new Exception($"Unsupported remote endpoint type '{_remoteEndPoint.GetType()}'."); } - _state.ConnectTcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + var tcs = _state.ConnectTcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); try { @@ -599,7 +601,7 @@ internal override ValueTask ConnectAsync(CancellationToken cancellationToken = d throw; } - return new ValueTask(_state.ConnectTcs.Task); + return new ValueTask(tcs.Task); } private ValueTask ShutdownAsync( @@ -680,11 +682,12 @@ private static uint NativeCallbackHandler( NetEventSource.Error(state, $"{state.TraceId} Exception occurred during handling {connectionEvent.Type} connection callback: {ex}"); } - if (state.ConnectTcs != null && !state.ConnectTcs.Task.IsCompleted) + if (state.ConnectTcs != null) { // This is opportunistic if we get exception and have ability to propagate it to caller. state.ConnectTcs.TrySetException(ex); state.Connection = null; + state.ConnectTcs = null; } else { From 9426974ee047d3fc870b95c2db7d3dab63a3836b Mon Sep 17 00:00:00 2001 From: wfurt Date: Wed, 28 Jul 2021 17:47:33 -0700 Subject: [PATCH 5/5] feedback from review --- .../System/Net/Quic/Implementations/MsQuic/MsQuicConnection.cs | 1 + .../System.Net.Quic/tests/FunctionalTests/MsQuicTests.cs | 1 - 2 files changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicConnection.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicConnection.cs index bf8b300b5b8e84..37998df2e9603b 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicConnection.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicConnection.cs @@ -577,6 +577,7 @@ internal override ValueTask ConnectAsync(CancellationToken cancellationToken = d throw new Exception($"Unsupported remote endpoint type '{_remoteEndPoint.GetType()}'."); } + // We store TCS to local variable to avoid NRE if callbacks finish fast and set _state.ConnectTcs to null. var tcs = _state.ConnectTcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); try diff --git a/src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicTests.cs b/src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicTests.cs index a8782bb1231537..0adb63860c4f3c 100644 --- a/src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicTests.cs +++ b/src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicTests.cs @@ -234,7 +234,6 @@ public async Task ConnectWithCertificateCallback() } [Fact] - [ActiveIssue("https://github.com/dotnet/runtime/issues/56454")] public async Task ConnectWithCertificateForDifferentName_Throws() { (X509Certificate2 certificate, _) = System.Net.Security.Tests.TestHelper.GenerateCertificates("localhost");