This is an automated email from the ASF dual-hosted git repository.
markt-asf pushed a commit to branch 9.0.x
in repository https://gitbox.apache.org/repos/asf/tomcat.git
The following commit(s) were added to refs/heads/9.0.x by this push:
new a72501c665 Review "Cap the header frame payload buffer expansion..."
a72501c665 is described below
commit a72501c6656eae8893865013afaaa7d2b8c51950
Author: Mark Thomas <[email protected]>
AuthorDate: Mon Nov 2 15:29:51 2026 +0000
Review "Cap the header frame payload buffer expansion..."
Need to account for the potential encoding/compression.
Impose a lower limit where practical.
Co-authored by: Claude / Sonnet 5.5
---
.../coyote/http2/Http2AsyncUpgradeHandler.java | 20 +-
.../apache/coyote/http2/Http2UpgradeHandler.java | 113 ++++---
.../apache/coyote/http2/LocalStrings.properties | 3 +-
.../coyote/http2/TestHttp2AsyncUpgradeHandler.java | 19 +-
.../coyote/http2/TestHttp2UpgradeHandler.java | 356 +++++++++++++++++++++
webapps/docs/changelog.xml | 9 +
6 files changed, 461 insertions(+), 59 deletions(-)
diff --git a/java/org/apache/coyote/http2/Http2AsyncUpgradeHandler.java
b/java/org/apache/coyote/http2/Http2AsyncUpgradeHandler.java
index 5aff09147c..fd85396311 100644
--- a/java/org/apache/coyote/http2/Http2AsyncUpgradeHandler.java
+++ b/java/org/apache/coyote/http2/Http2AsyncUpgradeHandler.java
@@ -224,7 +224,7 @@ public class Http2AsyncUpgradeHandler extends
Http2UpgradeHandler {
@Override
protected HeaderFrameBuffers getHeaderFrameBuffers(int initialPayloadSize)
{
- return new AsyncHeaderFrameBuffers(initialPayloadSize);
+ return new AsyncHeaderFrameBuffers(initialPayloadSize,
getHeaderWorkspaceLimit());
}
@@ -538,14 +538,16 @@ public class Http2AsyncUpgradeHandler extends
Http2UpgradeHandler {
private static class AsyncHeaderFrameBuffers implements HeaderFrameBuffers
{
int payloadSize;
+ private final int workspaceLimit;
private byte[] header;
private ByteBuffer payload;
private final List<ByteBuffer> bufs = new ArrayList<>();
- AsyncHeaderFrameBuffers(int initialPayloadSize) {
+ AsyncHeaderFrameBuffers(int initialPayloadSize, int workspaceLimit) {
this.payloadSize = initialPayloadSize;
+ this.workspaceLimit = workspaceLimit;
}
@Override
@@ -576,15 +578,13 @@ public class Http2AsyncUpgradeHandler extends
Http2UpgradeHandler {
@Override
public void expandPayload() {
- // The encoded headers are written as the payload of a single
- // frame so a buffer larger than the largest frame size that can
- // be represented is never usable. Fail rather than continue
- // doubling, which would eventually overflow the buffer size.
- if (payloadSize >= ConnectionSettingsBase.MAX_MAX_FRAME_SIZE) {
- throw new
IllegalStateException(sm.getString("upgradeHandler.headerPayloadTooBig",
-
Integer.toString(ConnectionSettingsBase.MAX_MAX_FRAME_SIZE)));
+ // Use the same workspace ceiling as the synchronous writer. The
shared header encoding path checks
+ // the actual payload size before this frame can be queued for
writing.
+ if (payloadSize >= workspaceLimit) {
+ throw new IllegalStateException(
+ sm.getString("upgradeHandler.headerWorkspaceTooBig",
Integer.toString(workspaceLimit)));
}
- payloadSize = Integer.min(payloadSize * 2,
ConnectionSettingsBase.MAX_MAX_FRAME_SIZE);
+ payloadSize = Integer.min(payloadSize * 2, workspaceLimit);
payload = ByteBuffer.allocate(payloadSize);
}
}
diff --git a/java/org/apache/coyote/http2/Http2UpgradeHandler.java
b/java/org/apache/coyote/http2/Http2UpgradeHandler.java
index 84393431c4..1345ab32e9 100644
--- a/java/org/apache/coyote/http2/Http2UpgradeHandler.java
+++ b/java/org/apache/coyote/http2/Http2UpgradeHandler.java
@@ -878,39 +878,58 @@ class Http2UpgradeHandler extends AbstractStream
implements InternalHttpUpgradeH
boolean first = true;
State state = null;
- while (state != State.COMPLETE) {
- headerFrameBuffers.startFrame();
- if (first && pushedStreamIdBytes != null) {
- headerFrameBuffers.getPayload().put(pushedStreamIdBytes);
- }
- state = getHpackEncoder().encode(mimeHeaders,
headerFrameBuffers.getPayload());
- headerFrameBuffers.getPayload().flip();
- if (state == State.COMPLETE ||
headerFrameBuffers.getPayload().limit() > 0) {
- ByteUtil.setThreeBytes(headerFrameBuffers.getHeader(), 0,
headerFrameBuffers.getPayload().limit());
- if (first) {
- first = false;
- if (pushedStreamIdBytes == null) {
- headerFrameBuffers.getHeader()[3] =
FrameType.HEADERS.getIdByte();
+ try {
+ while (state != State.COMPLETE) {
+ headerFrameBuffers.startFrame();
+ if (first && pushedStreamIdBytes != null) {
+ headerFrameBuffers.getPayload().put(pushedStreamIdBytes);
+ }
+ state = getHpackEncoder().encode(mimeHeaders,
headerFrameBuffers.getPayload());
+ headerFrameBuffers.getPayload().flip();
+ // The workspace may be larger than a frame. Check the encoded
payload before serializing its length or
+ // passing it to either the synchronous writer or the
asynchronous frame queue.
+ int framePayloadSize =
headerFrameBuffers.getPayload().remaining();
+ int maxFramePayloadSize = remoteSettings.getMaxFrameSize();
+ if (framePayloadSize > maxFramePayloadSize) {
+ throw new
IllegalStateException(sm.getString("upgradeHandler.headerPayloadTooBig",
+ Integer.toString(framePayloadSize),
Integer.toString(maxFramePayloadSize)));
+ }
+ if (state == State.COMPLETE ||
headerFrameBuffers.getPayload().limit() > 0) {
+ ByteUtil.setThreeBytes(headerFrameBuffers.getHeader(), 0,
headerFrameBuffers.getPayload().limit());
+ if (first) {
+ first = false;
+ if (pushedStreamIdBytes == null) {
+ headerFrameBuffers.getHeader()[3] =
FrameType.HEADERS.getIdByte();
+ } else {
+ headerFrameBuffers.getHeader()[3] =
FrameType.PUSH_PROMISE.getIdByte();
+ }
+ if (endOfStream) {
+ headerFrameBuffers.getHeader()[4] =
FLAG_END_OF_STREAM;
+ }
} else {
- headerFrameBuffers.getHeader()[3] =
FrameType.PUSH_PROMISE.getIdByte();
+ headerFrameBuffers.getHeader()[3] =
FrameType.CONTINUATION.getIdByte();
}
- if (endOfStream) {
- headerFrameBuffers.getHeader()[4] = FLAG_END_OF_STREAM;
+ if (state == State.COMPLETE) {
+ headerFrameBuffers.getHeader()[4] +=
FLAG_END_OF_HEADERS;
}
- } else {
- headerFrameBuffers.getHeader()[3] =
FrameType.CONTINUATION.getIdByte();
- }
- if (state == State.COMPLETE) {
- headerFrameBuffers.getHeader()[4] += FLAG_END_OF_HEADERS;
- }
- if (log.isTraceEnabled()) {
- log.trace(headerFrameBuffers.getPayload().limit() + "
bytes");
+ if (log.isTraceEnabled()) {
+ log.trace(headerFrameBuffers.getPayload().limit() + "
bytes");
+ }
+ ByteUtil.set31Bits(headerFrameBuffers.getHeader(), 5,
stream.getIdAsInt());
+ headerFrameBuffers.endFrame();
+ } else if (state == State.UNDERFLOW) {
+ headerFrameBuffers.expandPayload();
}
- ByteUtil.set31Bits(headerFrameBuffers.getHeader(), 5,
stream.getIdAsInt());
- headerFrameBuffers.endFrame();
- } else if (state == State.UNDERFLOW) {
- headerFrameBuffers.expandPayload();
}
+ } catch (IllegalStateException ise) {
+ /*
+ * The header block can not be written. Frames of the block may
already have been written (synchronous
+ * I/O) and the HPACK encoder may have updated its dynamic table
for fields the peer has not received. The
+ * connection can not be used any further so it is closed. The
exception is re-thrown to make the calling
+ * code aware of the problem.
+ */
+ closeConnection(new ConnectionException(ise.getMessage(),
Http2Error.INTERNAL_ERROR, ise));
+ throw ise;
}
headerFrameBuffers.endHeaders();
return headerFrameBuffers;
@@ -924,7 +943,24 @@ class Http2UpgradeHandler extends AbstractStream
implements InternalHttpUpgradeH
* @return the header frame buffers
*/
protected HeaderFrameBuffers getHeaderFrameBuffers(int initialPayloadSize)
{
- return new DefaultHeaderFrameBuffers(initialPayloadSize);
+ return new DefaultHeaderFrameBuffers(initialPayloadSize,
getHeaderWorkspaceLimit());
+ }
+
+
+ /**
+ * Obtain the maximum size to which the header encoding workspace may be
expanded. HPACK reserves space using
+ * uncompressed header lengths before attempting Huffman encoding. The
shortest Huffman codes use five bits per
+ * octet, so literals may need up to 8/5 of their encoded size as
workspace. Twice the peer's maximum frame payload
+ * size allows for this estimate and encoding overhead. Indexed fields
still need space for this estimate even
+ * though their encoded representation is only a few bytes. Allow at least
the peer's dynamic table capacity so
+ * existing entries remain usable after a frame size reduction. The
32-byte overhead of a table entry exceeds the
+ * encoder's 12-byte estimate overhead. This is only a workspace limit:
frame payloads are checked separately
+ * against the peer's advertised maximum before they are written.
+ *
+ * @return the workspace limit in bytes
+ */
+ protected final int getHeaderWorkspaceLimit() {
+ return Math.max(2 * remoteSettings.getMaxFrameSize(),
remoteSettings.getHeaderTableSize());
}
@@ -2326,9 +2362,11 @@ class Http2UpgradeHandler extends AbstractStream
implements InternalHttpUpgradeH
private class DefaultHeaderFrameBuffers implements HeaderFrameBuffers {
private final byte[] header;
+ private final int workspaceLimit;
private ByteBuffer payload;
- DefaultHeaderFrameBuffers(int initialPayloadSize) {
+ DefaultHeaderFrameBuffers(int initialPayloadSize, int workspaceLimit) {
+ this.workspaceLimit = workspaceLimit;
header = new byte[9];
payload = ByteBuffer.allocate(initialPayloadSize);
}
@@ -2368,16 +2406,13 @@ class Http2UpgradeHandler extends AbstractStream
implements InternalHttpUpgradeH
@Override
public void expandPayload() {
- // The encoded headers are written as the payload of a single
- // frame so a buffer larger than the largest frame size that can
- // be represented is never usable. Fail rather than continue
- // doubling, which would eventually overflow the buffer size.
- if (payload.capacity() >=
ConnectionSettingsBase.MAX_MAX_FRAME_SIZE) {
- throw new
IllegalStateException(sm.getString("upgradeHandler.headerPayloadTooBig",
-
Integer.toString(ConnectionSettingsBase.MAX_MAX_FRAME_SIZE)));
+ // Bound the encoding workspace independently of the encoded frame
payload. The ceiling also keeps
+ // doubling below the point where the buffer size could overflow.
+ if (payload.capacity() >= workspaceLimit) {
+ throw new IllegalStateException(
+ sm.getString("upgradeHandler.headerWorkspaceTooBig",
Integer.toString(workspaceLimit)));
}
- payload = ByteBuffer.allocate(
- Integer.min(payload.capacity() * 2,
ConnectionSettingsBase.MAX_MAX_FRAME_SIZE));
+ payload = ByteBuffer.allocate(Integer.min(payload.capacity() * 2,
workspaceLimit));
}
}
}
diff --git a/java/org/apache/coyote/http2/LocalStrings.properties
b/java/org/apache/coyote/http2/LocalStrings.properties
index 4ba4eb677b..db84c53571 100644
--- a/java/org/apache/coyote/http2/LocalStrings.properties
+++ b/java/org/apache/coyote/http2/LocalStrings.properties
@@ -147,7 +147,8 @@ upgradeHandler.enableRfc7540Priorities=Connection [{0}],
RFC 7540 priorities may
upgradeHandler.fallToDebug=\n\
\ Note: further occurrences of HTTP/2 stream errors will be logged at DEBUG
level.
upgradeHandler.goaway.debug=Connection [{0}], Goaway, Last stream [{1}], Error
code [{2}], Debug data [{3}]
-upgradeHandler.headerPayloadTooBig=The encoded headers do not fit in a buffer
with a size equal to the maximum permitted frame size of [{0}] bytes
+upgradeHandler.headerPayloadTooBig=The encoded header frame payload size of
[{0}] bytes exceeds the maximum permitted frame size of [{1}] bytes
+upgradeHandler.headerWorkspaceTooBig=The header encoding workspace cannot be
expanded beyond [{0}] bytes
upgradeHandler.init=Connection [{0}], State [{1}]
upgradeHandler.invalidPreface=Connection [{0}], Invalid connection preface
upgradeHandler.ioerror=Connection [{0}]
diff --git a/test/org/apache/coyote/http2/TestHttp2AsyncUpgradeHandler.java
b/test/org/apache/coyote/http2/TestHttp2AsyncUpgradeHandler.java
index 2043a57a23..f9637de212 100644
--- a/test/org/apache/coyote/http2/TestHttp2AsyncUpgradeHandler.java
+++ b/test/org/apache/coyote/http2/TestHttp2AsyncUpgradeHandler.java
@@ -28,31 +28,32 @@ public class TestHttp2AsyncUpgradeHandler {
/*
* AsyncHeaderFrameBuffers is a private, static nested class so reflection
is used to test that payload expansion is
- * capped at the maximum frame size that can be represented in a frame
header. Previously the payload size was
+ * capped at twice the maximum frame size that can be represented in a
frame header. Previously the payload size was
* simply doubled on every expansion which, for very large header blocks,
eventually overflowed the size resulting
* in an attempt to allocate a buffer with a negative capacity.
*/
@Test
- public void testPayloadExpansionIsCapped() throws Exception {
+ public void testWorkspaceExpansionIsCapped() throws Exception {
Class<?> clazz =
Class.forName("org.apache.coyote.http2.Http2AsyncUpgradeHandler$AsyncHeaderFrameBuffers");
- Constructor<?> constructor = clazz.getDeclaredConstructor(int.class);
+ Constructor<?> constructor = clazz.getDeclaredConstructor(int.class,
int.class);
constructor.setAccessible(true);
- Object buffers = constructor.newInstance(Integer.valueOf(1024));
+ int workspaceLimit = 2 * ConnectionSettingsBase.MAX_MAX_FRAME_SIZE;
+ Object buffers = constructor.newInstance(Integer.valueOf(1024),
Integer.valueOf(workspaceLimit));
Method expandPayload = clazz.getDeclaredMethod("expandPayload");
expandPayload.setAccessible(true);
int expectedSize = 1024;
- while (expectedSize < ConnectionSettingsBase.MAX_MAX_FRAME_SIZE) {
+ while (expectedSize < workspaceLimit) {
expandPayload.invoke(buffers);
- expectedSize = Integer.min(expectedSize * 2,
ConnectionSettingsBase.MAX_MAX_FRAME_SIZE);
+ expectedSize = Integer.min(expectedSize * 2, workspaceLimit);
Assert.assertEquals(expectedSize, readPayloadSize(clazz, buffers));
}
- // A buffer that is already as large as the largest permitted frame
- // can not be expanded further
+ // A buffer at the workspace limit cannot be expanded further. The
encoded payload is checked separately
+ // before it is written, since it may be much smaller than the space
reserved for encoding.
try {
expandPayload.invoke(buffers);
- Assert.fail("Expansion beyond the maximum permitted frame size was
expected to fail.");
+ Assert.fail("Expansion beyond the maximum permitted workspace size
was expected to fail.");
} catch (InvocationTargetException e) {
Assert.assertEquals(IllegalStateException.class,
e.getCause().getClass());
}
diff --git a/test/org/apache/coyote/http2/TestHttp2UpgradeHandler.java
b/test/org/apache/coyote/http2/TestHttp2UpgradeHandler.java
index 5f7ad85939..154a7e0d34 100644
--- a/test/org/apache/coyote/http2/TestHttp2UpgradeHandler.java
+++ b/test/org/apache/coyote/http2/TestHttp2UpgradeHandler.java
@@ -16,19 +16,375 @@
*/
package org.apache.coyote.http2;
+import java.io.DataInputStream;
+import java.io.IOException;
+import java.net.SocketTimeoutException;
import java.nio.ByteBuffer;
import java.nio.charset.StandardCharsets;
+import java.util.ArrayList;
import java.util.Base64;
+import java.util.List;
+
+import javax.servlet.ServletException;
+import javax.servlet.http.HttpServlet;
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletResponse;
import org.junit.Assert;
import org.junit.Test;
import org.apache.catalina.Context;
import org.apache.catalina.startup.Tomcat;
+import org.apache.coyote.http2.Http2UpgradeHandler.HeaderFrameBuffers;
import org.apache.tomcat.util.http.Method;
+import org.apache.tomcat.util.http.MimeHeaders;
+import org.apache.tomcat.util.net.SocketWrapperBase;
+import org.easymock.EasyMock;
public class TestHttp2UpgradeHandler extends Http2TestBase {
+ @Test
+ public void testLargeCompressedHeader() throws Exception {
+ HeaderWriteTest test = new HeaderWriteTest(useAsyncIO,
ConnectionSettingsBase.MAX_MAX_FRAME_SIZE);
+ String value = repeat("a", ConnectionSettingsBase.MAX_MAX_FRAME_SIZE);
+
+ test.write(value);
+
+ // The encoder needs space for the uncompressed value before it
attempts Huffman encoding. The resulting
+ // payload fits in one frame even though the workspace must be larger
than the maximum frame payload.
+ Assert.assertTrue(test.buffers.maxWorkspaceSize >
ConnectionSettingsBase.MAX_MAX_FRAME_SIZE);
+ MimeHeaders decoded = new MimeHeaders();
+ HpackDecoder decoder = new HpackDecoder();
+ decoder.setMaxHeaderSize(-1);
+ decoder.setHeaderEmitter(new HpackDecoder.HeaderEmitter() {
+ @Override
+ public void emitHeader(String name, String headerValue) {
+ decoded.addValue(name).setString(headerValue);
+ }
+
+ @Override
+ public void setHeaderException(StreamException streamException) {
+ Assert.fail(streamException.getMessage());
+ }
+
+ @Override
+ public void validateHeaders() {
+ // NO-OP
+ }
+ });
+ for (ByteBuffer payload : test.buffers.payloads) {
+ decoder.decode(payload);
+ }
+ Assert.assertEquals(value, decoded.getHeader("x"));
+ }
+
+
+ @Test
+ public void testHeaderPayloadAtMaximumSize() throws Exception {
+ int limit = ConnectionSettingsBase.MAX_MAX_FRAME_SIZE;
+ HeaderWriteTest test = new HeaderWriteTest(useAsyncIO, limit);
+ // 'X' does not benefit from Huffman encoding. The literal
representation uses one byte for its type, two
+ // bytes for the name and five bytes for the value length at this size.
+ test.write(repeat("X", limit - 8));
+ Assert.assertEquals(limit, test.buffers.maxPayloadSize);
+ }
+
+
+ @Test
+ public void testHeaderPayloadExceedsMaximumSize() throws Exception {
+ int limit = ConnectionSettingsBase.MAX_MAX_FRAME_SIZE;
+ HeaderWriteTest test = new HeaderWriteTest(useAsyncIO, limit);
+ Assert.assertThrows(IllegalStateException.class, () ->
test.write(repeat("X", limit)));
+ test.assertConnectionClosedWithInternalError();
+ // Encoding must reach the larger workspace so the actual payload,
rather than the uncompressed estimate,
+ // is checked. The recorder also verifies that no oversized payload
reaches endFrame().
+ Assert.assertTrue(test.buffers.maxWorkspaceSize > limit);
+ }
+
+
+ @Test
+ public void testHeaderPayloadAtPeerMaximumSize() throws Exception {
+ int limit = ConnectionSettingsBase.MIN_MAX_FRAME_SIZE;
+ HeaderWriteTest test = new HeaderWriteTest(useAsyncIO, limit);
+ // At this size the value length occupies three bytes, giving six
bytes of overhead in total.
+ test.write(repeat("X", limit - 6));
+ Assert.assertEquals(limit, test.buffers.maxPayloadSize);
+ }
+
+
+ @Test
+ public void testHeaderPayloadExceedsPeerMaximumSize() throws Exception {
+ HeaderWriteTest test = new HeaderWriteTest(useAsyncIO,
ConnectionSettingsBase.MIN_MAX_FRAME_SIZE);
+ Assert.assertThrows(IllegalStateException.class,
+ () -> test.write(repeat("X",
ConnectionSettingsBase.MIN_MAX_FRAME_SIZE)));
+ test.assertConnectionClosedWithInternalError();
+ }
+
+
+ @Test
+ public void testHeaderBlockExceedsPeerMaximumAfterWorkspaceExpansion()
throws Exception {
+ int limit = ConnectionSettingsBase.MIN_MAX_FRAME_SIZE;
+ HeaderWriteTest test = new HeaderWriteTest(useAsyncIO, limit);
+ MimeHeaders headers = new MimeHeaders();
+ /*
+ * The first field needs more workspace than the peer's frame size
limit (its uncompressed length is reserved
+ * before Huffman encoding) so the workspace is expanded beyond that
limit. Once expanded, the encoder packs
+ * the second field into the same frame. The two fields compress to
more than the peer's limit in total so the
+ * block is rejected rather than being split across frames.
+ */
+ headers.addValue("x").setString(repeat("a", 20000));
+ headers.addValue("y").setString(repeat("X", limit / 2));
+
+ Assert.assertThrows(IllegalStateException.class, () ->
test.write(headers));
+ test.assertConnectionClosedWithInternalError();
+ Assert.assertTrue("The workspace was not expanded beyond the peer's
frame size limit",
+ test.buffers.maxWorkspaceSize > limit);
+ // The block must never have been completed
+ Assert.assertFalse("A complete header block was written",
test.buffers.endOfHeadersWritten);
+ }
+
+
+ @Test
+ public void testHeaderPayloadTooLargeClosesConnection() throws Exception {
+ enableHttp2();
+
+ Tomcat tomcat = getTomcatInstance();
+
+ Context ctxt = getProgrammaticRootContext();
+ Tomcat.addServlet(ctxt, "simple", new SimpleServlet());
+ ctxt.addServletMapping("/simple", "simple");
+ Tomcat.addServlet(ctxt, "oversized", new OversizedHeaderServlet());
+ ctxt.addServletMapping("/oversized", "oversized");
+
+ tomcat.start();
+
+ openClientConnection();
+ doHttpUpgrade();
+ sendClientPreface();
+ validateHttp2InitialResponse();
+
+ byte[] frameHeader = new byte[9];
+ ByteBuffer headersPayload = ByteBuffer.allocate(128);
+ buildGetRequest(frameHeader, headersPayload, null, 3, "/oversized");
+ writeFrame(frameHeader, headersPayload);
+
+ /*
+ * The response header can't be sent in a frame of the size the client
permits. With synchronous I/O the
+ * client may see the start of the header block but it must never see
it completed. The server must close the
+ * connection with a GOAWAY rather than leave the connection running
with an unfinished header block.
+ */
+ DataInputStream in = new DataInputStream(s.getInputStream());
+ try {
+ boolean goaway = false;
+ while (!goaway) {
+ parser.readFrame();
+ String trace = output.getTrace();
+ Assert.assertFalse(trace, trace.contains("3-HeadersEnd"));
+ goaway = trace.startsWith("0-Goaway-[");
+ if (goaway) {
+ Assert.assertTrue(trace, trace.startsWith(
+ "0-Goaway-[3]-[" +
Http2Error.INTERNAL_ERROR.getCode() + "]-["));
+ }
+ output.clearTrace();
+ }
+ } catch (ConnectionException ce) {
+ /*
+ * With synchronous I/O the client has received the start of the
header block when the GOAWAY arrives. The
+ * test parser (correctly) rejects any frame other than a
CONTINUATION at that point, having read only the
+ * frame header. Read the GOAWAY payload directly to check the
last stream ID and the error code.
+ */
+ Assert.assertTrue(ce.getMessage(),
ce.getMessage().contains("headers in progress for stream [3]"));
+ Assert.assertEquals(3, in.readInt());
+ Assert.assertEquals(Http2Error.INTERNAL_ERROR.getCode(),
in.readInt() & 0xFFFFFFFFL);
+ }
+
+ // The server must then close the TCP connection. Reading the end of
the stream must not time out.
+ byte[] discard = new byte[1024];
+ try {
+ while (in.read(discard) != -1) {
+ // Discard any remaining data (the rest of the GOAWAY frame)
+ }
+ } catch (SocketTimeoutException ste) {
+ Assert.fail("The connection was not closed after the GOAWAY");
+ }
+ }
+
+
+ @Test
+ public void testIndexedHeaderAfterFrameSizeReduction() throws Exception {
+ int tableSize = ConnectionSettingsBase.MAX_HEADER_TABLE_SIZE;
+ HeaderWriteTest test = new HeaderWriteTest(useAsyncIO, tableSize);
+ test.handler.getRemoteSettings().set(Setting.HEADER_TABLE_SIZE,
tableSize);
+ String value = repeat("X", 40000);
+
+ // Populate the dynamic table while the peer permits a frame large
enough for the literal field.
+ test.write(value);
+ Assert.assertTrue(test.buffers.maxPayloadSize >
ConnectionSettingsBase.MIN_MAX_FRAME_SIZE);
+
+ // Reducing the frame limit does not evict dynamic table entries. The
encoder still reserves space using
+ // the field's uncompressed lengths, even though its indexed
representation occupies only one byte.
+ int frameSize = ConnectionSettingsBase.MIN_MAX_FRAME_SIZE;
+ test.handler.getRemoteSettings().set(Setting.MAX_FRAME_SIZE,
frameSize);
+ test.write(value);
+
+ Assert.assertTrue(test.buffers.maxWorkspaceSize > 2 * frameSize);
+ Assert.assertEquals(tableSize, test.buffers.maxWorkspaceSize);
+ ByteBuffer indexedPayload =
test.buffers.payloads.get(test.buffers.payloads.size() - 1);
+ Assert.assertEquals(1, indexedPayload.remaining());
+ Assert.assertEquals(0x80 | (Hpack.STATIC_TABLE_LENGTH + 1),
indexedPayload.get() & 0xFF);
+ }
+
+
+ @Test
+ public void testHeaderWorkspaceExpansionIsCapped() throws Exception {
+ HeaderWriteTest test = new HeaderWriteTest(useAsyncIO,
ConnectionSettingsBase.MAX_MAX_FRAME_SIZE);
+ HeaderFrameBuffers buffers =
test.handler.getHeaderFrameBuffers(ConnectionSettingsBase.MAX_MAX_FRAME_SIZE);
+ buffers.startFrame();
+ buffers.expandPayload();
+ Assert.assertEquals(2 * ConnectionSettingsBase.MAX_MAX_FRAME_SIZE,
buffers.getPayload().capacity());
+ Assert.assertThrows(IllegalStateException.class,
buffers::expandPayload);
+ }
+
+
+ private static String repeat(String s, int count) {
+ StringBuilder sb = new StringBuilder(s.length() * count);
+ for (int i = 0; i < count; i++) {
+ sb.append(s);
+ }
+ return sb.toString();
+ }
+
+
+ private static class HeaderWriteTest {
+ private final Http2UpgradeHandler handler;
+ private final Stream stream;
+ private RecordingHeaderFrameBuffers buffers;
+ private volatile Http2Exception closeException;
+
+ HeaderWriteTest(boolean async, int payloadLimit) throws
ConnectionException {
+ SocketWrapperBase<?> socket =
EasyMock.createNiceMock(SocketWrapperBase.class);
+ EasyMock.replay(socket);
+
+ Http2Protocol protocol = new Http2Protocol();
+ if (async) {
+ handler = new Http2AsyncUpgradeHandler(protocol, null, null) {
+ @Override
+ protected HeaderFrameBuffers getHeaderFrameBuffers(int
initialPayloadSize) {
+ buffers = new
RecordingHeaderFrameBuffers(super.getHeaderFrameBuffers(initialPayloadSize),
+ getRemoteSettings().getMaxFrameSize());
+ return buffers;
+ }
+
+ @Override
+ void closeConnection(Http2Exception ce) {
+ closeException = ce;
+ }
+ };
+ } else {
+ handler = new Http2UpgradeHandler(protocol, null, null) {
+ @Override
+ protected HeaderFrameBuffers getHeaderFrameBuffers(int
initialPayloadSize) {
+ buffers = new
RecordingHeaderFrameBuffers(super.getHeaderFrameBuffers(initialPayloadSize),
+ getRemoteSettings().getMaxFrameSize());
+ return buffers;
+ }
+
+ @Override
+ void closeConnection(Http2Exception ce) {
+ closeException = ce;
+ }
+ };
+ }
+ handler.getRemoteSettings().set(Setting.MAX_FRAME_SIZE,
payloadLimit);
+ handler.setSocketWrapper(socket);
+ stream = new Stream(Integer.valueOf(3), handler);
+ stream.state.receivedStartOfHeaders();
+ }
+
+ void write(String value) throws IOException {
+ MimeHeaders headers = new MimeHeaders();
+ headers.addValue("x").setString(value);
+ write(headers);
+ }
+
+ void assertConnectionClosedWithInternalError() {
+ Assert.assertNotNull("The connection was not closed",
closeException);
+ Assert.assertEquals(Http2Error.INTERNAL_ERROR,
closeException.getError());
+ }
+
+ void write(MimeHeaders headers) throws IOException {
+ handler.doWriteHeaders(stream, 0, headers, false,
Constants.DEFAULT_HEADERS_FRAME_SIZE);
+ }
+ }
+
+
+ private static class OversizedHeaderServlet extends HttpServlet {
+
+ private static final long serialVersionUID = 1L;
+
+ @Override
+ protected void doGet(HttpServletRequest req, HttpServletResponse resp)
throws ServletException, IOException {
+ // Does not compress to fit in a frame of the default maximum size
(16384 bytes)
+ resp.setHeader("x-oversized", repeat("X", 3 *
ConnectionSettingsBase.MIN_MAX_FRAME_SIZE));
+ resp.setContentType("text/plain");
+ resp.getWriter().print("OK");
+ }
+ }
+
+
+ private static class RecordingHeaderFrameBuffers implements
HeaderFrameBuffers {
+ private final HeaderFrameBuffers delegate;
+ private final int payloadLimit;
+ private final List<ByteBuffer> payloads = new ArrayList<>();
+ private int maxWorkspaceSize;
+ private int maxPayloadSize;
+ private boolean endOfHeadersWritten;
+
+ RecordingHeaderFrameBuffers(HeaderFrameBuffers delegate, int
payloadLimit) {
+ this.delegate = delegate;
+ this.payloadLimit = payloadLimit;
+ }
+
+ @Override
+ public void startFrame() {
+ delegate.startFrame();
+ }
+
+ @Override
+ public void endFrame() throws IOException {
+ ByteBuffer payload = getPayload();
+ Assert.assertTrue("Oversized payload passed to the writer",
payload.remaining() <= payloadLimit);
+ Assert.assertEquals(payload.remaining(),
ByteUtil.getThreeBytes(getHeader(), 0));
+ maxPayloadSize = Math.max(maxPayloadSize, payload.remaining());
+ endOfHeadersWritten |= (getHeader()[4] &
Http2UpgradeHandler.FLAG_END_OF_HEADERS) != 0;
+ payloads.add(payload.asReadOnlyBuffer());
+ delegate.endFrame();
+ }
+
+ @Override
+ public void endHeaders() throws IOException {
+ delegate.endHeaders();
+ }
+
+ @Override
+ public byte[] getHeader() {
+ return delegate.getHeader();
+ }
+
+ @Override
+ public ByteBuffer getPayload() {
+ ByteBuffer payload = delegate.getPayload();
+ maxWorkspaceSize = Math.max(maxWorkspaceSize, payload.capacity());
+ return payload;
+ }
+
+ @Override
+ public void expandPayload() {
+ delegate.expandPayload();
+ }
+ }
+
+
// https://bz.apache.org/bugzilla/show_bug.cgi?id=60970
@Test
public void testLargeHeader() throws Exception {
diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
index 8fee968a41..6f9fb937d6 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -472,6 +472,15 @@
subsequent <code>WebConnection.close()</code> closes the connection.
(markt)
</fix>
+ <fix>
+ Fix rejection of large HTTP/2 response headers that fit within a frame
+ after HPACK compression. Allow the encoding workspace to grow to the
+ larger of twice the peer's advertised maximum frame payload size and
+ the peer's dynamic table capacity, so indexed fields remain usable
after
+ a frame size reduction. Check the encoded payload against the peer's
+ frame size limit before writing it and close the connection with an
+ <code>INTERNAL_ERROR</code> if a header block can not be written.
(markt)
+ </fix>
</changelog>
</subsection>
<subsection name="Jasper">
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
