Merge pull request #11645 from jetty/jetty-12.0.x-11420-qpackDecoder

Issue #11420 - fix dynamic table referencing in QpackDecoder

jetty-core/jetty-http3/jetty-http3-qpack/src/main/java/org/eclipse/jetty/http3/qpack/QpackDecoder.java

@@ -379,7 +379,7 @@ public void onDuplicate(int index) throws QpackException
                 LOG.debug("Duplicate: index={}", index);
 
             DynamicTable dynamicTable = _context.getDynamicTable();
-            Entry referencedEntry = dynamicTable.get(index);
+            Entry referencedEntry = dynamicTable.getRelative(index);
 
             // Add the new Entry to the DynamicTable.
             Entry entry = new Entry(referencedEntry.getHttpField());
@@ -396,7 +396,7 @@ public void onInsertNameWithReference(int nameIndex, boolean isDynamicTableIndex
 
             StaticTable staticTable = QpackContext.getStaticTable();
             DynamicTable dynamicTable = _context.getDynamicTable();
-            Entry referencedEntry = isDynamicTableIndex ? dynamicTable.get(nameIndex) : staticTable.get(nameIndex);
+            Entry referencedEntry = isDynamicTableIndex ? dynamicTable.getRelative(nameIndex) : staticTable.get(nameIndex);
 
             // Add the new Entry to the DynamicTable.
             Entry entry = new Entry(new HttpField(referencedEntry.getHttpField().getHeader(), referencedEntry.getHttpField().getName(), value));

jetty-core/jetty-http3/jetty-http3-qpack/src/main/java/org/eclipse/jetty/http3/qpack/QpackEncoder.java

@@ -107,6 +107,11 @@ public QpackEncoder(Instruction.Handler handler)
         _parser = new EncoderInstructionParser(_instructionHandler);
     }
 
+    QpackContext getQpackContext()
+    {
+        return _context;
+    }
+
     Map<Long, StreamInfo> getStreamInfoMap()
     {
         return _streamInfoMap;

jetty-core/jetty-http3/jetty-http3-qpack/src/main/java/org/eclipse/jetty/http3/qpack/internal/EncodableEntry.java

@@ -68,14 +68,14 @@ public void encode(ByteBuffer buffer, int base)
             {
                 // Indexed Field Line with Static Reference.
                 buffer.put((byte)(0x80 | 0x40));
-                int relativeIndex = _entry.getIndex();
-                NBitIntegerEncoder.encode(buffer, 6, relativeIndex);
+                int index = _entry.getIndex();
+                NBitIntegerEncoder.encode(buffer, 6, index);
             }
             else if (_entry.getIndex() < base)
             {
                 // Indexed Field Line with Dynamic Reference.
                 buffer.put((byte)0x80);
-                int relativeIndex = base - (_entry.getIndex() + 1);
+                int relativeIndex = (base - 1) - _entry.getIndex();
                 NBitIntegerEncoder.encode(buffer, 6, relativeIndex);
             }
             else
@@ -100,7 +100,7 @@ public int getRequiredSize(int base)
             else if (_entry.getIndex() < base)
             {
                 // Indexed Field Line with Dynamic Reference.
-                int relativeIndex =  base - (_entry.getIndex() + 1);
+                int relativeIndex =  (base - 1) - _entry.getIndex();
                 return NBitIntegerEncoder.octetsNeeded(6, relativeIndex);
             }
             else
@@ -150,7 +150,7 @@ else if (_nameEntry.getIndex() < base)
             {
                 // Literal Field Line with Dynamic Name Reference.
                 buffer.put((byte)(0x40 | (allowIntermediary ? 0x20 : 0x00)));
-                int relativeIndex = base - (_nameEntry.getIndex() + 1);
+                int relativeIndex = (base - 1) - _nameEntry.getIndex();
                 NBitIntegerEncoder.encode(buffer, 4, relativeIndex);
             }
             else

jetty-core/jetty-http3/jetty-http3-qpack/src/main/java/org/eclipse/jetty/http3/qpack/internal/QpackContext.java

@@ -66,14 +66,6 @@ public Entry get(String name)
         return _dynamicTable.get(StringUtil.asciiToLowerCase(name));
     }
 
