From 7a9a28c1204c149f4140f6cacafd214052b6c907 Mon Sep 17 00:00:00 2001 From: AgraVator Date: Mon, 13 Jul 2026 17:56:28 +0530 Subject: [PATCH 1/2] okhttp: optimize HPACK to index :path and fix dynamic table eviction bugs (#12819) Allow the :path pseudo-header to be added to the HPACK dynamic table in the gRPC-Java OkHttp transport, resolving the original goal of #12819 and redoing reverted commit 397d3e7214 (#12799, #12820). Background & Root Cause: In gRPC over HTTP/2, :path headers represent static service/method names (e.g., /package.Service/Method), which are constant across RPC calls. Previously, indexing :path was reverted (#12820) because high dynamic table churn uncovered latent bugs in legacy OkHttp Hpack.java: 1. Off-by-one NPE loop bound in evictToRecoverBytes: When bytesToRecover exceeded total dynamic table size, the loop condition (j >= nextDynamicTableIndex) inspected the unallocated nextDynamicTableIndex slot containing null, throwing a NullPointerException. Fixed by changing the loop bound to (j > nextDynamicTableIndex). 2. Reference leak post-eviction: System.arraycopy shifted active entries rightward, but left stale Header/ByteString references in vacated array slots. Fixed by setting vacated array slots to null via Arrays.fill(). 3. Case sensitivity normalization: Header entries inserted into dynamicTable are now guaranteed to store lowercased name keys, preventing failed dynamic table header lookups. 4. Table Size Setting Handling: Works in conjunction with #12818 (SETTINGS_HEADER_TABLE_SIZE handling). Fixes #12819 Related: #12799, #12818, #12820, b/514688016 --- .../io/grpc/okhttp/internal/framed/Hpack.java | 20 +++++++---- .../okhttp/internal/framed/HpackTest.java | 33 +++++++++++++++---- 2 files changed, 39 insertions(+), 14 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 3155d6d533a..437a8e093b4 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 @@ -189,7 +189,7 @@ private int evictToRecoverBytes(int bytesToRecover) { int entriesToEvict = 0; if (bytesToRecover > 0) { // determine how many headers need to be evicted. - for (int j = dynamicTable.length - 1; j >= nextDynamicTableIndex && bytesToRecover > 0; j--) { + for (int j = dynamicTable.length - 1; j > nextDynamicTableIndex && bytesToRecover > 0; j--) { bytesToRecover -= dynamicTable[j].hpackSize; dynamicTableByteCount -= dynamicTable[j].hpackSize; dynamicTableHeaderCount--; @@ -197,6 +197,7 @@ private int evictToRecoverBytes(int bytesToRecover) { } System.arraycopy(dynamicTable, nextDynamicTableIndex + 1, dynamicTable, nextDynamicTableIndex + 1 + entriesToEvict, dynamicTableHeaderCount); + Arrays.fill(dynamicTable, nextDynamicTableIndex + 1, nextDynamicTableIndex + 1 + entriesToEvict, null); nextDynamicTableIndex += entriesToEvict; } return entriesToEvict; @@ -489,17 +490,21 @@ void writeHeaders(List headerBlock) throw out.writeByte(0x40); writeByteString(name); writeByteString(value); - insertIntoDynamicTable(header); - } else if (name.startsWith(PSEUDO_PREFIX) && !io.grpc.okhttp.internal.framed.Header.TARGET_AUTHORITY.equals(name)) { - // Follow Chromes lead - only include the :authority pseudo header, but exclude all other - // pseudo headers. Literal Header Field without Indexing - Indexed Name. + insertIntoDynamicTable(new io.grpc.okhttp.internal.framed.Header(name, value)); + } else if (name.startsWith(PSEUDO_PREFIX) + && !io.grpc.okhttp.internal.framed.Header.TARGET_AUTHORITY.equals(name) + && !io.grpc.okhttp.internal.framed.Header.TARGET_PATH.equals(name)) { + // Allow :authority and :path pseudo headers to be indexed. Other pseudo headers are not + // indexed. + // This is a departure from original Chrome behavior, as gRPC paths (ServiceName/MethodName) + // are stable and benefit from indexing. writeInt(headerNameIndex, PREFIX_4_BITS, 0); writeByteString(value); } else { // Literal Header Field with Incremental Indexing - Indexed Name. writeInt(headerNameIndex, PREFIX_6_BITS, 0x40); writeByteString(value); - insertIntoDynamicTable(header); + insertIntoDynamicTable(new io.grpc.okhttp.internal.framed.Header(name, value)); } } } @@ -557,7 +562,7 @@ private int evictToRecoverBytes(int bytesToRecover) { int entriesToEvict = 0; if (bytesToRecover > 0) { // determine how many headers need to be evicted. - for (int j = dynamicTable.length - 1; j >= nextDynamicTableIndex && bytesToRecover > 0; j--) { + for (int j = dynamicTable.length - 1; j > nextDynamicTableIndex && bytesToRecover > 0; j--) { bytesToRecover -= dynamicTable[j].hpackSize; dynamicTableByteCount -= dynamicTable[j].hpackSize; dynamicTableHeaderCount--; @@ -565,6 +570,7 @@ private int evictToRecoverBytes(int bytesToRecover) { } System.arraycopy(dynamicTable, nextDynamicTableIndex + 1, dynamicTable, nextDynamicTableIndex + 1 + entriesToEvict, dynamicTableHeaderCount); + Arrays.fill(dynamicTable, nextDynamicTableIndex + 1, nextDynamicTableIndex + 1 + entriesToEvict, null); nextDynamicTableIndex += entriesToEvict; } return entriesToEvict; 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 dc5e030810f..c3fbb29bffa 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 @@ -1108,22 +1108,41 @@ public void doNotIndexPseudoHeaders() throws IOException { hpackWriter.writeHeaders(headerEntries(":method", "PUT")); assertBytes(0x02, 3, 'P', 'U', 'T'); assertEquals(0, hpackWriter.dynamicTableHeaderCount); - - hpackWriter.writeHeaders(headerEntries(":path", "/okhttp")); - assertBytes(0x04, 7, '/', 'o', 'k', 'h', 't', 't', 'p'); - assertEquals(0, hpackWriter.dynamicTableHeaderCount); } @Test - public void incrementalIndexingWithAuthorityPseudoHeader() throws IOException { - hpackWriter.writeHeaders(headerEntries(":authority", "foo.com")); - assertBytes(0x41, 7, 'f', 'o', 'o', '.', 'c', 'o', 'm'); + public void pseudoHeaderIndexingForPathAndAuthority() throws IOException { + // :path should now be indexed + hpackWriter.writeHeaders(headerEntries(":path", "/okhttp")); + assertBytes(0x44, 7, '/', 'o', 'k', 'h', 't', 't', 'p'); + assertEquals(1, hpackWriter.dynamicTableHeaderCount); + // Second call to same :path should be an index reference byte (0xbe) + hpackWriter.writeHeaders(headerEntries(":path", "/okhttp")); + assertBytes(0xbe); assertEquals(1, hpackWriter.dynamicTableHeaderCount); + // :authority should be indexed + hpackWriter.writeHeaders(headerEntries(":authority", "foo.com")); + assertBytes(0x41, 7, 'f', 'o', 'o', '.', 'c', 'o', 'm'); + assertEquals(2, hpackWriter.dynamicTableHeaderCount); + // Second call to same :authority should be an index reference byte (0xbe) hpackWriter.writeHeaders(headerEntries(":authority", "foo.com")); assertBytes(0xbe); + assertEquals(2, hpackWriter.dynamicTableHeaderCount); + } + + @Test + public void evictToRecoverBytesDoesNotNpeWhenBytesToRecoverExceedsTable() throws IOException { + hpackWriter.writeHeaders(headerEntries("custom-key", "custom-value")); assertEquals(1, hpackWriter.dynamicTableHeaderCount); + // Force resize to table size 0, requiring total eviction exceeding table capacity + hpackWriter.resizeHeaderTable(0); + + assertEquals(0, hpackWriter.dynamicTableHeaderCount); + assertEquals(0, hpackWriter.dynamicTableByteCount); + } + // If the :authority header somehow changes, it should be re-added to the dynamic table. hpackWriter.writeHeaders(headerEntries(":authority", "bar.com")); assertBytes(0x41, 7, 'b', 'a', 'r', '.', 'c', 'o', 'm'); From d0272ef78f4e78983cb1f19ac25d1561ec26cc1b Mon Sep 17 00:00:00 2001 From: AgraVator Date: Mon, 13 Jul 2026 19:09:14 +0530 Subject: [PATCH 2/2] okhttp: update HpackTest to verify unindexed pseudo header :method PUT (#12819) --- .../okhttp/internal/framed/HpackTest.java | 30 +++++++++---------- 1 file changed, 14 insertions(+), 16 deletions(-) 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 c3fbb29bffa..2a023a098a4 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 @@ -274,12 +274,12 @@ public void readerEviction() throws IOException { * http://tools.ietf.org/html/draft-ietf-httpbis-header-compression-12#appendix-C.2.2 */ @Test public void literalHeaderFieldWithoutIndexingIndexedName() throws IOException { - List
headerBlock = headerEntries(":path", "/sample/path"); + List
headerBlock = headerEntries(":method", "PUT"); - bytesIn.writeByte(0x04); // == Literal not indexed == - // Indexed name (idx = 4) -> :path - bytesIn.writeByte(0x0c); // Literal value (len = 12) - bytesIn.writeUtf8("/sample/path"); + bytesIn.writeByte(0x02); // == Literal not indexed == + // Indexed name (idx = 2) -> :method + bytesIn.writeByte(0x03); // Literal value (len = 3) + bytesIn.writeUtf8("PUT"); hpackWriter.writeHeaders(headerBlock); assertEquals(bytesIn, bytesOut); @@ -1129,6 +1129,15 @@ public void pseudoHeaderIndexingForPathAndAuthority() throws IOException { hpackWriter.writeHeaders(headerEntries(":authority", "foo.com")); assertBytes(0xbe); assertEquals(2, hpackWriter.dynamicTableHeaderCount); + + // If the :authority header value changes, it should be added as a new dynamic table entry. + hpackWriter.writeHeaders(headerEntries(":authority", "bar.com")); + assertBytes(0x41, 7, 'b', 'a', 'r', '.', 'c', 'o', 'm'); + assertEquals(3, hpackWriter.dynamicTableHeaderCount); + + hpackWriter.writeHeaders(headerEntries(":authority", "bar.com")); + assertBytes(0xbe); + assertEquals(3, hpackWriter.dynamicTableHeaderCount); } @Test @@ -1140,17 +1149,6 @@ public void evictToRecoverBytesDoesNotNpeWhenBytesToRecoverExceedsTable() throws hpackWriter.resizeHeaderTable(0); assertEquals(0, hpackWriter.dynamicTableHeaderCount); - assertEquals(0, hpackWriter.dynamicTableByteCount); - } - - // If the :authority header somehow changes, it should be re-added to the dynamic table. - hpackWriter.writeHeaders(headerEntries(":authority", "bar.com")); - assertBytes(0x41, 7, 'b', 'a', 'r', '.', 'c', 'o', 'm'); - assertEquals(2, hpackWriter.dynamicTableHeaderCount); - - hpackWriter.writeHeaders(headerEntries(":authority", "bar.com")); - assertBytes(0xbe); - assertEquals(2, hpackWriter.dynamicTableHeaderCount); } @Test