Fixes #11822 - HTTP/2 responses exceeding SETTINGS_MAX_HEADER_LIST_SIZE do not result in RST_STREAM or GOAWAY.

jetty-core/jetty-http2/jetty-http2-client/src/main/java/org/eclipse/jetty/http2/client/HTTP2ClientConnectionFactory.java

@@ -95,8 +95,7 @@ private HTTP2ClientConnection(HTTP2Client client, EndPoint endpoint, HTTP2Client
         public void onOpen()
         {
             Map<Integer, Integer> settings = listener.onPreface(getSession());
-            if (settings == null)
-                settings = new HashMap<>();
+            settings = settings == null ? new HashMap<>() : new HashMap<>(settings);
 
             // Below we want to populate any settings to send to the server
             // that have a different default than what prescribed by the RFC.

jetty-core/jetty-http2/jetty-http2-common/src/main/java/org/eclipse/jetty/http2/internal/HTTP2Flusher.java

@@ -15,6 +15,7 @@
 
 import java.io.IOException;
 import java.nio.ByteBuffer;
+import java.nio.channels.ClosedChannelException;
 import java.util.ArrayDeque;
 import java.util.ArrayList;
 import java.util.Collection;
@@ -25,6 +26,7 @@
 import java.util.Queue;
 import java.util.Set;
 
+import org.eclipse.jetty.http2.ErrorCode;
 import org.eclipse.jetty.http2.FlowControlStrategy;
 import org.eclipse.jetty.http2.HTTP2Session;
 import org.eclipse.jetty.http2.HTTP2Stream;
@@ -106,6 +108,8 @@ public boolean append(HTTP2Session.Entry entry)
         try (AutoLock ignored = lock.lock())
         {
             closed = terminated;
+            if (closed instanceof HpackException.SessionException)
+                closed = null;
             if (closed == null)
             {
                 entries.offer(entry);
@@ -163,7 +167,12 @@ protected Action process() throws Throwable
         try (AutoLock ignored = lock.lock())
         {
             if (terminated != null)
-                throw terminated;
+            {
+                 if (terminated instanceof HpackException.SessionException)
+                     terminated = new ClosedChannelException().initCause(terminated);
+                 else
+                     throw terminated;
+            }
 
             WindowEntry windowEntry;
             while ((windowEntry = windows.poll()) != null)
@@ -248,6 +257,15 @@ protected Action process() throws Throwable
                     entry.failed(failure);
                     pending.remove();
                 }
+                catch (HpackException.SessionException failure)
+                {
+                    if (LOG.isDebugEnabled())
+                        LOG.debug("Failure generating {}", entry, failure);
+                    onSessionFailure(failure);
+                    // The method above will try to send
+                    // a GOAWAY, so we will iterate again.
+                    return Action.IDLE;
+                }
                 catch (Throwable failure)
                 {
                     // Failure to generate the entry is catastrophic.
@@ -339,7 +357,23 @@ protected void onCompleteSuccess()
     protected void onCompleteFailure(Throwable x)
     {
         accumulator.release();
+        Throwable closed = fail(x);
+        // If the failure came from within the
+        // flusher, we need to close the connection.
+        if (closed == null)
+            session.onWriteFailure(x);
+    }
+
+    private void onSessionFailure(Throwable x)
+    {
+        accumulator.release();
+        Throwable closed = fail(x);
+        if (closed == null)
+            session.close(ErrorCode.COMPRESSION_ERROR.code, null, NOOP);
+    }
 
+    private Throwable fail(Throwable x)
+    {
         Throwable closed;
         Set<HTTP2Session.Entry> allEntries;
         try (AutoLock ignored = lock.lock())
@@ -361,11 +395,7 @@ protected void onCompleteFailure(Throwable x)
         allEntries.addAll(pendingEntries);
         pendingEntries.clear();
         allEntries.forEach(entry -> entry.failed(x));
-
-        // If the failure came from within the
-        // flusher, we need to close the connection.
-        if (closed == null)
-            session.onWriteFailure(x);
+        return closed;
     }
 
     public void terminate(Throwable cause)

jetty-core/jetty-http2/jetty-http2-tests/src/test/java/org/eclipse/jetty/http2/tests/SettingsTest.java

@@ -16,7 +16,9 @@
 import java.nio.ByteBuffer;
 import java.util.HashMap;
 import java.util.Map;
+import java.util.concurrent.CompletableFuture;
 import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.ExecutionException;
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.atomic.AtomicReference;
 
@@ -37,9 +39,17 @@
 import org.eclipse.jetty.http2.hpack.HpackException;
 import org.eclipse.jetty.io.ByteBufferPool;
 import org.eclipse.jetty.util.Callback;
-import org.junit.jupiter.api.Assertions;
+import org.hamcrest.Matchers;
 import org.junit.jupiter.api.Test;
 
+import static org.awaitility.Awaitility.await;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.notNullValue;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
 public class SettingsTest extends AbstractTest
 {
     @Test
@@ -107,11 +117,11 @@ public void onClose(Session session, GoAwayFrame frame, Callback callback)
             .flip();
         ((HTTP2Session)clientSession).getEndPoint().write(Callback.NOOP, byteBuffer);
 
-        Assertions.assertFalse(serverSettingsLatch.get().await(1, TimeUnit.SECONDS));
-        Assertions.assertTrue(serverFailureLatch.await(5, TimeUnit.SECONDS));
-        Assertions.assertTrue(serverCloseLatch.await(5, TimeUnit.SECONDS));
-        Assertions.assertTrue(clientGoAwayLatch.await(5, TimeUnit.SECONDS));
-        Assertions.assertTrue(clientCloseLatch.await(5, TimeUnit.SECONDS));
+        assertFalse(serverSettingsLatch.get().await(1, TimeUnit.SECONDS));
+        assertTrue(serverFailureLatch.await(5, TimeUnit.SECONDS));
+        assertTrue(serverCloseLatch.await(5, TimeUnit.SECONDS));
+        assertTrue(clientGoAwayLatch.await(5, TimeUnit.SECONDS));
+        assertTrue(clientCloseLatch.await(5, TimeUnit.SECONDS));
     }
 
     @Test
@@ -181,11 +191,11 @@ public void onClose(Session session, GoAwayFrame frame, Callback callback)
             .flip();
         ((HTTP2Session)clientSession).getEndPoint().write(Callback.NOOP, byteBuffer);
 
-        Assertions.assertFalse(serverSettingsLatch.get().await(1, TimeUnit.SECONDS));
-        Assertions.assertTrue(serverFailureLatch.await(5, TimeUnit.SECONDS));
-        Assertions.assertTrue(serverCloseLatch.await(5, TimeUnit.SECONDS));
-        Assertions.assertTrue(clientGoAwayLatch.await(5, TimeUnit.SECONDS));
-        Assertions.assertTrue(clientCloseLatch.await(5, TimeUnit.SECONDS));
+        assertFalse(serverSettingsLatch.get().await(1, TimeUnit.SECONDS));
+        assertTrue(serverFailureLatch.await(5, TimeUnit.SECONDS));
+        assertTrue(serverCloseLatch.await(5, TimeUnit.SECONDS));
+        assertTrue(clientGoAwayLatch.await(5, TimeUnit.SECONDS));
+        assertTrue(clientCloseLatch.await(5, TimeUnit.SECONDS));
     }
 
     @Test
@@ -213,7 +223,7 @@ public Map<Integer, Integer> onPreface(Session session)
             }
         });
 
-        Assertions.assertTrue(serverFailureLatch.await(5, TimeUnit.SECONDS));
+        assertTrue(serverFailureLatch.await(5, TimeUnit.SECONDS));
     }
 
     @Test
