HPACK decoder fixes (dotnet/corefx#39907)

- Fix HPACK decoder failing inconsistently with 0-length header names. Fail fast with same exception message that HTTP1 uses. Resolves dotnet/corefx#39873.
- Fix `DynamicTable` returning incorrect values or throwing exceptions when it wraps its ring buffer. Resolves dotnet/corefx#39656.
- Moves HPACK encoder methods from `Http2LoopbackConnection` into their own class.
- Implement all forms of header encodings in HPACK encoder.
- Move a number of files where they belong into the ProductionCode folder in unit test project.

Commit migrated from dotnet/corefx@b7e60c9

src/libraries/Common/tests/System/Net/Http/HPackEncoder.cs

@@ -0,0 +1,194 @@
+// 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.
+
+using System.Diagnostics;
+using System.Text;
+
+namespace System.Net.Test.Common
+{
+    public static class HPackEncoder
+    {
+        /// <summary>
+        /// Encodes a dynamic table size update.
+        /// </summary>
+        /// <param name="newMaximumSize">The new maximum size of the dynamic table. This must be less than or equal to the connection's maximum table size setting, which defaults to 4096 bytes.</param>
+        /// <param name="headerBlock">A span to write the encoded header to.</param>
+        /// <returns>The number of bytes written to <paramref name="headerBlock"/>.</returns>
+        public static int EncodeDynamicTableSizeUpdate(int newMaximumSize, Span<byte> headerBlock)
+        {
+            return EncodeInteger(newMaximumSize, 0b00100000, 0b11100000, headerBlock);
+        }
+
+        /// <summary>
+        /// Encodes a header using an index for both name and value.
+        /// </summary>
+        /// <param name="headerIndex">The header index to encode.</param>
+        /// <param name="headerBlock">A span to write the encoded header to.</param>
+        /// <returns>The number of bytes written to <paramref name="headerBlock"/>.</returns>
+        public static int EncodeHeader(int headerIndex, Span<byte> headerBlock)
+        {
+            Debug.Assert(headerIndex > 0);
+            return EncodeInteger(headerIndex, 0b10000000, 0b10000000, headerBlock);
+        }
+
+        /// <summary>
+        /// Encodes a header using an indexed name and literal value.
+        /// </summary>
+        /// <param name="nameIdx">An index of a header containing the name for this header.</param>
+        /// <param name="value">A literal value to encode for this header.</param>
+        /// <param name="headerBlock">A span to write the encoded header to.</param>
+        /// <returns>The number of bytes written to <paramref name="headerBlock"/>.</returns>
+        public static int EncodeHeader(int nameIdx, string value, HPackFlags flags, Span<byte> headerBlock)
+        {
+            Debug.Assert(nameIdx > 0);
+            return EncodeHeaderImpl(nameIdx, null, value, flags, headerBlock);
+        }
+
+        /// <summary>
+        /// Encodes a header using a literal name and value.
+        /// </summary>
+        /// <param name="name">A literal name to encode for this header.</param>
+        /// <param name="value">A literal value to encode for this header.</param>
+        /// <param name="headerBlock">A span to write the encoded header to.</param>
+        /// <returns>The number of bytes written to <paramref name="headerBlock"/>.</returns>
+        public static int EncodeHeader(string name, string value, HPackFlags flags, Span<byte> headerBlock)
+        {
+            return EncodeHeaderImpl(0, name, value, flags, headerBlock);
+        }
+
+        private static int EncodeHeaderImpl(int nameIdx, string name, string value, HPackFlags flags, Span<byte> headerBlock)
+        {
+            const HPackFlags IndexingMask = HPackFlags.NeverIndexed | HPackFlags.NewIndexed | HPackFlags.WithoutIndexing;
+
+            Debug.Assert((nameIdx != 0) != (name != null), $"Only one of {nameof(nameIdx)} or {nameof(name)} can be used.");
+            Debug.Assert(name != null || (flags & HPackFlags.HuffmanEncodeName) == 0, "An indexed name can not be huffman encoded.");
+
+            byte prefix, prefixMask;
+
+            switch (flags & IndexingMask)
+            {
+                case HPackFlags.WithoutIndexing:
+                    prefix = 0;
+                    prefixMask = 0b11110000;
+                    break;
+                case HPackFlags.NewIndexed:
+                    prefix = 0b01000000;
+                    prefixMask = 0b11000000;
+                    break;
+                case HPackFlags.NeverIndexed:
+                    prefix = 0b00010000;
+                    prefixMask = 0b11110000;
+                    break;
+                default:
+                    throw new Exception("invalid indexing flag");
+            }
+
+            int bytesGenerated = EncodeInteger(nameIdx, prefix, prefixMask, headerBlock);
+
+            if (name != null)
+            {
+                bytesGenerated += EncodeString(name, headerBlock.Slice(bytesGenerated), (flags & HPackFlags.HuffmanEncodeName) != 0);
+            }
+
+            bytesGenerated += EncodeString(value, headerBlock.Slice(bytesGenerated), (flags & HPackFlags.HuffmanEncodeValue) != 0);
+            return bytesGenerated;
+        }
+
+        private static int EncodeString(string value, Span<byte> headerBlock, bool huffmanEncode)
+        {
+            byte[] data = Encoding.ASCII.GetBytes(value);
+            byte prefix;
+
+            if (!huffmanEncode)
+            {
+                prefix = 0;
+            }
+            else
+            {
+                int len = HuffmanEncoder.GetEncodedLength(data);
+
+                byte[] huffmanData = new byte[len];
+                HuffmanEncoder.Encode(data, huffmanData);
+
+                data = huffmanData;
+                prefix = 0x80;
+            }
+
+            int bytesGenerated = 0;
+
+            bytesGenerated += EncodeInteger(data.Length, prefix, 0x80, headerBlock);
+
+            data.AsSpan().CopyTo(headerBlock.Slice(bytesGenerated));
+            bytesGenerated += data.Length;
+
+            return bytesGenerated;
+        }
+
+        public static int EncodeInteger(int value, byte prefix, byte prefixMask, Span<byte> headerBlock)
+        {
+            byte prefixLimit = (byte)(~prefixMask);
+
+            if (value < prefixLimit)
+            {
+                headerBlock[0] = (byte)(prefix | value);
+                return 1;
+            }
+
+            headerBlock[0] = (byte)(prefix | prefixLimit);
+            int bytesGenerated = 1;
+
+            value -= prefixLimit;
+
+            while (value >= 0x80)
+            {
+                headerBlock[bytesGenerated] = (byte)((value & 0x7F) | 0x80);
+                value >>= 7;
+                bytesGenerated++;
+            }
+
+            headerBlock[bytesGenerated] = (byte)value;
+            bytesGenerated++;
+
+            return bytesGenerated;
+        }
+    }
+
+    public enum HPackFlags
+    {
+        /// <summary>
+        /// Encodes a header literal without indexing and without huffman encoding.
+        /// </summary>
+        None = 0,
+
+        /// <summary>
+        /// Applies Huffman encoding to the header's name.
+        /// </summary>
+        HuffmanEncodeName = 1,
+
+        /// <summary>
+        /// Applies Huffman encoding to the header's value.
+        /// </summary>
+        HuffmanEncodeValue = 2,
+
+        /// <summary>
+        /// Applies Huffman encoding to both the name and the value of the header.
+        /// </summary>
+        HuffmanEncode = HuffmanEncodeName | HuffmanEncodeValue,
+
+        /// <summary>
+        /// Encode a literal value without adding a new dynamic index. Intermediaries (such as a proxy) are still allowed to index the value when forwarding the header.
+        /// </summary>
+        WithoutIndexing = 0,
+
+        /// <summary>
+        /// Encode a literal value to a new dynamic index.
+        /// </summary>
+        NewIndexed = 4,
+
+        /// <summary>
+        /// Encode a literal value without adding a new dynamic index. Intermediaries (such as a proxy) must not index the value when forwarding the header.
+        /// </summary>
+        NeverIndexed = 8
+    }
+}

src/libraries/Common/tests/System/Net/Http/Http2LoopbackConnection.cs

@@ -3,6 +3,7 @@
 // See the LICENSE file in the project root for more information.
 
 using System.Collections.Generic;
+using System.Diagnostics;
 using System.IO;
 using System.Net.Http.Functional.Tests;
 using System.Net.Security;
@@ -336,34 +337,6 @@ private static (int bytesConsumed, int value) DecodeInteger(ReadOnlySpan<byte> h
             return (2, prefixMask + b);
         }
 
-        private static int EncodeInteger(int value, byte prefix, byte prefixMask, Span<byte> headerBlock)
-        {
-            byte prefixLimit = (byte)(~prefixMask);
-
-            if (value < prefixLimit)
-            {
-                headerBlock[0] = (byte)(prefix | value);
-                return 1;
-            }
-
-            headerBlock[0] = (byte)(prefix | prefixLimit);
-            int bytesGenerated = 1;
-
-            value -= prefixLimit;
-
-            while (value >= 0x80)
-            {
-                headerBlock[bytesGenerated] = (byte)((value & 0x7F) | 0x80);
-                value = value >> 7;
-                bytesGenerated++;
-            }
-
-            headerBlock[bytesGenerated] = (byte)value;
-            bytesGenerated++;
-
-            return bytesGenerated;
-        }
-
         private static (int bytesConsumed, string value) DecodeString(ReadOnlySpan<byte> headerBlock)
         {
             (int bytesConsumed, int stringLength) = DecodeInteger(headerBlock, 0b01111111);
@@ -382,36 +355,6 @@ private static (int bytesConsumed, string value) DecodeString(ReadOnlySpan<byte>
             }
         }
 
-        private static int EncodeString(string value, Span<byte> headerBlock, bool huffmanEncode)
-        {
-            byte[] data = Encoding.ASCII.GetBytes(value);
-            byte prefix;
-
-            if (!huffmanEncode)
-            {
-                prefix = 0;
-            }
-            else
-            {
-                int len = HuffmanEncoder.GetEncodedLength(data);
-
-                byte[] huffmanData = new byte[len];
-                HuffmanEncoder.Encode(data, huffmanData);
-
-                data = huffmanData;
-                prefix = 0x80;
-            }
-
-            int bytesGenerated = 0;
-
-            bytesGenerated += EncodeInteger(data.Length, prefix, 0x80, headerBlock);
-
-            data.AsSpan().CopyTo(headerBlock.Slice(bytesGenerated));
-            bytesGenerated += data.Length;
-
-            return bytesGenerated;
-        }
-
         private static readonly HttpHeaderData[] s_staticTable = new HttpHeaderData[]
         {
             new HttpHeaderData(":authority", ""),
@@ -537,20 +480,6 @@ private static (int bytesConsumed, HttpHeaderData headerData) DecodeHeader(ReadO
             }
         }
 
-        public static int EncodeHeader(HttpHeaderData headerData, Span<byte> headerBlock)
-        {
-            // Always encode as literal, no indexing.
-            int bytesGenerated = EncodeInteger(0, 0, 0b11110000, headerBlock);
-            bytesGenerated += EncodeString(headerData.Name, headerBlock.Slice(bytesGenerated), headerData.HuffmanEncoded);
-            bytesGenerated += EncodeString(headerData.Value, headerBlock.Slice(bytesGenerated), headerData.HuffmanEncoded);
-            return bytesGenerated;
-        }
-
-        public static int EncodeDynamicTableSizeUpdate(int newMaximumSize, Span<byte> headerBlock)
-        {
-            return EncodeInteger(newMaximumSize, 0b00100000, 0b11100000, headerBlock);
-        }
-
         public async Task<byte[]> ReadBodyAsync()
         {
             byte[] body = null;
@@ -679,14 +608,14 @@ public async Task SendResponseHeadersAsync(int streamId, bool endStream = true,
             if (!isTrailingHeader)
             {
                 string statusCodeString = ((int)statusCode).ToString();
-                bytesGenerated += EncodeHeader(new HttpHeaderData(":status", statusCodeString), headerBlock.AsSpan());
+                bytesGenerated += HPackEncoder.EncodeHeader(":status", statusCodeString, HPackFlags.None, headerBlock.AsSpan());
             }
 
             if (headers != null)
             {
                 foreach (HttpHeaderData headerData in headers)
                 {
-                    bytesGenerated += EncodeHeader(headerData, headerBlock.AsSpan().Slice(bytesGenerated));
+                    bytesGenerated += HPackEncoder.EncodeHeader(headerData.Name, headerData.Value, headerData.HuffmanEncoded ? HPackFlags.HuffmanEncode : HPackFlags.None, headerBlock.AsSpan(bytesGenerated));
                 }
             }
 

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HPack/DynamicTable.cs

@@ -34,7 +34,15 @@ public HeaderField this[int index]
                     throw new IndexOutOfRangeException();
                 }
 
-                return _buffer[_insertIndex == 0 ? _buffer.Length - 1 : _insertIndex - index - 1];
+                index = _insertIndex - index - 1;
+
+                if (index < 0)
+                {
+                    // _buffer is circular; wrap the index back around.
+                    index += _buffer.Length;
+                }
+
+                return _buffer[index];
             }
         }
 

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HPack/HPackDecoder.cs

