From bd143a674ba7fbb3c7e7e450c8102e9a4bd95172 Mon Sep 17 00:00:00 2001 From: Greg Wilkins Date: Wed, 22 Aug 2018 08:29:12 +1000 Subject: [PATCH 1/6] Issue #2787 BadMessage if bad query detected in dispatcher (#2827) Signed-off-by: Greg Wilkins --- .../org/eclipse/jetty/server/Request.java | 11 +++- .../eclipse/jetty/servlet/ServletHandler.java | 8 +++ .../eclipse/jetty/servlet/DispatcherTest.java | 64 ++++++++++++++++++- 3 files changed, 77 insertions(+), 6 deletions(-) diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java b/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java index 6068f7c34d0..b75d0e6548e 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java @@ -2390,8 +2390,6 @@ public class Request implements HttpServletRequest /* ------------------------------------------------------------ */ public void mergeQueryParameters(String oldQuery,String newQuery, boolean updateQueryString) { - // TODO This is seriously ugly - MultiMap newQueryParams = null; // Have to assume ENCODING because we can't know otherwise. if (newQuery!=null) @@ -2404,7 +2402,14 @@ public class Request implements HttpServletRequest if (oldQueryParams == null && oldQuery != null) { oldQueryParams = new MultiMap<>(); - UrlEncoded.decodeTo(oldQuery, oldQueryParams, getQueryEncoding()); + try + { + UrlEncoded.decodeTo(oldQuery, oldQueryParams, getQueryEncoding()); + } + catch(Throwable th) + { + throw new BadMessageException(400,"Bad query encoding",th); + } } MultiMap mergedQueryParams; diff --git a/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/ServletHandler.java b/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/ServletHandler.java index 269ad2b44c1..ee2f0e01d0c 100644 --- a/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/ServletHandler.java +++ b/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/ServletHandler.java @@ -50,6 +50,7 @@ import javax.servlet.http.HttpServlet; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; +import org.eclipse.jetty.http.BadMessageException; import org.eclipse.jetty.http.HttpHeader; import org.eclipse.jetty.http.HttpHeaderValue; import org.eclipse.jetty.http.PathMap; @@ -647,8 +648,15 @@ public class ServletHandler extends ScopedHandler else response.sendError(HttpServletResponse.SC_SERVICE_UNAVAILABLE); } + else if (th instanceof BadMessageException) + { + BadMessageException bme = (BadMessageException)th; + response.sendError(bme.getCode(),bme.getMessage()); + } else + { response.sendError(HttpServletResponse.SC_INTERNAL_SERVER_ERROR); + } } else { diff --git a/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/DispatcherTest.java b/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/DispatcherTest.java index e7fb37e280c..c2657eba638 100644 --- a/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/DispatcherTest.java +++ b/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/DispatcherTest.java @@ -18,6 +18,7 @@ package org.eclipse.jetty.servlet; +import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.startsWith; import static org.junit.Assert.assertEquals; @@ -59,6 +60,8 @@ import org.eclipse.jetty.server.handler.ResourceHandler; import org.eclipse.jetty.toolchain.test.MavenTestingUtils; import org.eclipse.jetty.util.TypeUtil; import org.eclipse.jetty.util.UrlEncoded; +import org.eclipse.jetty.util.log.Log; +import org.eclipse.jetty.util.log.StacklessLogging; import org.junit.After; import org.junit.Assert; import org.junit.Before; @@ -156,6 +159,42 @@ public class DispatcherTest assertEquals(expected, responses); } + @Test + public void testForwardWithBadParams() throws Exception + { + try(StacklessLogging nostack = new StacklessLogging(ServletHandler.class)) + { + Log.getLogger(ServletHandler.class).info("Expect Not valid UTF8 warnings..."); + _contextHandler.addServlet(AlwaysForwardServlet.class, "/forward/*"); + _contextHandler.addServlet(EchoServlet.class, "/echo/*"); + + String response; + + response = _connector.getResponse("GET /context/forward/?echo=allgood HTTP/1.0\n\n"); + assertThat(response,containsString(" 200 OK")); + assertThat(response,containsString("allgood")); + + response = _connector.getResponse("GET /context/forward/params?echo=allgood HTTP/1.0\n\n"); + assertThat(response,containsString(" 200 OK")); + assertThat(response,containsString("allgood")); + assertThat(response,containsString("forward")); + + response = _connector.getResponse("GET /context/forward/badparams?echo=badparams HTTP/1.0\n\n"); + assertThat(response,containsString(" 500 ")); + + response = _connector.getResponse("GET /context/forward/?echo=badclient&bad=%88%A4 HTTP/1.0\n\n"); + assertThat(response,containsString(" 400 ")); + + response = _connector.getResponse("GET /context/forward/params?echo=badclient&bad=%88%A4 HTTP/1.0\n\n"); + assertThat(response,containsString(" 400 ")); + + response = _connector.getResponse("GET /context/forward/badparams?echo=badclientandparam&bad=%88%A4 HTTP/1.0\n\n"); + assertThat(response,containsString(" 500 ")); + } + } + + + @Test public void testInclude() throws Exception { @@ -358,6 +397,20 @@ public class DispatcherTest dispatcher.forward(request, response); } } + + public static class AlwaysForwardServlet extends HttpServlet implements Servlet + { + @Override + protected void doGet(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException + { + if ("/params".equals(request.getPathInfo())) + getServletContext().getRequestDispatcher("/echo?echo=forward").forward(request, response); + else if ("/badparams".equals(request.getPathInfo())) + getServletContext().getRequestDispatcher("/echo?echo=forward&fbad=%88%A4").forward(request, response); + else + getServletContext().getRequestDispatcher("/echo").forward(request, response); + } + } public static class ForwardNonUTF8Servlet extends HttpServlet implements Servlet @@ -480,15 +533,20 @@ public class DispatcherTest @Override public void service(ServletRequest req, ServletResponse res) throws ServletException, IOException { - String echoText = req.getParameter("echo"); + String[] echoText = req.getParameterValues("echo"); - if ( echoText == null ) + if ( echoText == null || echoText.length==0) { throw new ServletException("echo is a required parameter"); } + else if (echoText.length==1) + { + res.getWriter().print(echoText[0]); + } else { - res.getWriter().print(echoText); + for (String text:echoText) + res.getWriter().print(text); } } } From 084d6ce4439005299469e6b010e2b855bcf780e0 Mon Sep 17 00:00:00 2001 From: Ivanov Anton Date: Sat, 25 Aug 2018 18:27:18 +0300 Subject: [PATCH 2/6] Issue #2860 Fix leakage of HttpDestinations in HttpClient Signed-off-by: Ivanov Anton --- .../jetty/client/PoolingHttpDestination.java | 32 +++++++++++++------ .../http/HttpDestinationOverHTTPTest.java | 26 +++++++++++++++ 2 files changed, 48 insertions(+), 10 deletions(-) diff --git a/jetty-client/src/main/java/org/eclipse/jetty/client/PoolingHttpDestination.java b/jetty-client/src/main/java/org/eclipse/jetty/client/PoolingHttpDestination.java index e56d1787d3a..9e61f404046 100644 --- a/jetty-client/src/main/java/org/eclipse/jetty/client/PoolingHttpDestination.java +++ b/jetty-client/src/main/java/org/eclipse/jetty/client/PoolingHttpDestination.java @@ -210,16 +210,7 @@ public abstract class PoolingHttpDestination extends HttpD if (getHttpExchanges().isEmpty()) { - if (getHttpClient().isRemoveIdleDestinations() && connectionPool.isEmpty()) - { - // There is a race condition between this thread removing the destination - // and another thread queueing a request to this same destination. - // If this destination is removed, but the request queued, a new connection - // will be opened, the exchange will be executed and eventually the connection - // will idle timeout and be closed. Meanwhile a new destination will be created - // in HttpClient and will be used for other requests. - getHttpClient().removeDestination(this); - } + removeDestinationIfIdle(); } else { @@ -237,6 +228,27 @@ public abstract class PoolingHttpDestination extends HttpD connectionPool.close(); } + public void abort(Throwable cause) + { + super.abort(cause); + if (getHttpExchanges().isEmpty()) { + removeDestinationIfIdle(); + } + } + + private void removeDestinationIfIdle() { + if (getHttpClient().isRemoveIdleDestinations() && connectionPool.isEmpty()) + { + // There is a race condition between this thread removing the destination + // and another thread queueing a request to this same destination. + // If this destination is removed, but the request queued, a new connection + // will be opened, the exchange will be executed and eventually the connection + // will idle timeout and be closed. Meanwhile a new destination will be created + // in HttpClient and will be used for other requests. + getHttpClient().removeDestination(this); + } + } + @Override public String toString() { diff --git a/jetty-client/src/test/java/org/eclipse/jetty/client/http/HttpDestinationOverHTTPTest.java b/jetty-client/src/test/java/org/eclipse/jetty/client/http/HttpDestinationOverHTTPTest.java index 9720ce97917..6929be4bac4 100644 --- a/jetty-client/src/test/java/org/eclipse/jetty/client/http/HttpDestinationOverHTTPTest.java +++ b/jetty-client/src/test/java/org/eclipse/jetty/client/http/HttpDestinationOverHTTPTest.java @@ -39,6 +39,7 @@ import org.eclipse.jetty.http.HttpHeaderValue; import org.eclipse.jetty.util.ssl.SslContextFactory; import org.hamcrest.Matchers; import org.junit.Assert; +import org.junit.Assume; import org.junit.Before; import org.junit.Test; @@ -280,6 +281,31 @@ public class HttpDestinationOverHTTPTest extends AbstractHttpClientServerTest Assert.assertNotSame(destinationBefore, destinationAfter); } + @Test + public void testDestinationIsRemovedAfterConnectionError() throws Exception + { + String host = "localhost"; + int port = connector.getLocalPort(); + client.setRemoveIdleDestinations(true); + Assume.assumeTrue("Destinations of a fresh client must be empty", client.getDestinations().isEmpty()); + + server.stop(); + Request request = client.newRequest(host, port).scheme(this.scheme); + try + { + request.send(); + Assume.assumeTrue("Request to a closed port must fail", false); + } catch (Exception ignored) { + } + + long deadline = System.currentTimeMillis() + 100; + while (!client.getDestinations().isEmpty() && System.currentTimeMillis() < deadline) + { + Thread.yield(); + } + Assert.assertTrue("Destination must be removed after connection error", client.getDestinations().isEmpty()); + } + private Connection timedPoll(Queue connections, long time, TimeUnit unit) throws InterruptedException { long start = System.nanoTime(); From a458bfaaf4ecde2d02416b8c9a26ea6c0fb7e20d Mon Sep 17 00:00:00 2001 From: Ivanov Anton Date: Sun, 26 Aug 2018 21:34:20 +0300 Subject: [PATCH 3/6] Issue #2860 fixes after review --- .../jetty/client/PoolingHttpDestination.java | 10 +++++----- .../client/http/HttpDestinationOverHTTPTest.java | 14 +++++++------- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/jetty-client/src/main/java/org/eclipse/jetty/client/PoolingHttpDestination.java b/jetty-client/src/main/java/org/eclipse/jetty/client/PoolingHttpDestination.java index 9e61f404046..1fbc1eba9e5 100644 --- a/jetty-client/src/main/java/org/eclipse/jetty/client/PoolingHttpDestination.java +++ b/jetty-client/src/main/java/org/eclipse/jetty/client/PoolingHttpDestination.java @@ -210,7 +210,7 @@ public abstract class PoolingHttpDestination extends HttpD if (getHttpExchanges().isEmpty()) { - removeDestinationIfIdle(); + tryRemoveIdleDestination(); } else { @@ -231,12 +231,12 @@ public abstract class PoolingHttpDestination extends HttpD public void abort(Throwable cause) { super.abort(cause); - if (getHttpExchanges().isEmpty()) { - removeDestinationIfIdle(); - } + if (getHttpExchanges().isEmpty()) + tryRemoveIdleDestination(); } - private void removeDestinationIfIdle() { + private void tryRemoveIdleDestination() + { if (getHttpClient().isRemoveIdleDestinations() && connectionPool.isEmpty()) { // There is a race condition between this thread removing the destination diff --git a/jetty-client/src/test/java/org/eclipse/jetty/client/http/HttpDestinationOverHTTPTest.java b/jetty-client/src/test/java/org/eclipse/jetty/client/http/HttpDestinationOverHTTPTest.java index 6929be4bac4..a1ea9f6f46c 100644 --- a/jetty-client/src/test/java/org/eclipse/jetty/client/http/HttpDestinationOverHTTPTest.java +++ b/jetty-client/src/test/java/org/eclipse/jetty/client/http/HttpDestinationOverHTTPTest.java @@ -39,7 +39,6 @@ import org.eclipse.jetty.http.HttpHeaderValue; import org.eclipse.jetty.util.ssl.SslContextFactory; import org.hamcrest.Matchers; import org.junit.Assert; -import org.junit.Assume; import org.junit.Before; import org.junit.Test; @@ -287,21 +286,22 @@ public class HttpDestinationOverHTTPTest extends AbstractHttpClientServerTest String host = "localhost"; int port = connector.getLocalPort(); client.setRemoveIdleDestinations(true); - Assume.assumeTrue("Destinations of a fresh client must be empty", client.getDestinations().isEmpty()); + Assert.assertTrue("Destinations of a fresh client must be empty", client.getDestinations().isEmpty()); server.stop(); Request request = client.newRequest(host, port).scheme(this.scheme); try { request.send(); - Assume.assumeTrue("Request to a closed port must fail", false); - } catch (Exception ignored) { + Assert.fail("Request to a closed port must fail"); + } catch (Exception ignored) + { } - long deadline = System.currentTimeMillis() + 100; - while (!client.getDestinations().isEmpty() && System.currentTimeMillis() < deadline) + long deadline = System.nanoTime() + TimeUnit.MILLISECONDS.toNanos(100); + while (!client.getDestinations().isEmpty() && System.nanoTime() < deadline) { - Thread.yield(); + Thread.sleep(10); } Assert.assertTrue("Destination must be removed after connection error", client.getDestinations().isEmpty()); } From 3d87265a0a94758bd0e97f51b894c8f83bfc4ceb Mon Sep 17 00:00:00 2001 From: Ivanov Anton Date: Sun, 26 Aug 2018 21:37:50 +0300 Subject: [PATCH 4/6] Issue #2860 fix after review --- .../eclipse/jetty/client/http/HttpDestinationOverHTTPTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/jetty-client/src/test/java/org/eclipse/jetty/client/http/HttpDestinationOverHTTPTest.java b/jetty-client/src/test/java/org/eclipse/jetty/client/http/HttpDestinationOverHTTPTest.java index a1ea9f6f46c..a068998bef8 100644 --- a/jetty-client/src/test/java/org/eclipse/jetty/client/http/HttpDestinationOverHTTPTest.java +++ b/jetty-client/src/test/java/org/eclipse/jetty/client/http/HttpDestinationOverHTTPTest.java @@ -294,7 +294,7 @@ public class HttpDestinationOverHTTPTest extends AbstractHttpClientServerTest { request.send(); Assert.fail("Request to a closed port must fail"); - } catch (Exception ignored) + } catch (Exception expected) { } From 03c0d58727a4555aab419577bbf0e25ae0c16211 Mon Sep 17 00:00:00 2001 From: Ivanov Anton Date: Mon, 27 Aug 2018 08:24:26 +0300 Subject: [PATCH 5/6] Issue #2860 fix after review --- .../eclipse/jetty/client/http/HttpDestinationOverHTTPTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/jetty-client/src/test/java/org/eclipse/jetty/client/http/HttpDestinationOverHTTPTest.java b/jetty-client/src/test/java/org/eclipse/jetty/client/http/HttpDestinationOverHTTPTest.java index a068998bef8..9e345afc093 100644 --- a/jetty-client/src/test/java/org/eclipse/jetty/client/http/HttpDestinationOverHTTPTest.java +++ b/jetty-client/src/test/java/org/eclipse/jetty/client/http/HttpDestinationOverHTTPTest.java @@ -298,7 +298,7 @@ public class HttpDestinationOverHTTPTest extends AbstractHttpClientServerTest { } - long deadline = System.nanoTime() + TimeUnit.MILLISECONDS.toNanos(100); + long deadline = System.nanoTime() + TimeUnit.SECONDS.toNanos(1); while (!client.getDestinations().isEmpty() && System.nanoTime() < deadline) { Thread.sleep(10); From 6211564f797ee2a09b8216c9aa0990f913bfa475 Mon Sep 17 00:00:00 2001 From: Ivanov Anton Date: Mon, 27 Aug 2018 08:28:07 +0300 Subject: [PATCH 6/6] Issue #2860 fix after review --- .../eclipse/jetty/client/http/HttpDestinationOverHTTPTest.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/jetty-client/src/test/java/org/eclipse/jetty/client/http/HttpDestinationOverHTTPTest.java b/jetty-client/src/test/java/org/eclipse/jetty/client/http/HttpDestinationOverHTTPTest.java index 9e345afc093..58a32f4f636 100644 --- a/jetty-client/src/test/java/org/eclipse/jetty/client/http/HttpDestinationOverHTTPTest.java +++ b/jetty-client/src/test/java/org/eclipse/jetty/client/http/HttpDestinationOverHTTPTest.java @@ -294,7 +294,8 @@ public class HttpDestinationOverHTTPTest extends AbstractHttpClientServerTest { request.send(); Assert.fail("Request to a closed port must fail"); - } catch (Exception expected) + } + catch (Exception expected) { }