@@ -242,7 +252,7 @@ public void onFailure(Session session, Throwable failure, Callback callback)
             }
         });
 
-        Assertions.assertTrue(clientFailureLatch.await(5, TimeUnit.SECONDS));
+        assertTrue(clientFailureLatch.await(5, TimeUnit.SECONDS));
     }
 
     @Test
@@ -304,9 +314,9 @@ public Stream.Listener onPush(Stream stream, PushPromiseFrame frame)
             }
         });
 
-        Assertions.assertTrue(serverPushFailureLatch.await(5, TimeUnit.SECONDS));
-        Assertions.assertTrue(clientResponseLatch.await(5, TimeUnit.SECONDS));
-        Assertions.assertFalse(clientPushLatch.await(1, TimeUnit.SECONDS));
+        assertTrue(serverPushFailureLatch.await(5, TimeUnit.SECONDS));
+        assertTrue(clientResponseLatch.await(5, TimeUnit.SECONDS));
+        assertFalse(clientPushLatch.await(1, TimeUnit.SECONDS));
     }
 
     @Test
@@ -361,6 +371,122 @@ public void onFailure(Session session, Throwable failure, Callback callback)
         HeadersFrame frame = new HeadersFrame(request, null, true);
         clientSession.newStream(frame, Stream.Listener.AUTO_DISCARD);
 
