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 8c0c7c1 Fix BZ 65118 - avoid an NPE when pruning streams
8c0c7c1 is described below
commit 8c0c7c177dc74421c3ef95d4f8b079775b4e75d8
Author: Mark Thomas <[email protected]>
AuthorDate: Mon Feb 1 15:30:27 2021 +0000
Fix BZ 65118 - avoid an NPE when pruning streams
The pruning process considers the parent/child relationship between
streams. The parent/child relationship is part of the priority tree so
the code needs to be holding the appropriate lock to ensure a consistent
view.
https://bz.apache.org/bugzilla/show_bug.cgi?id=65118
---
.../apache/coyote/http2/Http2UpgradeHandler.java | 78 ++++++++++++----------
webapps/docs/changelog.xml | 12 +++-
2 files changed, 52 insertions(+), 38 deletions(-)
diff --git a/java/org/apache/coyote/http2/Http2UpgradeHandler.java
b/java/org/apache/coyote/http2/Http2UpgradeHandler.java
index 1c52cce..9f1cd8a 100644
--- a/java/org/apache/coyote/http2/Http2UpgradeHandler.java
+++ b/java/org/apache/coyote/http2/Http2UpgradeHandler.java
@@ -1217,40 +1217,23 @@ public class Http2UpgradeHandler extends AbstractStream
implements InternalHttpU
// Step 1
// Iterator is in key order so we automatically have the oldest streams
// first
- for (AbstractNonZeroStream stream : streams.values()) {
- // Never remove active streams
- if (stream instanceof Stream && ((Stream) stream).isActive()) {
- continue;
- }
-
- if (stream.isClosedFinal()) {
- // This stream went from IDLE to CLOSED and is likely to have
- // been created by the client as part of the priority tree.
- // Candidate for step 3.
- candidatesStepThree.add(stream.getIdentifier());
- } else if (stream.getChildStreams().size() == 0) {
- // Prune it
- AbstractStream parent = stream.getParentStream();
- streams.remove(stream.getIdentifier());
- stream.detachFromParent();
- if (log.isDebugEnabled()) {
- log.debug(sm.getString("upgradeHandler.pruned",
connectionId, stream.getIdAsString()));
- }
- if (--toClose < 1) {
- return;
+ // Tests depend on parent/child relationship between streams so need to
+ // lock on priorityTreeLock to ensure a consistent view.
+ synchronized (priorityTreeLock) {
+ for (AbstractNonZeroStream stream : streams.values()) {
+ // Never remove active streams
+ if (stream instanceof Stream && ((Stream) stream).isActive()) {
+ continue;
}
- // If removing this child made the parent childless then see if
- // the parent can be removed.
- // Don't try and remove Stream 0 as that is the connection
- // Don't try and remove 'newer' streams. We'll get to them as
we
- // work through the ordered list of streams.
- while (toClose > 0 && parent.getIdAsInt() > 0 &&
parent.getIdAsInt() < stream.getIdAsInt() &&
- parent.getChildStreams().isEmpty()) {
- // This case is safe since we know parent ID > 0 therefore
- // this isn't the connection
- stream = (AbstractNonZeroStream) parent;
- parent = stream.getParentStream();
+ if (stream.isClosedFinal()) {
+ // This stream went from IDLE to CLOSED and is likely to
have
+ // been created by the client as part of the priority tree.
+ // Candidate for step 3.
+ candidatesStepThree.add(stream.getIdentifier());
+ } else if (stream.getChildStreams().size() == 0) {
+ // Prune it
+ AbstractStream parent = stream.getParentStream();
streams.remove(stream.getIdentifier());
stream.detachFromParent();
if (log.isDebugEnabled()) {
@@ -1259,12 +1242,33 @@ public class Http2UpgradeHandler extends AbstractStream
implements InternalHttpU
if (--toClose < 1) {
return;
}
- // Also need to remove this stream from the step 2 list
- candidatesStepTwo.remove(stream.getIdentifier());
+
+ // If removing this child made the parent childless then
see if
+ // the parent can be removed.
+ // Don't try and remove Stream 0 as that is the connection
+ // Don't try and remove 'newer' streams. We'll get to them
as we
+ // work through the ordered list of streams.
+ while (toClose > 0 && parent.getIdAsInt() > 0 &&
parent.getIdAsInt() < stream.getIdAsInt() &&
+ parent.getChildStreams().isEmpty()) {
+ // This case is safe since we know parent ID > 0
therefore
+ // this isn't the connection
+ stream = (AbstractNonZeroStream) parent;
+ parent = stream.getParentStream();
+ streams.remove(stream.getIdentifier());
+ stream.detachFromParent();
+ if (log.isDebugEnabled()) {
+ log.debug(sm.getString("upgradeHandler.pruned",
connectionId, stream.getIdAsString()));
+ }
+ if (--toClose < 1) {
+ return;
+ }
+ // Also need to remove this stream from the step 2 list
+ candidatesStepTwo.remove(stream.getIdentifier());
+ }
+ } else {
+ // Closed, with children. Candidate for step 2.
+ candidatesStepTwo.add(stream.getIdentifier());
}
- } else {
- // Closed, with children. Candidate for step 2.
- candidatesStepTwo.add(stream.getIdentifier());
}
}
diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
index 9b3aa82..b95fd57 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -103,7 +103,17 @@
They eventually become mixed with the numbered issues (i.e., numbered
issues do not "pop up" wrt. others).
-->
-<section name="Tomcat 8.5.63 (markt)" rtext="in development">
+<section name="Tomcat 9.0.44 (markt)" rtext="in development">
+ <subsection name="Coyote">
+ <changelog>
+ <fix>
+ <bug>65118</bug>: Fix a potential <code>NullPointerException</code>
when
+ pruning closed HTTP/2 streams from the connection. (markt)
+ </fix>
+ </changelog>
+ </subsection>
+</section>
+<section name="Tomcat 9.0.43 (markt)" rtext="release in progress">
<subsection name="Catalina">
<changelog>
<fix>
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
