Merge pull request #9798 from eclipse/jetty-10.0.x-QPACK-encoding

review and cleanup of HTTP/3 QPACK Integer and String encoding

jetty-http/src/main/java/org/eclipse/jetty/http/compression/NBitIntegerEncoder.java

@@ -25,96 +25,65 @@ private NBitIntegerEncoder()
     }
 
     /**
-     * @param n the prefix used to encode this long.
-     * @param i the integer to encode.
+     * @param prefix the prefix used to encode this long.
+     * @param value the integer to encode.
      * @return the number of octets it would take to encode the long.
      */
-    public static int octetsNeeded(int n, long i)
+    public static int octetsNeeded(int prefix, long value)
     {
-        if (n == 8)
-        {
-            int nbits = 0xFF;
-            i = i - nbits;
-            if (i < 0)
-                return 1;
-            if (i == 0)
-                return 2;
-            int lz = Long.numberOfLeadingZeros(i);
-            int log = 64 - lz;
-            return 1 + (log + 6) / 7;
-        }
+        if (prefix <= 0 || prefix > 8)
+            throw new IllegalArgumentException();
 
-        int nbits = 0xFF >>> (8 - n);
-        i = i - nbits;
-        if (i < 0)
-            return 0;
-        if (i == 0)
+        int nbits = 0xFF >>> (8 - prefix);
+        value = value - nbits;
+        if (value < 0)
             return 1;
-        int lz = Long.numberOfLeadingZeros(i);
+        if (value == 0)
+            return 2;
+        int lz = Long.numberOfLeadingZeros(value);
         int log = 64 - lz;
-        return (log + 6) / 7;
+
+        // The return value is 1 for the prefix + the number of 7-bit groups necessary to encode the value.
+        return 1 + (log + 6) / 7;
     }
 
     /**
      *
-     * @param buf the buffer to encode into.
-     * @param n the prefix used to encode this long.
-     * @param i the long to encode into the buffer.
+     * @param buffer the buffer to encode into.
+     * @param prefix the prefix used to encode this long.
+     * @param value the long to encode into the buffer.
      */
