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
The following commit(s) were added to refs/heads/8.5.x by this push:
new 63c1d4e Send WINDOW_UPDATE frames correctly when a DATA frame
includes padding
63c1d4e is described below
commit 63c1d4e607e6aaa7ca12cd9c8a09b16765adcd25
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
---
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 ++++++
5 files changed, 76 insertions(+), 27 deletions(-)
diff --git a/java/org/apache/coyote/http2/Http2Parser.java
b/java/org/apache/coyote/http2/Http2Parser.java
index 68dd528..a20ee33 100644
--- a/java/org/apache/coyote/http2/Http2Parser.java
+++ b/java/org/apache/coyote/http2/Http2Parser.java
@@ -195,7 +195,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 62e1423..2d1957a 100644
--- a/java/org/apache/coyote/http2/Http2UpgradeHandler.java
+++ b/java/org/apache/coyote/http2/Http2UpgradeHandler.java
@@ -792,9 +792,6 @@ public class Http2UpgradeHandler extends AbstractStream
implements InternalHttpU
*/
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];
@@ -802,17 +799,21 @@ public class Http2UpgradeHandler extends AbstractStream
implements InternalHttpU
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 ff70953..ce61d4b 100644
--- a/test/org/apache/coyote/http2/Http2TestBase.java
+++ b/test/org/apache/coyote/http2/Http2TestBase.java
@@ -459,19 +459,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 bba5959..67108b5 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -120,6 +120,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
