Skip to content

Commit

Permalink
HTTP/3 interop fixes (#42315)
Browse files Browse the repository at this point in the history 
* Fix SETTINGS frame creation.
* Only call tracing when it is enabled.
* Add descriptions for stream read states.
* Fix: SETTINGS frame parsing not discarding bytes from ArrayBuffer.
* Fix: ownership of ArrayBuffer could have resulted in double-free of arrays.
  • Loading branch information
@scalablecory
scalablecory authored Sep 17, 2020
1 parent db6c320 commit 254627c
Show file tree
Hide file tree
Showing 3 changed files with 147 additions and 85 deletions.
15 changes: 15 additions & 0 deletions ...braries/Common/src/System/Net/Http/aspnetcore/Quic/Implementations/MsQuic/MsQuicStream.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -966,9 +966,24 @@ private void ThrowIfDisposed()

private enum ReadState
{
/// <summary>
/// The stream is open, but there is no data available.
/// </summary>
None,

/// <summary>
/// Data is available in <see cref="_receiveQuicBuffers"/>.
/// </summary>
IndividualReadComplete,

/// <summary>
/// The peer has gracefully shutdown their sends / our receives; the stream's reads are complete.
/// </summary>
ReadsCompleted,

/// <summary>
/// User has aborted the stream, either via a cancellation token on ReadAsync(), or via AbortRead().
/// </summary>
Aborted
}

Expand Down
156 changes: 79 additions & 77 deletions src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http3Connection.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,17 +19,6 @@ internal sealed class Http3Connection : HttpConnectionBase, IDisposable
public static readonly Version HttpVersion30 = new Version(3, 0);
public static readonly SslApplicationProtocol Http3ApplicationProtocol = new SslApplicationProtocol("h3-29");

/// <summary>
/// If we receive a settings frame larger than this, tear down the connection with an error.
/// </summary>
private const int MaximumSettingsPayloadLength = 4096;

/// <summary>
/// Unknown frame types with a payload larger than this will result in tearing down the connection with an error.
/// Frames smaller than this will be ignored and drained.
/// </summary>
private const int MaximumUnknownFramePayloadLength = 4096;

private readonly HttpConnectionPool _pool;
private readonly HttpAuthority? _origin;
private readonly HttpAuthority _authority;
Expand Down Expand Up @@ -453,7 +442,7 @@ public static byte[] BuildSettingsFrame(HttpConnectionSettings settings)
buffer[2] = (byte)payloadLength;
buffer[3] = (byte)Http3SettingType.MaxHeaderListSize;

return buffer.Slice(4 + integerLength).ToArray();
return buffer.Slice(0, 4 + integerLength).ToArray();
}

