HPACK fixes and tests (dotnet#38324)

* HPACK correctness tests/updates. Resolves #31316. Fixes: - Fix check allowing out-of-bounds write in IntegerEncoder when an integer requires more than one byte and there is not enough buffer space. - Encode Content-Type with a "Literal Header Without Indexing -- Indexed Name" rather than with a literal name. Updates: (ported from ASP.NET HPACK code) - Dynamic table size update must be the first instruction in the header block. Throw an exception when not the case. - Throw an exception when we've reached the end of header data and we are still mid-parse. New: - Asserts, documentation, and tests for IntegerEncoder and IntegerDecoder. - Tests to verify HttpClient is correctly encoding the different variants of headers. - HPackDecoder tests (ported from ASP.NET) * Address review feedback and fix CI. * Address review feedback. * Fix licensing to use ASP.NET's licensing. Add a TPN.
almaleksia · Jun 12, 2019 · ed52659 · ed52659
1 parent dc76ccf
commit ed52659
Show file tree

Hide file tree

Showing 21 changed files with 900 additions and 39 deletions.
diff --git a/THIRD-PARTY-NOTICES.TXT b/THIRD-PARTY-NOTICES.TXT
@@ -8,6 +8,15 @@ bring it to our attention. Post an issue or email us:
 
 The attached notices are provided for information only.
 
+License notice for ASP.NET
+-------------------------------
+
+Copyright (c) .NET Foundation. All rights reserved.
+Licensed under the Apache License, Version 2.0.
+
+Available at
+https://github.com/aspnet/AspNetCore/blob/master/LICENSE.txt
+
 License notice for Slicing-by-8 
 -------------------------------
 

diff --git a/src/Common/tests/System/Net/Http/GenericLoopbackServer.cs b/src/Common/tests/System/Net/Http/GenericLoopbackServer.cs
@@ -44,12 +44,16 @@ public struct HttpHeaderData
     {
         public string Name { get; }
         public string Value { get; }
+        public byte[] Raw { get; }
 
-        public HttpHeaderData(string name, string value)
+        public HttpHeaderData(string name, string value, byte[] raw = null)
         {
             Name = name;
             Value = value;
+            Raw = raw;
         }
+
+        public override string ToString() => Name == null ? "<empty>" : (Name + ": " + (Value ?? string.Empty));
     }
 
     public class HttpRequestData

diff --git a/src/Common/tests/System/Net/Http/Http2LoopbackServer.cs b/src/Common/tests/System/Net/Http/Http2LoopbackServer.cs
@@ -579,6 +579,9 @@ public async Task<byte[]> ReadBodyAsync()
             {
                 (int bytesConsumed, HttpHeaderData headerData) = DecodeHeader(data.Span.Slice(i));
 
+                byte[] headerRaw = data.Span.Slice(i, bytesConsumed).ToArray();
+                headerData = new HttpHeaderData(headerData.Name, headerData.Value, headerRaw);
+
                 requestData.Headers.Add(headerData);
                 i += bytesConsumed;
             }

diff --git a/src/System.Net.Http/src/Resources/Strings.resx b/src/System.Net.Http/src/Resources/Strings.resx
@@ -1,4 +1,5 @@
-<root>
+<?xml version="1.0" encoding="utf-8"?>
+<root>
   <!-- 
     Microsoft ResX Schema 
     
@@ -509,4 +510,13 @@
   <data name="net_nego_not_supported_empty_target_with_defaultcreds" xml:space="preserve">
     <value>Target name should be non empty if default credentials are passed.</value>
   </data>
+  <data name="net_http_hpack_huffman_decode_failed" xml:space="preserve">
+    <value>Huffman-coded literal string failed to decode.</value>
+  </data>
+  <data name="net_http_hpack_incomplete_header_block" xml:space="preserve">
+    <value>Incomplete header block received.</value>
+  </data>
+  <data name="net_http_hpack_late_dynamic_table_size_update" xml:space="preserve">
+    <value>Dynamic table size update received after beginning of header block.</value>
+  </data>
 </root>
diff --git a/src/System.Net.Http/src/System/Net/Http/Headers/KnownHeaders.cs b/src/System.Net.Http/src/System/Net/Http/Headers/KnownHeaders.cs
@@ -37,7 +37,7 @@ internal static class KnownHeaders
         public static readonly KnownHeader ContentMD5 = new KnownHeader("Content-MD5", HttpHeaderType.Content, ByteArrayHeaderParser.Parser);
         public static readonly KnownHeader ContentRange = new KnownHeader("Content-Range", HttpHeaderType.Content, GenericHeaderParser.ContentRangeParser, null, StaticTable.ContentRange);
         public static readonly KnownHeader ContentSecurityPolicy = new KnownHeader("Content-Security-Policy");
-        public static readonly KnownHeader ContentType = new KnownHeader("Content-Type", HttpHeaderType.Content, MediaTypeHeaderParser.SingleValueParser);
+        public static readonly KnownHeader ContentType = new KnownHeader("Content-Type", HttpHeaderType.Content, MediaTypeHeaderParser.SingleValueParser, null, StaticTable.ContentType);
         public static readonly KnownHeader Cookie = new KnownHeader("Cookie", StaticTable.Cookie);
         public static readonly KnownHeader Cookie2 = new KnownHeader("Cookie2");
         public static readonly KnownHeader Date = new KnownHeader("Date", HttpHeaderType.General, DateHeaderParser.Parser, null, StaticTable.Date);

diff --git a/src/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HPack/DynamicTable.cs b/src/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HPack/DynamicTable.cs
@@ -1,6 +1,6 @@
-// Licensed to the .NET Foundation under one or more agreements.
-// The .NET Foundation licenses this file to you under the MIT license.
-// See the LICENSE file in the project root for more information.
+// Copyright (c) .NET Foundation. All rights reserved.
+// Licensed under the Apache License, Version 2.0.
+// See THIRD-PARTY-NOTICES.TXT in the project root for license information.
 
 namespace System.Net.Http.HPack
 {

diff --git a/src/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HPack/HPackDecoder.cs b/src/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HPack/HPackDecoder.cs
@@ -1,6 +1,6 @@
-// Licensed to the .NET Foundation under one or more agreements.
-// The .NET Foundation licenses this file to you under the MIT license.
-// See the LICENSE file in the project root for more information.
+// Copyright (c) .NET Foundation. All rights reserved.
+// Licensed under the Apache License, Version 2.0.
+// See THIRD-PARTY-NOTICES.TXT in the project root for license information.
 
 using System.Diagnostics;
 
@@ -97,6 +97,7 @@ private enum State
         private int _headerValueLength;
         private bool _index;
         private bool _huffman;
+        private bool _headersObserved;
 
         public HPackDecoder(int maxDynamicTableSize = DefaultHeaderTableSize)
             : this(maxDynamicTableSize, new DynamicTable(maxDynamicTableSize))
@@ -111,7 +112,7 @@ internal HPackDecoder(int maxDynamicTableSize, DynamicTable dynamicTable)
             _dynamicTable = dynamicTable;
         }
 
-        public void Decode(ReadOnlySpan<byte> data, HeaderCallback onHeader, object onHeaderState)
+        public void Decode(ReadOnlySpan<byte> data, bool endHeaders, HeaderCallback onHeader, object onHeaderState)
         {
             for (int i = 0; i < data.Length; i++)
             {
@@ -125,6 +126,8 @@ public void Decode(ReadOnlySpan<byte> data, HeaderCallback onHeader, object onHe
                         // Look at this once we have more concrete perf data.
                         if ((b & IndexedHeaderFieldMask) == IndexedHeaderFieldRepresentation)
                         {
+                            _headersObserved = true;
+
                             int val = b & ~IndexedHeaderFieldMask;
 
                             if (_integerDecoder.StartDecode((byte)val, IndexedHeaderFieldPrefix))
@@ -138,6 +141,8 @@ public void Decode(ReadOnlySpan<byte> data, HeaderCallback onHeader, object onHe
                         }
                         else if ((b & LiteralHeaderFieldWithIncrementalIndexingMask) == LiteralHeaderFieldWithIncrementalIndexingRepresentation)
                         {
+                            _headersObserved = true;
+
                             _index = true;
                             int val = b & ~LiteralHeaderFieldWithIncrementalIndexingMask;
 
@@ -156,6 +161,8 @@ public void Decode(ReadOnlySpan<byte> data, HeaderCallback onHeader, object onHe
                         }
                         else if ((b & LiteralHeaderFieldWithoutIndexingMask) == LiteralHeaderFieldWithoutIndexingRepresentation)
                         {
+                            _headersObserved = true;
+
                             _index = false;
                             int val = b & ~LiteralHeaderFieldWithoutIndexingMask;
 
@@ -174,6 +181,8 @@ public void Decode(ReadOnlySpan<byte> data, HeaderCallback onHeader, object onHe
                         }
                         else if ((b & LiteralHeaderFieldNeverIndexedMask) == LiteralHeaderFieldNeverIndexedRepresentation)
                         {
+                            _headersObserved = true;
+
                             _index = false;
                             int val = b & ~LiteralHeaderFieldNeverIndexedMask;
 
@@ -192,6 +201,15 @@ public void Decode(ReadOnlySpan<byte> data, HeaderCallback onHeader, object onHe
                         }
                         else if ((b & DynamicTableSizeUpdateMask) == DynamicTableSizeUpdateRepresentation)
                         {
+                            // https://tools.ietf.org/html/rfc7541#section-4.2
+                            // This dynamic table size
+                            // update MUST occur at the beginning of the first header block
+                            // following the change to the dynamic table size.
+                            if (_headersObserved)
+                            {
+                                throw new HPackDecodingException(SR.net_http_hpack_late_dynamic_table_size_update);
+                            }
+
                             if (_integerDecoder.StartDecode((byte)(b & ~DynamicTableSizeUpdateMask), DynamicTableSizeUpdatePrefix))
                             {
                                 // TODO: validate that it's less than what's defined via SETTINGS
@@ -315,6 +333,16 @@ public void Decode(ReadOnlySpan<byte> data, HeaderCallback onHeader, object onHe
                         throw new InternalException(_state);
                 }
             }
+
+            if (endHeaders)
+            {
+                if (_state != State.Ready)
+                {
+                    throw new HPackDecodingException(SR.net_http_hpack_incomplete_header_block);
+                }
+
+                _headersObserved = false;
+            }
         }
 
         public void CompleteDecode()
@@ -385,10 +413,10 @@ int Decode(ref byte[] dst)
                     _headerValueLength = Decode(ref _headerValueOctets);
                 }
             }
-            catch (HuffmanDecodingException)
+            catch (HuffmanDecodingException ex)
             {
                 // Error in huffman encoding.
-                throw new HPackDecodingException();
+                throw new HPackDecodingException(SR.net_http_hpack_huffman_decode_failed, ex);
             }
 
             _state = nextState;

diff --git a/src/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HPack/HPackDecodingException.cs b/src/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HPack/HPackDecodingException.cs
@@ -1,6 +1,6 @@
-// Licensed to the .NET Foundation under one or more agreements.
-// The .NET Foundation licenses this file to you under the MIT license.
-// See the LICENSE file in the project root for more information.
+// Copyright (c) .NET Foundation. All rights reserved.
+// Licensed under the Apache License, Version 2.0.
+// See THIRD-PARTY-NOTICES.TXT in the project root for license information.
 
 namespace System.Net.Http.HPack
 {
@@ -10,5 +10,13 @@ internal class HPackDecodingException : Exception
         public HPackDecodingException()
         {
         }
+
+        public HPackDecodingException(string message) : base(message)
+        {
+        }
+
+        public HPackDecodingException(string message, Exception innerException) : base(message, innerException)
+        {
+        }
     }
 }
diff --git a/src/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HPack/HPackEncoder.cs b/src/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HPack/HPackEncoder.cs
@@ -1,6 +1,6 @@
-// Licensed to the .NET Foundation under one or more agreements.
-// The .NET Foundation licenses this file to you under the MIT license.
-// See the LICENSE file in the project root for more information.
+// Copyright (c) .NET Foundation. All rights reserved.
+// Licensed under the Apache License, Version 2.0.
+// See THIRD-PARTY-NOTICES.TXT in the project root for license information.
 
 using System.Diagnostics;
 

diff --git a/src/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HPack/HeaderField.cs b/src/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HPack/HeaderField.cs
@@ -1,6 +1,6 @@
-// Licensed to the .NET Foundation under one or more agreements.
-// The .NET Foundation licenses this file to you under the MIT license.
-// See the LICENSE file in the project root for more information.
+// Copyright (c) .NET Foundation. All rights reserved.
+// Licensed under the Apache License, Version 2.0.
+// See THIRD-PARTY-NOTICES.TXT in the project root for license information.
 
 namespace System.Net.Http.HPack
 {

diff --git a/src/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HPack/Huffman.cs b/src/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HPack/Huffman.cs
@@ -1,6 +1,6 @@
-// Licensed to the .NET Foundation under one or more agreements.
-// The .NET Foundation licenses this file to you under the MIT license.
-// See the LICENSE file in the project root for more information.
+// Copyright (c) .NET Foundation. All rights reserved.
+// Licensed under the Apache License, Version 2.0.
+// See THIRD-PARTY-NOTICES.TXT in the project root for license information.
 
 using System.Diagnostics;
 

diff --git a/src/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HPack/HuffmanDecodingException.cs b/src/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HPack/HuffmanDecodingException.cs
@@ -1,6 +1,6 @@
-// Licensed to the .NET Foundation under one or more agreements.
-// The .NET Foundation licenses this file to you under the MIT license.
-// See the LICENSE file in the project root for more information.
+// Copyright (c) .NET Foundation. All rights reserved.
+// Licensed under the Apache License, Version 2.0.
+// See THIRD-PARTY-NOTICES.TXT in the project root for license information.
 
 namespace System.Net.Http.HPack
 {

diff --git a/src/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HPack/IntegerDecoder.cs b/src/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HPack/IntegerDecoder.cs
@@ -1,6 +1,8 @@
-// Licensed to the .NET Foundation under one or more agreements.
-// The .NET Foundation licenses this file to you under the MIT license.
-// See the LICENSE file in the project root for more information.
+// Copyright (c) .NET Foundation. All rights reserved.
+// Licensed under the Apache License, Version 2.0.
+// See THIRD-PARTY-NOTICES.TXT in the project root for license information.
+
+using System.Diagnostics;
 
 namespace System.Net.Http.HPack
 {
@@ -11,8 +13,15 @@ internal class IntegerDecoder
 
         public int Value { get; private set; }
 
+        /// <summary>
+        /// Decodes the first byte of the integer.
+        /// </summary>
+        /// <param name="prefixLength">The length of the prefix, in bits, that the integer was encoded with. Must be between 1 and 8.</param>
+        /// <returns>If the integer has been fully decoded, true. Otherwise, false -- <see cref="Decode(byte)"/> must be called on subsequent bytes.</returns>
         public bool StartDecode(byte b, int prefixLength)
         {
+            Debug.Assert(prefixLength >= 1 && prefixLength <= 8);
+
             if (b < ((1 << prefixLength) - 1))
             {
                 Value = b;
@@ -26,6 +35,10 @@ public bool StartDecode(byte b, int prefixLength)
             }
         }
 
+        /// <summary>
+        /// Decodes subsequent bytes of an integer.
+        /// </summary>
+        /// <returns>If the integer has been fully decoded, true. Otherwise, false -- <see cref="Decode(byte)"/> must be called on subsequent bytes.</returns>
         public bool Decode(byte b)
         {
             _i = _i + (b & 127) * (1 << _m);

diff --git a/src/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HPack/IntegerEncoder.cs b/src/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HPack/IntegerEncoder.cs
@@ -1,13 +1,26 @@
-// Licensed to the .NET Foundation under one or more agreements.
-// The .NET Foundation licenses this file to you under the MIT license.
-// See the LICENSE file in the project root for more information.
+// Copyright (c) .NET Foundation. All rights reserved.
+// Licensed under the Apache License, Version 2.0.
+// See THIRD-PARTY-NOTICES.TXT in the project root for license information.
+
+using System.Diagnostics;
 
 namespace System.Net.Http.HPack
 {
     internal static class IntegerEncoder
     {
+        /// <summary>
+        /// Encodes an integer into one or more bytes.
+        /// </summary>
+        /// <param name="value">The value to encode. Must not be negative.</param>
+        /// <param name="numBits">The length of the prefix, in bits, to encode <paramref name="value"/> within. Must be between 1 and 8.</param>
+        /// <param name="destination">The destination span to encode <paramref name="value"/> to.</param>
+        /// <param name="bytesWritten">The number of bytes used to encode <paramref name="value"/>.</param>
+        /// <returns>If <paramref name="destination"/> had enough storage to encode <paramref name="value"/>, true. Otherwise, false.</returns>
         public static bool Encode(int value, int numBits, Span<byte> destination, out int bytesWritten)
         {
+            Debug.Assert(value >= 0);
+            Debug.Assert(numBits >= 1 && numBits <= 8);
+
             if (destination.Length == 0)
             {
                 bytesWritten = 0;
@@ -40,7 +53,7 @@ public static bool Encode(int value, int numBits, Span<byte> destination, out in
                 {
                     destination[i++] = (byte)(value % 128 + 128);
 
-                    if (i > destination.Length)
+                    if (i >= destination.Length)
                     {
                         bytesWritten = 0;
                         return false;

diff --git a/src/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HPack/StaticTable.cs b/src/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HPack/StaticTable.cs
@@ -1,6 +1,6 @@
-// Licensed to the .NET Foundation under one or more agreements.
-// The .NET Foundation licenses this file to you under the MIT license.
-// See the LICENSE file in the project root for more information.
+// Copyright (c) .NET Foundation. All rights reserved.
+// Licensed under the Apache License, Version 2.0.
+// See THIRD-PARTY-NOTICES.TXT in the project root for license information.
 
 using System.Text;
 

diff --git a/src/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Connection.cs b/src/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Connection.cs
@@ -316,6 +316,7 @@ private async Task ProcessHeadersFrame(FrameHeader frameHeader)
 
             _hpackDecoder.Decode(
                 GetFrameData(_incomingBuffer.ActiveSpan.Slice(0, frameHeader.Length), frameHeader.PaddedFlag, frameHeader.PriorityFlag),
+                frameHeader.EndHeadersFlag,
                 s_http2StreamOnResponseHeader,
                 http2Stream);
             _incomingBuffer.Discard(frameHeader.Length);
@@ -331,6 +332,7 @@ private async Task ProcessHeadersFrame(FrameHeader frameHeader)
 
                 _hpackDecoder.Decode(
                     _incomingBuffer.ActiveSpan.Slice(0, frameHeader.Length),
+                    frameHeader.EndHeadersFlag,
                     s_http2StreamOnResponseHeader,
                     http2Stream);
                 _incomingBuffer.Discard(frameHeader.Length);