Repository: netty/netty
Issue: HpackDecoder's maxHeaderListSizeGoAway kills stream unnecessarily
Labels: improvement

Consider this a bit of a TODO for myself, although @Scottmitch or @normanmaurer may have opinions about avoiding killing the connection in this case.

### Expected behavior

Sending too large of headers would not unnecessarily kill the connection.

### Actual behavior

When maxHeaderListSizeGoAway is exceeded, HpackDecoder will throw a connection error, killing the entire connection.

```
                            // Check new header size against max header size
                            if ((long) index + nameLength > maxHeaderListSizeGoAway - headersLength) {
                                headerListSizeExceeded(maxHeaderListSizeGoAway);
                            }
```

Note, I'm not talking about the separate check later against maxHeaderListSize and throws a _stream_ error:
```
        // we have read all of our headers, and not exceeded maxHeaderListSizeGoAway see if we have
        // exceeded our actual maxHeaderListSize. This must be done here to prevent dynamic table
        // corruption
        if (headersLength > maxHeaderListSize) {
            headerListSizeExceeded(streamId, maxHeaderListSize, true);
        }
```

Even though both of those call `headerListSizeExceeded()`, they call different overloads of the method which will throw different types of exceptions (connection vs stream).

The value of maxHeaderListSizeGoAway is likely 1.25 the size of maxHeaderListSize, although I see there's some ways it could be something different and I didn't dig in more.

This killing of the connection seems unnecessary though, as:
1) It seems it would be pretty easy to throw away this batch of headers, by setting `headers` to a `Http2Headers` implementation that throws away further adds (similar complexity to `EmptyHttp2Headers`). This could be done any time `headersLength > maxHeaderListSize`.
2) When decode is called, the "entire header block is contained in {@code in}". So this protection doesn't protect us from buffering any more data; it only reduced the decoded headers size. That is, throwing away decoded headers as suggested in (1) does not seem to expose us to other problems.

Note also there are separate checks in `DefaultHttp2FrameReader.HeadersBlockBuilder` that protect against too large of compressed HEADERS+CONTINUATION. This is what constrains the size of `in`.

Issue comments:
I think what you said @ejona86 makes sense... want to do a pr ?
I worked on the change last night :) but didn't send it out because I was investigating further changes. Sent it out now.

Pull request review comments:
s/max/goAwayMax/
"positive or zero"?
Yeah, looks like it should be "non-negative". This was pre-existing code and just moved, but I can update it.
@ejona86 move this to the public method (as it was before) ?
@ejona86 should we just keep the method as well and deprecate it and ignore the second argument ?
final ?
?
Whatever you prefer 
Done.
It was a bug. `goAwayMax` wasn't used when it should have been. Fixed.
Done. This had been public in my changes until the last minute. Oops.
Unclear how many people are using that, but sure. It's easy. Done.
I don't tend to put `final` on private classes, but sure. Done.

Patch that resolved the issue:
diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2HeadersDecoder.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2HeadersDecoder.java
index 684fef03528..5d6320950ca 100644
--- a/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2HeadersDecoder.java
+++ b/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2HeadersDecoder.java
@@ -22,6 +22,7 @@
 import static io.netty.handler.codec.http2.Http2CodecUtil.DEFAULT_HEADER_LIST_SIZE;
 import static io.netty.handler.codec.http2.Http2CodecUtil.DEFAULT_INITIAL_HUFFMAN_DECODE_CAPACITY;
 import static io.netty.handler.codec.http2.Http2Error.COMPRESSION_ERROR;
+import static io.netty.handler.codec.http2.Http2Error.INTERNAL_ERROR;
 import static io.netty.handler.codec.http2.Http2Exception.connectionError;
 
 @UnstableApi
@@ -31,6 +32,7 @@ public class DefaultHttp2HeadersDecoder implements Http2HeadersDecoder, Http2Hea
 
     private final HpackDecoder hpackDecoder;
     private final boolean validateHeaders;
