HTTP/3 interop fixes (#42315)

* 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.

diff --git a/src/libraries/Common/src/System/Net/Http/aspnetcore/Quic/Implementations/MsQuic/MsQuicStream.cs b/src/libraries/Common/src/System/Net/Http/aspnetcore/Quic/Implementations/MsQuic/MsQuicStream.cs
index 3b01df5..3a0a353 100644 (file)
--- a/src/libraries/Common/src/System/Net/Http/aspnetcore/Quic/Implementations/MsQuic/MsQuicStream.cs
+++ b/src/libraries/Common/src/System/Net/Http/aspnetcore/Quic/Implementations/MsQuic/MsQuicStream.cs
@@ -966,9 +966,24 @@ namespace System.Net.Quic.Implementations.MsQuic
 
         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
         }
 

diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http3Connection.cs b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http3Connection.cs
index ffde369..c149d9b 100644 (file)
--- a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http3Connection.cs
+++ b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http3Connection.cs
@@ -19,17 +19,6 @@ namespace System.Net.Http
         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;
@@ -453,7 +442,7 @@ namespace System.Net.Http
             buffer[2] = (byte)payloadLength;
             buffer[3] = (byte)Http3SettingType.MaxHeaderListSize;
 
-            return buffer.Slice(4 + integerLength).ToArray();
+            return buffer.Slice(0, 4 + integerLength).ToArray();
         }
 
         /// <summary>
@@ -495,10 +484,11 @@ namespace System.Net.Http
         /// </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)
                     {
@@ -506,6 +496,8 @@ namespace System.Net.Http
                         throw new Http3ConnectionException(Http3ErrorCode.StreamCreationError);
                     }
 
+                    buffer = new ArrayBuffer(initialSize: 32, usePool: true);
+
                     int bytesRead;
 
                     try
@@ -542,7 +534,11 @@ namespace System.Net.Http
                             // 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)
@@ -562,7 +558,8 @@ namespace System.Net.Http
                                 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;
@@ -604,6 +601,10 @@ namespace System.Net.Http
             {
                 Abort(ex);
             }
+            finally
+            {
+                buffer.Dispose();
+            }
         }
 
         /// <summary>
@@ -611,59 +612,66 @@ namespace System.Net.Http
         /// </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;
+                    }
                 }
             }
 
@@ -700,15 +708,6 @@ namespace System.Net.Http
 
             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;
@@ -732,6 +731,15 @@ namespace System.Net.Http
 
                     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)
                     {
@@ -773,12 +781,6 @@ namespace System.Net.Http
 
             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)

diff --git a/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Http3.cs b/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Http3.cs
index 7bb317c..3b545ed 100644 (file)
--- a/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Http3.cs
+++ b/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Http3.cs
@@ -25,16 +25,10 @@ namespace System.Net.Http.Functional.Tests
         {
         }
 
-        /// <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
@@ -49,5 +43,56 @@ namespace System.Net.Http.Functional.Tests
             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
+            };
     }
 }