From 6306d175d499c0c5f9715cc6989fc032f882b608 Mon Sep 17 00:00:00 2001 From: Sheldon Date: Fri, 10 May 2024 23:19:05 -0700 Subject: [PATCH 1/5] HttpExchange/HttpRequest got retained by HttpSenderOverHTTP --- .../jetty/client/transport/internal/HttpSenderOverHTTP.java | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/transport/internal/HttpSenderOverHTTP.java b/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/transport/internal/HttpSenderOverHTTP.java index a372be100878..6c1a414371ac 100644 --- a/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/transport/internal/HttpSenderOverHTTP.java +++ b/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/transport/internal/HttpSenderOverHTTP.java @@ -40,7 +40,6 @@ public class HttpSenderOverHTTP extends HttpSender private final IteratingCallback headersCallback = new HeadersCallback(); private final IteratingCallback contentCallback = new ContentCallback(); private final HttpGenerator generator = new HttpGenerator(); - private HttpExchange exchange; private MetaData.Request metaData; private ByteBuffer contentByteBuffer; private boolean lastContent; @@ -63,7 +62,6 @@ protected void sendHeaders(HttpExchange exchange, ByteBuffer contentBuffer, bool { try { - this.exchange = exchange; this.contentByteBuffer = contentBuffer; this.lastContent = lastContent; this.callback = callback; @@ -92,7 +90,6 @@ protected void sendContent(HttpExchange exchange, ByteBuffer contentBuffer, bool { try { - this.exchange = exchange; this.contentByteBuffer = contentBuffer; this.lastContent = lastContent; this.callback = callback; @@ -158,6 +155,7 @@ private HeadersCallback() protected Action process() throws Exception { HttpClient httpClient = getHttpChannel().getHttpDestination().getHttpClient(); + HttpExchange exchange = getHttpChannel().getHttpExchange(); ByteBufferPool bufferPool = httpClient.getByteBufferPool(); boolean useDirectByteBuffers = httpClient.isUseOutputDirectByteBuffers(); while (true) From d49a07a27c337743d91b36c18cc3843046dfa1bc Mon Sep 17 00:00:00 2001 From: "sheldon_shao@intuit.com" Date: Tue, 29 Oct 2024 10:24:01 -0700 Subject: [PATCH 2/5] PR for https://github.com/jetty/jetty.project/issues/12436 --- .../org/eclipse/jetty/client/HttpClient.java | 9 +++++++++ .../transport/internal/HttpSenderOverHTTP.java | 18 +++++++++++++++--- 2 files changed, 24 insertions(+), 3 deletions(-) diff --git a/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/HttpClient.java b/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/HttpClient.java index 7edbf36e20b2..6b86af055fab 100644 --- a/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/HttpClient.java +++ b/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/HttpClient.java @@ -135,6 +135,7 @@ public class HttpClient extends ContainerLifeCycle private String defaultRequestContentType = "application/octet-stream"; private boolean useInputDirectByteBuffers = true; private boolean useOutputDirectByteBuffers = true; + private int maxRequestHeadersSize = 32 * 1024; private int maxResponseHeadersSize = -1; private Sweeper destinationSweeper; @@ -1141,4 +1142,12 @@ public ClientConnectionFactory newSslClientConnectionFactory(SslContextFactory.C sslContextFactory = getSslContextFactory(); return new SslClientConnectionFactory(sslContextFactory, getByteBufferPool(), getExecutor(), connectionFactory); } + + public int getMaxRequestHeadersSize() { + return maxRequestHeadersSize; + } + + public void setMaxRequestHeadersSize(int maxRequestHeadersSize) { + this.maxRequestHeadersSize = maxRequestHeadersSize; + } } diff --git a/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/transport/internal/HttpSenderOverHTTP.java b/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/transport/internal/HttpSenderOverHTTP.java index 6c1a414371ac..e14bd774b4ea 100644 --- a/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/transport/internal/HttpSenderOverHTTP.java +++ b/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/transport/internal/HttpSenderOverHTTP.java @@ -178,9 +178,21 @@ protected Action process() throws Exception } case HEADER_OVERFLOW: { - headerBuffer.release(); - headerBuffer = null; - throw new IllegalArgumentException("Request header too large"); + int maxRequestHeadersSize = httpClient.getMaxRequestHeadersSize(); + if (headerBuffer.capacity() < maxRequestHeadersSize) { + RetainableByteBuffer newHeaderBuffer = bufferPool.acquire(maxRequestHeadersSize, useDirectByteBuffers); + headerBuffer.getByteBuffer().flip(); + newHeaderBuffer.getByteBuffer().put(headerBuffer.getByteBuffer()); + RetainableByteBuffer toRelease = headerBuffer; + headerBuffer = newHeaderBuffer; + toRelease.release(); + break; + } + else { + headerBuffer.release(); + headerBuffer = null; + throw new IllegalArgumentException("Request header too large"); + } } case NEED_CHUNK: { From a3f25e9adf22bab1dfb64a6de45d649e5fadb525 Mon Sep 17 00:00:00 2001 From: "sheldon_shao@intuit.com" Date: Tue, 29 Oct 2024 12:03:44 -0700 Subject: [PATCH 3/5] Unit test for flexible header buffer --- .../client/http/HttpSenderOverHTTPTest.java | 188 ++++++++++++++++++ 1 file changed, 188 insertions(+) diff --git a/jetty-core/jetty-client/src/test/java/org/eclipse/jetty/client/http/HttpSenderOverHTTPTest.java b/jetty-core/jetty-client/src/test/java/org/eclipse/jetty/client/http/HttpSenderOverHTTPTest.java index 313fd841de41..c7101f466a59 100644 --- a/jetty-core/jetty-client/src/test/java/org/eclipse/jetty/client/http/HttpSenderOverHTTPTest.java +++ b/jetty-core/jetty-client/src/test/java/org/eclipse/jetty/client/http/HttpSenderOverHTTPTest.java @@ -16,7 +16,10 @@ import java.net.URI; import java.nio.ByteBuffer; import java.nio.charset.StandardCharsets; +import java.util.HashMap; import java.util.Locale; +import java.util.Map; +import java.util.Random; import java.util.concurrent.CountDownLatch; import java.util.concurrent.TimeUnit; @@ -29,6 +32,7 @@ import org.eclipse.jetty.client.Result; import org.eclipse.jetty.client.transport.HttpDestination; import org.eclipse.jetty.client.transport.internal.HttpConnectionOverHTTP; +import org.eclipse.jetty.http.HttpCookie; import org.eclipse.jetty.io.ByteArrayEndPoint; import org.eclipse.jetty.util.Promise; import org.hamcrest.Matchers; @@ -39,6 +43,7 @@ import static org.awaitility.Awaitility.await; import static org.hamcrest.MatcherAssert.assertThat; import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.wildfly.common.Assert.assertFalse; public class HttpSenderOverHTTPTest { @@ -302,4 +307,187 @@ public void onSuccess(Request request) assertTrue(headersLatch.await(5, TimeUnit.SECONDS)); assertTrue(successLatch.await(5, TimeUnit.SECONDS)); } + + private static Random rnd = new Random(); + private static final String CHARS = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz1234567890"; + + public static final int CHARS_LENGTH = CHARS.length(); + + protected static String getRandomString(int size) { + StringBuilder sb = new StringBuilder(size); + while (sb.length() < size) { // length of the random string. + int index = rnd.nextInt(CHARS_LENGTH); + sb.append(CHARS.charAt(index)); + } + return sb.toString(); + } + + @Test + public void testSmallHeadersSize() throws Exception + { + ByteArrayEndPoint endPoint = new ByteArrayEndPoint(); + HttpDestination destination = new HttpDestination(client, new Origin("http", "localhost", 8080)); + destination.start(); + HttpConnectionOverHTTP connection = new HttpConnectionOverHTTP(endPoint, destination, new Promise.Adapter()); + Request request = client.newRequest(URI.create("http://localhost/")); + request.agent(getRandomString(888)); //More than the request buffer size, but less than the default max request headers size + final CountDownLatch headersLatch = new CountDownLatch(1); + final CountDownLatch successLatch = new CountDownLatch(1); + final CountDownLatch failureLatch = new CountDownLatch(1); + request.listener(new Request.Listener() + { + @Override + public void onHeaders(Request request) + { + headersLatch.countDown(); + } + + @Override + public void onSuccess(Request request) + { + successLatch.countDown(); + } + + @Override + public void onFailure(Request request, Throwable failure) { + failureLatch.countDown(); + } + }); + connection.send(request, null); + + String requestString = endPoint.takeOutputString(); + assertTrue(requestString.startsWith("GET / HTTP/1.1\r\nAccept-Encoding: gzip\r\n")); + assertTrue(headersLatch.await(5, TimeUnit.SECONDS)); + assertTrue(successLatch.await(5, TimeUnit.SECONDS)); + } + + @Test + public void testMaxRequestHeadersSize() throws Exception + { + byte[] buffer = new byte[32 * 1024]; + ByteArrayEndPoint endPoint = new ByteArrayEndPoint(buffer, buffer.length); + HttpDestination destination = new HttpDestination(client, new Origin("http", "localhost", 8080)); + destination.start(); + HttpConnectionOverHTTP connection = new HttpConnectionOverHTTP(endPoint, destination, new Promise.Adapter()); + Request request = client.newRequest(URI.create("http://localhost/")); + //More than the request buffer size, but less than the default max request headers size + + int desiredHeadersSize = 20 * 1024; + int currentHeadersSize = 0; + int i = 0; + while(currentHeadersSize < desiredHeadersSize) { + final int index = i ++; + final String headerValue = getRandomString(800); + final int headerSize = headerValue.length(); + currentHeadersSize += headerSize; + request.cookie(new HttpCookie() { + @Override + public String getName() { + return "large" + index; + } + + @Override + public String getValue() { + return headerValue; + } + + @Override + public int getVersion() { + return 0; + } + + @Override + public Map getAttributes() { + return new HashMap<>(); + } + }); + } + + final CountDownLatch headersLatch = new CountDownLatch(1); + final CountDownLatch successLatch = new CountDownLatch(1); + request.listener(new Request.Listener() + { + @Override + public void onHeaders(Request request) + { + headersLatch.countDown(); + } + + @Override + public void onSuccess(Request request) + { + successLatch.countDown(); + } + }); + connection.send(request, null); + + String requestString = endPoint.takeOutputString(); + assertTrue(requestString.startsWith("GET / HTTP/1.1\r\nAccept-Encoding: gzip\r\n")); + assertTrue(headersLatch.await(5, TimeUnit.SECONDS)); + assertTrue(successLatch.await(5, TimeUnit.SECONDS)); + } + + @Test + public void testMaxRequestHeadersSizeOverflow() throws Exception + { + byte[] buffer = new byte[32 * 1024]; + ByteArrayEndPoint endPoint = new ByteArrayEndPoint(buffer, buffer.length); + HttpDestination destination = new HttpDestination(client, new Origin("http", "localhost", 8080)); + destination.start(); + HttpConnectionOverHTTP connection = new HttpConnectionOverHTTP(endPoint, destination, new Promise.Adapter()); + Request request = client.newRequest(URI.create("http://localhost/")); + //More than the request buffer size, but less than the default max request headers size + + int desiredHeadersSize = 35 * 1024; + int currentHeadersSize = 0; + int i = 0; + while(currentHeadersSize < desiredHeadersSize) { + final int index = i ++; + final String headerValue = getRandomString(800); + final int headerSize = headerValue.length(); + currentHeadersSize += headerSize; + request.cookie(new HttpCookie() { + @Override + public String getName() { + return "large" + index; + } + + @Override + public String getValue() { + return headerValue; + } + + @Override + public int getVersion() { + return 0; + } + + @Override + public Map getAttributes() { + return new HashMap<>(); + } + }); + } + + final CountDownLatch headersLatch = new CountDownLatch(1); + final CountDownLatch failureLatch = new CountDownLatch(1); + request.listener(new Request.Listener() + { + @Override + public void onHeaders(Request request) + { + headersLatch.countDown(); + } + + @Override + public void onFailure(Request request, Throwable failure) + { + failureLatch.countDown(); + } + }); + connection.send(request, null); + + assertTrue(headersLatch.await(5, TimeUnit.SECONDS)); + assertTrue(failureLatch.await(5, TimeUnit.SECONDS)); + } } From 73075cdfa00374cdaab9b3ba152d59fb23938781 Mon Sep 17 00:00:00 2001 From: "sheldon_shao@intuit.com" Date: Tue, 29 Oct 2024 12:59:54 -0700 Subject: [PATCH 4/5] Revert the getHttpExchange change --- .../jetty/client/transport/internal/HttpSenderOverHTTP.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/transport/internal/HttpSenderOverHTTP.java b/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/transport/internal/HttpSenderOverHTTP.java index 37419b8bd157..b1d26e64ff5c 100644 --- a/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/transport/internal/HttpSenderOverHTTP.java +++ b/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/transport/internal/HttpSenderOverHTTP.java @@ -155,7 +155,7 @@ private HeadersCallback() protected Action process() throws Exception { HttpClient httpClient = getHttpChannel().getHttpDestination().getHttpClient(); - HttpExchange exchange = getHttpChannel().getHttpExchange(); + HttpExchange exchange = getHttpExchange(); ByteBufferPool bufferPool = httpClient.getByteBufferPool(); boolean useDirectByteBuffers = httpClient.isUseOutputDirectByteBuffers(); From 187459d35a54d1fd5edeb52358afc1a217e880bc Mon Sep 17 00:00:00 2001 From: "sheldon_shao@intuit.com" Date: Tue, 29 Oct 2024 13:08:19 -0700 Subject: [PATCH 5/5] Fix the merging error --- .../src/main/java/org/eclipse/jetty/client/HttpClient.java | 1 + 1 file changed, 1 insertion(+) diff --git a/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/HttpClient.java b/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/HttpClient.java index df22373f911a..7a3a429d04b3 100644 --- a/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/HttpClient.java +++ b/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/HttpClient.java @@ -1149,6 +1149,7 @@ public int getMaxRequestHeadersSize() { public void setMaxRequestHeadersSize(int maxRequestHeadersSize) { this.maxRequestHeadersSize = maxRequestHeadersSize; + } @Override public void close() throws Exception