From fa0502db540b981fa51aec76bac9960edafd67c0 Mon Sep 17 00:00:00 2001 From: Simone Bordet Date: Mon, 9 Sep 2024 18:20:17 +0300 Subject: [PATCH] Fixes #12249 - HTTP/2 responses with Content-Length may have no content. (#12250) Fixed HttpReceiverOverHTTP2.read() to return a failed chunk if the stream has been reset. This closes the window where a RST_STREAM frame may be received, drain the DATA frames in HTTP2Stream, and the application reading exactly at that point. Signed-off-by: Simone Bordet --- .../internal/HttpReceiverOverHTTP2.java | 19 +++++-- .../HttpClientTransportOverHTTP2Test.java | 50 +++++++++++++++++++ .../transport/HttpClientContinueTest.java | 17 +++++-- .../transport/HttpClientContinueTest.java | 15 ++++-- 4 files changed, 89 insertions(+), 12 deletions(-) diff --git a/jetty-core/jetty-http2/jetty-http2-client-transport/src/main/java/org/eclipse/jetty/http2/client/transport/internal/HttpReceiverOverHTTP2.java b/jetty-core/jetty-http2/jetty-http2-client-transport/src/main/java/org/eclipse/jetty/http2/client/transport/internal/HttpReceiverOverHTTP2.java index bddb5d7396f..ba6835cfaf4 100644 --- a/jetty-core/jetty-http2/jetty-http2-client-transport/src/main/java/org/eclipse/jetty/http2/client/transport/internal/HttpReceiverOverHTTP2.java +++ b/jetty-core/jetty-http2/jetty-http2-client-transport/src/main/java/org/eclipse/jetty/http2/client/transport/internal/HttpReceiverOverHTTP2.java @@ -68,6 +68,7 @@ public class HttpReceiverOverHTTP2 extends HttpReceiver implements HTTP2Channel. Stream stream = getHttpChannel().getStream(); if (stream == null) return Content.Chunk.from(new EOFException("Channel has been released")); + Stream.Data data = stream.readData(); if (LOG.isDebugEnabled()) LOG.debug("Read stream data {} in {}", data, this); @@ -77,13 +78,25 @@ public class HttpReceiverOverHTTP2 extends HttpReceiver implements HTTP2Channel. stream.demand(); return null; } + DataFrame frame = data.frame(); boolean last = frame.remaining() == 0 && frame.isEndStream(); if (!last) - return Content.Chunk.asChunk(frame.getByteBuffer(), last, data); + return Content.Chunk.asChunk(frame.getByteBuffer(), false, data); + data.release(); - responseSuccess(getHttpExchange(), null); - return Content.Chunk.EOF; + + if (stream.isReset()) + { + Throwable failure = new EOFException("Stream has been reset"); + responseFailure(failure, Promise.noop()); + return Content.Chunk.from(failure); + } + else + { + responseSuccess(getHttpExchange(), null); + return Content.Chunk.EOF; + } } @Override diff --git a/jetty-core/jetty-http2/jetty-http2-tests/src/test/java/org/eclipse/jetty/http2/tests/HttpClientTransportOverHTTP2Test.java b/jetty-core/jetty-http2/jetty-http2-tests/src/test/java/org/eclipse/jetty/http2/tests/HttpClientTransportOverHTTP2Test.java index 94abef51874..2d5254ad095 100644 --- a/jetty-core/jetty-http2/jetty-http2-tests/src/test/java/org/eclipse/jetty/http2/tests/HttpClientTransportOverHTTP2Test.java +++ b/jetty-core/jetty-http2/jetty-http2-tests/src/test/java/org/eclipse/jetty/http2/tests/HttpClientTransportOverHTTP2Test.java @@ -46,6 +46,7 @@ import org.eclipse.jetty.client.Origin; import org.eclipse.jetty.client.Response; import org.eclipse.jetty.client.Result; import org.eclipse.jetty.http.HttpFields; +import org.eclipse.jetty.http.HttpHeader; import org.eclipse.jetty.http.HttpMethod; import org.eclipse.jetty.http.HttpStatus; import org.eclipse.jetty.http.HttpURI; @@ -91,6 +92,7 @@ import static org.hamcrest.Matchers.greaterThanOrEqualTo; import static org.hamcrest.Matchers.instanceOf; import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.not; +import static org.hamcrest.Matchers.notNullValue; import static org.hamcrest.Matchers.nullValue; import static org.junit.jupiter.api.Assertions.assertArrayEquals; import static org.junit.jupiter.api.Assertions.assertEquals; @@ -826,6 +828,54 @@ public class HttpClientTransportOverHTTP2Test extends AbstractTest assertEquals(HttpStatus.OK_200, response.getStatus()); } + @Test + public void testUnreadRequestContentDrainsResponseContent() throws Exception + { + start(new Handler.Abstract() + { + @Override + public boolean handle(Request request, org.eclipse.jetty.server.Response response, Callback callback) + { + // Do not read the request content, + // the server will reset the stream, + // then send a response with content. + ByteBuffer content = ByteBuffer.allocate(1024); + response.getHeaders().put(HttpHeader.CONTENT_LENGTH, content.remaining()); + response.write(true, content, callback); + return true; + } + }); + + AtomicReference contentSourceRef = new AtomicReference<>(); + AtomicReference chunkRef = new AtomicReference<>(); + CountDownLatch responseFailureLatch = new CountDownLatch(1); + AtomicReference resultRef = new AtomicReference<>(); + httpClient.newRequest("localhost", connector.getLocalPort()) + .method(HttpMethod.POST) + .body(new AsyncRequestContent(ByteBuffer.allocate(1024))) + .onResponseContentSource((response, contentSource) -> contentSourceRef.set(contentSource)) + // The request is failed before the response, verify that + // reading at the request failure event yields a failure chunk. + .onRequestFailure((request, failure) -> chunkRef.set(contentSourceRef.get().read())) + .onResponseFailure((response, failure) -> responseFailureLatch.countDown()) + .send(resultRef::set); + + // Wait for the RST_STREAM to arrive and drain the response content. + assertTrue(responseFailureLatch.await(5, TimeUnit.SECONDS)); + + // Verify that the chunk read at the request failure event is a failure chunk. + Content.Chunk chunk = chunkRef.get(); + assertTrue(Content.Chunk.isFailure(chunk, true)); + // Reading more also yields a failure chunk. + chunk = contentSourceRef.get().read(); + assertTrue(Content.Chunk.isFailure(chunk, true)); + + Result result = await().atMost(5, TimeUnit.SECONDS).until(resultRef::get, notNullValue()); + assertEquals(HttpStatus.OK_200, result.getResponse().getStatus()); + assertNotNull(result.getRequestFailure()); + assertNotNull(result.getResponseFailure()); + } + @Test @Tag("external") public void testExternalServer() throws Exception diff --git a/jetty-ee10/jetty-ee10-tests/jetty-ee10-test-client-transports/src/test/java/org/eclipse/jetty/ee10/test/client/transport/HttpClientContinueTest.java b/jetty-ee10/jetty-ee10-tests/jetty-ee10-test-client-transports/src/test/java/org/eclipse/jetty/ee10/test/client/transport/HttpClientContinueTest.java index 4626f0a7f5d..807ccd5dd05 100644 --- a/jetty-ee10/jetty-ee10-tests/jetty-ee10-test-client-transports/src/test/java/org/eclipse/jetty/ee10/test/client/transport/HttpClientContinueTest.java +++ b/jetty-ee10/jetty-ee10-tests/jetty-ee10-test-client-transports/src/test/java/org/eclipse/jetty/ee10/test/client/transport/HttpClientContinueTest.java @@ -269,11 +269,18 @@ public class HttpClientContinueTest extends AbstractTest { assertTrue(result.isFailed()); assertNotNull(result.getRequestFailure()); - assertNull(result.getResponseFailure()); - byte[] content = getContent(); - assertNotNull(content); - assertTrue(content.length > 0); assertEquals(error, result.getResponse().getStatus()); + Throwable responseFailure = result.getResponseFailure(); + // For HTTP/2 the response may fail because the + // server may not fully read the request content, + // and sends a reset that may drop the response + // content and cause the response failure. + if (responseFailure == null) + { + byte[] content = getContent(); + assertNotNull(content); + assertTrue(content.length > 0); + } latch.countDown(); } }); @@ -828,7 +835,7 @@ public class HttpClientContinueTest extends AbstractTest startServer(Transport.HTTP, new HttpServlet() { @Override - protected void service(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException + protected void service(HttpServletRequest request, HttpServletResponse response) throws IOException { assertEquals(0, request.getContentLengthLong()); assertNotNull(request.getHeader(HttpHeader.EXPECT.asString())); diff --git a/jetty-ee9/jetty-ee9-tests/jetty-ee9-test-client-transports/src/test/java/org/eclipse/jetty/ee9/test/client/transport/HttpClientContinueTest.java b/jetty-ee9/jetty-ee9-tests/jetty-ee9-test-client-transports/src/test/java/org/eclipse/jetty/ee9/test/client/transport/HttpClientContinueTest.java index 1f9b7d1fd9f..31203092dda 100644 --- a/jetty-ee9/jetty-ee9-tests/jetty-ee9-test-client-transports/src/test/java/org/eclipse/jetty/ee9/test/client/transport/HttpClientContinueTest.java +++ b/jetty-ee9/jetty-ee9-tests/jetty-ee9-test-client-transports/src/test/java/org/eclipse/jetty/ee9/test/client/transport/HttpClientContinueTest.java @@ -196,11 +196,18 @@ public class HttpClientContinueTest extends AbstractTest { assertTrue(result.isFailed()); assertNotNull(result.getRequestFailure()); - assertNull(result.getResponseFailure()); - byte[] content = getContent(); - assertNotNull(content); - assertTrue(content.length > 0); assertEquals(error, result.getResponse().getStatus()); + Throwable responseFailure = result.getResponseFailure(); + // For HTTP/2 the response may fail because the + // server may not fully read the request content, + // and sends a reset that may drop the response + // content and cause the response failure. + if (responseFailure == null) + { + byte[] content = getContent(); + assertNotNull(content); + assertTrue(content.length > 0); + } latch.countDown(); } });