From 6143e4358f621c76ff6393656aa7626680907b3e Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Wed, 30 Mar 2022 11:02:30 +1100 Subject: [PATCH 1/3] do not expect section ack for zero required insert count Signed-off-by: Lachlan Roberts --- .../java/org/eclipse/jetty/http3/qpack/QpackDecoder.java | 9 ++++++--- .../java/org/eclipse/jetty/http3/qpack/QpackEncoder.java | 8 ++++++++ 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/jetty-http3/http3-qpack/src/main/java/org/eclipse/jetty/http3/qpack/QpackDecoder.java b/jetty-http3/http3-qpack/src/main/java/org/eclipse/jetty/http3/qpack/QpackDecoder.java index 4129979ca68b..5beff005e929 100644 --- a/jetty-http3/http3-qpack/src/main/java/org/eclipse/jetty/http3/qpack/QpackDecoder.java +++ b/jetty-http3/http3-qpack/src/main/java/org/eclipse/jetty/http3/qpack/QpackDecoder.java @@ -166,7 +166,8 @@ public boolean decode(long streamId, ByteBuffer buffer, Handler handler) throws if (LOG.isDebugEnabled()) LOG.debug("Decoded: streamId={}, metadata={}", streamId, metaData); _metaDataNotifications.add(new MetaDataNotification(streamId, metaData, handler)); - _instructions.add(new SectionAcknowledgmentInstruction(streamId)); + if (requiredInsertCount > 0) + _instructions.add(new SectionAcknowledgmentInstruction(streamId)); } else { @@ -236,7 +237,8 @@ private void checkEncodedFieldSections() throws QpackException int insertCount = _context.getDynamicTable().getInsertCount(); for (EncodedFieldSection encodedFieldSection : _encodedFieldSections) { - if (encodedFieldSection.getRequiredInsertCount() <= insertCount) + int requiredInsertCount = encodedFieldSection.getRequiredInsertCount(); + if (requiredInsertCount <= insertCount) { long streamId = encodedFieldSection.getStreamId(); MetaData metaData = encodedFieldSection.decode(_context, _maxHeaderSize); @@ -244,7 +246,8 @@ private void checkEncodedFieldSections() throws QpackException LOG.debug("Decoded: streamId={}, metadata={}", streamId, metaData); _metaDataNotifications.add(new MetaDataNotification(streamId, metaData, encodedFieldSection.getHandler())); - _instructions.add(new SectionAcknowledgmentInstruction(streamId)); + if (requiredInsertCount > 0) + _instructions.add(new SectionAcknowledgmentInstruction(streamId)); } } } diff --git a/jetty-http3/http3-qpack/src/main/java/org/eclipse/jetty/http3/qpack/QpackEncoder.java b/jetty-http3/http3-qpack/src/main/java/org/eclipse/jetty/http3/qpack/QpackEncoder.java index 7105b5257bf9..e96ba17767fd 100644 --- a/jetty-http3/http3-qpack/src/main/java/org/eclipse/jetty/http3/qpack/QpackEncoder.java +++ b/jetty-http3/http3-qpack/src/main/java/org/eclipse/jetty/http3/qpack/QpackEncoder.java @@ -193,7 +193,15 @@ public void encode(ByteBuffer buffer, long streamId, MetaData metadata) throws Q requiredInsertCount = entryRequiredInsertCount; } + // We should not expect section acknowledgements for 0 required insert count. sectionInfo.setRequiredInsertCount(requiredInsertCount); + if (requiredInsertCount == 0) + { + streamInfo.remove(sectionInfo); + if (streamInfo.isEmpty()) + _streamInfoMap.remove(streamId); + } + int base = dynamicTable.getBase(); int encodedInsertCount = encodeInsertCount(requiredInsertCount, dynamicTable.getCapacity()); boolean signBit = base < requiredInsertCount; From 685d617a1b5b456ed4baa479e52da39c6e042cde Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Thu, 5 May 2022 16:09:55 +1000 Subject: [PATCH 2/3] Issue #7802 - add testing for qpack section acknowledgement Signed-off-by: Lachlan Roberts --- .../jetty/http3/qpack/QpackTestUtil.java | 60 ++++++++++++++ .../qpack/SectionAcknowledgmentTest.java | 79 +++++++++++++++++++ 2 files changed, 139 insertions(+) create mode 100644 jetty-http3/http3-qpack/src/test/java/org/eclipse/jetty/http3/qpack/SectionAcknowledgmentTest.java diff --git a/jetty-http3/http3-qpack/src/test/java/org/eclipse/jetty/http3/qpack/QpackTestUtil.java b/jetty-http3/http3-qpack/src/test/java/org/eclipse/jetty/http3/qpack/QpackTestUtil.java index 5645bdfd6054..78174a1965b7 100644 --- a/jetty-http3/http3-qpack/src/test/java/org/eclipse/jetty/http3/qpack/QpackTestUtil.java +++ b/jetty-http3/http3-qpack/src/test/java/org/eclipse/jetty/http3/qpack/QpackTestUtil.java @@ -16,6 +16,10 @@ import java.nio.ByteBuffer; import java.util.List; +import org.eclipse.jetty.http.HttpField; +import org.eclipse.jetty.http.HttpFields; +import org.eclipse.jetty.http.HttpVersion; +import org.eclipse.jetty.http.MetaData; import org.eclipse.jetty.io.ByteBufferPool; import org.eclipse.jetty.io.NullByteBufferPool; import org.eclipse.jetty.util.BufferUtil; @@ -27,6 +31,23 @@ public class QpackTestUtil { + public static ByteBuffer toBuffer(Instruction... instructions) + { + ByteBufferPool.Lease lease = new ByteBufferPool.Lease(new NullByteBufferPool()); + for (Instruction instruction : instructions) + { + instruction.encode(lease); + } + ByteBuffer combinedBuffer = BufferUtil.allocate(Math.toIntExact(lease.getTotalLength())); + BufferUtil.clearToFill(combinedBuffer); + for (ByteBuffer buffer : lease.getByteBuffers()) + { + combinedBuffer.put(buffer); + } + BufferUtil.flipToFlush(combinedBuffer, 0); + return combinedBuffer; + } + public static Matcher equalsHex(String expectedString) { expectedString = expectedString.replaceAll("\\s+", ""); @@ -56,4 +77,43 @@ public static String toHexString(Instruction instruction) { return BufferUtil.toHexString(toBuffer(List.of(instruction))); } + + public static ByteBuffer encode(QpackEncoder encoder, long streamId, MetaData metaData) throws QpackException + { + ByteBuffer buffer = BufferUtil.allocate(1024); + BufferUtil.clearToFill(buffer); + encoder.encode(buffer, streamId, metaData); + BufferUtil.flipToFlush(buffer, 0); + return buffer; + } + + public static HttpFields.Mutable toHttpFields(HttpField field) + { + return HttpFields.build().add(field); + } + + public static MetaData toMetaData(String name, String value) + { + return toMetaData(toHttpFields(new HttpField(name, value))); + } + + public static MetaData toMetaData(String method, String path, String scheme) + { + return toMetaData(method, path, scheme, null); + } + + public static MetaData toMetaData(String method, String path, String scheme, HttpFields.Mutable fields) + { + if (fields == null) + fields = HttpFields.build(); + fields.put(":scheme", scheme); + fields.put(":method", method); + fields.put(":path", path); + return new MetaData(HttpVersion.HTTP_3, fields); + } + + public static MetaData toMetaData(HttpFields fields) + { + return new MetaData(HttpVersion.HTTP_3, fields); + } } diff --git a/jetty-http3/http3-qpack/src/test/java/org/eclipse/jetty/http3/qpack/SectionAcknowledgmentTest.java b/jetty-http3/http3-qpack/src/test/java/org/eclipse/jetty/http3/qpack/SectionAcknowledgmentTest.java new file mode 100644 index 000000000000..b0446f304fdf --- /dev/null +++ b/jetty-http3/http3-qpack/src/test/java/org/eclipse/jetty/http3/qpack/SectionAcknowledgmentTest.java @@ -0,0 +1,79 @@ +// +// ======================================================================== +// Copyright (c) 1995-2022 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.http3.qpack.internal.instruction.SectionAcknowledgmentInstruction; +import org.eclipse.jetty.util.BufferUtil; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +import static org.eclipse.jetty.http3.qpack.QpackTestUtil.encode; +import static org.eclipse.jetty.http3.qpack.QpackTestUtil.toBuffer; +import static org.eclipse.jetty.http3.qpack.QpackTestUtil.toMetaData; +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.containsString; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertThrows; + +public class SectionAcknowledgmentTest +{ + private static final int MAX_BLOCKED_STREAMS = 5; + private static final int MAX_HEADER_SIZE = 1024; + + private QpackEncoder _encoder; + private QpackDecoder _decoder; + private TestDecoderHandler _decoderHandler; + private TestEncoderHandler _encoderHandler; + + @BeforeEach + public void before() + { + _encoderHandler = new TestEncoderHandler(); + _decoderHandler = new TestDecoderHandler(); + _encoder = new QpackEncoder(_encoderHandler, MAX_BLOCKED_STREAMS); + _decoder = new QpackDecoder(_decoderHandler, MAX_HEADER_SIZE); + } + + @Test + public void testSectionAcknowledgmentForZeroRequiredInsertCountOnDecoder() throws Exception + { + // Encode a header with only a value contained in the static table. + ByteBuffer buffer = encode(_encoder, 0, toMetaData("GET", "/", "http")); + + // No instruction since no addition to table. + Instruction instruction = _encoderHandler.getInstruction(); + assertNull(instruction); + + // Decoding should generate no instruction. + _decoder.decode(0, buffer, _decoderHandler); + instruction = _decoderHandler.getInstruction(); + assertNull(instruction); + } + + @Test + public void testSectionAcknowledgmentForZeroRequiredInsertCountOnEncoder() throws Exception + { + // Encode a header with only a value contained in the static table. + ByteBuffer buffer = encode(_encoder, 0, toMetaData("GET", "/", "http")); + System.err.println(BufferUtil.toDetailString(buffer)); + + // Parsing a section ack instruction on the encoder when we are not expecting it should result in QPACK_DECODER_STREAM_ERROR. + SectionAcknowledgmentInstruction instruction = new SectionAcknowledgmentInstruction(0); + ByteBuffer instructionBuffer = toBuffer(instruction); + QpackException error = assertThrows(QpackException.class, () -> _encoder.parseInstructions(instructionBuffer)); + assertThat(error.getMessage(), containsString("No StreamInfo for 0")); + } +} From 2e98a26f036557e9b0e7aa80b598225dd5c22fbe Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Fri, 13 May 2022 13:49:10 +1000 Subject: [PATCH 3/3] added extra checks in SectionAcknowledgmentTest Signed-off-by: Lachlan Roberts --- .../jetty/http3/qpack/SectionAcknowledgmentTest.java | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/jetty-http3/http3-qpack/src/test/java/org/eclipse/jetty/http3/qpack/SectionAcknowledgmentTest.java b/jetty-http3/http3-qpack/src/test/java/org/eclipse/jetty/http3/qpack/SectionAcknowledgmentTest.java index b0446f304fdf..bc2eda81ce68 100644 --- a/jetty-http3/http3-qpack/src/test/java/org/eclipse/jetty/http3/qpack/SectionAcknowledgmentTest.java +++ b/jetty-http3/http3-qpack/src/test/java/org/eclipse/jetty/http3/qpack/SectionAcknowledgmentTest.java @@ -15,6 +15,7 @@ import java.nio.ByteBuffer; +import org.eclipse.jetty.http3.qpack.QpackException.SessionException; import org.eclipse.jetty.http3.qpack.internal.instruction.SectionAcknowledgmentInstruction; import org.eclipse.jetty.util.BufferUtil; import org.junit.jupiter.api.BeforeEach; @@ -25,6 +26,8 @@ import static org.eclipse.jetty.http3.qpack.QpackTestUtil.toMetaData; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.greaterThan; import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertThrows; @@ -68,12 +71,13 @@ public void testSectionAcknowledgmentForZeroRequiredInsertCountOnEncoder() throw { // Encode a header with only a value contained in the static table. ByteBuffer buffer = encode(_encoder, 0, toMetaData("GET", "/", "http")); - System.err.println(BufferUtil.toDetailString(buffer)); + assertThat(BufferUtil.remaining(buffer), greaterThan(0L)); // Parsing a section ack instruction on the encoder when we are not expecting it should result in QPACK_DECODER_STREAM_ERROR. SectionAcknowledgmentInstruction instruction = new SectionAcknowledgmentInstruction(0); ByteBuffer instructionBuffer = toBuffer(instruction); - QpackException error = assertThrows(QpackException.class, () -> _encoder.parseInstructions(instructionBuffer)); + SessionException error = assertThrows(SessionException.class, () -> _encoder.parseInstructions(instructionBuffer)); + assertThat(error.getErrorCode(), equalTo(QpackException.QPACK_ENCODER_STREAM_ERROR)); assertThat(error.getMessage(), containsString("No StreamInfo for 0")); } }