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
The following commit(s) were added to refs/heads/master by this push:
new 03a1ccf Send WINDOW_UPDATE frames correctly when a DATA frame
includes padding
03a1ccf is described below
commit 03a1ccfa7c3cc48bb9dd4cc134b42cbb4a306c7d
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 3d17d98..ff58a7e 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 fa828c9..6b2369b 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 bb5e038..99792dc 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 90785f3..167fd94 100644
--- a/test/org/apache/coyote/http2/Http2TestBase.java
+++ b/test/org/apache/coyote/http2/Http2TestBase.java
@@ -468,19 +468,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 1bc6937..26d7820 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -135,6 +135,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
