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 b125576 Fix BZ 65118 - avoid an NPE when pruning streams
b125576 is described below
commit b125576d149a9e990407e78701d3f82137b828be
Author: Mark Thomas <ma...@apache.org>
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 | 4 ++
2 files changed, 45 insertions(+), 37 deletions(-)
diff --git a/java/org/apache/coyote/http2/Http2UpgradeHandler.java
b/java/org/apache/coyote/http2/Http2UpgradeHandler.java
index 947be5f..0903f04 100644
--- a/java/org/apache/coyote/http2/Http2UpgradeHandler.java
+++ b/java/org/apache/coyote/http2/Http2UpgradeHandler.java
@@ -1218,40 +1218,23 @@ class Http2UpgradeHandler extends AbstractStream
implements InternalHttpUpgradeH
// 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()) {
@@ -1260,12 +1243,33 @@ class Http2UpgradeHandler extends AbstractStream
implements InternalHttpUpgradeH
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 dc88e3c..9ad8ecd 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -111,6 +111,10 @@
<code>org.apache.coyote.http11.Http11AprProtocol</code>. Depends on
<code>tomcat-native</code> 1.2.26 and up. (minfrin)
</add>
+ <fix>
+ <bug>65118</bug>: Fix a potential <code>NullPointerException</code>
when
+ pruning closed HTTP/2 streams from the connection. (markt)
+ </fix>
</changelog>
</subsection>
</section>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