-    public static void encode(ByteBuffer buf, int n, long i)
+    public static void encode(ByteBuffer buffer, int prefix, long value)
     {
-        if (n == 8)
-        {
-            if (i < 0xFF)
-            {
-                buf.put((byte)i);
-            }
-            else
-            {
-                buf.put((byte)0xFF);
+        if (prefix <= 0 || prefix > 8)
+            throw new IllegalArgumentException();
 
-                long length = i - 0xFF;
-                while (true)
-                {
-                    if ((length & ~0x7F) == 0)
-                    {
-                        buf.put((byte)length);
-                        return;
-                    }
-                    else
-                    {
-                        buf.put((byte)((length & 0x7F) | 0x80));
-                        length >>>= 7;
-                    }
-                }
-            }
+        // If prefix is 8 we add an empty byte as we initially modify last byte from the buffer.
+        if (prefix == 8)
+            buffer.put((byte)0x00);
+
+        int bits = 0xFF >>> (8 - prefix);
+        int p = buffer.position() - 1;
+        if (value < bits)
+        {
+            buffer.put(p, (byte)((buffer.get(p) & ~bits) | value));
         }
         else
         {
-            int p = buf.position() - 1;
-            int bits = 0xFF >>> (8 - n);
-
-            if (i < bits)
+            buffer.put(p, (byte)(buffer.get(p) | bits));
+            long length = value - bits;
+            while (true)
             {
-                buf.put(p, (byte)((buf.get(p) & ~bits) | i));
-            }
-            else
-            {
-                buf.put(p, (byte)(buf.get(p) | bits));
-
-                long length = i - bits;
-                while (true)
+                // The value of ~0x7F is different to 0x80 because of all the 1s from the MSB.
+                if ((length & ~0x7FL) == 0)
+                {
+                    buffer.put((byte)length);
+                    return;
+                }
+                else
                 {
-                    if ((length & ~0x7F) == 0)
-                    {
-                        buf.put((byte)length);
-                        return;
-                    }
-                    else
-                    {
-                        buf.put((byte)((length & 0x7F) | 0x80));
-                        length >>>= 7;
-                    }
+                    buffer.put((byte)((length & 0x7F) | 0x80));
+                    length >>>= 7;
                 }
             }
         }

jetty-http/src/main/java/org/eclipse/jetty/http/compression/NBitStringEncoder.java

@@ -0,0 +1,77 @@
+//
+// ========================================================================
+// 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.http.compression;
+
+import java.nio.ByteBuffer;
+
+import org.eclipse.jetty.http.HttpTokens;
+
+public class NBitStringEncoder
+{
+    private NBitStringEncoder()
+    {
+    }
+
+    public static int octetsNeeded(int prefix, String value, boolean huffman)
+    {
+        if (prefix <= 0 || prefix > 8)
+            throw new IllegalArgumentException();
+
+        int contentPrefix = (prefix == 1) ? 8 : prefix - 1;
+        int encodedValueSize = huffman ? HuffmanEncoder.octetsNeeded(value) : value.length();
+        int encodedLengthSize = NBitIntegerEncoder.octetsNeeded(contentPrefix, encodedValueSize);
+
+        // If prefix was 1, then we count an extra byte needed for the prefix.
+        return encodedLengthSize + encodedValueSize + (prefix == 1 ? 1 : 0);
+    }
+
+    public static void encode(ByteBuffer buffer, int prefix, String value, boolean huffman)
+    {
+        if (prefix <= 0 || prefix > 8)
+            throw new IllegalArgumentException();
+
+        byte huffmanFlag = huffman ? (byte)(0x01 << (prefix - 1)) : (byte)0x00;
+        if (prefix == 8)
+        {
+            buffer.put(huffmanFlag);
+        }
+        else
+        {
+            int p = buffer.position() - 1;
+            buffer.put(p, (byte)(buffer.get(p) | huffmanFlag));
+        }
+
+        // Start encoding size & content in rest of prefix.
+        // If prefix was 1 we set it back to 8 to indicate to start on a new byte.
+        prefix = (prefix == 1) ? 8 : prefix - 1;
+
+        if (huffman)
+        {
+            int encodedValueSize = HuffmanEncoder.octetsNeeded(value);
+            NBitIntegerEncoder.encode(buffer, prefix, encodedValueSize);
+            HuffmanEncoder.encode(buffer, value);
+        }
+        else
+        {
+            int encodedValueSize = value.length();
+            NBitIntegerEncoder.encode(buffer, prefix, encodedValueSize);
+            for (int i = 0; i < encodedValueSize; i++)
+            {
+                char c = value.charAt(i);
+                c = HttpTokens.sanitizeFieldVchar(c);
+                buffer.put((byte)c);
+            }
+        }
+    }
+}

jetty-http/src/test/java/org/eclipse/jetty/http/NBitIntegerTest.java

@@ -18,6 +18,7 @@
 import org.eclipse.jetty.http.compression.NBitIntegerDecoder;
 import org.eclipse.jetty.http.compression.NBitIntegerEncoder;
 import org.eclipse.jetty.util.BufferUtil;
+import org.eclipse.jetty.util.StringUtil;
 import org.eclipse.jetty.util.TypeUtil;
 import org.junit.jupiter.api.Test;
 
@@ -31,17 +32,17 @@ public class NBitIntegerTest
     @Test
     public void testOctetsNeeded()
     {
-        assertEquals(0, NBitIntegerEncoder.octetsNeeded(5, 10));
-        assertEquals(2, NBitIntegerEncoder.octetsNeeded(5, 1337));
+        assertEquals(1, NBitIntegerEncoder.octetsNeeded(5, 10));
+        assertEquals(3, NBitIntegerEncoder.octetsNeeded(5, 1337));
         assertEquals(1, NBitIntegerEncoder.octetsNeeded(8, 42));
         assertEquals(3, NBitIntegerEncoder.octetsNeeded(8, 1337));
 
-        assertEquals(0, NBitIntegerEncoder.octetsNeeded(6, 62));
-        assertEquals(1, NBitIntegerEncoder.octetsNeeded(6, 63));
-        assertEquals(1, NBitIntegerEncoder.octetsNeeded(6, 64));
-        assertEquals(2, NBitIntegerEncoder.octetsNeeded(6, 63 + 0x00 + 0x80 * 0x01));
-        assertEquals(3, NBitIntegerEncoder.octetsNeeded(6, 63 + 0x00 + 0x80 * 0x80));
-        assertEquals(4, NBitIntegerEncoder.octetsNeeded(6, 63 + 0x00 + 0x80 * 0x80 * 0x80));
+        assertEquals(1, NBitIntegerEncoder.octetsNeeded(6, 62));
+        assertEquals(2, NBitIntegerEncoder.octetsNeeded(6, 63));
+        assertEquals(2, NBitIntegerEncoder.octetsNeeded(6, 64));
+        assertEquals(3, NBitIntegerEncoder.octetsNeeded(6, 63 + 0x00 + 0x80 * 0x01));
+        assertEquals(4, NBitIntegerEncoder.octetsNeeded(6, 63 + 0x00 + 0x80 * 0x80));
+        assertEquals(5, NBitIntegerEncoder.octetsNeeded(6, 63 + 0x00 + 0x80 * 0x80 * 0x80));
     }
 
     @Test
@@ -87,7 +88,7 @@ public void testEncode(int n, int i, String expected)
         String r = TypeUtil.toHexString(BufferUtil.toArray(buf));
         assertEquals(expected, r);
 
-        assertEquals(expected.length() / 2, (n < 8 ? 1 : 0) + NBitIntegerEncoder.octetsNeeded(n, i));
+        assertEquals(expected.length() / 2, NBitIntegerEncoder.octetsNeeded(n, i));
     }
 
     @Test
@@ -163,8 +164,7 @@ public void testEncodeExampleD12()
         NBitIntegerEncoder.encode(buf, 5, 1337);
         BufferUtil.flipToFlush(buf, p);
 
-        String r = TypeUtil.toHexString(BufferUtil.toArray(buf));
-
+        String r = StringUtil.toHexString(BufferUtil.toArray(buf));
         assertEquals("881f9a0a", r);
     }
 

