This is an automated email from the ASF dual-hosted git repository.
markt-asf pushed a commit to branch 10.1.x
in repository https://gitbox.apache.org/repos/asf/tomcat.git
The following commit(s) were added to refs/heads/10.1.x by this push:
new 19d4061562 Fix an allocation leak in the HTTP/2 backlog tracking on
stream reset
19d4061562 is described below
commit 19d40615620fe145e88536e2bd63c5f01077c253
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 ec53004c85..bb84fdc791 100644
--- a/java/org/apache/coyote/http2/Http2UpgradeHandler.java
+++ b/java/org/apache/coyote/http2/Http2UpgradeHandler.java
@@ -328,7 +328,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
@@ -543,8 +543,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
*/
@@ -780,8 +780,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
*/
@@ -829,11 +829,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
@@ -988,7 +987,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
@@ -1104,24 +1103,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;
@@ -1163,8 +1164,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",
@@ -1230,7 +1237,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<>();
@@ -1352,6 +1359,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;
}
@@ -2048,6 +2056,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(
@@ -2116,7 +2165,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 514c82220b..c2c648b388 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">
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
