Skip to content

Commit

Permalink
Fixes #11822 - HTTP/2 responses exceeding SETTINGS_MAX_HEADER_LIST_SI…
Browse files Browse the repository at this point in the history 
…ZE do not result in RST_STREAM or GOAWAY.

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>
  • Loading branch information
@sbordet
sbordet committed Aug 14, 2024
1 parent cc2cef1 commit d26f06d
Show file tree
Hide file tree
Showing 3 changed files with 180 additions and 25 deletions.
3 changes: 1 addition & 2 deletions ...tp2-client/src/main/java/org/eclipse/jetty/http2/client/HTTP2ClientConnectionFactory.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
42 changes: 36 additions & 6 deletions ...http2/jetty-http2-common/src/main/java/org/eclipse/jetty/http2/internal/HTTP2Flusher.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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())
Expand All @@ -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)
Expand Down
160 changes: 143 additions & 17 deletions ...tty-http2/jetty-http2-tests/src/test/java/org/eclipse/jetty/http2/tests/SettingsTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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));
}
}

0 comments on commit d26f06d

Please sign in to comment.