jetty-http2/http2-hpack/src/main/java/org/eclipse/jetty/http2/hpack/HpackContext.java

@@ -464,7 +464,7 @@ public static class StaticEntry extends Entry
                 if (huffmanLen < 0)
                     throw new IllegalStateException("bad value");
                 int lenLen = NBitIntegerEncoder.octetsNeeded(7, huffmanLen);
-                _huffmanValue = new byte[1 + lenLen + huffmanLen];
+                _huffmanValue = new byte[lenLen + huffmanLen];
                 ByteBuffer buffer = ByteBuffer.wrap(_huffmanValue);
 
                 // Indicate Huffman

jetty-http2/http2-hpack/src/main/java/org/eclipse/jetty/http2/hpack/HpackEncoder.java

@@ -25,12 +25,12 @@
 import org.eclipse.jetty.http.HttpMethod;
 import org.eclipse.jetty.http.HttpScheme;
 import org.eclipse.jetty.http.HttpStatus;
-import org.eclipse.jetty.http.HttpTokens;
 import org.eclipse.jetty.http.HttpVersion;
 import org.eclipse.jetty.http.MetaData;
 import org.eclipse.jetty.http.PreEncodedHttpField;
 import org.eclipse.jetty.http.compression.HuffmanEncoder;
 import org.eclipse.jetty.http.compression.NBitIntegerEncoder;
+import org.eclipse.jetty.http.compression.NBitStringEncoder;
 import org.eclipse.jetty.http2.hpack.HpackContext.Entry;
 import org.eclipse.jetty.http2.hpack.HpackContext.StaticEntry;
 import org.eclipse.jetty.util.BufferUtil;
@@ -337,7 +337,7 @@ public void encode(ByteBuffer buffer, HttpField field)
                 buffer.put((byte)0x80);
                 NBitIntegerEncoder.encode(buffer, 7, index);
                 if (_debug)
-                    encoding = "IdxField" + (entry.isStatic() ? "S" : "") + (1 + NBitIntegerEncoder.octetsNeeded(7, index));
+                    encoding = "IdxField" + (entry.isStatic() ? "S" : "") + NBitIntegerEncoder.octetsNeeded(7, index);
             }
         }
         else
@@ -470,25 +470,6 @@ private void encodeName(ByteBuffer buffer, byte mask, int bits, String name, Ent
 
     static void encodeValue(ByteBuffer buffer, boolean huffman, String value)
     {
-        if (huffman)
-        {
-            // huffman literal value
-            buffer.put((byte)0x80);
-            int needed = HuffmanEncoder.octetsNeeded(value);
-            NBitIntegerEncoder.encode(buffer, 7, needed);
-            HuffmanEncoder.encode(buffer, value);
-        }
-        else
-        {
-            // add literal assuming iso_8859_1
-            buffer.put((byte)0x00).mark();
-            NBitIntegerEncoder.encode(buffer, 7, value.length());
-            for (int i = 0; i < value.length(); i++)
-            {
-                char c = value.charAt(i);
-                c = HttpTokens.sanitizeFieldVchar(c);
-                buffer.put((byte)c);
-            }
-        }
+        NBitStringEncoder.encode(buffer, 8, value, huffman);
     }
 }