From 1d685e195ef57c209bffd45cc4dbc88f30fd5deb Mon Sep 17 00:00:00 2001 From: David Fowler Date: Fri, 3 Mar 2017 10:04:44 -0800 Subject: [PATCH] Parser cleanup and remove line continuation header error (#1431) - Cleaned up some parsing logic (removed locals etc) - Removing line continuation errors cleaned up code duplication a little bit --- .../Internal/Http/KestrelHttpParser.cs | 141 ++++-------------- .../Internal/Http/PipelineExtensions.cs | 2 + .../FrameTests.cs | 2 +- test/shared/HttpParsingData.cs | 2 +- 4 files changed, 31 insertions(+), 116 deletions(-) diff --git a/src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/KestrelHttpParser.cs b/src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/KestrelHttpParser.cs index a92c330b05..7ce4097985 100644 --- a/src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/KestrelHttpParser.cs +++ b/src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/KestrelHttpParser.cs @@ -61,17 +61,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Internal.Http end = buffer.Move(end, 1); var startLineBuffer = buffer.Slice(start, end); - if (startLineBuffer.IsSingleSpan) - { - // No copies, directly use the one and only span - span = startLineBuffer.First.Span; - } - else - { - // We're not a single span here but we can use pooled arrays to avoid allocations in the rare case - span = new Span(new byte[startLineBuffer.Length]); - startLineBuffer.CopyTo(span); - } + span = startLineBuffer.ToSpan(); } var pathStart = -1; @@ -80,10 +70,10 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Internal.Http var pathEnd = -1; var versionStart = -1; - HttpVersion httpVersion = HttpVersion.Unknown; + var httpVersion = HttpVersion.Unknown; HttpMethod method; Span customMethod; - int i = 0; + var i = 0; var length = span.Length; var done = false; @@ -320,25 +310,11 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Internal.Http return false; } - if (lineEnd != bufferEnd) - { - lineEnd = buffer.Move(lineEnd, 1); - } + // Make sure LF is included in lineEnd + lineEnd = buffer.Move(lineEnd, 1); var headerBuffer = buffer.Slice(consumed, lineEnd); - - Span span; - if (headerBuffer.IsSingleSpan) - { - // No copies, directly use the one and only span - span = headerBuffer.First.Span; - } - else - { - // We're not a single span here but we can use pooled arrays to avoid allocations in the rare case - span = new Span(new byte[headerBuffer.Length]); - headerBuffer.CopyTo(span); - } + var span = headerBuffer.ToSpan(); if (!TakeSingleHeader(span, out var nameStart, out var nameEnd, out var valueStart, out var valueEnd)) { @@ -347,73 +323,41 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Internal.Http // Skip the reader forward past the header line reader.Skip(span.Length); - - // Before accepting the header line, we need to see at least one character - // > so we can make sure there's no space or tab - var next = reader.Peek(); - - // TODO: We don't need to reject the line here, we can use the state machine - // to store the fact that we're reading a header value - if (next == -1) - { - // If we can't see the next char then reject the entire line - return false; - } - - if (next == ByteSpace || next == ByteTab) - { - // From https://tools.ietf.org/html/rfc7230#section-3.2.4: - // - // Historically, HTTP header field values could be extended over - // multiple lines by preceding each extra line with at least one space - // or horizontal tab (obs-fold). This specification deprecates such - // line folding except within the message/http media type - // (Section 8.3.1). A sender MUST NOT generate a message that includes - // line folding (i.e., that has any field-value that contains a match to - // the obs-fold rule) unless the message is intended for packaging - // within the message/http media type. - // - // A server that receives an obs-fold in a request message that is not - // within a message/http container MUST either reject the message by - // sending a 400 (Bad Request), preferably with a representation - // explaining that obsolete line folding is unacceptable, or replace - // each received obs-fold with one or more SP octets prior to - // interpreting the field value or forwarding the message downstream. - RejectRequest(RequestRejectionReason.HeaderValueLineFoldingNotSupported); - } + consumed = reader.Cursor; + consumedBytes += span.Length; var nameBuffer = span.Slice(nameStart, nameEnd - nameStart); var valueBuffer = span.Slice(valueStart, valueEnd - valueStart); - consumedBytes += span.Length; handler.OnHeader(nameBuffer, valueBuffer); - - consumed = reader.Cursor; } } private unsafe bool TakeMessageHeadersSingleSpan(T handler, Span headersSpan, out int consumedBytes) where T : IHttpHeadersHandler { consumedBytes = 0; + var length = headersSpan.Length; - var remaining = headersSpan.Length; - var index = 0; fixed (byte* data = &headersSpan.DangerousGetPinnableReference()) { - while (true) + while (consumedBytes < length) { - if (remaining < 2) - { - return false; - } + var ch1 = data[consumedBytes]; + var ch2 = -1; - var ch1 = data[index]; - var ch2 = data[index + 1]; + if (consumedBytes + 1 < length) + { + ch2 = data[consumedBytes + 1]; + } if (ch1 == ByteCR) { // Check for final CRLF. - if (ch2 == ByteLF) + if (ch2 == -1) + { + return false; + } + else if (ch2 == ByteLF) { consumedBytes += 2; return true; @@ -427,61 +371,30 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Internal.Http RejectRequest(RequestRejectionReason.HeaderLineMustNotStartWithWhitespace); } - var endOfLineIndex = IndexOf(data, index, headersSpan.Length, ByteLF); + var lineEnd = IndexOf(data, consumedBytes, headersSpan.Length, ByteLF); - if (endOfLineIndex == -1) + if (lineEnd == -1) { return false; } - var span = new Span(data + index, (endOfLineIndex - index + 1)); - index += span.Length; + var span = new Span(data + consumedBytes, (lineEnd - consumedBytes + 1)); if (!TakeSingleHeader(span, out var nameStart, out var nameEnd, out var valueStart, out var valueEnd)) { return false; } - if ((endOfLineIndex + 1) >= headersSpan.Length) - { - return false; - } - - // Before accepting the header line, we need to see at least one character - // > so we can make sure there's no space or tab - var next = data[index]; - - if (next == ByteSpace || next == ByteTab) - { - // From https://tools.ietf.org/html/rfc7230#section-3.2.4: - // - // Historically, HTTP header field values could be extended over - // multiple lines by preceding each extra line with at least one space - // or horizontal tab (obs-fold). This specification deprecates such - // line folding except within the message/http media type - // (Section 8.3.1). A sender MUST NOT generate a message that includes - // line folding (i.e., that has any field-value that contains a match to - // the obs-fold rule) unless the message is intended for packaging - // within the message/http media type. - // - // A server that receives an obs-fold in a request message that is not - // within a message/http container MUST either reject the message by - // sending a 400 (Bad Request), preferably with a representation - // explaining that obsolete line folding is unacceptable, or replace - // each received obs-fold with one or more SP octets prior to - // interpreting the field value or forwarding the message downstream. - RejectRequest(RequestRejectionReason.HeaderValueLineFoldingNotSupported); - } + consumedBytes += span.Length; var nameBuffer = span.Slice(nameStart, nameEnd - nameStart); var valueBuffer = span.Slice(valueStart, valueEnd - valueStart); handler.OnHeader(nameBuffer, valueBuffer); - - consumedBytes += span.Length; - remaining -= span.Length; } } + + return false; } private unsafe int IndexOf(byte* data, int index, int length, byte value) diff --git a/src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/PipelineExtensions.cs b/src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/PipelineExtensions.cs index c7a92e7639..a37670cfa6 100644 --- a/src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/PipelineExtensions.cs +++ b/src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/PipelineExtensions.cs @@ -3,6 +3,7 @@ using System; using System.IO.Pipelines; +using System.Runtime.CompilerServices; using System.Threading; using System.Threading.Tasks; using Microsoft.AspNetCore.Server.Kestrel.Internal.Infrastructure; @@ -77,6 +78,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Internal.Http return result; } + [MethodImpl(MethodImplOptions.AggressiveInlining)] public static Span ToSpan(this ReadableBuffer buffer) { if (buffer.IsSingleSpan) diff --git a/test/Microsoft.AspNetCore.Server.KestrelTests/FrameTests.cs b/test/Microsoft.AspNetCore.Server.KestrelTests/FrameTests.cs index f4b4339479..0f3185e6c8 100644 --- a/test/Microsoft.AspNetCore.Server.KestrelTests/FrameTests.cs +++ b/test/Microsoft.AspNetCore.Server.KestrelTests/FrameTests.cs @@ -199,7 +199,7 @@ namespace Microsoft.AspNetCore.Server.KestrelTests var exception = Assert.Throws(() => _frame.TakeMessageHeaders(readableBuffer, (FrameRequestHeaders)_frame.RequestHeaders, out _consumed, out _examined)); _socketInput.Reader.Advance(_consumed, _examined); - Assert.Equal("Header value line folding not supported.", exception.Message); + Assert.Equal("Header line must not start with whitespace.", exception.Message); Assert.Equal(StatusCodes.Status400BadRequest, exception.StatusCode); } diff --git a/test/shared/HttpParsingData.cs b/test/shared/HttpParsingData.cs index b38afb96c5..a5a83c58c6 100644 --- a/test/shared/HttpParsingData.cs +++ b/test/shared/HttpParsingData.cs @@ -287,7 +287,7 @@ namespace Microsoft.AspNetCore.Testing return new[] { - Tuple.Create(headersWithLineFolding,"Header value line folding not supported."), + Tuple.Create(headersWithLineFolding,"Header line must not start with whitespace."), Tuple.Create(headersWithCRInValue,"Header value must not contain CR characters."), Tuple.Create(headersWithMissingColon,"No ':' character found in header line."), Tuple.Create(headersStartingWithWhitespace, "Header line must not start with whitespace."),