This is an automated email from the ASF dual-hosted git repository.
markt 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 c9856e0 Send WINDOW_UPDATE frames correctly when a DATA frame
includes padding
c9856e0 is described below
commit c9856e0851519e7eee6c3d680e9ffe05f27e205e
Author: Mark Thomas <ma...@apache.org>
AuthorDate: Fri Aug 28 13:04:43 2020 +0100
Send WINDOW_UPDATE frames correctly when a DATA frame includes padding
When a Stream was closed the window update frame was not sent for the
connection. When zero length padding was used, an update frame was not
sent. In both cases the result would be a smaller flow control window
than intended.
Bugs detected by Travis CI
---
.../coyote/http2/Http2AsyncUpgradeHandler.java | 27 ++++++++--------
java/org/apache/coyote/http2/Http2Parser.java | 2 +-
.../apache/coyote/http2/Http2UpgradeHandler.java | 27 ++++++++--------
test/org/apache/coyote/http2/Http2TestBase.java | 28 +++++++++++++----
.../apache/coyote/http2/TestHttp2Section_6_1.java | 36 +++++++++++++++++-----
webapps/docs/changelog.xml | 10 ++++++
6 files changed, 91 insertions(+), 39 deletions(-)
diff --git a/java/org/apache/coyote/http2/Http2AsyncUpgradeHandler.java
b/java/org/apache/coyote/http2/Http2AsyncUpgradeHandler.java
index 545292f..e730737 100644
--- a/java/org/apache/coyote/http2/Http2AsyncUpgradeHandler.java
+++ b/java/org/apache/coyote/http2/Http2AsyncUpgradeHandler.java
@@ -229,23 +229,26 @@ public class Http2AsyncUpgradeHandler extends
Http2UpgradeHandler {
@Override
void writeWindowUpdate(Stream stream, int increment, boolean
applicationInitiated)
throws IOException {
- if (!stream.canWrite()) {
- return;
- }
// Build window update frame for stream 0
byte[] frame = new byte[13];
ByteUtil.setThreeBytes(frame, 0, 4);
frame[3] = FrameType.WINDOW_UPDATE.getIdByte();
ByteUtil.set31Bits(frame, 9, increment);
- // Change stream Id
- byte[] frame2 = new byte[13];
- ByteUtil.setThreeBytes(frame2, 0, 4);
- frame2[3] = FrameType.WINDOW_UPDATE.getIdByte();
- ByteUtil.set31Bits(frame2, 9, increment);
- ByteUtil.set31Bits(frame2, 5, stream.getIdAsInt());
- socketWrapper.write(BlockingMode.SEMI_BLOCK,
protocol.getWriteTimeout(),
- TimeUnit.MILLISECONDS, null, SocketWrapperBase.COMPLETE_WRITE,
errorCompletion,
- ByteBuffer.wrap(frame), ByteBuffer.wrap(frame2));
+ if (stream.canWrite()) {
+ // Change stream Id
+ byte[] frame2 = new byte[13];
+ ByteUtil.setThreeBytes(frame2, 0, 4);
+ frame2[3] = FrameType.WINDOW_UPDATE.getIdByte();
+ ByteUtil.set31Bits(frame2, 9, increment);
+ ByteUtil.set31Bits(frame2, 5, stream.getIdAsInt());
+ socketWrapper.write(BlockingMode.SEMI_BLOCK,
protocol.getWriteTimeout(),
+ TimeUnit.MILLISECONDS, null,
SocketWrapperBase.COMPLETE_WRITE, errorCompletion,
+ ByteBuffer.wrap(frame), ByteBuffer.wrap(frame2));
+ } else {
+ socketWrapper.write(BlockingMode.SEMI_BLOCK,
protocol.getWriteTimeout(),
+ TimeUnit.MILLISECONDS, null,
SocketWrapperBase.COMPLETE_WRITE, errorCompletion,
+ ByteBuffer.wrap(frame));
+ }
handleAsyncException();
}
diff --git a/java/org/apache/coyote/http2/Http2Parser.java
b/java/org/apache/coyote/http2/Http2Parser.java
index 38f0b13..4725a1f 100644
--- a/java/org/apache/coyote/http2/Http2Parser.java
+++ b/java/org/apache/coyote/http2/Http2Parser.java
@@ -207,7 +207,7 @@ class Http2Parser {
output.endRequestBodyFrame(streamId);
}
}
- if (padLength > 0) {
+ if (Flags.hasPadding(flags)) {
output.swallowedPadding(streamId, padLength);
}
}
diff --git a/java/org/apache/coyote/http2/Http2UpgradeHandler.java
b/java/org/apache/coyote/http2/Http2UpgradeHandler.java
index a1feb4d..e63f174 100644
--- a/java/org/apache/coyote/http2/Http2UpgradeHandler.java
+++ b/java/org/apache/coyote/http2/Http2UpgradeHandler.java
@@ -795,9 +795,6 @@ class Http2UpgradeHandler extends AbstractStream implements
InternalHttpUpgradeH
*/
void writeWindowUpdate(Stream stream, int increment, boolean
applicationInitiated)
throws IOException {
- if (!stream.canWrite()) {
- return;
- }
synchronized (socketWrapper) {
// Build window update frame for stream 0
byte[] frame = new byte[13];
@@ -805,17 +802,21 @@ class Http2UpgradeHandler extends AbstractStream
implements InternalHttpUpgradeH
frame[3] = FrameType.WINDOW_UPDATE.getIdByte();
ByteUtil.set31Bits(frame, 9, increment);
socketWrapper.write(true, frame, 0, frame.length);
- // Change stream Id and re-use
- ByteUtil.set31Bits(frame, 5, stream.getIdAsInt());
- try {
- socketWrapper.write(true, frame, 0, frame.length);
- socketWrapper.flush(true);
- } catch (IOException ioe) {
- if (applicationInitiated) {
- handleAppInitiatedIOException(ioe);
- } else {
- throw ioe;
+ if (stream.canWrite()) {
+ // Change stream Id and re-use
+ ByteUtil.set31Bits(frame, 5, stream.getIdAsInt());
+ try {
+ socketWrapper.write(true, frame, 0, frame.length);
+ socketWrapper.flush(true);
+ } catch (IOException ioe) {
+ if (applicationInitiated) {
+ handleAppInitiatedIOException(ioe);
+ } else {
+ throw ioe;
+ }
}
+ } else {
+ socketWrapper.flush(true);
}
}
}
diff --git a/test/org/apache/coyote/http2/Http2TestBase.java
b/test/org/apache/coyote/http2/Http2TestBase.java
index e982c8e..ca72aed 100644
--- a/test/org/apache/coyote/http2/Http2TestBase.java
+++ b/test/org/apache/coyote/http2/Http2TestBase.java
@@ -466,19 +466,35 @@ public abstract class Http2TestBase extends
TomcatBaseTest {
protected void readSimplePostResponse(boolean padding) throws
Http2Exception, IOException {
- if (padding) {
- // Window updates for padding
- parser.readFrame(true);
- parser.readFrame(true);
- }
+ /*
+ * If there is padding there will always be a window update for the
+ * connection and, depending on timing, there may be an update for the
+ * stream. The Window updates for padding (if present) may appear at
any
+ * time. The comments in the code below are only indicative of what the
+ * frames are likely to contain. Actual frame order with padding may be
+ * different.
+ */
+
// Connection window update after reading request body
parser.readFrame(true);
// Stream window update after reading request body
parser.readFrame(true);
// Headers
parser.readFrame(true);
- // Body
+ // Body (includes end of stream)
parser.readFrame(true);
+
+ if (padding) {
+ // Connection window update for padding
+ parser.readFrame(true);
+
+ // If EndOfStream has not been received then the stream window
+ // update must have been received so a further frame needs to be
+ // read for EndOfStream.
+ if (!output.getTrace().contains("EndOfStream")) {
+ parser.readFrame(true);
+ }
+ }
}
diff --git a/test/org/apache/coyote/http2/TestHttp2Section_6_1.java
b/test/org/apache/coyote/http2/TestHttp2Section_6_1.java
index 20405d4..8e3d948 100644
--- a/test/org/apache/coyote/http2/TestHttp2Section_6_1.java
+++ b/test/org/apache/coyote/http2/TestHttp2Section_6_1.java
@@ -62,14 +62,21 @@ public class TestHttp2Section_6_1 extends Http2TestBase {
sendSimplePostRequest(3, padding);
readSimplePostResponse(true);
- // The window update for the padding could occur anywhere since it
- // happens on a different thead to the response.
+ // The window updates for padding could occur anywhere since they
+ // happen on a different thread to the response.
+ // The connection window update is always present if there is
+ // padding.
String trace = output.getTrace();
- String paddingWindowUpdate =
"0-WindowSize-[9]\n3-WindowSize-[9]\n";
-
+ String paddingWindowUpdate = "0-WindowSize-[9]\n";
Assert.assertTrue(trace, trace.contains(paddingWindowUpdate));
trace = trace.replace(paddingWindowUpdate, "");
+ // The stream window update may or may not be present depending on
+ // timing. Remove it if present.
+ if (trace.contains("3-WindowSize-[9]\n")) {
+ trace = trace.replace("3-WindowSize-[9]\n", "");
+ }
+
Assert.assertEquals("0-WindowSize-[119]\n" +
"3-WindowSize-[119]\n" +
"3-HeadersStart\n" +
@@ -155,8 +162,23 @@ public class TestHttp2Section_6_1 extends Http2TestBase {
byte[] padding = new byte[0];
sendSimplePostRequest(3, padding);
- // Since padding is zero length, response looks like there is none.
- readSimplePostResponse(false);
+ readSimplePostResponse(true);
+
+ // The window updates for padding could occur anywhere since they
+ // happen on a different thread to the response.
+ // The connection window update is always present if there is
+ // padding.
+ String trace = output.getTrace();
+ String paddingWindowUpdate = "0-WindowSize-[1]\n";
+ Assert.assertTrue(trace, trace.contains(paddingWindowUpdate));
+ trace = trace.replace(paddingWindowUpdate, "");
+
+ // The stream window update may or may not be present depending on
+ // timing. Remove it if present.
+ paddingWindowUpdate = "3-WindowSize-[1]\n";
+ if (trace.contains(paddingWindowUpdate)) {
+ trace = trace.replace(paddingWindowUpdate, "");
+ }
Assert.assertEquals("0-WindowSize-[127]\n" +
"3-WindowSize-[127]\n" +
@@ -166,6 +188,6 @@ public class TestHttp2Section_6_1 extends Http2TestBase {
"3-Header-[date]-[Wed, 11 Nov 2015 19:18:42 GMT]\n" +
"3-HeadersEnd\n" +
"3-Body-127\n" +
- "3-EndOfStream\n", output.getTrace());
+ "3-EndOfStream\n", trace);
}
}
diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
index 6cf117a..a48cfbb 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -131,6 +131,16 @@
the stream immediately if it is waiting for an allocation when the flow
control error occurs. (markt)
</fix>
+ <fix>
+ Ensure that window update frames are sent for HTTP/2 connections to
+ account for DATA frames containing padding including when the
associated
+ stream has been closed. (markt)
+ </fix>
+ <fix>
+ Ensure that window update frames are sent for HTTP/2 connections and
+ streams to account for DATA frames containing zero-length padding.
+ (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
