Repository: netty/netty
Issue: HTTP/2 disallows non-standard settings
Labels: defect

Currently the `Http2Settings` class disallows setting non-standard settings, which goes against the spec.


Issue comments:

Pull request review comments:
@nmittler why you changed this to public ? Was this by mistake ?

@nmittler - Correct me if I'm wrong but I'll take a shot...

@normanmaurer - I think because before this change we only allowed access to the "standard" set of settings (only 6 of them) via the other public methods in this class (i.e. `maxHeaderListSize`).

@nmittler - We should consider the duplication between this and `Long get(int)`.  The [spec](https://tools.ietf.org/html/draft-ietf-httpbis-http2-17#section-6.5.1) calls for `value` being `an unsigned 32-bit value.`....we should call this out in the comments if we decide to expose this method publicly.  We may not want to expose this and just leave `Long get(int)` as the public method. WDYT?

@Scottmitch I think this method would be useful, so let me just clarify in the javadocs.

`on the value` -> `on the return of {@link #get(int)}`.

Done.


Patch that resolved the issue:
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 308ac81ea6c..88bdd859ba8 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
@@ -69,6 +69,7 @@ public final class Http2CodecUtil {
     public static final int SETTINGS_INITIAL_WINDOW_SIZE = 4;
     public static final int SETTINGS_MAX_FRAME_SIZE = 5;
     public static final int 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_CONCURRENT_STREAMS = MAX_UNSIGNED_INT;
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 574145dc5e8..c2fd28787da 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
@@ -23,6 +23,7 @@
 import static io.netty.handler.codec.http2.Http2CodecUtil.MIN_CONCURRENT_STREAMS;
 import static io.netty.handler.codec.http2.Http2CodecUtil.MIN_INITIAL_WINDOW_SIZE;
 import static io.netty.handler.codec.http2.Http2CodecUtil.MIN_HEADER_LIST_SIZE;
+import static io.netty.handler.codec.http2.Http2CodecUtil.NUM_STANDARD_SETTINGS;
 import static io.netty.handler.codec.http2.Http2CodecUtil.SETTINGS_ENABLE_PUSH;
 import static io.netty.handler.codec.http2.Http2CodecUtil.SETTINGS_HEADER_TABLE_SIZE;
 import static io.netty.handler.codec.http2.Http2CodecUtil.SETTINGS_INITIAL_WINDOW_SIZE;
@@ -39,9 +40,14 @@
  * methods for standard settings.
  */
 public final class Http2Settings extends IntObjectHashMap<Long> {
+    /**
+     * Default capacity based on the number of standard settings from the HTTP/2 spec, adjusted so that adding all of
+     * the standard settings will not cause the map capacity to change.
+     */
+    private static final int DEFAULT_CAPACITY = (int) (NUM_STANDARD_SETTINGS / DEFAULT_LOAD_FACTOR) + 1;
 
     public Http2Settings() {
-        this(6 /* number of standard settings */);
+        this(DEFAULT_CAPACITY);
     }
 
     public Http2Settings(int initialCapacity, float loadFactor) {
@@ -53,9 +59,10 @@ public Http2Settings(int initialCapacity) {
     }
 
     /**
-     * Overrides the superclass method to perform verification of standard HTTP/2 settings.
+     * Adds the given setting key/value pair. For standard settings defined by the HTTP/2 spec, performs
+     * validation on the values.
      *
-     * @throws IllegalArgumentException if verification of the setting fails.
+     * @throws IllegalArgumentException if verification for a standard HTTP/2 setting fails.
      */
     @Override
     public Long put(int key, Long value) {
@@ -176,7 +183,12 @@ public Http2Settings copyFrom(Http2Settings settings) {
         return this;
     }
 
-    Integer getIntValue(int key) {
+    /**
+     * A helper method that returns {@link Long#intValue()} on the return of {@link #get(int)}, if present. Note that
+     * if the range of the value exceeds {@link Integer#MAX_VALUE}, the {@link #get(int)} method should
+     * be used instead to avoid truncation of the value.
+     */
+    public Integer getIntValue(int key) {
         Long value = get(key);
         if (value == null) {
             return null;
@@ -220,7 +232,8 @@ private static void verifyStandardSetting(int key, Long value) {
                 }
                 break;
             default:
-                throw new IllegalArgumentException("key");
+                // Non-standard HTTP/2 setting - don't do validation.
+                break;
         }
     }
 
diff --git a/common/src/main/java/io/netty/util/collection/IntObjectHashMap.java b/common/src/main/java/io/netty/util/collection/IntObjectHashMap.java
index 0ae4a8be753..f01e3093da5 100644
--- a/common/src/main/java/io/netty/util/collection/IntObjectHashMap.java
+++ b/common/src/main/java/io/netty/util/collection/IntObjectHashMap.java
@@ -33,10 +33,10 @@
 public class IntObjectHashMap<V> implements IntObjectMap<V>, Iterable<IntObjectMap.Entry<V>> {
 
     /** Default initial capacity. Used if not specified in the constructor */
-    private static final int DEFAULT_CAPACITY = 11;
+    public static final int DEFAULT_CAPACITY = 11;
 
     /** Default load factor. Used if not specified in the constructor */
-    private static final float DEFAULT_LOAD_FACTOR = 0.5f;
+    public static final float DEFAULT_LOAD_FACTOR = 0.5f;
 
     /**
      * Placeholder for null values, so we can use the actual null to mean available.


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 38e8d2ce7fd..06c25d7fa84 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
@@ -61,4 +61,10 @@ public void standardSettingsShouldBeSet() {
         assertEquals(MAX_FRAME_SIZE_UPPER_BOUND, (int) settings.maxFrameSize());
         assertEquals(4L, (long) settings.maxHeaderListSize());
     }
+
+    @Test
+    public void nonStandardSettingsShouldBeSet() {
+        settings.put(0, 123L);
+        assertEquals(123L, (long) settings.get(0));
+    }
 }