-    public Entry get(int index)
-    {
-        if (index <= StaticTable.STATIC_SIZE)
-            return __staticTable.get(index);
-
-        return _dynamicTable.get(index);
-    }
-
     /**
      * Get the relative Index of an entry.
      * @param entry the entry to get the index of.
@@ -83,7 +75,6 @@ public int indexOf(Entry entry)
     {
         if (entry.isStatic())
             return entry.getIndex();
-
-        return _dynamicTable.index(entry);
+        return _dynamicTable.relativeIndexOf(entry);
     }
 }

jetty-core/jetty-http3/jetty-http3-qpack/src/main/java/org/eclipse/jetty/http3/qpack/internal/parser/EncodedFieldSection.java

@@ -229,7 +229,7 @@ public IndexedField(boolean dynamicTable, int index)
         public HttpField decode(QpackContext context)
         {
             if (_dynamicTable)
-                return context.getDynamicTable().getAbsolute(_base - (_index + 1)).getHttpField();
+                return context.getDynamicTable().getRelative(_base, _index).getHttpField();
             else
                 return QpackContext.getStaticTable().get(_index).getHttpField();
         }
@@ -247,7 +247,7 @@ public PostBaseIndexedField(int index)
         @Override
         public HttpField decode(QpackContext context)
         {
-            return context.getDynamicTable().getAbsolute(_base + _index).getHttpField();
+            return context.getDynamicTable().getPostBase(_base, _index).getHttpField();
         }
     }
 
@@ -272,7 +272,7 @@ public HttpField decode(QpackContext context)
         {
             HttpField field;
             if (_dynamicTable)
-                field = context.getDynamicTable().getAbsolute(_base - (_nameIndex + 1)).getHttpField();
+                field = context.getDynamicTable().getRelative(_base, _nameIndex).getHttpField();
             else
                 field = QpackContext.getStaticTable().get(_nameIndex).getHttpField();
 
@@ -296,7 +296,7 @@ public PostBaseIndexedNameField(boolean allowEncoding, int nameIndex, String val
         @Override
         public HttpField decode(QpackContext context)
         {
-            HttpField field = context.getDynamicTable().getAbsolute(_base + _nameIndex).getHttpField();
+            HttpField field = context.getDynamicTable().getPostBase(_base, _nameIndex).getHttpField();
             return new HttpField(field.getHeader(), field.getName(), _value);
         }
     }

jetty-core/jetty-http3/jetty-http3-qpack/src/main/java/org/eclipse/jetty/http3/qpack/internal/table/DynamicTable.java

@@ -115,17 +115,9 @@ public boolean canInsert(HttpField field)
      * @param entry the entry to find the relative index of.
      * @return the relative index of this entry.
      */
