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

…-wsocket Backported HTTP2 fixed
tomitribe · Apr 3, 2024 · 0e97d8b · 0e97d8b
2 parents c32e9b9 + d380718
commit 0e97d8b
Show file tree

Hide file tree

Showing 6 changed files with 32 additions and 10 deletions.
diff --git 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.7
+version.suffix=-TT.8
 version.dev=
 
 # ----- Build tools -----

diff --git a/java/org/apache/coyote/http2/Http2Parser.java b/java/org/apache/coyote/http2/Http2Parser.java
@@ -289,6 +289,9 @@ protected void readHeadersFrame(int streamId, int flags, int payloadSize, ByteBu
 
         swallowPayload(streamId, FrameType.HEADERS.getId(), padLength, true, buffer);
 
+        // Validate the headers so far
+        hpackDecoder.getHeaderEmitter().validateHeaders();
+
         if (Flags.isEndOfHeaders(flags)) {
             onHeadersComplete(streamId);
         } else {
@@ -467,6 +470,9 @@ protected void readContinuationFrame(int streamId, int flags, int payloadSize, B
 
         readHeaderPayload(streamId, payloadSize, buffer);
 
+        // Validate the headers so far
+        hpackDecoder.getHeaderEmitter().validateHeaders();
+
         if (endOfHeaders) {
             headersCurrentStream = -1;
             onHeadersComplete(streamId);
@@ -633,11 +639,6 @@ protected void onHeadersComplete(int streamId) throws Http2Exception {
                     Http2Error.COMPRESSION_ERROR);
         }
 
-        // Delay validation (and triggering any exception) until this point
-        // since all the headers still have to be read if a StreamException is
-        // going to be thrown.
-        hpackDecoder.getHeaderEmitter().validateHeaders();
-
         synchronized (output) {
             output.headersEnd(streamId);
 

diff --git a/java/org/apache/coyote/http2/Http2UpgradeHandler.java b/java/org/apache/coyote/http2/Http2UpgradeHandler.java
@@ -92,7 +92,7 @@ class Http2UpgradeHandler extends AbstractStream implements InternalHttpUpgradeH
 
     private static final String HTTP2_SETTINGS_HEADER = "HTTP2-Settings";
 
-    private static final HeaderSink HEADER_SINK = new HeaderSink();
+    protected static final HeaderSink HEADER_SINK = new HeaderSink();
 
     private final Object priorityTreeLock = new Object();
 

diff --git a/java/org/apache/coyote/http2/Stream.java b/java/org/apache/coyote/http2/Stream.java
@@ -492,7 +492,7 @@ public void validateHeaders() throws StreamException {
         if (headerException == null) {
             return;
         }
-
+        handler.getHpackDecoder().setHeaderEmitter(Http2UpgradeHandler.HEADER_SINK);
         throw headerException;
     }
 

diff --git a/test/org/apache/coyote/http2/TestHttp2Limits.java b/test/org/apache/coyote/http2/TestHttp2Limits.java
@@ -168,7 +168,7 @@ public void testHeaderLimits1x32kin1kChunks() throws Exception {
         // 500ms per frame write delay to give server a chance to process the
         // stream reset and the connection reset before the request is fully
         // sent.
-        doTestHeaderLimits(1, 32*1024, 1024, 500, FailureMode.CONNECTION_RESET);
+        doTestHeaderLimits(1, 32 * 1024, 1024, 500, FailureMode.STREAM_RESET_THEN_CONNECTION_RESET);
     }
 
 
@@ -286,6 +286,13 @@ private void doTestHeaderLimits(int headerCount, int headerSize, int maxHeaderPa
             Assert.assertNull(e);
             break;
         }
+        case STREAM_RESET_THEN_CONNECTION_RESET: {
+                // Expect a stream reset
+                parser.readFrame();
+                Assert.assertEquals("3-RST-[11]\n", output.getTrace());
+                output.clearTrace();
+        }
+            //$FALL-THROUGH$
         case CONNECTION_RESET: {
             // This message uses i18n and needs to be used in a regular
             // expression (since we don't know the connection ID). Generate the
@@ -541,6 +548,10 @@ private void doTestPostWithTrailerHeaders(int maxTrailerCount, int maxTrailerSiz
             Assert.assertEquals("3-RST-[11]\n", output.getTrace());
             break;
         }
+        case STREAM_RESET_THEN_CONNECTION_RESET: {
+            Assert.fail("Not used");
+            break;
+        }
         case CONNECTION_RESET: {
             // NIO2 can sometimes send window updates depending timing
             skipWindowSizeFrames();
@@ -563,7 +574,7 @@ private enum FailureMode {
         NONE,
         STREAM_RESET,
         CONNECTION_RESET,
-
+        STREAM_RESET_THEN_CONNECTION_RESET,
     }
 
 

diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
@@ -189,6 +189,11 @@
       <fix>
         Improvements to HTTP/2 overhead protection. (markt)
       </fix>
+      <fix>
+        Remove the remaining reference to a stream once the stream has been
+        recycled. This makes the stream eligible for garbage collection earlier
+        and thereby improves scalability. (markt)
+      </fix>
     </changelog>
   </subsection>
   <subsection name="Jasper">
@@ -267,6 +272,11 @@
         Fix a regression in refactoring for Hashtables which caused mbeans to
         lose many of their attributes. (remm)
       </fix>
+      <fix>
+        Improve error reporting to HTTP/2 clients for header processing errors
+        by reporting problems at the end of the frame where the error was
+        detected rather than at the end of the headers. (markt)
+      </fix>
     </changelog>
   </subsection>
 </section>