Skip to content

Commit

Permalink
wrap exceptions from callbacks in QuicError.CallbackError (#88614)
Browse files Browse the repository at this point in the history 
* wrap exceptions from callbacks in QuicError.CallbackError

* feedback
  • Loading branch information
@wfurt
wfurt committed Jul 17, 2023
1 parent 90b85ea commit bf78b40
Show file tree
Hide file tree
Showing 5 changed files with 57 additions and 17 deletions.
15 changes: 3 additions & 12 deletions src/libraries/System.Net.Quic/src/Resources/Strings.resx
View file
Expand Up @@ -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>
Expand All @@ -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>
Expand All @@ -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>

10 changes: 9 additions & 1 deletion src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.SslConnectionOptions.cs
View file
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand All @@ -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
Expand Down
8 changes: 7 additions & 1 deletion src/libraries/System.Net.Quic/src/System/Net/Quic/QuicListener.cs
View file
Expand Up @@ -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))
Expand Down Expand Up @@ -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.
}
Expand Down
3 changes: 2 additions & 1 deletion src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicTests.cs
View file
Expand Up @@ -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
Expand Down
38 changes: 36 additions & 2 deletions src/libraries/System.Net.Quic/tests/FunctionalTests/QuicListenerTests.cs
View file
Expand Up @@ -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]
Expand Down

0 comments on commit bf78b40

Please sign in to comment.