@@ -254,6 +254,11 @@ public void Decode(ReadOnlySpan<byte> data, bool endHeaders, HeaderCallback onHe
 
                         if (_integerDecoder.StartDecode((byte)(b & ~HuffmanMask), StringLengthPrefix))
                         {
+                            if (_integerDecoder.Value == 0)
+                            {
+                                throw new HPackDecodingException(SR.Format(SR.net_http_invalid_response_header_name, ""));
+                            }
+
                             OnStringLength(_integerDecoder.Value, nextState: State.HeaderName);
                         }
                         else
@@ -265,6 +270,10 @@ public void Decode(ReadOnlySpan<byte> data, bool endHeaders, HeaderCallback onHe
                     case State.HeaderNameLengthContinue:
                         if (_integerDecoder.Decode(b))
                         {
+                            // IntegerDecoder disallows overlong encodings, where an integer is encoded with more bytes than is strictly required.
+                            // 0 should always be represented by a single byte, so we shouldn't need to check for it in the continuation case.
+                            Debug.Assert(_integerDecoder.Value != 0, "A header name length of 0 should never be encoded with a continuation byte.");
+
                             OnStringLength(_integerDecoder.Value, nextState: State.HeaderName);
                         }
 
@@ -302,6 +311,10 @@ public void Decode(ReadOnlySpan<byte> data, bool endHeaders, HeaderCallback onHe
                     case State.HeaderValueLengthContinue:
                         if (_integerDecoder.Decode(b))
                         {
+                            // IntegerDecoder disallows overlong encodings where an integer is encoded with more bytes than is strictly required.
+                            // 0 should always be represented by a single byte, so we shouldn't need to check for it in the continuation case.
+                            Debug.Assert(_integerDecoder.Value != 0, "A header value length of 0 should never be encoded with a continuation byte.");
+
                             OnStringLength(_integerDecoder.Value, nextState: State.HeaderValue);
                         }
 

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HPack/HeaderField.cs

@@ -29,6 +29,6 @@ public HeaderField(ReadOnlySpan<byte> name, ReadOnlySpan<byte> value)
 
         public int Length => GetLength(Name.Length, Value.Length);
 
-        public static int GetLength(int nameLength, int valueLenth) => nameLength + valueLenth + 32;
+        public static int GetLength(int nameLength, int valueLenth) => nameLength + valueLenth + RfcOverhead;
     }
 }

src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Headers.cs

@@ -265,5 +265,23 @@ public async Task SendAsync_GetWithInvalidHostHeader_ThrowsException()
                 await Assert.ThrowsAsync<HttpRequestException>(() => client.SendAsync(m));
             }
         }
+
+        [Fact]
+        public async Task SendAsync_WithZeroLengthHeaderName_Throws()
+        {
+            await LoopbackServerFactory.CreateClientAndServerAsync(
+                async uri =>
+                {
+                    using HttpClient client = CreateHttpClient();
+                    await Assert.ThrowsAsync<HttpRequestException>(() => client.GetAsync(uri));
+                },
+                async server =>
+                {
+                    await server.HandleRequestAsync(headers: new[]
+                    {
+                        new HttpHeaderData("", "foo")
+                    });
+                });
+        }
     }
 }