+    private long maxHeaderListSizeGoAway;
 
     /**
      * Used to calculate an exponential moving average of header sizes to get an estimate of how large the data
@@ -79,6 +81,8 @@ public DefaultHttp2HeadersDecoder(boolean validateHeaders, long maxHeaderListSiz
     DefaultHttp2HeadersDecoder(boolean validateHeaders, HpackDecoder hpackDecoder) {
         this.hpackDecoder = ObjectUtil.checkNotNull(hpackDecoder, "hpackDecoder");
         this.validateHeaders = validateHeaders;
+        this.maxHeaderListSizeGoAway =
+                Http2CodecUtil.calculateMaxHeaderListSizeGoAway(hpackDecoder.getMaxHeaderListSize());
     }
 
     @Override
@@ -93,7 +97,12 @@ public long maxHeaderTableSize() {
 
     @Override
     public void maxHeaderListSize(long max, long goAwayMax) throws Http2Exception {
-        hpackDecoder.setMaxHeaderListSize(max, goAwayMax);
+        if (goAwayMax < max || goAwayMax < 0) {
+            throw connectionError(INTERNAL_ERROR, "Header List Size GO_AWAY %d must be non-negative and >= %d",
+                    goAwayMax, max);
+        }
+        hpackDecoder.setMaxHeaderListSize(max);
+        this.maxHeaderListSizeGoAway = goAwayMax;
     }
 
     @Override
@@ -103,7 +112,7 @@ public long maxHeaderListSize() {
 
     @Override
     public long maxHeaderListSizeGoAway() {
-        return hpackDecoder.getMaxHeaderListSizeGoAway();
+        return maxHeaderListSizeGoAway;
     }
 
     @Override
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
index 67d6aa9944b..078c0798918 100644
--- 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
@@ -42,7 +42,6 @@
 import static io.netty.handler.codec.http2.Http2CodecUtil.MIN_HEADER_TABLE_SIZE;
 import static io.netty.handler.codec.http2.Http2CodecUtil.headerListSizeExceeded;
 import static io.netty.handler.codec.http2.Http2Error.COMPRESSION_ERROR;
-import static io.netty.handler.codec.http2.Http2Error.INTERNAL_ERROR;
 import static io.netty.handler.codec.http2.Http2Error.PROTOCOL_ERROR;
 import static io.netty.handler.codec.http2.Http2Exception.connectionError;
 import static io.netty.handler.codec.http2.Http2Headers.PseudoHeaderName.getPseudoHeader;
@@ -84,7 +83,6 @@ final class HpackDecoder {
 
     private final HpackDynamicTable hpackDynamicTable;
     private final HpackHuffmanDecoder hpackHuffmanDecoder;
-    private long maxHeaderListSizeGoAway;
     private long maxHeaderListSize;
     private long maxDynamicTableSize;
     private long encoderMaxDynamicTableSize;
@@ -108,7 +106,6 @@ final class HpackDecoder {
      */
     HpackDecoder(long maxHeaderListSize, int initialHuffmanDecodeCapacity, int maxHeaderTableSize) {
         this.maxHeaderListSize = checkPositive(maxHeaderListSize, "maxHeaderListSize");
-        this.maxHeaderListSizeGoAway = Http2CodecUtil.calculateMaxHeaderListSizeGoAway(maxHeaderListSize);
 
         maxDynamicTableSize = encoderMaxDynamicTableSize = maxHeaderTableSize;
         maxDynamicTableSizeChangeRequired = false;
@@ -122,8 +119,18 @@ final class HpackDecoder {
      * This method assumes the entire header block is contained in {@code in}.
      */
     public void decode(int streamId, ByteBuf in, Http2Headers headers, boolean validateHeaders) throws Http2Exception {
+        Http2HeadersSink sink = new Http2HeadersSink(headers, maxHeaderListSize);
+        decode(in, sink, validateHeaders);
+
+        // we have read all of our headers. See if we have exceeded our maxHeaderListSize. We must
+        // delay throwing until this point to prevent dynamic table corruption
+        if (sink.exceededMaxLength()) {
+            headerListSizeExceeded(streamId, maxHeaderListSize, true);
+        }
+    }
+
+    private void decode(ByteBuf in, Sink sink, boolean validateHeaders) throws Http2Exception {
         int index = 0;
-        long headersLength = 0;
         int nameLength = 0;
         int valueLength = 0;
         byte state = READ_HEADER_REPRESENTATION;
@@ -151,8 +158,7 @@ public void decode(int streamId, ByteBuf in, Http2Headers headers, boolean valid
                             default:
                                 HpackHeaderField indexedHeader = getIndexedHeader(index);
                                 headerType = validate(indexedHeader.name, headerType, validateHeaders);
-                                headersLength = addHeader(headers, indexedHeader.name, indexedHeader.value,
-                                        headersLength);
+                                sink.appendToHeaderList(indexedHeader.name, indexedHeader.value);
                         }
                     } else if ((b & 0x40) == 0x40) {
                         // Literal Header Field with Incremental Indexing
@@ -193,11 +199,11 @@ public void decode(int streamId, ByteBuf in, Http2Headers headers, boolean valid
                                 state = READ_INDEXED_HEADER_NAME;
                                 break;
                             default:
-                            // Index was stored as the prefix
-                            name = readName(index);
+                                // Index was stored as the prefix
+                                name = readName(index);
                                 headerType = validate(name, headerType, validateHeaders);
-                            nameLength = name.length();
-                            state = READ_LITERAL_HEADER_VALUE_LENGTH_PREFIX;
+                                nameLength = name.length();
+                                state = READ_LITERAL_HEADER_VALUE_LENGTH_PREFIX;
                         }
                     }
                     break;
@@ -210,7 +216,7 @@ public void decode(int streamId, ByteBuf in, Http2Headers headers, boolean valid
                 case READ_INDEXED_HEADER:
                     HpackHeaderField indexedHeader = getIndexedHeader(decodeULE128(in, index));
                     headerType = validate(indexedHeader.name, headerType, validateHeaders);
-                    headersLength = addHeader(headers, indexedHeader.name, indexedHeader.value, headersLength);
+                    sink.appendToHeaderList(indexedHeader.name, indexedHeader.value);
                     state = READ_HEADER_REPRESENTATION;
                     break;
 
@@ -229,9 +235,6 @@ public void decode(int streamId, ByteBuf in, Http2Headers headers, boolean valid
                     if (index == 0x7f) {
                         state = READ_LITERAL_HEADER_NAME_LENGTH;
                     } else {
-                        if (index > maxHeaderListSizeGoAway - headersLength) {
-                            headerListSizeExceeded(maxHeaderListSizeGoAway);
-                        }
                         nameLength = index;
                         state = READ_LITERAL_HEADER_NAME;
                     }
@@ -241,9 +244,6 @@ public void decode(int streamId, ByteBuf in, Http2Headers headers, boolean valid
                     // Header Name is a Literal String
                     nameLength = decodeULE128(in, index);
 
-                    if (nameLength > maxHeaderListSizeGoAway - headersLength) {
-                        headerListSizeExceeded(maxHeaderListSizeGoAway);
-                    }
                     state = READ_LITERAL_HEADER_NAME;
                     break;
 
@@ -269,14 +269,10 @@ public void decode(int streamId, ByteBuf in, Http2Headers headers, boolean valid
                             break;
                         case 0:
                             headerType = validate(name, headerType, validateHeaders);
-                            headersLength = insertHeader(headers, name, EMPTY_STRING, indexType, headersLength);
+                            insertHeader(sink, name, EMPTY_STRING, indexType);
                             state = READ_HEADER_REPRESENTATION;
                             break;
                         default:
-                            // Check new header size against max header size
-                            if ((long) index + nameLength > maxHeaderListSizeGoAway - headersLength) {
-                                headerListSizeExceeded(maxHeaderListSizeGoAway);
-                            }
                             valueLength = index;
                             state = READ_LITERAL_HEADER_VALUE;
                     }
@@ -287,10 +283,6 @@ public void decode(int streamId, ByteBuf in, Http2Headers headers, boolean valid
                     // Header Value is a Literal String
                     valueLength = decodeULE128(in, index);
 
-                    // Check new header size against max header size
-                    if ((long) valueLength + nameLength > maxHeaderListSizeGoAway - headersLength) {
-                        headerListSizeExceeded(maxHeaderListSizeGoAway);
-                    }
                     state = READ_LITERAL_HEADER_VALUE;
                     break;
 
@@ -302,7 +294,7 @@ public void decode(int streamId, ByteBuf in, Http2Headers headers, boolean valid
 
                     CharSequence value = readStringLiteral(in, valueLength, huffmanEncoded);
                     headerType = validate(name, headerType, validateHeaders);
-                    headersLength = insertHeader(headers, name, value, indexType, headersLength);
+                    insertHeader(sink, name, value, indexType);
                     state = READ_HEADER_REPRESENTATION;
                     break;
 
@@ -311,13 +303,6 @@ public void decode(int streamId, ByteBuf in, Http2Headers headers, boolean valid
             }
         }
 
-        // we have read all of our headers, and not exceeded maxHeaderListSizeGoAway see if we have
-        // exceeded our actual maxHeaderListSize. This must be done here to prevent dynamic table
-        // corruption
-        if (headersLength > maxHeaderListSize) {
-            headerListSizeExceeded(streamId, maxHeaderListSize, true);
-        }
-
         if (state != READ_HEADER_REPRESENTATION) {
             throw connectionError(COMPRESSION_ERROR, "Incomplete header block fragment.");
         }
@@ -341,27 +326,27 @@ public void setMaxHeaderTableSize(long maxHeaderTableSize) throws Http2Exception
         }
     }
 
+    /**
+     * @deprecated use {@link #setmaxHeaderListSize(long)}; {@code maxHeaderListSizeGoAway} is
+     *     ignored
+     */
+    @Deprecated
     public void setMaxHeaderListSize(long maxHeaderListSize, long maxHeaderListSizeGoAway) throws Http2Exception {
-        if (maxHeaderListSizeGoAway < maxHeaderListSize || maxHeaderListSizeGoAway < 0) {
-            throw connectionError(INTERNAL_ERROR, "Header List Size GO_AWAY %d must be positive and >= %d",
-                    maxHeaderListSizeGoAway, maxHeaderListSize);
-        }
+        setMaxHeaderListSize(maxHeaderListSize);
+    }
+
+    public void setMaxHeaderListSize(long maxHeaderListSize) throws Http2Exception {
         if (maxHeaderListSize < MIN_HEADER_LIST_SIZE || maxHeaderListSize > MAX_HEADER_LIST_SIZE) {
             throw connectionError(PROTOCOL_ERROR, "Header List Size must be >= %d and <= %d but was %d",
                     MIN_HEADER_TABLE_SIZE, MAX_HEADER_TABLE_SIZE, maxHeaderListSize);
         }
         this.maxHeaderListSize = maxHeaderListSize;
-        this.maxHeaderListSizeGoAway = maxHeaderListSizeGoAway;
     }
 
     public long getMaxHeaderListSize() {
         return maxHeaderListSize;
     }
 
-    public long getMaxHeaderListSizeGoAway() {
-        return maxHeaderListSizeGoAway;
-    }
-
     /**
      * Return the maximum table size. This is the maximum size allowed by both the encoder and the
      * decoder.
@@ -450,9 +435,9 @@ private HpackHeaderField getIndexedHeader(int index) throws Http2Exception {
         throw INDEX_HEADER_ILLEGAL_INDEX_VALUE;
     }
 
-    private long insertHeader(Http2Headers headers, CharSequence name, CharSequence value,
-                              IndexType indexType, long headerSize) throws Http2Exception {
-        headerSize = addHeader(headers, name, value, headerSize);
+    private void insertHeader(Sink sink, CharSequence name, CharSequence value,
+                              IndexType indexType) throws Http2Exception {
+        sink.appendToHeaderList(name, value);
 
         switch (indexType) {
             case NONE:
@@ -466,18 +451,6 @@ private long insertHeader(Http2Headers headers, CharSequence name, CharSequence
             default:
                 throw new Error("should not reach here");
         }
-
-        return headerSize;
-    }
-
-    private long addHeader(Http2Headers headers, CharSequence name, CharSequence value, long headersLength)
-            throws Http2Exception {
-        headersLength += HpackHeaderField.sizeOf(name, value);
-        if (headersLength > maxHeaderListSizeGoAway) {
-            headerListSizeExceeded(maxHeaderListSizeGoAway);
-        }
-        headers.add(name, value);
-        return headersLength;
     }
 
     private CharSequence readStringLiteral(ByteBuf in, int length, boolean huffmanEncoded) throws Http2Exception {
@@ -553,4 +526,35 @@ private enum HeaderType {
         REQUEST_PSEUDO_HEADER,
         RESPONSE_PSEUDO_HEADER
     }
+
+    private interface Sink {
+        void appendToHeaderList(CharSequence name, CharSequence value);
+    }
+
+    private static final class Http2HeadersSink implements Sink {
+        private final Http2Headers headers;
+        private final long maxHeaderListSize;
+        private long headersLength;
+        private boolean exceededMaxLength;
+
+        public Http2HeadersSink(Http2Headers headers, long maxHeaderListSize) {
+            this.headers = headers;
+            this.maxHeaderListSize = maxHeaderListSize;
+        }
+
+        @Override
+        public void appendToHeaderList(CharSequence name, CharSequence value) {
+            headersLength += HpackHeaderField.sizeOf(name, value);
+            if (headersLength > maxHeaderListSize) {
+                exceededMaxLength = true;
+            }
+            if (!exceededMaxLength) {
+                headers.add(name, value);
+            }
+        }
+
+        public boolean exceededMaxLength() {
+            return exceededMaxLength;
+        }
+    }
 }


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
index 4652a1fceb6..ba0117553eb 100644
--- 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
@@ -431,15 +431,13 @@ public void testLiteralNeverIndexedWithLargeValue() throws Http2Exception {
     }
 
     @Test
-    public void testDecodeLargerThanMaxHeaderListSizeButSmallerThanMaxHeaderListSizeUpdatesDynamicTable()
-            throws Http2Exception {
+    public void testDecodeLargerThanMaxHeaderListSizeUpdatesDynamicTable() throws Http2Exception {
         ByteBuf in = Unpooled.buffer(300);
         try {
-            hpackDecoder.setMaxHeaderListSize(200, 300);
+            hpackDecoder.setMaxHeaderListSize(200);
             HpackEncoder hpackEncoder = new HpackEncoder(true);
 
             // encode headers that are slightly larger than maxHeaderListSize
-            // but smaller than maxHeaderListSizeGoAway
             Http2Headers toEncode = new DefaultHttp2Headers();
             toEncode.add("test_1", "1");
             toEncode.add("test_2", "2");
@@ -447,8 +445,7 @@ public void testDecodeLargerThanMaxHeaderListSizeButSmallerThanMaxHeaderListSize
             toEncode.add("test_3", "3");
             hpackEncoder.encodeHeaders(1, in, toEncode, NEVER_SENSITIVE);
 
-            // decode the headers, we should get an exception, but
-            // the decoded headers object should contain all of the headers
+            // decode the headers, we should get an exception
             Http2Headers decoded = new DefaultHttp2Headers();
             try {
                 hpackDecoder.decode(1, in, decoded, true);
@@ -457,8 +454,18 @@ public void testDecodeLargerThanMaxHeaderListSizeButSmallerThanMaxHeaderListSize
                 assertTrue(e instanceof Http2Exception.HeaderListSizeException);
             }
 
-            assertEquals(4, decoded.size());
-            assertTrue(decoded.contains("test_3"));
+            // but the dynamic table should have been updated, so that later blocks
+            // can refer to earlier headers
+            in.clear();
+            // 0x80, "indexed header field representation"
+            // index 62, the first (most recent) dynamic table entry
+            in.writeByte(0x80 | 62);
+            Http2Headers decoded2 = new DefaultHttp2Headers();
+            hpackDecoder.decode(1, in, decoded2, true);
+
+            Http2Headers golden = new DefaultHttp2Headers();
+            golden.add("test_3", "3");
+            assertEquals(golden, decoded2);
         } finally {
             in.release();
         }
@@ -468,11 +475,10 @@ public void testDecodeLargerThanMaxHeaderListSizeButSmallerThanMaxHeaderListSize
     public void testDecodeCountsNamesOnlyOnce() throws Http2Exception {
         ByteBuf in = Unpooled.buffer(200);
         try {
-            hpackDecoder.setMaxHeaderListSize(3500, 4000);
+            hpackDecoder.setMaxHeaderListSize(3500);
             HpackEncoder hpackEncoder = new HpackEncoder(true);
 
             // encode headers that are slightly larger than maxHeaderListSize
-            // but smaller than maxHeaderListSizeGoAway
             Http2Headers toEncode = new DefaultHttp2Headers();
             toEncode.add(String.format("%03000d", 0).replace('0', 'f'), "value");
             toEncode.add("accept", "value");
@@ -493,7 +499,7 @@ public void testAccountForHeaderOverhead() throws Exception {
             String headerName = "12345";
             String headerValue = "56789";
             long headerSize = headerName.length() + headerValue.length();
-            hpackDecoder.setMaxHeaderListSize(headerSize, 100);
+            hpackDecoder.setMaxHeaderListSize(headerSize);
             HpackEncoder hpackEncoder = new HpackEncoder(true);
 
             Http2Headers toEncode = new DefaultHttp2Headers();
diff --git a/codec-http2/src/test/java/io/netty/handler/codec/http2/HpackEncoderTest.java b/codec-http2/src/test/java/io/netty/handler/codec/http2/HpackEncoderTest.java
index b1dce05c38b..de049a6e506 100644
--- a/codec-http2/src/test/java/io/netty/handler/codec/http2/HpackEncoderTest.java
+++ b/codec-http2/src/test/java/io/netty/handler/codec/http2/HpackEncoderTest.java
@@ -74,7 +74,7 @@ public void testWillEncode16MBHeaderByDefault() throws Http2Exception {
 
         try {
             hpackEncoder.encodeHeaders(0, buf, headersIn, Http2HeadersEncoder.NEVER_SENSITIVE);
-            hpackDecoder.setMaxHeaderListSize(bigHeaderSize + 1024, bigHeaderSize + 1024);
+            hpackDecoder.setMaxHeaderListSize(bigHeaderSize + 1024);
             hpackDecoder.decode(0, buf, headersOut, false);
         } finally {
             buf.release();