From a4ea2c64b7066bc1a5d0e063df1ece03394c5013 Mon Sep 17 00:00:00 2001
From: Eric Anderson <ejona@google.com>
Date: Mon, 20 Apr 2026 14:54:29 -0700
Subject: [PATCH] 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
---
 .../io/grpc/okhttp/internal/framed/Hpack.java | 10 ++++
 .../okhttp/internal/framed/HpackTest.java     | 56 ++++++++++++++++---
 2 files changed, 59 insertions(+), 7 deletions(-)

diff --git a/okhttp/third_party/okhttp/main/java/io/grpc/okhttp/internal/framed/Hpack.java b/okhttp/third_party/okhttp/main/java/io/grpc/okhttp/internal/framed/Hpack.java
index 484cc5673dc..3155d6d533a 100644
--- a/okhttp/third_party/okhttp/main/java/io/grpc/okhttp/internal/framed/Hpack.java
+++ b/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) {
diff --git a/okhttp/third_party/okhttp/test/java/io/grpc/okhttp/internal/framed/HpackTest.java b/okhttp/third_party/okhttp/test/java/io/grpc/okhttp/internal/framed/HpackTest.java
index 26580f85e54..dc5e030810f 100644
--- a/okhttp/third_party/okhttp/test/java/io/grpc/okhttp/internal/framed/HpackTest.java
+++ b/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 {