-        Assertions.assertTrue(clientFailureLatch.await(5, TimeUnit.SECONDS));
+        assertTrue(clientFailureLatch.await(5, TimeUnit.SECONDS));
+    }
+
+    @Test
+    public void testMaxHeaderListSizeExceededServerSendsGoAway() throws Exception
+    {
+        int maxHeadersSize = 512;
+        start(new ServerSessionListener()
+        {
+            @Override
+            public void onSettings(Session session, SettingsFrame frame)
+            {
+                ((HTTP2Session)session).getParser().getHpackDecoder().setMaxHeaderListSize(maxHeadersSize);
+            }
+        });
+
+        CountDownLatch goAwayLatch = new CountDownLatch(1);
+        Session clientSession = newClientSession(new Session.Listener()
+        {
+            @Override
+            public void onGoAway(Session session, GoAwayFrame frame)
+            {
+                goAwayLatch.countDown();
+            }
+        });
+        HttpFields requestHeaders = HttpFields.build()
+            .put("X-Large", "x".repeat(maxHeadersSize * 2));
+        MetaData.Request request = newRequest("GET", requestHeaders);
+        HeadersFrame frame = new HeadersFrame(request, null, true);
+        Stream stream = clientSession.newStream(frame, new Stream.Listener() {}).get(5, TimeUnit.SECONDS);
+
+        // The request can be sent by the client, the server will reject it.
+        // The spec suggests to send 431, but we do not want to "taint" the
+        // HPACK context with large headers.
+        assertNotNull(stream);
+
+        // The server should send a GOAWAY.
+        assertTrue(goAwayLatch.await(5, TimeUnit.SECONDS));
+    }
+
+    @Test
+    public void testMaxHeaderListSizeExceededByClient() throws Exception
+    {
+        int maxHeadersSize = 512;
+        CountDownLatch goAwayLatch = new CountDownLatch(1);
+        start(new ServerSessionListener()
+        {
+            @Override
+            public Map<Integer, Integer> onPreface(Session session)
+            {
+                return Map.of(SettingsFrame.MAX_HEADER_LIST_SIZE, maxHeadersSize);
+            }
+
+            @Override
+            public void onGoAway(Session session, GoAwayFrame frame)
+            {
+                goAwayLatch.countDown();
+            }
+        });
+
+        Session clientSession = newClientSession(new Session.Listener() {});
+        HttpFields requestHeaders = HttpFields.build()
+            .put("X-Large", "x".repeat(maxHeadersSize * 2));
+        MetaData.Request request = newRequest("GET", requestHeaders);
+        HeadersFrame frame = new HeadersFrame(request, null, true);
+
+        Throwable failure = assertThrows(ExecutionException.class,
+            () -> clientSession.newStream(frame, new Stream.Listener() {}).get(5, TimeUnit.SECONDS))
+            .getCause();
+        // The HPACK context is compromised trying to encode the large header.
+        assertThat(failure, Matchers.instanceOf(HpackException.SessionException.class));
+
+        assertTrue(goAwayLatch.await(5, TimeUnit.SECONDS));
+    }
+
+    @Test
+    public void testMaxHeaderListSizeExceededByServer() throws Exception
+    {
+        int maxHeadersSize = 512;
+        AtomicReference<CompletableFuture<Stream>> responseRef = new AtomicReference<>();
+        start(new ServerSessionListener()
+        {
+            @Override
+            public Stream.Listener onNewStream(Stream stream, HeadersFrame frame)
+            {
+                HttpFields responseHeaders = HttpFields.build()
+                    .put("X-Large", "x".repeat(maxHeadersSize * 2));
+                MetaData.Response response = new MetaData.Response(HttpStatus.OK_200, null, HttpVersion.HTTP_2, responseHeaders);
+                responseRef.set(stream.headers(new HeadersFrame(stream.getId(), response, null, true)));
+                return null;
+            }
+        });
+
+        CountDownLatch goAwayLatch = new CountDownLatch(1);
+        Session clientSession = newClientSession(new Session.Listener()
+        {
+            @Override
+            public Map<Integer, Integer> onPreface(Session session)
+            {
+                return Map.of(SettingsFrame.MAX_HEADER_LIST_SIZE, maxHeadersSize);
+            }
+
+            @Override
+            public void onGoAway(Session session, GoAwayFrame frame)
+            {
+                goAwayLatch.countDown();
+            }
+        });
+        MetaData.Request request = newRequest("GET", HttpFields.EMPTY);
+        HeadersFrame frame = new HeadersFrame(request, null, true);
+        clientSession.newStream(frame, new Stream.Listener() {});
+
+        CompletableFuture<Stream> completable = await().atMost(5, TimeUnit.SECONDS).until(responseRef::get, notNullValue());
+        Throwable failure = assertThrows(ExecutionException.class, () -> completable.get(5, TimeUnit.SECONDS)).getCause();
+        assertThat(failure, Matchers.instanceOf(HpackException.SessionException.class));
+
+        assertTrue(goAwayLatch.await(5, TimeUnit.SECONDS));
     }
 }