From 1ee79f548c0187c72306732db7b576895227542d Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Mon, 1 May 2023 12:12:15 +1000 Subject: [PATCH] Issue #9554 - additional cleanups and testing Signed-off-by: Lachlan Roberts --- .../jetty/http2/hpack/MetaDataBuilder.java | 7 ++-- .../http3/qpack/internal/EncodableEntry.java | 21 ++++++++++-- .../LiteralNameEntryInstruction.java | 2 +- .../qpack/DecoderInstructionParserTest.java | 32 +++++++++++++++++++ 4 files changed, 57 insertions(+), 5 deletions(-) diff --git a/jetty-http2/http2-hpack/src/main/java/org/eclipse/jetty/http2/hpack/MetaDataBuilder.java b/jetty-http2/http2-hpack/src/main/java/org/eclipse/jetty/http2/hpack/MetaDataBuilder.java index 151ff9308fb..e431aa9ffd9 100644 --- a/jetty-http2/http2-hpack/src/main/java/org/eclipse/jetty/http2/hpack/MetaDataBuilder.java +++ b/jetty-http2/http2-hpack/src/main/java/org/eclipse/jetty/http2/hpack/MetaDataBuilder.java @@ -290,10 +290,13 @@ public class MetaDataBuilder */ public void checkSize(int length, boolean huffman) throws SessionException { + if (length < 0) + throw new IllegalArgumentException(); + // Apply a huffman fudge factor if (huffman) - length = (length * 4) / 3; - if ((_size + length) > _maxSize) + length = Math.multiplyExact(length, 4) / 3; + if (Math.addExact(_size, length) > _maxSize) throw new SessionException("Header too large %d > %d", _size + length, _maxSize); } } diff --git a/jetty-http3/http3-qpack/src/main/java/org/eclipse/jetty/http3/qpack/internal/EncodableEntry.java b/jetty-http3/http3-qpack/src/main/java/org/eclipse/jetty/http3/qpack/internal/EncodableEntry.java index 542232dca9c..4d354b43544 100644 --- a/jetty-http3/http3-qpack/src/main/java/org/eclipse/jetty/http3/qpack/internal/EncodableEntry.java +++ b/jetty-http3/http3-qpack/src/main/java/org/eclipse/jetty/http3/qpack/internal/EncodableEntry.java @@ -182,9 +182,26 @@ public abstract class EncodableEntry public int getRequiredSize(int base) { String value = getValue(); - int relativeIndex = _nameEntry.getIndex() - base; int valueLength = _huffman ? HuffmanEncoder.octetsNeeded(value) : value.length(); - return 1 + NBitIntegerEncoder.octetsNeeded(4, relativeIndex) + 1 + NBitIntegerEncoder.octetsNeeded(7, valueLength) + valueLength; + + int nameOctets; + if (_nameEntry.isStatic()) + { + int relativeIndex = _nameEntry.getIndex(); + nameOctets = NBitIntegerEncoder.octetsNeeded(4, relativeIndex); + } + else if (_nameEntry.getIndex() < base) + { + int relativeIndex = base - (_nameEntry.getIndex() + 1); + nameOctets = NBitIntegerEncoder.octetsNeeded(4, relativeIndex); + } + else + { + int relativeIndex = _nameEntry.getIndex() - base; + nameOctets = NBitIntegerEncoder.octetsNeeded(3, relativeIndex); + } + + return 1 + nameOctets + 1 + NBitIntegerEncoder.octetsNeeded(7, valueLength) + valueLength; } @Override diff --git a/jetty-http3/http3-qpack/src/main/java/org/eclipse/jetty/http3/qpack/internal/instruction/LiteralNameEntryInstruction.java b/jetty-http3/http3-qpack/src/main/java/org/eclipse/jetty/http3/qpack/internal/instruction/LiteralNameEntryInstruction.java index 8ebed39a0c0..79947c0a43f 100644 --- a/jetty-http3/http3-qpack/src/main/java/org/eclipse/jetty/http3/qpack/internal/instruction/LiteralNameEntryInstruction.java +++ b/jetty-http3/http3-qpack/src/main/java/org/eclipse/jetty/http3/qpack/internal/instruction/LiteralNameEntryInstruction.java @@ -82,7 +82,7 @@ public class LiteralNameEntryInstruction implements Instruction else { buffer.put((byte)(0x00)); - NBitIntegerEncoder.encode(buffer, 5, _value.length()); + NBitIntegerEncoder.encode(buffer, 7, _value.length()); buffer.put(_value.getBytes(StandardCharsets.ISO_8859_1)); } diff --git a/jetty-http3/http3-qpack/src/test/java/org/eclipse/jetty/http3/qpack/DecoderInstructionParserTest.java b/jetty-http3/http3-qpack/src/test/java/org/eclipse/jetty/http3/qpack/DecoderInstructionParserTest.java index 6bcc7ab239e..460a4e92c0e 100644 --- a/jetty-http3/http3-qpack/src/test/java/org/eclipse/jetty/http3/qpack/DecoderInstructionParserTest.java +++ b/jetty-http3/http3-qpack/src/test/java/org/eclipse/jetty/http3/qpack/DecoderInstructionParserTest.java @@ -14,18 +14,26 @@ package org.eclipse.jetty.http3.qpack; import java.nio.ByteBuffer; +import java.util.List; +import org.eclipse.jetty.http3.qpack.internal.instruction.DuplicateInstruction; +import org.eclipse.jetty.http3.qpack.internal.instruction.IndexedNameEntryInstruction; +import org.eclipse.jetty.http3.qpack.internal.instruction.SetCapacityInstruction; import org.eclipse.jetty.http3.qpack.internal.parser.DecoderInstructionParser; +import org.eclipse.jetty.io.ByteBufferPool; +import org.eclipse.jetty.io.NullByteBufferPool; 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.hamcrest.Matchers.is; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertTrue; public class DecoderInstructionParserTest { + private final NullByteBufferPool bufferPool = new NullByteBufferPool(); private DecoderInstructionParser _instructionParser; private DecoderParserDebugHandler _handler; @@ -41,6 +49,11 @@ public class DecoderInstructionParserTest { // Set Dynamic Table Capacity=220. ByteBuffer buffer = QpackTestUtil.hexToBuffer("3fbd 01"); + + // Assert that our generated value is equal to that of the spec example. + ByteBuffer encodedValue = getEncodedValue(new SetCapacityInstruction(220)); + assertThat(buffer, equalTo(encodedValue)); + _instructionParser.parse(buffer); assertThat(_handler.setCapacities.poll(), is(220)); assertTrue(_handler.isEmpty()); @@ -51,6 +64,11 @@ public class DecoderInstructionParserTest { // Duplicate (Relative Index = 2). ByteBuffer buffer = QpackTestUtil.hexToBuffer("02"); + + // Assert that our generated value is equal to that of the spec example. + ByteBuffer encodedValue = getEncodedValue(new DuplicateInstruction(2)); + assertThat(buffer, equalTo(encodedValue)); + _instructionParser.parse(buffer); assertThat(_handler.duplicates.poll(), is(2)); assertTrue(_handler.isEmpty()); @@ -61,6 +79,11 @@ public class DecoderInstructionParserTest { // Insert With Name Reference to Static Table, Index=0 (:authority=www.example.com). ByteBuffer buffer = QpackTestUtil.hexToBuffer("c00f 7777 772e 6578 616d 706c 652e 636f 6d"); + + // Assert that our generated value is equal to that of the spec example. + ByteBuffer encodedValue = getEncodedValue(new IndexedNameEntryInstruction(false, 0, false, "www.example.com")); + assertThat(buffer, equalTo(encodedValue)); + _instructionParser.parse(buffer); DecoderParserDebugHandler.ReferencedEntry entry = _handler.referencedNameEntries.poll(); assertNotNull(entry); @@ -94,4 +117,13 @@ public class DecoderInstructionParserTest // There are no other instructions received. assertTrue(_handler.isEmpty()); } + + private ByteBuffer getEncodedValue(Instruction instruction) + { + ByteBufferPool.Lease lease = new ByteBufferPool.Lease(bufferPool); + instruction.encode(lease); + List byteBuffers = lease.getByteBuffers(); + assertThat(byteBuffers.size(), equalTo(1)); + return byteBuffers.get(0); + } }