Skip to content

Commit

Permalink
Check for sentinel value when setting HTTP/3 error code (#57976)
Browse files Browse the repository at this point in the history
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
  • Loading branch information
amcasey authored Sep 20, 2024
1 parent 74bdb8b commit a717e7f
Show file tree
Hide file tree
Showing 3 changed files with 87 additions and 4 deletions.
11 changes: 8 additions & 3 deletions src/Servers/Kestrel/Core/src/Internal/Http3/Http3Connection.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down Expand Up @@ -505,12 +508,14 @@ public async Task ProcessRequestsAsync<TContext>(IHttpApplication<TContext> 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);
}
}

Expand Down Expand Up @@ -546,7 +551,7 @@ public async Task ProcessRequestsAsync<TContext>(IHttpApplication<TContext> 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)
Expand Down Expand Up @@ -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)
Expand Down
2 changes: 1 addition & 1 deletion src/Servers/Kestrel/shared/test/Http3/Http3InMemory.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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"
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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<IProtocolErrorCodeFeature>();
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;

/// <summary>
/// After <paramref name="skipCount"/> calls to <see cref="AcceptAsync"/>, the next call will throw a <see cref="QuicException"/>
/// (after waiting for <see cref="_requestTcs"/> to be set).
///
/// <paramref name="abortTcs"/> lets this type signal that <see cref="Abort"/> has been called.
/// </summary>
public ThrowingMultiplexedConnectionContext(Http3InMemory testBase, int skipCount, TaskCompletionSource requestTcs, TaskCompletionSource abortTcs)
: base(testBase)
{
_skipCount = skipCount;
_requestTcs = requestTcs;
_abortTcs = abortTcs;
}

public override async ValueTask<ConnectionContext> 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<ConnectionContext> MakeRequestAsync(int index, KeyValuePair<string, string>[] headers, bool sendData, bool waitForServerDispose)
{
var requestStream = await Http3Api.CreateRequestStream(headers, endStream: !sendData);
Expand Down

0 comments on commit a717e7f

Please sign in to comment.