HTTP/2 Max Header List Size Bug

Motivation:
If the HPACK Decoder detects that SETTINGS_MAX_HEADER_LIST_SIZE has been violated it aborts immediately and sends a RST_STREAM frame for what ever stream caused the issue. Because HPACK is stateful this means that the HPACK state may become out of sync between peers, and the issue won't be detected until the next headers frame. We should make a best effort to keep processing to keep the HPACK state in sync with our peer, or completely close the connection.
If the HPACK Encoder is configured to verify SETTINGS_MAX_HEADER_LIST_SIZE it checks the limit and encodes at the same time. This may result in modifying the HPACK local state but not sending the headers to the peer if SETTINGS_MAX_HEADER_LIST_SIZE is violated. This will also lead to an inconsistency in HPACK state that will be flagged at some later time.

Modifications:
- HPACK Decoder now has 2 levels of limits related to SETTINGS_MAX_HEADER_LIST_SIZE. The first will attempt to keep processing data and send a RST_STREAM after all data is processed. The second will send a GO_AWAY and close the entire connection.
- When the HPACK Encoder enforces SETTINGS_MAX_HEADER_LIST_SIZE it should not modify the HPACK state until the size has been checked.
- https://tools.ietf.org/html/rfc7540#section-6.5.2 states that the initial value of SETTINGS_MAX_HEADER_LIST_SIZE is "unlimited". We currently use 8k as a limit. We should honor the specifications default value so we don't unintentionally close a connection before the remote peer is aware of the local settings.
- Remove unnecessary object allocation in DefaultHttp2HeadersDecoder and DefaultHttp2HeadersEncoder.

Result:
Fixes netty#6209.

codec-http2/src/main/java/io/netty/handler/codec/http2/AbstractHttp2ConnectionHandlerBuilder.java

@@ -19,6 +19,7 @@
 import io.netty.handler.codec.http2.Http2HeadersEncoder.SensitivityDetector;
 import io.netty.util.internal.UnstableApi;
 
+import static io.netty.handler.codec.http2.Http2CodecUtil.DEFAULT_HEADER_LIST_SIZE;
 import static io.netty.util.internal.ObjectUtil.checkNotNull;
 import static java.util.concurrent.TimeUnit.MILLISECONDS;
 import static java.util.concurrent.TimeUnit.SECONDS;
@@ -76,7 +77,7 @@ public abstract class AbstractHttp2ConnectionHandlerBuilder<T extends Http2Conne
     private static final SensitivityDetector DEFAULT_HEADER_SENSITIVITY_DETECTOR = Http2HeadersEncoder.NEVER_SENSITIVE;
 
     // The properties that can always be set.
-    private Http2Settings initialSettings = new Http2Settings();
+    private Http2Settings initialSettings = new Http2Settings().maxHeaderListSize(DEFAULT_HEADER_LIST_SIZE);
     private Http2FrameListener frameListener;
     private long gracefulShutdownTimeoutMillis = DEFAULT_GRACEFUL_SHUTDOWN_TIMEOUT_MILLIS;
 
