This is an automated email from the ASF dual-hosted git repository.
markt-asf pushed a commit to branch 11.0.x
in repository https://gitbox.apache.org/repos/asf/tomcat.git
The following commit(s) were added to refs/heads/11.0.x by this push:
new 2765ee697c Fix a regression in HALF_CLOSED_REMOTE unexpected frame
handling fix
2765ee697c is described below
commit 2765ee697ce16e7aef15939ecbac01680f047a63
Author: Mark Thomas <[email protected]>
AuthorDate: Tue Sep 1 12:12:10 2026 +0100
Fix a regression in HALF_CLOSED_REMOTE unexpected frame handling fix
Switching to StreamException for an unexpected Headers frame introduced
an HPACK desynchronization issue as the connection was no longer closed
but the HEADERS frame remain unprocessed.
---
java/org/apache/coyote/http2/HeaderSink.java | 22 +++++-
java/org/apache/coyote/http2/HpackDecoder.java | 8 +--
java/org/apache/coyote/http2/Http2Parser.java | 15 ++--
test/org/apache/coyote/http2/TestHttp2Limits.java | 22 +-----
.../apache/coyote/http2/TestHttp2Section_5_1.java | 82 +++++++++++++++++++++-
5 files changed, 112 insertions(+), 37 deletions(-)
diff --git a/java/org/apache/coyote/http2/HeaderSink.java
b/java/org/apache/coyote/http2/HeaderSink.java
index 791fe57d3f..ef67c4e914 100644
--- a/java/org/apache/coyote/http2/HeaderSink.java
+++ b/java/org/apache/coyote/http2/HeaderSink.java
@@ -19,11 +19,25 @@ package org.apache.coyote.http2;
import org.apache.coyote.http2.HpackDecoder.HeaderEmitter;
/**
- * Purpose of this class is to silently swallow any headers. It is used once
the connection close process has started if
- * headers for new streams are received.
+ * The purpose of this class is to swallow headers.
+ * <p>
+ * Reporting of stream level errors needs to be delayed until after the
headers have been fully read so that the
+ * server's HPACK decoder remains synchronized with the client's encoder. This
class can be used to ignore such errors
+ * completely (e.g. when new HEADERS are received after the connection close
process has started) or it can be used to
+ * report an error once processing completes (e.g. when HEADERS are received
in the half-closed (remote) state).
*/
class HeaderSink implements HeaderEmitter {
+ private final StreamException se;
+
+ HeaderSink() {
+ this(null);
+ }
+
+ HeaderSink(StreamException se) {
+ this.se = se;
+ }
+
@Override
public void emitHeader(String name, String value) {
// NO-OP
@@ -31,7 +45,9 @@ class HeaderSink implements HeaderEmitter {
@Override
public void validateHeaders() throws StreamException {
- // NO-OP
+ if (se != null) {
+ throw new StreamException(se.getMessage(), se.getError(),
se.getStreamId(), se);
+ }
}
@Override
diff --git a/java/org/apache/coyote/http2/HpackDecoder.java
b/java/org/apache/coyote/http2/HpackDecoder.java
index 066769dc17..ae14e63169 100644
--- a/java/org/apache/coyote/http2/HpackDecoder.java
+++ b/java/org/apache/coyote/http2/HpackDecoder.java
@@ -397,10 +397,10 @@ public class HpackDecoder {
void setHeaderException(StreamException streamException);
/**
- * Are the headers pass to the recipient so far valid? The decoder
needs to process all the headers to maintain
- * state even if there is a problem. In addition, it is easy for the
intended recipient to track if the complete
- * set of headers is valid since to do that state needs to be
maintained between the parsing of the initial
- * headers and the parsing of any trailer headers. The recipient is
the best place to maintain that state.
+ * Are the headers passed to the recipient valid? The decoder needs to
process all the headers to maintain state
+ * even if there is a problem. In addition, it is easy for the
intended recipient to track if the complete set
+ * of headers is valid since to do that state needs to be maintained
between the parsing of the initial headers
+ * and the parsing of any trailer headers. The recipient is the best
place to maintain that state.
*
* @throws StreamException If the headers received to date are not
valid
*/
diff --git a/java/org/apache/coyote/http2/Http2Parser.java
b/java/org/apache/coyote/http2/Http2Parser.java
index 05c631da96..653de3ee42 100644
--- a/java/org/apache/coyote/http2/Http2Parser.java
+++ b/java/org/apache/coyote/http2/Http2Parser.java
@@ -241,8 +241,7 @@ class Http2Parser {
try {
hpackDecoder.setHeaderEmitter(output.headersStart(streamId,
headersEndStream));
} catch (StreamException se) {
- swallowPayload(streamId, FrameType.HEADERS.getId(), payloadSize,
false, buffer);
- throw se;
+ hpackDecoder.setHeaderEmitter(new HeaderSink(se));
}
int padLength = 0;
@@ -294,10 +293,9 @@ class Http2Parser {
swallowPayload(streamId, FrameType.HEADERS.getId(), padLength, true,
buffer);
- // Validate the headers so far
- hpackDecoder.getHeaderEmitter().validateHeaders();
-
if (Flags.isEndOfHeaders(flags)) {
+ // Validate the headers once complete
+ hpackDecoder.getHeaderEmitter().validateHeaders();
onHeadersComplete(streamId);
} else {
headersCurrentStream = streamId;
@@ -463,11 +461,12 @@ class Http2Parser {
readHeaderPayload(streamId, payloadSize, buffer);
- // Validate the headers so far
- hpackDecoder.getHeaderEmitter().validateHeaders();
-
if (endOfHeaders) {
headersCurrentStream = -1;
+
+ // Validate the headers once complete
+ hpackDecoder.getHeaderEmitter().validateHeaders();
+
onHeadersComplete(streamId);
}
}
diff --git a/test/org/apache/coyote/http2/TestHttp2Limits.java
b/test/org/apache/coyote/http2/TestHttp2Limits.java
index 697cbe813f..7fb3581cb9 100644
--- a/test/org/apache/coyote/http2/TestHttp2Limits.java
+++ b/test/org/apache/coyote/http2/TestHttp2Limits.java
@@ -166,7 +166,7 @@ public class TestHttp2Limits extends Http2TestBase {
// 500ms per frame write delay to give server a chance to process the
// stream reset and the connection reset before the request is fully
// sent.
- doTestHeaderLimits(1, 32 * 1024, 1024, 500,
FailureMode.STREAM_RESET_THEN_CONNECTION_RESET);
+ doTestHeaderLimits(1, 32 * 1024, 1024, 500,
FailureMode.CONNECTION_RESET);
}
@@ -280,21 +280,6 @@ public class TestHttp2Limits extends Http2TestBase {
Assert.assertNull(e);
break;
}
- case STREAM_RESET_THEN_CONNECTION_RESET: {
- // Expect a stream reset
- // On some platform / Connector combinations the TCP
connection close
- // will be processed before the client gets a chance to read
the
- // connection close frame which will trigger an
- // IOException when we try to read the frame.
- try {
- parser.readFrame();
- Assert.assertEquals("3-RST-[11]\n", output.getTrace());
- output.clearTrace();
- } catch (IOException ioe) {
- // Expected on some platforms
- }
- }
- //$FALL-THROUGH$
case CONNECTION_RESET: {
// This message uses i18n and needs to be used in a regular
// expression (since we don't know the connection ID).
Generate the
@@ -539,10 +524,6 @@ public class TestHttp2Limits extends Http2TestBase {
Assert.assertEquals("3-RST-[11]\n", output.getTrace());
break;
}
- case STREAM_RESET_THEN_CONNECTION_RESET: {
- Assert.fail("Not used");
- break;
- }
case CONNECTION_RESET: {
// NIO2 can sometimes send window updates depending timing
skipWindowSizeFrames();
@@ -565,7 +546,6 @@ public class TestHttp2Limits extends Http2TestBase {
NONE,
STREAM_RESET,
CONNECTION_RESET,
- STREAM_RESET_THEN_CONNECTION_RESET,
}
diff --git a/test/org/apache/coyote/http2/TestHttp2Section_5_1.java
b/test/org/apache/coyote/http2/TestHttp2Section_5_1.java
index 740f98b48e..3e2d5ce5d9 100644
--- a/test/org/apache/coyote/http2/TestHttp2Section_5_1.java
+++ b/test/org/apache/coyote/http2/TestHttp2Section_5_1.java
@@ -17,6 +17,8 @@
package org.apache.coyote.http2;
import java.nio.ByteBuffer;
+import java.util.ArrayList;
+import java.util.List;
import java.util.logging.Level;
import java.util.logging.Logger;
@@ -57,7 +59,7 @@ public class TestHttp2Section_5_1 extends Http2TestBase {
@Test
- public void halfClosedRemoteInvalidFrame() throws Exception {
+ public void testHalfClosedRemoteInvalidFrame() throws Exception {
http2Connect();
// This half-closes the stream since it includes the end of stream flag
@@ -73,6 +75,84 @@ public class TestHttp2Section_5_1 extends Http2TestBase {
}
+ @Test
+ public void testHalfClosedRemoteHeadersFrameMaintainsHpackState() throws
Exception {
+ doTestHalfClosedRemoteHeadersMaintainsHpackState(false);
+ }
+
+
+ @Test
+ public void testHalfClosedRemoteContinuationFrameMaintainsHpackState()
throws Exception {
+ doTestHalfClosedRemoteHeadersMaintainsHpackState(true);
+ }
+
+
+ private void doTestHalfClosedRemoteHeadersMaintainsHpackState(boolean
useContinuation) throws Exception {
+ http2Connect();
+
+ // This test may trigger overhead protection so disable it.
+ http2Protocol.setOverheadResetFactor(0);
+
+ // Prevent the response body from completing so stream 3 remains
half-closed (remote).
+ sendSettings(0, false, new SettingValue(4, 0));
+ parser.readFrame();
+ output.clearTrace();
+
+ // Send a request on stream 3
+ sendSimpleGetRequest(3);
+
+ /*
+ * Send another (invalid) request on stream 3 (will cause HEADERS to
be received in the "half-closed (remote)"
+ * state). This request create a dynamic table entry for the
'x-hpack-test' header.
+ */
+ String headerName = "x-hpack-test";
+ String headerValue = "value";
+ byte[] frameHeader = new byte[9];
+ ByteBuffer headersPayload = ByteBuffer.allocate(128);
+
+ if (useContinuation) {
+ buildSimpleGetRequestPart1(frameHeader, headersPayload, 3);
+ writeFrame(frameHeader, headersPayload);
+
+ frameHeader = new byte[9];
+ headersPayload = ByteBuffer.allocate(128);
+ List<Header> continuationHeaders = new ArrayList<>(1);
+ continuationHeaders.add(new Header(headerName, headerValue));
+ buildSimpleGetRequestPart2(frameHeader, headersPayload,
continuationHeaders, 3);
+ writeFrame(frameHeader, headersPayload);
+ } else {
+ List<Header> invalidHeaders = new ArrayList<>(1);
+ invalidHeaders.add(new Header(headerName, headerValue));
+ buildGetRequest(frameHeader, headersPayload, null, invalidHeaders,
3);
+ writeFrame(frameHeader, headersPayload);
+ }
+
+ /*
+ * Create a valid request on Stream 5. This request uses the dynamic
table entry for the 'x-hpack-test' header
+ * created by the invalid field block above.
+ */
+ frameHeader = new byte[9];
+ headersPayload = ByteBuffer.allocate(128);
+ List<Header> validHeaders = new ArrayList<>(5);
+ validHeaders.add(new Header(":method", Method.GET));
+ validHeaders.add(new Header(":scheme", "http"));
+ validHeaders.add(new Header(":path", "/empty"));
+ validHeaders.add(new Header(":authority", "localhost:" + getPort()));
+ validHeaders.add(new Header(headerName, headerValue));
+ buildGetRequest(frameHeader, headersPayload, null, validHeaders, 5);
+ writeFrame(frameHeader, headersPayload);
+
+ while (!output.getTrace().contains("5-EndOfStream") &&
!output.getTrace().contains("5-RST-")) {
+ System.out.println(output.getTrace());
+ parser.readFrame();
+ }
+
+ String trace = output.getTrace();
+ Assert.assertTrue(trace, trace.contains("3-RST-[" +
Http2Error.STREAM_CLOSED.getCode() + "]"));
+ Assert.assertTrue(trace, trace.contains(getEmptyResponseTrace(5)));
+ }
+
+
@Test
public void testClosedInvalidFrame01() throws Exception {
// HTTP2 upgrade
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
