Repository: netty/netty (Java)

Issue:
hpackdecoder doesn't account for header overhead
Http/2 says for max header list size:

>    SETTINGS_MAX_HEADER_LIST_SIZE (0x6):  This advisory setting informs a
>       peer of the maximum size of header list that the sender is
>       prepared to accept, in octets.  The value is based on the
>       uncompressed size of header fields, including the length of the
>       name and value in octets plus an overhead of 32 octets for each
>       header field.
> 
>       For any given request, a lower limit than what is advertised MAY
>       be enforced.  The initial value of this setting is unlimited.

In HpackEncoder, we account for the 32 bytes, but when decoding, we don't.

The way I found this was by setting up a finagle proxy talking to a finagle server.  When I curled the server directly, I was way beneath the limit, but when I curled the proxy, I got 431s, since it correctly accounted for the 32 bytes overhead on the client-side in HpackEncoder.


Hints:
@mosesn are you planing to work on a PR / unit test ?
Not right now, maybe in the next couple months?  It's not causing us a problem, it just made debugging an issue we ran into a bit harder.
@mosesn got it
I may have a go at it. Sounds simple enough 😝 
@madgnome yay :)

Patch:
diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/HpackDecoder.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/HpackDecoder.java
--- a/codec-http2/src/main/java/io/netty/handler/codec/http2/HpackDecoder.java
+++ b/codec-http2/src/main/java/io/netty/handler/codec/http2/HpackDecoder.java
@@ -429,7 +429,7 @@ private long insertHeader(int streamId, Http2Headers headers, CharSequence name,
 
     private long addHeader(int streamId, Http2Headers headers, CharSequence name, CharSequence value,
                            long headersLength) throws Http2Exception {
-        headersLength += name.length() + value.length();
+        headersLength += HpackHeaderField.sizeOf(name, value);
         if (headersLength > maxHeaderListSizeGoAway) {
             headerListSizeExceeded(maxHeaderListSizeGoAway);
         }


Test patch:
diff --git a/codec-http2/src/test/java/io/netty/handler/codec/http2/HpackDecoderTest.java b/codec-http2/src/test/java/io/netty/handler/codec/http2/HpackDecoderTest.java
--- a/codec-http2/src/test/java/io/netty/handler/codec/http2/HpackDecoderTest.java
+++ b/codec-http2/src/test/java/io/netty/handler/codec/http2/HpackDecoderTest.java
@@ -35,14 +35,19 @@
 import io.netty.buffer.Unpooled;
 import io.netty.util.internal.StringUtil;
 import org.junit.Before;
+import org.junit.Rule;
 import org.junit.Test;
+import org.junit.rules.ExpectedException;
 
 import static io.netty.handler.codec.http2.HpackDecoder.decodeULE128;
 import static io.netty.handler.codec.http2.Http2HeadersEncoder.NEVER_SENSITIVE;
 import static io.netty.util.AsciiString.EMPTY_STRING;
 import static io.netty.util.AsciiString.of;
 import static java.lang.Integer.MAX_VALUE;
+import static org.hamcrest.CoreMatchers.is;
+import static org.hamcrest.Matchers.greaterThanOrEqualTo;
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertThat;
 import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
 import static org.mockito.Mockito.mock;
@@ -52,6 +57,9 @@
 import static org.mockito.Mockito.verifyNoMoreInteractions;
 
 public class HpackDecoderTest {
+    @Rule
+    public ExpectedException expectedException = ExpectedException.none();
+
     private HpackDecoder hpackDecoder;
     private Http2Headers mockHeaders;
 
@@ -425,9 +433,9 @@ public void testLiteralNeverIndexedWithLargeValue() throws Http2Exception {
     @Test
     public void testDecodeLargerThanMaxHeaderListSizeButSmallerThanMaxHeaderListSizeUpdatesDynamicTable()
         throws Http2Exception {
-        ByteBuf in = Unpooled.buffer(200);
+        ByteBuf in = Unpooled.buffer(300);
         try {
-            hpackDecoder.setMaxHeaderListSize(100, 200);
+            hpackDecoder.setMaxHeaderListSize(200, 300);
             HpackEncoder hpackEncoder = new HpackEncoder(true);
 
             // encode headers that are slightly larger than maxHeaderListSize
@@ -477,4 +485,31 @@ public void testDecodeCountsNamesOnlyOnce() throws Http2Exception {
             in.release();
         }
     }
+
+    @Test
+    public void testAccountForHeaderOverhead() throws Exception {
+        ByteBuf in = Unpooled.buffer(100);
+        try {
+            String headerName = "12345";
+            String headerValue = "56789";
+            long headerSize = headerName.length() + headerValue.length();
+            hpackDecoder.setMaxHeaderListSize(headerSize, 100);
+            HpackEncoder hpackEncoder = new HpackEncoder(true);
+
+            Http2Headers toEncode = new DefaultHttp2Headers();
+            toEncode.add(headerName, headerValue);
+            hpackEncoder.encodeHeaders(1, in, toEncode, NEVER_SENSITIVE);
+
+            Http2Headers decoded = new DefaultHttp2Headers();
+
+            // SETTINGS_MAX_HEADER_LIST_SIZE is big enough for the header to fit...
+            assertThat(hpackDecoder.getMaxHeaderListSize(), is(greaterThanOrEqualTo(headerSize)));
+
+            // ... but decode should fail because we add some overhead for each header entry
+            expectedException.expect(Http2Exception.HeaderListSizeException.class);
+            hpackDecoder.decode(1, in, decoded);
+        } finally {
+            in.release();
+        }
+    }
 }