Eclipse
/
jetty.project
mirror of https://github.com/jetty/jetty.project.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

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 <simone.bordet@gmail.com>
This commit is contained in:
Simone Bordet 2024-08-26 12:33:17 +03:00 committed by GitHub
parent b13f3ccc15
commit 0a56509130
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
5 changed files with 210 additions and 33 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

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

@ -95,8 +95,7 @@ public class HTTP2ClientConnectionFactory implements ClientConnectionFactory
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.

5
jetty-core/jetty-http2/jetty-http2-common/src/main/java/org/eclipse/jetty/http2/HTTP2Session.java
View File

@ -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()

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

@ -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<HTTP2Session.Entry> 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)

13
jetty-core/jetty-http2/jetty-http2-tests/src/test/java/org/eclipse/jetty/http2/tests/HTTP2ServerTest.java
View File

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

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

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