-    public int index(Entry entry)
+    public int relativeIndexOf(Entry entry)
     {
-        if (_entries.isEmpty())
-            throw new IllegalArgumentException("Invalid Index");
-
-        Entry firstEntry = _entries.get(0);
-        int index = entry.getIndex() - firstEntry.getIndex();
-        if (index >= _entries.size())
-            throw new IllegalArgumentException("Invalid Index");
-
-        return index;
+        return _absoluteIndex - 1 - entry.getIndex();
     }
 
     /**
@@ -146,7 +138,7 @@ public Entry getAbsolute(int absoluteIndex)
         if (index < 0 || index >= _entries.size())
             throw new IllegalArgumentException("Invalid Index " + index);
 
-        return _entries.get(index);
+        return get(index);
     }
 
     public Entry get(int index)
@@ -164,9 +156,24 @@ public Entry get(HttpField field)
         return _fieldMap.get(field);
     }
 
+    public Entry getRelative(int index)
+    {
+        return getRelative(getInsertCount(), index);
+    }
+
+    public Entry getRelative(int base, int index)
+    {
+        return getAbsolute(base - 1 - index);
+    }
+
+    public Entry getPostBase(int base, int index)
+    {
+        return getAbsolute(base + index);
+    }
+
     public int getBase()
     {
-        if (_entries.size() == 0)
+        if (_entries.isEmpty())
             return _absoluteIndex;
         return _entries.get(0).getIndex();
     }

jetty-core/jetty-http3/jetty-http3-qpack/src/test/java/org/eclipse/jetty/http3/qpack/QpackDecoderTest.java

@@ -0,0 +1,116 @@
+//
+// ========================================================================
+// Copyright (c) 1995 Mort Bay Consulting Pty Ltd and others.
+//
+// This program and the accompanying materials are made available under the
+// terms of the Eclipse Public License v. 2.0 which is available at
+// https://www.eclipse.org/legal/epl-2.0, or the Apache License, Version 2.0
+// which is available at https://www.apache.org/licenses/LICENSE-2.0.
+//
+// SPDX-License-Identifier: EPL-2.0 OR Apache-2.0
+// ========================================================================
+//
+
+package org.eclipse.jetty.http3.qpack;
+
+import java.nio.ByteBuffer;
+
+import org.eclipse.jetty.http.MetaData;
+import org.eclipse.jetty.http3.qpack.internal.table.Entry;
+import org.eclipse.jetty.util.BufferUtil;
+import org.eclipse.jetty.util.NanoTime;
+import org.eclipse.jetty.util.StringUtil;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.equalTo;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+public class QpackDecoderTest
+{
+    private QpackDecoder _decoder;
+    private TestDecoderHandler _decoderHandler;
+
+    @BeforeEach
+    public void before()
+    {
+        _decoderHandler = new TestDecoderHandler();
+        _decoder = new QpackDecoder(_decoderHandler);
+        _decoder.setBeginNanoTimeSupplier(NanoTime::now);
+    }
+
+    @Test
+    public void testDynamicNameReference() throws Exception
+    {
+        _decoder.setMaxTableCapacity(2048);
+        QpackDecoder.InstructionHandler instructionHandler = _decoder.getInstructionHandler();
+        instructionHandler.onSetDynamicTableCapacity(2048);
+
+        instructionHandler.onInsertWithLiteralName("name0", "value0");
+        instructionHandler.onInsertWithLiteralName("name1", "value1");
+        instructionHandler.onInsertWithLiteralName("name2", "value2");
+        instructionHandler.onInsertWithLiteralName("name3", "value3");
+        instructionHandler.onInsertNameWithReference(5, false, "static0");
+        instructionHandler.onInsertWithLiteralName("name4", "value4");
+        instructionHandler.onInsertNameWithReference(2, true, "dynamic0");
+        instructionHandler.onDuplicate(6);
+
+        // Indexes into the static table are absolute.
+        Entry entry = _decoder.getQpackContext().getDynamicTable().get(4);
+        assertThat(entry.getHttpField().getName(), equalTo("cookie"));
+        assertThat(entry.getHttpField().getValue(), equalTo("static0"));
+
+        // Named reference is relative to the most recently inserted entry.
+        entry = _decoder.getQpackContext().getDynamicTable().get(6);
+        assertThat(entry.getHttpField().getName(), equalTo("name3"));
+        assertThat(entry.getHttpField().getValue(), equalTo("dynamic0"));
+
+        // Duplicate reference is relative to the most recently inserted entry.
+        entry = _decoder.getQpackContext().getDynamicTable().get(7);
+        assertThat(entry.getHttpField().getName(), equalTo("name0"));
+        assertThat(entry.getHttpField().getValue(), equalTo("value0"));
+    }
+
+    @Test
+    public void testDecodeRequest() throws Exception
+    {
+        _decoder.setMaxTableCapacity(2048);
+        QpackDecoder.InstructionHandler instructionHandler = _decoder.getInstructionHandler();
+        instructionHandler.onSetDynamicTableCapacity(2048);
+
+        instructionHandler.onInsertNameWithReference(0, false, "licensed.app");
+        instructionHandler.onInsertWithLiteralName("sec-ch-ua", "\"Not A(Brand\";v=\"99\", \"Brave\";v=\"121\", \"Chromium\";v=\"121\"");
+        instructionHandler.onInsertWithLiteralName("sec-ch-ua-mobile", "?0");
+        instructionHandler.onInsertWithLiteralName("sec-ch-ua-platform", "Windows");
+        instructionHandler.onInsertWithLiteralName("dnt", "1");
+        instructionHandler.onInsertNameWithReference(95, false, "Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/121.0.0.0 Safari/537.36");
+        instructionHandler.onInsertNameWithReference(29, false, "text/html,application/xhtml+xml,application/xml;q=0.9,image/avif,image/webp,image/apng,*/*;q=0.8");
+        instructionHandler.onInsertWithLiteralName("sec-gpc", "1");
+        instructionHandler.onInsertWithLiteralName("sec-fetch-site", "none");
+        instructionHandler.onInsertWithLiteralName("sec-fetch-mode", "navigate");
+        instructionHandler.onInsertWithLiteralName("sec-fetch-user", "?1");
+        instructionHandler.onInsertWithLiteralName("sec-fetch-dest", "value=document");
+        instructionHandler.onInsertNameWithReference(72, false, "en-US,en;q=0.9");
+        instructionHandler.onInsertNameWithReference(8, false, "2024-02-17T02:19:47.4433882Z");
+        instructionHandler.onInsertNameWithReference(1, false, "/login/GoogleLogin.js");
+        instructionHandler.onInsertNameWithReference(90, false, "https://licensed.app");
+        instructionHandler.onInsertNameWithReference(7, true, "same-origin");
+        instructionHandler.onInsertNameWithReference(7, true, "cors");
+        instructionHandler.onInsertNameWithReference(6, true, "script");
+        instructionHandler.onInsertNameWithReference(13, false, "https://licensed.app/");
+
+        assertTrue(_decoder.decode(0, fromHex("1500D193D78592848f918e90Dd8c83828180Df87"), _decoderHandler));
+        MetaData metaData = _decoderHandler.getMetaData();
+
+        // Check headers were correctly referenced from dynamic table.
+        assertThat(metaData.getHttpFields().get("sec-fetch-site"), equalTo("same-origin"));
+        assertThat(metaData.getHttpFields().get("sec-fetch-mode"), equalTo("cors"));
+        assertThat(metaData.getHttpFields().get("sec-fetch-dest"), equalTo("script"));
+    }
+
+    private ByteBuffer fromHex(String hex)
+    {
+        return BufferUtil.toBuffer(StringUtil.fromHexString(hex));
+    }
+}

