okhttp: HPACK should fail on varint overflow

This does reduce the largest supported integer from just less than 2^32
to slightly more than 2^29, which does not seem a significant loss.

It would previously produce a corrupted integer, which makes debugging
annoying. Note that continuations can contain just zeros and should
still be detected as resulting in overflow, without waiting for any
eventual 1.

We could leave the encoder supporting up to 2^32-1, but it just seems
wrong to encode values that the same implementation couldn't decode.

Noticed by @August829

Author: ejona86

okhttp/third_party/okhttp/main/java/io/grpc/okhttp/internal/framed/Hpack.java

@@ -354,6 +354,13 @@ int readInt(int firstByte, int prefixMask) throws IOException {
         if ((b & 0x80) != 0) { // Equivalent to (b >= 128) since b is in [0..255].
           result += (b & 0x7f) << shift;
           shift += 7;
+          // We can safely store 31 bits, and then next byte will have 7 more bits. While the next
+          // byte may not have high bits set to cause an overflow, that's only useful for 256+ MiB
+          // values, which is excessive. This also gives us at least one bit of spare, which is
+          // necessary to store the carry from the addition.
+          if (shift >= 28) {
+            throw new IOException("Varint overflowed");
+          }
         } else {
           result += b << shift; // Last byte.
           break;
@@ -508,6 +515,9 @@ void writeInt(int value, int prefixMask, int bits) throws IOException {
       // Write the mask to start a multibyte value.
       out.writeByte(bits | prefixMask);
       value -= prefixMask;
+      if (value > 0xfffffff) {
+        throw new IOException("Varint would overflow reader");
+      }
 
       // Write 7 bits at a time 'til we're done.
       while (value >= 0x80) {

okhttp/third_party/okhttp/test/java/io/grpc/okhttp/internal/framed/HpackTest.java

@@ -455,7 +455,7 @@ public void theSameHeaderAfterOneIncrementalIndexed() throws IOException {
       hpackReader.readHeaders();
       fail();
     } catch (IOException e) {
-      assertEquals("Header index too large -2147483521", e.getMessage());
+      assertEquals("Varint overflowed", e.getMessage());
     }
   }
 
@@ -497,7 +497,7 @@ public void theSameHeaderAfterOneIncrementalIndexed() throws IOException {
       hpackReader.readHeaders();
       fail();
     } catch (IOException e) {
-      assertEquals("Invalid dynamic table size update -2147483648", e.getMessage());
+      assertEquals("Varint overflowed", e.getMessage());
     }
   }
 
@@ -856,11 +856,53 @@ private void checkReadThirdRequestWithHuffman() {
     assertBytes(0xe0 | 31, 154, 10);
   }
 
-  @Test public void max31BitValue() throws IOException {
-    hpackWriter.writeInt(0x7fffffff, 31, 0);
-    assertBytes(31, 224, 255, 255, 255, 7);
-    assertEquals(0x7fffffff,
-        newReader(byteStream(224, 255, 255, 255, 7)).readInt(31, 31));
+  @Test public void max29BitValue() throws IOException {
+    hpackWriter.writeInt(0x100000fe, 0xff, 0xff);
+    assertBytes(0xff, 0xff, 0xff, 0xff, 0x7f);
+    assertEquals(0x100000fe,
+        newReader(byteStream(0xff, 0xff, 0xff, 0x7f)).readInt(0xff, 0xff));
+  }
+
+  @Test public void beyondMax29BitValue() throws IOException {
+    try {
+      hpackWriter.writeInt(0x100000ff, 0xff, 0xff);
+      fail();
+    } catch (IOException e) {
+      assertEquals("Varint would overflow reader", e.getMessage());
+    }
+    try {
+      newReader(byteStream(0xff, 0xff, 0xff, 0xff, 0x80)).readInt(0xff, 0xff);
+      fail();
+    } catch (IOException e) {
+      assertEquals("Varint overflowed", e.getMessage());
+    }
+  }
+
+  @Test public void beyondMax29BitValue_smallPrefix() throws IOException {
+    try {
+      hpackWriter.writeInt(0x10000001, 1, 1);
+      fail();
+    } catch (IOException e) {
+      assertEquals("Varint would overflow reader", e.getMessage());
+    }
+    try {
+      newReader(byteStream(0xff, 0xff, 0xff, 0xff, 0x80)).readInt(1, 1);
+      fail();
+    } catch (IOException e) {
+      assertEquals("Varint overflowed", e.getMessage());
+    }
+  }
+
+  @Test public void readerAbortsLongVarintsWithZeros() throws IOException {
+    try {
+      // The reader should fail before getting to the end, because it will overflow as soon as there
+      // is a 1 bit, and the only reason to use this many continuations is to eventually have a 1
+      // bit.
+      newReader(byteStream(0x80, 0x80, 0x80, 0x80, 0x80)).readInt(31, 31);
+      fail();
+    } catch (IOException e) {
+      assertEquals("Varint overflowed", e.getMessage());
+    }
   }
 
   @Test public void prefixMask() throws IOException {