From d87133f31ae740a2df73e322ea4bdc2705b843d2 Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Tue, 17 Sep 2024 17:16:44 -0700 Subject: [PATCH 1/5] Check for sentinel value when setting HTTP/3 error code If no error code has been set, `IProtocolErrorFeature.Error` will be `-1`. If we pass that through verbatim, it will be caught by validation in the setter (ironically, of the same property on the same feature object), resulting in an exception and a Critical (but apparently benign) log message. Fixes #57933 --- src/Servers/Kestrel/Core/src/Internal/Http3/Http3Connection.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/Servers/Kestrel/Core/src/Internal/Http3/Http3Connection.cs b/src/Servers/Kestrel/Core/src/Internal/Http3/Http3Connection.cs index e0f300839104..16b61b2e6f85 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http3/Http3Connection.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http3/Http3Connection.cs @@ -546,7 +546,8 @@ public async Task ProcessRequestsAsync(IHttpApplication appl } // Complete - Abort(CreateConnectionAbortError(error, clientAbort), (Http3ErrorCode)_errorCodeFeature.Error, reason); + var errorCode = _errorCodeFeature.Error == -1 ? Http3ErrorCode.NoError : (Http3ErrorCode)_errorCodeFeature.Error; + Abort(CreateConnectionAbortError(error, clientAbort), errorCode, reason); // Wait for active requests to complete. while (_activeRequestCount > 0) From 117003a3e03ba89f2c846b8afad7b59685b658a9 Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Wed, 18 Sep 2024 17:08:47 -0700 Subject: [PATCH 2/5] Cover a couple other consumers of IProtocolErrorCodeFeature --- .../Core/src/Internal/Http3/Http3Connection.cs | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/src/Servers/Kestrel/Core/src/Internal/Http3/Http3Connection.cs b/src/Servers/Kestrel/Core/src/Internal/Http3/Http3Connection.cs index 16b61b2e6f85..d9164cda6965 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http3/Http3Connection.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http3/Http3Connection.cs @@ -505,12 +505,14 @@ public async Task ProcessRequestsAsync(IHttpApplication appl } } + var errorCode = GetErrorCodeOrNoError(); + // Abort active request streams. lock (_streams) { foreach (var stream in _streams.Values) { - stream.Abort(CreateConnectionAbortError(error, clientAbort), (Http3ErrorCode)_errorCodeFeature.Error); + stream.Abort(CreateConnectionAbortError(error, clientAbort), errorCode); } } @@ -546,7 +548,6 @@ public async Task ProcessRequestsAsync(IHttpApplication appl } // Complete - var errorCode = _errorCodeFeature.Error == -1 ? Http3ErrorCode.NoError : (Http3ErrorCode)_errorCodeFeature.Error; Abort(CreateConnectionAbortError(error, clientAbort), errorCode, reason); // Wait for active requests to complete. @@ -906,7 +907,7 @@ public void OnInputOrOutputCompleted() TryStopAcceptingStreams(); // Abort the connection using the error code the client used. For a graceful close, this should be H3_NO_ERROR. - Abort(new ConnectionAbortedException(CoreStrings.ConnectionAbortedByClient), (Http3ErrorCode)_errorCodeFeature.Error, ConnectionEndReason.TransportCompleted); + Abort(new ConnectionAbortedException(CoreStrings.ConnectionAbortedByClient), GetErrorCodeOrNoError(), ConnectionEndReason.TransportCompleted); } internal WebTransportSession OpenNewWebTransportSession(Http3Stream http3Stream) @@ -924,6 +925,11 @@ internal WebTransportSession OpenNewWebTransportSession(Http3Stream http3Stream) return session; } + private Http3ErrorCode GetErrorCodeOrNoError() + { + return _errorCodeFeature.Error <= 0 ? Http3ErrorCode.NoError : (Http3ErrorCode)_errorCodeFeature.Error; + } + private static class GracefulCloseInitiator { public const int None = 0; From 1393ecc2667738457aeaa4787d524b2c1253c7ac Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Thu, 19 Sep 2024 10:18:14 -0700 Subject: [PATCH 3/5] Add explanatory comment Co-authored-by: James Newton-King --- src/Servers/Kestrel/Core/src/Internal/Http3/Http3Connection.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/Servers/Kestrel/Core/src/Internal/Http3/Http3Connection.cs b/src/Servers/Kestrel/Core/src/Internal/Http3/Http3Connection.cs index d9164cda6965..7f7213853cc6 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http3/Http3Connection.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http3/Http3Connection.cs @@ -927,7 +927,8 @@ internal WebTransportSession OpenNewWebTransportSession(Http3Stream http3Stream) private Http3ErrorCode GetErrorCodeOrNoError() { - return _errorCodeFeature.Error <= 0 ? Http3ErrorCode.NoError : (Http3ErrorCode)_errorCodeFeature.Error; + // The default error value is -1. If it hasn't been changed before abort is called then default to HTTP/3's NoError value. + return _errorCodeFeature.Error == -1 ? Http3ErrorCode.NoError : (Http3ErrorCode)_errorCodeFeature.Error; } private static class GracefulCloseInitiator From 0f72ac7855fcf19a406a37711400c03753761e09 Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Thu, 19 Sep 2024 10:23:25 -0700 Subject: [PATCH 4/5] Use a property --- .../Core/src/Internal/Http3/Http3Connection.cs | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/src/Servers/Kestrel/Core/src/Internal/Http3/Http3Connection.cs b/src/Servers/Kestrel/Core/src/Internal/Http3/Http3Connection.cs index 7f7213853cc6..92777ea1d79c 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http3/Http3Connection.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http3/Http3Connection.cs @@ -101,6 +101,9 @@ private void UpdateHighestOpenedRequestStreamId(long streamId) public string ConnectionId => _context.ConnectionId; public ITimeoutControl TimeoutControl => _context.TimeoutControl; + // The default error value is -1. If it hasn't been changed before abort is called then default to HTTP/3's NoError value. + private Http3ErrorCode Http3ErrorCodeOrNoError => _errorCodeFeature.Error == -1 ? Http3ErrorCode.NoError : (Http3ErrorCode)_errorCodeFeature.Error; + public void StopProcessingNextRequest(ConnectionEndReason reason) => StopProcessingNextRequest(serverInitiated: true, reason); @@ -505,7 +508,7 @@ public async Task ProcessRequestsAsync(IHttpApplication appl } } - var errorCode = GetErrorCodeOrNoError(); + var errorCode = Http3ErrorCodeOrNoError; // Abort active request streams. lock (_streams) @@ -907,7 +910,7 @@ public void OnInputOrOutputCompleted() TryStopAcceptingStreams(); // Abort the connection using the error code the client used. For a graceful close, this should be H3_NO_ERROR. - Abort(new ConnectionAbortedException(CoreStrings.ConnectionAbortedByClient), GetErrorCodeOrNoError(), ConnectionEndReason.TransportCompleted); + Abort(new ConnectionAbortedException(CoreStrings.ConnectionAbortedByClient), Http3ErrorCodeOrNoError, ConnectionEndReason.TransportCompleted); } internal WebTransportSession OpenNewWebTransportSession(Http3Stream http3Stream) @@ -925,12 +928,6 @@ internal WebTransportSession OpenNewWebTransportSession(Http3Stream http3Stream) return session; } - private Http3ErrorCode GetErrorCodeOrNoError() - { - // The default error value is -1. If it hasn't been changed before abort is called then default to HTTP/3's NoError value. - return _errorCodeFeature.Error == -1 ? Http3ErrorCode.NoError : (Http3ErrorCode)_errorCodeFeature.Error; - } - private static class GracefulCloseInitiator { public const int None = 0; From f42dbccd3102ea2f2b4b3fed488aaa7094b7dcd4 Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Thu, 19 Sep 2024 13:52:05 -0700 Subject: [PATCH 5/5] Add a regression test --- .../shared/test/Http3/Http3InMemory.cs | 2 +- .../Http3/Http3ConnectionTests.cs | 78 +++++++++++++++++++ 2 files changed, 79 insertions(+), 1 deletion(-) diff --git a/src/Servers/Kestrel/shared/test/Http3/Http3InMemory.cs b/src/Servers/Kestrel/shared/test/Http3/Http3InMemory.cs index 730e80862de8..64ebcdb07b41 100644 --- a/src/Servers/Kestrel/shared/test/Http3/Http3InMemory.cs +++ b/src/Servers/Kestrel/shared/test/Http3/Http3InMemory.cs @@ -225,7 +225,7 @@ public void TriggerTick(TimeSpan timeSpan = default) public async Task InitializeConnectionAsync(RequestDelegate application) { - MultiplexedConnectionContext = new TestMultiplexedConnectionContext(this) + MultiplexedConnectionContext ??= new TestMultiplexedConnectionContext(this) { ConnectionId = "TEST" }; diff --git a/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http3/Http3ConnectionTests.cs b/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http3/Http3ConnectionTests.cs index 4471e7db42f3..06d96adc238f 100644 --- a/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http3/Http3ConnectionTests.cs +++ b/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http3/Http3ConnectionTests.cs @@ -701,6 +701,84 @@ await Http3Api.InitializeConnectionAsync(async c => await tcs.Task; } + [Fact] + public async Task ErrorCodeIsValidOnConnectionTimeout() + { + // This test loosely repros the scenario in https://github.com/dotnet/aspnetcore/issues/57933. + // In particular, there's a request from the server and, once a response has been sent, + // the (simulated) transport throws a QuicException that surfaces through AcceptAsync. + // This test confirms that Http3Connection.ProcessRequestsAsync doesn't (indirectly) cause + // IProtocolErrorCodeFeature.Error to be set to (or left at) -1, which System.Net.Quic will + // not accept. + + // Used to signal that a request has been sent and a response has been received + var requestTcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + // Used to signal that the connection context has been aborted + var abortTcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + + // InitializeConnectionAsync consumes the connection context, so set it first + Http3Api.MultiplexedConnectionContext = new ThrowingMultiplexedConnectionContext(Http3Api, skipCount: 2, requestTcs, abortTcs); + await Http3Api.InitializeConnectionAsync(_echoApplication); + + await Http3Api.CreateControlStream(); + await Http3Api.GetInboundControlStream(); + var requestStream = await Http3Api.CreateRequestStream(Headers, endStream: true); + var responseHeaders = await requestStream.ExpectHeadersAsync(); + + await requestStream.ExpectReceiveEndOfStream(); + await requestStream.OnDisposedTask.DefaultTimeout(); + + requestTcs.SetResult(); + + // By the time the connection context is aborted, the error code feature has been updated + await abortTcs.Task.DefaultTimeout(); + + Http3Api.CloseServerGracefully(); + + var errorCodeFeature = Http3Api.MultiplexedConnectionContext.Features.Get(); + Assert.InRange(errorCodeFeature.Error, 0, (1L << 62) - 1); // Valid range for HTTP/3 error codes + } + + private sealed class ThrowingMultiplexedConnectionContext : TestMultiplexedConnectionContext + { + private int _skipCount; + private readonly TaskCompletionSource _requestTcs; + private readonly TaskCompletionSource _abortTcs; + + /// + /// After calls to , the next call will throw a + /// (after waiting for to be set). + /// + /// lets this type signal that has been called. + /// + public ThrowingMultiplexedConnectionContext(Http3InMemory testBase, int skipCount, TaskCompletionSource requestTcs, TaskCompletionSource abortTcs) + : base(testBase) + { + _skipCount = skipCount; + _requestTcs = requestTcs; + _abortTcs = abortTcs; + } + + public override async ValueTask AcceptAsync(CancellationToken cancellationToken = default) + { + if (_skipCount-- <= 0) + { + await _requestTcs.Task.DefaultTimeout(); + throw new System.Net.Quic.QuicException( + System.Net.Quic.QuicError.ConnectionTimeout, + applicationErrorCode: null, + "Connection timed out waiting for a response from the peer."); + } + return await base.AcceptAsync(cancellationToken); + } + + public override void Abort(ConnectionAbortedException abortReason) + { + _abortTcs.SetResult(); + base.Abort(abortReason); + } + } + private async Task MakeRequestAsync(int index, KeyValuePair[] headers, bool sendData, bool waitForServerDispose) { var requestStream = await Http3Api.CreateRequestStream(headers, endStream: !sendData);