HTTP/2: Improve incoming header performance

src/Servers/Kestrel/Core/src/Internal/Http/HttpHeaders.Generated.cs

@@ -7714,12 +7714,11 @@ public unsafe void Append(ReadOnlySpan<byte> name, ReadOnlySpan<byte> value, boo
         }
 
         [MethodImpl(MethodImplOptions.AggressiveOptimization)]
-        public unsafe bool TryHPackAppend(int index, ReadOnlySpan<byte> value)
+        public unsafe bool TryHPackAppend(int index, ReadOnlySpan<byte> value, bool checkForNewlineChars)
         {
             ref StringValues values = ref Unsafe.AsRef<StringValues>(null);
             var nameStr = string.Empty;
             var flag = 0L;
-            var checkForNewlineChars = true;
 
             // Does the HPack static index match any "known" headers
             switch (index)

src/Servers/Kestrel/Core/src/Internal/Http2/Http2Connection.cs

@@ -1265,28 +1265,41 @@ private void UpdateConnectionState()
 
     public void OnHeader(ReadOnlySpan<byte> name, ReadOnlySpan<byte> value)
     {
-        OnHeaderCore(index: null, indexedValue: false, name, value);
+        OnHeaderCore(HeaderType.NameAndValue, staticTableIndex: null, name, value);
+    }
+
+    public void OnDynamicIndexedHeader(int? index, ReadOnlySpan<byte> name, ReadOnlySpan<byte> value)
+    {
+        OnHeaderCore(HeaderType.Dynamic, index, name, value);
     }
 
     public void OnStaticIndexedHeader(int index)
     {
         Debug.Assert(index <= H2StaticTable.Count);
 
         ref readonly var entry = ref H2StaticTable.Get(index - 1);
-        OnHeaderCore(index, indexedValue: true, entry.Name, entry.Value);
+        OnHeaderCore(HeaderType.Static, index, entry.Name, entry.Value);
     }
 
     public void OnStaticIndexedHeader(int index, ReadOnlySpan<byte> value)
     {
         Debug.Assert(index <= H2StaticTable.Count);
 
-        OnHeaderCore(index, indexedValue: false, H2StaticTable.Get(index - 1).Name, value);
+        OnHeaderCore(HeaderType.StaticAndValue, index, H2StaticTable.Get(index - 1).Name, value);
+    }
+
+    private enum HeaderType
+    {
+        Static,
+        StaticAndValue,
+        Dynamic,
+        NameAndValue
     }
 
     // We can't throw a Http2StreamErrorException here, it interrupts the header decompression state and may corrupt subsequent header frames on other streams.
     // For now these either need to be connection errors or BadRequests. If we want to downgrade any of them to stream errors later then we need to
     // rework the flow so that the remaining headers are drained and the decompression state is maintained.
-    private void OnHeaderCore(int? index, bool indexedValue, ReadOnlySpan<byte> name, ReadOnlySpan<byte> value)
+    private void OnHeaderCore(HeaderType headerType, int? staticTableIndex, ReadOnlySpan<byte> name, ReadOnlySpan<byte> value)
     {
         Debug.Assert(_currentHeadersStream != null);
 
@@ -1298,32 +1311,59 @@ private void OnHeaderCore(int? index, bool indexedValue, ReadOnlySpan<byte> name
             throw new Http2ConnectionErrorException(CoreStrings.BadRequest_HeadersExceedMaxTotalSize, Http2ErrorCode.PROTOCOL_ERROR);
         }
 
-        if (index != null)
-        {
-            ValidateStaticHeader(index.Value, value);
-        }
-        else
-        {
-            ValidateHeader(name, value);
-        }
-
         try
         {
             if (_requestHeaderParsingState == RequestHeaderParsingState.Trailers)
             {
+                // Just use name + value bytes and do full validation for request trailers.
+                // Potential performance improvement here to check for indexed headers and optimize validation.
+                UpdateHeaderParsingState(value, GetPseudoHeaderField(name));
+                ValidateHeaderContent(name, value);
+
                 _currentHeadersStream.OnTrailer(name, value);
             }
             else
             {
                 // Throws BadRequest for header count limit breaches.
                 // Throws InvalidOperation for bad encoding.
-                if (index != null)
+                switch (headerType)
                 {
-                    _currentHeadersStream.OnHeader(index.Value, indexedValue, name, value);
-                }
-                else
-                {
-                    _currentHeadersStream.OnHeader(name, value, checkForNewlineChars : true);
+                    case HeaderType.Static:
+                        UpdateHeaderParsingState(value, GetPseudoHeaderField(staticTableIndex.GetValueOrDefault()));
+
+                        _currentHeadersStream.OnHeader(staticTableIndex.GetValueOrDefault(), indexedValue: true, name, value);
+                        break;
+                    case HeaderType.StaticAndValue:
+                        UpdateHeaderParsingState(value, GetPseudoHeaderField(staticTableIndex.GetValueOrDefault()));
+
+                        // Value is new will get validated (i.e. check value doesn't contain newlines)
+                        _currentHeadersStream.OnHeader(staticTableIndex.GetValueOrDefault(), indexedValue: false, name, value);
+                        break;
+                    case HeaderType.Dynamic:
+                        // It is faster to set a header using a static table index than a name.
+                        if (staticTableIndex != null)
+                        {
+                            UpdateHeaderParsingState(value, GetPseudoHeaderField(staticTableIndex.GetValueOrDefault()));
+
+                            _currentHeadersStream.OnHeader(staticTableIndex.GetValueOrDefault(), indexedValue: true, name, value);
+                        }
+                        else
+                        {
+                            UpdateHeaderParsingState(value, GetPseudoHeaderField(name));
+
+                            _currentHeadersStream.OnHeader(name, value, checkForNewlineChars: false);
+                        }
+                        break;
+                    case HeaderType.NameAndValue:
+                        UpdateHeaderParsingState(value, GetPseudoHeaderField(name));
+
+                        // Header and value are new and will get validated (i.e. check name is lower-case, check value doesn't contain newlines)
+                        ValidateHeaderContent(name, value);
+                        _currentHeadersStream.OnHeader(name, value, checkForNewlineChars: true);
+                        break;
+                    default:
+                        Debug.Fail($"Unexpected header type: {headerType}");
+                        break;
                 }
             }
         }
@@ -1340,12 +1380,8 @@ private void OnHeaderCore(int? index, bool indexedValue, ReadOnlySpan<byte> name
     public void OnHeadersComplete(bool endStream)
         => _currentHeadersStream!.OnHeadersComplete();
 
-    private void ValidateHeader(ReadOnlySpan<byte> name, ReadOnlySpan<byte> value)
+    private void ValidateHeaderContent(ReadOnlySpan<byte> name, ReadOnlySpan<byte> value)
     {
-        // Clients will normally send pseudo headers as an indexed header.
-        // Because pseudo headers can still be sent by name we need to check for them.
-        UpdateHeaderParsingState(value, GetPseudoHeaderField(name));
-
         if (IsConnectionSpecificHeaderField(name, value))
         {
             throw new Http2ConnectionErrorException(CoreStrings.HttpErrorConnectionSpecificHeaderField, Http2ErrorCode.PROTOCOL_ERROR);
@@ -1369,33 +1405,6 @@ private void ValidateHeader(ReadOnlySpan<byte> name, ReadOnlySpan<byte> value)
         }
     }
 
-    private void ValidateStaticHeader(int index, ReadOnlySpan<byte> value)
-    {
-        var headerField = index switch
-        {
-            1 => PseudoHeaderFields.Authority,
-            2 => PseudoHeaderFields.Method,
-            3 => PseudoHeaderFields.Method,
-            4 => PseudoHeaderFields.Path,
-            5 => PseudoHeaderFields.Path,
-            6 => PseudoHeaderFields.Scheme,
-            7 => PseudoHeaderFields.Scheme,
-            8 => PseudoHeaderFields.Status,
-            9 => PseudoHeaderFields.Status,
-            10 => PseudoHeaderFields.Status,
-            11 => PseudoHeaderFields.Status,
-            12 => PseudoHeaderFields.Status,
-            13 => PseudoHeaderFields.Status,
-            14 => PseudoHeaderFields.Status,
-            _ => PseudoHeaderFields.None
-        };
-
-        UpdateHeaderParsingState(value, headerField);
-
-        // http://httpwg.org/specs/rfc7540.html#rfc.section.8.1.2
-        // No need to validate if header name if it is specified using a static index.
-    }
-
     private void UpdateHeaderParsingState(ReadOnlySpan<byte> value, PseudoHeaderFields headerField)
     {
         // http://httpwg.org/specs/rfc7540.html#rfc.section.8.1.2.1
@@ -1464,6 +1473,32 @@ implementations to these vulnerabilities.*/
         }
     }
 
+    private static PseudoHeaderFields GetPseudoHeaderField(int staticTableIndex)
+    {
+        Debug.Assert(staticTableIndex > 0, "Static table starts at 1.");
+
+        var headerField = staticTableIndex switch
+        {
+            1 => PseudoHeaderFields.Authority,
+            2 => PseudoHeaderFields.Method,
+            3 => PseudoHeaderFields.Method,
+            4 => PseudoHeaderFields.Path,
+            5 => PseudoHeaderFields.Path,
+            6 => PseudoHeaderFields.Scheme,
+            7 => PseudoHeaderFields.Scheme,
+            8 => PseudoHeaderFields.Status,
+            9 => PseudoHeaderFields.Status,
+            10 => PseudoHeaderFields.Status,
+            11 => PseudoHeaderFields.Status,
+            12 => PseudoHeaderFields.Status,
+            13 => PseudoHeaderFields.Status,
+            14 => PseudoHeaderFields.Status,
+            _ => PseudoHeaderFields.None
+        };
+
+        return headerField;
+    }
+
     private static PseudoHeaderFields GetPseudoHeaderField(ReadOnlySpan<byte> name)
     {
         if (name.IsEmpty || name[0] != (byte)':')

src/Servers/Kestrel/Core/src/Internal/Http2/Http2Stream.cs

@@ -675,7 +675,9 @@ public override void OnHeader(int index, bool indexedValue, ReadOnlySpan<byte> n
         // HPack append will return false if the index is not a known request header.
         // For example, someone could send the index of "Server" (a response header) in the request.
         // If that happens then fallback to using Append with the name bytes.
-        if (!HttpRequestHeaders.TryHPackAppend(index, value))
+        //
+        // If the value is indexed then we know it doesn't contain new lines and can skip checking.
+        if (!HttpRequestHeaders.TryHPackAppend(index, value, checkForNewlineChars: !indexedValue))
         {
             AppendHeader(name, value);
         }

src/Servers/Kestrel/Core/src/PublicAPI.Unshipped.txt

@@ -1 +1,2 @@
 #nullable enable
+Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http.IHttpHeadersHandler.OnDynamicIndexedHeader(int? index, System.ReadOnlySpan<byte> name, System.ReadOnlySpan<byte> value) -> void

src/Servers/Kestrel/shared/KnownHeaders.cs

@@ -1303,12 +1303,11 @@ public unsafe void Append(ReadOnlySpan<byte> name, ReadOnlySpan<byte> value, boo
         }}
 
         [MethodImpl(MethodImplOptions.AggressiveOptimization)]
-        public unsafe bool TryHPackAppend(int index, ReadOnlySpan<byte> value)
+        public unsafe bool TryHPackAppend(int index, ReadOnlySpan<byte> value, bool checkForNewlineChars)
         {{
             ref StringValues values = ref Unsafe.AsRef<StringValues>(null);
             var nameStr = string.Empty;
             var flag = 0L;
-            var checkForNewlineChars = true;
 
             // Does the HPack static index match any ""known"" headers
             {AppendHPackSwitch(GroupHPack(loop.Headers))}

src/Servers/Kestrel/test/InMemory.FunctionalTests/Http2/Http2StreamTests.cs

@@ -26,6 +26,27 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Tests;
 
 public class Http2StreamTests : Http2TestBase
 {
+    [Theory]
+    [InlineData("\r")]
+    [InlineData("\n")]
+    [InlineData("\r\n")]
+    public async Task HEADERS_Received_NewLineCharactersInValue_ConnectionError(string headerValue)
+    {
+        var headers = new[]
+        {
+            new KeyValuePair<string, string>(HeaderNames.Method, "GET"),
+            new KeyValuePair<string, string>(HeaderNames.Path, "/"),
+            new KeyValuePair<string, string>(HeaderNames.Scheme, "http"),
+            new KeyValuePair<string, string>(HeaderNames.Authority, "localhost:80"),
+            new KeyValuePair<string, string>("TestHeader", headerValue),
+        };
+        await InitializeConnectionAsync(_noopApplication);
+
+        await StartStreamAsync(1, headers, endStream: true);
+
+        await WaitForConnectionErrorAsync<Exception>(ignoreNonGoAwayFrames: false, 1, Http2ErrorCode.PROTOCOL_ERROR, "Malformed request: invalid headers.");
+    }
+
     [Fact]
     public async Task HEADERS_Received_EmptyMethod_Reset()
     {

src/Shared/runtime/Http2/Hpack/DynamicTable.cs

@@ -46,6 +46,11 @@ public ref readonly HeaderField this[int index]
         }
 
         public void Insert(ReadOnlySpan<byte> name, ReadOnlySpan<byte> value)
+        {
+            Insert(staticTableIndex: null, name, value);
+        }
+
+        public void Insert(int? staticTableIndex, ReadOnlySpan<byte> name, ReadOnlySpan<byte> value)
         {
             int entryLength = HeaderField.GetLength(name.Length, value.Length);
             EnsureAvailable(entryLength);
@@ -59,7 +64,7 @@ public void Insert(ReadOnlySpan<byte> name, ReadOnlySpan<byte> value)
                 return;
             }
 
-            var entry = new HeaderField(name, value);
+            var entry = new HeaderField(staticTableIndex, name, value);
             _buffer[_insertIndex] = entry;
             _insertIndex = (_insertIndex + 1) % _buffer.Length;
             _size += entry.Length;