HTTP/2 HPACK Decoder VarInt Improvement

Motivation:
HTTP/2 Decoder#decodeULE128 current will tolerate more bytes than necessary when attempted to detect overflow. The usage of this method also currently requires an additional overflow conditional.

Modifications:
- Integrate the first byte into Decoder#decodeULE128 which allows us to detect overflow reliably and avoid overflow checks outside of this method.

Result:
Less conditionals and earlier overflow detection in Decoder#decodeULE128

codec-http2/src/main/java/io/netty/handler/codec/http2/internal/hpack/Decoder.java

@@ -39,6 +39,7 @@ import io.netty.util.AsciiString;
 
 import static io.netty.handler.codec.http2.Http2Error.COMPRESSION_ERROR;
 import static io.netty.handler.codec.http2.Http2Error.ENHANCE_YOUR_CALM;
+import static io.netty.handler.codec.http2.Http2Error.valueOf;
 import static io.netty.handler.codec.http2.Http2Exception.connectionError;
 import static io.netty.util.AsciiString.EMPTY_STRING;
 import static io.netty.util.internal.ThrowableUtil.unknownStackTrace;
@@ -173,36 +174,18 @@ public final class Decoder {
                     break;
 
                 case READ_MAX_DYNAMIC_TABLE_SIZE:
-                    int maxSize = decodeULE128(in) + index;
+                    setDynamicTableSize(decodeULE128(in, index));
-
-                    if (maxSize < 0) { // Check for numerical overflow
-                        throw DECODE_DECOMPRESSION_EXCEPTION;
-                    }
-
-                    setDynamicTableSize(maxSize);
                     state = READ_HEADER_REPRESENTATION;
                     break;
 
                 case READ_INDEXED_HEADER:
-                    int headerIndex = decodeULE128(in) + index;
+                    headersLength = indexHeader(decodeULE128(in, index), headers, headersLength);
-
-                    if (headerIndex < 0) { // Check for numerical overflow
-                        throw DECODE_DECOMPRESSION_EXCEPTION;
-                    }
-
-                    headersLength = indexHeader(headerIndex, headers, headersLength);
                     state = READ_HEADER_REPRESENTATION;
                     break;
 
                 case READ_INDEXED_HEADER_NAME:
                     // Header Name matches an entry in the Header Table
-                    int nameIndex = decodeULE128(in) + index;
+                    name = readName(decodeULE128(in, index));
-
-                    if (nameIndex < 0) { // Check for numerical overflow
-                        throw DECODE_DECOMPRESSION_EXCEPTION;
-                    }
-
-                    name = readName(nameIndex);
                     state = READ_LITERAL_HEADER_VALUE_LENGTH_PREFIX;
                     break;
 
@@ -223,11 +206,7 @@ public final class Decoder {
 
                 case READ_LITERAL_HEADER_NAME_LENGTH:
                     // Header Name is a Literal String
-                    nameLength = decodeULE128(in) + index;
+                    nameLength = decodeULE128(in, index);
-
-                    if (nameLength < 0) { // Check for numerical overflow
-                        throw DECODE_DECOMPRESSION_EXCEPTION;
-                    }
 
                     if (nameLength > maxHeadersLength - headersLength) {
                         maxHeaderSizeExceeded();
@@ -271,11 +250,7 @@ public final class Decoder {
 
                 case READ_LITERAL_HEADER_VALUE_LENGTH:
                     // Header Value is a Literal String
-                    valueLength = decodeULE128(in) + index;
+                    valueLength = decodeULE128(in, index);
-
-                    if (valueLength < 0) { // Check for numerical overflow
-                        throw DECODE_DECOMPRESSION_EXCEPTION;
-                    }
 
                     // Check new header size against max header size
                     if ((long) valueLength + nameLength > maxHeadersLength - headersLength) {
@@ -429,21 +404,25 @@ public final class Decoder {
     }
 
     // Unsigned Little Endian Base 128 Variable-Length Integer Encoding
-    private static int decodeULE128(ByteBuf in) throws Http2Exception {
+    private static int decodeULE128(ByteBuf in, int result) throws Http2Exception {
+        assert result <= 0x7f && result >= 0;
         final int writerIndex = in.writerIndex();
-        for (int readerIndex = in.readerIndex(), shift = 0, result = 0;
+        for (int readerIndex = in.readerIndex(), shift = 0;
              readerIndex < writerIndex; ++readerIndex, shift += 7) {
             byte b = in.getByte(readerIndex);
-            if (shift == 28 && (b & 0xF8) != 0) {
+            if (shift == 28 && ((b & 0x80) != 0 || b > 6)) {
+                // the maximum value that can be represented by a signed 32 bit number is:
+                // 0x7f + 0x7f + (0x7f << 7) + (0x7f << 14) + (0x7f << 21) + (0x6 << 28)
+                // this means any more shifts will result in overflow so we should break out and throw an error.
                 in.readerIndex(readerIndex + 1);
                 break;
             }
 
             if ((b & 0x80) == 0) {
                 in.readerIndex(readerIndex + 1);
-                return result | ((b & 0x7F) << shift);
+                return result + ((b & 0x7F) << shift);
             }
-            result |= (b & 0x7F) << shift;
+            result += (b & 0x7F) << shift;
         }
 
         throw DECODE_ULE_128_DECOMPRESSION_EXCEPTION;

codec-http2/src/test/java/io/netty/handler/codec/http2/internal/hpack/DecoderTest.java

@@ -40,6 +40,7 @@ import org.junit.Test;
 
 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.junit.Assert.assertEquals;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.reset;
@@ -148,7 +149,7 @@ public class DecoderTest {
     @Test(expected = Http2Exception.class)
     public void testInsidiousIndex() throws Http2Exception {
         // Insidious index so the last shift causes sign overflow
-        decode("FF8080808008");
+        decode("FF8080808007");
     }
 
     @Test
@@ -174,10 +175,20 @@ public class DecoderTest {
 
     @Test(expected = Http2Exception.class)
     public void testInsidiousMaxDynamicTableSize() throws Http2Exception {
+        decoder.setMaxHeaderTableSize(MAX_VALUE);
         // max header table size sign overflow
         decode("3FE1FFFFFF07");
     }
 
+    @Test
+    public void testMaxValidDynamicTableSize() throws Http2Exception {
+        decoder.setMaxHeaderTableSize(MAX_VALUE);
+        String baseValue = "3FE1FFFFFF0";
+        for (int i = 0; i < 7; ++i) {
+            decode(baseValue + i);
+        }
+    }
+
     @Test
     public void testReduceMaxDynamicTableSize() throws Http2Exception {
         decoder.setMaxHeaderTableSize(0);