Mark,
On 1/30/23 10:04, ma...@apache.org wrote:
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 08443cd8ac Fix BZ 66455 - avoid ClassCastException when processing
window update
08443cd8ac is described below
commit 08443cd8acb3c82cde7e28f13c7a95dd08547e44
Author: Mark Thomas <ma...@apache.org>
AuthorDate: Mon Jan 30 11:57:20 2023 +0000
Fix BZ 66455 - avoid ClassCastException when processing window update
https://bz.apache.org/bugzilla/show_bug.cgi?id=66455
---
.../apache/coyote/http2/Http2UpgradeHandler.java | 2 +-
.../apache/coyote/http2/TestHttp2Section_5_3.java | 50 ++++++++++++++++++++++
webapps/docs/changelog.xml | 6 +++
3 files changed, 57 insertions(+), 1 deletion(-)
diff --git a/java/org/apache/coyote/http2/Http2UpgradeHandler.java
b/java/org/apache/coyote/http2/Http2UpgradeHandler.java
index c14eeafc08..f38b8beb90 100644
--- a/java/org/apache/coyote/http2/Http2UpgradeHandler.java
+++ b/java/org/apache/coyote/http2/Http2UpgradeHandler.java
@@ -1037,13 +1037,13 @@ class Http2UpgradeHandler extends AbstractStream
implements InternalHttpUpgradeH
if (stream.getConnectionAllocationRequested() > 0) {
stream.setConnectionAllocationMade(stream.getConnectionAllocationRequested());
stream.setConnectionAllocationRequested(0);
+ result.add(stream);
}
}
remaining -= backLogSize;
backLogSize = 0;
super.incrementWindowSize(remaining);
- result.addAll(backLogStreams);
backLogStreams.clear();
} else {
allocate(this, remaining);
diff --git a/test/org/apache/coyote/http2/TestHttp2Section_5_3.java
b/test/org/apache/coyote/http2/TestHttp2Section_5_3.java
index 32fdb9fed6..4a50528dd6 100644
--- a/test/org/apache/coyote/http2/TestHttp2Section_5_3.java
+++ b/test/org/apache/coyote/http2/TestHttp2Section_5_3.java
@@ -242,6 +242,56 @@ public class TestHttp2Section_5_3 extends Http2TestBase {
}
+ @Test
+ public void testReleaseFullBacklog() throws Exception {
+
+ http2Connect();
+
+ // This test uses small window updates that will trigger the excessive
+ // overhead protection so disable it.
+ http2Protocol.setOverheadWindowUpdateThreshold(0);
+ // May also see (rarely, depends on timing) sequential 1 byte data
+ // frames on the same Stream
+ http2Protocol.setOverheadDataThreshold(0);
+
+
+ // Default connection window size is 64k - 1. Initial request will have
+ // used 8k (56k -1). Increase it to 57k
+ sendWindowUpdate(0, 1 + 1024);
+
+ // Use up 56k of the connection window
+ for (int i = 3; i < 17; i += 2) {
+ sendSimpleGetRequest(i);
+ readSimpleGetResponse();
+ }
All of the above numbers look bonkers. Surely some of it has to do with
the fact that I don;t understand the underlying implementation.
Is the default window size really 65535 bytes (64k - 1)? That would be
very unusual (is this some kind of spec-mandated window size? still
seems weird). It seems more likely that it is a full 64k bytes and that
the max index into that array would be 64k - 1.
How does sending a window update of 1025 increase the connection window
size to 57k? Why won't 1024 do it? Or 512? Or 1024 * 57? Or (64 - 8) *
1024 - 1?
How does sending 7 updates with odd i values between 3 and 17 fill the
first 56k of the connection window? Why use 3 - 17 instead of 0 - 6? Or,
if you need some single-digit i values and some double-digits, why not
use more straightforward loop bounds. It just looks very strange with no
explanation.
I hope these questions aren't just annoying. I'm sure you know what you
are doing. But I know if I was looking at this code to figure out why
the unit test was failing in the future, I would be at a serious loss to
understand what it was even doing in the first place.
-chris
+
+ output.clearTrace();
+
+ // At this point the connection window should be 1k and any new stream
+ // should have a window of 64k
+
+ // Create priority tree. This test requires a blocked stream to depend
on a closed stream
+ sendPriority(17, 15, 15);
+
+ // Process a request on stream 17.
+ // This should consume the connection window and put streams 15 and 17
in the backlog.
+ sendSimpleGetRequest(17);
+ // 17-headers, 17-1k-body
+ parser.readFrame();
+ parser.readFrame();
+ output.clearTrace();
+
+ // At this point 17 is blocked because the connection window is zero
+
+ // Send a large enough Window update to free the whole backlog
+ sendWindowUpdate(0, 8 * 1024);
+
+ parser.readFrame();
+
+ Assert.assertEquals("17-Body-7168\n17-EndOfStream\n",
output.getTrace());
+ }
+
+
private int[] parseBodyFrame(String output) {
String[] parts = output.trim().split("-");
if (parts.length != 3 || !"Body".equals(parts[1])) {
diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
index 01b994cc13..69fe963b5e 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -124,6 +124,12 @@
ensure that the end of stream flag is set on the headers frame and
that
no data frame is sent. (markt)
</fix>
+ <fix>
+ <bug>66455</bug>: Fix the cause of a potential
+ <code>ClassCastException</code> when processing a
+ <code>WINDOW_UPDATE</code> frame on an HTTP/2 connection where the flow
+ control window for the overall connection has been exhausted. (markt)
+ </fix>
</changelog>
</subsection>
<subsection name="Jasper">
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
