Fix HTTP3 header decoder buffer allocation (dotnet#78862)

* Add test for literal field without name reference * Fix header name buffer allocation * Add more tests * Unified QPackDecoderTest test files * Fix variable name * Fixed HPackDecoder and ported QPack tests * Feedback --------- Co-authored-by: ManickaP <mapichov@microsoft.com>
ManickaP · May 9, 2023 · baa1662 · baa1662
1 parent 4af6023
commit baa1662
Show file tree

Hide file tree

Showing 2 changed files with 112 additions and 5 deletions.
diff --git a/src/libraries/Common/src/System/Net/Http/aspnetcore/Http2/Hpack/HPackDecoder.cs b/src/libraries/Common/src/System/Net/Http/aspnetcore/Http2/Hpack/HPackDecoder.cs
@@ -187,12 +187,11 @@ private void DecodeInternal(ReadOnlySpan<byte> data, IHttpHeadersHandler handler
             // will no longer be valid.
             if (_headerNameRange != null)
             {
-                EnsureStringCapacity(ref _headerNameOctets);
+                EnsureStringCapacity(ref _headerNameOctets, _headerNameLength);
                 _headerName = _headerNameOctets;
 
                 ReadOnlySpan<byte> headerBytes = data.Slice(_headerNameRange.GetValueOrDefault().start, _headerNameRange.GetValueOrDefault().length);
                 headerBytes.CopyTo(_headerName);
-                _headerNameLength = headerBytes.Length;
                 _headerNameRange = null;
             }
         }
@@ -427,6 +426,7 @@ private void ParseHeaderName(ReadOnlySpan<byte> data, ref int currentIndex, IHtt
             {
                 // Fast path. Store the range rather than copying.
                 _headerNameRange = (start: currentIndex, count);
+                _headerNameLength = _stringLength;
                 currentIndex += count;
 
                 _state = State.HeaderValueLength;
@@ -616,11 +616,12 @@ int Decode(ref byte[] dst)
             _state = nextState;
         }
 
-        private void EnsureStringCapacity(ref byte[] dst)
+        private void EnsureStringCapacity(ref byte[] dst, int stringLength = -1)
         {
-            if (dst.Length < _stringLength)
+            stringLength = stringLength >= 0 ? stringLength : _stringLength;
+            if (dst.Length < stringLength)
             {
-                dst = new byte[Math.Max(_stringLength, dst.Length * 2)];
+                dst = new byte[Math.Max(stringLength, dst.Length * 2)];
             }
         }
 

diff --git a/src/libraries/Common/tests/Tests/System/Net/aspnetcore/Http2/HPackDecoderTest.cs b/src/libraries/Common/tests/Tests/System/Net/aspnetcore/Http2/HPackDecoderTest.cs
@@ -46,8 +46,13 @@ public class HPackDecoderTests
 
         private const string _headerNameString = "new-header";
 
+        // On purpose longer than 4096 (DefaultStringOctetsSize from HPackDecoder) to trigger https://github.com/dotnet/runtime/issues/78516
+        private static readonly string _literalHeaderNameString = string.Concat(Enumerable.Range(0, 4100).Select(c => (char)('a' + (c % 26))));
+
         private static readonly byte[] _headerNameBytes = Encoding.ASCII.GetBytes(_headerNameString);
 
+        private static readonly byte[] _literalHeaderNameBytes = Encoding.ASCII.GetBytes(_literalHeaderNameString);
+
         // n     e     w       -      h     e     a     d     e     r      *
         // 10101000 10111110 00010110 10011100 10100011 10010000 10110110 01111111
         private static readonly byte[] _headerNameHuffmanBytes = new byte[] { 0xa8, 0xbe, 0x16, 0x9c, 0xa3, 0x90, 0xb6, 0x7f };
@@ -64,6 +69,12 @@ public class HPackDecoderTests
             .Concat(_headerNameBytes)
             .ToArray();
 
+        // size = 4096 ==> 0x7f, 0x81, 0x1f (7+) prefixed integer
+        // size = 4100 ==> 0x7f, 0x85, 0x1f (7+) prefixed integer
+        private static readonly byte[] _literalHeaderName = new byte[] { 0x7f, 0x85, 0x1f } // 4100
+            .Concat(_literalHeaderNameBytes)
+            .ToArray();
+
         private static readonly byte[] _headerNameHuffman = new byte[] { (byte)(0x80 | _headerNameHuffmanBytes.Length) }
             .Concat(_headerNameHuffmanBytes)
             .ToArray();
@@ -392,6 +403,101 @@ public void DecodesLiteralHeaderFieldNeverIndexed_IndexedName_OutOfRange_Error()
             Assert.Empty(_handler.DecodedHeaders);
         }
 
+        [Fact]
+        public void DecodesLiteralHeaderFieldNeverIndexed_NewName_SingleBuffer()
+        {
+            byte[] encoded = _literalHeaderFieldWithoutIndexingNewName
+                .Concat(_literalHeaderName)
+                .Concat(_headerValue)
+                .ToArray();
+
+            _decoder.Decode(encoded, endHeaders: true, handler: _handler);
+
+            Assert.Equal(1, _handler.DecodedHeaders.Count);
+            Assert.True(_handler.DecodedHeaders.ContainsKey(_literalHeaderNameString));
+            Assert.Equal(_headerValueString, _handler.DecodedHeaders[_literalHeaderNameString]);
+        }
+
+        [Fact]
+        public void DecodesLiteralHeaderFieldNeverIndexed_NewName_NameLengthBrokenIntoSeparateBuffers()
+        {
+            byte[] encoded = _literalHeaderFieldWithoutIndexingNewName
+                .Concat(_literalHeaderName)
+                .Concat(_headerValue)
+                .ToArray();
+
+            _decoder.Decode(encoded[..1], endHeaders: false, handler: _handler);
+            _decoder.Decode(encoded[1..], endHeaders: true, handler: _handler);
+
+            Assert.Equal(1, _handler.DecodedHeaders.Count);
+            Assert.True(_handler.DecodedHeaders.ContainsKey(_literalHeaderNameString));
+            Assert.Equal(_headerValueString, _handler.DecodedHeaders[_literalHeaderNameString]);
+        }
+
+        [Fact]
+        public void DecodesLiteralHeaderFieldNeverIndexed_NewName_NameBrokenIntoSeparateBuffers()
+        {
+            byte[] encoded = _literalHeaderFieldWithoutIndexingNewName
+                .Concat(_literalHeaderName)
+                .Concat(_headerValue)
+                .ToArray();
+
+            _decoder.Decode(encoded[..(_literalHeaderNameString.Length / 2)], endHeaders: false, handler: _handler);
+            _decoder.Decode(encoded[(_literalHeaderNameString.Length / 2)..], endHeaders: true, handler: _handler);
+
+            Assert.Equal(1, _handler.DecodedHeaders.Count);
+            Assert.True(_handler.DecodedHeaders.ContainsKey(_literalHeaderNameString));
+            Assert.Equal(_headerValueString, _handler.DecodedHeaders[_literalHeaderNameString]);
+        }
+
+        [Fact]
+        public void DecodesLiteralHeaderFieldNeverIndexed_NewName_NameAndValueBrokenIntoSeparateBuffers()
+        {
+            byte[] encoded = _literalHeaderFieldWithoutIndexingNewName
+                .Concat(_literalHeaderName)
+                .Concat(_headerValue)
+                .ToArray();
+
+            _decoder.Decode(encoded[..^_headerValue.Length], endHeaders: false, handler: _handler);
+            _decoder.Decode(encoded[^_headerValue.Length..], endHeaders: true, handler: _handler);
+
+            Assert.Equal(1, _handler.DecodedHeaders.Count);
+            Assert.True(_handler.DecodedHeaders.ContainsKey(_literalHeaderNameString));
+            Assert.Equal(_headerValueString, _handler.DecodedHeaders[_literalHeaderNameString]);
+        }
+
+        [Fact]
+        public void DecodesLiteralHeaderFieldNeverIndexed_NewName_ValueLengthBrokenIntoSeparateBuffers()
+        {
+            byte[] encoded = _literalHeaderFieldWithoutIndexingNewName
+                .Concat(_literalHeaderName)
+                .Concat(_headerValue)
+                .ToArray();
+
+            _decoder.Decode(encoded[..^(_headerValue.Length - 1)], endHeaders: false, handler: _handler);
+            _decoder.Decode(encoded[^(_headerValue.Length - 1)..], endHeaders: true, handler: _handler);
+
+            Assert.Equal(1, _handler.DecodedHeaders.Count);
+            Assert.True(_handler.DecodedHeaders.ContainsKey(_literalHeaderNameString));
+            Assert.Equal(_headerValueString, _handler.DecodedHeaders[_literalHeaderNameString]);
+        }
+
+        [Fact]
+        public void DecodesLiteralHeaderFieldNeverIndexed_NewName_ValueBrokenIntoSeparateBuffers()
+        {
+            byte[] encoded = _literalHeaderFieldWithoutIndexingNewName
+                .Concat(_literalHeaderName)
+                .Concat(_headerValue)
+                .ToArray();
+
+            _decoder.Decode(encoded[..^(_headerValueString.Length / 2)], endHeaders: false, handler: _handler);
+            _decoder.Decode(encoded[^(_headerValueString.Length / 2)..], endHeaders: true, handler: _handler);
+
+            Assert.Equal(1, _handler.DecodedHeaders.Count);
+            Assert.True(_handler.DecodedHeaders.ContainsKey(_literalHeaderNameString));
+            Assert.Equal(_headerValueString, _handler.DecodedHeaders[_literalHeaderNameString]);
+        }
+
         [Fact]
         public void DecodesDynamicTableSizeUpdate()
         {