@@ -334,7 +335,10 @@ protected T build() {
     }
 
     private T buildFromConnection(Http2Connection connection) {
-        Http2FrameReader reader = new DefaultHttp2FrameReader(isValidateHeaders());
+        Long maxHeaderListSize = initialSettings.maxHeaderListSize();
+        Http2FrameReader reader = new DefaultHttp2FrameReader(maxHeaderListSize == null ?
+                new DefaultHttp2HeadersDecoder(isValidateHeaders()) :
+                new DefaultHttp2HeadersDecoder(isValidateHeaders(), maxHeaderListSize));
         Http2FrameWriter writer = encoderIgnoreMaxHeaderListSize == null ?
                 new DefaultHttp2FrameWriter(headerSensitivityDetector()) :
                 new DefaultHttp2FrameWriter(headerSensitivityDetector(), encoderIgnoreMaxHeaderListSize);

codec-http2/src/main/java/io/netty/handler/codec/http2/DecoratingHttp2FrameWriter.java

@@ -14,14 +14,14 @@
  */
 package io.netty.handler.codec.http2;
 
-import static io.netty.util.internal.ObjectUtil.checkNotNull;
-
 import io.netty.buffer.ByteBuf;
 import io.netty.channel.ChannelFuture;
 import io.netty.channel.ChannelHandlerContext;
 import io.netty.channel.ChannelPromise;
 import io.netty.util.internal.UnstableApi;
 
+import static io.netty.util.internal.ObjectUtil.checkNotNull;
+
 /**
  * Decorator around another {@link Http2FrameWriter} instance.
  */
@@ -41,14 +41,14 @@ public ChannelFuture writeData(ChannelHandlerContext ctx, int streamId, ByteBuf
 
     @Override
     public ChannelFuture writeHeaders(ChannelHandlerContext ctx, int streamId, Http2Headers headers, int padding,
-                                      boolean endStream, ChannelPromise promise) {
+                                      boolean endStream, ChannelPromise promise) throws Http2Exception {
         return delegate.writeHeaders(ctx, streamId, headers, padding, endStream, promise);
     }
 
     @Override
     public ChannelFuture writeHeaders(ChannelHandlerContext ctx, int streamId, Http2Headers headers,
                                       int streamDependency, short weight, boolean exclusive, int padding,
-                                      boolean endStream, ChannelPromise promise) {
+                                      boolean endStream, ChannelPromise promise) throws Http2Exception {
         return delegate
                 .writeHeaders(ctx, streamId, headers, streamDependency, weight, exclusive, padding, endStream, promise);
     }
@@ -82,7 +82,8 @@ public ChannelFuture writePing(ChannelHandlerContext ctx, boolean ack, ByteBuf d
 
     @Override
     public ChannelFuture writePushPromise(ChannelHandlerContext ctx, int streamId, int promisedStreamId,
-                                          Http2Headers headers, int padding, ChannelPromise promise) {
+                                          Http2Headers headers, int padding, ChannelPromise promise)
+            throws Http2Exception {
         return delegate.writePushPromise(ctx, streamId, promisedStreamId, headers, padding, promise);
     }
 

codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2Connection.java

@@ -369,14 +369,16 @@ final DefaultPropertyKey verifyKey(PropertyKey key) {
      * Simple stream implementation. Streams can be compared to each other by priority.
      */
     private class DefaultStream implements Http2Stream {
+        private static final byte SENT_STATE_RST = 0x1;
+        private static final byte SENT_STATE_HEADERS = 0x2;
+        private static final byte SENT_STATE_PUSHPROMISE = 0x4;
         private final int id;
         private final PropertyMap properties = new PropertyMap();
         private State state;
         private short weight = DEFAULT_PRIORITY_WEIGHT;
         private DefaultStream parent;
         private IntObjectMap<DefaultStream> children = IntCollections.emptyMap();
-        private boolean resetSent;
-        private boolean headersSent;
+        private byte sentState;
 
         DefaultStream(int id, State state) {
             this.id = id;
@@ -395,24 +397,35 @@ public final State state() {
 
         @Override
         public boolean isResetSent() {
-            return resetSent;
+            return (sentState & SENT_STATE_RST) != 0;
         }
 
         @Override
         public Http2Stream resetSent() {
-            resetSent = true;
+            sentState |= SENT_STATE_RST;
             return this;
         }
 
         @Override
         public Http2Stream headersSent() {
-            headersSent = true;
+            sentState |= SENT_STATE_HEADERS;
             return this;
         }
 
         @Override
         public boolean isHeadersSent() {
-            return headersSent;
+            return (sentState & SENT_STATE_HEADERS) != 0;
+        }
+
+        @Override
+        public Http2Stream pushPromiseSent() {
+            sentState |= SENT_STATE_PUSHPROMISE;
+            return this;
+        }
+
+        @Override
+        public boolean isPushPromiseSent() {
+            return (sentState & SENT_STATE_PUSHPROMISE) != 0;
         }
 
         @Override
@@ -837,6 +850,16 @@ public Http2Stream headersSent() {
         public boolean isHeadersSent() {
             throw new UnsupportedOperationException();
         }
+
+        @Override
+        public Http2Stream pushPromiseSent() {
+            throw new UnsupportedOperationException();
+        }
+
+        @Override
+        public boolean isPushPromiseSent() {
+            throw new UnsupportedOperationException();
+        }
     }
 
     /**

codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2ConnectionDecoder.java

@@ -122,13 +122,13 @@ public void decodeFrame(ChannelHandlerContext ctx, ByteBuf in, List<Object> out)
     public Http2Settings localSettings() {
         Http2Settings settings = new Http2Settings();
         Http2FrameReader.Configuration config = frameReader.configuration();
-        Http2HeaderTable headerTable = config.headerTable();
+        Http2HeadersDecoder.Configuration headersConfig = config.headersConfiguration();
         Http2FrameSizePolicy frameSizePolicy = config.frameSizePolicy();
         settings.initialWindowSize(flowController().initialWindowSize());
         settings.maxConcurrentStreams(connection.remote().maxActiveStreams());
-        settings.headerTableSize(headerTable.maxHeaderTableSize());
+        settings.headerTableSize(headersConfig.maxHeaderTableSize());
         settings.maxFrameSize(frameSizePolicy.maxFrameSize());
-        settings.maxHeaderListSize(headerTable.maxHeaderListSize());
+        settings.maxHeaderListSize(headersConfig.maxHeaderListSize());
         if (!connection.isServer()) {
             // Only set the pushEnabled flag if this is a client endpoint.
             settings.pushEnabled(connection.local().allowPushTo());
@@ -141,6 +141,18 @@ public void close() {
         frameReader.close();
     }
 
+    /**
+     * Calculate the threshold in bytes which should trigger a {@code GO_AWAY} if a set of headers exceeds this amount.
+     * @param maxHeaderListSize
+     *      <a href="https://tools.ietf.org/html/rfc7540#section-6.5.2">SETTINGS_MAX_HEADER_LIST_SIZE</a> for the local
+     *      endpoint.
+     * @return the threshold in bytes which should trigger a {@code GO_AWAY} if a set of headers exceeds this amount.
+     */
+    protected long calculateMaxHeaderListSizeGoAway(long maxHeaderListSize) {
+        // This is equivalent to `maxHeaderListSize * 1.25` but we avoid floating point multiplication.
+        return maxHeaderListSize + (maxHeaderListSize >>> 2);
+    }
+
     private int unconsumedBytes(Http2Stream stream) {
         return flowController().unconsumedBytes(stream);
     }
@@ -383,11 +395,13 @@ public void onSettingsAckRead(ChannelHandlerContext ctx) throws Http2Exception {
 
         /**
          * Applies settings sent from the local endpoint.
+         * <p>
+         * This method is only called after the local settings have been acknowledged from the remote endpoint.
          */
         private void applyLocalSettings(Http2Settings settings) throws Http2Exception {
             Boolean pushEnabled = settings.pushEnabled();
             final Http2FrameReader.Configuration config = frameReader.configuration();
-            final Http2HeaderTable headerTable = config.headerTable();
+            final Http2HeadersDecoder.Configuration headerConfig = config.headersConfiguration();
             final Http2FrameSizePolicy frameSizePolicy = config.frameSizePolicy();
             if (pushEnabled != null) {
                 if (connection.isServer()) {
@@ -404,12 +418,12 @@ private void applyLocalSettings(Http2Settings settings) throws Http2Exception {
 
             Long headerTableSize = settings.headerTableSize();
             if (headerTableSize != null) {
-                headerTable.maxHeaderTableSize(headerTableSize);
+                headerConfig.maxHeaderTableSize(headerTableSize);
             }
 
             Long maxHeaderListSize = settings.maxHeaderListSize();
             if (maxHeaderListSize != null) {
-                headerTable.maxHeaderListSize(maxHeaderListSize);
+                headerConfig.maxHeaderListSize(maxHeaderListSize, calculateMaxHeaderListSizeGoAway(maxHeaderListSize));
             }
 
             Integer maxFrameSize = settings.maxFrameSize();

codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2ConnectionEncoder.java

@@ -77,7 +77,7 @@ public final Http2RemoteFlowController flowController() {
     public void remoteSettings(Http2Settings settings) throws Http2Exception {
         Boolean pushEnabled = settings.pushEnabled();
         Http2FrameWriter.Configuration config = configuration();
-        Http2HeaderTable outboundHeaderTable = config.headerTable();
+        Http2HeadersEncoder.Configuration outboundHeaderConfig = config.headersConfiguration();
         Http2FrameSizePolicy outboundFrameSizePolicy = config.frameSizePolicy();
         if (pushEnabled != null) {
             if (!connection.isServer() && pushEnabled) {
@@ -95,12 +95,12 @@ public void remoteSettings(Http2Settings settings) throws Http2Exception {
 
         Long headerTableSize = settings.headerTableSize();
         if (headerTableSize != null) {
-            outboundHeaderTable.maxHeaderTableSize((int) min(headerTableSize, MAX_VALUE));
+            outboundHeaderConfig.maxHeaderTableSize((int) min(headerTableSize, MAX_VALUE));
         }
 
         Long maxHeaderListSize = settings.maxHeaderListSize();
         if (maxHeaderListSize != null) {
-            outboundHeaderTable.maxHeaderListSize(maxHeaderListSize);
+            outboundHeaderConfig.maxHeaderListSize(maxHeaderListSize);
         }
 
         Integer maxFrameSize = settings.maxFrameSize();
@@ -197,14 +197,12 @@ public void operationComplete(ChannelFuture future) throws Exception {
                 flowController.addFlowControlled(stream,
                         new FlowControlledHeaders(stream, headers, streamDependency, weight, exclusive, padding,
                                                  endOfStream, promise));
-                stream.headersSent();
                 return promise;
             }
-        } catch (Http2NoMoreStreamIdsException e) {
-            lifecycleManager.onError(ctx, e);
-            return promise.setFailure(e);
-        } catch (Throwable e) {
-            return promise.setFailure(e);
+        } catch (Throwable t) {
+            lifecycleManager.onError(ctx, t);
+            promise.tryFailure(t);
+            return promise;
         }
     }
 
@@ -275,11 +273,16 @@ public ChannelFuture writePushPromise(ChannelHandlerContext ctx, int streamId, i
             Http2Stream stream = requireStream(streamId);
             // Reserve the promised stream.
             connection.local().reservePushStream(promisedStreamId, stream);
-        } catch (Throwable e) {
-            return promise.setFailure(e);
-        }
 
-        return frameWriter.writePushPromise(ctx, streamId, promisedStreamId, headers, padding, promise);
+            ChannelFuture future = frameWriter.writePushPromise(ctx, streamId, promisedStreamId, headers, padding,
+                                                                promise);
+            stream.pushPromiseSent();
+            return future;
+        } catch (Throwable t) {
+            lifecycleManager.onError(ctx, t);
+            promise.tryFailure(t);
+            return promise;
+        }
     }
 
     @Override
@@ -447,14 +450,15 @@ public void error(ChannelHandlerContext ctx, Throwable cause) {
         }
 
         @Override
-        public void write(ChannelHandlerContext ctx, int allowedBytes) {
+        public void write(ChannelHandlerContext ctx, int allowedBytes) throws Http2Exception {
             if (promise.isVoid()) {
                 promise = ctx.newPromise();
             }
             promise.addListener(this);
 
             frameWriter.writeHeaders(ctx, stream.id(), headers, streamDependency, weight, exclusive,
                     padding, endOfStream, promise);
+            stream.headersSent();
         }
 
         @Override

codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2FrameReader.java

@@ -95,8 +95,8 @@ public DefaultHttp2FrameReader(Http2HeadersDecoder headersDecoder) {
     }
 
     @Override
-    public Http2HeaderTable headerTable() {
-        return headersDecoder.configuration().headerTable();
+    public Http2HeadersDecoder.Configuration headersConfiguration() {
+        return headersDecoder.configuration();
     }
 
     @Override
@@ -673,7 +673,7 @@ protected class HeadersBlockBuilder {
          */
         private void headerSizeExceeded() throws Http2Exception {
             close();
-            headerListSizeExceeded(streamId, headersDecoder.configuration().headerTable().maxHeaderListSize(), true);
+            headerListSizeExceeded(headersDecoder.configuration().maxHeaderListSizeGoAway());
         }
 
         /**
@@ -687,7 +687,7 @@ private void headerSizeExceeded() throws Http2Exception {
          */
         final void addFragment(ByteBuf fragment, ByteBufAllocator alloc, boolean endOfHeaders) throws Http2Exception {
             if (headerBlock == null) {
-                if (fragment.readableBytes() > headersDecoder.configuration().headerTable().maxHeaderListSize()) {
+                if (fragment.readableBytes() > headersDecoder.configuration().maxHeaderListSizeGoAway()) {
                     headerSizeExceeded();
                 }
                 if (endOfHeaders) {
@@ -700,7 +700,7 @@ final void addFragment(ByteBuf fragment, ByteBufAllocator alloc, boolean endOfHe
                 }
                 return;
             }
-            if (headersDecoder.configuration().headerTable().maxHeaderListSize() - fragment.readableBytes() <
+            if (headersDecoder.configuration().maxHeaderListSizeGoAway() - fragment.readableBytes() <
                     headerBlock.readableBytes()) {
                 headerSizeExceeded();
             }