From 0a565091301d8de6d37cd23c0a6ffd2eff0dbd2e Mon Sep 17 00:00:00 2001 From: Simone Bordet Date: Mon, 26 Aug 2024 12:33:17 +0300 Subject: [PATCH] Fixes #11822 - HTTP/2 responses exceeding SETTINGS_MAX_HEADER_LIST_SIZE do not result in RST_STREAM or GOAWAY. (#12165) Now HpackException.SessionException is treated specially in HTTP2Flusher. It is caught, it fails all the entries, and then tries to send a GOAWAY, which will be the only frame allowed into the HTTP2FLusher at that point. Signed-off-by: Simone Bordet --- .../client/HTTP2ClientConnectionFactory.java | 3 +- .../org/eclipse/jetty/http2/HTTP2Session.java | 5 + .../jetty/http2/internal/HTTP2Flusher.java | 62 +++++-- .../jetty/http2/tests/HTTP2ServerTest.java | 13 +- .../jetty/http2/tests/SettingsTest.java | 160 ++++++++++++++++-- 5 files changed, 210 insertions(+), 33 deletions(-) diff --git a/jetty-core/jetty-http2/jetty-http2-client/src/main/java/org/eclipse/jetty/http2/client/HTTP2ClientConnectionFactory.java b/jetty-core/jetty-http2/jetty-http2-client/src/main/java/org/eclipse/jetty/http2/client/HTTP2ClientConnectionFactory.java index 048eab4d062..dc2469e2d89 100644 --- a/jetty-core/jetty-http2/jetty-http2-client/src/main/java/org/eclipse/jetty/http2/client/HTTP2ClientConnectionFactory.java +++ b/jetty-core/jetty-http2/jetty-http2-client/src/main/java/org/eclipse/jetty/http2/client/HTTP2ClientConnectionFactory.java @@ -95,8 +95,7 @@ public class HTTP2ClientConnectionFactory implements ClientConnectionFactory public void onOpen() { Map 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. diff --git a/jetty-core/jetty-http2/jetty-http2-common/src/main/java/org/eclipse/jetty/http2/HTTP2Session.java b/jetty-core/jetty-http2/jetty-http2-common/src/main/java/org/eclipse/jetty/http2/HTTP2Session.java index 981036e64a0..7aec552d872 100644 --- a/jetty-core/jetty-http2/jetty-http2-common/src/main/java/org/eclipse/jetty/http2/HTTP2Session.java +++ b/jetty-core/jetty-http2/jetty-http2-common/src/main/java/org/eclipse/jetty/http2/HTTP2Session.java @@ -1248,6 +1248,11 @@ public abstract class HTTP2Session extends ContainerLifeCycle implements Session this.stream = stream; } + public Frame frame() + { + return frame; + } + public abstract int getFrameBytesGenerated(); public int getDataBytesRemaining() diff --git a/jetty-core/jetty-http2/jetty-http2-common/src/main/java/org/eclipse/jetty/http2/internal/HTTP2Flusher.java b/jetty-core/jetty-http2/jetty-http2-common/src/main/java/org/eclipse/jetty/http2/internal/HTTP2Flusher.java index 36b11c794dd..505ca16da58 100644 --- a/jetty-core/jetty-http2/jetty-http2-common/src/main/java/org/eclipse/jetty/http2/internal/HTTP2Flusher.java +++ b/jetty-core/jetty-http2/jetty-http2-common/src/main/java/org/eclipse/jetty/http2/internal/HTTP2Flusher.java @@ -15,6 +15,7 @@ package org.eclipse.jetty.http2.internal; 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,9 +26,11 @@ import java.util.List; 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; +import org.eclipse.jetty.http2.frames.FrameType; import org.eclipse.jetty.http2.frames.WindowUpdateFrame; import org.eclipse.jetty.http2.hpack.HpackException; import org.eclipse.jetty.io.ByteBufferPool; @@ -92,10 +95,9 @@ public class HTTP2Flusher extends IteratingCallback implements Dumpable entries.offerFirst(entry); if (LOG.isDebugEnabled()) LOG.debug("Prepended {}, entries={}", entry, entries.size()); + return true; } } - if (closed == null) - return true; closed(entry, closed); return false; } @@ -106,15 +108,17 @@ public class HTTP2Flusher extends IteratingCallback implements Dumpable try (AutoLock ignored = lock.lock()) { closed = terminated; + // If it was not possible to HPACK encode, then allow to send a GOAWAY. + if (closed instanceof HpackException.SessionException && entry.frame().getType() == FrameType.GO_AWAY) + closed = null; if (closed == null) { entries.offer(entry); if (LOG.isDebugEnabled()) LOG.debug("Appended {}, entries={}, {}", entry, entries.size(), this); + return true; } } - if (closed == null) - return true; closed(entry, closed); return false; } @@ -130,10 +134,9 @@ public class HTTP2Flusher extends IteratingCallback implements Dumpable list.forEach(entries::offer); if (LOG.isDebugEnabled()) LOG.debug("Appended {}, entries={} {}", list, entries.size(), this); + return true; } } - if (closed == null) - return true; list.forEach(entry -> closed(entry, closed)); return false; } @@ -163,7 +166,21 @@ public class HTTP2Flusher extends IteratingCallback implements Dumpable try (AutoLock ignored = lock.lock()) { if (terminated != null) - throw terminated; + { + boolean rethrow = true; + if (terminated instanceof HpackException.SessionException) + { + HTTP2Session.Entry entry = entries.peek(); + if (entry != null && entry.frame().getType() == FrameType.GO_AWAY) + { + // Allow a SessionException to be processed once to send a GOAWAY. + terminated = new ClosedChannelException().initCause(terminated); + rethrow = false; + } + } + if (rethrow) + throw terminated; + } WindowEntry windowEntry; while ((windowEntry = windows.poll()) != null) @@ -248,6 +265,15 @@ public class HTTP2Flusher extends IteratingCallback implements Dumpable 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 +365,23 @@ public class HTTP2Flusher extends IteratingCallback implements Dumpable 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 allEntries; try (AutoLock ignored = lock.lock()) @@ -361,11 +403,7 @@ public class HTTP2Flusher extends IteratingCallback implements Dumpable 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) diff --git a/jetty-core/jetty-http2/jetty-http2-tests/src/test/java/org/eclipse/jetty/http2/tests/HTTP2ServerTest.java b/jetty-core/jetty-http2/jetty-http2-tests/src/test/java/org/eclipse/jetty/http2/tests/HTTP2ServerTest.java index ec30896c9ad..d4e4116baec 100644 --- a/jetty-core/jetty-http2/jetty-http2-tests/src/test/java/org/eclipse/jetty/http2/tests/HTTP2ServerTest.java +++ b/jetty-core/jetty-http2/jetty-http2-tests/src/test/java/org/eclipse/jetty/http2/tests/HTTP2ServerTest.java @@ -428,11 +428,20 @@ public class HTTP2ServerTest extends AbstractServerTest } output.flush(); + AtomicBoolean goAway = new AtomicBoolean(); Parser parser = new Parser(bufferPool, 8192); - parser.init(new Parser.Listener() {}); + parser.init(new Parser.Listener() + { + @Override + public void onGoAway(GoAwayFrame frame) + { + goAway.set(true); + } + }); boolean closed = parseResponse(client, parser); - assertTrue(closed); + assertFalse(closed); + assertTrue(goAway.get()); } } } diff --git a/jetty-core/jetty-http2/jetty-http2-tests/src/test/java/org/eclipse/jetty/http2/tests/SettingsTest.java b/jetty-core/jetty-http2/jetty-http2-tests/src/test/java/org/eclipse/jetty/http2/tests/SettingsTest.java index ac461de250c..f7224ec82ed 100644 --- a/jetty-core/jetty-http2/jetty-http2-tests/src/test/java/org/eclipse/jetty/http2/tests/SettingsTest.java +++ b/jetty-core/jetty-http2/jetty-http2-tests/src/test/java/org/eclipse/jetty/http2/tests/SettingsTest.java @@ -16,7 +16,9 @@ package org.eclipse.jetty.http2.tests; 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.frames.SettingsFrame; 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 class SettingsTest extends AbstractTest .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 class SettingsTest extends AbstractTest .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 class SettingsTest extends AbstractTest } }); - Assertions.assertTrue(serverFailureLatch.await(5, TimeUnit.SECONDS)); + assertTrue(serverFailureLatch.await(5, TimeUnit.SECONDS)); } @Test @@ -242,7 +252,7 @@ public class SettingsTest extends AbstractTest } }); - Assertions.assertTrue(clientFailureLatch.await(5, TimeUnit.SECONDS)); + assertTrue(clientFailureLatch.await(5, TimeUnit.SECONDS)); } @Test @@ -304,9 +314,9 @@ public class SettingsTest extends AbstractTest } }); - 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 class SettingsTest extends AbstractTest 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 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> 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 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 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)); } }