wrap exceptions from callbacks in QuicError.CallbackError (#88614)

wfurt · web-flow · commit bf78b40fa91c · 2023-07-17T16:08:01.000-06:00
* wrap exceptions from callbacks in QuicError.CallbackError

* feedback
diff --git a/src/libraries/System.Net.Quic/src/Resources/Strings.resx b/src/libraries/System.Net.Quic/src/Resources/Strings.resx
@@ -195,21 +195,12 @@
   <data name="net_quic_accept_not_allowed" xml:space="preserve">
     <value>QuicConnection is configured to not accept any streams.</value>
   </data>
-  <data name="net_quic_address_in_use" xml:space="preserve">
-    <value>The local address is already in use.</value>
-  </data>
-  <data name="net_quic_host_unreachable" xml:space="preserve">
-    <value>The server is currently unreachable.</value>
-  </data>
   <data name="net_quic_connection_refused" xml:space="preserve">
     <value>The server refused the connection.</value>
   </data>
   <data name="net_quic_protocol_error" xml:space="preserve">
     <value>A QUIC protocol error was encountered</value>
   </data>
-  <data name="net_quic_alpn_in_use" xml:space="preserve">
-    <value>Another QUIC listener is already listening on one of the requested application protocols on the same port.</value>
-  </data>
   <data name="net_quic_ver_neg_error" xml:space="preserve">
     <value>A version negotiation error was encountered.</value>
   </data>
@@ -219,9 +210,6 @@
   <data name="net_quic_connection_idle" xml:space="preserve">
     <value>The connection timed out from inactivity.</value>
   </data>
-  <data name="net_quic_invalid_address" xml:space="preserve">
-    <value>Binding to socket failed, likely caused by a family mismatch between local and remote address.</value>
-  </data>
   <data name="net_quic_auth" xml:space="preserve">
     <value>Authentication failed: {0}.</value>
   </data>
@@ -239,5 +227,8 @@
   <data name="net_InvalidSocketAddressSize" xml:space="preserve">
     <value>The supplied {0} is an invalid size for the {1} end point.</value>
   </data>
+  <data name="net_quic_callback_error" xml:space="preserve">
+    <value>User configured callback failed.</value>
+  </data>
 </root>
 
diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.SslConnectionOptions.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.SslConnectionOptions.cs
@@ -68,6 +68,7 @@ public unsafe int ValidateCertificate(QUIC_BUFFER* certificatePtr, QUIC_BUFFER*
             SslPolicyErrors sslPolicyErrors = SslPolicyErrors.None;
             IntPtr certificateBuffer = 0;
             int certificateLength = 0;
+            bool wrapException = false;
 
             X509Chain? chain = null;
             X509Certificate2? result = null;
@@ -130,8 +131,10 @@ public unsafe int ValidateCertificate(QUIC_BUFFER* certificatePtr, QUIC_BUFFER*
                 int status = QUIC_STATUS_SUCCESS;
                 if (_validationCallback is not null)
                 {
+                    wrapException = true;
                     if (!_validationCallback(_connection, result, chain, sslPolicyErrors))
                     {
+                        wrapException = false;
                         if (_isClient)
                         {
                             throw new AuthenticationException(SR.net_quic_cert_custom_validation);
@@ -153,9 +156,14 @@ public unsafe int ValidateCertificate(QUIC_BUFFER* certificatePtr, QUIC_BUFFER*
                 certificate = result;
                 return status;
             }
-            catch
+            catch (Exception ex)
             {
                 result?.Dispose();
+                if (wrapException)
+                {
+                    throw new QuicException(QuicError.CallbackError, null, SR.net_quic_callback_error, ex);
+                }
+
                 throw;
             }
             finally
diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicListener.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicListener.cs
@@ -209,13 +209,16 @@ public async ValueTask<QuicConnection> AcceptConnectionAsync(CancellationToken c
     /// <param name="clientHello">The TLS ClientHello data.</param>
     private async void StartConnectionHandshake(QuicConnection connection, SslClientHelloInfo clientHello)
     {
+        bool wrapException = false;
         CancellationToken cancellationToken = default;
         try
         {
             using CancellationTokenSource linkedCts = CancellationTokenSource.CreateLinkedTokenSource(_disposeCts.Token);
             linkedCts.CancelAfter(QuicDefaults.HandshakeTimeout);
             cancellationToken = linkedCts.Token;
+            wrapException = true;
             QuicServerConnectionOptions options = await _connectionOptionsCallback(connection, clientHello, cancellationToken).ConfigureAwait(false);
+            wrapException = false;
             options.Validate(nameof(options)); // Validate and fill in defaults for the options.
             await connection.FinishHandshakeAsync(options, clientHello.ServerName, cancellationToken).ConfigureAwait(false);
             if (!_acceptQueue.Writer.TryWrite(connection))
@@ -267,7 +270,10 @@ private async void StartConnectionHandshake(QuicConnection connection, SslClient
             }
 
             await connection.DisposeAsync().ConfigureAwait(false);
-            if (!_acceptQueue.Writer.TryWrite(ex))
+            if (!_acceptQueue.Writer.TryWrite(
+                    wrapException ?
+                        ExceptionDispatchInfo.SetCurrentStackTrace(new QuicException(QuicError.CallbackError, null, SR.net_quic_callback_error, ex)) :
+                        ex))
             {
                 // Channel has been closed, connection is already disposed, do nothing.
             }
diff --git a/src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicTests.cs b/src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicTests.cs
@@ -384,7 +384,8 @@ public async Task CertificateCallbackThrowPropagates()
 
             clientOptions.ClientAuthenticationOptions.TargetHost = "foobar1";
 
-            await Assert.ThrowsAsync<ArithmeticException>(() => CreateQuicConnection(clientOptions).AsTask());
+            Exception exception = await AssertThrowsQuicExceptionAsync(QuicError.CallbackError, async () => await CreateQuicConnection(clientOptions));
+            Assert.True(exception.InnerException is ArithmeticException);
             await Assert.ThrowsAsync<AuthenticationException>(async () => await listener.AcceptConnectionAsync());
 
             // Make sure the listener is still usable and there is no lingering bad connection
diff --git a/src/libraries/System.Net.Quic/tests/FunctionalTests/QuicListenerTests.cs b/src/libraries/System.Net.Quic/tests/FunctionalTests/QuicListenerTests.cs
@@ -96,8 +96,42 @@ public async Task AcceptConnectionAsync_ThrowingOptionsCallback_Throws(bool useF
             await using QuicListener listener = await CreateQuicListener(listenerOptions);
 
             ValueTask<QuicConnection> connectTask = CreateQuicConnection(listener.LocalEndPoint);
-            Exception exception = await Assert.ThrowsAsync<Exception>(async () => await listener.AcceptConnectionAsync());
-            Assert.Equal(expectedMessage, exception.Message);
+
+            Exception exception = await AssertThrowsQuicExceptionAsync(QuicError.CallbackError, async () => await listener.AcceptConnectionAsync());
+            Assert.NotNull(exception.InnerException);
+            Assert.Equal(expectedMessage, exception.InnerException.Message);
+            await Assert.ThrowsAsync<AuthenticationException>(() => connectTask.AsTask());
+        }
+
+        [Fact]
+        public async Task AcceptConnectionAsync_ThrowingCallbackOde_KeepRunning()
+        {
+            bool firstRun = true;
+
+            QuicListenerOptions listenerOptions = CreateQuicListenerOptions();
+            // Throw an exception, which should throw the same from accept.
+            listenerOptions.ConnectionOptionsCallback = (_, _, _) =>
+            {
+                if (firstRun)
+                {
+                    firstRun = false;
+                    throw new ObjectDisposedException("failed");
+                }
+
+                return ValueTask.FromResult(CreateQuicServerOptions());
+            };
+            await using QuicListener listener = await CreateQuicListener(listenerOptions);
+
+            ValueTask<QuicConnection> connectTask = CreateQuicConnection(listener.LocalEndPoint);
+
+            Exception exception = await AssertThrowsQuicExceptionAsync(QuicError.CallbackError, async () => await listener.AcceptConnectionAsync());
+            Assert.True(exception.InnerException is ObjectDisposedException);
+            await Assert.ThrowsAsync<AuthenticationException>(() => connectTask.AsTask());
+
+            // Throwing ODE in callback should keep Listener running
+            connectTask = CreateQuicConnection(listener.LocalEndPoint);
+            await using QuicConnection serverConnection = await listener.AcceptConnectionAsync();
+            await using QuicConnection clientConnection = await connectTask;
         }
 
         [Theory]

Original file line number	Diff line number	Diff line change
`@@ -68,6 +68,7 @@ public unsafe int ValidateCertificate(QUIC_BUFFER* certificatePtr, QUIC_BUFFER*`
`68`	`68`	`SslPolicyErrors sslPolicyErrors = SslPolicyErrors.None;`
`69`	`69`	`IntPtr certificateBuffer = 0;`
`70`	`70`	`int certificateLength = 0;`
	`71`	`+ bool wrapException = false;`
`71`	`72`
`72`	`73`	`X509Chain? chain = null;`
`73`	`74`	`X509Certificate2? result = null;`
`@@ -130,8 +131,10 @@ public unsafe int ValidateCertificate(QUIC_BUFFER* certificatePtr, QUIC_BUFFER*`
`130`	`131`	`int status = QUIC_STATUS_SUCCESS;`
`131`	`132`	`if (_validationCallback is not null)`
`132`	`133`	`{`
	`134`	`+ wrapException = true;`
`133`	`135`	`if (!_validationCallback(_connection, result, chain, sslPolicyErrors))`
`134`	`136`	`{`
	`137`	`+ wrapException = false;`
`135`	`138`	`if (_isClient)`
`136`	`139`	`{`
`137`	`140`	`throw new AuthenticationException(SR.net_quic_cert_custom_validation);`
`@@ -153,9 +156,14 @@ public unsafe int ValidateCertificate(QUIC_BUFFER* certificatePtr, QUIC_BUFFER*`
`153`	`156`	`certificate = result;`
`154`	`157`	`return status;`
`155`	`158`	`}`
`156`		`- catch`
	`159`	`+ catch (Exception ex)`
`157`	`160`	`{`
`158`	`161`	`result?.Dispose();`
	`162`	`+ if (wrapException)`
	`163`	`+ {`
	`164`	`+ throw new QuicException(QuicError.CallbackError, null, SR.net_quic_callback_error, ex);`
	`165`	`+ }`
	`166`	`+`
`159`	`167`	`throw;`
`160`	`168`	`}`
`161`	`169`	`finally`
Original file line number	Diff line number	Diff line change
`@@ -209,13 +209,16 @@ public async ValueTask<QuicConnection> AcceptConnectionAsync(CancellationToken c`
`209`	`209`	`/// <param name="clientHello">The TLS ClientHello data.</param>`
`210`	`210`	`private async void StartConnectionHandshake(QuicConnection connection, SslClientHelloInfo clientHello)`
`211`	`211`	`{`
	`212`	`+ bool wrapException = false;`
`212`	`213`	`CancellationToken cancellationToken = default;`
`213`	`214`	`try`
`214`	`215`	`{`
`215`	`216`	`using CancellationTokenSource linkedCts = CancellationTokenSource.CreateLinkedTokenSource(_disposeCts.Token);`
`216`	`217`	`linkedCts.CancelAfter(QuicDefaults.HandshakeTimeout);`
`217`	`218`	`cancellationToken = linkedCts.Token;`
	`219`	`+ wrapException = true;`
`218`	`220`	`QuicServerConnectionOptions options = await _connectionOptionsCallback(connection, clientHello, cancellationToken).ConfigureAwait(false);`
	`221`	`+ wrapException = false;`
`219`	`222`	`options.Validate(nameof(options)); // Validate and fill in defaults for the options.`
`220`	`223`	`await connection.FinishHandshakeAsync(options, clientHello.ServerName, cancellationToken).ConfigureAwait(false);`
`221`	`224`	`if (!_acceptQueue.Writer.TryWrite(connection))`
`@@ -267,7 +270,10 @@ private async void StartConnectionHandshake(QuicConnection connection, SslClient`
`267`	`270`	`}`
`268`	`271`
`269`	`272`	`await connection.DisposeAsync().ConfigureAwait(false);`
`270`		`- if (!_acceptQueue.Writer.TryWrite(ex))`
	`273`	`+ if (!_acceptQueue.Writer.TryWrite(`
	`274`	`+ wrapException ?`
	`275`	`+ ExceptionDispatchInfo.SetCurrentStackTrace(new QuicException(QuicError.CallbackError, null, SR.net_quic_callback_error, ex)) :`
	`276`	`+ ex))`
`271`	`277`	`{`
`272`	`278`	`// Channel has been closed, connection is already disposed, do nothing.`
`273`	`279`	`}`