From a6dab84ffc6d0d3b1ee3a5c3965cd43869e18537 Mon Sep 17 00:00:00 2001 From: James Newton-King Date: Tue, 23 Nov 2021 14:41:38 +1300 Subject: [PATCH 1/3] Fix Kestrel psuedo header reuse --- .../Internal/Http/HttpHeaders.Generated.cs | 7 +++- .../src/Internal/Http/HttpRequestHeaders.cs | 6 +++- .../Core/src/Internal/Http2/Http2Stream.cs | 8 ++--- .../Core/src/Internal/Http3/Http3Stream.cs | 8 ++--- src/Servers/Kestrel/shared/KnownHeaders.cs | 11 ++++++ .../Http2/Http2ConnectionTests.cs | 6 ++++ .../Http2/Http2TestBase.cs | 2 ++ .../Http3/Http3ConnectionTests.cs | 36 +++++++++++++++++++ 8 files changed, 74 insertions(+), 10 deletions(-) diff --git a/src/Servers/Kestrel/Core/src/Internal/Http/HttpHeaders.Generated.cs b/src/Servers/Kestrel/Core/src/Internal/Http/HttpHeaders.Generated.cs index 239420f96d83..cda9a769e69d 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http/HttpHeaders.Generated.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http/HttpHeaders.Generated.cs @@ -7102,6 +7102,11 @@ protected override bool CopyToFast(KeyValuePair[] array, i return true; } + internal void ClearPsuedoRequestHeaders() + { + _psuedoBits = _bits & 240; + _bits &= ~240; + } [MethodImpl(MethodImplOptions.AggressiveInlining)] internal static unsafe ushort ReadUnalignedLittleEndian_ushort(ref byte source) @@ -17014,4 +17019,4 @@ public bool MoveNext() } } } -} +} \ No newline at end of file diff --git a/src/Servers/Kestrel/Core/src/Internal/Http/HttpRequestHeaders.cs b/src/Servers/Kestrel/Core/src/Internal/Http/HttpRequestHeaders.cs index 1681e0702cf4..e6dddbc9731b 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http/HttpRequestHeaders.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http/HttpRequestHeaders.cs @@ -19,6 +19,7 @@ internal sealed partial class HttpRequestHeaders : HttpHeaders { private EnumeratorCache? _enumeratorCache; private long _previousBits; + private long _psuedoBits; public bool ReuseHeaderValues { get; set; } public Func EncodingSelector { get; set; } @@ -59,11 +60,14 @@ protected override void ClearFast() else { // If we are reusing headers, store the currently set headers for comparison later - _previousBits = _bits; + // Psuedo header bits were cleared at the start of a request to hide from the user. + // Keep those values for reuse. + _previousBits = _bits | _psuedoBits; } // Mark no headers as currently in use _bits = 0; + _psuedoBits = 0; // Clear ContentLength and any unknown headers as we will never reuse them _contentLength = null; MaybeUnknown?.Clear(); diff --git a/src/Servers/Kestrel/Core/src/Internal/Http2/Http2Stream.cs b/src/Servers/Kestrel/Core/src/Internal/Http2/Http2Stream.cs index 3fef4ab222b7..9227346050bf 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http2/Http2Stream.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http2/Http2Stream.cs @@ -208,6 +208,10 @@ protected override bool TryParseRequest(ReadResult result, out bool endConnectio // We don't need any of the parameters because we don't implement BeginRead to actually // do the reading from a pipeline, nor do we use endConnection to report connection-level errors. endConnection = !TryValidatePseudoHeaders(); + + // Suppress pseduo headers from the public headers collection. + HttpRequestHeaders.ClearPsuedoRequestHeaders(); + return true; } @@ -249,7 +253,6 @@ private bool TryValidatePseudoHeaders() // enabling the use of HTTP to interact with non - HTTP services. // A common example is TLS termination. var headerScheme = HttpRequestHeaders.HeaderScheme.ToString(); - HttpRequestHeaders.HeaderScheme = default; // Suppress pseduo headers from the public headers collection. if (!ReferenceEquals(headerScheme, Scheme) && !string.Equals(headerScheme, Scheme, StringComparison.OrdinalIgnoreCase)) { @@ -266,7 +269,6 @@ private bool TryValidatePseudoHeaders() // :path (and query) - Required // Must start with / except may be * for OPTIONS var path = HttpRequestHeaders.HeaderPath.ToString(); - HttpRequestHeaders.HeaderPath = default; // Suppress pseduo headers from the public headers collection. RawTarget = path; // OPTIONS - https://tools.ietf.org/html/rfc7540#section-8.1.2.3 @@ -304,7 +306,6 @@ private bool TryValidateMethod() { // :method _methodText = HttpRequestHeaders.HeaderMethod.ToString(); - HttpRequestHeaders.HeaderMethod = default; // Suppress pseduo headers from the public headers collection. Method = HttpUtilities.GetKnownMethod(_methodText); if (Method == HttpMethod.None) @@ -331,7 +332,6 @@ private bool TryValidateAuthorityAndHost(out string hostText) // Prefer this over Host var authority = HttpRequestHeaders.HeaderAuthority; - HttpRequestHeaders.HeaderAuthority = default; // Suppress pseduo headers from the public headers collection. var host = HttpRequestHeaders.HeaderHost; if (!StringValues.IsNullOrEmpty(authority)) { diff --git a/src/Servers/Kestrel/Core/src/Internal/Http3/Http3Stream.cs b/src/Servers/Kestrel/Core/src/Internal/Http3/Http3Stream.cs index 5318a1f7c9f2..0e0f7808bd78 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http3/Http3Stream.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http3/Http3Stream.cs @@ -755,6 +755,10 @@ protected override MessageBody CreateMessageBody() protected override bool TryParseRequest(ReadResult result, out bool endConnection) { endConnection = !TryValidatePseudoHeaders(); + + // Suppress pseduo headers from the public headers collection. + HttpRequestHeaders.ClearPsuedoRequestHeaders(); + return true; } @@ -791,7 +795,6 @@ private bool TryValidatePseudoHeaders() // proxy or gateway can translate requests for non - HTTP schemes, // enabling the use of HTTP to interact with non - HTTP services. var headerScheme = HttpRequestHeaders.HeaderScheme.ToString(); - HttpRequestHeaders.HeaderScheme = default; // Suppress pseduo headers from the public headers collection. if (!ReferenceEquals(headerScheme, Scheme) && !string.Equals(headerScheme, Scheme, StringComparison.OrdinalIgnoreCase)) { @@ -808,7 +811,6 @@ private bool TryValidatePseudoHeaders() // :path (and query) - Required // Must start with / except may be * for OPTIONS var path = HttpRequestHeaders.HeaderPath.ToString(); - HttpRequestHeaders.HeaderPath = default; // Suppress pseduo headers from the public headers collection. RawTarget = path; // OPTIONS - https://tools.ietf.org/html/rfc7540#section-8.1.2.3 @@ -847,7 +849,6 @@ private bool TryValidateMethod() { // :method _methodText = HttpRequestHeaders.HeaderMethod.ToString(); - HttpRequestHeaders.HeaderMethod = default; // Suppress pseduo headers from the public headers collection. Method = HttpUtilities.GetKnownMethod(_methodText); if (Method == Http.HttpMethod.None) @@ -874,7 +875,6 @@ private bool TryValidateAuthorityAndHost(out string hostText) // Prefer this over Host var authority = HttpRequestHeaders.HeaderAuthority; - HttpRequestHeaders.HeaderAuthority = default; // Suppress pseduo headers from the public headers collection. var host = HttpRequestHeaders.HeaderHost; if (!StringValues.IsNullOrEmpty(authority)) { diff --git a/src/Servers/Kestrel/shared/KnownHeaders.cs b/src/Servers/Kestrel/shared/KnownHeaders.cs index aff9a476bed8..8e5d38df807e 100644 --- a/src/Servers/Kestrel/shared/KnownHeaders.cs +++ b/src/Servers/Kestrel/shared/KnownHeaders.cs @@ -59,6 +59,7 @@ public class KnownHeaders .ToArray(); public static readonly long InvalidH2H3ResponseHeadersBits; + public static readonly long PsuedoRequestHeadersBits; static KnownHeaders() { @@ -263,6 +264,11 @@ static KnownHeaders() .Where(header => invalidH2H3ResponseHeaders.Contains(header.Name)) .Select(header => 1L << header.Index) .Aggregate((a, b) => a | b); + + PsuedoRequestHeadersBits = RequestHeaders + .Where(header => PsuedoHeaderNames.Contains(header.Identifier)) + .Select(header => 1L << header.Index) + .Aggregate((a, b) => a | b); } static string Each(IEnumerable values, Func formatter) @@ -1249,6 +1255,11 @@ internal unsafe void CopyToFast(ref BufferWriter output) }} }} while (tempBits != 0); }}" : "")}{(loop.ClassName == "HttpRequestHeaders" ? $@" + internal void ClearPsuedoRequestHeaders() + {{ + _psuedoBits = _bits & {PsuedoRequestHeadersBits}; + _bits &= ~{PsuedoRequestHeadersBits}; + }} {Each(new string[] { "ushort", "uint", "ulong" }, type => $@" [MethodImpl(MethodImplOptions.AggressiveInlining)] internal static unsafe {type} ReadUnalignedLittleEndian_{type}(ref byte source) diff --git a/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http2/Http2ConnectionTests.cs b/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http2/Http2ConnectionTests.cs index 8bad89e959df..6c9ca6e0105b 100644 --- a/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http2/Http2ConnectionTests.cs +++ b/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http2/Http2ConnectionTests.cs @@ -179,6 +179,8 @@ await ExpectAsync(Http2FrameType.HEADERS, withStreamId: 1); var contentType1 = _receivedHeaders["Content-Type"]; + var authority1 = _receivedRequestFields.Authority; + var path1 = _receivedRequestFields.Path; // TriggerTick will trigger the stream to be returned to the pool so we can assert it TriggerTick(); @@ -194,8 +196,12 @@ await ExpectAsync(Http2FrameType.HEADERS, withStreamId: 3); var contentType2 = _receivedHeaders["Content-Type"]; + var authority2 = _receivedRequestFields.Authority; + var path2 = _receivedRequestFields.Path; Assert.Same(contentType1, contentType2); + Assert.Same(authority1, authority2); + Assert.Same(path1, path2); await StopConnectionAsync(expectedLastStreamId: 3, ignoreNonGoAwayFrames: false); } diff --git a/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http2/Http2TestBase.cs b/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http2/Http2TestBase.cs index 822557f255c3..66dde30a7625 100644 --- a/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http2/Http2TestBase.cs +++ b/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http2/Http2TestBase.cs @@ -182,6 +182,7 @@ public Http2TestBase() _receivedRequestFields.Scheme = context.Request.Scheme; _receivedRequestFields.Path = context.Request.Path.Value; _receivedRequestFields.RawTarget = context.Features.Get().RawTarget; + _receivedRequestFields.Authority = context.Request.Host.Value; foreach (var header in context.Request.Headers) { _receivedHeaders[header.Key] = header.Value.ToString(); @@ -1413,5 +1414,6 @@ public class RequestFields public string Scheme { get; set; } public string Path { get; set; } public string RawTarget { get; set; } + public string Authority { get; set; } } } diff --git a/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http3/Http3ConnectionTests.cs b/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http3/Http3ConnectionTests.cs index 8ce552ab1e15..e781613b29e2 100644 --- a/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http3/Http3ConnectionTests.cs +++ b/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http3/Http3ConnectionTests.cs @@ -336,6 +336,42 @@ public async Task StreamPool_MultipleStreamsInSequence_PooledStreamReused() Assert.Same(streamContext1, streamContext2); } + [Fact] + public async Task StreamPool_MultipleStreamsInSequence_KnownHeaderReused() + { + var headers = new[] + { + new KeyValuePair(HeaderNames.Method, "Custom"), + new KeyValuePair(HeaderNames.Path, "/"), + new KeyValuePair(HeaderNames.Scheme, "http"), + new KeyValuePair(HeaderNames.Authority, "localhost:80"), + new KeyValuePair(HeaderNames.ContentType, "application/json"), + }; + + string contentType = null; + string authority = null; + await Http3Api.InitializeConnectionAsync(async context => + { + contentType = context.Request.ContentType; + authority = context.Request.Host.Value; + await _echoApplication(context); + }); + + var streamContext1 = await MakeRequestAsync(0, headers, sendData: true, waitForServerDispose: true); + var contentType1 = contentType; + var authority1 = authority; + + var streamContext2 = await MakeRequestAsync(1, headers, sendData: true, waitForServerDispose: true); + var contentType2 = contentType; + var authority2 = authority; + + Assert.NotNull(contentType1); + Assert.NotNull(authority1); + + Assert.Same(contentType1, contentType2); + Assert.Same(authority1, authority2); + } + [Theory] [InlineData(10)] [InlineData(100)] From e52ba1341fe90462cf4381efc9c10c8434927a10 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=A9bastien=20Ros?= Date: Tue, 23 Nov 2021 07:02:46 -0800 Subject: [PATCH 2/3] Update HttpHeaders.Generated.cs --- .../Kestrel/Core/src/Internal/Http/HttpHeaders.Generated.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Servers/Kestrel/Core/src/Internal/Http/HttpHeaders.Generated.cs b/src/Servers/Kestrel/Core/src/Internal/Http/HttpHeaders.Generated.cs index cda9a769e69d..cf4e546d81e4 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http/HttpHeaders.Generated.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http/HttpHeaders.Generated.cs @@ -17019,4 +17019,4 @@ public bool MoveNext() } } } -} \ No newline at end of file +} From eee6d60c8f6e515cca4c0fbf97063c13ad366a28 Mon Sep 17 00:00:00 2001 From: James Newton-King Date: Wed, 24 Nov 2021 07:48:03 +1300 Subject: [PATCH 3/3] PR feedback --- .../src/Internal/Http/HttpHeaders.Generated.cs | 6 +++--- .../src/Internal/Http/HttpRequestHeaders.cs | 10 +++++----- .../Core/src/Internal/Http2/Http2Stream.cs | 4 ++-- .../Core/src/Internal/Http3/Http3Stream.cs | 4 ++-- .../Core/test/HttpRequestHeadersTests.cs | 18 ++++++++++++++++++ src/Servers/Kestrel/shared/KnownHeaders.cs | 16 ++++++++-------- 6 files changed, 38 insertions(+), 20 deletions(-) diff --git a/src/Servers/Kestrel/Core/src/Internal/Http/HttpHeaders.Generated.cs b/src/Servers/Kestrel/Core/src/Internal/Http/HttpHeaders.Generated.cs index cf4e546d81e4..eee8a7cb47b4 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http/HttpHeaders.Generated.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http/HttpHeaders.Generated.cs @@ -7102,9 +7102,9 @@ protected override bool CopyToFast(KeyValuePair[] array, i return true; } - internal void ClearPsuedoRequestHeaders() + internal void ClearPseudoRequestHeaders() { - _psuedoBits = _bits & 240; + _pseudoBits = _bits & 240; _bits &= ~240; } @@ -17019,4 +17019,4 @@ public bool MoveNext() } } } -} +} \ No newline at end of file diff --git a/src/Servers/Kestrel/Core/src/Internal/Http/HttpRequestHeaders.cs b/src/Servers/Kestrel/Core/src/Internal/Http/HttpRequestHeaders.cs index e6dddbc9731b..cd8e4404dfee 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http/HttpRequestHeaders.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http/HttpRequestHeaders.cs @@ -19,7 +19,7 @@ internal sealed partial class HttpRequestHeaders : HttpHeaders { private EnumeratorCache? _enumeratorCache; private long _previousBits; - private long _psuedoBits; + private long _pseudoBits; public bool ReuseHeaderValues { get; set; } public Func EncodingSelector { get; set; } @@ -55,19 +55,19 @@ protected override void ClearFast() if (!ReuseHeaderValues) { // If we aren't reusing headers clear them all - Clear(_bits); + Clear(_bits | _pseudoBits); } else { // If we are reusing headers, store the currently set headers for comparison later - // Psuedo header bits were cleared at the start of a request to hide from the user. + // Pseudo header bits were cleared at the start of a request to hide from the user. // Keep those values for reuse. - _previousBits = _bits | _psuedoBits; + _previousBits = _bits | _pseudoBits; } // Mark no headers as currently in use _bits = 0; - _psuedoBits = 0; + _pseudoBits = 0; // Clear ContentLength and any unknown headers as we will never reuse them _contentLength = null; MaybeUnknown?.Clear(); diff --git a/src/Servers/Kestrel/Core/src/Internal/Http2/Http2Stream.cs b/src/Servers/Kestrel/Core/src/Internal/Http2/Http2Stream.cs index 9227346050bf..39d3539276cc 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http2/Http2Stream.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http2/Http2Stream.cs @@ -209,8 +209,8 @@ protected override bool TryParseRequest(ReadResult result, out bool endConnectio // do the reading from a pipeline, nor do we use endConnection to report connection-level errors. endConnection = !TryValidatePseudoHeaders(); - // Suppress pseduo headers from the public headers collection. - HttpRequestHeaders.ClearPsuedoRequestHeaders(); + // Suppress pseudo headers from the public headers collection. + HttpRequestHeaders.ClearPseudoRequestHeaders(); return true; } diff --git a/src/Servers/Kestrel/Core/src/Internal/Http3/Http3Stream.cs b/src/Servers/Kestrel/Core/src/Internal/Http3/Http3Stream.cs index 0e0f7808bd78..e08b462d3808 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http3/Http3Stream.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http3/Http3Stream.cs @@ -756,8 +756,8 @@ protected override bool TryParseRequest(ReadResult result, out bool endConnectio { endConnection = !TryValidatePseudoHeaders(); - // Suppress pseduo headers from the public headers collection. - HttpRequestHeaders.ClearPsuedoRequestHeaders(); + // Suppress pseudo headers from the public headers collection. + HttpRequestHeaders.ClearPseudoRequestHeaders(); return true; } diff --git a/src/Servers/Kestrel/Core/test/HttpRequestHeadersTests.cs b/src/Servers/Kestrel/Core/test/HttpRequestHeadersTests.cs index 73d977812965..f8609896cd3e 100644 --- a/src/Servers/Kestrel/Core/test/HttpRequestHeadersTests.cs +++ b/src/Servers/Kestrel/Core/test/HttpRequestHeadersTests.cs @@ -4,6 +4,7 @@ using System; using System.Collections.Generic; using System.Linq; +using System.Reflection; using System.Text; using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http; @@ -112,6 +113,23 @@ public void EntriesCanBeEnumeratedAfterResets() EnumerateEntries((IDictionary)headers); } + [Fact] + public void ClearPseudoRequestHeadersPlusResetClearsHeaderReferenceValue() + { + const BindingFlags privateFlags = BindingFlags.NonPublic | BindingFlags.Instance; + + HttpRequestHeaders headers = new HttpRequestHeaders(reuseHeaderValues: false); + headers.HeaderMethod = "GET"; + headers.ClearPseudoRequestHeaders(); + headers.Reset(); + + // Hacky but required because header references is private. + var headerReferences = typeof(HttpRequestHeaders).GetField("_headers", privateFlags).GetValue(headers); + var methodValue = (StringValues)headerReferences.GetType().GetField("_Method").GetValue(headerReferences); + + Assert.Equal(StringValues.Empty, methodValue); + } + [Fact] public void EnumeratorNotReusedBeforeReset() { diff --git a/src/Servers/Kestrel/shared/KnownHeaders.cs b/src/Servers/Kestrel/shared/KnownHeaders.cs index 8e5d38df807e..079df9639367 100644 --- a/src/Servers/Kestrel/shared/KnownHeaders.cs +++ b/src/Servers/Kestrel/shared/KnownHeaders.cs @@ -39,7 +39,7 @@ public class KnownHeaders HeaderNames.DNT, }; - public static readonly string[] PsuedoHeaderNames = new[] + public static readonly string[] PseudoHeaderNames = new[] { "Authority", // :authority "Method", // :method @@ -50,7 +50,7 @@ public class KnownHeaders public static readonly string[] NonApiHeaders = ObsoleteHeaderNames - .Concat(PsuedoHeaderNames) + .Concat(PseudoHeaderNames) .ToArray(); public static readonly string[] ApiHeaderNames = @@ -59,7 +59,7 @@ public class KnownHeaders .ToArray(); public static readonly long InvalidH2H3ResponseHeadersBits; - public static readonly long PsuedoRequestHeadersBits; + public static readonly long PseudoRequestHeadersBits; static KnownHeaders() { @@ -265,8 +265,8 @@ static KnownHeaders() .Select(header => 1L << header.Index) .Aggregate((a, b) => a | b); - PsuedoRequestHeadersBits = RequestHeaders - .Where(header => PsuedoHeaderNames.Contains(header.Identifier)) + PseudoRequestHeadersBits = RequestHeaders + .Where(header => PseudoHeaderNames.Contains(header.Identifier)) .Select(header => 1L << header.Index) .Aggregate((a, b) => a | b); } @@ -1255,10 +1255,10 @@ internal unsafe void CopyToFast(ref BufferWriter output) }} }} while (tempBits != 0); }}" : "")}{(loop.ClassName == "HttpRequestHeaders" ? $@" - internal void ClearPsuedoRequestHeaders() + internal void ClearPseudoRequestHeaders() {{ - _psuedoBits = _bits & {PsuedoRequestHeadersBits}; - _bits &= ~{PsuedoRequestHeadersBits}; + _pseudoBits = _bits & {PseudoRequestHeadersBits}; + _bits &= ~{PseudoRequestHeadersBits}; }} {Each(new string[] { "ushort", "uint", "ulong" }, type => $@" [MethodImpl(MethodImplOptions.AggressiveInlining)]