This is an automated email from the ASF dual-hosted git repository.
markt pushed a commit to branch 8.5.x
in repository https://gitbox.apache.org/repos/asf/tomcat.git
commit f32ecf47dde6a452559065ad999db7a75eb7ecd4
Author: Mark Thomas <ma...@apache.org>
AuthorDate: Mon Mar 15 17:57:41 2021 +0000
Fix BZ 65179 - avoid flow control window exhaustion
Ensure that the connection level flow control window from the client to
the server is updated when handling DATA frames received for completed
streams else the flow control window may become exhausted
https://bz.apache.org/bugzilla/show_bug.cgi?id=65179
---
.../apache/coyote/http2/AbstractNonZeroStream.java | 16 +++
java/org/apache/coyote/http2/Http2Parser.java | 66 +++++++----
.../apache/coyote/http2/Http2UpgradeHandler.java | 22 ++--
java/org/apache/coyote/http2/RecycledStream.java | 30 ++++-
java/org/apache/coyote/http2/Stream.java | 42 +++++--
test/org/apache/coyote/http2/Http2TestBase.java | 7 +-
test/org/apache/coyote/http2/TestFlowControl.java | 131 +++++++++++++++++++++
webapps/docs/changelog.xml | 6 +
8 files changed, 264 insertions(+), 56 deletions(-)
diff --git a/java/org/apache/coyote/http2/AbstractNonZeroStream.java
b/java/org/apache/coyote/http2/AbstractNonZeroStream.java
index 38761ad..0368c4f 100644
--- a/java/org/apache/coyote/http2/AbstractNonZeroStream.java
+++ b/java/org/apache/coyote/http2/AbstractNonZeroStream.java
@@ -16,6 +16,7 @@
*/
package org.apache.coyote.http2;
+import java.nio.ByteBuffer;
import java.util.Iterator;
import org.apache.juli.logging.Log;
@@ -31,6 +32,8 @@ abstract class AbstractNonZeroStream extends AbstractStream {
private static final Log log =
LogFactory.getLog(AbstractNonZeroStream.class);
private static final StringManager sm =
StringManager.getManager(AbstractNonZeroStream.class);
+ protected static final ByteBuffer ZERO_LENGTH_BYTEBUFFER =
ByteBuffer.allocate(0);
+
protected final StreamStateMachine state;
private volatile int weight = Constants.DEFAULT_WEIGHT;
@@ -140,4 +143,17 @@ abstract class AbstractNonZeroStream extends
AbstractStream {
final void checkState(FrameType frameType) throws Http2Exception {
state.checkFrameType(frameType);
}
+
+
+ /**
+ * Obtain the ByteBuffer to store DATA frame payload data for this stream
+ * that has been received from the client.
+ *
+ * @return {@code null} if the DATA frame payload can be swallowed, or a
+ * ByteBuffer with at least enough space remaining for the current
+ * flow control window for stream data from the client.
+ */
+ abstract ByteBuffer getInputByteBuffer();
+
+ abstract void receivedData(int payloadSize) throws Http2Exception;
}
diff --git a/java/org/apache/coyote/http2/Http2Parser.java
b/java/org/apache/coyote/http2/Http2Parser.java
index 1cf6f77..3f0e1b8 100644
--- a/java/org/apache/coyote/http2/Http2Parser.java
+++ b/java/org/apache/coyote/http2/Http2Parser.java
@@ -170,7 +170,7 @@ class Http2Parser {
swallowPayload(streamId, FrameType.DATA.getId(), dataLength,
false);
// Process padding before sending any notifications in case padding
// is invalid.
- if (padLength > 0) {
+ if (Flags.hasPadding(flags)) {
swallowPayload(streamId, FrameType.DATA.getId(), padLength,
true);
}
if (endOfStream) {
@@ -178,16 +178,19 @@ class Http2Parser {
}
} else {
synchronized (dest) {
- if (dest.remaining() < dataLength) {
- swallowPayload(streamId, FrameType.DATA.getId(),
dataLength, false);
+ if (dest.remaining() < payloadSize) {
// Client has sent more data than permitted by Window size
+ swallowPayload(streamId, FrameType.DATA.getId(),
dataLength, false);
+ if (Flags.hasPadding(flags)) {
+ swallowPayload(streamId, FrameType.DATA.getId(),
padLength, true);
+ }
throw new
StreamException(sm.getString("http2Parser.processFrameData.window",
connectionId),
Http2Error.FLOW_CONTROL_ERROR, streamId);
}
input.fill(true, dest, dataLength);
// Process padding before sending any notifications in case
// padding is invalid.
- if (padLength > 0) {
+ if (Flags.hasPadding(flags)) {
swallowPayload(streamId, FrameType.DATA.getId(),
padLength, true);
}
if (endOfStream) {
@@ -196,9 +199,6 @@ class Http2Parser {
output.endRequestBodyFrame(streamId);
}
}
- if (Flags.hasPadding(flags)) {
- output.onSwallowedDataFramePayload(streamId, padLength);
- }
}
@@ -464,10 +464,11 @@ class Http2Parser {
try {
swallowPayload(streamId, frameTypeId, payloadSize, false);
} catch (ConnectionException e) {
- // Will never happen because swallow() is called with mustBeZero
set
+ // Will never happen because swallowPayload() is called with
isPadding set
// to false
+ } finally {
+ output.onSwallowedUnknownFrame(streamId, frameTypeId, flags,
payloadSize);
}
- output.onSwallowedUnknownFrame(streamId, frameTypeId, flags,
payloadSize);
}
@@ -491,24 +492,39 @@ class Http2Parser {
log.debug(sm.getString("http2Parser.swallow.debug", connectionId,
Integer.toString(streamId), Integer.toString(len)));
}
- if (len == 0) {
- return;
- }
int read = 0;
- byte[] buffer = new byte[1024];
- while (read < len) {
- int thisTime = Math.min(buffer.length, len - read);
- input.fill(true, buffer, 0, thisTime);
- if (isPadding) {
- // Validate the padding is zero since receiving non-zero
padding
- // is a strong indication of either a faulty client or a server
- // side bug.
- for (int i = 0; i < thisTime; i++) {
- if (buffer[i] != 0) {
- throw new
ConnectionException(sm.getString("http2Parser.nonZeroPadding",
- connectionId, Integer.toString(streamId)),
Http2Error.PROTOCOL_ERROR);
+ int thisTime = 0;
+ try {
+ if (len == 0) {
+ return;
+ }
+ byte[] buffer = new byte[1024];
+ while (read < len) {
+ thisTime = Math.min(buffer.length, len - read);
+ input.fill(true, buffer, 0, thisTime);
+ if (isPadding) {
+ // Validate the padding is zero since receiving non-zero
padding
+ // is a strong indication of either a faulty client or a
server
+ // side bug.
+ for (int i = 0; i < thisTime; i++) {
+ if (buffer[i] != 0) {
+ throw new
ConnectionException(sm.getString("http2Parser.nonZeroPadding",
+ connectionId, Integer.toString(streamId)),
Http2Error.PROTOCOL_ERROR);
+ }
}
}
+ read += thisTime;
+ }
+ } finally {
+ if (FrameType.DATA.getIdByte() == frameTypeId) {
+ if (isPadding) {
+ // Need to add 1 for the padding length bytes that was also
+ // part of the payload.
+ len += 1;
+ }
+ if (len > 0) {
+ output.onSwallowedDataFramePayload(streamId, len);
+ }
}
read += thisTime;
}
@@ -654,7 +670,7 @@ class Http2Parser {
// Data frames
ByteBuffer startRequestBodyFrame(int streamId, int payloadSize,
boolean endOfStream) throws Http2Exception;
- void endRequestBodyFrame(int streamId) throws Http2Exception;
+ void endRequestBodyFrame(int streamId) throws Http2Exception,
IOException;
void receivedEndOfStream(int streamId) throws ConnectionException;
/**
* Notification triggered when the parser swallows some or all of a
DATA
diff --git a/java/org/apache/coyote/http2/Http2UpgradeHandler.java
b/java/org/apache/coyote/http2/Http2UpgradeHandler.java
index 3c82718..68fe75e 100644
--- a/java/org/apache/coyote/http2/Http2UpgradeHandler.java
+++ b/java/org/apache/coyote/http2/Http2UpgradeHandler.java
@@ -1574,20 +1574,14 @@ class Http2UpgradeHandler extends AbstractStream
implements InternalHttpUpgradeH
}
AbstractNonZeroStream abstractNonZeroStream =
getAbstractNonZeroStream(streamId, true);
- if (abstractNonZeroStream instanceof Stream) {
- Stream stream = (Stream) abstractNonZeroStream;
- stream.checkState(FrameType.DATA);
- stream.receivedData(payloadSize);
- return stream.getInputByteBuffer();
- } else {
- abstractNonZeroStream.checkState(FrameType.DATA);
- return null;
- }
+ abstractNonZeroStream.checkState(FrameType.DATA);
+ abstractNonZeroStream.receivedData(payloadSize);
+ return abstractNonZeroStream.getInputByteBuffer();
}
@Override
- public void endRequestBodyFrame(int streamId) throws Http2Exception {
+ public void endRequestBodyFrame(int streamId) throws Http2Exception,
IOException {
AbstractNonZeroStream abstractNonZeroStream =
getAbstractNonZeroStream(streamId, true);
if (abstractNonZeroStream instanceof Stream) {
((Stream)
abstractNonZeroStream).getInputBuffer().onDataAvailable();
@@ -1610,11 +1604,9 @@ class Http2UpgradeHandler extends AbstractStream
implements InternalHttpUpgradeH
@Override
- public void onSwallowedDataFramePayload(int streamId, int
swallowedDataBytesCount) throws
- ConnectionException, IOException {
- AbstractNonZeroStream abstractNonZeroStream =
getAbstractNonZeroStream(streamId, true);
- // +1 is for the payload byte used to define the padding length
- writeWindowUpdate(abstractNonZeroStream, swallowedDataBytesCount + 1,
false);
+ public void onSwallowedDataFramePayload(int streamId, int
swallowedDataBytesCount) throws IOException {
+ AbstractNonZeroStream abstractNonZeroStream =
getAbstractNonZeroStream(streamId);
+ writeWindowUpdate(abstractNonZeroStream, swallowedDataBytesCount,
false);
}
diff --git a/java/org/apache/coyote/http2/RecycledStream.java
b/java/org/apache/coyote/http2/RecycledStream.java
index b2b3e32..730936b 100644
--- a/java/org/apache/coyote/http2/RecycledStream.java
+++ b/java/org/apache/coyote/http2/RecycledStream.java
@@ -16,6 +16,8 @@
*/
package org.apache.coyote.http2;
+import java.nio.ByteBuffer;
+
/**
* Represents a closed stream in the priority tree. Used in preference to the
* full {@link Stream} as has much lower memory usage.
@@ -23,10 +25,12 @@ package org.apache.coyote.http2;
class RecycledStream extends AbstractNonZeroStream {
private final String connectionId;
+ private int remainingFlowControlWindow;
- RecycledStream(String connectionId, Integer identifier, StreamStateMachine
state) {
+ RecycledStream(String connectionId, Integer identifier, StreamStateMachine
state, int remainingFlowControlWindow) {
super(identifier, state);
this.connectionId = connectionId;
+ this.remainingFlowControlWindow = remainingFlowControlWindow;
}
@@ -41,5 +45,27 @@ class RecycledStream extends AbstractNonZeroStream {
void incrementWindowSize(int increment) throws Http2Exception {
// NO-OP
}
-}
+
+ @Override
+ void receivedData(int payloadSize) throws ConnectionException {
+ remainingFlowControlWindow -= payloadSize;
+ }
+
+
+ /**
+ * {@inheritDoc}
+ * <p>
+ * This implementation will return an zero length ByteBuffer to trigger a
+ * flow control error if more DATA frame payload than the remaining flow
+ * control window is received for this recycled stream.
+ */
+ @Override
+ ByteBuffer getInputByteBuffer() {
+ if (remainingFlowControlWindow < 0) {
+ return ZERO_LENGTH_BYTEBUFFER;
+ } else {
+ return null;
+ }
+ }
+}
diff --git a/java/org/apache/coyote/http2/Stream.java
b/java/org/apache/coyote/http2/Stream.java
index 788bf27..a0110c0 100644
--- a/java/org/apache/coyote/http2/Stream.java
+++ b/java/org/apache/coyote/http2/Stream.java
@@ -478,9 +478,13 @@ class Stream extends AbstractNonZeroStream implements
HeaderEmitter {
}
+ @Override
final ByteBuffer getInputByteBuffer() {
if (inputBuffer == null) {
- return null;
+ // This must either be a push or an HTTP upgrade. Either way there
+ // should not be a request body so return a zero length ByteBuffer
+ // to trigger a flow control error.
+ return ZERO_LENGTH_BYTEBUFFER;
}
return inputBuffer.getInBuffer();
}
@@ -508,7 +512,8 @@ class Stream extends AbstractNonZeroStream implements
HeaderEmitter {
}
- final void receivedData(int payloadSize) throws ConnectionException {
+ @Override
+ final void receivedData(int payloadSize) throws Http2Exception {
contentLengthReceived += payloadSize;
long contentLengthHeader = coyoteRequest.getContentLengthLong();
if (contentLengthHeader > -1 && contentLengthReceived >
contentLengthHeader) {
@@ -609,6 +614,7 @@ class Stream extends AbstractNonZeroStream implements
HeaderEmitter {
state.sendReset();
cancelAllocationRequests();
handler.sendStreamReset(se);
+ inputBuffer.swallowUnread();
} catch (IOException ioe) {
ConnectionException ce = new ConnectionException(
sm.getString("stream.reset.fail"),
Http2Error.PROTOCOL_ERROR, ioe);
@@ -637,7 +643,8 @@ class Stream extends AbstractNonZeroStream implements
HeaderEmitter {
if (log.isDebugEnabled()) {
log.debug(sm.getString("stream.recycle", getConnectionId(),
getIdAsString()));
}
- handler.replaceStream(this, new RecycledStream(getConnectionId(),
getIdentifier(), state));
+ handler.replaceStream(
+ this, new RecycledStream(getConnectionId(), getIdentifier(),
state, getInputByteBuffer().remaining()));
}
@@ -987,7 +994,8 @@ class Stream extends AbstractNonZeroStream implements
HeaderEmitter {
// 'write mode'.
private volatile ByteBuffer inBuffer;
private volatile boolean readInterest;
- private boolean resetReceived = false;
+ private volatile boolean closed;
+ private boolean resetReceived;
/**
* @deprecated Unused. Will be removed in Tomcat 9. Use
@@ -1175,8 +1183,10 @@ class Stream extends AbstractNonZeroStream implements
HeaderEmitter {
/*
* Called after placing some data in the inBuffer.
*/
- final synchronized boolean onDataAvailable() {
- if (readInterest) {
+ final synchronized void onDataAvailable() throws IOException {
+ if (closed) {
+ swallowUnread();
+ } else if (readInterest) {
if (log.isDebugEnabled()) {
log.debug(sm.getString("stream.inputBuffer.dispatch"));
}
@@ -1186,7 +1196,6 @@ class Stream extends AbstractNonZeroStream implements
HeaderEmitter {
// the incoming connection and streams are processed on their
// own.
coyoteRequest.action(ActionCode.DISPATCH_EXECUTE, null);
- return true;
} else {
if (log.isDebugEnabled()) {
log.debug(sm.getString("stream.inputBuffer.signal"));
@@ -1194,7 +1203,6 @@ class Stream extends AbstractNonZeroStream implements
HeaderEmitter {
synchronized (inBuffer) {
inBuffer.notifyAll();
}
- return false;
}
}
@@ -1211,13 +1219,13 @@ class Stream extends AbstractNonZeroStream implements
HeaderEmitter {
private final void ensureBuffersExist() {
- if (inBuffer == null) {
+ if (inBuffer == null && !closed) {
// The client must obey Tomcat's window size when sending so
// this is the initial window size set by Tomcat that the
client
// uses (i.e. the local setting is required here).
int size = handler.getLocalSettings().getInitialWindowSize();
synchronized (this) {
- if (inBuffer == null) {
+ if (inBuffer == null && !closed) {
inBuffer = ByteBuffer.allocate(size);
outBuffer = new byte[size];
}
@@ -1242,5 +1250,19 @@ class Stream extends AbstractNonZeroStream implements
HeaderEmitter {
}
}
}
+
+ private final void swallowUnread() throws IOException {
+ if (inBuffer != null) {
+ synchronized (inputBuffer) {
+ closed = true;
+ int unreadByteCount = inBuffer.position();
+ if (unreadByteCount > 0) {
+ inBuffer.position(0);
+ inBuffer.limit(inBuffer.limit() - unreadByteCount);
+ handler.onSwallowedDataFramePayload(getIdAsInt(),
unreadByteCount);
+ }
+ }
+ }
+ }
}
}
diff --git a/test/org/apache/coyote/http2/Http2TestBase.java
b/test/org/apache/coyote/http2/Http2TestBase.java
index f459a40..a848c55 100644
--- a/test/org/apache/coyote/http2/Http2TestBase.java
+++ b/test/org/apache/coyote/http2/Http2TestBase.java
@@ -1198,10 +1198,9 @@ public abstract class Http2TestBase extends
TomcatBaseTest {
@Override
public void onSwallowedDataFramePayload(int streamId, int
swallowedDataBytesCount) {
- trace.append(streamId);
- trace.append("-SwallowedDataFramePayload-[");
- trace.append(swallowedDataBytesCount);
- trace.append("]\n");
+ // NO-OP
+ // Many tests swallow request bodies which triggers this
+ // notification. It is added to the trace to reduce noise.
}
diff --git a/test/org/apache/coyote/http2/TestFlowControl.java
b/test/org/apache/coyote/http2/TestFlowControl.java
new file mode 100644
index 0000000..3841a1d
--- /dev/null
+++ b/test/org/apache/coyote/http2/TestFlowControl.java
@@ -0,0 +1,131 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.coyote.http2;
+
+import java.io.IOException;
+import java.nio.ByteBuffer;
+
+import org.junit.Assert;
+import org.junit.Test;
+
+import org.apache.tomcat.util.http.MimeHeaders;
+
+public class TestFlowControl extends Http2TestBase {
+
+ /*
+ * https://tomcat.markmail.org/thread/lijsebphms7hr3zj
+ */
+ @Test
+ public void testNotFound() throws Exception {
+
+ http2Connect();
+
+ // Connection and per-stream flow control default to 64k-1 bytes
+
+ // Set up a POST request to a non-existent end-point
+ // Generate headers
+ byte[] headersFrameHeader = new byte[9];
+ ByteBuffer headersPayload = ByteBuffer.allocate(128);
+
+ MimeHeaders headers = new MimeHeaders();
+ headers.addValue(":method").setString("POST");
+ headers.addValue(":scheme").setString("http");
+ headers.addValue(":path").setString("/path-does-not-exist");
+ headers.addValue(":authority").setString("localhost:" + getPort());
+ headers.addValue("content-length").setLong(65536);
+ hpackEncoder.encode(headers, headersPayload);
+
+ headersPayload.flip();
+
+ ByteUtil.setThreeBytes(headersFrameHeader, 0, headersPayload.limit());
+ headersFrameHeader[3] = FrameType.HEADERS.getIdByte();
+ // Flags. end of headers (0x04)
+ headersFrameHeader[4] = 0x04;
+ // Stream id
+ ByteUtil.set31Bits(headersFrameHeader, 5, 3);
+
+ writeFrame(headersFrameHeader, headersPayload);
+
+ // Generate body
+ // Max data payload is 16k
+ byte[] dataFrameHeader = new byte[9];
+ ByteBuffer dataPayload = ByteBuffer.allocate(16 * 1024);
+
+ while (dataPayload.hasRemaining()) {
+ dataPayload.put((byte) 'x');
+ }
+ dataPayload.flip();
+
+ // Size
+ ByteUtil.setThreeBytes(dataFrameHeader, 0, dataPayload.limit());
+ ByteUtil.set31Bits(dataFrameHeader, 5, 3);
+
+ // Read the 404 error page
+ // headers
+ parser.readFrame(true);
+ // body
+ parser.readFrame(true);
+ // reset (becasue the request body was not fully read)
+ parser.readFrame(true);
+ // Validate response
+ Assert.assertEquals(
+ "3-HeadersStart\n" +
+ "3-Header-[:status]-[404]\n" +
+ "3-Header-[content-type]-[text/html;charset=utf-8]\n" +
+ "3-Header-[content-language]-[en]\n" +
+ "3-Header-[content-length]-[692]\n" +
+ "3-Header-[date]-[Wed, 11 Nov 2015 19:18:42 GMT]\n" +
+ "3-HeadersEnd\n" +
+ "3-Body-692\n" +
+ "3-EndOfStream\n" +
+ "3-RST-[8]\n", output.getTrace());
+ output.clearTrace();
+
+ // Write 3*16k=48k of request body
+ int count = 0;
+ while (count < 3) {
+ writeFrame(dataFrameHeader, dataPayload);
+ waitForWindowSize(0);
+ dataPayload.position(0);
+ count++;
+ }
+
+ // EOS
+ dataFrameHeader[4] = 0x01;
+ writeFrame(dataFrameHeader, dataPayload);
+ waitForWindowSize(0);
+ }
+
+
+ /*
+ * This might be unnecessary but given the potential for timing differences
+ * across different systems a more robust approach seems prudent.
+ */
+ private void waitForWindowSize(int streamId) throws Http2Exception,
IOException {
+ String prefix = streamId + "-WindowSize-";
+ boolean found = false;
+ String trace;
+ do {
+ parser.readFrame(true);
+ trace = output.getTrace();
+ output.clearTrace();
+ System.out.println(trace);
+ found = trace.startsWith(prefix);
+ } while (!found);
+ }
+}
+
diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
index 2be4fd4..d77a083 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -123,6 +123,12 @@
Simplify the closing on an HTTP/2 stream when an error condition is
present. (markt)
</scode>
+ <fix>
+ <bug>65179</bug>: Ensure that the connection level flow control window
+ from the client to the server is updated when handling DATA frames
+ received for completed streams else the flow control window may become
+ exhausted. (markt)
+ </fix>
</changelog>
</subsection>
</section>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