jetty-core/jetty-http3/jetty-http3-tests/src/test/java/org/eclipse/jetty/http3/tests/HttpClientTransportOverHTTP3Test.java

@@ -14,13 +14,15 @@
 package org.eclipse.jetty.http3.tests;
 
 import java.nio.ByteBuffer;
+import java.util.Arrays;
 import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.atomic.AtomicInteger;
 import java.util.concurrent.atomic.AtomicReference;
 
 import org.eclipse.jetty.client.ContentResponse;
 import org.eclipse.jetty.client.Response;
+import org.eclipse.jetty.http.HttpFields;
 import org.eclipse.jetty.http.HttpScheme;
 import org.eclipse.jetty.http.HttpStatus;
 import org.eclipse.jetty.http.HttpVersion;
@@ -30,6 +32,8 @@
 import org.eclipse.jetty.util.Callback;
 import org.junit.jupiter.api.Test;
 
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.equalTo;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertFalse;
 import static org.junit.jupiter.api.Assertions.assertTrue;
@@ -236,4 +240,45 @@ public boolean handle(Request request, org.eclipse.jetty.server.Response respons
 
         assertTrue(latch.await(5, TimeUnit.SECONDS));
     }
+
+    @Test
+    public void testDynamicTableReference() throws Exception
+    {
+        start(new Handler.Abstract()
+        {
+            @Override
+            public boolean handle(Request request, org.eclipse.jetty.server.Response response, Callback callback)
+            {
+                HttpFields.Mutable headers = response.getHeaders();
+                headers.add("header1", "value1");
+                headers.add("header2", "value2");
+                headers.add("header3", "value3");
+                headers.add("header4", "value4");
+                headers.add("header5", "value5");
+
+                // This header should reference the named header already in the dynamic table.
+                headers.add("header5", "value6");
+                response.write(true, null, callback);
+                return true;
+            }
+        });
+
+        ContentResponse response = httpClient.newRequest("localhost", connector.getLocalPort())
+            .scheme(HttpScheme.HTTPS.asString())
+            .timeout(5, TimeUnit.SECONDS)
+            .send();
+
+        assertThat(response.getStatus(), equalTo(HttpStatus.OK_200));
+        assertHeader(response, "header1", "value1");
+        assertHeader(response, "header2", "value2");
+        assertHeader(response, "header3", "value3");
+        assertHeader(response, "header4", "value4");
+        assertHeader(response, "header5", "value5", "value6");
+    }
+
+    private void assertHeader(ContentResponse response, String header, String... values)
+    {
+        assertThat(response.getHeaders().getValuesList(header), equalTo(Arrays.asList(values)));
+    }
+
 }

jetty-core/jetty-http3/jetty-http3-tests/src/test/resources/jetty-logging.properties

@@ -2,4 +2,5 @@
 org.eclipse.jetty.jmx.LEVEL=INFO
 #org.eclipse.jetty.http3.LEVEL=DEBUG
 #org.eclipse.jetty.quic.LEVEL=DEBUG
+#org.eclipse.jetty.http3.qpack.QpackDecoder.LEVEL=DEBUG
 org.eclipse.jetty.quic.quiche.LEVEL=INFO