Jetty 9.4.x #4890 do not index large fields (#4927)

* Issue #4890 Large HTTP2 Fields encoded

When choosing a strategy to encode a HTTP2 field, do not use indexed
if the field is larger than the dynamic table.

Signed-off-by: Greg Wilkins <gregw@webtide.com>

* Issue #4890 Large HTTP2 Fields encoded

Fixed checkstyle in test

Signed-off-by: Greg Wilkins <gregw@webtide.com>

* Issue #4890 Large HTTP2 Fields encoded

Only index 0 content-length

Signed-off-by: Greg Wilkins <gregw@webtide.com>

* Issue #4890 Large HTTP2 Fields encoded

Fixed comments

Signed-off-by: Greg Wilkins <gregw@webtide.com>

* Issue #4890 Large HTTP2 Fields encoded

Don't parse int

Signed-off-by: Greg Wilkins <gregw@webtide.com>

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

@@ -299,11 +299,11 @@ public class HpackEncoder
 
         String encoding = null;
 
-        // Is there an entry for the field?
+        // Is there an index entry for the field?
         Entry entry = _context.get(field);
         if (entry != null)
         {
-            // Known field entry, so encode it as indexed
+            // This is a known indexed field, send as static or dynamic indexed.
             if (entry.isStatic())
             {
                 buffer.put(((StaticEntry)entry).getEncodedField());
@@ -321,10 +321,10 @@ public class HpackEncoder
         }
         else
         {
-            // Unknown field entry, so we will have to send literally.
+            // Unknown field entry, so we will have to send literally, but perhaps add an index.
             final boolean indexed;
 
-            // But do we know it's name?
+            // Do we know its name?
             HttpHeader header = field.getHeader();
 
             // Select encoding strategy
@@ -342,12 +342,11 @@ public class HpackEncoder
                     if (_debug)
                         encoding = indexed ? "PreEncodedIdx" : "PreEncoded";
                 }
-                // has the custom header name been seen before?
+                else if (name == null && fieldSize < _context.getMaxDynamicTableSize())
-                else if (name == null)
                 {
-                    // unknown name and value, so let's index this just in case it is
+                    // unknown name and value that will fit in dynamic table, so let's index
-                    // the first time we have seen a custom name or a custom field.
+                    // this just in case it is the first time we have seen a custom name or a
-                    // unless the name is changing, this is worthwhile
+                    // custom field.  Unless the name is once only, this is worthwhile
                     indexed = true;
                     encodeName(buffer, (byte)0x40, 6, field.getName(), null);
                     encodeValue(buffer, true, field.getValue());
@@ -356,7 +355,7 @@ public class HpackEncoder
                 }
                 else
                 {
-                    // known custom name, but unknown value.
+                    // Known name, but different value.
                     // This is probably a custom field with changing value, so don't index.
                     indexed = false;
                     encodeName(buffer, (byte)0x00, 4, field.getName(), null);
@@ -395,9 +394,9 @@ public class HpackEncoder
                             (huffman ? "HuffV" : "LitV") +
                             (neverIndex ? "!!Idx" : "!Idx");
                 }
-                else if (fieldSize >= _context.getMaxDynamicTableSize() || header == HttpHeader.CONTENT_LENGTH && field.getValue().length() > 2)
+                else if (fieldSize >= _context.getMaxDynamicTableSize() || header == HttpHeader.CONTENT_LENGTH && !"0".equals(field.getValue()))
                 {
-                    // Non indexed if field too large or a content length for 3 digits or more
+                    // The field is too large or a non zero content length, so do not index.
                     indexed = false;
                     encodeName(buffer, (byte)0x00, 4, header.asString(), name);
                     encodeValue(buffer, true, field.getValue());

jetty-http2/http2-hpack/src/test/java/org/eclipse/jetty/http2/hpack/HpackEncoderTest.java

@@ -22,6 +22,7 @@ import java.nio.ByteBuffer;
 
 import org.eclipse.jetty.http.HttpField;
 import org.eclipse.jetty.http.HttpFields;
+import org.eclipse.jetty.http.HttpHeader;
 import org.eclipse.jetty.http.HttpVersion;
 import org.eclipse.jetty.http.MetaData;
 import org.eclipse.jetty.util.BufferUtil;
@@ -145,6 +146,54 @@ public class HpackEncoderTest
         assertEquals(5, encoder.getHpackContext().size());
     }
 
+    @Test
+    public void testLargeFieldsNotIndexed()
+    {
+        HpackEncoder encoder = new HpackEncoder(38 * 5);
+        HpackContext ctx = encoder.getHpackContext();
+
+        ByteBuffer buffer = BufferUtil.allocate(4096);
+
+        // Index little fields
+        int pos = BufferUtil.flipToFill(buffer);
+        encoder.encode(buffer, new HttpField("Name", "Value"));
+        BufferUtil.flipToFlush(buffer, pos);
+        int dynamicTableSize = ctx.getDynamicTableSize();
+        assertThat(dynamicTableSize, Matchers.greaterThan(0));
+
+        // Do not index big field
+        StringBuilder largeName = new StringBuilder("largeName-");
+        String filler = "XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX";
+        while (largeName.length() < ctx.getMaxDynamicTableSize())
+            largeName.append(filler, 0, Math.min(filler.length(), ctx.getMaxDynamicTableSize() - largeName.length()));
+        pos = BufferUtil.flipToFill(buffer);
+        encoder.encode(buffer, new HttpField(largeName.toString(), "Value"));
+        BufferUtil.flipToFlush(buffer, pos);
+        assertThat(ctx.getDynamicTableSize(), Matchers.is(dynamicTableSize));
+    }
+
+    @Test
+    public void testIndexContentLength()
+    {
+        HpackEncoder encoder = new HpackEncoder(38 * 5);
+        HpackContext ctx = encoder.getHpackContext();
+
+        ByteBuffer buffer = BufferUtil.allocate(4096);
+
+        // Index zero content length
+        int pos = BufferUtil.flipToFill(buffer);
+        encoder.encode(buffer, new HttpField(HttpHeader.CONTENT_LENGTH, "0"));
+        BufferUtil.flipToFlush(buffer, pos);
+        int dynamicTableSize = ctx.getDynamicTableSize();
+        assertThat(dynamicTableSize, Matchers.greaterThan(0));
+
+        // Do not index non zero content length
+        pos = BufferUtil.flipToFill(buffer);
+        encoder.encode(buffer, new HttpField(HttpHeader.CONTENT_LENGTH, "42"));
+        BufferUtil.flipToFlush(buffer, pos);
+        assertThat(ctx.getDynamicTableSize(), Matchers.is(dynamicTableSize));
+    }
+
     @Test
     public void testNeverIndexSetCookie() throws Exception
     {