Repository: netty/netty
Issue: HTTP/2: SETTINGS_HEADER_TABLE_SIZE should be an unsigned int
Labels: defect

The HTTP/2 spec demands that the max value for `SETTINGS_HEADER_TABLE_SIZE` should be an unsigned 32-bit integer. However, it seems that some [limitations in HPACK](https://github.com/netty/netty/blob/4.1/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2CodecUtil.java#L90) prevent us from supporting that.

h2spec test suite 6.5 fails due to this limitation. The exception is `Setting HEADER_TABLE_SIZE is invalid: 4294967295`.

```
        × Sends a SETTINGS frame
          - The endpoint MUST sends a SETTINGS frame with ACK.
            Expected: SETTINGS frame (Flags: 1)
              Actual: GOAWAY frame (Length: 56, Flags: 0, ErrorCode: PROTOCOL_ERROR)
```


Issue comments:
@buchgr will look into it.

Should be fixed by https://github.com/netty/netty/pull/5798

Fixed by https://github.com/netty/netty/pull/5798


Pull request review comments:
This will first cast `capacity` to int, then do the division. I think you need to wrap the division with parantheses?

doh! right... 

This cast doesn't seem safe, for `maxHeaderTableSize` gt `MAX_INT`?

@buchgr why ? Shouldn't  it just be converted to a signed int ?

@normanmaurer what if it doesn't fit a signed int?

@buchgr good point... Maybe we should do a range check in `setMaxHeaderTableSize(...)` and check its not bigger then `0xFFFFFFFFL`?

@normanmaurer I think that would be good.

done

Does this need to check if capacity is greater than 1<<32 ?

yep let me check for uint as well

Does here need to check as well?

done

done

@normanmaurer You should probably also add a comment to this line, explaining why it's safe.

@normanmaurer - it seems like this may be problematic if `maxHeaderTableSize > Integer.MAX_INT`. Do we have a unit test covering this case?

@normanmaurer - should this be of type `long` now?

should we instead check around the bounds `MAX_HEADER_TABLE_SIZE` and `MAX_HEADER_TABLE_SIZE+1`?


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 037cd9c9038..a20ecda44ca 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
@@ -23,6 +23,8 @@
 
 import static io.netty.handler.codec.http2.Http2CodecUtil.DEFAULT_HEADER_TABLE_SIZE;
 import static io.netty.handler.codec.http2.Http2CodecUtil.DEFAULT_MAX_HEADER_SIZE;
+import static io.netty.handler.codec.http2.Http2CodecUtil.MAX_HEADER_TABLE_SIZE;
+import static io.netty.handler.codec.http2.Http2CodecUtil.MIN_HEADER_TABLE_SIZE;
 import static io.netty.handler.codec.http2.Http2Error.COMPRESSION_ERROR;
 import static io.netty.handler.codec.http2.Http2Error.PROTOCOL_ERROR;
 import static io.netty.handler.codec.http2.Http2Exception.connectionError;
@@ -99,9 +101,10 @@ public Http2Headers decodeHeaders(ByteBuf headerBlock) throws Http2Exception {
      */
     private final class Http2HeaderTableDecoder extends DefaultHttp2HeaderTableListSize implements Http2HeaderTable {
         @Override
-        public void maxHeaderTableSize(int max) throws Http2Exception {
-            if (max < 0) {
-                throw connectionError(PROTOCOL_ERROR, "Header Table Size must be non-negative but was %d", max);
+        public void maxHeaderTableSize(long max) throws Http2Exception {
+            if (max < MIN_HEADER_TABLE_SIZE || max > MAX_HEADER_TABLE_SIZE) {
+                throw connectionError(PROTOCOL_ERROR, "Header Table Size must be >= %d and <= %d but was %d",
+                        MIN_HEADER_TABLE_SIZE, MAX_HEADER_TABLE_SIZE, max);
             }
             try {
                 decoder.setMaxHeaderTableSize(max);
@@ -111,7 +114,7 @@ public void maxHeaderTableSize(int max) throws Http2Exception {
         }
 
         @Override
-        public int maxHeaderTableSize() {
+        public long maxHeaderTableSize() {
             return decoder.getMaxHeaderTableSize();
         }
     }
diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2HeadersEncoder.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2HeadersEncoder.java
index 534e37b4d92..2157fc9b404 100644
--- a/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2HeadersEncoder.java
+++ b/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2HeadersEncoder.java
@@ -23,6 +23,8 @@
 import java.util.Map.Entry;
 
 import static io.netty.handler.codec.http2.Http2CodecUtil.DEFAULT_HEADER_TABLE_SIZE;
+import static io.netty.handler.codec.http2.Http2CodecUtil.MAX_HEADER_TABLE_SIZE;
+import static io.netty.handler.codec.http2.Http2CodecUtil.MIN_HEADER_TABLE_SIZE;
 import static io.netty.handler.codec.http2.Http2Error.COMPRESSION_ERROR;
 import static io.netty.handler.codec.http2.Http2Error.PROTOCOL_ERROR;
 import static io.netty.handler.codec.http2.Http2Exception.connectionError;
@@ -89,9 +91,10 @@ private void encodeHeader(ByteBuf out, CharSequence key, CharSequence value) {
      */
     private final class Http2HeaderTableEncoder extends DefaultHttp2HeaderTableListSize implements Http2HeaderTable {
         @Override
-        public void maxHeaderTableSize(int max) throws Http2Exception {
-            if (max < 0) {
-                throw connectionError(PROTOCOL_ERROR, "Header Table Size must be non-negative but was %d", max);
+        public void maxHeaderTableSize(long max) throws Http2Exception {
+            if (max < MIN_HEADER_TABLE_SIZE || max > MAX_HEADER_TABLE_SIZE) {
+                throw connectionError(PROTOCOL_ERROR, "Header Table Size must be >= %d and <= %d but was %d",
+                        MIN_HEADER_TABLE_SIZE, MAX_HEADER_TABLE_SIZE, max);
             }
             try {
                 // No headers should be emitted. If they are, we throw.
@@ -102,7 +105,7 @@ public void maxHeaderTableSize(int max) throws Http2Exception {
         }
 
         @Override
-        public int maxHeaderTableSize() {
+        public long maxHeaderTableSize() {
             return encoder.getMaxHeaderTableSize();
         }
     }
diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2CodecUtil.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2CodecUtil.java
index 8079bd00f7a..5a4c91ee89b 100644
--- a/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2CodecUtil.java
+++ b/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2CodecUtil.java
@@ -87,7 +87,7 @@ public final class Http2CodecUtil {
     public static final char SETTINGS_MAX_HEADER_LIST_SIZE = 6;
     public static final int NUM_STANDARD_SETTINGS = 6;
 
-    public static final int MAX_HEADER_TABLE_SIZE = Integer.MAX_VALUE; // Size limited by HPACK library
+    public static final long MAX_HEADER_TABLE_SIZE = MAX_UNSIGNED_INT;
     public static final long MAX_CONCURRENT_STREAMS = MAX_UNSIGNED_INT;
     public static final int MAX_INITIAL_WINDOW_SIZE = Integer.MAX_VALUE;
     public static final int MAX_FRAME_SIZE_LOWER_BOUND = 0x4000;
diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2HeaderTable.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2HeaderTable.java
index b9636471db1..b009a7fe2e7 100644
--- a/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2HeaderTable.java
+++ b/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2HeaderTable.java
@@ -24,12 +24,12 @@ public interface Http2HeaderTable {
     /**
      * Sets the maximum size of the HPACK header table used for decoding HTTP/2 headers.
      */
-    void maxHeaderTableSize(int max) throws Http2Exception;
+    void maxHeaderTableSize(long max) throws Http2Exception;
 
     /**
      * Gets the maximum size of the HPACK header table used for decoding HTTP/2 headers.
      */
-    int maxHeaderTableSize();
+    long maxHeaderTableSize();
 
     /**
      * Sets the maximum allowed header elements.
diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2Settings.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2Settings.java
index 39695942057..7ef0c4c8e8d 100644
--- a/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2Settings.java
+++ b/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2Settings.java
@@ -87,7 +87,7 @@ public Long headerTableSize() {
      *
      * @throws IllegalArgumentException if verification of the setting fails.
      */
-    public Http2Settings headerTableSize(int value) {
+    public Http2Settings headerTableSize(long value) {
         put(SETTINGS_HEADER_TABLE_SIZE, Long.valueOf(value));
         return this;
     }
diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/internal/hpack/Decoder.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/internal/hpack/Decoder.java
index b67a9dc4ca2..a4dc6c0be22 100644
--- a/codec-http2/src/main/java/io/netty/handler/codec/http2/internal/hpack/Decoder.java
+++ b/codec-http2/src/main/java/io/netty/handler/codec/http2/internal/hpack/Decoder.java
@@ -74,7 +74,7 @@ public final class Decoder {
     private final DynamicTable dynamicTable;
     private final HuffmanDecoder huffmanDecoder;
     private final int maxHeadersLength;
-    private int maxDynamicTableSize;
+    private long maxDynamicTableSize;
     private int encoderMaxDynamicTableSize;
     private boolean maxDynamicTableSizeChangeRequired;
 
@@ -305,7 +305,7 @@ public void decode(ByteBuf in, Http2Headers headers) throws Http2Exception {
      * Set the maximum table size. If this is below the maximum size of the dynamic table used by
      * the encoder, the beginning of the next header block MUST signal this change.
      */
-    public void setMaxHeaderTableSize(int maxHeaderTableSize) {
+    public void setMaxHeaderTableSize(long maxHeaderTableSize) {
         maxDynamicTableSize = maxHeaderTableSize;
         if (maxDynamicTableSize < encoderMaxDynamicTableSize) {
             // decoder requires less space than encoder
@@ -319,7 +319,7 @@ public void setMaxHeaderTableSize(int maxHeaderTableSize) {
      * Return the maximum table size. This is the maximum size allowed by both the encoder and the
      * decoder.
      */
-    public int getMaxHeaderTableSize() {
+    public long getMaxHeaderTableSize() {
         return dynamicTable.capacity();
     }
 
@@ -333,7 +333,7 @@ int length() {
     /**
      * Return the size of the dynamic table. Exposed for testing.
      */
-    int size() {
+    long size() {
         return dynamicTable.size();
     }
 
diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/internal/hpack/DynamicTable.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/internal/hpack/DynamicTable.java
index a623b5b29a5..a8a67c037a2 100644
--- a/codec-http2/src/main/java/io/netty/handler/codec/http2/internal/hpack/DynamicTable.java
+++ b/codec-http2/src/main/java/io/netty/handler/codec/http2/internal/hpack/DynamicTable.java
@@ -31,6 +31,8 @@
  */
 package io.netty.handler.codec.http2.internal.hpack;
 
+import static io.netty.handler.codec.http2.Http2CodecUtil.MAX_HEADER_TABLE_SIZE;
+import static io.netty.handler.codec.http2.Http2CodecUtil.MIN_HEADER_TABLE_SIZE;
 import static io.netty.handler.codec.http2.internal.hpack.HeaderField.HEADER_ENTRY_OVERHEAD;
 
 final class DynamicTable {
@@ -40,7 +42,7 @@ final class DynamicTable {
     int head;
     int tail;
     private int size;
-    private int capacity = -1; // ensure setCapacity creates the array
+    private long capacity = -1; // ensure setCapacity creates the array
 
     /**
      * Creates a new dynamic table with the specified initial capacity.
@@ -65,14 +67,14 @@ public int length() {
     /**
      * Return the current size of the dynamic table. This is the sum of the size of the entries.
      */
-    public int size() {
+    public long size() {
         return size;
     }
 
     /**
      * Return the maximum allowable size of the dynamic table.
      */
-    public int capacity() {
+    public long capacity() {
         return capacity;
     }
 
@@ -149,11 +151,10 @@ public void clear() {
      * Set the maximum size of the dynamic table. Entries are evicted from the dynamic table until
      * the size of the table is less than or equal to the maximum size.
      */
-    public void setCapacity(int capacity) {
-        if (capacity < 0) {
-            throw new IllegalArgumentException("Illegal Capacity: " + capacity);
+    public void setCapacity(long capacity) {
+        if (capacity < MIN_HEADER_TABLE_SIZE || capacity > MAX_HEADER_TABLE_SIZE) {
+            throw new IllegalArgumentException("capacity is invalid: " + capacity);
         }
-
         // initially capacity will be -1 so init won't return here
         if (this.capacity == capacity) {
             return;
@@ -169,7 +170,7 @@ public void setCapacity(int capacity) {
             }
         }
 
-        int maxEntries = capacity / HEADER_ENTRY_OVERHEAD;
+        int maxEntries = (int) (capacity / HEADER_ENTRY_OVERHEAD);
         if (capacity % HEADER_ENTRY_OVERHEAD != 0) {
             maxEntries++;
         }
@@ -192,8 +193,8 @@ public void setCapacity(int capacity) {
             }
         }
 
-        this.tail = 0;
-        this.head = tail + len;
-        this.headerFields = tmp;
+        tail = 0;
+        head = tail + len;
+        headerFields = tmp;
     }
 }
diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/internal/hpack/Encoder.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/internal/hpack/Encoder.java
index a4c9e86140a..c28a950e05e 100644
--- a/codec-http2/src/main/java/io/netty/handler/codec/http2/internal/hpack/Encoder.java
+++ b/codec-http2/src/main/java/io/netty/handler/codec/http2/internal/hpack/Encoder.java
@@ -37,6 +37,8 @@
 
 import java.util.Arrays;
 
+import static io.netty.handler.codec.http2.Http2CodecUtil.MAX_HEADER_TABLE_SIZE;
+import static io.netty.handler.codec.http2.Http2CodecUtil.MIN_HEADER_TABLE_SIZE;
 import static io.netty.handler.codec.http2.internal.hpack.HpackUtil.IndexType.INCREMENTAL;
 import static io.netty.handler.codec.http2.internal.hpack.HpackUtil.IndexType.NEVER;
 import static io.netty.handler.codec.http2.internal.hpack.HpackUtil.IndexType.NONE;
@@ -52,22 +54,22 @@ public final class Encoder {
             AsciiString.EMPTY_STRING, Integer.MAX_VALUE, null);
     private final HuffmanEncoder huffmanEncoder = new HuffmanEncoder();
     private final byte hashMask;
-    private int size;
-    private int capacity;
+    private long size;
+    private long capacity;
 
     /**
      * Creates a new encoder.
      */
-    public Encoder(int maxHeaderTableSize) {
+    public Encoder(long maxHeaderTableSize) {
         this(maxHeaderTableSize, 16);
     }
 
     /**
      * Creates a new encoder.
      */
-    public Encoder(int maxHeaderTableSize, int arraySizeHint) {
-        if (maxHeaderTableSize < 0) {
-            throw new IllegalArgumentException("Illegal Capacity: " + maxHeaderTableSize);
+    public Encoder(long maxHeaderTableSize, int arraySizeHint) {
+        if (maxHeaderTableSize < MIN_HEADER_TABLE_SIZE || maxHeaderTableSize > MAX_HEADER_TABLE_SIZE) {
+            throw new IllegalArgumentException("maxHeaderTableSize is invalid: " + maxHeaderTableSize);
         }
         capacity = maxHeaderTableSize;
         // Enforce a bound of [2, 128] because hashMask is a byte. The max possible value of hashMask is one less
@@ -133,22 +135,23 @@ public void encodeHeader(ByteBuf out, CharSequence name, CharSequence value, boo
     /**
      * Set the maximum table size.
      */
-    public void setMaxHeaderTableSize(ByteBuf out, int maxHeaderTableSize) {
-        if (maxHeaderTableSize < 0) {
-            throw new IllegalArgumentException("Illegal Capacity: " + maxHeaderTableSize);
+    public void setMaxHeaderTableSize(ByteBuf out, long maxHeaderTableSize) {
+        if (maxHeaderTableSize < MIN_HEADER_TABLE_SIZE || maxHeaderTableSize > MAX_HEADER_TABLE_SIZE) {
+            throw new IllegalArgumentException("maxHeaderTableSize is invalid: " + maxHeaderTableSize);
         }
         if (capacity == maxHeaderTableSize) {
             return;
         }
         capacity = maxHeaderTableSize;
         ensureCapacity(0);
-        encodeInteger(out, 0x20, 5, maxHeaderTableSize);
+        // Casting to integer is safe as we verified the maxHeaderTableSize is a valid unsigned int.
+        encodeInteger(out, 0x20, 5, (int) maxHeaderTableSize);
     }
 
     /**
      * Return the maximum table size.
      */
-    public int getMaxHeaderTableSize() {
+    public long getMaxHeaderTableSize() {
         return capacity;
     }
 
@@ -252,7 +255,7 @@ int length() {
     /**
      * Return the size of the dynamic table. Exposed for testing.
      */
-    int size() {
+    long size() {
         return size;
     }
 


Test patch:
diff --git a/codec-http2/src/test/java/io/netty/handler/codec/http2/Http2SettingsTest.java b/codec-http2/src/test/java/io/netty/handler/codec/http2/Http2SettingsTest.java
index cb33ca136e0..514d53e8770 100644
--- a/codec-http2/src/test/java/io/netty/handler/codec/http2/Http2SettingsTest.java
+++ b/codec-http2/src/test/java/io/netty/handler/codec/http2/Http2SettingsTest.java
@@ -20,6 +20,7 @@
 import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertTrue;
 
+import io.netty.handler.codec.http.HttpUtil;
 import org.junit.Before;
 import org.junit.Test;
 
@@ -86,4 +87,21 @@ public void boundarySettingsShouldBeSet() {
         settings.maxHeaderListSize((int) settingsValueUpperBound);
         assertEquals(Integer.MAX_VALUE, (long) settings.maxHeaderListSize());
     }
+
+    @Test
+    public void headerTableSizeUnsignedInt() {
+        final long value = 1L << 31;
+        settings.put(Http2CodecUtil.SETTINGS_HEADER_TABLE_SIZE, (Long) value);
+        assertEquals(value, (long) settings.get(Http2CodecUtil.SETTINGS_HEADER_TABLE_SIZE));
+    }
+
+    @Test(expected = IllegalArgumentException.class)
+    public void headerTableSizeBoundCheck() {
+        settings.put(Http2CodecUtil.SETTINGS_HEADER_TABLE_SIZE, (Long) Long.MAX_VALUE);
+    }
+
+    @Test(expected = IllegalArgumentException.class)
+    public void headerTableSizeBoundCheck2() {
+        settings.put(Http2CodecUtil.SETTINGS_HEADER_TABLE_SIZE, Long.valueOf(-1L));
+    }
 }