From 646010e30905a3eda2d0a1b19e84ba179e69c87d Mon Sep 17 00:00:00 2001 From: Greg Wilkins Date: Mon, 1 Jun 2020 14:12:50 +0200 Subject: [PATCH] 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 * Issue #4890 Large HTTP2 Fields encoded Fixed checkstyle in test Signed-off-by: Greg Wilkins * Issue #4890 Large HTTP2 Fields encoded Only index 0 content-length Signed-off-by: Greg Wilkins * Issue #4890 Large HTTP2 Fields encoded Fixed comments Signed-off-by: Greg Wilkins * Issue #4890 Large HTTP2 Fields encoded Don't parse int Signed-off-by: Greg Wilkins --- .../jetty/http2/hpack/HpackEncoder.java | 23 +++++---- .../jetty/http2/hpack/HpackEncoderTest.java | 49 +++++++++++++++++++ 2 files changed, 60 insertions(+), 12 deletions(-) diff --git a/jetty-http2/http2-hpack/src/main/java/org/eclipse/jetty/http2/hpack/HpackEncoder.java b/jetty-http2/http2-hpack/src/main/java/org/eclipse/jetty/http2/hpack/HpackEncoder.java index 0788806b34b..444b100afde 100644 --- a/jetty-http2/http2-hpack/src/main/java/org/eclipse/jetty/http2/hpack/HpackEncoder.java +++ b/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) + else if (name == null && fieldSize < _context.getMaxDynamicTableSize()) { - // unknown name and value, so let's index this just in case it is - // the first time we have seen a custom name or a custom field. - // unless the name is changing, this is worthwhile + // unknown name and value that will fit in dynamic table, so let's index + // this just in case it is the first time we have seen a custom name or a + // 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()); diff --git a/jetty-http2/http2-hpack/src/test/java/org/eclipse/jetty/http2/hpack/HpackEncoderTest.java b/jetty-http2/http2-hpack/src/test/java/org/eclipse/jetty/http2/hpack/HpackEncoderTest.java index e60ea65ba11..f9d8367dde5 100644 --- a/jetty-http2/http2-hpack/src/test/java/org/eclipse/jetty/http2/hpack/HpackEncoderTest.java +++ b/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 {