/// <summary>
Expand Down Expand Up @@ -495,17 +484,20 @@ private async Task AcceptStreamsAsync()
/// </summary>
private async Task ProcessServerStreamAsync(QuicStream stream)
{
ArrayBuffer buffer = default;

try
{
await using (stream.ConfigureAwait(false))
using (var buffer = new ArrayBuffer(initialSize: 32, usePool: true))
{
if (stream.CanWrite)
{
// Server initiated bidirectional streams are either push streams or extensions, and we support neither.
throw new Http3ConnectionException(Http3ErrorCode.StreamCreationError);
}

buffer = new ArrayBuffer(initialSize: 32, usePool: true);

int bytesRead;

try
Expand Down Expand Up @@ -542,7 +534,11 @@ private async Task ProcessServerStreamAsync(QuicStream stream)
// Discard the stream type header.
buffer.Discard(1);

await ProcessServerControlStreamAsync(stream, buffer).ConfigureAwait(false);
// Ownership of buffer is transferred to ProcessServerControlStreamAsync.
ArrayBuffer bufferCopy = buffer;
buffer = default;

await ProcessServerControlStreamAsync(stream, bufferCopy).ConfigureAwait(false);
return;
case (byte)Http3StreamType.QPackDecoder:
if (Interlocked.Exchange(ref _haveServerQpackDecodeStream, 1) != 0)
Expand All @@ -562,7 +558,8 @@ private async Task ProcessServerStreamAsync(QuicStream stream)
throw new Http3ConnectionException(Http3ErrorCode.StreamCreationError);
}

// The stream must not be closed, but we aren't using QPACK right now -- ignore.
// We haven't enabled QPack in our SETTINGS frame, so we shouldn't receive any meaningful data here.
// However, the standard says the stream must not be closed for the lifetime of the connection. Just ignore any data.
buffer.Dispose();
await stream.CopyToAsync(Stream.Null).ConfigureAwait(false);
return;
Expand Down Expand Up @@ -604,66 +601,77 @@ private async Task ProcessServerStreamAsync(QuicStream stream)
{
Abort(ex);
}
finally
{
buffer.Dispose();
}
}

/// <summary>
/// Reads the server's control stream.
/// </summary>
private async Task ProcessServerControlStreamAsync(QuicStream stream, ArrayBuffer buffer)
{
(Http3FrameType? frameType, long payloadLength) = await ReadFrameEnvelopeAsync().ConfigureAwait(false);

if (frameType == null)
using (buffer)
{
// Connection closed prematurely, expected SETTINGS frame.
throw new Http3ConnectionException(Http3ErrorCode.ClosedCriticalStream);
}
// Read the first frame of the control stream. Per spec:
// A SETTINGS frame MUST be sent as the first frame of each control stream.

if (frameType != Http3FrameType.Settings)
{
// Expected SETTINGS as first frame of control stream.
throw new Http3ConnectionException(Http3ErrorCode.MissingSettings);
}
(Http3FrameType? frameType, long payloadLength) = await ReadFrameEnvelopeAsync().ConfigureAwait(false);

await ProcessSettingsFrameAsync(payloadLength).ConfigureAwait(false);
if (frameType == null)
{
// Connection closed prematurely, expected SETTINGS frame.
throw new Http3ConnectionException(Http3ErrorCode.ClosedCriticalStream);
}

while (true)
{
(frameType, payloadLength) = await ReadFrameEnvelopeAsync().ConfigureAwait(false);
if (frameType != Http3FrameType.Settings)
{
throw new Http3ConnectionException(Http3ErrorCode.MissingSettings);
}

await ProcessSettingsFrameAsync(payloadLength).ConfigureAwait(false);

// Read subsequent frames.

switch (frameType)
while (true)
{
case Http3FrameType.GoAway:
await ProcessGoAwayFameAsync(payloadLength).ConfigureAwait(false);
break;
case Http3FrameType.Settings:
// Only a single SETTINGS frame is supported.
throw new Http3ConnectionException(Http3ErrorCode.UnexpectedFrame);
case Http3FrameType.Headers:
case Http3FrameType.Data:
case Http3FrameType.MaxPushId:
case Http3FrameType.DuplicatePush:
// Servers should not send these frames to a control stream.
throw new Http3ConnectionException(Http3ErrorCode.UnexpectedFrame);
case Http3FrameType.PushPromise:
case Http3FrameType.CancelPush:
// Because we haven't sent any MAX_PUSH_ID frame, it is invalid to receive any push-related frames as they will all reference a too-large ID.
throw new Http3ConnectionException(Http3ErrorCode.IdError);
case null:
// End of stream reached. If we're shutting down, stop looping. Otherwise, this is an error (this stream should not be closed for life of connection).
bool shuttingDown;
lock (SyncObj)
{
shuttingDown = ShuttingDown;
}
if (!shuttingDown)
{
throw new Http3ConnectionException(Http3ErrorCode.ClosedCriticalStream);
}
return;
default:
await SkipUnknownPayloadAsync(frameType.GetValueOrDefault(), payloadLength).ConfigureAwait(false);
break;
(frameType, payloadLength) = await ReadFrameEnvelopeAsync().ConfigureAwait(false);

switch (frameType)
{
case Http3FrameType.GoAway:
await ProcessGoAwayFameAsync(payloadLength).ConfigureAwait(false);
break;
case Http3FrameType.Settings:
// If an endpoint receives a second SETTINGS frame on the control stream, the endpoint MUST respond with a connection error of type H3_FRAME_UNEXPECTED.
throw new Http3ConnectionException(Http3ErrorCode.UnexpectedFrame);
case Http3FrameType.Headers:
case Http3FrameType.Data:
case Http3FrameType.MaxPushId:
case Http3FrameType.DuplicatePush:
// Servers should not send these frames to a control stream.
throw new Http3ConnectionException(Http3ErrorCode.UnexpectedFrame);
case Http3FrameType.PushPromise:
case Http3FrameType.CancelPush:
// Because we haven't sent any MAX_PUSH_ID frame, it is invalid to receive any push-related frames as they will all reference a too-large ID.
throw new Http3ConnectionException(Http3ErrorCode.IdError);
case null:
// End of stream reached. If we're shutting down, stop looping. Otherwise, this is an error (this stream should not be closed for life of connection).
bool shuttingDown;
lock (SyncObj)
{
shuttingDown = ShuttingDown;
}
if (!shuttingDown)
{
throw new Http3ConnectionException(Http3ErrorCode.ClosedCriticalStream);
}
return;
default:
await SkipUnknownPayloadAsync(frameType.GetValueOrDefault(), payloadLength).ConfigureAwait(false);
break;
}
}
}

Expand Down Expand Up @@ -700,15 +708,6 @@ private async Task ProcessServerControlStreamAsync(QuicStream stream, ArrayBuffe

async ValueTask ProcessSettingsFrameAsync(long settingsPayloadLength)
{
if (settingsPayloadLength > MaximumSettingsPayloadLength)
{
if (NetEventSource.Log.IsEnabled())
{
Trace($"Received SETTINGS frame with {settingsPayloadLength} byte payload exceeding the {MaximumSettingsPayloadLength} byte maximum.");
}
throw new Http3ConnectionException(Http3ErrorCode.ExcessiveLoad);
}

while (settingsPayloadLength != 0)
{
long settingId, settingValue;
Expand All @@ -732,6 +731,15 @@ async ValueTask ProcessSettingsFrameAsync(long settingsPayloadLength)

settingsPayloadLength -= bytesRead;

if (settingsPayloadLength < 0)
{
// An integer was encoded past the payload length.
// A frame payload that contains additional bytes after the identified fields or a frame payload that terminates before the end of the identified fields MUST be treated as a connection error of type H3_FRAME_ERROR.
throw new Http3ConnectionException(Http3ErrorCode.FrameError);
}

buffer.Discard(bytesRead);

// Only support this single setting. Skip others.
if (settingId == (long)Http3SettingType.MaxHeaderListSize)
{
Expand Down Expand Up @@ -773,12 +781,6 @@ async ValueTask ProcessGoAwayFameAsync(long goawayPayloadLength)

async ValueTask SkipUnknownPayloadAsync(Http3FrameType frameType, long payloadLength)
{
if (payloadLength > MaximumUnknownFramePayloadLength)
{
Trace($"Received unknown frame type 0x{(long)frameType:x} with {payloadLength} byte payload exceeding the {MaximumUnknownFramePayloadLength} byte maximum.");
throw new Http3ConnectionException(Http3ErrorCode.ExcessiveLoad);
}

while (payloadLength != 0)
{
if (buffer.ActiveLength == 0)
Expand Down
61 changes: 53 additions & 8 deletions src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Http3.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,16 +25,10 @@ public HttpClientHandlerTest_Http3(ITestOutputHelper output) : base(output)
{
}

/// <summary>
/// These are public interop test servers for various QUIC and HTTP/3 implementations,
/// taken from https://github.com/quicwg/base-drafts/wiki/Implementations
/// </summary>
[OuterLoop]
[Theory]
[InlineData("https://quic.rocks:4433/")] // Chromium
[InlineData("https://www.litespeedtech.com/")] // LiteSpeed
[InlineData("https://quic.tech:8443/")] // Cloudflare
public async Task Public_Interop_Success(string uri)
[MemberData(nameof(InteropUris))]
public async Task Public_Interop_ExactVersion_Success(string uri)
{
using HttpClient client = CreateHttpClient();
using HttpRequestMessage request = new HttpRequestMessage
Expand All @@ -49,5 +43,56 @@ public async Task Public_Interop_Success(string uri)
Assert.Equal(HttpStatusCode.OK, response.StatusCode);
Assert.Equal(3, response.Version.Major);
}

[OuterLoop]
[Theory]
[MemberData(nameof(InteropUris))]
public async Task Public_Interop_Upgrade_Success(string uri)
{
using HttpClient client = CreateHttpClient();

// First request uses HTTP/1 or HTTP/2 and receives an Alt-Svc either by header or (with HTTP/2) by frame.

using (HttpRequestMessage requestA = new HttpRequestMessage
{
Method = HttpMethod.Get,
RequestUri = new Uri(uri, UriKind.Absolute),
Version = HttpVersion30,
VersionPolicy = HttpVersionPolicy.RequestVersionOrLower
})
{
using HttpResponseMessage responseA = await client.SendAsync(requestA).TimeoutAfter(20_000);
Assert.Equal(HttpStatusCode.OK, responseA.StatusCode);
Assert.NotEqual(3, responseA.Version.Major);
}

// Second request uses HTTP/3.

using (HttpRequestMessage requestB = new HttpRequestMessage
{
Method = HttpMethod.Get,
RequestUri = new Uri(uri, UriKind.Absolute),
Version = HttpVersion30,
VersionPolicy = HttpVersionPolicy.RequestVersionOrLower
})
{
using HttpResponseMessage responseB = await client.SendAsync(requestB).TimeoutAfter(20_000);

Assert.Equal(HttpStatusCode.OK, responseB.StatusCode);
Assert.NotEqual(3, responseB.Version.Major);
}
}

/// <summary>
/// These are public interop test servers for various QUIC and HTTP/3 implementations,
/// taken from https://github.com/quicwg/base-drafts/wiki/Implementations
/// </summary>
public static TheoryData<string> InteropUris() =>
new TheoryData<string>
{
{ "https://quic.rocks:4433/" }, // Chromium
{ "https://http3-test.litespeedtech.com:4433/" }, // LiteSpeed
{ "https://quic.tech:8443/" } // Cloudflare
};
}
}

0 comments on commit 254627c

Please sign in to comment.