From b30eee23c1e565909a9c6f7e087dab50bf622237 Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Tue, 17 Sep 2024 17:16:44 -0700 Subject: [PATCH] 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/Internal/Http3/Http3Connection.cs | 11 ++- .../shared/test/Http3/Http3InMemory.cs | 2 +- .../Http3/Http3ConnectionTests.cs | 78 +++++++++++++++++++ 3 files changed, 87 insertions(+), 4 deletions(-) diff --git a/src/Servers/Kestrel/Core/src/Internal/Http3/Http3Connection.cs b/src/Servers/Kestrel/Core/src/Internal/Http3/Http3Connection.cs index e0f300839104..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,12 +508,14 @@ public async Task ProcessRequestsAsync(IHttpApplication appl } } + var errorCode = Http3ErrorCodeOrNoError; + // 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 +551,7 @@ public async Task ProcessRequestsAsync(IHttpApplication appl } // Complete - Abort(CreateConnectionAbortError(error, clientAbort), (Http3ErrorCode)_errorCodeFeature.Error, reason); + Abort(CreateConnectionAbortError(error, clientAbort), errorCode, reason); // Wait for active requests to complete. while (_activeRequestCount > 0) @@ -905,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), (Http3ErrorCode)_errorCodeFeature.Error, ConnectionEndReason.TransportCompleted); + Abort(new ConnectionAbortedException(CoreStrings.ConnectionAbortedByClient), Http3ErrorCodeOrNoError, ConnectionEndReason.TransportCompleted); } internal WebTransportSession OpenNewWebTransportSession(Http3Stream http3Stream) 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);