This is an automated email from the ASF dual-hosted git repository.
markt-asf 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 0747dd58cc Fix an allocation leak in the HTTP/2 backlog tracking on stream reset
0747dd58cc is described below
commit 0747dd58cc631f90e044df246bd2ede6e2b48250
Author: Mark Thomas <[email protected]>
AuthorDate: Fri Jul 31 17:46:56 2026 +0100
Fix an allocation leak in the HTTP/2 backlog tracking on stream reset
---
java/org/apache/coyote/http2/AbstractStream.java | 13 ++-
.../apache/coyote/http2/Http2UpgradeHandler.java | 99 ++++++++++++++++------
.../apache/coyote/http2/TestHttp2Section_5_2.java | 49 +++++++++++
webapps/docs/changelog.xml | 4 +
4 files changed, 136 insertions(+), 29 deletions(-)
diff --git a/java/org/apache/coyote/http2/AbstractStream.java b/java/org/apache/coyote/http2/AbstractStream.java
index 90c186092b..bb0f0d1bc1 100644
--- a/java/org/apache/coyote/http2/AbstractStream.java
+++ b/java/org/apache/coyote/http2/AbstractStream.java
@@ -181,8 +181,11 @@ abstract class AbstractStream {
* @param connectionAllocationRequested the value
*/
final void setConnectionAllocationRequested(int connectionAllocationRequested) {
- log.trace(sm.getString("abstractStream.setConnectionAllocationRequested", getConnectionId(), getIdAsString(),
- Integer.toString(this.connectionAllocationRequested), Integer.toString(connectionAllocationRequested)));
+ if (log.isTraceEnabled()) {
+ log.trace(sm.getString("abstractStream.setConnectionAllocationRequested", getConnectionId(),
+ getIdAsString(), Integer.toString(this.connectionAllocationRequested),
+ Integer.toString(connectionAllocationRequested)));
+ }
this.connectionAllocationRequested = connectionAllocationRequested;
}
@@ -201,8 +204,10 @@ abstract class AbstractStream {
* @param connectionAllocationMade the value
*/
final void setConnectionAllocationMade(int connectionAllocationMade) {
- log.trace(sm.getString("abstractStream.setConnectionAllocationMade", getConnectionId(), getIdAsString(),
- Integer.toString(this.connectionAllocationMade), Integer.toString(connectionAllocationMade)));
+ if (log.isTraceEnabled()) {
+ log.trace(sm.getString("abstractStream.setConnectionAllocationMade", getConnectionId(), getIdAsString(),
+ Integer.toString(this.connectionAllocationMade), Integer.toString(connectionAllocationMade)));
+ }
this.connectionAllocationMade = connectionAllocationMade;
}
diff --git a/java/org/apache/coyote/http2/Http2UpgradeHandler.java b/java/org/apache/coyote/http2/Http2UpgradeHandler.java
index 1b5eb6d694..210a949872 100644
--- a/java/org/apache/coyote/http2/Http2UpgradeHandler.java
+++ b/java/org/apache/coyote/http2/Http2UpgradeHandler.java
@@ -325,7 +325,7 @@ class Http2UpgradeHandler extends AbstractStream implements InternalHttpUpgradeH
* Process the connection initialization, sending initial ping and processing the first stream.
*
* @param webConnection the web connection, may be null for direct HTTP/2
- * @param stream the initial stream
+ * @param stream the initial stream
*/
protected void processConnection(WebConnection webConnection, Stream stream) {
// Send a ping to get an idea of round trip time as early as possible
@@ -540,8 +540,8 @@ class Http2UpgradeHandler extends AbstractStream implements InternalHttpUpgradeH
/**
- * Sets the connection timeout based on the current number of active streams.
- * When no streams are active, uses the keep-alive timeout. Otherwise keeps the connection open.
+ * Sets the connection timeout based on the current number of active streams. When no streams are active, uses the
+ * keep-alive timeout. Otherwise keeps the connection open.
*
* @param streamCount the current number of active streams
*/
@@ -777,8 +777,8 @@ class Http2UpgradeHandler extends AbstractStream implements InternalHttpUpgradeH
* Write a GOAWAY frame to signal the peer that no more streams will be accepted.
*
* @param maxStreamId the maximum stream ID processed
- * @param errorCode the error code
- * @param debugMsg optional debug message
+ * @param errorCode the error code
+ * @param debugMsg optional debug message
*
* @throws IOException if an I/O error occurs
*/
@@ -826,11 +826,10 @@ class Http2UpgradeHandler extends AbstractStream implements InternalHttpUpgradeH
/**
- * Write headers for a stream without synchronizing on socketWrapper.
- * Separate method to allow Http2AsyncUpgradeHandler to call this code without synchronizing on socketWrapper since
- * it doesn't need to.
+ * Write headers for a stream without synchronizing on socketWrapper. Separate method to allow
+ * Http2AsyncUpgradeHandler to call this code without synchronizing on socketWrapper since it doesn't need to.
*
- * @param stream the stream to write headers for
+ * @param stream the stream to write headers for
* @param mimeHeaders the headers to write
* @param endOfStream whether this is the end of the stream
* @param payloadSize the initial payload size for the header frame
@@ -985,7 +984,7 @@ class Http2UpgradeHandler extends AbstractStream implements InternalHttpUpgradeH
* Handles an I/O error on the socket underlying the HTTP/2 connection when it is triggered by application code
* (usually reading the request or writing the response). Such I/O errors are fatal so the connection is closed. The
* exception is re-thrown to make the client code aware of the problem.
- *
+ * <p>
* Note: We can not rely on this exception reaching the socket processor since the application code may swallow it.
*
* @param ioe the I/O exception
@@ -1101,24 +1100,26 @@ class Http2UpgradeHandler extends AbstractStream implements InternalHttpUpgradeH
* stream was not already added to the backlog due to a partial reservation (see next else if block)
* add it to the backlog so it can obtain an allocation when capacity is available.
*/
- if (stream.getConnectionAllocationMade() == 0 && stream.getConnectionAllocationRequested() == 0) {
+ if (stream.getConnectionAllocationRequested() == 0) {
stream.setConnectionAllocationRequested(reservation);
backLogSize += reservation;
backLogStreams.add(stream);
}
} else if (windowSize < reservation) {
/*
- * The connection window has some capacity but not enough to fill this reservation. Allocate what
- * capacity is available and add the stream to the backlog so it can obtain a further allocation
- * when capacity is available.
+ * The connection window has some capacity but not enough to fill this reservation. If the stream
+ * has not been granted an allocation and the stream was not already added to the backlog, allocate
+ * what capacity is available and add the stream to the backlog so it can obtain a further
+ * allocation when capacity is available.
*/
- allocation = (int) windowSize;
- decrementWindowSize(allocation);
- int reservationRemaining = reservation - allocation;
- stream.setConnectionAllocationRequested(reservationRemaining);
- backLogSize += reservationRemaining;
- backLogStreams.add(stream);
-
+ if (stream.getConnectionAllocationRequested() == 0) {
+ allocation = (int) windowSize;
+ decrementWindowSize(allocation);
+ int reservationRemaining = reservation - allocation;
+ stream.setConnectionAllocationRequested(reservationRemaining);
+ backLogSize += reservationRemaining;
+ backLogStreams.add(stream);
+ }
} else {
// The connection window has sufficient capacity for this reservation. Allocate the full amount.
allocation = reservation;
@@ -1160,8 +1161,14 @@ class Http2UpgradeHandler extends AbstractStream implements InternalHttpUpgradeH
// stream is closing
stream.doStreamCancel(msg, error);
} else {
- allocation = stream.getConnectionAllocationMade();
- stream.setConnectionAllocationMade(0);
+ // Don't consume the allocation if the stream can no longer use it.
+ if (stream.canWrite()) {
+ allocation = stream.getConnectionAllocationMade();
+ stream.setConnectionAllocationMade(0);
+ } else {
+ stream.doStreamCancel(sm.getString("upgradeHandler.clientCancel"),
+ Http2Error.STREAM_CLOSED);
+ }
}
} catch (InterruptedException e) {
throw new IOException(sm.getString("upgradeHandler.windowSizeReservationInterrupted",
@@ -1227,7 +1234,7 @@ class Http2UpgradeHandler extends AbstractStream implements InternalHttpUpgradeH
}
- private Set<AbstractStream> releaseBackLog(int increment) throws Http2Exception {
+ private Set<AbstractStream> releaseBackLog(final int increment) throws Http2Exception {
windowAllocationLock.lock();
try {
Set<AbstractStream> result = new HashSet<>();
@@ -1349,6 +1356,7 @@ class Http2UpgradeHandler extends AbstractStream implements InternalHttpUpgradeH
int allocatedThisTime = Math.min(allocation, stream.getConnectionAllocationRequested());
stream.setConnectionAllocationRequested(stream.getConnectionAllocationRequested() - allocatedThisTime);
stream.setConnectionAllocationMade(stream.getConnectionAllocationMade() + allocatedThisTime);
+ backLogSize -= allocatedThisTime;
leftToAllocate = leftToAllocate - allocatedThisTime;
}
@@ -2045,6 +2053,47 @@ class Http2UpgradeHandler extends AbstractStream implements InternalHttpUpgradeH
log.trace(sm.getString("upgradeHandler.replace.first", getConnectionId(), original.getIdAsString()));
}
streams.put(original.getIdentifier(), replacement);
+
+ int made;
+ original.windowAllocationLock.lock();
+ try {
+ windowAllocationLock.lock();
+ try {
+ /*
+ * If the stream being replaced is still in the backlog (usually because it has been reset) remove
+ * the stream from the backlog along with its allocation request
+ */
+ if (backLogStreams.remove(original)) {
+ // Remove unallocated request from the backlog
+ backLogSize -= original.getConnectionAllocationRequested();
+ // Not strictly necessary, but set for consistency
+ original.setConnectionAllocationRequested(0);
+ }
+ made = original.getConnectionAllocationMade();
+ // Not strictly necessary, but set for consistency
+ original.setConnectionAllocationMade(0);
+ } finally {
+ windowAllocationLock.unlock();
+ }
+ } finally {
+ original.windowAllocationLock.unlock();
+ }
+
+ /*
+ * If the stream had received an allocation but not used it, return that allocation to the connection
+ * window.
+ */
+ if (made > 0) {
+ try {
+ incrementWindowSize(made);
+ } catch (Http2Exception e) {
+ /*
+ * Should not happen in normal usage. The exception only occurs if the Window size is increased
+ * beyond 2^31-1. If a client tries hard enough, it will be able to break its own connection.
+ */
+ throw new IllegalStateException(e);
+ }
+ }
} else {
if (log.isTraceEnabled()) {
log.trace(
@@ -2100,7 +2149,7 @@ class Http2UpgradeHandler extends AbstractStream implements InternalHttpUpgradeH
* Handle a received PING frame.
*
* @param payload the PING payload
- * @param ack whether this is a PING ACK
+ * @param ack whether this is a PING ACK
*
* @throws IOException if an I/O error occurs
*/
diff --git a/test/org/apache/coyote/http2/TestHttp2Section_5_2.java b/test/org/apache/coyote/http2/TestHttp2Section_5_2.java
index fbdd148de4..6c26c7728f 100644
--- a/test/org/apache/coyote/http2/TestHttp2Section_5_2.java
+++ b/test/org/apache/coyote/http2/TestHttp2Section_5_2.java
@@ -114,4 +114,53 @@ public class TestHttp2Section_5_2 extends Http2TestBase {
sendWindowUpdate(3, 8192);
parser.readFrame();
}
+
+
+ @Test
+ public void testFlowControlAndStreamReset() throws Exception {
+ // At start of test Stream 3 has 1k of 8k written and no capacity left
+
+ // Empty the connection window
+ sendWindowUpdate(3, 7 * 1024);
+ parser.readFrame();
+
+ sendWindowUpdate(0, 1);
+
+ for (int i = 5; i < 17; i += 2) {
+ sendSimpleGetRequest(i);
+ parser.readFrame();
+ parser.readFrame();
+ sendWindowUpdate(i, 7 * 1024);
+ parser.readFrame();
+ }
+
+ // Connection flow control window is now empty
+
+ // Put a stream on the backlog and then immediately cancel it
+ sendSimpleGetRequest(17);
+ sendRst(17, Http2Error.NO_ERROR.getCode());
+ // Read headers
+ parser.readFrame();
+ // Read reset from server
+ parser.readFrame();
+
+ // Increase default window size to 8k
+ sendSettings(0, false, new SettingValue(4, 8 * 1024));
+ // Settings ACK
+ parser.readFrame();
+
+ sendSimpleGetRequest(19);
+ // Read headers
+ parser.readFrame();
+
+ // Clear trace as what happens from this point is of primary interest in this test
+ output.clearTrace();
+
+ // This should release the entire body for stream 19
+ sendWindowUpdate(19, 7 * 1024);
+ sendWindowUpdate(0, 8 * 1024);
+
+ parser.readFrame();
+ Assert.assertEquals("19-Body-8192\n19-EndOfStream\n", output.getTrace());
+ }
}
diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
index 7b24439277..e5c8694597 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -240,6 +240,10 @@
Additional checks (enabled by default) have also been added for the
directory where the Unix Domain Socket will be created.(markt)
</add>
+ <fix>
+ Fix an allocation leak in the HTTP/2 backlog tracking when a stream is
+ reset. (markt)
+ </fix>
</changelog>
</subsection>
<subsection name="Jasper">
lmpx.com only provides a reader for public news (NNTP) servers. It is not
affiliated with the servers or forums shown here and is not responsible for
the content of articles, which is written by their respective authors.