From eb5e0d9d8e2e4881465676ff9fb68413018418c4 Mon Sep 17 00:00:00 2001 From: Mark Thomas Date: Fri, 18 Nov 2022 05:07:17 -0600 Subject: [PATCH 1/7] Fix bug with HTTP/2 stream reset and active connection count When an HTTP/2 stream was reset, the current active stream count was not reduced. If enough resets occurred on a connection, the current active stream count limit was reached and no new streams could be created on that connection. --- .../http2/Http2AsyncUpgradeHandler.java | 4 ++ .../coyote/http2/Http2UpgradeHandler.java | 4 ++ .../apache/coyote/http2/Http2TestBase.java | 12 ++--- .../coyote/http2/TestHttp2UpgradeHandler.java | 51 +++++++++++++++++++ webapps/docs/changelog.xml | 6 +++ 5 files changed, 71 insertions(+), 6 deletions(-) diff --git a/java/org/apache/coyote/http2/Http2AsyncUpgradeHandler.java b/java/org/apache/coyote/http2/Http2AsyncUpgradeHandler.java index d79a4b7283fc..5901f1f227c1 100644 --- a/java/org/apache/coyote/http2/Http2AsyncUpgradeHandler.java +++ b/java/org/apache/coyote/http2/Http2AsyncUpgradeHandler.java @@ -151,7 +151,11 @@ void sendStreamReset(StreamStateMachine state, StreamException se) throws IOExce // order. synchronized (sendResetLock) { if (state != null) { + boolean active = state.isActive(); state.sendReset(); + if (active) { + activeRemoteStreamCount.decrementAndGet(); + } } socketWrapper.write(BlockingMode.SEMI_BLOCK, protocol.getWriteTimeout(), diff --git a/java/org/apache/coyote/http2/Http2UpgradeHandler.java b/java/org/apache/coyote/http2/Http2UpgradeHandler.java index bd4a1f845040..b91e328892e1 100644 --- a/java/org/apache/coyote/http2/Http2UpgradeHandler.java +++ b/java/org/apache/coyote/http2/Http2UpgradeHandler.java @@ -593,7 +593,11 @@ void sendStreamReset(StreamStateMachine state, StreamException se) throws IOExce // order. synchronized (socketWrapper) { if (state != null) { + boolean active = state.isActive(); state.sendReset(); + if (active) { + activeRemoteStreamCount.decrementAndGet(); + } } socketWrapper.write(true, rstFrame, 0, rstFrame.length); socketWrapper.flush(true); diff --git a/test/org/apache/coyote/http2/Http2TestBase.java b/test/org/apache/coyote/http2/Http2TestBase.java index 92c2f8024cd7..f918c9860a95 100644 --- a/test/org/apache/coyote/http2/Http2TestBase.java +++ b/test/org/apache/coyote/http2/Http2TestBase.java @@ -167,7 +167,7 @@ protected void validateHttp2InitialResponse(long maxConcurrentStreams) throws Ex parser.readFrame(); parser.readFrame(); - Assert.assertEquals("0-Settings-[3]-[200]\n" + + Assert.assertEquals("0-Settings-[3]-[" + maxConcurrentStreams + "]\n" + "0-Settings-End\n" + "0-Settings-Ack\n" + "0-Ping-[0,0,0,0,0,0,0,1]\n" + @@ -613,11 +613,11 @@ protected void enableHttp2(long maxConcurrentStreams, boolean tls, long readTime Assert.assertTrue(connector.setProperty("useAsyncIO", Boolean.toString(useAsyncIO))); http2Protocol = new UpgradableHttp2Protocol(); // Short timeouts for now. May need to increase these for CI systems. - http2Protocol.setReadTimeout(10000); - http2Protocol.setWriteTimeout(10000); - http2Protocol.setKeepAliveTimeout(25000); - http2Protocol.setStreamReadTimeout(5000); - http2Protocol.setStreamWriteTimeout(5000); + http2Protocol.setReadTimeout(readTimeout); + http2Protocol.setWriteTimeout(writeTimeout); + http2Protocol.setKeepAliveTimeout(keepAliveTimeout); + http2Protocol.setStreamReadTimeout(streamReadTimout); + http2Protocol.setStreamWriteTimeout(streamWriteTimeout); http2Protocol.setMaxConcurrentStreams(maxConcurrentStreams); http2Protocol.setHttp11Protocol((AbstractHttp11Protocol) connector.getProtocolHandler()); connector.addUpgradeProtocol(http2Protocol); diff --git a/test/org/apache/coyote/http2/TestHttp2UpgradeHandler.java b/test/org/apache/coyote/http2/TestHttp2UpgradeHandler.java index d9f555e63e36..a681bbd4b1ba 100644 --- a/test/org/apache/coyote/http2/TestHttp2UpgradeHandler.java +++ b/test/org/apache/coyote/http2/TestHttp2UpgradeHandler.java @@ -217,4 +217,55 @@ public void testActiveConnectionCountAndClientTimeout() throws Exception { dataFramePayload.clear(); } } + + + @Test + public void testActiveConnectionCountAndClientTimeout() throws Exception { + + enableHttp2(2, false, 10000, 10000, 4000, 2000, 2000); + + Tomcat tomcat = getTomcatInstance(); + + Context ctxt = tomcat.addContext("", null); + Tomcat.addServlet(ctxt, "simple", new SimpleServlet()); + ctxt.addServletMappingDecoded("/simple", "simple"); + + tomcat.start(); + + openClientConnection(); + doHttpUpgrade(); + sendClientPreface(); + validateHttp2InitialResponse(2); + + byte[] frameHeader = new byte[9]; + ByteBuffer headersPayload = ByteBuffer.allocate(128); + + byte[] dataFrameHeader = new byte[9]; + ByteBuffer dataFramePayload = ByteBuffer.allocate(128); + + // Should be able to make more than 2 requests even if they timeout + // since they should be removed from active connections once they + // timeout + for (int stream = 3; stream < 8; stream += 2) { + // Don't write the body. Allow the read to timeout. + buildPostRequest(frameHeader, headersPayload, false, dataFrameHeader, dataFramePayload, null, stream); + writeFrame(frameHeader, headersPayload); + + // 500 response (triggered by IOException trying to read body that never arrived) + parser.readFrame(); + Assert.assertTrue(output.getTrace(), output.getTrace().startsWith( + stream + "-HeadersStart\n" + + stream + "-Header-[:status]-[500]\n")); + output.clearTrace(); + + // reset frame + parser.readFrame(); + Assert.assertEquals(stream + "-RST-[11]\n", output.getTrace()); + output.clearTrace(); + + // Prepare buffers for re-use + headersPayload.clear(); + dataFramePayload.clear(); + } + } } diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml index 3a75374417a8..0cfbb0c34660 100644 --- a/webapps/docs/changelog.xml +++ b/webapps/docs/changelog.xml @@ -170,6 +170,12 @@ + + When an HTTP/2 stream was reset, the current active stream count was not + reduced. If enough resets occurred on a connection, the current active + stream count limit was reached and no new streams could be created on + that connection. (markt) + 66276: Fix incorrect class cast when adding a descendant of HTTP/2 streams. (lihan) From 651e6a2d5b75a53912c38bf47b71dca49bd69ece Mon Sep 17 00:00:00 2001 From: Cesar Hernandez Date: Tue, 16 Jul 2024 16:48:54 -0600 Subject: [PATCH 2/7] Fix TestHttp2UpgradeHandler.java test after backport done related to: Fix bug with HTTP/2 stream reset and active connection count --- .../coyote/http2/TestHttp2UpgradeHandler.java | 57 ++----------------- 1 file changed, 4 insertions(+), 53 deletions(-) diff --git a/test/org/apache/coyote/http2/TestHttp2UpgradeHandler.java b/test/org/apache/coyote/http2/TestHttp2UpgradeHandler.java index a681bbd4b1ba..78cd59c3ad45 100644 --- a/test/org/apache/coyote/http2/TestHttp2UpgradeHandler.java +++ b/test/org/apache/coyote/http2/TestHttp2UpgradeHandler.java @@ -168,57 +168,8 @@ private void doTestUpgradeWithRequestBody(boolean usePost, boolean useReader, bo } } - //Disabling this tests as this was introduced in 700d7d9 and it's out of the scope for CVE-2023-46589 patching. - //@Test - public void testActiveConnectionCountAndClientTimeout() throws Exception { - - enableHttp2(2, false, 10000, 10000, 4000, 2000, 2000); - - Tomcat tomcat = getTomcatInstance(); - - Context ctxt = tomcat.addContext("", null); - Tomcat.addServlet(ctxt, "simple", new SimpleServlet()); - ctxt.addServletMappingDecoded("/simple", "simple"); - - tomcat.start(); - - openClientConnection(); - doHttpUpgrade(); - sendClientPreface(); - validateHttp2InitialResponse(2); - - byte[] frameHeader = new byte[9]; - ByteBuffer headersPayload = ByteBuffer.allocate(128); - - byte[] dataFrameHeader = new byte[9]; - ByteBuffer dataFramePayload = ByteBuffer.allocate(128); - - // Should be able to make more than 2 requests even if they timeout - // since they should be removed from active connections once they - // timeout - for (int stream = 3; stream < 8; stream += 2) { - // Don't write the body. Allow the read to timeout. - buildPostRequest(frameHeader, headersPayload, false, dataFrameHeader, dataFramePayload, null, stream); - writeFrame(frameHeader, headersPayload); - - // 400 response (triggered by IOException trying to read body that never arrived) - parser.readFrame(); - Assert.assertTrue(output.getTrace(), - output.getTrace().startsWith(stream + "-HeadersStart\n" + stream + "-Header-[:status]-[400]\n")); - output.clearTrace(); - - // reset frame - parser.readFrame(); - Assert.assertEquals(stream + "-RST-[11]\n", output.getTrace()); - output.clearTrace(); - - // Prepare buffers for re-use - headersPayload.clear(); - dataFramePayload.clear(); - } - } - - + //CH: Enabling this test but updated to return 400 since that was later introduced in 10.1.x via + //commit dc38c17394916d6d2bf19da664e0a89660f3cdd1 @Test public void testActiveConnectionCountAndClientTimeout() throws Exception { @@ -251,11 +202,11 @@ public void testActiveConnectionCountAndClientTimeout() throws Exception { buildPostRequest(frameHeader, headersPayload, false, dataFrameHeader, dataFramePayload, null, stream); writeFrame(frameHeader, headersPayload); - // 500 response (triggered by IOException trying to read body that never arrived) + // 400 response (triggered by IOException trying to read body that never arrived) parser.readFrame(); Assert.assertTrue(output.getTrace(), output.getTrace().startsWith( stream + "-HeadersStart\n" + - stream + "-Header-[:status]-[500]\n")); + stream + "-Header-[:status]-[400]\n")); output.clearTrace(); // reset frame From 9e3641b51d3a963632db755d4925115d2bacc242 Mon Sep 17 00:00:00 2001 From: Mark Thomas Date: Wed, 26 Jul 2023 06:23:39 -0600 Subject: [PATCH 3/7] Refactor to reduce pinning in HTTP/2 code when using virtual threads --- .../http2/Http2AsyncUpgradeHandler.java | 29 ++++++++++++------- 1 file changed, 18 insertions(+), 11 deletions(-) diff --git a/java/org/apache/coyote/http2/Http2AsyncUpgradeHandler.java b/java/org/apache/coyote/http2/Http2AsyncUpgradeHandler.java index 5901f1f227c1..884f17860cf2 100644 --- a/java/org/apache/coyote/http2/Http2AsyncUpgradeHandler.java +++ b/java/org/apache/coyote/http2/Http2AsyncUpgradeHandler.java @@ -26,6 +26,8 @@ import java.util.List; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicReference; +import java.util.concurrent.locks.Lock; +import java.util.concurrent.locks.ReentrantLock; import jakarta.servlet.http.WebConnection; @@ -44,9 +46,9 @@ public class Http2AsyncUpgradeHandler extends Http2UpgradeHandler { // Ensures headers are generated and then written for one thread at a time. // Because of the compression used, headers need to be written to the // network in the same order they are generated. - private final Object headerWriteLock = new Object(); + private final Lock headerWriteLock = new ReentrantLock(); // Ensures thread triggers the stream reset is the first to send a RST frame - private final Object sendResetLock = new Object(); + private final Lock sendResetLock = new ReentrantLock(); private final AtomicReference error = new AtomicReference<>(); private final AtomicReference applicationIOE = new AtomicReference<>(); @@ -149,7 +151,8 @@ void sendStreamReset(StreamStateMachine state, StreamException se) throws IOExce // may see out of order RST frames which may hard to follow if // the client is unaware the RST frames may be received out of // order. - synchronized (sendResetLock) { + sendResetLock.lock(); + try { if (state != null) { boolean active = state.isActive(); state.sendReset(); @@ -158,9 +161,10 @@ void sendStreamReset(StreamStateMachine state, StreamException se) throws IOExce } } - socketWrapper.write(BlockingMode.SEMI_BLOCK, protocol.getWriteTimeout(), - TimeUnit.MILLISECONDS, null, SocketWrapperBase.COMPLETE_WRITE, errorCompletion, - ByteBuffer.wrap(rstFrame)); + socketWrapper.write(BlockingMode.SEMI_BLOCK, protocol.getWriteTimeout(), TimeUnit.MILLISECONDS, null, + SocketWrapperBase.COMPLETE_WRITE, errorCompletion, ByteBuffer.wrap(rstFrame)); + } finally { + sendResetLock.unlock(); } handleAsyncException(); } @@ -194,17 +198,20 @@ protected void writeGoAwayFrame(int maxStreamId, long errorCode, byte[] debugMsg @Override - void writeHeaders(Stream stream, int pushedStreamId, MimeHeaders mimeHeaders, - boolean endOfStream, int payloadSize) throws IOException { - synchronized (headerWriteLock) { - AsyncHeaderFrameBuffers headerFrameBuffers = (AsyncHeaderFrameBuffers) - doWriteHeaders(stream, pushedStreamId, mimeHeaders, endOfStream, payloadSize); + void writeHeaders(Stream stream, int pushedStreamId, MimeHeaders mimeHeaders, boolean endOfStream, int payloadSize) + throws IOException { + headerWriteLock.lock(); + try { + AsyncHeaderFrameBuffers headerFrameBuffers = (AsyncHeaderFrameBuffers) doWriteHeaders(stream, + pushedStreamId, mimeHeaders, endOfStream, payloadSize); if (headerFrameBuffers != null) { socketWrapper.write(BlockingMode.SEMI_BLOCK, protocol.getWriteTimeout(), TimeUnit.MILLISECONDS, null, SocketWrapperBase.COMPLETE_WRITE, applicationErrorCompletion, headerFrameBuffers.bufs.toArray(BYTEBUFFER_ARRAY)); handleAsyncException(); } + } finally { + headerWriteLock.unlock(); } if (endOfStream) { stream.sentEndOfStream(); From c065dfd38d3248ae4955e2b8538639b30a147bbc Mon Sep 17 00:00:00 2001 From: Mark Thomas Date: Mon, 7 Aug 2023 10:13:55 -0600 Subject: [PATCH 4/7] Fix race condition that was causing intermittent CI failures Ensure that, if there is no request body, the stream is notified that end-of-stream has been received before passing processing of the request to a container thread. --- java/org/apache/coyote/http2/Http2Parser.java | 6 +-- .../coyote/http2/Http2UpgradeHandler.java | 50 +++++++++++++------ .../apache/coyote/http2/Http2TestBase.java | 19 ++++--- webapps/docs/changelog.xml | 4 ++ 4 files changed, 52 insertions(+), 27 deletions(-) diff --git a/java/org/apache/coyote/http2/Http2Parser.java b/java/org/apache/coyote/http2/Http2Parser.java index 8588d15bb6ff..bf1b52e190e5 100644 --- a/java/org/apache/coyote/http2/Http2Parser.java +++ b/java/org/apache/coyote/http2/Http2Parser.java @@ -640,10 +640,9 @@ protected void onHeadersComplete(int streamId) throws Http2Exception { } synchronized (output) { - output.headersEnd(streamId); + output.headersEnd(streamId, headersEndStream); if (headersEndStream) { - output.receivedEndOfStream(streamId); headersEndStream = false; } } @@ -799,7 +798,8 @@ static interface Output { HeaderEmitter headersStart(int streamId, boolean headersEndStream) throws Http2Exception, IOException; void headersContinue(int payloadSize, boolean endOfHeaders); - void headersEnd(int streamId) throws Http2Exception; + + void headersEnd(int streamId, boolean endOfStream) throws Http2Exception; // Priority frames (also headers) void reprioritise(int streamId, int parentStreamId, boolean exclusive, int weight) diff --git a/java/org/apache/coyote/http2/Http2UpgradeHandler.java b/java/org/apache/coyote/http2/Http2UpgradeHandler.java index b91e328892e1..339bbb574d17 100644 --- a/java/org/apache/coyote/http2/Http2UpgradeHandler.java +++ b/java/org/apache/coyote/http2/Http2UpgradeHandler.java @@ -1620,20 +1620,6 @@ public void endRequestBodyFrame(int streamId, int dataLength) throws Http2Except } - @Override - public void receivedEndOfStream(int streamId) throws ConnectionException { - AbstractNonZeroStream abstractNonZeroStream = - getAbstractNonZeroStream(streamId, connectionState.get().isNewStreamAllowed()); - if (abstractNonZeroStream instanceof Stream) { - Stream stream = (Stream) abstractNonZeroStream; - stream.receivedEndOfStream(); - if (!stream.isActive()) { - setConnectionTimeoutForStreamCount(activeRemoteStreamCount.decrementAndGet()); - } - } - } - - @Override public void onSwallowedDataFramePayload(int streamId, int swallowedDataBytesCount) throws IOException { AbstractNonZeroStream abstractNonZeroStream = getAbstractNonZeroStream(streamId); @@ -1736,10 +1722,11 @@ public void headersContinue(int payloadSize, boolean endOfHeaders) { @Override - public void headersEnd(int streamId) throws Http2Exception { + public void headersEnd(int streamId, boolean endOfStream) throws Http2Exception { AbstractNonZeroStream abstractNonZeroStream = getAbstractNonZeroStream(streamId, connectionState.get().isNewStreamAllowed()); if (abstractNonZeroStream instanceof Stream) { + boolean processStream = false; setMaxProcessedStream(streamId); Stream stream = (Stream) abstractNonZeroStream; if (stream.isActive()) { @@ -1756,9 +1743,40 @@ public void headersEnd(int streamId) throws Http2Exception { // Valid new stream reduces the overhead count reduceOverheadCount(FrameType.HEADERS); - processStreamOnContainerThread(stream); + processStream = true; } } + /* + * Need to process end of stream before calling processStreamOnContainerThread to avoid a race condition + * where the container thread finishes before end of stream is processed, thinks the request hasn't been + * fully read so issues a RST with error code 0 (NO_ERROR) to tell the client not to send the request body, + * if any. This breaks tests and generates unnecessary RST messages for standard clients. + */ + if (endOfStream) { + receivedEndOfStream(stream); + } + if (processStream) { + processStreamOnContainerThread(stream); + } + } + } + + + @Override + public void receivedEndOfStream(int streamId) throws ConnectionException { + AbstractNonZeroStream abstractNonZeroStream = + getAbstractNonZeroStream(streamId, connectionState.get().isNewStreamAllowed()); + if (abstractNonZeroStream instanceof Stream) { + Stream stream = (Stream) abstractNonZeroStream; + receivedEndOfStream(stream); + } + } + + + private void receivedEndOfStream(Stream stream) throws ConnectionException { + stream.receivedEndOfStream(); + if (!stream.isActive()) { + setConnectionTimeoutForStreamCount(activeRemoteStreamCount.decrementAndGet()); } } diff --git a/test/org/apache/coyote/http2/Http2TestBase.java b/test/org/apache/coyote/http2/Http2TestBase.java index f918c9860a95..b2ed8bd34264 100644 --- a/test/org/apache/coyote/http2/Http2TestBase.java +++ b/test/org/apache/coyote/http2/Http2TestBase.java @@ -1087,13 +1087,6 @@ public void endRequestBodyFrame(int streamId, int dataLength) throws Http2Except } - @Override - public void receivedEndOfStream(int streamId) { - lastStreamId = Integer.toString(streamId); - trace.append(lastStreamId + "-EndOfStream\n"); - } - - @Override public HeaderEmitter headersStart(int streamId, boolean headersEndStream) { lastStreamId = Integer.toString(streamId); @@ -1149,8 +1142,18 @@ public void headersContinue(int payloadSize, boolean endOfHeaders) { @Override - public void headersEnd(int streamId) { + public void headersEnd(int streamId, boolean endOfStream) { trace.append(streamId + "-HeadersEnd\n"); + if (endOfStream) { + receivedEndOfStream(streamId) ; + } + } + + + @Override + public void receivedEndOfStream(int streamId) { + lastStreamId = Integer.toString(streamId); + trace.append(lastStreamId + "-EndOfStream\n"); } diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml index 0cfbb0c34660..7cb6e12a85db 100644 --- a/webapps/docs/changelog.xml +++ b/webapps/docs/changelog.xml @@ -200,6 +200,10 @@ recycled. This makes the stream eligible for garbage collection earlier and thereby improves scalability. (markt) + + Correct a race condition that could cause spurious RST messages to be + sent after the response had been written to an HTTP/2 stream. (markt) + From 59bce1f0ad819577d2ecca0b9f9cf75be8d6c982 Mon Sep 17 00:00:00 2001 From: remm Date: Thu, 19 Oct 2023 06:50:05 -0600 Subject: [PATCH 5/7] Refactor decrement using a common method --- .../coyote/http2/Http2AsyncUpgradeHandler.java | 2 +- .../apache/coyote/http2/Http2UpgradeHandler.java | 13 +++++++++---- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/java/org/apache/coyote/http2/Http2AsyncUpgradeHandler.java b/java/org/apache/coyote/http2/Http2AsyncUpgradeHandler.java index 884f17860cf2..6ffae27eefd6 100644 --- a/java/org/apache/coyote/http2/Http2AsyncUpgradeHandler.java +++ b/java/org/apache/coyote/http2/Http2AsyncUpgradeHandler.java @@ -157,7 +157,7 @@ void sendStreamReset(StreamStateMachine state, StreamException se) throws IOExce boolean active = state.isActive(); state.sendReset(); if (active) { - activeRemoteStreamCount.decrementAndGet(); + decrementActiveRemoteStreamCount(); } } diff --git a/java/org/apache/coyote/http2/Http2UpgradeHandler.java b/java/org/apache/coyote/http2/Http2UpgradeHandler.java index 339bbb574d17..7ec26578d0c9 100644 --- a/java/org/apache/coyote/http2/Http2UpgradeHandler.java +++ b/java/org/apache/coyote/http2/Http2UpgradeHandler.java @@ -296,6 +296,11 @@ protected void processStreamOnContainerThread(Stream stream) { } + protected void decrementActiveRemoteStreamCount() { + setConnectionTimeoutForStreamCount(activeRemoteStreamCount.decrementAndGet()); + } + + void processStreamOnContainerThread(StreamProcessor streamProcessor, SocketEvent event) { StreamRunnable streamRunnable = new StreamRunnable(streamProcessor, event); if (streamConcurrency == null) { @@ -596,7 +601,7 @@ void sendStreamReset(StreamStateMachine state, StreamException se) throws IOExce boolean active = state.isActive(); state.sendReset(); if (active) { - activeRemoteStreamCount.decrementAndGet(); + decrementActiveRemoteStreamCount(); } } socketWrapper.write(true, rstFrame, 0, rstFrame.length); @@ -1733,7 +1738,7 @@ public void headersEnd(int streamId, boolean endOfStream) throws Http2Exception if (stream.receivedEndOfHeaders()) { if (localSettings.getMaxConcurrentStreams() < activeRemoteStreamCount.incrementAndGet()) { - setConnectionTimeoutForStreamCount(activeRemoteStreamCount.decrementAndGet()); + decrementActiveRemoteStreamCount(); // Ignoring maxConcurrentStreams increases the overhead count increaseOverheadCount(FrameType.HEADERS); throw new StreamException(sm.getString("upgradeHandler.tooManyRemoteStreams", @@ -1776,7 +1781,7 @@ public void receivedEndOfStream(int streamId) throws ConnectionException { private void receivedEndOfStream(Stream stream) throws ConnectionException { stream.receivedEndOfStream(); if (!stream.isActive()) { - setConnectionTimeoutForStreamCount(activeRemoteStreamCount.decrementAndGet()); + decrementActiveRemoteStreamCount(); } } @@ -1802,7 +1807,7 @@ public void reset(int streamId, long errorCode) throws Http2Exception { boolean active = stream.isActive(); stream.receiveReset(errorCode); if (active) { - activeRemoteStreamCount.decrementAndGet(); + decrementActiveRemoteStreamCount(); } } } From 4c0ca65475fc665e8b47c20d50deb3b30f7c878e Mon Sep 17 00:00:00 2001 From: Mark Thomas Date: Wed, 8 May 2024 01:36:43 -0600 Subject: [PATCH 6/7] Make counting of active streams more robust --- .../coyote/http2/Http2AsyncUpgradeHandler.java | 2 +- .../coyote/http2/Http2UpgradeHandler.java | 18 +++++++++--------- java/org/apache/coyote/http2/Stream.java | 15 +++++++++++++++ webapps/docs/changelog.xml | 4 ++++ 4 files changed, 29 insertions(+), 10 deletions(-) diff --git a/java/org/apache/coyote/http2/Http2AsyncUpgradeHandler.java b/java/org/apache/coyote/http2/Http2AsyncUpgradeHandler.java index 6ffae27eefd6..dbd6429271af 100644 --- a/java/org/apache/coyote/http2/Http2AsyncUpgradeHandler.java +++ b/java/org/apache/coyote/http2/Http2AsyncUpgradeHandler.java @@ -157,7 +157,7 @@ void sendStreamReset(StreamStateMachine state, StreamException se) throws IOExce boolean active = state.isActive(); state.sendReset(); if (active) { - decrementActiveRemoteStreamCount(); + decrementActiveRemoteStreamCount(getStream(se.getStreamId())); } } diff --git a/java/org/apache/coyote/http2/Http2UpgradeHandler.java b/java/org/apache/coyote/http2/Http2UpgradeHandler.java index 7ec26578d0c9..d33e0a82e0e6 100644 --- a/java/org/apache/coyote/http2/Http2UpgradeHandler.java +++ b/java/org/apache/coyote/http2/Http2UpgradeHandler.java @@ -296,8 +296,8 @@ protected void processStreamOnContainerThread(Stream stream) { } - protected void decrementActiveRemoteStreamCount() { - setConnectionTimeoutForStreamCount(activeRemoteStreamCount.decrementAndGet()); + protected void decrementActiveRemoteStreamCount(Stream stream) { + setConnectionTimeoutForStreamCount(stream.decrementAndGetActiveRemoteStreamCount()); } @@ -601,7 +601,7 @@ void sendStreamReset(StreamStateMachine state, StreamException se) throws IOExce boolean active = state.isActive(); state.sendReset(); if (active) { - decrementActiveRemoteStreamCount(); + decrementActiveRemoteStreamCount(getStream(se.getStreamId())); } } socketWrapper.write(true, rstFrame, 0, rstFrame.length); @@ -1170,7 +1170,7 @@ private synchronized int allocate(AbstractStream stream, int allocation) { } - private Stream getStream(int streamId) { + Stream getStream(int streamId) { Integer key = Integer.valueOf(streamId); AbstractStream result = streams.get(key); if (result instanceof Stream) { @@ -1644,6 +1644,7 @@ public HeaderEmitter headersStart(int streamId, boolean headersEndStream) Stream stream = getStream(streamId, false); if (stream == null) { stream = createRemoteStream(streamId); + activeRemoteStreamCount.incrementAndGet(); } if (streamId < maxActiveRemoteStreamId) { throw new ConnectionException(sm.getString("upgradeHandler.stream.old", @@ -1736,9 +1737,8 @@ public void headersEnd(int streamId, boolean endOfStream) throws Http2Exception Stream stream = (Stream) abstractNonZeroStream; if (stream.isActive()) { if (stream.receivedEndOfHeaders()) { - - if (localSettings.getMaxConcurrentStreams() < activeRemoteStreamCount.incrementAndGet()) { - decrementActiveRemoteStreamCount(); + if (localSettings.getMaxConcurrentStreams() < activeRemoteStreamCount.get()) { + decrementActiveRemoteStreamCount(stream); // Ignoring maxConcurrentStreams increases the overhead count increaseOverheadCount(FrameType.HEADERS); throw new StreamException(sm.getString("upgradeHandler.tooManyRemoteStreams", @@ -1781,7 +1781,7 @@ public void receivedEndOfStream(int streamId) throws ConnectionException { private void receivedEndOfStream(Stream stream) throws ConnectionException { stream.receivedEndOfStream(); if (!stream.isActive()) { - decrementActiveRemoteStreamCount(); + decrementActiveRemoteStreamCount(stream); } } @@ -1807,7 +1807,7 @@ public void reset(int streamId, long errorCode) throws Http2Exception { boolean active = stream.isActive(); stream.receiveReset(errorCode); if (active) { - decrementActiveRemoteStreamCount(); + decrementActiveRemoteStreamCount(stream); } } } diff --git a/java/org/apache/coyote/http2/Stream.java b/java/org/apache/coyote/http2/Stream.java index 967643ff6412..df1252f3a718 100644 --- a/java/org/apache/coyote/http2/Stream.java +++ b/java/org/apache/coyote/http2/Stream.java @@ -27,6 +27,7 @@ import java.util.Locale; import java.util.Map; import java.util.Set; +import java.util.concurrent.atomic.AtomicBoolean; import java.util.function.Supplier; import org.apache.coyote.ActionCode; @@ -86,6 +87,7 @@ class Stream extends AbstractNonZeroStream implements HeaderEmitter { private final StreamInputBuffer inputBuffer; private final StreamOutputBuffer streamOutputBuffer = new StreamOutputBuffer(); private final Http2OutputBuffer http2OutputBuffer = new Http2OutputBuffer(coyoteResponse, streamOutputBuffer); + private final AtomicBoolean removedFromActiveCount = new AtomicBoolean(false); // State machine would be too much overhead private int headerState = HEADER_STATE_START; @@ -830,6 +832,19 @@ int getWindowUpdateSizeToWrite(int increment) { return result; } + int decrementAndGetActiveRemoteStreamCount() { + /* + * Protect against mis-counting of active streams. This method should only be called once per stream but since + * the count of active streams is used to enforce the maximum concurrent streams limit, make sure each stream is + * only removed from the active count exactly once. + */ + if (removedFromActiveCount.compareAndSet(false, true)) { + return handler.activeRemoteStreamCount.decrementAndGet(); + } else { + return handler.activeRemoteStreamCount.get(); + } + } + private static void push(final Http2UpgradeHandler handler, final Request request, final Stream stream) throws IOException { diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml index 7cb6e12a85db..c9537bf7b488 100644 --- a/webapps/docs/changelog.xml +++ b/webapps/docs/changelog.xml @@ -204,6 +204,10 @@ Correct a race condition that could cause spurious RST messages to be sent after the response had been written to an HTTP/2 stream. (markt) + + Make counting of active HTTP/2 streams per connection more robust. + (markt) + From 0487daa27511be95e857f236142ade32a945bbe8 Mon Sep 17 00:00:00 2001 From: Cesar Hernandez Date: Tue, 16 Jul 2024 18:12:12 -0600 Subject: [PATCH 7/7] Prepare for release Prepare for release 10.0.28-TT.9 --- build.properties.default | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/build.properties.default b/build.properties.default index 3329ecae1d7d..9b925d6466b2 100644 --- a/build.properties.default +++ b/build.properties.default @@ -33,7 +33,7 @@ version.major=10 version.minor=0 version.build=28 version.patch=0 -version.suffix=-TT.8 +version.suffix=-TT.9 version.dev= # ----- Build tools -----