You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by ma...@apache.org on 2017/07/17 19:33:12 UTC
svn commit: r1802195 - in /tomcat/trunk: java/org/apache/coyote/http2/
test/org/apache/coyote/http2/ webapps/docs/
Author: markt
Date: Mon Jul 17 19:33:11 2017
New Revision: 1802195
URL: http://svn.apache.org/viewvc?rev=1802195&view=rev
Log:
Improve the handling of HTTP/2 stream resets due to excessive headers when a continuation frame is used.
Modified:
tomcat/trunk/java/org/apache/coyote/http2/HeaderSink.java
tomcat/trunk/java/org/apache/coyote/http2/HpackDecoder.java
tomcat/trunk/java/org/apache/coyote/http2/Http2Parser.java
tomcat/trunk/java/org/apache/coyote/http2/Stream.java
tomcat/trunk/test/org/apache/coyote/http2/Http2TestBase.java
tomcat/trunk/test/org/apache/coyote/http2/TestHpack.java
tomcat/trunk/test/org/apache/coyote/http2/TestHttp2Limits.java
tomcat/trunk/webapps/docs/changelog.xml
Modified: tomcat/trunk/java/org/apache/coyote/http2/HeaderSink.java
URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/coyote/http2/HeaderSink.java?rev=1802195&r1=1802194&r2=1802195&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/coyote/http2/HeaderSink.java (original)
+++ tomcat/trunk/java/org/apache/coyote/http2/HeaderSink.java Mon Jul 17 19:33:11 2017
@@ -34,4 +34,11 @@ class HeaderSink implements HeaderEmitte
public void validateHeaders() throws StreamException {
// NO-OP
}
+
+ @Override
+ public void setHeaderException(StreamException streamException) {
+ // NO-OP
+ // The connection is already closing so no need to process additional
+ // errors
+ }
}
Modified: tomcat/trunk/java/org/apache/coyote/http2/HpackDecoder.java
URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/coyote/http2/HpackDecoder.java?rev=1802195&r1=1802194&r2=1802195&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/coyote/http2/HpackDecoder.java (original)
+++ tomcat/trunk/java/org/apache/coyote/http2/HpackDecoder.java Mon Jul 17 19:33:11 2017
@@ -356,6 +356,16 @@ public class HpackDecoder {
void emitHeader(String name, String value) throws HpackException;
/**
+ * Inform the recipient of the headers that a stream error needs to be
+ * triggered using the given message when {@link #validateHeaders()} is
+ * called. This is used when the Parser becomes aware of an error that
+ * is not visible to the recipient.
+ *
+ * @param streamException The exception to use when resetting the stream
+ */
+ 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 the intended recipient to
Modified: tomcat/trunk/java/org/apache/coyote/http2/Http2Parser.java
URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/coyote/http2/Http2Parser.java?rev=1802195&r1=1802194&r2=1802195&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/coyote/http2/Http2Parser.java (original)
+++ tomcat/trunk/java/org/apache/coyote/http2/Http2Parser.java Mon Jul 17 19:33:11 2017
@@ -45,7 +45,6 @@ class Http2Parser {
ByteBuffer.allocate(Constants.DEFAULT_HEADER_READ_BUFFER_SIZE);
private volatile int headersCurrentStream = -1;
private volatile boolean headersEndStream = false;
- private volatile boolean streamReset = false;
Http2Parser(String connectionId, Input input, Output output) {
this.connectionId = connectionId;
@@ -379,8 +378,8 @@ class Http2Parser {
readHeaderPayload(streamId, payloadSize);
if (Flags.isEndOfHeaders(flags)) {
- onHeadersComplete(streamId);
headersCurrentStream = -1;
+ onHeadersComplete(streamId);
}
}
@@ -427,16 +426,18 @@ class Http2Parser {
headerReadBuffer.compact();
remaining -= toRead;
- if (hpackDecoder.isHeaderCountExceeded() && !streamReset) {
- streamReset = true;
- throw new StreamException(sm.getString("http2Parser.headerLimitCount", connectionId,
- Integer.valueOf(streamId)), Http2Error.ENHANCE_YOUR_CALM, streamId);
+ if (hpackDecoder.isHeaderCountExceeded()) {
+ StreamException headerException = new StreamException(sm.getString(
+ "http2Parser.headerLimitCount", connectionId, Integer.valueOf(streamId)),
+ Http2Error.ENHANCE_YOUR_CALM, streamId);
+ hpackDecoder.getHeaderEmitter().setHeaderException(headerException);
}
- if (hpackDecoder.isHeaderSizeExceeded(headerReadBuffer.position()) && !streamReset) {
- streamReset = true;
- throw new StreamException(sm.getString("http2Parser.headerLimitSize", connectionId,
- Integer.valueOf(streamId)), Http2Error.ENHANCE_YOUR_CALM, streamId);
+ if (hpackDecoder.isHeaderSizeExceeded(headerReadBuffer.position())) {
+ StreamException headerException = new StreamException(sm.getString(
+ "http2Parser.headerLimitSize", connectionId, Integer.valueOf(streamId)),
+ Http2Error.ENHANCE_YOUR_CALM, streamId);
+ hpackDecoder.getHeaderEmitter().setHeaderException(headerException);
}
if (hpackDecoder.isHeaderSwallowSizeExceeded(headerReadBuffer.position())) {
@@ -444,8 +445,6 @@ class Http2Parser {
connectionId, Integer.valueOf(streamId)), Http2Error.ENHANCE_YOUR_CALM);
}
}
-
- hpackDecoder.getHeaderEmitter().validateHeaders();
}
@@ -457,6 +456,11 @@ class Http2Parser {
Http2Error.COMPRESSION_ERROR);
}
+ // Delay validation (and triggering any exception) until this point
+ // since all the headers still have to be read if a StreamException is
+ // going to be thrown.
+ hpackDecoder.getHeaderEmitter().validateHeaders();
+
output.headersEnd(streamId);
if (headersEndStream) {
@@ -468,11 +472,6 @@ class Http2Parser {
if (headerReadBuffer.capacity() > Constants.DEFAULT_HEADER_READ_BUFFER_SIZE) {
headerReadBuffer = ByteBuffer.allocate(Constants.DEFAULT_HEADER_READ_BUFFER_SIZE);
}
-
- // Clear the 'stream has been reset' flag, if set
- if (streamReset) {
- streamReset = false;
- }
}
Modified: tomcat/trunk/java/org/apache/coyote/http2/Stream.java
URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/coyote/http2/Stream.java?rev=1802195&r1=1802194&r2=1802195&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/coyote/http2/Stream.java (original)
+++ tomcat/trunk/java/org/apache/coyote/http2/Stream.java Mon Jul 17 19:33:11 2017
@@ -70,7 +70,7 @@ class Stream extends AbstractStream impl
private final StreamStateMachine state;
// State machine would be too much overhead
private int headerState = HEADER_STATE_START;
- private String headerStateErrorMsg = null;
+ private StreamException headerException = null;
// TODO: null these when finished to reduce memory used by closed stream
private final Request coyoteRequest;
private StringBuilder cookieHeader = null;
@@ -256,7 +256,7 @@ class Stream extends AbstractStream impl
}
}
- if (headerStateErrorMsg != null) {
+ if (headerException != null) {
// Don't bother processing the header since the stream is going to
// be reset anyway
return;
@@ -265,8 +265,9 @@ class Stream extends AbstractStream impl
boolean pseudoHeader = name.charAt(0) == ':';
if (pseudoHeader && headerState != HEADER_STATE_PSEUDO) {
- headerStateErrorMsg = sm.getString("stream.header.unexpectedPseudoHeader",
- getConnectionId(), getIdentifier(), name);
+ headerException = new StreamException(sm.getString(
+ "stream.header.unexpectedPseudoHeader", getConnectionId(), getIdentifier(),
+ name), Http2Error.PROTOCOL_ERROR, getIdentifier().intValue());
// No need for further processing. The stream will be reset.
return;
}
@@ -352,8 +353,9 @@ class Stream extends AbstractStream impl
coyoteRequest.setExpectation(true);
}
if (pseudoHeader) {
- headerStateErrorMsg = sm.getString("stream.header.unknownPseudoHeader",
- getConnectionId(), getIdentifier(), name);
+ headerException = new StreamException(sm.getString(
+ "stream.header.unknownPseudoHeader", getConnectionId(), getIdentifier(),
+ name), Http2Error.PROTOCOL_ERROR, getIdentifier().intValue());
}
if (headerState == HEADER_STATE_TRAILER) {
@@ -368,13 +370,20 @@ class Stream extends AbstractStream impl
@Override
+ public void setHeaderException(StreamException streamException) {
+ if (headerException == null) {
+ headerException = streamException;
+ }
+ }
+
+
+ @Override
public void validateHeaders() throws StreamException {
- if (headerStateErrorMsg == null) {
+ if (headerException == null) {
return;
}
- throw new StreamException(headerStateErrorMsg, Http2Error.PROTOCOL_ERROR,
- getIdentifier().intValue());
+ throw headerException;
}
Modified: tomcat/trunk/test/org/apache/coyote/http2/Http2TestBase.java
URL: http://svn.apache.org/viewvc/tomcat/trunk/test/org/apache/coyote/http2/Http2TestBase.java?rev=1802195&r1=1802194&r2=1802195&view=diff
==============================================================================
--- tomcat/trunk/test/org/apache/coyote/http2/Http2TestBase.java (original)
+++ tomcat/trunk/test/org/apache/coyote/http2/Http2TestBase.java Mon Jul 17 19:33:11 2017
@@ -937,6 +937,12 @@ public abstract class Http2TestBase exte
@Override
+ public void setHeaderException(StreamException streamException) {
+ // NO-OP: Accept anything the server sends for the unit tests
+ }
+
+
+ @Override
public void headersEnd(int streamId) {
trace.append(streamId + "-HeadersEnd\n");
}
Modified: tomcat/trunk/test/org/apache/coyote/http2/TestHpack.java
URL: http://svn.apache.org/viewvc/tomcat/trunk/test/org/apache/coyote/http2/TestHpack.java?rev=1802195&r1=1802194&r2=1802195&view=diff
==============================================================================
--- tomcat/trunk/test/org/apache/coyote/http2/TestHpack.java (original)
+++ tomcat/trunk/test/org/apache/coyote/http2/TestHpack.java Mon Jul 17 19:33:11 2017
@@ -80,6 +80,10 @@ public class TestHpack {
headers.setValue(name).setString(value);
}
@Override
+ public void setHeaderException(StreamException streamException) {
+ // NO-OP
+ }
+ @Override
public void validateHeaders() throws StreamException {
// NO-OP
}
Modified: tomcat/trunk/test/org/apache/coyote/http2/TestHttp2Limits.java
URL: http://svn.apache.org/viewvc/tomcat/trunk/test/org/apache/coyote/http2/TestHttp2Limits.java?rev=1802195&r1=1802194&r2=1802195&view=diff
==============================================================================
--- tomcat/trunk/test/org/apache/coyote/http2/TestHttp2Limits.java (original)
+++ tomcat/trunk/test/org/apache/coyote/http2/TestHttp2Limits.java Mon Jul 17 19:33:11 2017
@@ -22,6 +22,9 @@ import java.util.ArrayList;
import java.util.List;
import java.util.Random;
+import org.hamcrest.Description;
+import org.hamcrest.TypeSafeMatcher;
+
import org.junit.Assert;
import org.junit.Test;
@@ -34,7 +37,7 @@ public class TestHttp2Limits extends Htt
@Test
public void testHeaderLimits1x128() throws Exception {
// Well within limits
- doTestHeaderLimits(1, 128, 0);
+ doTestHeaderLimits(1, 128, FailureMode.NONE);
}
@@ -42,21 +45,22 @@ public class TestHttp2Limits extends Htt
public void testHeaderLimits100x32() throws Exception {
// Just within default maxHeaderCount
// Note request has 4 standard headers
- doTestHeaderLimits(96, 32, 0);
+ doTestHeaderLimits(96, 32, FailureMode.NONE);
}
@Test
public void testHeaderLimits101x32() throws Exception {
// Just above default maxHeaderCount
- doTestHeaderLimits(97, 32, 1);
+ doTestHeaderLimits(97, 32, FailureMode.STREAM_RESET);
}
@Test
public void testHeaderLimits20x32WithLimit10() throws Exception {
// Check lower count limit is enforced
- doTestHeaderLimits(20, 32, -1, 10, Constants.DEFAULT_MAX_HEADER_SIZE, 0, 1);
+ doTestHeaderLimits(20, 32, -1, 10, Constants.DEFAULT_MAX_HEADER_SIZE, 0,
+ FailureMode.STREAM_RESET);
}
@@ -64,42 +68,44 @@ public class TestHttp2Limits extends Htt
public void testHeaderLimits8x1144() throws Exception {
// Just within default maxHttpHeaderSize
// per header overhead plus standard 3 headers
- doTestHeaderLimits(7, 1144, 0);
+ doTestHeaderLimits(7, 1144, FailureMode.NONE);
}
@Test
public void testHeaderLimits8x1145() throws Exception {
// Just above default maxHttpHeaderSize
- doTestHeaderLimits(7, 1145, 1);
+ doTestHeaderLimits(7, 1145, FailureMode.STREAM_RESET);
}
@Test
public void testHeaderLimits3x1024WithLimit2048() throws Exception {
// Check lower size limit is enforced
- doTestHeaderLimits(3, 1024, -1, Constants.DEFAULT_MAX_HEADER_COUNT, 2 * 1024, 0, 1);
+ doTestHeaderLimits(3, 1024, -1, Constants.DEFAULT_MAX_HEADER_COUNT, 2 * 1024, 0,
+ FailureMode.STREAM_RESET);
}
@Test
public void testHeaderLimits1x12k() throws Exception {
// Bug 60232
- doTestHeaderLimits(1, 12*1024, 1);
+ doTestHeaderLimits(1, 12*1024, FailureMode.STREAM_RESET);
}
@Test
public void testHeaderLimits1x12kin1kChunks() throws Exception {
// Bug 60232
- doTestHeaderLimits(1, 12*1024, 1024, 1);
+ doTestHeaderLimits(1, 12*1024, 1024, FailureMode.STREAM_RESET);
}
@Test
public void testHeaderLimits1x12kin1kChunksThenNewRequest() throws Exception {
// Bug 60232
- doTestHeaderLimits(1, 12*1024, 1024, 1);
+ doTestHeaderLimits(1, 12*1024, 1024, FailureMode.STREAM_RESET);
+
output.clearTrace();
sendSimpleGetRequest(5);
@@ -112,7 +118,7 @@ public class TestHttp2Limits extends Htt
@Test
public void testHeaderLimits1x32k() throws Exception {
// Bug 60232
- doTestHeaderLimits(1, 32*1024, 1);
+ doTestHeaderLimits(1, 32*1024, FailureMode.CONNECTION_RESET);
}
@@ -122,44 +128,45 @@ public class TestHttp2Limits extends Htt
// 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, 2);
+ doTestHeaderLimits(1, 32*1024, 1024, 500, FailureMode.CONNECTION_RESET);
}
@Test
public void testHeaderLimits1x128k() throws Exception {
// Bug 60232
- doTestHeaderLimits(1, 128*1024, 2);
+ doTestHeaderLimits(1, 128*1024, FailureMode.CONNECTION_RESET);
}
@Test
public void testHeaderLimits1x512k() throws Exception {
// Bug 60232
- doTestHeaderLimits(1, 512*1024, 2);
+ doTestHeaderLimits(1, 512*1024, FailureMode.CONNECTION_RESET);
}
@Test
public void testHeaderLimits10x512k() throws Exception {
// Bug 60232
- doTestHeaderLimits(10, 512*1024, 2);
+ doTestHeaderLimits(10, 512*1024, FailureMode.CONNECTION_RESET);
}
- private void doTestHeaderLimits(int headerCount, int headerSize, int failMode) throws Exception {
+ private void doTestHeaderLimits(int headerCount, int headerSize, FailureMode failMode)
+ throws Exception {
doTestHeaderLimits(headerCount, headerSize, -1, failMode);
}
private void doTestHeaderLimits(int headerCount, int headerSize, int maxHeaderPayloadSize,
- int failMode) throws Exception {
+ FailureMode failMode) throws Exception {
doTestHeaderLimits(headerCount, headerSize, maxHeaderPayloadSize, 0, failMode);
}
private void doTestHeaderLimits(int headerCount, int headerSize, int maxHeaderPayloadSize,
- int delayms, int failMode) throws Exception {
+ int delayms, FailureMode failMode) throws Exception {
doTestHeaderLimits(headerCount, headerSize, maxHeaderPayloadSize,
Constants.DEFAULT_MAX_HEADER_COUNT, Constants.DEFAULT_MAX_HEADER_SIZE, delayms,
failMode);
@@ -167,7 +174,8 @@ public class TestHttp2Limits extends Htt
private void doTestHeaderLimits(int headerCount, int headerSize, int maxHeaderPayloadSize,
- int maxHeaderCount, int maxHeaderSize, int delayms, int failMode) throws Exception {
+ int maxHeaderCount, int maxHeaderSize, int delayms, FailureMode failMode)
+ throws Exception {
// Build the custom headers
List<String[]> customHeaders = new ArrayList<>();
@@ -226,35 +234,28 @@ public class TestHttp2Limits extends Htt
}
switch (failMode) {
- case 0: {
+ case NONE: {
// Expect a normal response
readSimpleGetResponse();
Assert.assertEquals(getSimpleResponseTrace(3), output.getTrace());
Assert.assertNull(e);
break;
}
- case 1: {
+ case STREAM_RESET: {
// Expect a stream reset
parser.readFrame(true);
Assert.assertEquals("3-RST-[11]\n", output.getTrace());
Assert.assertNull(e);
break;
}
- case 2: {
- // Behaviour depends on timing. If reset is processed fast enough,
- // frames will be swallowed before the connection reset limit is
- // reached
- if (e == null) {
- parser.readFrame(true);
- Assert.assertEquals("3-RST-[11]\n", output.getTrace());
- Assert.assertNull(e);
- }
- // Else is non-null as expected for a connection reset
+ case CONNECTION_RESET: {
+ // Connection reset. Connection ID will vary so use a pattern
+ parser.readFrame(true);
+ Assert.assertThat(output.getTrace(), RegexMatcher.matchesRegex(
+ "0-Goaway-\\[1\\]-\\[11\\]-\\[Connection \\[\\d++\\], Stream \\[3\\], .*"));
+ // e may or may not be null here
break;
}
- default: {
- Assert.fail("Unknown failure mode");
- }
}
}
@@ -400,24 +401,26 @@ public class TestHttp2Limits extends Htt
@Test
public void doTestPostWithTrailerHeadersDefaultLimit() throws Exception{
doTestPostWithTrailerHeaders(Constants.DEFAULT_MAX_TRAILER_COUNT,
- Constants.DEFAULT_MAX_TRAILER_SIZE, true);
+ Constants.DEFAULT_MAX_TRAILER_SIZE, FailureMode.NONE);
}
@Test
public void doTestPostWithTrailerHeadersCount0() throws Exception{
- doTestPostWithTrailerHeaders(0, Constants.DEFAULT_MAX_TRAILER_SIZE, false);
+ doTestPostWithTrailerHeaders(0, Constants.DEFAULT_MAX_TRAILER_SIZE,
+ FailureMode.STREAM_RESET);
}
@Test
public void doTestPostWithTrailerHeadersSize0() throws Exception{
- doTestPostWithTrailerHeaders(Constants.DEFAULT_MAX_TRAILER_COUNT, 0, false);
+ doTestPostWithTrailerHeaders(Constants.DEFAULT_MAX_TRAILER_COUNT, 0,
+ FailureMode.CONNECTION_RESET);
}
- private void doTestPostWithTrailerHeaders(int maxTrailerCount, int maxTrailerSize, boolean ok)
- throws Exception {
+ private void doTestPostWithTrailerHeaders(int maxTrailerCount, int maxTrailerSize,
+ FailureMode failMode) throws Exception {
enableHttp2();
Http2Protocol http2Protocol =
@@ -449,8 +452,9 @@ public class TestHttp2Limits extends Htt
// Trailers
writeFrame(trailerFrameHeader, trailerPayload);
- parser.readFrame(true);
- if (ok) {
+ switch (failMode) {
+ case NONE: {
+ parser.readFrame(true);
parser.readFrame(true);
parser.readFrame(true);
parser.readFrame(true);
@@ -468,8 +472,56 @@ public class TestHttp2Limits extends Htt
"\n" +
"3-EndOfStream\n",
output.getTrace());
- } else {
+ break;
+ }
+ case STREAM_RESET: {
+ parser.readFrame(true);
Assert.assertEquals("3-RST-[11]\n", output.getTrace());
+ break;
+ }
+ case CONNECTION_RESET: {
+ // Connection reset. Connection ID will vary so use a pattern
+ parser.readFrame(true);
+ Assert.assertThat(output.getTrace(), RegexMatcher.matchesRegex(
+ "0-Goaway-\\[3\\]-\\[11\\]-\\[Connection \\[\\d++\\], Stream \\[3\\], .*"));
+ break;
+ }
+ }
+ }
+
+
+ private enum FailureMode {
+ NONE,
+ STREAM_RESET,
+ CONNECTION_RESET,
+
+ }
+
+
+ private static class RegexMatcher extends TypeSafeMatcher<String> {
+
+ private final String pattern;
+
+
+ public RegexMatcher(String pattern) {
+ this.pattern = pattern;
+ }
+
+
+ @Override
+ public void describeTo(Description description) {
+ description.appendText("match to regular expression pattern [" + pattern + "]");
+
+ }
+
+ @Override
+ protected boolean matchesSafely(String item) {
+ return item.matches(pattern);
+ }
+
+
+ public static RegexMatcher matchesRegex(String pattern) {
+ return new RegexMatcher(pattern);
}
}
}
Modified: tomcat/trunk/webapps/docs/changelog.xml
URL: http://svn.apache.org/viewvc/tomcat/trunk/webapps/docs/changelog.xml?rev=1802195&r1=1802194&r2=1802195&view=diff
==============================================================================
--- tomcat/trunk/webapps/docs/changelog.xml (original)
+++ tomcat/trunk/webapps/docs/changelog.xml Mon Jul 17 19:33:11 2017
@@ -85,6 +85,10 @@
where each key has a separate password. Based on a patch by Frank
Taffelt. (markt)
</fix>
+ <fix>
+ Improve the handling of HTTP/2 stream resets due to excessive headers
+ when a continuation frame is used. (markt)
+ </fix>
</changelog>
</subsection>
<subsection name="Jasper">
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org