This is an automated email from the ASF dual-hosted git repository.
markt pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/tomcat.git
commit d47d872b323f360073e8b8aae397ffff833d73de
Author: Mark Thomas <ma...@apache.org>
AuthorDate: Mon Aug 17 18:51:11 2020 +0100
BZ 64621: Improve handling of HTTP/2 stream resets form client
---
.../apache/coyote/http2/Http2UpgradeHandler.java | 35 +++++++----
.../apache/coyote/http2/LocalStrings.properties | 1 +
java/org/apache/coyote/http2/Stream.java | 8 +--
.../apache/coyote/http2/TestHttp2Section_5_1.java | 70 +++++++++++++++++++++-
webapps/docs/changelog.xml | 4 ++
5 files changed, 102 insertions(+), 16 deletions(-)
diff --git a/java/org/apache/coyote/http2/Http2UpgradeHandler.java
b/java/org/apache/coyote/http2/Http2UpgradeHandler.java
index dd3eb8e..5ca89be 100644
--- a/java/org/apache/coyote/http2/Http2UpgradeHandler.java
+++ b/java/org/apache/coyote/http2/Http2UpgradeHandler.java
@@ -898,17 +898,28 @@ class Http2UpgradeHandler extends AbstractStream
implements InternalHttpUpgradeH
tracker = backLogStreams.get(stream);
}
if (tracker != null &&
tracker.getUnusedAllocation() == 0) {
- if (log.isDebugEnabled()) {
-
log.debug(sm.getString("upgradeHandler.noAllocation",
- connectionId,
stream.getIdentifier()));
+ String msg;
+ Http2Error error;
+ if (stream.isActive()) {
+ if (log.isDebugEnabled()) {
+
log.debug(sm.getString("upgradeHandler.noAllocation",
+ connectionId,
stream.getIdentifier()));
+ }
+ // No allocation
+ // Close the connection. Do this first
since
+ // closing the stream will raise an
exception.
+ close();
+ msg = sm.getString("stream.writeTimeout");
+ error = Http2Error.ENHANCE_YOUR_CALM;
+ } else {
+ msg = sm.getString("stream.clientAbort");
+ error = Http2Error.CANCEL;
}
- // No allocation
- // Close the connection. Do this first since
- // closing the stream will raise an exception
- close();
- // Close the stream (in app code so need to
- // signal to app stream is closing)
- stream.doWriteTimeout();
+ // Close the stream
+ // This thread is in application code so need
+ // to signal to the application that the
+ // stream is closing
+ stream.doStreamAbort(msg, error);
}
} catch (InterruptedException e) {
throw new IOException(sm.getString(
@@ -1552,8 +1563,12 @@ class Http2UpgradeHandler extends AbstractStream
implements InternalHttpUpgradeH
@Override
public void reset(int streamId, long errorCode) throws Http2Exception {
Stream stream = getStream(streamId, true);
+ boolean active = stream.isActive();
stream.checkState(FrameType.RST);
stream.receiveReset(errorCode);
+ if (active) {
+ activeRemoteStreamCount.decrementAndGet();
+ }
}
diff --git a/java/org/apache/coyote/http2/LocalStrings.properties
b/java/org/apache/coyote/http2/LocalStrings.properties
index f9ef251..fadfa0f 100644
--- a/java/org/apache/coyote/http2/LocalStrings.properties
+++ b/java/org/apache/coyote/http2/LocalStrings.properties
@@ -74,6 +74,7 @@ http2Parser.swallow.debug=Connection [{0}], Stream [{1}],
Swallowed [{2}] bytes
pingManager.roundTripTime=Connection [{0}] Round trip time measured as [{1}]ns
+stream.clientAbort=Client reset the stream before the response was complete
stream.closed=Connection [{0}], Stream [{1}], Unable to write to stream once
it has been closed
stream.header.case=Connection [{0}], Stream [{1}], HTTP header name [{2}] must
be in lower case
stream.header.connection=Connection [{0}], Stream [{1}], HTTP header
[connection] is not permitted in an HTTP/2 request
diff --git a/java/org/apache/coyote/http2/Stream.java
b/java/org/apache/coyote/http2/Stream.java
index 8b0a501..fbc550a 100644
--- a/java/org/apache/coyote/http2/Stream.java
+++ b/java/org/apache/coyote/http2/Stream.java
@@ -275,7 +275,7 @@ class Stream extends AbstractStream implements
HeaderEmitter {
allocationManager.waitForStream(writeTimeout);
windowSize = getWindowSize();
if (windowSize == 0) {
- doWriteTimeout();
+ doStreamAbort(sm.getString("stream.writeTimeout"),
Http2Error.ENHANCE_YOUR_CALM);
}
} catch (InterruptedException e) {
// Possible shutdown / rst or similar. Use an IOException
to
@@ -299,10 +299,8 @@ class Stream extends AbstractStream implements
HeaderEmitter {
}
- void doWriteTimeout() throws CloseNowException {
- String msg = sm.getString("stream.writeTimeout");
- StreamException se = new StreamException(
- msg, Http2Error.ENHANCE_YOUR_CALM, getIdAsInt());
+ void doStreamAbort(String msg, Http2Error error) throws CloseNowException {
+ StreamException se = new StreamException(msg, error, getIdAsInt());
// Prevent the application making further writes
streamOutputBuffer.closed = true;
// Prevent Tomcat's error handling trying to write
diff --git a/test/org/apache/coyote/http2/TestHttp2Section_5_1.java
b/test/org/apache/coyote/http2/TestHttp2Section_5_1.java
index 0b937a9..163f99a 100644
--- a/test/org/apache/coyote/http2/TestHttp2Section_5_1.java
+++ b/test/org/apache/coyote/http2/TestHttp2Section_5_1.java
@@ -189,7 +189,7 @@ public class TestHttp2Section_5_1 extends Http2TestBase {
@Test
- public void testExceedMaxActiveStreams() throws Exception {
+ public void testExceedMaxActiveStreams01() throws Exception {
// http2Connect() - modified
enableHttp2(1);
configureAndStartWebApplication();
@@ -256,6 +256,74 @@ public class TestHttp2Section_5_1 extends Http2TestBase {
@Test
+ public void testExceedMaxActiveStreams02() throws Exception {
+
+ // http2Connect() - modified
+ enableHttp2(1);
+ configureAndStartWebApplication();
+ openClientConnection();
+ doHttpUpgrade();
+ sendClientPreface();
+
+ // validateHttp2InitialResponse() - modified
+ parser.readFrame(true);
+ parser.readFrame(true);
+ parser.readFrame(true);
+ parser.readFrame(true);
+ parser.readFrame(true);
+
+ Assert.assertEquals("0-Settings-[3]-[1]\n" +
+ "0-Settings-End\n" +
+ "0-Settings-Ack\n" +
+ "0-Ping-[0,0,0,0,0,0,0,1]\n" +
+ getSimpleResponseTrace(1)
+ , output.getTrace());
+ output.clearTrace();
+
+ sendLargeGetRequest(3);
+
+ sendSimpleGetRequest(5);
+
+ // Default connection window size is 64k-1.
+ // Initial request will have used 8k leaving 56k-1.
+ // Stream window will be 64k-1.
+ // Expecting
+ // 1 * headers
+ // 56k-1 of body (7 * ~8k)
+ // 1 * error
+ // for a total of 9 frames (could be in any order)
+ for (int i = 0; i < 9; i++) {
+ parser.readFrame(true);
+ }
+
+ Assert.assertTrue(output.getTrace(),
+ output.getTrace().contains("5-RST-[" +
+ Http2Error.REFUSED_STREAM.getCode() + "]"));
+ output.clearTrace();
+
+ // Connection window is zero.
+ // Stream window is 8k
+
+ // Reset stream 3 (client cancel)
+ sendRst(3, Http2Error.NO_ERROR.getCode());
+ // Client reset triggers a write error which in turn triggers a server
+ // reset
+ parser.readFrame(true);
+ Assert.assertEquals("3-RST-[8]\n", output.getTrace());
+ output.clearTrace();
+
+ // Open up the connection window.
+ sendWindowUpdate(0, (1 << 31) - 2);
+
+ // Confirm another request can be sent once concurrency falls back
below limit
+ sendSimpleGetRequest(7);
+ parser.readFrame(true);
+ parser.readFrame(true);
+ Assert.assertEquals(getSimpleResponseTrace(7), output.getTrace());
+ }
+
+
+ @Test
public void testErrorOnWaitingStream() throws Exception {
// http2Connect() - modified
enableHttp2(1);
diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
index 159c2ef..fea0ba0 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -109,6 +109,10 @@
is closed in one thread at the same time as the poller is processing an
event for that socket in another. (markt)
</fix>
+ <fix>
+ <bug>64621</bug>: Improve handling HTTP/2 stream reset frames received
+ from clients. (markt)
+ </fix>
</changelog>
</subsection>
<subsection name="WebSocket">
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
