okhttp: HPACK should fail on varint overflow

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 {