Merge pull request #43 from cesarhernandezgt/tomcat-10.0.x-TT.x-patch…

…-2afae30

Prepare for release 10.0.28-TT.9

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 -----

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<Throwable> error = new AtomicReference<>();
     private final AtomicReference<IOException> applicationIOE = new AtomicReference<>();
 
@@ -149,14 +151,20 @@ 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();
+                if (active) {
+                    decrementActiveRemoteStreamCount(getStream(se.getStreamId()));
+                }
             }
 
-            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();
     }
@@ -190,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();

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)

java/org/apache/coyote/http2/Http2UpgradeHandler.java

@@ -296,6 +296,11 @@ protected void processStreamOnContainerThread(Stream stream) {
     }
 
 
+    protected void decrementActiveRemoteStreamCount(Stream stream) {
+        setConnectionTimeoutForStreamCount(stream.decrementAndGetActiveRemoteStreamCount());
+    }
+
+
     void processStreamOnContainerThread(StreamProcessor streamProcessor, SocketEvent event) {
         StreamRunnable streamRunnable = new StreamRunnable(streamProcessor, event);
         if (streamConcurrency == null) {
@@ -593,7 +598,11 @@ void sendStreamReset(StreamStateMachine state, StreamException se) throws IOExce
         // order.
         synchronized (socketWrapper) {
             if (state != null) {
+                boolean active = state.isActive();
                 state.sendReset();
+                if (active) {
+                    decrementActiveRemoteStreamCount(getStream(se.getStreamId()));
+                }
             }
             socketWrapper.write(true, rstFrame, 0, rstFrame.length);
             socketWrapper.flush(true);
@@ -1161,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) {
@@ -1616,20 +1625,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);
@@ -1649,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",
@@ -1732,17 +1728,17 @@ 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()) {
                 if (stream.receivedEndOfHeaders()) {
-
-                    if (localSettings.getMaxConcurrentStreams() < activeRemoteStreamCount.incrementAndGet()) {
-                        setConnectionTimeoutForStreamCount(activeRemoteStreamCount.decrementAndGet());
+                    if (localSettings.getMaxConcurrentStreams() < activeRemoteStreamCount.get()) {
+                        decrementActiveRemoteStreamCount(stream);
                         // Ignoring maxConcurrentStreams increases the overhead count
                         increaseOverheadCount(FrameType.HEADERS);
                         throw new StreamException(sm.getString("upgradeHandler.tooManyRemoteStreams",
@@ -1752,9 +1748,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()) {
+            decrementActiveRemoteStreamCount(stream);
         }
     }
 
@@ -1780,7 +1807,7 @@ public void reset(int streamId, long errorCode) throws Http2Exception  {
             boolean active = stream.isActive();
             stream.receiveReset(errorCode);
             if (active) {
-                activeRemoteStreamCount.decrementAndGet();
+                decrementActiveRemoteStreamCount(stream);
             }
         }
     }

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 {

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);
@@ -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");
         }
 
 

test/org/apache/coyote/http2/TestHttp2UpgradeHandler.java

@@ -168,8 +168,9 @@ 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
+    //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 {
 
         enableHttp2(2, false, 10000, 10000, 4000, 2000, 2000);
@@ -203,8 +204,9 @@ public void testActiveConnectionCountAndClientTimeout() throws Exception {
 
             // 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"));
+            Assert.assertTrue(output.getTrace(), output.getTrace().startsWith(
+                    stream + "-HeadersStart\n" +
+                    stream + "-Header-[:status]-[400]\n"));
             output.clearTrace();
 
             // reset frame

webapps/docs/changelog.xml

@@ -170,6 +170,12 @@
   </subsection>
   <subsection name="Coyote">
     <changelog>
+      <fix>
+        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)
+      </fix>
       <fix>
         <bug>66276</bug>: Fix incorrect class cast when adding
         a descendant of HTTP/2 streams. (lihan)
@@ -194,6 +200,14 @@
         recycled. This makes the stream eligible for garbage collection earlier
         and thereby improves scalability. (markt)
       </fix>
+      <fix>
+        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)
+      </fix>
+      <fix>
+        Make counting of active HTTP/2 streams per connection more robust.
+        (markt)
+      </fix>
     </changelog>
   </subsection>
   <subsection name="Jasper">