From da39f93124a529332b6cc71ae99147d9d8f2f889 Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Mon, 19 Dec 2022 22:36:06 +1100 Subject: [PATCH 01/22] Issue #9066 - parse multipart form data when getParameters is called Signed-off-by: Lachlan Roberts --- .../ee10/servlet/ServletContextRequest.java | 110 ++++++++++++++++-- 1 file changed, 99 insertions(+), 11 deletions(-) diff --git a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletContextRequest.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletContextRequest.java index fcbcf0b4fe6..72257be2de9 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletContextRequest.java +++ b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletContextRequest.java @@ -14,10 +14,13 @@ package org.eclipse.jetty.ee10.servlet; import java.io.BufferedReader; +import java.io.ByteArrayOutputStream; import java.io.IOException; +import java.io.InputStream; import java.io.InputStreamReader; import java.io.UnsupportedEncodingException; import java.nio.charset.Charset; +import java.nio.charset.StandardCharsets; import java.nio.charset.UnsupportedCharsetException; import java.security.Principal; import java.util.ArrayList; @@ -71,6 +74,7 @@ import org.eclipse.jetty.http.HttpVersion; import org.eclipse.jetty.http.MimeTypes; import org.eclipse.jetty.http.pathmap.MatchedPath; import org.eclipse.jetty.http.pathmap.PathSpec; +import org.eclipse.jetty.io.RuntimeIOException; import org.eclipse.jetty.server.ConnectionMetaData; import org.eclipse.jetty.server.FormFields; import org.eclipse.jetty.server.Request; @@ -81,6 +85,7 @@ import org.eclipse.jetty.session.Session; import org.eclipse.jetty.session.SessionManager; import org.eclipse.jetty.util.Fields; import org.eclipse.jetty.util.HostPort; +import org.eclipse.jetty.util.IO; import org.eclipse.jetty.util.StringUtil; import org.eclipse.jetty.util.URIUtil; import org.slf4j.Logger; @@ -775,7 +780,64 @@ public class ServletContextRequest extends ContextRequest if (contentType == null || !MimeTypes.Type.MULTIPART_FORM_DATA.is(HttpField.valueParameters(contentType, null))) throw new ServletException("Unsupported Content-Type [%s], expected [%s]".formatted(contentType, MimeTypes.Type.MULTIPART_FORM_DATA.asString())); if (_parts == null) + { _parts = ServletMultiPartFormData.from(this); + Collection parts = _parts.getParts(); + + String formCharset = null; + Part charsetPart = _parts.getPart("_charset_"); + if (charsetPart != null) + { + try (InputStream is = charsetPart.getInputStream()) + { + ByteArrayOutputStream os = new ByteArrayOutputStream(); + IO.copy(is, os); + formCharset = os.toString(StandardCharsets.UTF_8); + } + } + + /* + Select Charset to use for this part. (NOTE: charset behavior is for the part value only and not the part header/field names) + 1. Use the part specific charset as provided in that part's Content-Type header; else + 2. Use the overall default charset. Determined by: + a. if part name _charset_ exists, use that part's value. + b. if the request.getCharacterEncoding() returns a value, use that. + (note, this can be either from the charset field on the request Content-Type + header, or from a manual call to request.setCharacterEncoding()) + c. use utf-8. + */ + Charset defaultCharset; + if (formCharset != null) + defaultCharset = Charset.forName(formCharset); + else if (getCharacterEncoding() != null) + defaultCharset = Charset.forName(getCharacterEncoding()); + else + defaultCharset = StandardCharsets.UTF_8; + + ByteArrayOutputStream os = null; + for (Part p : parts) + { + if (p.getSubmittedFileName() == null) + { + // Servlet Spec 3.0 pg 23, parts without filename must be put into params. + String charset = null; + if (p.getContentType() != null) + charset = MimeTypes.getCharsetFromContentType(p.getContentType()); + + try (InputStream is = p.getInputStream()) + { + if (os == null) + os = new ByteArrayOutputStream(); + IO.copy(is, os); + + String content = os.toString(charset == null ? defaultCharset : Charset.forName(charset)); + _contentParameters.add(p.getName(), content); + } + os.reset(); + } + } + } + return _parts.getParts(); } @@ -1076,18 +1138,44 @@ public class ServletContextRequest extends ContextRequest // by a processing happening after a form-based authentication. if (_contentParameters == null) { - try + + int contentLength = getContentLength(); + if (contentLength != 0 && _inputState == INPUT_NONE) { - int maxKeys = getServletRequestState().getContextHandler().getMaxFormKeys(); - int maxContentSize = getServletRequestState().getContextHandler().getMaxFormContentSize(); - _contentParameters = FormFields.from(getRequest(), maxKeys, maxContentSize).get(); - if (_contentParameters == null || _contentParameters.isEmpty()) - _contentParameters = NO_PARAMS; - } - catch (IllegalStateException | IllegalArgumentException | ExecutionException | InterruptedException e) - { - LOG.warn(e.toString()); - throw new BadMessageException("Unable to parse form content", e); + String baseType = HttpField.valueParameters(getContentType(), null); + if (MimeTypes.Type.FORM_ENCODED.is(baseType) && + getConnectionMetaData().getHttpConfiguration().isFormEncodedMethod(getMethod())) + { + try + { + int maxKeys = getServletRequestState().getContextHandler().getMaxFormKeys(); + int maxContentSize = getServletRequestState().getContextHandler().getMaxFormContentSize(); + _contentParameters = FormFields.from(getRequest(), maxKeys, maxContentSize).get(); + if (_contentParameters == null || _contentParameters.isEmpty()) + _contentParameters = NO_PARAMS; + } + catch (IllegalStateException | IllegalArgumentException | ExecutionException | + InterruptedException e) + { + LOG.warn(e.toString()); + throw new BadMessageException("Unable to parse form content", e); + } + } + else if (MimeTypes.Type.MULTIPART_FORM_DATA.is(baseType) && + getAttribute(__MULTIPART_CONFIG_ELEMENT) != null) + { + try + { + getParts(); + } + catch (IOException | ServletException e) + { + String msg = "Unable to extract content parameters"; + if (LOG.isDebugEnabled()) + LOG.debug(msg, e); + throw new RuntimeIOException(msg, e); + } + } } } } From 04328172cb796a04686fd75e4e6e5d7f283ae25a Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Tue, 20 Dec 2022 21:45:18 +1100 Subject: [PATCH 02/22] add test to reproduce some issue with multipart Signed-off-by: Lachlan Roberts --- .../ee10/servlet/MultiPartServletTest.java | 37 +++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/MultiPartServletTest.java b/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/MultiPartServletTest.java index 73b29b5102f..24ae531e235 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/MultiPartServletTest.java +++ b/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/MultiPartServletTest.java @@ -54,6 +54,7 @@ import org.eclipse.jetty.server.handler.gzip.GzipHandler; import org.eclipse.jetty.util.IO; import org.eclipse.jetty.util.component.LifeCycle; import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; import static java.nio.charset.StandardCharsets.UTF_8; @@ -245,4 +246,40 @@ public class MultiPartServletTest assertThat(parts.size(), is(1)); assertThat(parts.get(0).getContentAsString(UTF_8), is(contentString)); } + + @Test + public void testDoubleReadFromPart() throws Exception + { + start(new HttpServlet() + { + @Override + protected void doPost(HttpServletRequest req, HttpServletResponse resp) throws ServletException, IOException + { + resp.setContentType("text/plain"); + for (Part part : req.getParts()) + { + resp.getWriter().println("Part: name=" + part.getName() + ", size=" + part.getSize() + ", content=" + IO.toString(part.getInputStream())); + resp.getWriter().println("Part: name=" + part.getName() + ", size=" + part.getSize() + ", content=" + IO.toString(part.getInputStream())); + } + } + }); + + String contentString = "the quick brown fox jumps over the lazy dog, " + + "the quick brown fox jumps over the lazy dog"; + StringRequestContent content = new StringRequestContent(contentString); + MultiPartRequestContent multiPart = new MultiPartRequestContent(); + multiPart.addPart(new MultiPart.ContentSourcePart("myPart", null, HttpFields.EMPTY, content)); + multiPart.close(); + + ContentResponse response = client.newRequest("localhost", connector.getLocalPort()) + .scheme(HttpScheme.HTTP.asString()) + .method(HttpMethod.POST) + .body(multiPart) + .send(); + + assertEquals(200, response.getStatus()); + System.err.println(response.getContentAsString()); + // TODO: create proper asserts + Assertions.fail(); + } } From 1ff251c9b303bbab8dff9d2e94e4b50844f6dda5 Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Thu, 12 Jan 2023 19:20:33 +1100 Subject: [PATCH 03/22] add more tests for multipart Signed-off-by: Lachlan Roberts --- .../ee10/servlet/MultiPartServletTest.java | 152 +++++++++++++++++- .../ee9/servlet/MultiPartServletTest.java | 119 ++++++++++++++ 2 files changed, 267 insertions(+), 4 deletions(-) diff --git a/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/MultiPartServletTest.java b/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/MultiPartServletTest.java index 24ae531e235..d2aa5b8b617 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/MultiPartServletTest.java +++ b/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/MultiPartServletTest.java @@ -37,6 +37,7 @@ import org.eclipse.jetty.client.api.Response; import org.eclipse.jetty.client.util.BytesRequestContent; import org.eclipse.jetty.client.util.InputStreamResponseListener; import org.eclipse.jetty.client.util.MultiPartRequestContent; +import org.eclipse.jetty.client.util.OutputStreamRequestContent; import org.eclipse.jetty.client.util.StringRequestContent; import org.eclipse.jetty.http.HttpFields; import org.eclipse.jetty.http.HttpHeader; @@ -55,11 +56,13 @@ import org.eclipse.jetty.util.IO; import org.eclipse.jetty.util.component.LifeCycle; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import static java.nio.charset.StandardCharsets.UTF_8; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.startsWith; import static org.junit.jupiter.api.Assertions.assertEquals; @@ -73,18 +76,26 @@ public class MultiPartServletTest private ServerConnector connector; private HttpClient client; private Path tmpDir; + private String tmpDirString; + + @BeforeEach + public void before() throws Exception + { + tmpDir = Files.createTempDirectory(MultiPartServletTest.class.getSimpleName()); + tmpDirString = tmpDir.toAbsolutePath().toString(); + } private void start(HttpServlet servlet) throws Exception { - tmpDir = Files.createTempDirectory(MultiPartServletTest.class.getSimpleName()); + start(servlet, new MultipartConfigElement(tmpDirString, MAX_FILE_SIZE, -1, 0)); + } + private void start(HttpServlet servlet, MultipartConfigElement config) throws Exception + { server = new Server(); connector = new ServerConnector(server); server.addConnector(connector); - MultipartConfigElement config = new MultipartConfigElement(tmpDir.toAbsolutePath().toString(), - MAX_FILE_SIZE, -1, 0); - ServletContextHandler contextHandler = new ServletContextHandler(server, "/"); ServletHolder servletHolder = new ServletHolder(servlet); servletHolder.getRegistration().setMultipartConfig(config); @@ -110,6 +121,139 @@ public class MultiPartServletTest IO.delete(tmpDir.toFile()); } + @Test + public void testLargePart() throws Exception + { + start(new HttpServlet() + { + @Override + protected void service(HttpServletRequest req, HttpServletResponse resp) throws ServletException, IOException + { + req.getParameterMap(); + req.getParts(); + resp.setStatus(200); + resp.getWriter().print("success"); + resp.getWriter().close(); + } + }, new MultipartConfigElement(tmpDirString)); + + OutputStreamRequestContent content = new OutputStreamRequestContent(); + MultiPartRequestContent multiPart = new MultiPartRequestContent(); + multiPart.addPart(new MultiPart.ContentSourcePart("param", null, null, content)); + multiPart.close(); + + InputStreamResponseListener listener = new InputStreamResponseListener(); + client.newRequest("localhost", connector.getLocalPort()) + .path("/defaultConfig") + .scheme(HttpScheme.HTTP.asString()) + .method(HttpMethod.POST) + .body(multiPart) + .send(listener); + + // Write large amount of content to the part. + byte[] byteArray = new byte[1024 * 1024]; + Arrays.fill(byteArray, (byte)1); + for (int i = 0; i < 1024 * 2; i++) + { + content.getOutputStream().write(byteArray); + } + content.close(); + + Response response = listener.get(2, TimeUnit.MINUTES); + assertThat(response.getStatus(), equalTo(HttpStatus.BAD_REQUEST_400)); + String responseContent = IO.toString(listener.getInputStream()); + assertThat(responseContent, containsString("Unable to parse form content")); + assertThat(responseContent, containsString("Form is larger than max length")); + } + + @Test + public void testManyParts() throws Exception + { + start(new HttpServlet() + { + @Override + protected void service(HttpServletRequest req, HttpServletResponse resp) throws ServletException, IOException + { + req.getParameterMap(); + req.getParts(); + resp.setStatus(200); + resp.getWriter().print("success"); + resp.getWriter().close(); + } + }, new MultipartConfigElement(tmpDirString)); + + byte[] byteArray = new byte[1024]; + Arrays.fill(byteArray, (byte)1); + + MultiPartRequestContent multiPart = new MultiPartRequestContent(); + for (int i = 0; i < 1024 * 1024; i++) + { + BytesRequestContent content = new BytesRequestContent(byteArray); + multiPart.addPart(new MultiPart.ContentSourcePart("part" + i, null, null, content)); + } + multiPart.close(); + + InputStreamResponseListener listener = new InputStreamResponseListener(); + client.newRequest("localhost", connector.getLocalPort()) + .path("/defaultConfig") + .scheme(HttpScheme.HTTP.asString()) + .method(HttpMethod.POST) + .body(multiPart) + .send(listener); + + Response response = listener.get(30, TimeUnit.SECONDS); + assertThat(response.getStatus(), equalTo(HttpStatus.BAD_REQUEST_400)); + String responseContent = IO.toString(listener.getInputStream()); + assertThat(responseContent, containsString("Unable to parse form content")); + assertThat(responseContent, containsString("Form with too many keys")); + } + + @Test + public void testMaxRequestSize() throws Exception + { + start(new HttpServlet() + { + @Override + protected void service(HttpServletRequest req, HttpServletResponse resp) throws ServletException, IOException + { + req.getParameterMap(); + req.getParts(); + resp.setStatus(200); + resp.getWriter().print("success"); + resp.getWriter().close(); + + } + }, new MultipartConfigElement(tmpDirString, -1, 1024, 1024 * 1024 * 8)); + + OutputStreamRequestContent content = new OutputStreamRequestContent(); + MultiPartRequestContent multiPart = new MultiPartRequestContent(); + multiPart.addPart(new MultiPart.ContentSourcePart("param", null, null, content)); + multiPart.close(); + + InputStreamResponseListener listener = new InputStreamResponseListener(); + client.newRequest("localhost", connector.getLocalPort()) + .path("/requestSizeLimit") + .scheme(HttpScheme.HTTP.asString()) + .method(HttpMethod.POST) + .body(multiPart) + .send(listener); + + // Write large amount of content to the part. + byte[] byteArray = new byte[1024 * 1024]; + Arrays.fill(byteArray, (byte)1); + for (int i = 0; i < 1024 * 2; i++) + { + content.getOutputStream().write(byteArray); + } + content.close(); + + Response response = listener.get(30, TimeUnit.SECONDS); + assertThat(response.getStatus(), equalTo(HttpStatus.BAD_REQUEST_400)); + String responseContent = IO.toString(listener.getInputStream()); + assertThat(responseContent, containsString("Unable to parse form content")); + assertThat(responseContent, containsString("max length exceeded")); + } + @Test public void testSimpleMultiPart() throws Exception { diff --git a/jetty-ee9/jetty-ee9-servlet/src/test/java/org/eclipse/jetty/ee9/servlet/MultiPartServletTest.java b/jetty-ee9/jetty-ee9-servlet/src/test/java/org/eclipse/jetty/ee9/servlet/MultiPartServletTest.java index 2e8dd49ee29..e7cd959d0cf 100644 --- a/jetty-ee9/jetty-ee9-servlet/src/test/java/org/eclipse/jetty/ee9/servlet/MultiPartServletTest.java +++ b/jetty-ee9/jetty-ee9-servlet/src/test/java/org/eclipse/jetty/ee9/servlet/MultiPartServletTest.java @@ -35,6 +35,7 @@ import org.eclipse.jetty.client.api.Response; import org.eclipse.jetty.client.util.BytesRequestContent; import org.eclipse.jetty.client.util.InputStreamResponseListener; import org.eclipse.jetty.client.util.MultiPartRequestContent; +import org.eclipse.jetty.client.util.OutputStreamRequestContent; import org.eclipse.jetty.client.util.StringRequestContent; import org.eclipse.jetty.ee9.nested.HttpChannel; import org.eclipse.jetty.ee9.nested.MultiPartFormInputStream; @@ -42,6 +43,7 @@ import org.eclipse.jetty.http.HttpFields; import org.eclipse.jetty.http.HttpHeader; import org.eclipse.jetty.http.HttpMethod; import org.eclipse.jetty.http.HttpScheme; +import org.eclipse.jetty.http.HttpStatus; import org.eclipse.jetty.http.MimeTypes; import org.eclipse.jetty.http.MultiPart; import org.eclipse.jetty.logging.StacklessLogging; @@ -55,6 +57,7 @@ import org.junit.jupiter.api.Test; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.startsWith; import static org.junit.jupiter.api.Assertions.assertEquals; @@ -69,6 +72,21 @@ public class MultiPartServletTest private static final int MAX_FILE_SIZE = 512 * 1024; private static final int LARGE_MESSAGE_SIZE = 1024 * 1024; + private static final int MAX_REQUEST_SIZE = 1024 * 1024 * 8; + + + public static class RequestParameterServlet extends HttpServlet + { + @Override + protected void doPost(HttpServletRequest req, HttpServletResponse resp) throws ServletException, IOException + { + req.getParameterMap(); + req.getParts(); + resp.setStatus(200); + resp.getWriter().print("success"); + resp.getWriter().close(); + } + } public static class MultiPartServlet extends HttpServlet { @@ -119,11 +137,19 @@ public class MultiPartServletTest MultipartConfigElement config = new MultipartConfigElement(tmpDir.toAbsolutePath().toString(), MAX_FILE_SIZE, -1, 1); + MultipartConfigElement requestSizedConfig = new MultipartConfigElement(tmpDir.toAbsolutePath().toString(), + -1, MAX_REQUEST_SIZE, 1); + MultipartConfigElement defaultConfig = new MultipartConfigElement(tmpDir.toAbsolutePath().toString(), + -1, -1, 1); ServletContextHandler contextHandler = new ServletContextHandler(ServletContextHandler.SESSIONS); contextHandler.setContextPath("/"); ServletHolder servletHolder = contextHandler.addServlet(MultiPartServlet.class, "/"); servletHolder.getRegistration().setMultipartConfig(config); + servletHolder = contextHandler.addServlet(RequestParameterServlet.class, "/defaultConfig"); + servletHolder.getRegistration().setMultipartConfig(defaultConfig); + servletHolder = contextHandler.addServlet(RequestParameterServlet.class, "/requestSizeLimit"); + servletHolder.getRegistration().setMultipartConfig(requestSizedConfig); servletHolder = contextHandler.addServlet(MultiPartEchoServlet.class, "/echo"); servletHolder.getRegistration().setMultipartConfig(config); @@ -149,6 +175,99 @@ public class MultiPartServletTest IO.delete(tmpDir.toFile()); } + @Test + public void testLargePart() throws Exception + { + OutputStreamRequestContent content = new OutputStreamRequestContent(); + MultiPartRequestContent multiPart = new MultiPartRequestContent(); + multiPart.addPart(new MultiPart.ContentSourcePart("param", null, null, content)); + multiPart.close(); + + InputStreamResponseListener listener = new InputStreamResponseListener(); + client.newRequest("localhost", connector.getLocalPort()) + .path("/defaultConfig") + .scheme(HttpScheme.HTTP.asString()) + .method(HttpMethod.POST) + .body(multiPart) + .send(listener); + + // Write large amount of content to the part. + byte[] byteArray = new byte[1024 * 1024]; + Arrays.fill(byteArray, (byte)1); + for (int i = 0; i < 1024 * 2; i++) + { + content.getOutputStream().write(byteArray); + } + content.close(); + + Response response = listener.get(2, TimeUnit.MINUTES); + assertThat(response.getStatus(), equalTo(HttpStatus.BAD_REQUEST_400)); + String responseContent = IO.toString(listener.getInputStream()); + assertThat(responseContent, containsString("Unable to parse form content")); + assertThat(responseContent, containsString("Form is larger than max length")); + } + + @Test + public void testManyParts() throws Exception + { + byte[] byteArray = new byte[1024]; + Arrays.fill(byteArray, (byte)1); + + MultiPartRequestContent multiPart = new MultiPartRequestContent(); + for (int i = 0; i < 1024 * 1024; i++) + { + BytesRequestContent content = new BytesRequestContent(byteArray); + multiPart.addPart(new MultiPart.ContentSourcePart("part" + i, null, null, content)); + } + multiPart.close(); + + InputStreamResponseListener listener = new InputStreamResponseListener(); + client.newRequest("localhost", connector.getLocalPort()) + .path("/defaultConfig") + .scheme(HttpScheme.HTTP.asString()) + .method(HttpMethod.POST) + .body(multiPart) + .send(listener); + + Response response = listener.get(30, TimeUnit.SECONDS); + assertThat(response.getStatus(), equalTo(HttpStatus.BAD_REQUEST_400)); + String responseContent = IO.toString(listener.getInputStream()); + assertThat(responseContent, containsString("Unable to parse form content")); + assertThat(responseContent, containsString("Form with too many keys")); + } + + @Test + public void testMaxRequestSize() throws Exception + { + OutputStreamRequestContent content = new OutputStreamRequestContent(); + MultiPartRequestContent multiPart = new MultiPartRequestContent(); + multiPart.addPart(new MultiPart.ContentSourcePart("param", null, null, content)); + multiPart.close(); + + InputStreamResponseListener listener = new InputStreamResponseListener(); + client.newRequest("localhost", connector.getLocalPort()) + .path("/requestSizeLimit") + .scheme(HttpScheme.HTTP.asString()) + .method(HttpMethod.POST) + .body(multiPart) + .send(listener); + + // Write large amount of content to the part. + byte[] byteArray = new byte[1024 * 1024]; + Arrays.fill(byteArray, (byte)1); + for (int i = 0; i < 1024 * 2; i++) + { + content.getOutputStream().write(byteArray); + } + content.close(); + + Response response = listener.get(30, TimeUnit.SECONDS); + assertThat(response.getStatus(), equalTo(HttpStatus.BAD_REQUEST_400)); + String responseContent = IO.toString(listener.getInputStream()); + assertThat(responseContent, containsString("Unable to parse form content")); + assertThat(responseContent, containsString("Request exceeds maxRequestSize")); + } + @Test public void testTempFilesDeletedOnError() throws Exception { From 02f9cc0eb3e91f16f6c1a2cfc19c4769d24dd048 Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Thu, 12 Jan 2023 19:21:00 +1100 Subject: [PATCH 04/22] fixes to pass new multipart test cases Signed-off-by: Lachlan Roberts --- .../org/eclipse/jetty/http/MultiPart.java | 70 ++------------ .../eclipse/jetty/http/MultiPartFormData.java | 26 +++++ .../ee10/servlet/ServletContextRequest.java | 95 ++++++++++++------- .../servlet/ServletMultiPartFormData.java | 27 ++++-- .../ee10/servlet/ServletRequestState.java | 2 +- .../ee9/nested/MultiPartFormInputStream.java | 19 +++- .../org/eclipse/jetty/ee9/nested/Request.java | 26 ++++- 7 files changed, 154 insertions(+), 111 deletions(-) diff --git a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPart.java b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPart.java index 358ac4e329a..301ee3b3a33 100644 --- a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPart.java +++ b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPart.java @@ -1263,93 +1263,37 @@ public class MultiPart private void notifyPartBegin() { - try - { - listener.onPartBegin(); - } - catch (Throwable x) - { - if (LOG.isDebugEnabled()) - LOG.debug("failure while notifying listener {}", listener, x); - } + listener.onPartBegin(); } private void notifyPartHeader(String name, String value) { - try - { - listener.onPartHeader(name, value); - } - catch (Throwable x) - { - if (LOG.isDebugEnabled()) - LOG.debug("failure while notifying listener {}", listener, x); - } + listener.onPartHeader(name, value); } private void notifyPartHeaders() { - try - { - listener.onPartHeaders(); - } - catch (Throwable x) - { - if (LOG.isDebugEnabled()) - LOG.debug("failure while notifying listener {}", listener, x); - } + listener.onPartHeaders(); } private void notifyPartContent(Content.Chunk chunk) { - try - { - listener.onPartContent(chunk); - } - catch (Throwable x) - { - if (LOG.isDebugEnabled()) - LOG.debug("failure while notifying listener {}", listener, x); - } + listener.onPartContent(chunk); } private void notifyPartEnd() { - try - { - listener.onPartEnd(); - } - catch (Throwable x) - { - if (LOG.isDebugEnabled()) - LOG.debug("failure while notifying listener {}", listener, x); - } + listener.onPartEnd(); } private void notifyComplete() { - try - { - listener.onComplete(); - } - catch (Throwable x) - { - if (LOG.isDebugEnabled()) - LOG.debug("failure while notifying listener {}", listener, x); - } + listener.onComplete(); } private void notifyFailure(Throwable failure) { - try - { - listener.onFailure(failure); - } - catch (Throwable x) - { - if (LOG.isDebugEnabled()) - LOG.debug("failure while notifying listener {}", listener, x); - } + listener.onFailure(failure); } /** diff --git a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartFormData.java b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartFormData.java index 6f2c1986843..2fc54c5b69d 100644 --- a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartFormData.java +++ b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartFormData.java @@ -79,6 +79,8 @@ public class MultiPartFormData extends CompletableFuture= maxParts) + throw new IllegalStateException(String.format("Form with too many keys [%d > %d]", numParts, maxParts)); + } + @Override public void onPart(String name, String fileName, HttpFields headers) { diff --git a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletContextRequest.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletContextRequest.java index 72257be2de9..bcf9173a88b 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletContextRequest.java +++ b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletContextRequest.java @@ -40,6 +40,7 @@ import java.util.concurrent.ExecutionException; import jakarta.servlet.AsyncContext; import jakarta.servlet.AsyncListener; import jakarta.servlet.DispatcherType; +import jakarta.servlet.MultipartConfigElement; import jakarta.servlet.RequestDispatcher; import jakarta.servlet.ServletConnection; import jakarta.servlet.ServletContext; @@ -776,12 +777,21 @@ public class ServletContextRequest extends ContextRequest @Override public Collection getParts() throws IOException, ServletException { - String contentType = getContentType(); - if (contentType == null || !MimeTypes.Type.MULTIPART_FORM_DATA.is(HttpField.valueParameters(contentType, null))) - throw new ServletException("Unsupported Content-Type [%s], expected [%s]".formatted(contentType, MimeTypes.Type.MULTIPART_FORM_DATA.asString())); if (_parts == null) { - _parts = ServletMultiPartFormData.from(this); + String contentType = getContentType(); + if (contentType == null || !MimeTypes.Type.MULTIPART_FORM_DATA.is(HttpField.valueParameters(contentType, null))) + throw new ServletException("Unsupported Content-Type [%s], expected [%s]".formatted(contentType, MimeTypes.Type.MULTIPART_FORM_DATA.asString())); + + MultipartConfigElement config = (MultipartConfigElement)getAttribute(__MULTIPART_CONFIG_ELEMENT); + if (config == null) + throw new IllegalStateException("No multipart config for servlet"); + + ServletContextHandler contextHandler = getContext().getServletContextHandler(); + int maxFormContentSize = contextHandler.getMaxFormContentSize(); + int maxFormKeys = contextHandler.getMaxFormKeys(); + + _parts = ServletMultiPartFormData.from(this, maxFormKeys); Collection parts = _parts.getParts(); String formCharset = null; @@ -814,11 +824,16 @@ public class ServletContextRequest extends ContextRequest else defaultCharset = StandardCharsets.UTF_8; + long formContentSize = 0; ByteArrayOutputStream os = null; for (Part p : parts) { if (p.getSubmittedFileName() == null) { + formContentSize = Math.addExact(formContentSize, p.getSize()); + if (formContentSize > maxFormContentSize) + throw new IllegalStateException("Form is larger than max length " + maxFormContentSize); + // Servlet Spec 3.0 pg 23, parts without filename must be put into params. String charset = null; if (p.getContentType() != null) @@ -831,6 +846,8 @@ public class ServletContextRequest extends ContextRequest IO.copy(is, os); String content = os.toString(charset == null ? defaultCharset : Charset.forName(charset)); + if (_contentParameters == null) + _contentParameters = new Fields(); _contentParameters.add(p.getName(), content); } os.reset(); @@ -1138,44 +1155,52 @@ public class ServletContextRequest extends ContextRequest // by a processing happening after a form-based authentication. if (_contentParameters == null) { - - int contentLength = getContentLength(); - if (contentLength != 0 && _inputState == INPUT_NONE) + try { - String baseType = HttpField.valueParameters(getContentType(), null); - if (MimeTypes.Type.FORM_ENCODED.is(baseType) && - getConnectionMetaData().getHttpConfiguration().isFormEncodedMethod(getMethod())) + int contentLength = getContentLength(); + if (contentLength != 0 && _inputState == INPUT_NONE) { - try + String baseType = HttpField.valueParameters(getContentType(), null); + if (MimeTypes.Type.FORM_ENCODED.is(baseType) && + getConnectionMetaData().getHttpConfiguration().isFormEncodedMethod(getMethod())) { - int maxKeys = getServletRequestState().getContextHandler().getMaxFormKeys(); - int maxContentSize = getServletRequestState().getContextHandler().getMaxFormContentSize(); - _contentParameters = FormFields.from(getRequest(), maxKeys, maxContentSize).get(); - if (_contentParameters == null || _contentParameters.isEmpty()) - _contentParameters = NO_PARAMS; + try + { + int maxKeys = getServletRequestState().getContextHandler().getMaxFormKeys(); + int maxContentSize = getServletRequestState().getContextHandler().getMaxFormContentSize(); + _contentParameters = FormFields.from(getRequest(), maxKeys, maxContentSize).get(); + } + catch (IllegalStateException | IllegalArgumentException | ExecutionException | + InterruptedException e) + { + LOG.warn(e.toString()); + throw new BadMessageException("Unable to parse form content", e); + } } - catch (IllegalStateException | IllegalArgumentException | ExecutionException | - InterruptedException e) + else if (MimeTypes.Type.MULTIPART_FORM_DATA.is(baseType) && + getAttribute(__MULTIPART_CONFIG_ELEMENT) != null) { - LOG.warn(e.toString()); - throw new BadMessageException("Unable to parse form content", e); - } - } - else if (MimeTypes.Type.MULTIPART_FORM_DATA.is(baseType) && - getAttribute(__MULTIPART_CONFIG_ELEMENT) != null) - { - try - { - getParts(); - } - catch (IOException | ServletException e) - { - String msg = "Unable to extract content parameters"; - if (LOG.isDebugEnabled()) - LOG.debug(msg, e); - throw new RuntimeIOException(msg, e); + try + { + getParts(); + } + catch (IOException | ServletException e) + { + String msg = "Unable to extract content parameters"; + if (LOG.isDebugEnabled()) + LOG.debug(msg, e); + throw new RuntimeIOException(msg, e); + } } } + + if (_contentParameters == null || _contentParameters.isEmpty()) + _contentParameters = NO_PARAMS; + } + catch (IllegalStateException | IllegalArgumentException e) + { + LOG.warn(e.toString()); + throw new BadMessageException("Unable to parse form content", e); } } } diff --git a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletMultiPartFormData.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletMultiPartFormData.java index f6634997c74..6ccab6e7e2f 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletMultiPartFormData.java +++ b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletMultiPartFormData.java @@ -56,6 +56,21 @@ public class ServletMultiPartFormData * @see org.eclipse.jetty.server.handler.DelayedHandler */ public static Parts from(ServletContextRequest.ServletApiRequest request) throws IOException + { + return from(request, ServletContextHandler.DEFAULT_MAX_FORM_KEYS); + } + + /** + *

Parses the request content assuming it is a multipart content, + * and returns a {@link Parts} objects that can be used to access + * individual {@link Part}s.

+ * + * @param request the HTTP request with multipart content + * @return a {@link Parts} object to access the individual {@link Part}s + * @throws IOException if reading the request content fails + * @see org.eclipse.jetty.server.handler.DelayedHandler + */ + public static Parts from(ServletContextRequest.ServletApiRequest request, int maxParts) throws IOException { try { @@ -65,7 +80,7 @@ public class ServletMultiPartFormData return new Parts(formData); // TODO set the files directory - return new ServletMultiPartFormData().parse(request); + return new ServletMultiPartFormData().parse(request, maxParts); } catch (Throwable x) { @@ -73,7 +88,7 @@ public class ServletMultiPartFormData } } - private Parts parse(ServletContextRequest.ServletApiRequest request) throws IOException + private Parts parse(ServletContextRequest.ServletApiRequest request, int maxParts) throws IOException { MultipartConfigElement config = (MultipartConfigElement)request.getAttribute(ServletContextRequest.__MULTIPART_CONFIG_ELEMENT); if (config == null) @@ -84,6 +99,7 @@ public class ServletMultiPartFormData throw new IllegalStateException("No multipart boundary parameter in Content-Type"); MultiPartFormData formData = new MultiPartFormData(boundary); + formData.setMaxParts(maxParts); File tmpDirFile = (File)request.getServletContext().getAttribute(ServletContext.TEMPDIR); if (tmpDirFile == null) @@ -148,21 +164,18 @@ public class ServletMultiPartFormData private final MultiPartFormData _formData; private final MultiPart.Part _part; private final long _length; - private final InputStream _input; private ServletPart(MultiPartFormData formData, MultiPart.Part part) { _formData = formData; _part = part; - Content.Source content = part.getContent(); - _length = content.getLength(); - _input = Content.Source.asInputStream(content); + _length = _part.getContent().getLength(); } @Override public InputStream getInputStream() throws IOException { - return _input; + return Content.Source.asInputStream(_part.getContent()); } @Override diff --git a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletRequestState.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletRequestState.java index 42eda0c5b03..ebebb146e4a 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletRequestState.java +++ b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletRequestState.java @@ -851,7 +851,7 @@ public class ServletRequestState request.setAttribute(ERROR_EXCEPTION_TYPE, th.getClass()); // Set Jetty specific attributes. - request.setAttribute(ErrorProcessor.ERROR_EXCEPTION, null); + request.setAttribute(ErrorProcessor.ERROR_EXCEPTION, th); // Ensure any async lifecycle is ended! _requestState = RequestState.BLOCKING; diff --git a/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/MultiPartFormInputStream.java b/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/MultiPartFormInputStream.java index 9c8ba258a38..f264a799408 100644 --- a/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/MultiPartFormInputStream.java +++ b/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/MultiPartFormInputStream.java @@ -97,6 +97,8 @@ public class MultiPartFormInputStream private final MultipartConfigElement _config; private final File _contextTmpDir; private final String _contentType; + private final int _maxParts; + private int _numParts = 0; private volatile Throwable _err; private volatile Path _tmpDir; private volatile boolean _deleteOnExit; @@ -380,9 +382,20 @@ public class MultiPartFormInputStream * @param in Request input stream * @param contentType Content-Type header * @param config MultipartConfigElement - * @param contextTmpDir jakarta.servlet.context.tempdir + * @param contextTmpDir javax.servlet.context.tempdir */ public MultiPartFormInputStream(InputStream in, String contentType, MultipartConfigElement config, File contextTmpDir) + { + this(in, contentType, config, contextTmpDir, org.eclipse.jetty.ee9.nested.ContextHandler.DEFAULT_MAX_FORM_KEYS); + } + + /** + * @param in Request input stream + * @param contentType Content-Type header + * @param config MultipartConfigElement + * @param contextTmpDir javax.servlet.context.tempdir + */ + public MultiPartFormInputStream(InputStream in, String contentType, MultipartConfigElement config, File contextTmpDir, int maxParts) { // Must be a multipart request. _contentType = contentType; @@ -391,6 +404,7 @@ public class MultiPartFormInputStream _contextTmpDir = (contextTmpDir != null) ? contextTmpDir : new File(System.getProperty("java.io.tmpdir")); _config = (config != null) ? config : new MultipartConfigElement(_contextTmpDir.getAbsolutePath()); + _maxParts = maxParts; if (in instanceof ServletInputStream) { @@ -809,6 +823,9 @@ public class MultiPartFormInputStream public void startPart() { reset(); + _numParts++; + if (_numParts >= _maxParts) + throw new IllegalStateException(String.format("Form with too many keys [%d > %d]", _numParts, _maxParts)); } @Override diff --git a/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/Request.java b/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/Request.java index e63daeaccf0..8e0d36ee1c9 100644 --- a/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/Request.java +++ b/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/Request.java @@ -2030,9 +2030,22 @@ public class Request implements HttpServletRequest if (config == null) throw new IllegalStateException("No multipart config for servlet"); - _multiParts = newMultiParts(config); + int maxFormContentSize = ContextHandler.DEFAULT_MAX_FORM_CONTENT_SIZE; + int maxFormKeys = ContextHandler.DEFAULT_MAX_FORM_KEYS; + if (_context != null) + { + ContextHandler contextHandler = _context.getContextHandler(); + maxFormContentSize = contextHandler.getMaxFormContentSize(); + maxFormKeys = contextHandler.getMaxFormKeys(); + } + else + { + maxFormContentSize = lookupServerAttribute(ContextHandler.MAX_FORM_CONTENT_SIZE_KEY, maxFormContentSize); + maxFormKeys = lookupServerAttribute(ContextHandler.MAX_FORM_KEYS_KEY, maxFormKeys); + } + + _multiParts = newMultiParts(config, maxFormKeys); Collection parts = _multiParts.getParts(); - setNonComplianceViolationsOnRequest(); String formCharset = null; Part charsetPart = _multiParts.getPart("_charset_"); @@ -2064,11 +2077,16 @@ public class Request implements HttpServletRequest else defaultCharset = StandardCharsets.UTF_8; + long formContentSize = 0; ByteArrayOutputStream os = null; for (Part p : parts) { if (p.getSubmittedFileName() == null) { + formContentSize = Math.addExact(formContentSize, p.getSize()); + if (formContentSize > maxFormContentSize) + throw new IllegalStateException("Form is larger than max length " + maxFormContentSize); + // Servlet Spec 3.0 pg 23, parts without filename must be put into params. String charset = null; if (p.getContentType() != null) @@ -2109,10 +2127,10 @@ public class Request implements HttpServletRequest setAttribute(HttpCompliance.VIOLATIONS_ATTR, violations); } - private MultiPartFormInputStream newMultiParts(MultipartConfigElement config) throws IOException + private MultiPartFormInputStream newMultiParts(MultipartConfigElement config, int maxParts) throws IOException { return new MultiPartFormInputStream(getInputStream(), getContentType(), config, - (_context != null ? (File)_context.getAttribute("jakarta.servlet.context.tempdir") : null)); + (_context != null ? (File)_context.getAttribute("jakarta.servlet.context.tempdir") : null), maxParts); } @Override From 3816e4b98ad49d08273b1fdd9124e9e0e4125e5a Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Mon, 16 Jan 2023 11:52:04 +1100 Subject: [PATCH 05/22] fix multipart test cases, add more api to the core MultiPart class Signed-off-by: Lachlan Roberts --- .../org/eclipse/jetty/http/MultiPart.java | 134 +++++++++++++----- .../jetty/http/MultiPartByteRanges.java | 25 +++- .../eclipse/jetty/http/MultiPartFormData.java | 15 +- .../jetty/io/content/ChunksContentSource.java | 9 -- .../server/internal/HttpChannelState.java | 23 +++ .../org/eclipse/jetty/util/BufferUtil.java | 37 +++++ .../servlet/ServletMultiPartFormData.java | 28 +++- .../ee10/servlet/MultiPartServletTest.java | 6 +- 8 files changed, 215 insertions(+), 62 deletions(-) diff --git a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPart.java b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPart.java index 301ee3b3a33..7ab7b392152 100644 --- a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPart.java +++ b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPart.java @@ -33,6 +33,7 @@ import java.util.Queue; import java.util.concurrent.ThreadLocalRandom; import org.eclipse.jetty.io.Content; +import org.eclipse.jetty.io.Retainable; import org.eclipse.jetty.io.content.ByteBufferContentSource; import org.eclipse.jetty.io.content.ChunksContentSource; import org.eclipse.jetty.io.content.PathContentSource; @@ -65,6 +66,7 @@ import static java.nio.charset.StandardCharsets.UTF_8; */ public class MultiPart { + private static final Logger LOG = LoggerFactory.getLogger(MultiPart.class); private static final int MAX_BOUNDARY_LENGTH = 70; private MultiPart() @@ -122,6 +124,8 @@ public class MultiPart private final String name; private final String fileName; private final HttpFields fields; + protected Path path; + private boolean temporary = true; public Part(String name, String fileName, HttpFields fields) { @@ -169,6 +173,11 @@ public class MultiPart */ public abstract Content.Source getContent(); + public long getLength() + { + return getContent().getLength(); + } + /** *

Returns the content of this part as a string.

*

The charset used to decode the bytes is: @@ -207,6 +216,11 @@ public class MultiPart return fields; } + public Path getPath() + { + return path; + } + /** *

Writes the content of this part to the given path.

* @@ -215,10 +229,38 @@ public class MultiPart */ public void writeTo(Path path) throws IOException { - try (OutputStream out = Files.newOutputStream(path)) + + if (this.path == null) { - IO.copy(Content.Source.asInputStream(getContent()), out); + try (OutputStream out = Files.newOutputStream(path)) + { + IO.copy(Content.Source.asInputStream(getContent()), out); + } + this.path = path; } + else + { + this.path = Files.move(this.path, path, StandardCopyOption.REPLACE_EXISTING); + this.temporary = false; + } + } + + public void delete() throws IOException + { + if (this.path != null) + Files.delete(this.path); + } + + public void close(Throwable failure) throws IOException + { + // TODO: why do we need this, no one ever uses the failure?? + if (temporary) + delete(); + } + + public final void close() throws IOException + { + close(null); } } @@ -228,7 +270,7 @@ public class MultiPart */ public static class ByteBufferPart extends Part { - private final Content.Source content; + private final List content; private final long length; public ByteBufferPart(String name, String fileName, HttpFields fields, ByteBuffer... buffers) @@ -239,14 +281,20 @@ public class MultiPart public ByteBufferPart(String name, String fileName, HttpFields fields, List content) { super(name, fileName, fields); - this.content = new ByteBufferContentSource(content); this.length = content.stream().mapToLong(Buffer::remaining).sum(); + this.content = content; + } + + @Override + public long getLength() + { + return length; } @Override public Content.Source getContent() { - return content; + return new ByteBufferContentSource(content); } @Override @@ -267,20 +315,33 @@ public class MultiPart */ public static class ChunksPart extends Part { - private final Content.Source content; + private final List content; private final long length; public ChunksPart(String name, String fileName, HttpFields fields, List content) { super(name, fileName, fields); - this.content = new ChunksContentSource(content); + this.content = content; this.length = content.stream().mapToLong(c -> c.getByteBuffer().remaining()).sum(); } + @Override + public long getLength() + { + return length; + } + @Override public Content.Source getContent() { - return content; + return new ChunksContentSource(content.stream().map(Content.Chunk::slice).toList()); + } + + @Override + public void close(Throwable failure) throws IOException + { + super.close(failure); + content.forEach(Retainable::release); } @Override @@ -301,36 +362,18 @@ public class MultiPart */ public static class PathPart extends Part { - private final PathContentSource content; - public PathPart(String name, String fileName, HttpFields fields, Path path) { super(name, fileName, fields); - this.content = new PathContentSource(path); - } - - public Path getPath() - { - return content.getPath(); + this.path = path; } @Override - public Content.Source getContent() - { - return content; - } - - @Override - public void writeTo(Path path) throws IOException - { - Files.move(getPath(), path, StandardCopyOption.REPLACE_EXISTING); - } - - public void delete() + public long getLength() { try { - Files.delete(getPath()); + return Files.size(path); } catch (IOException x) { @@ -338,6 +381,12 @@ public class MultiPart } } + @Override + public Content.Source getContent() + { + return new PathContentSource(path); + } + @Override public String toString() { @@ -356,7 +405,7 @@ public class MultiPart */ public static class ContentSourcePart extends Part { - private final Content.Source content; + private Content.Source content; public ContentSourcePart(String name, String fileName, HttpFields fields, Content.Source content) { @@ -367,7 +416,9 @@ public class MultiPart @Override public Content.Source getContent() { - return content; + Content.Source c = content; + content = null; + return c; } @Override @@ -435,6 +486,7 @@ public class MultiPart private Runnable demand; private Content.Chunk.Error errorChunk; private Part part; + private Content.Source partContent; public AbstractContentSource(String boundary) { @@ -564,6 +616,7 @@ public class MultiPart else { part = parts.poll(); + partContent = part.getContent(); state = State.HEADERS; yield Content.Chunk.from(firstBoundary.slice(), false); } @@ -572,6 +625,7 @@ public class MultiPart case MIDDLE -> { part = null; + partContent = null; try (AutoLock ignored = lock.lock()) { if (parts.isEmpty()) @@ -589,6 +643,7 @@ public class MultiPart else { part = parts.poll(); + partContent = part.getContent(); state = State.HEADERS; yield Content.Chunk.from(middleBoundary.slice(), false); } @@ -628,7 +683,7 @@ public class MultiPart } case CONTENT -> { - Content.Chunk chunk = part.getContent().read(); + Content.Chunk chunk = partContent.read(); if (chunk == null || chunk instanceof Content.Chunk.Error) yield chunk; if (chunk.isLast()) @@ -674,7 +729,7 @@ public class MultiPart if (state == State.CONTENT) { - part.getContent().demand(() -> + partContent.demand(() -> { try (AutoLock ignoredAgain = lock.lock()) { @@ -706,7 +761,18 @@ public class MultiPart drained = List.copyOf(parts); parts.clear(); } - drained.forEach(part -> part.getContent().fail(failure)); + drained.forEach(part -> + { + try + { + part.close(failure); + } + catch (IOException e) + { + if (LOG.isDebugEnabled()) + LOG.debug("Error closing part", e); + } + }); invoker.run(this::invokeDemandCallback); } diff --git a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartByteRanges.java b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartByteRanges.java index ee210ce1eca..a843ae8e5af 100644 --- a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartByteRanges.java +++ b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartByteRanges.java @@ -24,6 +24,8 @@ import java.util.concurrent.CompletableFuture; import org.eclipse.jetty.io.Content; import org.eclipse.jetty.util.thread.AutoLock; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; /** *

A {@link CompletableFuture} that is completed when a multipart/byteranges @@ -54,6 +56,8 @@ import org.eclipse.jetty.util.thread.AutoLock; */ public class MultiPartByteRanges extends CompletableFuture { + private static final Logger LOG = LoggerFactory.getLogger(MultiPartByteRanges.class); + private final PartsListener listener = new PartsListener(); private final MultiPart.Parser parser; @@ -244,7 +248,8 @@ public class MultiPartByteRanges extends CompletableFuture(parts); parts.clear(); } - toFail.forEach(part -> part.getContent().fail(cause)); + toFail.forEach(part -> + { + try + { + part.close(cause); + } + catch (IOException e) + { + if (LOG.isDebugEnabled()) + LOG.debug("Error clsoing part", e); + } + }); } } } diff --git a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartFormData.java b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartFormData.java index 2fc54c5b69d..167ed3c8ca2 100644 --- a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartFormData.java +++ b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartFormData.java @@ -14,6 +14,7 @@ package org.eclipse.jetty.http; import java.io.Closeable; +import java.io.IOException; import java.nio.ByteBuffer; import java.nio.channels.NonWritableChannelException; import java.nio.channels.SeekableByteChannel; @@ -504,6 +505,7 @@ public class MultiPartFormData extends CompletableFuture= maxParts) throw new IllegalStateException(String.format("Form with too many keys [%d > %d]", numParts, maxParts)); @@ -565,10 +567,15 @@ public class MultiPartFormData extends CompletableFuture toFail = List.of(); try (AutoLock ignored = lock.lock()) { if (terminated != null) return; terminated = Content.Chunk.from(failure); - if (iterator != null) - { - toFail = new ArrayList<>(); - iterator.forEachRemaining(toFail::add); - } } - toFail.forEach(Content.Chunk::release); } } diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java index 188bd60a23b..c41046eeae6 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java @@ -34,6 +34,8 @@ import org.eclipse.jetty.http.HttpStatus; import org.eclipse.jetty.http.HttpURI; import org.eclipse.jetty.http.HttpVersion; import org.eclipse.jetty.http.MetaData; +import org.eclipse.jetty.http.MultiPart; +import org.eclipse.jetty.http.MultiPartFormData; import org.eclipse.jetty.http.PreEncodedHttpField; import org.eclipse.jetty.http.Trailers; import org.eclipse.jetty.http.UriCompliance; @@ -646,6 +648,27 @@ public class HttpChannelState implements HttpChannel, Components requestLog.log(_request.getLoggedRequest(), _request._response); } + + // Clean up any multipart tmp files and release any associated resources. + MultiPartFormData multiParts = (MultiPartFormData)_request.getAttribute(MultiPartFormData.class.getName()); + if (multiParts != null) + { + MultiPartFormData.Parts parts = multiParts.getNow(null); + if (parts != null) + { + for (MultiPart.Part p : parts) + { + try + { + p.close(); + } + catch (Throwable e) + { + LOG.warn("Errors deleting multipart tmp files", e); + } + } + } + } } finally { diff --git a/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/BufferUtil.java b/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/BufferUtil.java index 0da50c764ca..df01247f9a7 100644 --- a/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/BufferUtil.java +++ b/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/BufferUtil.java @@ -648,6 +648,43 @@ public class BufferUtil } } + public static int readFrom(InputStream is, ByteBuffer buffer) throws IOException + { + if (buffer.hasArray()) + { + int read = is.read(buffer.array(), buffer.arrayOffset() + buffer.limit(), buffer.capacity() - buffer.limit()); + buffer.limit(buffer.limit() + read); + return read; + } + else + { + int totalRead = 0; + ByteBuffer tmp = allocate(8192); + while (BufferUtil.space(tmp) > 0 && BufferUtil.space(buffer) > 0) + { + int read = is.read(tmp.array(), 0, Math.min(BufferUtil.space(tmp), BufferUtil.space(buffer))); + if (read == 0) + { + break; + } + else if (read < 0) + { + if (totalRead == 0) + return -1; + break; + } + totalRead += read; + tmp.position(0); + tmp.limit(read); + + int pos = BufferUtil.flipToFill(buffer); + BufferUtil.put(tmp, buffer); + BufferUtil.flipToFlush(buffer, pos); + } + return totalRead; + } + } + public static void writeTo(ByteBuffer buffer, OutputStream out) throws IOException { if (buffer.hasArray()) diff --git a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletMultiPartFormData.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletMultiPartFormData.java index 6ccab6e7e2f..98fbaeee20d 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletMultiPartFormData.java +++ b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletMultiPartFormData.java @@ -29,9 +29,11 @@ import org.eclipse.jetty.http.HttpHeader; import org.eclipse.jetty.http.MultiPart; import org.eclipse.jetty.http.MultiPartFormData; import org.eclipse.jetty.io.AbstractConnection; +import org.eclipse.jetty.io.ByteBufferPool; import org.eclipse.jetty.io.Connection; import org.eclipse.jetty.io.Content; import org.eclipse.jetty.server.ConnectionMetaData; +import org.eclipse.jetty.util.BufferUtil; import org.eclipse.jetty.util.IO; import org.eclipse.jetty.util.StringUtil; @@ -98,7 +100,9 @@ public class ServletMultiPartFormData if (boundary == null) throw new IllegalStateException("No multipart boundary parameter in Content-Type"); + // Store MultiPartFormData as attribute on request so it is released by the HttpChannel. MultiPartFormData formData = new MultiPartFormData(boundary); + request.setAttribute(MultiPartFormData.class.getName(), formData); formData.setMaxParts(maxParts); File tmpDirFile = (File)request.getServletContext().getAttribute(ServletContext.TEMPDIR); @@ -115,19 +119,30 @@ public class ServletMultiPartFormData ConnectionMetaData connectionMetaData = request.getRequest().getConnectionMetaData(); formData.setPartHeadersMaxLength(connectionMetaData.getHttpConfiguration().getRequestHeaderSize()); + ByteBufferPool byteBufferPool = request.getRequest().getComponents().getByteBufferPool(); Connection connection = connectionMetaData.getConnection(); int bufferSize = connection instanceof AbstractConnection c ? c.getInputBufferSize() : 2048; - byte[] buffer = new byte[bufferSize]; InputStream input = request.getInputStream(); while (true) { - int read = input.read(buffer); - if (read < 0) + ByteBuffer buffer = byteBufferPool.newByteBuffer(bufferSize, false); + boolean readEof = false; + while (BufferUtil.space(buffer) > 0) + { + int read = BufferUtil.readFrom(input, buffer); + if (read < 0) + { + readEof = true; + break; + } + } + + formData.parse(Content.Chunk.from(buffer, false, byteBuffer -> byteBufferPool.release(buffer))); + if (readEof) { formData.parse(Content.Chunk.EOF); break; } - formData.parse(Content.Chunk.from(ByteBuffer.wrap(buffer, 0, read), false)); } return new Parts(formData); @@ -169,7 +184,7 @@ public class ServletMultiPartFormData { _formData = formData; _part = part; - _length = _part.getContent().getLength(); + _length = _part.getLength(); } @Override @@ -215,8 +230,7 @@ public class ServletMultiPartFormData @Override public void delete() throws IOException { - if (_part instanceof MultiPart.PathPart pathPart) - pathPart.delete(); + _part.delete(); } @Override diff --git a/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/MultiPartServletTest.java b/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/MultiPartServletTest.java index d2aa5b8b617..69230d1827a 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/MultiPartServletTest.java +++ b/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/MultiPartServletTest.java @@ -55,7 +55,6 @@ import org.eclipse.jetty.server.handler.gzip.GzipHandler; import org.eclipse.jetty.util.IO; import org.eclipse.jetty.util.component.LifeCycle; import org.junit.jupiter.api.AfterEach; -import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -422,8 +421,7 @@ public class MultiPartServletTest .send(); assertEquals(200, response.getStatus()); - System.err.println(response.getContentAsString()); - // TODO: create proper asserts - Assertions.fail(); + assertThat(response.getContentAsString(), containsString("Part: name=myPart, size=88, content=the quick brown fox jumps over the lazy dog, the quick brown fox jumps over the lazy dog\n" + + "Part: name=myPart, size=88, content=the quick brown fox jumps over the lazy dog, the quick brown fox jumps over the lazy dog")); } } From 1e20abc902f25104e58d3eb0a7b8081228431faf Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Mon, 16 Jan 2023 19:07:51 +1100 Subject: [PATCH 06/22] improvements to multipart Signed-off-by: Lachlan Roberts --- .../client/util/MultiPartRequestContent.java | 2 +- .../util/MultiPartRequestContentTest.java | 22 +++---- .../org/eclipse/jetty/http/MultiPart.java | 58 ++++++++++++------- .../jetty/http/MultiPartByteRanges.java | 4 +- .../eclipse/jetty/http/MultiPartFormData.java | 25 +++++++- .../jetty/http/MultiPartCaptureTest.java | 10 ++-- .../jetty/http/MultiPartFormDataTest.java | 20 +++---- .../org/eclipse/jetty/http/MultiPartTest.java | 18 +++--- .../server/internal/HttpChannelState.java | 19 +----- .../jetty/server/MultiPartByteRangesTest.java | 6 +- .../handler/MultiPartFormDataHandlerTest.java | 6 +- .../ResourceHandlerByteRangesTest.java | 4 +- .../servlet/ServletMultiPartFormData.java | 6 +- 13 files changed, 109 insertions(+), 91 deletions(-) diff --git a/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/util/MultiPartRequestContent.java b/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/util/MultiPartRequestContent.java index 9018b9ea472..0068a5692e7 100644 --- a/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/util/MultiPartRequestContent.java +++ b/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/util/MultiPartRequestContent.java @@ -70,7 +70,7 @@ public class MultiPartRequestContent extends MultiPartFormData.ContentSource imp if (headers.contains(HttpHeader.CONTENT_TYPE)) return headers; - Content.Source partContent = part.getContent(); + Content.Source partContent = part.getNewContent(); if (partContent instanceof Request.Content requestContent) { String contentType = requestContent.getContentType(); diff --git a/jetty-core/jetty-client/src/test/java/org/eclipse/jetty/client/util/MultiPartRequestContentTest.java b/jetty-core/jetty-client/src/test/java/org/eclipse/jetty/client/util/MultiPartRequestContentTest.java index 3bcc08df439..8e20301db14 100644 --- a/jetty-core/jetty-client/src/test/java/org/eclipse/jetty/client/util/MultiPartRequestContentTest.java +++ b/jetty-core/jetty-client/src/test/java/org/eclipse/jetty/client/util/MultiPartRequestContentTest.java @@ -129,7 +129,7 @@ public class MultiPartRequestContentTest extends AbstractHttpClientServerTest int equal = contentType.lastIndexOf('='); Charset charset = Charset.forName(contentType.substring(equal + 1)); assertEquals(encoding, charset); - assertEquals(value, Content.Source.asString(part.getContent(), charset)); + assertEquals(value, Content.Source.asString(part.getNewContent(), charset)); } }); @@ -163,7 +163,7 @@ public class MultiPartRequestContentTest extends AbstractHttpClientServerTest MultiPart.Part part = parts.iterator().next(); assertEquals(name, part.getName()); assertEquals("text/plain", part.getHeaders().get(HttpHeader.CONTENT_TYPE)); - assertArrayEquals(data, Content.Source.asByteBuffer(part.getContent()).array()); + assertArrayEquals(data, Content.Source.asByteBuffer(part.getNewContent()).array()); } }); @@ -215,8 +215,8 @@ public class MultiPartRequestContentTest extends AbstractHttpClientServerTest assertEquals(name, part.getName()); assertEquals(contentType, part.getHeaders().get(HttpHeader.CONTENT_TYPE)); assertEquals(fileName, part.getFileName()); - assertEquals(data.length, part.getContent().getLength()); - assertArrayEquals(data, Content.Source.asByteBuffer(part.getContent()).array()); + assertEquals(data.length, part.getNewContent().getLength()); + assertArrayEquals(data, Content.Source.asByteBuffer(part.getNewContent()).array()); } }); @@ -271,8 +271,8 @@ public class MultiPartRequestContentTest extends AbstractHttpClientServerTest assertEquals(name, part.getName()); assertEquals(contentType, part.getHeaders().get(HttpHeader.CONTENT_TYPE)); assertEquals(tmpPath.getFileName().toString(), part.getFileName()); - assertEquals(Files.size(tmpPath), part.getContent().getLength()); - assertEquals(data, Content.Source.asString(part.getContent(), encoding)); + assertEquals(Files.size(tmpPath), part.getNewContent().getLength()); + assertEquals(data, Content.Source.asString(part.getNewContent(), encoding)); } }); @@ -323,14 +323,14 @@ public class MultiPartRequestContentTest extends AbstractHttpClientServerTest assertEquals(field, fieldPart.getName()); assertEquals(contentType, fieldPart.getHeaders().get(HttpHeader.CONTENT_TYPE)); - assertEquals(value, Content.Source.asString(fieldPart.getContent(), encoding)); + assertEquals(value, Content.Source.asString(fieldPart.getNewContent(), encoding)); assertEquals(headerValue, fieldPart.getHeaders().get(headerName)); assertEquals(fileField, filePart.getName()); assertEquals("application/octet-stream", filePart.getHeaders().get(HttpHeader.CONTENT_TYPE)); assertEquals(tmpPath.getFileName().toString(), filePart.getFileName()); - assertEquals(Files.size(tmpPath), filePart.getContent().getLength()); - assertArrayEquals(data, Content.Source.asByteBuffer(filePart.getContent()).array()); + assertEquals(Files.size(tmpPath), filePart.getNewContent().getLength()); + assertArrayEquals(data, Content.Source.asByteBuffer(filePart.getNewContent()).array()); } }); @@ -367,11 +367,11 @@ public class MultiPartRequestContentTest extends AbstractHttpClientServerTest MultiPart.Part fieldPart = parts.get(0); MultiPart.Part filePart = parts.get(1); - assertEquals(value, Content.Source.asString(fieldPart.getContent(), encoding)); + assertEquals(value, Content.Source.asString(fieldPart.getNewContent(), encoding)); assertEquals("file", filePart.getName()); assertEquals("application/octet-stream", filePart.getHeaders().get(HttpHeader.CONTENT_TYPE)); assertEquals("fileName", filePart.getFileName()); - assertArrayEquals(fileData, Content.Source.asByteBuffer(filePart.getContent()).array()); + assertArrayEquals(fileData, Content.Source.asByteBuffer(filePart.getNewContent()).array()); } }); diff --git a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPart.java b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPart.java index 7ab7b392152..b39d3475312 100644 --- a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPart.java +++ b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPart.java @@ -126,6 +126,7 @@ public class MultiPart private final HttpFields fields; protected Path path; private boolean temporary = true; + private Content.Source content; public Part(String name, String fileName, HttpFields fields) { @@ -162,7 +163,8 @@ public class MultiPart } /** - *

Returns the content of this part.

+ *

Returns the content of this part as a {@link Content.Source}.

+ *

Calling this method multiple times will return the same instance.

*

The content type and content encoding are specified in this part's * {@link #getHeaders() headers}.

*

The content encoding may be specified by the part named {@code _charset_}, @@ -171,11 +173,30 @@ public class MultiPart * * @return the content of this part */ - public abstract Content.Source getContent(); + public Content.Source getContentSource() + { + if (content == null) + content = getNewContent(); + return content; + } + + /** + *

Returns the content of this part as a new {@link Content.Source}

+ *

If the content is reproducible, invoking this method multiple times will return + * a different independent instance for every invocation.

+ *

If the content is not reproducible, subsequent calls to this method will return null.

+ *

The content type and content encoding are specified in this part's {@link #getHeaders() headers}.

+ *

The content encoding may be specified by the part named {@code _charset_}, + * as specified in + * RFC 7578, section 4.6.

+ * + * @return the content of this part as a new {@link Content.Source} + */ + public abstract Content.Source getNewContent(); public long getLength() { - return getContent().getLength(); + return getNewContent().getLength(); } /** @@ -200,7 +221,7 @@ public class MultiPart Charset charset = defaultCharset != null ? defaultCharset : UTF_8; if (charsetName != null) charset = Charset.forName(charsetName); - return Content.Source.asString(getContent(), charset); + return Content.Source.asString(getNewContent(), charset); } catch (IOException x) { @@ -229,12 +250,11 @@ public class MultiPart */ public void writeTo(Path path) throws IOException { - if (this.path == null) { try (OutputStream out = Files.newOutputStream(path)) { - IO.copy(Content.Source.asInputStream(getContent()), out); + IO.copy(Content.Source.asInputStream(getNewContent()), out); } this.path = path; } @@ -251,17 +271,11 @@ public class MultiPart Files.delete(this.path); } - public void close(Throwable failure) throws IOException + public void close() throws IOException { - // TODO: why do we need this, no one ever uses the failure?? if (temporary) delete(); } - - public final void close() throws IOException - { - close(null); - } } /** @@ -292,7 +306,7 @@ public class MultiPart } @Override - public Content.Source getContent() + public Content.Source getNewContent() { return new ByteBufferContentSource(content); } @@ -332,15 +346,15 @@ public class MultiPart } @Override - public Content.Source getContent() + public Content.Source getNewContent() { return new ChunksContentSource(content.stream().map(Content.Chunk::slice).toList()); } @Override - public void close(Throwable failure) throws IOException + public void close() throws IOException { - super.close(failure); + super.close(); content.forEach(Retainable::release); } @@ -382,7 +396,7 @@ public class MultiPart } @Override - public Content.Source getContent() + public Content.Source getNewContent() { return new PathContentSource(path); } @@ -414,7 +428,7 @@ public class MultiPart } @Override - public Content.Source getContent() + public Content.Source getNewContent() { Content.Source c = content; content = null; @@ -616,7 +630,7 @@ public class MultiPart else { part = parts.poll(); - partContent = part.getContent(); + partContent = part.getNewContent(); state = State.HEADERS; yield Content.Chunk.from(firstBoundary.slice(), false); } @@ -643,7 +657,7 @@ public class MultiPart else { part = parts.poll(); - partContent = part.getContent(); + partContent = part.getNewContent(); state = State.HEADERS; yield Content.Chunk.from(middleBoundary.slice(), false); } @@ -765,7 +779,7 @@ public class MultiPart { try { - part.close(failure); + part.close(); } catch (IOException e) { diff --git a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartByteRanges.java b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartByteRanges.java index a843ae8e5af..80117431dc5 100644 --- a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartByteRanges.java +++ b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartByteRanges.java @@ -265,7 +265,7 @@ public class MultiPartByteRanges extends CompletableFuture +public class MultiPartFormData extends CompletableFuture implements Closeable { private static final Logger LOG = LoggerFactory.getLogger(MultiPartFormData.class); @@ -569,7 +569,7 @@ public class MultiPartFormData extends CompletableFuture charSetParts = allParts.get("_charset_"); if (charSetParts != null) { - defaultCharset = Promise.Completable.with(p -> Content.Source.asString(charSetParts.get(0).getContent(), StandardCharsets.US_ASCII, p)) + defaultCharset = Promise.Completable.with(p -> Content.Source.asString(charSetParts.get(0).getNewContent(), StandardCharsets.US_ASCII, p)) .get(); } @@ -255,8 +255,8 @@ public class MultiPartCaptureTest assertThat("Part[" + expected.name + "]", parts, is(notNullValue())); MultiPart.Part part = parts.get(0); String charset = getCharsetFromContentType(part.getHeaders().get(HttpHeader.CONTENT_TYPE), defaultCharset); - assertTrue(part.getContent().rewind()); - String partContent = Content.Source.asString(part.getContent(), Charset.forName(charset)); + assertTrue(part.getNewContent().rewind()); + String partContent = Content.Source.asString(part.getNewContent(), Charset.forName(charset)); assertThat("Part[" + expected.name + "].contents", partContent, containsString(expected.value)); } @@ -276,8 +276,8 @@ public class MultiPartCaptureTest assertThat("Part[" + expected.name + "]", parts, is(notNullValue())); MultiPart.Part part = parts.get(0); MessageDigest digest = MessageDigest.getInstance("SHA1"); - assertTrue(part.getContent().rewind()); - try (InputStream partInputStream = Content.Source.asInputStream(part.getContent()); + assertTrue(part.getNewContent().rewind()); + try (InputStream partInputStream = Content.Source.asInputStream(part.getNewContent()); DigestOutputStream digester = new DigestOutputStream(OutputStream.nullOutputStream(), digest)) { IO.copy(partInputStream, digester); diff --git a/jetty-core/jetty-http/src/test/java/org/eclipse/jetty/http/MultiPartFormDataTest.java b/jetty-core/jetty-http/src/test/java/org/eclipse/jetty/http/MultiPartFormDataTest.java index 7b30c229326..80c2ef614b7 100644 --- a/jetty-core/jetty-http/src/test/java/org/eclipse/jetty/http/MultiPartFormDataTest.java +++ b/jetty-core/jetty-http/src/test/java/org/eclipse/jetty/http/MultiPartFormDataTest.java @@ -189,25 +189,25 @@ public class MultiPartFormDataTest MultiPart.Part fileName = parts.getFirst("fileName"); assertThat(fileName, notNullValue()); - Content.Source partContent = fileName.getContent(); + Content.Source partContent = fileName.getNewContent(); assertThat(partContent.getLength(), is(3L)); assertThat(Content.Source.asString(partContent), is("abc")); MultiPart.Part desc = parts.getFirst("desc"); assertThat(desc, notNullValue()); - partContent = desc.getContent(); + partContent = desc.getNewContent(); assertThat(partContent.getLength(), is(3L)); assertThat(Content.Source.asString(partContent), is("123")); MultiPart.Part title = parts.getFirst("title"); assertThat(title, notNullValue()); - partContent = title.getContent(); + partContent = title.getNewContent(); assertThat(partContent.getLength(), is(3L)); assertThat(Content.Source.asString(partContent), is("ttt")); MultiPart.Part datafile = parts.getFirst("datafile5239138112980980385.txt"); assertThat(datafile, notNullValue()); - partContent = datafile.getContent(); + partContent = datafile.getNewContent(); assertThat(partContent.getLength(), is(3L)); assertThat(Content.Source.asString(partContent), is("000")); } @@ -275,11 +275,11 @@ public class MultiPartFormDataTest assertThat(parts.size(), is(2)); MultiPart.Part part1 = parts.getFirst("field1"); assertThat(part1, notNullValue()); - Content.Source partContent = part1.getContent(); + Content.Source partContent = part1.getNewContent(); assertThat(Content.Source.asString(partContent), is("Joe Blow")); MultiPart.Part part2 = parts.getFirst("stuff"); assertThat(part2, notNullValue()); - partContent = part2.getContent(); + partContent = part2.getNewContent(); assertThat(Content.Source.asString(partContent), is("aaaabbbbb")); } @@ -312,7 +312,7 @@ public class MultiPartFormDataTest assertThat(parts.size(), is(1)); MultiPart.Part part2 = parts.getFirst("stuff"); assertThat(part2, notNullValue()); - Content.Source partContent = part2.getContent(); + Content.Source partContent = part2.getNewContent(); assertThat(Content.Source.asString(partContent), is("aaaabbbbb")); } @@ -340,7 +340,7 @@ public class MultiPartFormDataTest assertThat(part, instanceOf(MultiPart.PathPart.class)); MultiPart.PathPart pathPart = (MultiPart.PathPart)part; assertTrue(Files.exists(pathPart.getPath())); - assertEquals("ABCDEFGHIJKLMNOPQRSTUVWXYZ", Content.Source.asString(part.getContent())); + assertEquals("ABCDEFGHIJKLMNOPQRSTUVWXYZ", Content.Source.asString(part.getNewContent())); } @Test @@ -422,13 +422,13 @@ public class MultiPartFormDataTest MultiPart.Part part1 = parts.get(0); assertThat(part1, instanceOf(MultiPart.ChunksPart.class)); - assertEquals(chunk, Content.Source.asString(part1.getContent())); + assertEquals(chunk, Content.Source.asString(part1.getNewContent())); MultiPart.Part part2 = parts.get(1); assertThat(part2, instanceOf(MultiPart.PathPart.class)); MultiPart.PathPart pathPart2 = (MultiPart.PathPart)part2; assertTrue(Files.exists(pathPart2.getPath())); - assertEquals(chunk.repeat(4), Content.Source.asString(part2.getContent())); + assertEquals(chunk.repeat(4), Content.Source.asString(part2.getNewContent())); } @Test diff --git a/jetty-core/jetty-http/src/test/java/org/eclipse/jetty/http/MultiPartTest.java b/jetty-core/jetty-http/src/test/java/org/eclipse/jetty/http/MultiPartTest.java index 8c99d8c00b8..18b1fd45ad1 100644 --- a/jetty-core/jetty-http/src/test/java/org/eclipse/jetty/http/MultiPartTest.java +++ b/jetty-core/jetty-http/src/test/java/org/eclipse/jetty/http/MultiPartTest.java @@ -361,11 +361,11 @@ public class MultiPartTest MultiPart.Part part1 = listener.parts.get(0); assertEquals("value", part1.getHeaders().get("name")); - assertEquals("Hello", Content.Source.asString(part1.getContent())); + assertEquals("Hello", Content.Source.asString(part1.getNewContent())); MultiPart.Part part2 = listener.parts.get(1); assertEquals("9001", part2.getHeaders().get("powerLevel")); - assertEquals("secondary\r\ncontent", Content.Source.asString(part2.getContent())); + assertEquals("secondary\r\ncontent", Content.Source.asString(part2.getNewContent())); assertEquals(0, data.remaining()); } @@ -397,11 +397,11 @@ public class MultiPartTest MultiPart.Part part1 = listener.parts.get(0); assertEquals("value", part1.getHeaders().get("name")); - assertEquals("Hello", Content.Source.asString(part1.getContent())); + assertEquals("Hello", Content.Source.asString(part1.getNewContent())); MultiPart.Part part2 = listener.parts.get(1); assertEquals("9001", part2.getHeaders().get("powerLevel")); - assertEquals("secondary\ncontent", Content.Source.asString(part2.getContent())); + assertEquals("secondary\ncontent", Content.Source.asString(part2.getNewContent())); assertEquals(0, data.remaining()); } @@ -457,7 +457,7 @@ public class MultiPartTest assertEquals(1, listener.parts.size()); MultiPart.Part part = listener.parts.get(0); assertEquals("value", part.getHeaders().get("name")); - assertEquals("", Content.Source.asString(part.getContent())); + assertEquals("", Content.Source.asString(part.getNewContent())); } @Test @@ -477,7 +477,7 @@ public class MultiPartTest assertEquals(1, listener.parts.size()); MultiPart.Part part = listener.parts.get(0); assertEquals("value", part.getHeaders().get("name")); - assertEquals("", Content.Source.asString(part.getContent())); + assertEquals("", Content.Source.asString(part.getNewContent())); } @Test @@ -508,7 +508,7 @@ public class MultiPartTest assertEquals(1, listener.parts.size()); MultiPart.Part part = listener.parts.get(0); assertEquals("value", part.getHeaders().get("name")); - assertThat(Content.Source.asString(part.getContent()), is(""" + assertThat(Content.Source.asString(part.getNewContent()), is(""" Hello\r this is not a --BOUNDARY\r that's a boundary""")); @@ -532,7 +532,7 @@ public class MultiPartTest assertThat(epilogueBuffer.remaining(), is(0)); assertEquals(1, listener.parts.size()); MultiPart.Part part = listener.parts.get(0); - assertThat(Content.Source.asByteBuffer(part.getContent()), is(ByteBuffer.wrap(random))); + assertThat(Content.Source.asByteBuffer(part.getNewContent()), is(ByteBuffer.wrap(random))); } @Test @@ -556,7 +556,7 @@ public class MultiPartTest assertEquals(1, listener.parts.size()); MultiPart.Part part = listener.parts.get(0); assertEquals("value", part.getHeaders().get("name")); - assertEquals("Hello", Content.Source.asString(part.getContent())); + assertEquals("Hello", Content.Source.asString(part.getNewContent())); } @Test diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java index c41046eeae6..ca5a944153d 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java @@ -34,7 +34,6 @@ import org.eclipse.jetty.http.HttpStatus; import org.eclipse.jetty.http.HttpURI; import org.eclipse.jetty.http.HttpVersion; import org.eclipse.jetty.http.MetaData; -import org.eclipse.jetty.http.MultiPart; import org.eclipse.jetty.http.MultiPartFormData; import org.eclipse.jetty.http.PreEncodedHttpField; import org.eclipse.jetty.http.Trailers; @@ -652,23 +651,7 @@ public class HttpChannelState implements HttpChannel, Components // Clean up any multipart tmp files and release any associated resources. MultiPartFormData multiParts = (MultiPartFormData)_request.getAttribute(MultiPartFormData.class.getName()); if (multiParts != null) - { - MultiPartFormData.Parts parts = multiParts.getNow(null); - if (parts != null) - { - for (MultiPart.Part p : parts) - { - try - { - p.close(); - } - catch (Throwable e) - { - LOG.warn("Errors deleting multipart tmp files", e); - } - } - } - } + multiParts.close(); } finally { diff --git a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/MultiPartByteRangesTest.java b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/MultiPartByteRangesTest.java index 5aafb96a91a..f955f5ea909 100644 --- a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/MultiPartByteRangesTest.java +++ b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/MultiPartByteRangesTest.java @@ -119,11 +119,11 @@ public class MultiPartByteRangesTest assertEquals(3, parts.size()); MultiPart.Part part1 = parts.get(0); - assertEquals("12", Content.Source.asString(part1.getContent())); + assertEquals("12", Content.Source.asString(part1.getNewContent())); MultiPart.Part part2 = parts.get(1); - assertEquals("456", Content.Source.asString(part2.getContent())); + assertEquals("456", Content.Source.asString(part2.getNewContent())); MultiPart.Part part3 = parts.get(2); - assertEquals("CDEF", Content.Source.asString(part3.getContent())); + assertEquals("CDEF", Content.Source.asString(part3.getNewContent())); } } } diff --git a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/handler/MultiPartFormDataHandlerTest.java b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/handler/MultiPartFormDataHandlerTest.java index 2c449efe687..a365abed169 100644 --- a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/handler/MultiPartFormDataHandlerTest.java +++ b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/handler/MultiPartFormDataHandlerTest.java @@ -80,7 +80,7 @@ public class MultiPartFormDataHandlerTest .whenComplete((parts, failure) -> { if (parts != null) - Content.copy(parts.get(0).getContent(), response, callback); + Content.copy(parts.get(0).getNewContent(), response, callback); else Response.writeError(request, response, callback, failure); }); @@ -129,7 +129,7 @@ public class MultiPartFormDataHandlerTest MultiPartFormData formData = (MultiPartFormData)request.getAttribute(MultiPartFormData.class.getName()); assertNotNull(formData); MultiPart.Part part = formData.get().get(0); - Content.copy(part.getContent(), response, callback); + Content.copy(part.getNewContent(), response, callback); return true; } }); @@ -321,7 +321,7 @@ public class MultiPartFormDataHandlerTest HttpFields headers2 = part2.getHeaders(); assertEquals(2, headers2.size()); assertEquals("application/octet-stream", headers2.get(HttpHeader.CONTENT_TYPE)); - assertEquals(32, part2.getContent().getLength()); + assertEquals(32, part2.getNewContent().getLength()); } } } diff --git a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/handler/ResourceHandlerByteRangesTest.java b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/handler/ResourceHandlerByteRangesTest.java index 05e803ed1b6..8247eff178a 100644 --- a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/handler/ResourceHandlerByteRangesTest.java +++ b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/handler/ResourceHandlerByteRangesTest.java @@ -181,10 +181,10 @@ public class ResourceHandlerByteRangesTest assertEquals(2, parts.size()); MultiPart.Part part1 = parts.get(0); assertEquals("text/plain", part1.getHeaders().get(HttpHeader.CONTENT_TYPE)); - assertEquals("234", Content.Source.asString(part1.getContent())); + assertEquals("234", Content.Source.asString(part1.getNewContent())); MultiPart.Part part2 = parts.get(1); assertEquals("text/plain", part2.getHeaders().get(HttpHeader.CONTENT_TYPE)); - assertEquals("xyz", Content.Source.asString(part2.getContent())); + assertEquals("xyz", Content.Source.asString(part2.getNewContent())); } } } diff --git a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletMultiPartFormData.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletMultiPartFormData.java index 98fbaeee20d..28ad413be6f 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletMultiPartFormData.java +++ b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletMultiPartFormData.java @@ -76,7 +76,7 @@ public class ServletMultiPartFormData { try { - // Look for a previously read and parsed MultiPartFormData from the DelayedHandler + // Look for a previously read and parsed MultiPartFormData from the DelayedHandler. MultiPartFormData formData = (MultiPartFormData)request.getAttribute(MultiPartFormData.class.getName()); if (formData != null) return new Parts(formData); @@ -127,7 +127,7 @@ public class ServletMultiPartFormData { ByteBuffer buffer = byteBufferPool.newByteBuffer(bufferSize, false); boolean readEof = false; - while (BufferUtil.space(buffer) > 0) + while (BufferUtil.space(buffer) > bufferSize / 2) { int read = BufferUtil.readFrom(input, buffer); if (read < 0) @@ -190,7 +190,7 @@ public class ServletMultiPartFormData @Override public InputStream getInputStream() throws IOException { - return Content.Source.asInputStream(_part.getContent()); + return Content.Source.asInputStream(_part.getNewContent()); } @Override From 5bcfeaac09ef9c0162abb868c5ffbb1bc24f4f66 Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Tue, 17 Jan 2023 14:09:45 +1100 Subject: [PATCH 07/22] improvements to multipart Signed-off-by: Lachlan Roberts --- .../client/util/MultiPartRequestContent.java | 2 +- .../util/MultiPartRequestContentTest.java | 22 +-- .../org/eclipse/jetty/http/MultiPart.java | 128 ++++++++++++++---- .../jetty/http/MultiPartByteRanges.java | 2 +- .../eclipse/jetty/http/MultiPartFormData.java | 68 ++++------ .../jetty/http/MultiPartCaptureTest.java | 10 +- .../jetty/http/MultiPartFormDataTest.java | 20 +-- .../org/eclipse/jetty/http/MultiPartTest.java | 18 +-- .../jetty/server/handler/DelayedHandler.java | 2 +- .../server/internal/HttpChannelState.java | 8 +- .../jetty/server/MultiPartByteRangesTest.java | 6 +- .../handler/MultiPartFormDataHandlerTest.java | 16 +-- .../ResourceHandlerByteRangesTest.java | 4 +- .../servlet/ServletMultiPartFormData.java | 19 +-- .../ee10/servlet/MultiPartServletTest.java | 12 -- 15 files changed, 191 insertions(+), 146 deletions(-) diff --git a/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/util/MultiPartRequestContent.java b/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/util/MultiPartRequestContent.java index 0068a5692e7..eb5ddaede32 100644 --- a/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/util/MultiPartRequestContent.java +++ b/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/util/MultiPartRequestContent.java @@ -70,7 +70,7 @@ public class MultiPartRequestContent extends MultiPartFormData.ContentSource imp if (headers.contains(HttpHeader.CONTENT_TYPE)) return headers; - Content.Source partContent = part.getNewContent(); + Content.Source partContent = part.newContentSource(); if (partContent instanceof Request.Content requestContent) { String contentType = requestContent.getContentType(); diff --git a/jetty-core/jetty-client/src/test/java/org/eclipse/jetty/client/util/MultiPartRequestContentTest.java b/jetty-core/jetty-client/src/test/java/org/eclipse/jetty/client/util/MultiPartRequestContentTest.java index 8e20301db14..6ca29407eb9 100644 --- a/jetty-core/jetty-client/src/test/java/org/eclipse/jetty/client/util/MultiPartRequestContentTest.java +++ b/jetty-core/jetty-client/src/test/java/org/eclipse/jetty/client/util/MultiPartRequestContentTest.java @@ -129,7 +129,7 @@ public class MultiPartRequestContentTest extends AbstractHttpClientServerTest int equal = contentType.lastIndexOf('='); Charset charset = Charset.forName(contentType.substring(equal + 1)); assertEquals(encoding, charset); - assertEquals(value, Content.Source.asString(part.getNewContent(), charset)); + assertEquals(value, Content.Source.asString(part.newContentSource(), charset)); } }); @@ -163,7 +163,7 @@ public class MultiPartRequestContentTest extends AbstractHttpClientServerTest MultiPart.Part part = parts.iterator().next(); assertEquals(name, part.getName()); assertEquals("text/plain", part.getHeaders().get(HttpHeader.CONTENT_TYPE)); - assertArrayEquals(data, Content.Source.asByteBuffer(part.getNewContent()).array()); + assertArrayEquals(data, Content.Source.asByteBuffer(part.newContentSource()).array()); } }); @@ -215,8 +215,8 @@ public class MultiPartRequestContentTest extends AbstractHttpClientServerTest assertEquals(name, part.getName()); assertEquals(contentType, part.getHeaders().get(HttpHeader.CONTENT_TYPE)); assertEquals(fileName, part.getFileName()); - assertEquals(data.length, part.getNewContent().getLength()); - assertArrayEquals(data, Content.Source.asByteBuffer(part.getNewContent()).array()); + assertEquals(data.length, part.newContentSource().getLength()); + assertArrayEquals(data, Content.Source.asByteBuffer(part.newContentSource()).array()); } }); @@ -271,8 +271,8 @@ public class MultiPartRequestContentTest extends AbstractHttpClientServerTest assertEquals(name, part.getName()); assertEquals(contentType, part.getHeaders().get(HttpHeader.CONTENT_TYPE)); assertEquals(tmpPath.getFileName().toString(), part.getFileName()); - assertEquals(Files.size(tmpPath), part.getNewContent().getLength()); - assertEquals(data, Content.Source.asString(part.getNewContent(), encoding)); + assertEquals(Files.size(tmpPath), part.newContentSource().getLength()); + assertEquals(data, Content.Source.asString(part.newContentSource(), encoding)); } }); @@ -323,14 +323,14 @@ public class MultiPartRequestContentTest extends AbstractHttpClientServerTest assertEquals(field, fieldPart.getName()); assertEquals(contentType, fieldPart.getHeaders().get(HttpHeader.CONTENT_TYPE)); - assertEquals(value, Content.Source.asString(fieldPart.getNewContent(), encoding)); + assertEquals(value, Content.Source.asString(fieldPart.newContentSource(), encoding)); assertEquals(headerValue, fieldPart.getHeaders().get(headerName)); assertEquals(fileField, filePart.getName()); assertEquals("application/octet-stream", filePart.getHeaders().get(HttpHeader.CONTENT_TYPE)); assertEquals(tmpPath.getFileName().toString(), filePart.getFileName()); - assertEquals(Files.size(tmpPath), filePart.getNewContent().getLength()); - assertArrayEquals(data, Content.Source.asByteBuffer(filePart.getNewContent()).array()); + assertEquals(Files.size(tmpPath), filePart.newContentSource().getLength()); + assertArrayEquals(data, Content.Source.asByteBuffer(filePart.newContentSource()).array()); } }); @@ -367,11 +367,11 @@ public class MultiPartRequestContentTest extends AbstractHttpClientServerTest MultiPart.Part fieldPart = parts.get(0); MultiPart.Part filePart = parts.get(1); - assertEquals(value, Content.Source.asString(fieldPart.getNewContent(), encoding)); + assertEquals(value, Content.Source.asString(fieldPart.newContentSource(), encoding)); assertEquals("file", filePart.getName()); assertEquals("application/octet-stream", filePart.getHeaders().get(HttpHeader.CONTENT_TYPE)); assertEquals("fileName", filePart.getFileName()); - assertArrayEquals(fileData, Content.Source.asByteBuffer(filePart.getNewContent()).array()); + assertArrayEquals(fileData, Content.Source.asByteBuffer(filePart.newContentSource()).array()); } }); diff --git a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPart.java b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPart.java index b39d3475312..4455af9e140 100644 --- a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPart.java +++ b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPart.java @@ -176,7 +176,7 @@ public class MultiPart public Content.Source getContentSource() { if (content == null) - content = getNewContent(); + content = newContentSource(); return content; } @@ -192,11 +192,11 @@ public class MultiPart * * @return the content of this part as a new {@link Content.Source} */ - public abstract Content.Source getNewContent(); + public abstract Content.Source newContentSource(); public long getLength() { - return getNewContent().getLength(); + return newContentSource().getLength(); } /** @@ -221,7 +221,7 @@ public class MultiPart Charset charset = defaultCharset != null ? defaultCharset : UTF_8; if (charsetName != null) charset = Charset.forName(charsetName); - return Content.Source.asString(getNewContent(), charset); + return Content.Source.asString(newContentSource(), charset); } catch (IOException x) { @@ -237,11 +237,6 @@ public class MultiPart return fields; } - public Path getPath() - { - return path; - } - /** *

Writes the content of this part to the given path.

* @@ -254,7 +249,7 @@ public class MultiPart { try (OutputStream out = Files.newOutputStream(path)) { - IO.copy(Content.Source.asInputStream(getNewContent()), out); + IO.copy(Content.Source.asInputStream(newContentSource()), out); } this.path = path; } @@ -306,7 +301,7 @@ public class MultiPart } @Override - public Content.Source getNewContent() + public Content.Source newContentSource() { return new ByteBufferContentSource(content); } @@ -346,7 +341,7 @@ public class MultiPart } @Override - public Content.Source getNewContent() + public Content.Source newContentSource() { return new ChunksContentSource(content.stream().map(Content.Chunk::slice).toList()); } @@ -395,8 +390,13 @@ public class MultiPart } } + public Path getPath() + { + return path; + } + @Override - public Content.Source getNewContent() + public Content.Source newContentSource() { return new PathContentSource(path); } @@ -409,7 +409,7 @@ public class MultiPart hashCode(), getName(), getFileName(), - getPath() + path ); } } @@ -428,7 +428,7 @@ public class MultiPart } @Override - public Content.Source getNewContent() + public Content.Source newContentSource() { Content.Source c = content; content = null; @@ -630,7 +630,7 @@ public class MultiPart else { part = parts.poll(); - partContent = part.getNewContent(); + partContent = part.newContentSource(); state = State.HEADERS; yield Content.Chunk.from(firstBoundary.slice(), false); } @@ -657,7 +657,7 @@ public class MultiPart else { part = parts.poll(); - partContent = part.getNewContent(); + partContent = part.newContentSource(); state = State.HEADERS; yield Content.Chunk.from(middleBoundary.slice(), false); } @@ -743,7 +743,7 @@ public class MultiPart if (state == State.CONTENT) { - partContent.demand(() -> + part.getContentSource().demand(() -> { try (AutoLock ignoredAgain = lock.lock()) { @@ -850,6 +850,8 @@ public class MultiPart private int trailingWhiteSpaces; private String fieldName; private String fieldValue; + private long maxParts; + private int numParts = 0; public Parser(String boundary, Listener listener) { @@ -881,6 +883,22 @@ public class MultiPart this.partHeadersMaxLength = partHeadersMaxLength; } + /** + * @return the maximum number of parts that can be parsed from the multipart content. + */ + public long getMaxParts() + { + return maxParts; + } + + /** + * @param maxParts the maximum number of parts that can be parsed from the multipart content. + */ + public void setMaxParts(long maxParts) + { + this.maxParts = maxParts; + } + /** *

Resets this parser to make it ready to parse again a multipart/form-data content.

*/ @@ -939,6 +957,10 @@ public class MultiPart } else if (type == HttpTokens.Type.LF) { + numParts++; + if (numParts >= maxParts) + throw new IllegalStateException(String.format("Form with too many keys [%d > %d]", numParts, maxParts)); + notifyPartBegin(); state = State.HEADER_START; trailingWhiteSpaces = 0; @@ -1343,37 +1365,93 @@ public class MultiPart private void notifyPartBegin() { - listener.onPartBegin(); + try + { + listener.onPartBegin(); + } + catch (Throwable x) + { + if (LOG.isDebugEnabled()) + LOG.debug("failure while notifying listener {}", listener, x); + } } private void notifyPartHeader(String name, String value) { - listener.onPartHeader(name, value); + try + { + listener.onPartHeader(name, value); + } + catch (Throwable x) + { + if (LOG.isDebugEnabled()) + LOG.debug("failure while notifying listener {}", listener, x); + } } private void notifyPartHeaders() { - listener.onPartHeaders(); + try + { + listener.onPartHeaders(); + } + catch (Throwable x) + { + if (LOG.isDebugEnabled()) + LOG.debug("failure while notifying listener {}", listener, x); + } } private void notifyPartContent(Content.Chunk chunk) { - listener.onPartContent(chunk); + try + { + listener.onPartContent(chunk); + } + catch (Throwable x) + { + if (LOG.isDebugEnabled()) + LOG.debug("failure while notifying listener {}", listener, x); + } } private void notifyPartEnd() { - listener.onPartEnd(); + try + { + listener.onPartEnd(); + } + catch (Throwable x) + { + if (LOG.isDebugEnabled()) + LOG.debug("failure while notifying listener {}", listener, x); + } } private void notifyComplete() { - listener.onComplete(); + try + { + listener.onComplete(); + } + catch (Throwable x) + { + if (LOG.isDebugEnabled()) + LOG.debug("failure while notifying listener {}", listener, x); + } } private void notifyFailure(Throwable failure) { - listener.onFailure(failure); + try + { + listener.onFailure(failure); + } + catch (Throwable x) + { + if (LOG.isDebugEnabled()) + LOG.debug("failure while notifying listener {}", listener, x); + } } /** diff --git a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartByteRanges.java b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartByteRanges.java index 80117431dc5..4f4feebd1d4 100644 --- a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartByteRanges.java +++ b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartByteRanges.java @@ -265,7 +265,7 @@ public class MultiPartByteRanges extends CompletableFuture implements Closeable +public class MultiPartFormData extends CompletableFuture { private static final Logger LOG = LoggerFactory.getLogger(MultiPartFormData.class); @@ -80,8 +80,6 @@ public class MultiPartFormData extends CompletableFutureAn ordered list of {@link MultiPart.Part}s that can * be accessed by index or by name, or iterated over.

*/ - public static class Parts implements Iterable + public class Parts implements Iterable { - private final String boundary; private final List parts; - private Parts(String boundary, List parts) + private Parts(List parts) { - this.boundary = boundary; this.parts = parts; } - /** - * @return the boundary string - */ - public String getBoundary() + public MultiPartFormData getMultiPartFormData() { - return boundary; + return MultiPartFormData.this; } /** @@ -383,6 +376,21 @@ public class MultiPartFormData extends CompletableFuture= maxParts) - throw new IllegalStateException(String.format("Form with too many keys [%d > %d]", numParts, maxParts)); - } - @Override public void onPart(String name, String fileName, HttpFields headers) { @@ -536,7 +535,7 @@ public class MultiPartFormData extends CompletableFuture getParts() @@ -629,25 +628,4 @@ public class MultiPartFormData extends CompletableFuture charSetParts = allParts.get("_charset_"); if (charSetParts != null) { - defaultCharset = Promise.Completable.with(p -> Content.Source.asString(charSetParts.get(0).getNewContent(), StandardCharsets.US_ASCII, p)) + defaultCharset = Promise.Completable.with(p -> Content.Source.asString(charSetParts.get(0).newContentSource(), StandardCharsets.US_ASCII, p)) .get(); } @@ -255,8 +255,8 @@ public class MultiPartCaptureTest assertThat("Part[" + expected.name + "]", parts, is(notNullValue())); MultiPart.Part part = parts.get(0); String charset = getCharsetFromContentType(part.getHeaders().get(HttpHeader.CONTENT_TYPE), defaultCharset); - assertTrue(part.getNewContent().rewind()); - String partContent = Content.Source.asString(part.getNewContent(), Charset.forName(charset)); + assertTrue(part.newContentSource().rewind()); + String partContent = Content.Source.asString(part.newContentSource(), Charset.forName(charset)); assertThat("Part[" + expected.name + "].contents", partContent, containsString(expected.value)); } @@ -276,8 +276,8 @@ public class MultiPartCaptureTest assertThat("Part[" + expected.name + "]", parts, is(notNullValue())); MultiPart.Part part = parts.get(0); MessageDigest digest = MessageDigest.getInstance("SHA1"); - assertTrue(part.getNewContent().rewind()); - try (InputStream partInputStream = Content.Source.asInputStream(part.getNewContent()); + assertTrue(part.newContentSource().rewind()); + try (InputStream partInputStream = Content.Source.asInputStream(part.newContentSource()); DigestOutputStream digester = new DigestOutputStream(OutputStream.nullOutputStream(), digest)) { IO.copy(partInputStream, digester); diff --git a/jetty-core/jetty-http/src/test/java/org/eclipse/jetty/http/MultiPartFormDataTest.java b/jetty-core/jetty-http/src/test/java/org/eclipse/jetty/http/MultiPartFormDataTest.java index 80c2ef614b7..6eeff622a34 100644 --- a/jetty-core/jetty-http/src/test/java/org/eclipse/jetty/http/MultiPartFormDataTest.java +++ b/jetty-core/jetty-http/src/test/java/org/eclipse/jetty/http/MultiPartFormDataTest.java @@ -189,25 +189,25 @@ public class MultiPartFormDataTest MultiPart.Part fileName = parts.getFirst("fileName"); assertThat(fileName, notNullValue()); - Content.Source partContent = fileName.getNewContent(); + Content.Source partContent = fileName.newContentSource(); assertThat(partContent.getLength(), is(3L)); assertThat(Content.Source.asString(partContent), is("abc")); MultiPart.Part desc = parts.getFirst("desc"); assertThat(desc, notNullValue()); - partContent = desc.getNewContent(); + partContent = desc.newContentSource(); assertThat(partContent.getLength(), is(3L)); assertThat(Content.Source.asString(partContent), is("123")); MultiPart.Part title = parts.getFirst("title"); assertThat(title, notNullValue()); - partContent = title.getNewContent(); + partContent = title.newContentSource(); assertThat(partContent.getLength(), is(3L)); assertThat(Content.Source.asString(partContent), is("ttt")); MultiPart.Part datafile = parts.getFirst("datafile5239138112980980385.txt"); assertThat(datafile, notNullValue()); - partContent = datafile.getNewContent(); + partContent = datafile.newContentSource(); assertThat(partContent.getLength(), is(3L)); assertThat(Content.Source.asString(partContent), is("000")); } @@ -275,11 +275,11 @@ public class MultiPartFormDataTest assertThat(parts.size(), is(2)); MultiPart.Part part1 = parts.getFirst("field1"); assertThat(part1, notNullValue()); - Content.Source partContent = part1.getNewContent(); + Content.Source partContent = part1.newContentSource(); assertThat(Content.Source.asString(partContent), is("Joe Blow")); MultiPart.Part part2 = parts.getFirst("stuff"); assertThat(part2, notNullValue()); - partContent = part2.getNewContent(); + partContent = part2.newContentSource(); assertThat(Content.Source.asString(partContent), is("aaaabbbbb")); } @@ -312,7 +312,7 @@ public class MultiPartFormDataTest assertThat(parts.size(), is(1)); MultiPart.Part part2 = parts.getFirst("stuff"); assertThat(part2, notNullValue()); - Content.Source partContent = part2.getNewContent(); + Content.Source partContent = part2.newContentSource(); assertThat(Content.Source.asString(partContent), is("aaaabbbbb")); } @@ -340,7 +340,7 @@ public class MultiPartFormDataTest assertThat(part, instanceOf(MultiPart.PathPart.class)); MultiPart.PathPart pathPart = (MultiPart.PathPart)part; assertTrue(Files.exists(pathPart.getPath())); - assertEquals("ABCDEFGHIJKLMNOPQRSTUVWXYZ", Content.Source.asString(part.getNewContent())); + assertEquals("ABCDEFGHIJKLMNOPQRSTUVWXYZ", Content.Source.asString(part.newContentSource())); } @Test @@ -422,13 +422,13 @@ public class MultiPartFormDataTest MultiPart.Part part1 = parts.get(0); assertThat(part1, instanceOf(MultiPart.ChunksPart.class)); - assertEquals(chunk, Content.Source.asString(part1.getNewContent())); + assertEquals(chunk, Content.Source.asString(part1.newContentSource())); MultiPart.Part part2 = parts.get(1); assertThat(part2, instanceOf(MultiPart.PathPart.class)); MultiPart.PathPart pathPart2 = (MultiPart.PathPart)part2; assertTrue(Files.exists(pathPart2.getPath())); - assertEquals(chunk.repeat(4), Content.Source.asString(part2.getNewContent())); + assertEquals(chunk.repeat(4), Content.Source.asString(part2.newContentSource())); } @Test diff --git a/jetty-core/jetty-http/src/test/java/org/eclipse/jetty/http/MultiPartTest.java b/jetty-core/jetty-http/src/test/java/org/eclipse/jetty/http/MultiPartTest.java index 18b1fd45ad1..5560a495e8c 100644 --- a/jetty-core/jetty-http/src/test/java/org/eclipse/jetty/http/MultiPartTest.java +++ b/jetty-core/jetty-http/src/test/java/org/eclipse/jetty/http/MultiPartTest.java @@ -361,11 +361,11 @@ public class MultiPartTest MultiPart.Part part1 = listener.parts.get(0); assertEquals("value", part1.getHeaders().get("name")); - assertEquals("Hello", Content.Source.asString(part1.getNewContent())); + assertEquals("Hello", Content.Source.asString(part1.newContentSource())); MultiPart.Part part2 = listener.parts.get(1); assertEquals("9001", part2.getHeaders().get("powerLevel")); - assertEquals("secondary\r\ncontent", Content.Source.asString(part2.getNewContent())); + assertEquals("secondary\r\ncontent", Content.Source.asString(part2.newContentSource())); assertEquals(0, data.remaining()); } @@ -397,11 +397,11 @@ public class MultiPartTest MultiPart.Part part1 = listener.parts.get(0); assertEquals("value", part1.getHeaders().get("name")); - assertEquals("Hello", Content.Source.asString(part1.getNewContent())); + assertEquals("Hello", Content.Source.asString(part1.newContentSource())); MultiPart.Part part2 = listener.parts.get(1); assertEquals("9001", part2.getHeaders().get("powerLevel")); - assertEquals("secondary\ncontent", Content.Source.asString(part2.getNewContent())); + assertEquals("secondary\ncontent", Content.Source.asString(part2.newContentSource())); assertEquals(0, data.remaining()); } @@ -457,7 +457,7 @@ public class MultiPartTest assertEquals(1, listener.parts.size()); MultiPart.Part part = listener.parts.get(0); assertEquals("value", part.getHeaders().get("name")); - assertEquals("", Content.Source.asString(part.getNewContent())); + assertEquals("", Content.Source.asString(part.newContentSource())); } @Test @@ -477,7 +477,7 @@ public class MultiPartTest assertEquals(1, listener.parts.size()); MultiPart.Part part = listener.parts.get(0); assertEquals("value", part.getHeaders().get("name")); - assertEquals("", Content.Source.asString(part.getNewContent())); + assertEquals("", Content.Source.asString(part.newContentSource())); } @Test @@ -508,7 +508,7 @@ public class MultiPartTest assertEquals(1, listener.parts.size()); MultiPart.Part part = listener.parts.get(0); assertEquals("value", part.getHeaders().get("name")); - assertThat(Content.Source.asString(part.getNewContent()), is(""" + assertThat(Content.Source.asString(part.newContentSource()), is(""" Hello\r this is not a --BOUNDARY\r that's a boundary""")); @@ -532,7 +532,7 @@ public class MultiPartTest assertThat(epilogueBuffer.remaining(), is(0)); assertEquals(1, listener.parts.size()); MultiPart.Part part = listener.parts.get(0); - assertThat(Content.Source.asByteBuffer(part.getNewContent()), is(ByteBuffer.wrap(random))); + assertThat(Content.Source.asByteBuffer(part.newContentSource()), is(ByteBuffer.wrap(random))); } @Test @@ -556,7 +556,7 @@ public class MultiPartTest assertEquals(1, listener.parts.size()); MultiPart.Part part = listener.parts.get(0); assertEquals("value", part.getHeaders().get("name")); - assertEquals("Hello", Content.Source.asString(part.getNewContent())); + assertEquals("Hello", Content.Source.asString(part.newContentSource())); } @Test diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/DelayedHandler.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/DelayedHandler.java index e862905048a..5680b82e4a7 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/DelayedHandler.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/DelayedHandler.java @@ -269,13 +269,13 @@ public class DelayedHandler extends Handler.Wrapper super(handler, wrapped, response, callback); String boundary = MultiPart.extractBoundary(contentType); _formData = boundary == null ? null : new MultiPartFormData(boundary); - getRequest().setAttribute(MultiPartFormData.class.getName(), _formData); } private void process(MultiPartFormData.Parts parts, Throwable x) { if (x == null) { + getRequest().setAttribute(MultiPartFormData.Parts.class.getName(), parts); super.process(); } else diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java index ca5a944153d..17384c26355 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java @@ -34,7 +34,7 @@ import org.eclipse.jetty.http.HttpStatus; import org.eclipse.jetty.http.HttpURI; import org.eclipse.jetty.http.HttpVersion; import org.eclipse.jetty.http.MetaData; -import org.eclipse.jetty.http.MultiPartFormData; +import org.eclipse.jetty.http.MultiPartFormData.Parts; import org.eclipse.jetty.http.PreEncodedHttpField; import org.eclipse.jetty.http.Trailers; import org.eclipse.jetty.http.UriCompliance; @@ -649,9 +649,9 @@ public class HttpChannelState implements HttpChannel, Components } // Clean up any multipart tmp files and release any associated resources. - MultiPartFormData multiParts = (MultiPartFormData)_request.getAttribute(MultiPartFormData.class.getName()); - if (multiParts != null) - multiParts.close(); + Parts parts = (Parts)_request.getAttribute(Parts.class.getName()); + if (parts != null) + parts.close(); } finally { diff --git a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/MultiPartByteRangesTest.java b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/MultiPartByteRangesTest.java index f955f5ea909..204c552381b 100644 --- a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/MultiPartByteRangesTest.java +++ b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/MultiPartByteRangesTest.java @@ -119,11 +119,11 @@ public class MultiPartByteRangesTest assertEquals(3, parts.size()); MultiPart.Part part1 = parts.get(0); - assertEquals("12", Content.Source.asString(part1.getNewContent())); + assertEquals("12", Content.Source.asString(part1.newContentSource())); MultiPart.Part part2 = parts.get(1); - assertEquals("456", Content.Source.asString(part2.getNewContent())); + assertEquals("456", Content.Source.asString(part2.newContentSource())); MultiPart.Part part3 = parts.get(2); - assertEquals("CDEF", Content.Source.asString(part3.getNewContent())); + assertEquals("CDEF", Content.Source.asString(part3.newContentSource())); } } } diff --git a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/handler/MultiPartFormDataHandlerTest.java b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/handler/MultiPartFormDataHandlerTest.java index a365abed169..cf8cba7064b 100644 --- a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/handler/MultiPartFormDataHandlerTest.java +++ b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/handler/MultiPartFormDataHandlerTest.java @@ -80,7 +80,7 @@ public class MultiPartFormDataHandlerTest .whenComplete((parts, failure) -> { if (parts != null) - Content.copy(parts.get(0).getNewContent(), response, callback); + Content.copy(parts.get(0).newContentSource(), response, callback); else Response.writeError(request, response, callback, failure); }); @@ -126,10 +126,10 @@ public class MultiPartFormDataHandlerTest public boolean process(Request request, Response response, Callback callback) throws Exception { processLatch.countDown(); - MultiPartFormData formData = (MultiPartFormData)request.getAttribute(MultiPartFormData.class.getName()); - assertNotNull(formData); - MultiPart.Part part = formData.get().get(0); - Content.copy(part.getNewContent(), response, callback); + MultiPartFormData.Parts parts = (MultiPartFormData.Parts)request.getAttribute(MultiPartFormData.Parts.class.getName()); + assertNotNull(parts); + MultiPart.Part part = parts.get(0); + Content.copy(part.newContentSource(), response, callback); return true; } }); @@ -195,8 +195,8 @@ public class MultiPartFormDataHandlerTest { if (parts != null) { - response.getHeaders().put(HttpHeader.CONTENT_TYPE, "multipart/form-data; boundary=\"%s\"".formatted(parts.getBoundary())); - MultiPartFormData.ContentSource source = new MultiPartFormData.ContentSource(parts.getBoundary()); + response.getHeaders().put(HttpHeader.CONTENT_TYPE, "multipart/form-data; boundary=\"%s\"".formatted(parts.getMultiPartFormData().getBoundary())); + MultiPartFormData.ContentSource source = new MultiPartFormData.ContentSource(parts.getMultiPartFormData().getBoundary()); source.setPartHeadersMaxLength(1024); parts.forEach(source::addPart); source.close(); @@ -321,7 +321,7 @@ public class MultiPartFormDataHandlerTest HttpFields headers2 = part2.getHeaders(); assertEquals(2, headers2.size()); assertEquals("application/octet-stream", headers2.get(HttpHeader.CONTENT_TYPE)); - assertEquals(32, part2.getNewContent().getLength()); + assertEquals(32, part2.newContentSource().getLength()); } } } diff --git a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/handler/ResourceHandlerByteRangesTest.java b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/handler/ResourceHandlerByteRangesTest.java index 8247eff178a..0495089f89f 100644 --- a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/handler/ResourceHandlerByteRangesTest.java +++ b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/handler/ResourceHandlerByteRangesTest.java @@ -181,10 +181,10 @@ public class ResourceHandlerByteRangesTest assertEquals(2, parts.size()); MultiPart.Part part1 = parts.get(0); assertEquals("text/plain", part1.getHeaders().get(HttpHeader.CONTENT_TYPE)); - assertEquals("234", Content.Source.asString(part1.getNewContent())); + assertEquals("234", Content.Source.asString(part1.newContentSource())); MultiPart.Part part2 = parts.get(1); assertEquals("text/plain", part2.getHeaders().get(HttpHeader.CONTENT_TYPE)); - assertEquals("xyz", Content.Source.asString(part2.getNewContent())); + assertEquals("xyz", Content.Source.asString(part2.newContentSource())); } } } diff --git a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletMultiPartFormData.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletMultiPartFormData.java index 28ad413be6f..22680abb252 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletMultiPartFormData.java +++ b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletMultiPartFormData.java @@ -77,9 +77,9 @@ public class ServletMultiPartFormData try { // Look for a previously read and parsed MultiPartFormData from the DelayedHandler. - MultiPartFormData formData = (MultiPartFormData)request.getAttribute(MultiPartFormData.class.getName()); - if (formData != null) - return new Parts(formData); + MultiPartFormData.Parts parts = (MultiPartFormData.Parts)request.getAttribute(MultiPartFormData.Parts.class.getName()); + if (parts != null) + return new Parts(parts); // TODO set the files directory return new ServletMultiPartFormData().parse(request, maxParts); @@ -102,7 +102,6 @@ public class ServletMultiPartFormData // Store MultiPartFormData as attribute on request so it is released by the HttpChannel. MultiPartFormData formData = new MultiPartFormData(boundary); - request.setAttribute(MultiPartFormData.class.getName(), formData); formData.setMaxParts(maxParts); File tmpDirFile = (File)request.getServletContext().getAttribute(ServletContext.TEMPDIR); @@ -123,7 +122,7 @@ public class ServletMultiPartFormData Connection connection = connectionMetaData.getConnection(); int bufferSize = connection instanceof AbstractConnection c ? c.getInputBufferSize() : 2048; InputStream input = request.getInputStream(); - while (true) + while (!formData.isDone()) { ByteBuffer buffer = byteBufferPool.newByteBuffer(bufferSize, false); boolean readEof = false; @@ -145,7 +144,9 @@ public class ServletMultiPartFormData } } - return new Parts(formData); + Parts parts = new Parts(formData.join()); + request.setAttribute(Parts.class.getName(), parts); + return parts; } /** @@ -155,9 +156,9 @@ public class ServletMultiPartFormData { private final List parts = new ArrayList<>(); - public Parts(MultiPartFormData formData) + public Parts(MultiPartFormData.Parts parts) { - formData.join().forEach(part -> parts.add(new ServletPart(formData, part))); + parts.forEach(part -> this.parts.add(new ServletPart(parts.getMultiPartFormData(), part))); } public Part getPart(String name) @@ -190,7 +191,7 @@ public class ServletMultiPartFormData @Override public InputStream getInputStream() throws IOException { - return Content.Source.asInputStream(_part.getNewContent()); + return Content.Source.asInputStream(_part.newContentSource()); } @Override diff --git a/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/MultiPartServletTest.java b/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/MultiPartServletTest.java index 69230d1827a..1850e9dfd75 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/MultiPartServletTest.java +++ b/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/MultiPartServletTest.java @@ -129,10 +129,6 @@ public class MultiPartServletTest protected void service(HttpServletRequest req, HttpServletResponse resp) throws ServletException, IOException { req.getParameterMap(); - req.getParts(); - resp.setStatus(200); - resp.getWriter().print("success"); - resp.getWriter().close(); } }, new MultipartConfigElement(tmpDirString)); @@ -174,10 +170,6 @@ public class MultiPartServletTest protected void service(HttpServletRequest req, HttpServletResponse resp) throws ServletException, IOException { req.getParameterMap(); - req.getParts(); - resp.setStatus(200); - resp.getWriter().print("success"); - resp.getWriter().close(); } }, new MultipartConfigElement(tmpDirString)); @@ -216,10 +208,6 @@ public class MultiPartServletTest protected void service(HttpServletRequest req, HttpServletResponse resp) throws ServletException, IOException { req.getParameterMap(); - req.getParts(); - resp.setStatus(200); - resp.getWriter().print("success"); - resp.getWriter().close(); } }, new MultipartConfigElement(tmpDirString, -1, 1024, 1024 * 1024 * 8)); From d4682f4b85c3e66981bb0544b795db185f8909f9 Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Wed, 25 Jan 2023 10:39:20 +1100 Subject: [PATCH 08/22] Fix testMaxRequest size test in MultiPartServletTest Signed-off-by: Lachlan Roberts --- .../client/InputStreamResponseListener.java | 2 +- .../jetty/client/MultiPartRequestContent.java | 2 +- .../server/internal/HttpStreamOverFCGI.java | 6 +++ .../org/eclipse/jetty/http/MultiPart.java | 50 ++++++++++--------- .../jetty/http/MultiPartByteRanges.java | 13 +---- .../eclipse/jetty/http/MultiPartFormData.java | 11 +--- .../server/internal/HttpStreamOverHTTP2.java | 2 +- .../server/internal/HttpStreamOverHTTP3.java | 8 +++ .../jetty/server/HttpConfiguration.java | 23 +++++++++ .../org/eclipse/jetty/server/HttpStream.java | 13 +++-- .../org/eclipse/jetty/server/Request.java | 8 +++ .../org/eclipse/jetty/server/Response.java | 26 +++------- .../jetty/server/handler/DelayedHandler.java | 2 +- .../server/internal/HttpChannelState.java | 13 +++++ .../jetty/server/internal/HttpConnection.java | 12 +++++ .../eclipse/jetty/server/MockHttpStream.java | 6 +++ .../jetty/session/TestableRequest.java | 6 +++ .../ee10/servlet/AsyncContentProducer.java | 21 ++------ .../jetty/ee10/servlet/ServletChannel.java | 3 +- .../ee10/servlet/MultiPartServletTest.java | 30 +++++++---- .../eclipse/jetty/ee9/nested/RequestTest.java | 6 +++ .../jetty/ee9/nested/ResponseTest.java | 6 +++ 22 files changed, 169 insertions(+), 100 deletions(-) diff --git a/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/InputStreamResponseListener.java b/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/InputStreamResponseListener.java index ffaca7eb256..922773902b8 100644 --- a/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/InputStreamResponseListener.java +++ b/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/InputStreamResponseListener.java @@ -302,7 +302,7 @@ public class InputStreamResponseListener extends Listener.Adapter break; if (failure != null) - throw toIOException(failure); + throw new IOException(toIOException(failure)); if (closed) throw new AsynchronousCloseException(); diff --git a/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/MultiPartRequestContent.java b/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/MultiPartRequestContent.java index 8d19c31704a..b9105fcc487 100644 --- a/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/MultiPartRequestContent.java +++ b/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/MultiPartRequestContent.java @@ -69,7 +69,7 @@ public class MultiPartRequestContent extends MultiPartFormData.ContentSource imp if (headers.contains(HttpHeader.CONTENT_TYPE)) return headers; - Content.Source partContent = part.newContentSource(); + Content.Source partContent = part.getContentSource(); if (partContent instanceof Request.Content requestContent) { String contentType = requestContent.getContentType(); diff --git a/jetty-core/jetty-fcgi/jetty-fcgi-server/src/main/java/org/eclipse/jetty/fcgi/server/internal/HttpStreamOverFCGI.java b/jetty-core/jetty-fcgi/jetty-fcgi-server/src/main/java/org/eclipse/jetty/fcgi/server/internal/HttpStreamOverFCGI.java index 885d6924c87..2eed78b6b2c 100644 --- a/jetty-core/jetty-fcgi/jetty-fcgi-server/src/main/java/org/eclipse/jetty/fcgi/server/internal/HttpStreamOverFCGI.java +++ b/jetty-core/jetty-fcgi/jetty-fcgi-server/src/main/java/org/eclipse/jetty/fcgi/server/internal/HttpStreamOverFCGI.java @@ -300,6 +300,12 @@ public class HttpStreamOverFCGI implements HttpStream return _committed; } + @Override + public Throwable consumeAvailable() + { + return HttpStream.consumeAvailable(this, _httpChannel.getConnectionMetaData().getHttpConfiguration()); + } + @Override public void succeeded() { diff --git a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPart.java b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPart.java index 54a68fe3410..ac6b60cab02 100644 --- a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPart.java +++ b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPart.java @@ -266,10 +266,24 @@ public class MultiPart Files.delete(this.path); } - public void close() throws IOException + public void close() { - if (temporary) - delete(); + try + { + if (temporary) + delete(); + } + catch (Throwable t) + { + if (LOG.isDebugEnabled()) + LOG.debug("Error closing part {}", this, t); + } + } + + public void fail(Throwable t) + { + getContentSource().fail(t); + close(); } } @@ -348,7 +362,7 @@ public class MultiPart } @Override - public void close() throws IOException + public void close() { super.close(); content.forEach(Retainable::release); @@ -425,7 +439,7 @@ public class MultiPart public ContentSourcePart(String name, String fileName, HttpFields fields, Content.Source content) { super(name, fileName, fields); - this.content = content; + this.content = Objects.requireNonNull(content); } @Override @@ -501,7 +515,6 @@ public class MultiPart private Runnable demand; private Content.Chunk.Error errorChunk; private Part part; - private Content.Source partContent; public AbstractContentSource(String boundary) { @@ -631,7 +644,6 @@ public class MultiPart else { part = parts.poll(); - partContent = part.newContentSource(); state = State.HEADERS; yield Content.Chunk.from(firstBoundary.slice(), false); } @@ -640,7 +652,6 @@ public class MultiPart case MIDDLE -> { part = null; - partContent = null; try (AutoLock ignored = lock.lock()) { if (parts.isEmpty()) @@ -658,7 +669,6 @@ public class MultiPart else { part = parts.poll(); - partContent = part.newContentSource(); state = State.HEADERS; yield Content.Chunk.from(middleBoundary.slice(), false); } @@ -698,7 +708,7 @@ public class MultiPart } case CONTENT -> { - Content.Chunk chunk = partContent.read(); + Content.Chunk chunk = part.getContentSource().read(); if (chunk == null || chunk instanceof Content.Chunk.Error) yield chunk; if (!chunk.isLast()) @@ -758,29 +768,21 @@ public class MultiPart @Override public void fail(Throwable failure) { + Part part; List drained; try (AutoLock ignored = lock.lock()) { - if (closed && parts.isEmpty()) - return; if (errorChunk != null) return; errorChunk = Content.Chunk.from(failure); drained = List.copyOf(parts); parts.clear(); + part = this.part; + this.part = null; } - drained.forEach(part -> - { - try - { - part.close(); - } - catch (IOException e) - { - if (LOG.isDebugEnabled()) - LOG.debug("Error closing part", e); - } - }); + if (part != null) + part.fail(failure); + drained.forEach(p -> p.fail(failure)); invoker.run(this::invokeDemandCallback); } diff --git a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartByteRanges.java b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartByteRanges.java index 5877599a4f0..3c0d7153d91 100644 --- a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartByteRanges.java +++ b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartByteRanges.java @@ -326,18 +326,7 @@ public class MultiPartByteRanges extends CompletableFuture(parts); parts.clear(); } - toFail.forEach(part -> - { - try - { - part.close(); - } - catch (IOException e) - { - if (LOG.isDebugEnabled()) - LOG.debug("Error clsoing part", e); - } - }); + toFail.forEach(MultiPart.Part::close); } } } diff --git a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartFormData.java b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartFormData.java index 092d5a5b89d..a9a713600ea 100644 --- a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartFormData.java +++ b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartFormData.java @@ -14,7 +14,6 @@ package org.eclipse.jetty.http; import java.io.Closeable; -import java.io.IOException; import java.nio.ByteBuffer; import java.nio.channels.NonWritableChannelException; import java.nio.channels.SeekableByteChannel; @@ -566,15 +565,7 @@ public class MultiPartFormData extends CompletableFutureAn interface that allows a request object to be customized @@ -716,6 +717,28 @@ public class HttpConfiguration implements Dumpable _serverAuthority = authority; } + /** + * Sets the maximum amount of bytes that will be read from the HttpStream if the content is not fully consumed by the + * application. If this is unable to consume to EOF then the connection will be made non-persistent. + * + * @param maxUnconsumedRequestContentBytes the maximum amount of unconsumed bytes that will be read from the HttpStream. + */ + public void setMaxUnconsumedRequestContentBytes(long maxUnconsumedRequestContentBytes) + { + _maxUnconsumedRequestContentBytes = maxUnconsumedRequestContentBytes; + } + + /** + * Gets the maximum amount of bytes that will be read from the HttpStream if the content is not fully consumed by the + * application. If this is unable to consume to EOF then the connection will be made non-persistent. + * + * @return the maximum amount of unconsumed bytes that will be read from the HttpStream. + */ + public long getMaxUnconsumedRequestContentBytes() + { + return _maxUnconsumedRequestContentBytes; + } + @Override public String dump() { diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpStream.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpStream.java index 6c228c2be04..0bea0d2a677 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpStream.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpStream.java @@ -104,18 +104,23 @@ public interface HttpStream extends Callback return null; } - default Throwable consumeAvailable() + Throwable consumeAvailable(); + + static Throwable consumeAvailable(HttpStream stream, HttpConfiguration httpConfig) { - while (true) + long consumedRequestContentBytes = 0; + long maxUnconsumedRequestContentBytes = httpConfig.getMaxUnconsumedRequestContentBytes(); + while (consumedRequestContentBytes < maxUnconsumedRequestContentBytes) { // We can always just read again here as EOF and Error content will be persistently returned. - Content.Chunk content = read(); + Content.Chunk content = stream.read(); // if we cannot read to EOF then fail the stream rather than wait for unconsumed content if (content == null) return new IOException("Content not consumed"); // Always release any returned content. This is a noop for EOF and Error content. + consumedRequestContentBytes += content.remaining(); content.release(); // if the input failed, then fail the stream for same reason @@ -125,6 +130,8 @@ public interface HttpStream extends Callback if (content.isLast()) return null; } + + return new IOException("Content not consumed"); } class Wrapper implements HttpStream diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java index 926b1733566..fb656212221 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java @@ -228,6 +228,8 @@ public interface Request extends Attributes, Content.Source @Override Content.Chunk read(); + boolean consumeAvailable(); + /** *

Pushes the given {@code resource} to the client.

* @@ -607,6 +609,12 @@ public interface Request extends Attributes, Content.Source return getWrapped().read(); } + @Override + public boolean consumeAvailable() + { + return getWrapped().consumeAvailable(); + } + @Override public void demand(Runnable demandCallback) { diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Response.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Response.java index 80c211bb92c..fbc1d44c5de 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Response.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Response.java @@ -315,13 +315,17 @@ public interface Response extends Content.Sink } static void ensureConsumeAvailableOrNotPersistent(Request request, Response response) + { + if (request.consumeAvailable()) + return; + ensureNotPersistent(request, response); + } + + static void ensureNotPersistent(Request request, Response response) { switch (request.getConnectionMetaData().getHttpVersion()) { case HTTP_1_0: - if (consumeAvailable(request)) - return; - // Remove any keep-alive value in Connection headers response.getHeaders().computeField(HttpHeader.CONNECTION, (h, fields) -> { @@ -338,9 +342,6 @@ public interface Response extends Content.Sink break; case HTTP_1_1: - if (consumeAvailable(request)) - return; - // Add close value to Connection headers response.getHeaders().computeField(HttpHeader.CONNECTION, (h, fields) -> { @@ -374,19 +375,6 @@ public interface Response extends Content.Sink } } - static boolean consumeAvailable(Request request) - { - while (true) - { - Content.Chunk chunk = request.read(); - if (chunk == null) - return false; - chunk.release(); - if (chunk.isLast()) - return true; - } - } - class Wrapper implements Response { private final Request _request; diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/DelayedHandler.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/DelayedHandler.java index 8c8c13f6eae..97770f7fd28 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/DelayedHandler.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/DelayedHandler.java @@ -319,7 +319,7 @@ public class DelayedHandler extends Handler.Wrapper private void readAndParse() { - while (true) + while (!_formData.isDone()) { Content.Chunk chunk = getRequest().read(); if (chunk == null) diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java index 17384c26355..d1883fafa0d 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java @@ -879,6 +879,19 @@ public class HttpChannelState implements HttpChannel, Components return chunk; } + @Override + public boolean consumeAvailable() + { + HttpStream stream; + try (AutoLock ignored = _lock.lock()) + { + HttpChannelState httpChannel = lockedGetHttpChannel(); + stream = httpChannel._stream; + } + + return stream.consumeAvailable() != null; + } + @Override public void demand(Runnable demandCallback) { diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpConnection.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpConnection.java index 2a5196447b2..a4ec39834a6 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpConnection.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpConnection.java @@ -53,6 +53,7 @@ import org.eclipse.jetty.io.EndPoint; import org.eclipse.jetty.io.EofException; import org.eclipse.jetty.io.RetainableByteBuffer; import org.eclipse.jetty.io.RetainableByteBufferPool; +import org.eclipse.jetty.io.RuntimeIOException; import org.eclipse.jetty.io.WriteFlusher; import org.eclipse.jetty.io.ssl.SslConnection; import org.eclipse.jetty.server.ConnectionFactory; @@ -597,6 +598,8 @@ public class HttpConnection extends AbstractConnection implements Runnable, Writ if (LOG.isDebugEnabled()) LOG.debug("{} parse {}", this, _retainableByteBuffer); + if (_parser.isTerminated()) + throw new RuntimeIOException("Parser is terminated"); boolean handle = _parser.parseNext(_retainableByteBuffer == null ? BufferUtil.EMPTY_BUFFER : _retainableByteBuffer.getBuffer()); if (LOG.isDebugEnabled()) @@ -1136,6 +1139,15 @@ public class HttpConnection extends AbstractConnection implements Runnable, Writ _uri.path("/"); } + @Override + public Throwable consumeAvailable() + { + Throwable result = HttpStream.consumeAvailable(this, getHttpConfiguration()); + if (result != null) + _generator.setPersistent(false); + return result; + } + public void parsedHeader(HttpField field) { HttpHeader header = field.getHeader(); diff --git a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/MockHttpStream.java b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/MockHttpStream.java index a2ccaf1f004..96e07c67e07 100644 --- a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/MockHttpStream.java +++ b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/MockHttpStream.java @@ -225,6 +225,12 @@ public class MockHttpStream implements HttpStream return response != null && response.getStatus() >= 200; } + @Override + public Throwable consumeAvailable() + { + return null; + } + public boolean isComplete() { return _completed.getCount() == 0; diff --git a/jetty-core/jetty-session/src/test/java/org/eclipse/jetty/session/TestableRequest.java b/jetty-core/jetty-session/src/test/java/org/eclipse/jetty/session/TestableRequest.java index c58e9655d39..385463af2c5 100644 --- a/jetty-core/jetty-session/src/test/java/org/eclipse/jetty/session/TestableRequest.java +++ b/jetty-core/jetty-session/src/test/java/org/eclipse/jetty/session/TestableRequest.java @@ -143,6 +143,12 @@ public class TestableRequest implements Request return null; } + @Override + public boolean consumeAvailable() + { + return false; + } + @Override public void demand(Runnable demandCallback) { diff --git a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/AsyncContentProducer.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/AsyncContentProducer.java index 39f809480e5..8979484c9a1 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/AsyncContentProducer.java +++ b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/AsyncContentProducer.java @@ -60,11 +60,9 @@ class AsyncContentProducer implements ContentProducer // Make sure that asking this instance for chunks between // recycle() and reopen() will only produce error chunks. - if (_chunk == null) - _chunk = RECYCLED_ERROR_CHUNK; - // The chunk must be fully consumed. - else if (!_chunk.isLast() || _chunk.hasRemaining()) - throw new IllegalStateException("ContentProducer with unconsumed chunk cannot be recycled"); + if (_chunk != null) + _chunk.release(); + _chunk = RECYCLED_ERROR_CHUNK; } @Override @@ -182,18 +180,7 @@ class AsyncContentProducer implements ContentProducer private boolean consumeAvailableChunks() { - ServletContextRequest request = _servletChannel.getServletContextRequest(); - while (true) - { - Content.Chunk chunk = request.read(); - if (chunk == null) - return false; - - chunk.release(); - - if (chunk.isLast()) - return true; - } + return _servletChannel.getServletContextRequest().consumeAvailable(); } @Override diff --git a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java index f35020774af..8f52fde2c40 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java +++ b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java @@ -489,7 +489,8 @@ public class ServletChannel // from the failed dispatch, then we try to consume it here and if we fail we add a // Connection:close. This can't be deferred to COMPLETE as the response will be committed // by then. - Response.ensureConsumeAvailableOrNotPersistent(_servletContextRequest, _servletContextRequest.getResponse()); + if (!_httpInput.consumeAvailable()) + Response.ensureNotPersistent(_servletContextRequest, _servletContextRequest.getResponse()); ContextHandler.ScopedContext context = (ContextHandler.ScopedContext)_servletContextRequest.getAttribute(ErrorHandler.ERROR_CONTEXT); Request.Processor errorProcessor = ErrorHandler.getErrorProcessor(getServer(), context == null ? null : context.getContextHandler()); diff --git a/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/MultiPartServletTest.java b/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/MultiPartServletTest.java index d6f2fdc283b..39f4a772ca4 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/MultiPartServletTest.java +++ b/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/MultiPartServletTest.java @@ -48,6 +48,7 @@ import org.eclipse.jetty.http.HttpTester; import org.eclipse.jetty.http.MultiPart; import org.eclipse.jetty.http.MultiPartFormData; import org.eclipse.jetty.io.Content; +import org.eclipse.jetty.io.EofException; import org.eclipse.jetty.logging.StacklessLogging; import org.eclipse.jetty.server.Server; import org.eclipse.jetty.server.ServerConnector; @@ -62,10 +63,12 @@ import static java.nio.charset.StandardCharsets.UTF_8; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.instanceOf; import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.startsWith; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.fail; public class MultiPartServletTest { @@ -208,7 +211,6 @@ public class MultiPartServletTest protected void service(HttpServletRequest req, HttpServletResponse resp) throws ServletException, IOException { req.getParameterMap(); - } }, new MultipartConfigElement(tmpDirString, -1, 1024, 1024 * 1024 * 8)); @@ -225,20 +227,28 @@ public class MultiPartServletTest .body(multiPart) .send(listener); - // Write large amount of content to the part. - byte[] byteArray = new byte[1024 * 1024]; - Arrays.fill(byteArray, (byte)1); - for (int i = 0; i < 1024 * 2; i++) + Throwable writeError = null; + try { - content.getOutputStream().write(byteArray); + // Write large amount of content to the part. + byte[] byteArray = new byte[1024 * 1024]; + Arrays.fill(byteArray, (byte)1); + for (int i = 0; i < 1024 * 1024; i++) + { + content.getOutputStream().write(byteArray); + } + fail("We should never be able to write all the content."); + } + catch (Exception e) + { + writeError = e; } - content.close(); + assertThat(writeError, instanceOf(EofException.class)); + + // We should get 400 response, for some reason reading the content throws EofException. Response response = listener.get(30, TimeUnit.SECONDS); assertThat(response.getStatus(), equalTo(HttpStatus.BAD_REQUEST_400)); - String responseContent = IO.toString(listener.getInputStream()); - assertThat(responseContent, containsString("Unable to parse form content")); - assertThat(responseContent, containsString("max length exceeded")); } @Test diff --git a/jetty-ee9/jetty-ee9-nested/src/test/java/org/eclipse/jetty/ee9/nested/RequestTest.java b/jetty-ee9/jetty-ee9-nested/src/test/java/org/eclipse/jetty/ee9/nested/RequestTest.java index fa239aed722..1f9a2fd6aad 100644 --- a/jetty-ee9/jetty-ee9-nested/src/test/java/org/eclipse/jetty/ee9/nested/RequestTest.java +++ b/jetty-ee9/jetty-ee9-nested/src/test/java/org/eclipse/jetty/ee9/nested/RequestTest.java @@ -2345,6 +2345,12 @@ public class RequestTest return null; } + @Override + public boolean consumeAvailable() + { + return false; + } + @Override public void demand(Runnable demandCallback) { diff --git a/jetty-ee9/jetty-ee9-nested/src/test/java/org/eclipse/jetty/ee9/nested/ResponseTest.java b/jetty-ee9/jetty-ee9-nested/src/test/java/org/eclipse/jetty/ee9/nested/ResponseTest.java index 30026fab46e..0e356336114 100644 --- a/jetty-ee9/jetty-ee9-nested/src/test/java/org/eclipse/jetty/ee9/nested/ResponseTest.java +++ b/jetty-ee9/jetty-ee9-nested/src/test/java/org/eclipse/jetty/ee9/nested/ResponseTest.java @@ -2348,6 +2348,12 @@ public class ResponseTest return Content.Chunk.EOF; } + @Override + public boolean consumeAvailable() + { + return true; + } + @Override public void demand(Runnable demandCallback) { From 2dfe7e60210cd65896fa6f9c36a29810df591282 Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Wed, 25 Jan 2023 10:49:30 +1100 Subject: [PATCH 09/22] fix some more tests in MultiPartServletTest Signed-off-by: Lachlan Roberts --- .../org/eclipse/jetty/http/MultiPart.java | 4 ++-- .../java/org/eclipse/jetty/io/Retainable.java | 20 +++++++++++++++++++ .../ee10/servlet/MultiPartServletTest.java | 1 + 3 files changed, 23 insertions(+), 2 deletions(-) diff --git a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPart.java b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPart.java index ac6b60cab02..36c23a4e472 100644 --- a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPart.java +++ b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPart.java @@ -358,7 +358,7 @@ public class MultiPart public Content.Source newContentSource() { return new ChunksContentSource(content.stream().map(c -> - Content.Chunk.asChunk(c.getByteBuffer().slice(), c.isLast(), c)).toList()); + Content.Chunk.asChunk(c.getByteBuffer().slice(), c.isLast(), Retainable.NOOP)).toList()); } @Override @@ -954,7 +954,7 @@ public class MultiPart else if (type == HttpTokens.Type.LF) { numParts++; - if (numParts >= maxParts) + if (numParts > maxParts) throw new IllegalStateException(String.format("Form with too many keys [%d > %d]", numParts, maxParts)); notifyPartBegin(); diff --git a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/Retainable.java b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/Retainable.java index d8e7dcabff5..1d99f5fce79 100644 --- a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/Retainable.java +++ b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/Retainable.java @@ -26,6 +26,26 @@ import java.util.concurrent.atomic.AtomicInteger; */ public interface Retainable { + Retainable NOOP = new Retainable() + { + @Override + public boolean canRetain() + { + return true; + } + + @Override + public void retain() + { + } + + @Override + public boolean release() + { + return true; + } + }; + /** *

Returns whether this resource is referenced counted by calls to {@link #retain()} * and {@link #release()}.

diff --git a/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/MultiPartServletTest.java b/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/MultiPartServletTest.java index 39f4a772ca4..bc6d2277e64 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/MultiPartServletTest.java +++ b/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/MultiPartServletTest.java @@ -379,6 +379,7 @@ public class MultiPartServletTest String contentType = headers.get(HttpHeader.CONTENT_TYPE); String boundary = MultiPart.extractBoundary(contentType); MultiPartFormData formData = new MultiPartFormData(boundary); + formData.setMaxParts(1); InputStream inputStream = new GZIPInputStream(responseStream.getInputStream()); formData.parse(Content.Chunk.from(ByteBuffer.wrap(IO.readBytes(inputStream)), true)); From 738712438cbb64bc7cd6a7d745901d4b91e78307 Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Mon, 30 Jan 2023 13:20:51 +1100 Subject: [PATCH 10/22] changes from review Signed-off-by: Lachlan Roberts --- .../util/MultiPartRequestContentTest.java | 22 +++++----- .../org/eclipse/jetty/http/MultiPart.java | 11 ++--- .../jetty/http/MultiPartCaptureTest.java | 10 ++--- .../jetty/http/MultiPartFormDataTest.java | 20 ++++----- .../org/eclipse/jetty/http/MultiPartTest.java | 18 ++++---- .../jetty/server/MultiPartByteRangesTest.java | 6 +-- .../handler/MultiPartFormDataHandlerTest.java | 6 +-- .../ResourceHandlerByteRangesTest.java | 4 +- .../ee10/servlet/ServletContextRequest.java | 6 --- .../servlet/ServletMultiPartFormData.java | 5 +-- .../ee10/servlet/MultiPartServletTest.java | 44 +++++++++++++++++++ 11 files changed, 94 insertions(+), 58 deletions(-) diff --git a/jetty-core/jetty-client/src/test/java/org/eclipse/jetty/client/util/MultiPartRequestContentTest.java b/jetty-core/jetty-client/src/test/java/org/eclipse/jetty/client/util/MultiPartRequestContentTest.java index 65cd18799ed..8eabdf7be03 100644 --- a/jetty-core/jetty-client/src/test/java/org/eclipse/jetty/client/util/MultiPartRequestContentTest.java +++ b/jetty-core/jetty-client/src/test/java/org/eclipse/jetty/client/util/MultiPartRequestContentTest.java @@ -135,7 +135,7 @@ public class MultiPartRequestContentTest extends AbstractHttpClientServerTest int equal = contentType.lastIndexOf('='); Charset charset = Charset.forName(contentType.substring(equal + 1)); assertEquals(encoding, charset); - assertEquals(value, Content.Source.asString(part.newContentSource(), charset)); + assertEquals(value, Content.Source.asString(part.getContentSource(), charset)); } }); @@ -169,7 +169,7 @@ public class MultiPartRequestContentTest extends AbstractHttpClientServerTest MultiPart.Part part = parts.iterator().next(); assertEquals(name, part.getName()); assertEquals("text/plain", part.getHeaders().get(HttpHeader.CONTENT_TYPE)); - assertArrayEquals(data, Content.Source.asByteBuffer(part.newContentSource()).array()); + assertArrayEquals(data, Content.Source.asByteBuffer(part.getContentSource()).array()); } }); @@ -221,8 +221,8 @@ public class MultiPartRequestContentTest extends AbstractHttpClientServerTest assertEquals(name, part.getName()); assertEquals(contentType, part.getHeaders().get(HttpHeader.CONTENT_TYPE)); assertEquals(fileName, part.getFileName()); - assertEquals(data.length, part.newContentSource().getLength()); - assertArrayEquals(data, Content.Source.asByteBuffer(part.newContentSource()).array()); + assertEquals(data.length, part.getContentSource().getLength()); + assertArrayEquals(data, Content.Source.asByteBuffer(part.getContentSource()).array()); } }); @@ -277,8 +277,8 @@ public class MultiPartRequestContentTest extends AbstractHttpClientServerTest assertEquals(name, part.getName()); assertEquals(contentType, part.getHeaders().get(HttpHeader.CONTENT_TYPE)); assertEquals(tmpPath.getFileName().toString(), part.getFileName()); - assertEquals(Files.size(tmpPath), part.newContentSource().getLength()); - assertEquals(data, Content.Source.asString(part.newContentSource(), encoding)); + assertEquals(Files.size(tmpPath), part.getContentSource().getLength()); + assertEquals(data, Content.Source.asString(part.getContentSource(), encoding)); } }); @@ -329,14 +329,14 @@ public class MultiPartRequestContentTest extends AbstractHttpClientServerTest assertEquals(field, fieldPart.getName()); assertEquals(contentType, fieldPart.getHeaders().get(HttpHeader.CONTENT_TYPE)); - assertEquals(value, Content.Source.asString(fieldPart.newContentSource(), encoding)); + assertEquals(value, Content.Source.asString(fieldPart.getContentSource(), encoding)); assertEquals(headerValue, fieldPart.getHeaders().get(headerName)); assertEquals(fileField, filePart.getName()); assertEquals("application/octet-stream", filePart.getHeaders().get(HttpHeader.CONTENT_TYPE)); assertEquals(tmpPath.getFileName().toString(), filePart.getFileName()); - assertEquals(Files.size(tmpPath), filePart.newContentSource().getLength()); - assertArrayEquals(data, Content.Source.asByteBuffer(filePart.newContentSource()).array()); + assertEquals(Files.size(tmpPath), filePart.getContentSource().getLength()); + assertArrayEquals(data, Content.Source.asByteBuffer(filePart.getContentSource()).array()); } }); @@ -373,11 +373,11 @@ public class MultiPartRequestContentTest extends AbstractHttpClientServerTest MultiPart.Part fieldPart = parts.get(0); MultiPart.Part filePart = parts.get(1); - assertEquals(value, Content.Source.asString(fieldPart.newContentSource(), encoding)); + assertEquals(value, Content.Source.asString(fieldPart.getContentSource(), encoding)); assertEquals("file", filePart.getName()); assertEquals("application/octet-stream", filePart.getHeaders().get(HttpHeader.CONTENT_TYPE)); assertEquals("fileName", filePart.getFileName()); - assertArrayEquals(fileData, Content.Source.asByteBuffer(filePart.newContentSource()).array()); + assertArrayEquals(fileData, Content.Source.asByteBuffer(filePart.getContentSource()).array()); } }); diff --git a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPart.java b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPart.java index 36c23a4e472..e3d67e7038f 100644 --- a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPart.java +++ b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPart.java @@ -172,6 +172,7 @@ public class MultiPart * RFC 7578, section 4.6.

* * @return the content of this part + * @see #newContentSource() */ public Content.Source getContentSource() { @@ -196,7 +197,7 @@ public class MultiPart public long getLength() { - return newContentSource().getLength(); + return getContentSource().getLength(); } /** @@ -846,8 +847,8 @@ public class MultiPart private int trailingWhiteSpaces; private String fieldName; private String fieldValue; - private long maxParts; - private int numParts = 0; + private long maxParts = 1000; + private int numParts; public Parser(String boundary, Listener listener) { @@ -880,7 +881,7 @@ public class MultiPart } /** - * @return the maximum number of parts that can be parsed from the multipart content. + * @return the maximum number of parts that can be parsed from the multipart content (-1 for unlimited parts). */ public long getMaxParts() { @@ -888,7 +889,7 @@ public class MultiPart } /** - * @param maxParts the maximum number of parts that can be parsed from the multipart content. + * @param maxParts the maximum number of parts that can be parsed from the multipart content (-1 for unlimited parts). */ public void setMaxParts(long maxParts) { diff --git a/jetty-core/jetty-http/src/test/java/org/eclipse/jetty/http/MultiPartCaptureTest.java b/jetty-core/jetty-http/src/test/java/org/eclipse/jetty/http/MultiPartCaptureTest.java index 80b9cd74a46..2ff2e3d9ff9 100644 --- a/jetty-core/jetty-http/src/test/java/org/eclipse/jetty/http/MultiPartCaptureTest.java +++ b/jetty-core/jetty-http/src/test/java/org/eclipse/jetty/http/MultiPartCaptureTest.java @@ -245,7 +245,7 @@ public class MultiPartCaptureTest List charSetParts = allParts.get("_charset_"); if (charSetParts != null) { - defaultCharset = Promise.Completable.with(p -> Content.Source.asString(charSetParts.get(0).newContentSource(), StandardCharsets.US_ASCII, p)) + defaultCharset = Promise.Completable.with(p -> Content.Source.asString(charSetParts.get(0).getContentSource(), StandardCharsets.US_ASCII, p)) .get(); } @@ -255,8 +255,8 @@ public class MultiPartCaptureTest assertThat("Part[" + expected.name + "]", parts, is(notNullValue())); MultiPart.Part part = parts.get(0); String charset = getCharsetFromContentType(part.getHeaders().get(HttpHeader.CONTENT_TYPE), defaultCharset); - assertTrue(part.newContentSource().rewind()); - String partContent = Content.Source.asString(part.newContentSource(), Charset.forName(charset)); + assertTrue(part.getContentSource().rewind()); + String partContent = Content.Source.asString(part.getContentSource(), Charset.forName(charset)); assertThat("Part[" + expected.name + "].contents", partContent, containsString(expected.value)); } @@ -276,8 +276,8 @@ public class MultiPartCaptureTest assertThat("Part[" + expected.name + "]", parts, is(notNullValue())); MultiPart.Part part = parts.get(0); MessageDigest digest = MessageDigest.getInstance("SHA1"); - assertTrue(part.newContentSource().rewind()); - try (InputStream partInputStream = Content.Source.asInputStream(part.newContentSource()); + assertTrue(part.getContentSource().rewind()); + try (InputStream partInputStream = Content.Source.asInputStream(part.getContentSource()); DigestOutputStream digester = new DigestOutputStream(OutputStream.nullOutputStream(), digest)) { IO.copy(partInputStream, digester); diff --git a/jetty-core/jetty-http/src/test/java/org/eclipse/jetty/http/MultiPartFormDataTest.java b/jetty-core/jetty-http/src/test/java/org/eclipse/jetty/http/MultiPartFormDataTest.java index 6eeff622a34..2fb00ea3b94 100644 --- a/jetty-core/jetty-http/src/test/java/org/eclipse/jetty/http/MultiPartFormDataTest.java +++ b/jetty-core/jetty-http/src/test/java/org/eclipse/jetty/http/MultiPartFormDataTest.java @@ -189,25 +189,25 @@ public class MultiPartFormDataTest MultiPart.Part fileName = parts.getFirst("fileName"); assertThat(fileName, notNullValue()); - Content.Source partContent = fileName.newContentSource(); + Content.Source partContent = fileName.getContentSource(); assertThat(partContent.getLength(), is(3L)); assertThat(Content.Source.asString(partContent), is("abc")); MultiPart.Part desc = parts.getFirst("desc"); assertThat(desc, notNullValue()); - partContent = desc.newContentSource(); + partContent = desc.getContentSource(); assertThat(partContent.getLength(), is(3L)); assertThat(Content.Source.asString(partContent), is("123")); MultiPart.Part title = parts.getFirst("title"); assertThat(title, notNullValue()); - partContent = title.newContentSource(); + partContent = title.getContentSource(); assertThat(partContent.getLength(), is(3L)); assertThat(Content.Source.asString(partContent), is("ttt")); MultiPart.Part datafile = parts.getFirst("datafile5239138112980980385.txt"); assertThat(datafile, notNullValue()); - partContent = datafile.newContentSource(); + partContent = datafile.getContentSource(); assertThat(partContent.getLength(), is(3L)); assertThat(Content.Source.asString(partContent), is("000")); } @@ -275,11 +275,11 @@ public class MultiPartFormDataTest assertThat(parts.size(), is(2)); MultiPart.Part part1 = parts.getFirst("field1"); assertThat(part1, notNullValue()); - Content.Source partContent = part1.newContentSource(); + Content.Source partContent = part1.getContentSource(); assertThat(Content.Source.asString(partContent), is("Joe Blow")); MultiPart.Part part2 = parts.getFirst("stuff"); assertThat(part2, notNullValue()); - partContent = part2.newContentSource(); + partContent = part2.getContentSource(); assertThat(Content.Source.asString(partContent), is("aaaabbbbb")); } @@ -312,7 +312,7 @@ public class MultiPartFormDataTest assertThat(parts.size(), is(1)); MultiPart.Part part2 = parts.getFirst("stuff"); assertThat(part2, notNullValue()); - Content.Source partContent = part2.newContentSource(); + Content.Source partContent = part2.getContentSource(); assertThat(Content.Source.asString(partContent), is("aaaabbbbb")); } @@ -340,7 +340,7 @@ public class MultiPartFormDataTest assertThat(part, instanceOf(MultiPart.PathPart.class)); MultiPart.PathPart pathPart = (MultiPart.PathPart)part; assertTrue(Files.exists(pathPart.getPath())); - assertEquals("ABCDEFGHIJKLMNOPQRSTUVWXYZ", Content.Source.asString(part.newContentSource())); + assertEquals("ABCDEFGHIJKLMNOPQRSTUVWXYZ", Content.Source.asString(part.getContentSource())); } @Test @@ -422,13 +422,13 @@ public class MultiPartFormDataTest MultiPart.Part part1 = parts.get(0); assertThat(part1, instanceOf(MultiPart.ChunksPart.class)); - assertEquals(chunk, Content.Source.asString(part1.newContentSource())); + assertEquals(chunk, Content.Source.asString(part1.getContentSource())); MultiPart.Part part2 = parts.get(1); assertThat(part2, instanceOf(MultiPart.PathPart.class)); MultiPart.PathPart pathPart2 = (MultiPart.PathPart)part2; assertTrue(Files.exists(pathPart2.getPath())); - assertEquals(chunk.repeat(4), Content.Source.asString(part2.newContentSource())); + assertEquals(chunk.repeat(4), Content.Source.asString(part2.getContentSource())); } @Test diff --git a/jetty-core/jetty-http/src/test/java/org/eclipse/jetty/http/MultiPartTest.java b/jetty-core/jetty-http/src/test/java/org/eclipse/jetty/http/MultiPartTest.java index 0afc87ff85a..d1d4a3524c8 100644 --- a/jetty-core/jetty-http/src/test/java/org/eclipse/jetty/http/MultiPartTest.java +++ b/jetty-core/jetty-http/src/test/java/org/eclipse/jetty/http/MultiPartTest.java @@ -361,11 +361,11 @@ public class MultiPartTest MultiPart.Part part1 = listener.parts.get(0); assertEquals("value", part1.getHeaders().get("name")); - assertEquals("Hello", Content.Source.asString(part1.newContentSource())); + assertEquals("Hello", Content.Source.asString(part1.getContentSource())); MultiPart.Part part2 = listener.parts.get(1); assertEquals("9001", part2.getHeaders().get("powerLevel")); - assertEquals("secondary\r\ncontent", Content.Source.asString(part2.newContentSource())); + assertEquals("secondary\r\ncontent", Content.Source.asString(part2.getContentSource())); assertEquals(0, data.remaining()); } @@ -397,11 +397,11 @@ public class MultiPartTest MultiPart.Part part1 = listener.parts.get(0); assertEquals("value", part1.getHeaders().get("name")); - assertEquals("Hello", Content.Source.asString(part1.newContentSource())); + assertEquals("Hello", Content.Source.asString(part1.getContentSource())); MultiPart.Part part2 = listener.parts.get(1); assertEquals("9001", part2.getHeaders().get("powerLevel")); - assertEquals("secondary\ncontent", Content.Source.asString(part2.newContentSource())); + assertEquals("secondary\ncontent", Content.Source.asString(part2.getContentSource())); assertEquals(0, data.remaining()); } @@ -457,7 +457,7 @@ public class MultiPartTest assertEquals(1, listener.parts.size()); MultiPart.Part part = listener.parts.get(0); assertEquals("value", part.getHeaders().get("name")); - assertEquals("", Content.Source.asString(part.newContentSource())); + assertEquals("", Content.Source.asString(part.getContentSource())); } @Test @@ -477,7 +477,7 @@ public class MultiPartTest assertEquals(1, listener.parts.size()); MultiPart.Part part = listener.parts.get(0); assertEquals("value", part.getHeaders().get("name")); - assertEquals("", Content.Source.asString(part.newContentSource())); + assertEquals("", Content.Source.asString(part.getContentSource())); } @Test @@ -508,7 +508,7 @@ public class MultiPartTest assertEquals(1, listener.parts.size()); MultiPart.Part part = listener.parts.get(0); assertEquals("value", part.getHeaders().get("name")); - assertThat(Content.Source.asString(part.newContentSource()), is(""" + assertThat(Content.Source.asString(part.getContentSource()), is(""" Hello\r this is not a --BOUNDARY\r that's a boundary""")); @@ -532,7 +532,7 @@ public class MultiPartTest assertThat(epilogueBuffer.remaining(), is(0)); assertEquals(1, listener.parts.size()); MultiPart.Part part = listener.parts.get(0); - assertThat(Content.Source.asByteBuffer(part.newContentSource()), is(ByteBuffer.wrap(random))); + assertThat(Content.Source.asByteBuffer(part.getContentSource()), is(ByteBuffer.wrap(random))); } @Test @@ -556,7 +556,7 @@ public class MultiPartTest assertEquals(1, listener.parts.size()); MultiPart.Part part = listener.parts.get(0); assertEquals("value", part.getHeaders().get("name")); - assertEquals("Hello", Content.Source.asString(part.newContentSource())); + assertEquals("Hello", Content.Source.asString(part.getContentSource())); } @Test diff --git a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/MultiPartByteRangesTest.java b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/MultiPartByteRangesTest.java index 204c552381b..96b07fecfb0 100644 --- a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/MultiPartByteRangesTest.java +++ b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/MultiPartByteRangesTest.java @@ -119,11 +119,11 @@ public class MultiPartByteRangesTest assertEquals(3, parts.size()); MultiPart.Part part1 = parts.get(0); - assertEquals("12", Content.Source.asString(part1.newContentSource())); + assertEquals("12", Content.Source.asString(part1.getContentSource())); MultiPart.Part part2 = parts.get(1); - assertEquals("456", Content.Source.asString(part2.newContentSource())); + assertEquals("456", Content.Source.asString(part2.getContentSource())); MultiPart.Part part3 = parts.get(2); - assertEquals("CDEF", Content.Source.asString(part3.newContentSource())); + assertEquals("CDEF", Content.Source.asString(part3.getContentSource())); } } } diff --git a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/handler/MultiPartFormDataHandlerTest.java b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/handler/MultiPartFormDataHandlerTest.java index cf8cba7064b..e7619610811 100644 --- a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/handler/MultiPartFormDataHandlerTest.java +++ b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/handler/MultiPartFormDataHandlerTest.java @@ -80,7 +80,7 @@ public class MultiPartFormDataHandlerTest .whenComplete((parts, failure) -> { if (parts != null) - Content.copy(parts.get(0).newContentSource(), response, callback); + Content.copy(parts.get(0).getContentSource(), response, callback); else Response.writeError(request, response, callback, failure); }); @@ -129,7 +129,7 @@ public class MultiPartFormDataHandlerTest MultiPartFormData.Parts parts = (MultiPartFormData.Parts)request.getAttribute(MultiPartFormData.Parts.class.getName()); assertNotNull(parts); MultiPart.Part part = parts.get(0); - Content.copy(part.newContentSource(), response, callback); + Content.copy(part.getContentSource(), response, callback); return true; } }); @@ -321,7 +321,7 @@ public class MultiPartFormDataHandlerTest HttpFields headers2 = part2.getHeaders(); assertEquals(2, headers2.size()); assertEquals("application/octet-stream", headers2.get(HttpHeader.CONTENT_TYPE)); - assertEquals(32, part2.newContentSource().getLength()); + assertEquals(32, part2.getContentSource().getLength()); } } } diff --git a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/handler/ResourceHandlerByteRangesTest.java b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/handler/ResourceHandlerByteRangesTest.java index 0495089f89f..b4ec605b7cc 100644 --- a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/handler/ResourceHandlerByteRangesTest.java +++ b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/handler/ResourceHandlerByteRangesTest.java @@ -181,10 +181,10 @@ public class ResourceHandlerByteRangesTest assertEquals(2, parts.size()); MultiPart.Part part1 = parts.get(0); assertEquals("text/plain", part1.getHeaders().get(HttpHeader.CONTENT_TYPE)); - assertEquals("234", Content.Source.asString(part1.newContentSource())); + assertEquals("234", Content.Source.asString(part1.getContentSource())); MultiPart.Part part2 = parts.get(1); assertEquals("text/plain", part2.getHeaders().get(HttpHeader.CONTENT_TYPE)); - assertEquals("xyz", Content.Source.asString(part2.newContentSource())); + assertEquals("xyz", Content.Source.asString(part2.getContentSource())); } } } diff --git a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletContextRequest.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletContextRequest.java index 425bbde02c2..94988772cf9 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletContextRequest.java +++ b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletContextRequest.java @@ -13,16 +13,12 @@ package org.eclipse.jetty.ee10.servlet; -import java.io.ByteArrayOutputStream; -import java.io.InputStream; import java.nio.charset.Charset; -import java.nio.charset.StandardCharsets; import java.util.ArrayList; import java.util.EventListener; import java.util.List; import jakarta.servlet.AsyncListener; -import jakarta.servlet.MultipartConfigElement; import jakarta.servlet.ServletRequest; import jakarta.servlet.ServletRequestAttributeListener; import jakarta.servlet.ServletRequestWrapper; @@ -33,13 +29,11 @@ import org.eclipse.jetty.http.HttpMethod; import org.eclipse.jetty.http.pathmap.MatchedPath; import org.eclipse.jetty.http.pathmap.MatchedResource; import org.eclipse.jetty.http.pathmap.PathSpec; -import org.eclipse.jetty.io.RuntimeIOException; import org.eclipse.jetty.server.Request; import org.eclipse.jetty.server.Response; import org.eclipse.jetty.server.handler.ContextHandler; import org.eclipse.jetty.server.handler.ContextRequest; import org.eclipse.jetty.util.Fields; -import org.eclipse.jetty.util.IO; import org.slf4j.Logger; import org.slf4j.LoggerFactory; diff --git a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletMultiPartFormData.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletMultiPartFormData.java index d9cd0fad16b..0e5b06055f3 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletMultiPartFormData.java +++ b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletMultiPartFormData.java @@ -179,13 +179,11 @@ public class ServletMultiPartFormData { private final MultiPartFormData _formData; private final MultiPart.Part _part; - private final long _length; private ServletPart(MultiPartFormData formData, MultiPart.Part part) { _formData = formData; _part = part; - _length = _part.getLength(); } @Override @@ -215,13 +213,12 @@ public class ServletMultiPartFormData @Override public long getSize() { - return _length; + return _part.getLength(); } @Override public void write(String fileName) throws IOException { - // TODO This should simply move a part that is already on the file system. Path filePath = Path.of(fileName); if (!filePath.isAbsolute()) filePath = _formData.getFilesDirectory().resolve(filePath).normalize(); diff --git a/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/MultiPartServletTest.java b/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/MultiPartServletTest.java index bc6d2277e64..e58ef4852ea 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/MultiPartServletTest.java +++ b/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/MultiPartServletTest.java @@ -22,6 +22,7 @@ import java.nio.file.Files; import java.nio.file.Path; import java.util.Arrays; import java.util.Collection; +import java.util.Map; import java.util.concurrent.TimeUnit; import java.util.zip.GZIPInputStream; @@ -65,6 +66,7 @@ import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.instanceOf; import static org.hamcrest.Matchers.is; +import static org.hamcrest.Matchers.not; import static org.hamcrest.Matchers.startsWith; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertNotNull; @@ -423,4 +425,46 @@ public class MultiPartServletTest assertThat(response.getContentAsString(), containsString("Part: name=myPart, size=88, content=the quick brown fox jumps over the lazy dog, the quick brown fox jumps over the lazy dog\n" + "Part: name=myPart, size=88, content=the quick brown fox jumps over the lazy dog, the quick brown fox jumps over the lazy dog")); } + + @Test + public void testPartAsParameter() throws Exception + { + start(new HttpServlet() + { + @Override + protected void doPost(HttpServletRequest req, HttpServletResponse resp) throws ServletException, IOException + { + resp.setContentType("text/plain"); + Map parameterMap = req.getParameterMap(); + for (Map.Entry entry : parameterMap.entrySet()) + { + assertThat(entry.getValue().length, equalTo(1)); + resp.getWriter().println("Parameter: " + entry.getKey() + "=" + entry.getValue()[0]); + } + } + }); + + String contentString = "the quick brown fox jumps over the lazy dog, " + + "the quick brown fox jumps over the lazy dog"; + MultiPartRequestContent multiPart = new MultiPartRequestContent(); + multiPart.addPart(new MultiPart.ContentSourcePart("part1", null, HttpFields.EMPTY, new StringRequestContent(contentString))); + multiPart.addPart(new MultiPart.ContentSourcePart("part2", null, HttpFields.EMPTY, new StringRequestContent(contentString))); + multiPart.addPart(new MultiPart.ContentSourcePart("part3", null, HttpFields.EMPTY, new StringRequestContent(contentString))); + multiPart.addPart(new MultiPart.ContentSourcePart("partFileName", "myFile", HttpFields.EMPTY, new StringRequestContent(contentString))); + multiPart.close(); + + ContentResponse response = client.newRequest("localhost", connector.getLocalPort()) + .scheme(HttpScheme.HTTP.asString()) + .method(HttpMethod.POST) + .body(multiPart) + .send(); + + assertEquals(200, response.getStatus()); + String responseContent = response.getContentAsString(); + assertThat(responseContent, containsString("Parameter: part1=" + contentString)); + assertThat(responseContent, containsString("Parameter: part2=" + contentString)); + assertThat(responseContent, containsString("Parameter: part3=" + contentString)); + assertThat(responseContent, not(containsString("Parameter: partFileName=" + contentString))); + } + } From 719c60d70dd860a30257125a42549d7c28f81d8e Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Tue, 31 Jan 2023 11:42:40 +1100 Subject: [PATCH 11/22] changes from review Signed-off-by: Lachlan Roberts --- .../org/eclipse/jetty/http/MultiPart.java | 102 ++++++++---------- .../jetty/ee10/servlet/ServletApiRequest.java | 13 +-- .../ee10/servlet/MultiPartServletTest.java | 1 - 3 files changed, 44 insertions(+), 72 deletions(-) diff --git a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPart.java b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPart.java index e3d67e7038f..eb22ad5f71d 100644 --- a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPart.java +++ b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPart.java @@ -18,7 +18,6 @@ import java.io.EOFException; import java.io.IOException; import java.io.OutputStream; import java.io.UncheckedIOException; -import java.nio.Buffer; import java.nio.ByteBuffer; import java.nio.charset.Charset; import java.nio.file.Files; @@ -41,6 +40,7 @@ import org.eclipse.jetty.util.BufferUtil; import org.eclipse.jetty.util.IO; import org.eclipse.jetty.util.QuotedStringTokenizer; import org.eclipse.jetty.util.SearchPattern; +import org.eclipse.jetty.util.StaticException; import org.eclipse.jetty.util.StringUtil; import org.eclipse.jetty.util.Utf8StringBuilder; import org.eclipse.jetty.util.thread.AutoLock; @@ -121,18 +121,32 @@ public class MultiPart */ public abstract static class Part { + private static final Throwable SENTINEL_CLOSE_EXCEPTION = new StaticException("Closed"); + private final String name; private final String fileName; private final HttpFields fields; - protected Path path; + private final Content.Source contentSource; + private Path path; private boolean temporary = true; - private Content.Source content; public Part(String name, String fileName, HttpFields fields) + { + this(name, fileName, fields, null); + } + + private Part(String name, String fileName, HttpFields fields, Path path) { this.name = name; this.fileName = fileName; this.fields = fields != null ? fields : HttpFields.EMPTY; + this.path = path; + this.contentSource = newContentSource(); + } + + private Path getPath() + { + return path; } /** @@ -164,7 +178,7 @@ public class MultiPart /** *

Returns the content of this part as a {@link Content.Source}.

- *

Calling this method multiple times will return the same instance.

+ *

Calling this method multiple times will return the same instance, which can only be consumed once.

*

The content type and content encoding are specified in this part's * {@link #getHeaders() headers}.

*

The content encoding may be specified by the part named {@code _charset_}, @@ -176,9 +190,7 @@ public class MultiPart */ public Content.Source getContentSource() { - if (content == null) - content = newContentSource(); - return content; + return contentSource; } /** @@ -246,6 +258,7 @@ public class MultiPart */ public void writeTo(Path path) throws IOException { + this.temporary = false; if (this.path == null) { try (OutputStream out = Files.newOutputStream(path)) @@ -257,7 +270,6 @@ public class MultiPart else { this.path = Files.move(this.path, path, StandardCopyOption.REPLACE_EXISTING); - this.temporary = false; } } @@ -269,22 +281,22 @@ public class MultiPart public void close() { - try - { - if (temporary) - delete(); - } - catch (Throwable t) - { - if (LOG.isDebugEnabled()) - LOG.debug("Error closing part {}", this, t); - } + fail(SENTINEL_CLOSE_EXCEPTION); } public void fail(Throwable t) { - getContentSource().fail(t); - close(); + try + { + getContentSource().fail(t); + if (temporary) + delete(); + } + catch (Throwable x) + { + if (LOG.isDebugEnabled()) + LOG.debug("Error closing part {}", this, x); + } } } @@ -295,7 +307,6 @@ public class MultiPart public static class ByteBufferPart extends Part { private final List content; - private final long length; public ByteBufferPart(String name, String fileName, HttpFields fields, ByteBuffer... buffers) { @@ -305,16 +316,9 @@ public class MultiPart public ByteBufferPart(String name, String fileName, HttpFields fields, List content) { super(name, fileName, fields); - this.length = content.stream().mapToLong(Buffer::remaining).sum(); this.content = content; } - @Override - public long getLength() - { - return length; - } - @Override public Content.Source newContentSource() { @@ -329,7 +333,7 @@ public class MultiPart hashCode(), getName(), getFileName(), - length + getLength() ); } } @@ -340,19 +344,11 @@ public class MultiPart public static class ChunksPart extends Part { private final List content; - private final long length; public ChunksPart(String name, String fileName, HttpFields fields, List content) { super(name, fileName, fields); this.content = content; - this.length = content.stream().mapToLong(c -> c.getByteBuffer().remaining()).sum(); - } - - @Override - public long getLength() - { - return length; } @Override @@ -377,7 +373,7 @@ public class MultiPart hashCode(), getName(), getFileName(), - length + getLength() ); } } @@ -389,32 +385,18 @@ public class MultiPart { public PathPart(String name, String fileName, HttpFields fields, Path path) { - super(name, fileName, fields); - this.path = path; - } - - @Override - public long getLength() - { - try - { - return Files.size(path); - } - catch (IOException x) - { - throw new UncheckedIOException(x); - } + super(name, fileName, fields, path); } public Path getPath() { - return path; + return super.getPath(); } @Override public Content.Source newContentSource() { - return new PathContentSource(path); + return new PathContentSource(getPath()); } @Override @@ -425,7 +407,7 @@ public class MultiPart hashCode(), getName(), getFileName(), - path + getPath() ); } } @@ -459,7 +441,7 @@ public class MultiPart hashCode(), getName(), getFileName(), - content.getLength() + getLength() ); } } @@ -881,7 +863,7 @@ public class MultiPart } /** - * @return the maximum number of parts that can be parsed from the multipart content (-1 for unlimited parts). + * @return the maximum number of parts that can be parsed from the multipart content (0 for no parts allowed, -1 for unlimited parts). */ public long getMaxParts() { @@ -889,7 +871,7 @@ public class MultiPart } /** - * @param maxParts the maximum number of parts that can be parsed from the multipart content (-1 for unlimited parts). + * @param maxParts the maximum number of parts that can be parsed from the multipart content (0 for no parts allowed, -1 for unlimited parts). */ public void setMaxParts(long maxParts) { @@ -955,7 +937,7 @@ public class MultiPart else if (type == HttpTokens.Type.LF) { numParts++; - if (numParts > maxParts) + if (maxParts >= 0 && numParts > maxParts) throw new IllegalStateException(String.format("Form with too many keys [%d > %d]", numParts, maxParts)); notifyPartBegin(); diff --git a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletApiRequest.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletApiRequest.java index 76cb554306e..3a1b79a2e31 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletApiRequest.java +++ b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletApiRequest.java @@ -14,7 +14,6 @@ package org.eclipse.jetty.ee10.servlet; import java.io.BufferedReader; -import java.io.ByteArrayOutputStream; import java.io.IOException; import java.io.InputStream; import java.io.InputStreamReader; @@ -577,9 +576,7 @@ public class ServletApiRequest implements HttpServletRequest { try (InputStream is = charsetPart.getInputStream()) { - ByteArrayOutputStream os = new ByteArrayOutputStream(); - IO.copy(is, os); - formCharset = os.toString(StandardCharsets.UTF_8); + formCharset = IO.toString(is, StandardCharsets.UTF_8); } } @@ -602,7 +599,6 @@ public class ServletApiRequest implements HttpServletRequest defaultCharset = StandardCharsets.UTF_8; long formContentSize = 0; - ByteArrayOutputStream os = null; for (Part p : parts) { if (p.getSubmittedFileName() == null) @@ -618,16 +614,11 @@ public class ServletApiRequest implements HttpServletRequest try (InputStream is = p.getInputStream()) { - if (os == null) - os = new ByteArrayOutputStream(); - IO.copy(is, os); - - String content = os.toString(charset == null ? defaultCharset : Charset.forName(charset)); + String content = IO.toString(is, charset == null ? defaultCharset : Charset.forName(charset)); if (_contentParameters == null) _contentParameters = new Fields(); _contentParameters.add(p.getName(), content); } - os.reset(); } } } diff --git a/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/MultiPartServletTest.java b/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/MultiPartServletTest.java index e58ef4852ea..5bbccf73da8 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/MultiPartServletTest.java +++ b/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/MultiPartServletTest.java @@ -466,5 +466,4 @@ public class MultiPartServletTest assertThat(responseContent, containsString("Parameter: part3=" + contentString)); assertThat(responseContent, not(containsString("Parameter: partFileName=" + contentString))); } - } From 8133dd6c9ec0929ce32105b2112137c2c1bd98ce Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Tue, 31 Jan 2023 14:29:42 +1100 Subject: [PATCH 12/22] changes from review Signed-off-by: Lachlan Roberts --- .../org/eclipse/jetty/http/MultiPart.java | 14 ++++++++----- .../jetty/http/MultiPartByteRanges.java | 4 ---- .../eclipse/jetty/http/MultiPartFormData.java | 5 +++-- .../java/org/eclipse/jetty/io/Retainable.java | 20 ------------------- .../org/eclipse/jetty/server/HttpStream.java | 6 +++--- .../jetty/server/handler/DelayedHandler.java | 2 +- 6 files changed, 16 insertions(+), 35 deletions(-) diff --git a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPart.java b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPart.java index eb22ad5f71d..37127a5ea1c 100644 --- a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPart.java +++ b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPart.java @@ -32,7 +32,6 @@ import java.util.Queue; import java.util.concurrent.ThreadLocalRandom; import org.eclipse.jetty.io.Content; -import org.eclipse.jetty.io.Retainable; import org.eclipse.jetty.io.content.ByteBufferContentSource; import org.eclipse.jetty.io.content.ChunksContentSource; import org.eclipse.jetty.io.content.PathContentSource; @@ -119,7 +118,7 @@ public class MultiPart *

A part has an optional name, an optional fileName, * optional headers and an optional content.

*/ - public abstract static class Part + public abstract static class Part implements Closeable { private static final Throwable SENTINEL_CLOSE_EXCEPTION = new StaticException("Closed"); @@ -204,6 +203,7 @@ public class MultiPart * RFC 7578, section 4.6.

* * @return the content of this part as a new {@link Content.Source} + * @see #getContentSource() */ public abstract Content.Source newContentSource(); @@ -279,6 +279,7 @@ public class MultiPart Files.delete(this.path); } + @Override public void close() { fail(SENTINEL_CLOSE_EXCEPTION); @@ -354,15 +355,18 @@ public class MultiPart @Override public Content.Source newContentSource() { - return new ChunksContentSource(content.stream().map(c -> - Content.Chunk.asChunk(c.getByteBuffer().slice(), c.isLast(), Retainable.NOOP)).toList()); + List newChunks = content.stream() + .map(chunk -> Content.Chunk.asChunk(chunk.getByteBuffer().slice(), chunk.isLast(), chunk)) + .peek(Content.Chunk::retain) + .toList(); + return new ChunksContentSource(newChunks); } @Override public void close() { super.close(); - content.forEach(Retainable::release); + content.forEach(Content.Chunk::release); } @Override diff --git a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartByteRanges.java b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartByteRanges.java index 3c0d7153d91..ed4fd95123a 100644 --- a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartByteRanges.java +++ b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartByteRanges.java @@ -24,8 +24,6 @@ import java.util.concurrent.CompletableFuture; import org.eclipse.jetty.io.Content; import org.eclipse.jetty.util.thread.AutoLock; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; /** *

A {@link CompletableFuture} that is completed when a multipart/byteranges @@ -56,8 +54,6 @@ import org.slf4j.LoggerFactory; */ public class MultiPartByteRanges extends CompletableFuture { - private static final Logger LOG = LoggerFactory.getLogger(MultiPartByteRanges.class); - private final PartsListener listener = new PartsListener(); private final MultiPart.Parser parser; diff --git a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartFormData.java b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartFormData.java index a9a713600ea..552f35c1e5c 100644 --- a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartFormData.java +++ b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartFormData.java @@ -386,7 +386,8 @@ public class MultiPartFormData extends CompletableFutureReturns whether this resource is referenced counted by calls to {@link #retain()} * and {@link #release()}.

diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpStream.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpStream.java index 0bea0d2a677..6946df400d3 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpStream.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpStream.java @@ -110,10 +110,10 @@ public interface HttpStream extends Callback { long consumedRequestContentBytes = 0; long maxUnconsumedRequestContentBytes = httpConfig.getMaxUnconsumedRequestContentBytes(); - while (consumedRequestContentBytes < maxUnconsumedRequestContentBytes) + while (maxUnconsumedRequestContentBytes < 0 || consumedRequestContentBytes < maxUnconsumedRequestContentBytes) { // We can always just read again here as EOF and Error content will be persistently returned. - Content.Chunk content = stream.read(); + Chunk content = stream.read(); // if we cannot read to EOF then fail the stream rather than wait for unconsumed content if (content == null) @@ -124,7 +124,7 @@ public interface HttpStream extends Callback content.release(); // if the input failed, then fail the stream for same reason - if (content instanceof Content.Chunk.Error error) + if (content instanceof Chunk.Error error) return error.getCause(); if (content.isLast()) diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/DelayedHandler.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/DelayedHandler.java index 97770f7fd28..f56b909701f 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/DelayedHandler.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/DelayedHandler.java @@ -291,7 +291,7 @@ public class DelayedHandler extends Handler.Wrapper { // We must execute here as even though we have consumed all the input, we are probably // invoked in a demand runnable that is serialized with any write callbacks that might be done in process - getRequest().getContext().execute(super::process); + getRequest().getContext().execute(this::process); } else { From 88c6f20199b99ba2e7417a391a8d17dd03673ee8 Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Wed, 1 Feb 2023 12:08:38 +1100 Subject: [PATCH 13/22] changes from review Signed-off-by: Lachlan Roberts --- .../org/eclipse/jetty/http/MultiPart.java | 13 +++++----- .../jetty/http/MultiPartByteRanges.java | 12 ++++++--- .../eclipse/jetty/http/MultiPartFormData.java | 26 ++++++++----------- .../jetty/http/MultiPartCaptureTest.java | 7 ++--- .../jetty/io/content/ChunksContentSource.java | 15 +++++++++++ 5 files changed, 44 insertions(+), 29 deletions(-) diff --git a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPart.java b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPart.java index 37127a5ea1c..30d442c0b3b 100644 --- a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPart.java +++ b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPart.java @@ -120,7 +120,7 @@ public class MultiPart */ public abstract static class Part implements Closeable { - private static final Throwable SENTINEL_CLOSE_EXCEPTION = new StaticException("Closed"); + private static final Throwable CLOSE_EXCEPTION = new StaticException("Closed"); private final String name; private final String fileName; @@ -202,7 +202,7 @@ public class MultiPart * as specified in * RFC 7578, section 4.6.

* - * @return the content of this part as a new {@link Content.Source} + * @return the content of this part as a new {@link Content.Source} or null if the content cannot be consumed multiple times. * @see #getContentSource() */ public abstract Content.Source newContentSource(); @@ -258,7 +258,6 @@ public class MultiPart */ public void writeTo(Path path) throws IOException { - this.temporary = false; if (this.path == null) { try (OutputStream out = Files.newOutputStream(path)) @@ -266,10 +265,12 @@ public class MultiPart IO.copy(Content.Source.asInputStream(newContentSource()), out); } this.path = path; + this.temporary = false; } else { this.path = Files.move(this.path, path, StandardCopyOption.REPLACE_EXISTING); + this.temporary = false; } } @@ -282,7 +283,7 @@ public class MultiPart @Override public void close() { - fail(SENTINEL_CLOSE_EXCEPTION); + fail(CLOSE_EXCEPTION); } public void fail(Throwable t) @@ -349,6 +350,7 @@ public class MultiPart public ChunksPart(String name, String fileName, HttpFields fields, List content) { super(name, fileName, fields); + content.forEach(Content.Chunk::retain); this.content = content; } @@ -356,8 +358,7 @@ public class MultiPart public Content.Source newContentSource() { List newChunks = content.stream() - .map(chunk -> Content.Chunk.asChunk(chunk.getByteBuffer().slice(), chunk.isLast(), chunk)) - .peek(Content.Chunk::retain) + .map(chunk -> Content.Chunk.from(chunk.getByteBuffer().slice(), chunk.isLast())) .toList(); return new ChunksContentSource(newChunks); } diff --git a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartByteRanges.java b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartByteRanges.java index ed4fd95123a..edb69378740 100644 --- a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartByteRanges.java +++ b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartByteRanges.java @@ -23,6 +23,7 @@ import java.util.List; import java.util.concurrent.CompletableFuture; import org.eclipse.jetty.io.Content; +import org.eclipse.jetty.io.Retainable; import org.eclipse.jetty.util.thread.AutoLock; /** @@ -294,6 +295,7 @@ public class MultiPartByteRanges extends CompletableFuture toFail; + List partsToFail; + List partChunksToFail; try (AutoLock ignored = lock.lock()) { if (failure != null) return; failure = cause; - toFail = new ArrayList<>(parts); + partsToFail = new ArrayList<>(parts); parts.clear(); + partChunksToFail = new ArrayList<>(partChunks); + partChunks.clear(); } - toFail.forEach(MultiPart.Part::close); + partsToFail.forEach(p -> p.fail(cause)); + partChunksToFail.forEach(Retainable::release); } } } diff --git a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartFormData.java b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartFormData.java index 552f35c1e5c..abc69a2c880 100644 --- a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartFormData.java +++ b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartFormData.java @@ -28,6 +28,8 @@ import java.util.Objects; import java.util.concurrent.CompletableFuture; import org.eclipse.jetty.io.Content; +import org.eclipse.jetty.io.Retainable; +import org.eclipse.jetty.util.IO; import org.eclipse.jetty.util.QuotedStringTokenizer; import org.eclipse.jetty.util.thread.AutoLock; import org.slf4j.Logger; @@ -380,15 +382,7 @@ public class MultiPartFormData extends CompletableFuture toFail; + List partsToFail; + List partChunksToFail; try (AutoLock ignored = lock.lock()) { if (failure != null) return; failure = cause; - toFail = new ArrayList<>(parts); + partsToFail = new ArrayList<>(parts); parts.clear(); + partChunksToFail = new ArrayList<>(partChunks); + partChunks.clear(); } - for (MultiPart.Part part : toFail) - { - part.fail(cause); - } + partsToFail.forEach(p -> p.fail(cause)); + partChunksToFail.forEach(Retainable::release); close(); delete(); } diff --git a/jetty-core/jetty-http/src/test/java/org/eclipse/jetty/http/MultiPartCaptureTest.java b/jetty-core/jetty-http/src/test/java/org/eclipse/jetty/http/MultiPartCaptureTest.java index 2ff2e3d9ff9..8dabc7d7f11 100644 --- a/jetty-core/jetty-http/src/test/java/org/eclipse/jetty/http/MultiPartCaptureTest.java +++ b/jetty-core/jetty-http/src/test/java/org/eclipse/jetty/http/MultiPartCaptureTest.java @@ -51,7 +51,6 @@ import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.greaterThan; import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.notNullValue; -import static org.junit.jupiter.api.Assertions.assertTrue; public class MultiPartCaptureTest { @@ -255,8 +254,7 @@ public class MultiPartCaptureTest assertThat("Part[" + expected.name + "]", parts, is(notNullValue())); MultiPart.Part part = parts.get(0); String charset = getCharsetFromContentType(part.getHeaders().get(HttpHeader.CONTENT_TYPE), defaultCharset); - assertTrue(part.getContentSource().rewind()); - String partContent = Content.Source.asString(part.getContentSource(), Charset.forName(charset)); + String partContent = Content.Source.asString(part.newContentSource(), Charset.forName(charset)); assertThat("Part[" + expected.name + "].contents", partContent, containsString(expected.value)); } @@ -276,8 +274,7 @@ public class MultiPartCaptureTest assertThat("Part[" + expected.name + "]", parts, is(notNullValue())); MultiPart.Part part = parts.get(0); MessageDigest digest = MessageDigest.getInstance("SHA1"); - assertTrue(part.getContentSource().rewind()); - try (InputStream partInputStream = Content.Source.asInputStream(part.getContentSource()); + try (InputStream partInputStream = Content.Source.asInputStream(part.newContentSource()); DigestOutputStream digester = new DigestOutputStream(OutputStream.nullOutputStream(), digest)) { IO.copy(partInputStream, digester); diff --git a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/content/ChunksContentSource.java b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/content/ChunksContentSource.java index 85328ac0569..074e4a87d96 100644 --- a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/content/ChunksContentSource.java +++ b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/content/ChunksContentSource.java @@ -13,8 +13,10 @@ package org.eclipse.jetty.io.content; +import java.util.ArrayList; import java.util.Collection; import java.util.Iterator; +import java.util.List; import org.eclipse.jetty.io.Content; import org.eclipse.jetty.util.thread.AutoLock; @@ -38,6 +40,7 @@ public class ChunksContentSource implements Content.Source public ChunksContentSource(Collection chunks) { + chunks.forEach(Content.Chunk::retain); this.chunks = chunks; this.length = chunks.stream().mapToLong(c -> c.getByteBuffer().remaining()).sum(); } @@ -113,11 +116,23 @@ public class ChunksContentSource implements Content.Source @Override public void fail(Throwable failure) { + List chunksToRelease; try (AutoLock ignored = lock.lock()) { if (terminated != null) return; terminated = Content.Chunk.from(failure); + if (iterator != null) + { + chunksToRelease = new ArrayList<>(); + iterator.forEachRemaining(chunksToRelease::add); + } + else + { + chunksToRelease = List.copyOf(chunks); + } } + + chunksToRelease.forEach(Content.Chunk::release); } } From 02823ecfad7d77132f4e515ba5566ae99f017bcc Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Wed, 1 Feb 2023 14:49:35 +1100 Subject: [PATCH 14/22] fix for retainable merge Signed-off-by: Lachlan Roberts --- .../jetty/ee10/servlet/ServletMultiPartFormData.java | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletMultiPartFormData.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletMultiPartFormData.java index 0e5b06055f3..55087110208 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletMultiPartFormData.java +++ b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletMultiPartFormData.java @@ -29,9 +29,10 @@ import org.eclipse.jetty.http.HttpHeader; import org.eclipse.jetty.http.MultiPart; import org.eclipse.jetty.http.MultiPartFormData; import org.eclipse.jetty.io.AbstractConnection; -import org.eclipse.jetty.io.ByteBufferPool; import org.eclipse.jetty.io.Connection; import org.eclipse.jetty.io.Content; +import org.eclipse.jetty.io.RetainableByteBuffer; +import org.eclipse.jetty.io.RetainableByteBufferPool; import org.eclipse.jetty.server.ConnectionMetaData; import org.eclipse.jetty.util.BufferUtil; import org.eclipse.jetty.util.IO; @@ -118,14 +119,15 @@ public class ServletMultiPartFormData ConnectionMetaData connectionMetaData = request.getRequest().getConnectionMetaData(); formData.setPartHeadersMaxLength(connectionMetaData.getHttpConfiguration().getRequestHeaderSize()); - ByteBufferPool byteBufferPool = request.getRequest().getComponents().getByteBufferPool(); + RetainableByteBufferPool byteBufferPool = request.getRequest().getComponents().getRetainableByteBufferPool(); Connection connection = connectionMetaData.getConnection(); int bufferSize = connection instanceof AbstractConnection c ? c.getInputBufferSize() : 2048; InputStream input = request.getInputStream(); while (!formData.isDone()) { - ByteBuffer buffer = byteBufferPool.newByteBuffer(bufferSize, false); + RetainableByteBuffer retainable = byteBufferPool.acquire(bufferSize, false); boolean readEof = false; + ByteBuffer buffer = retainable.getByteBuffer(); while (BufferUtil.space(buffer) > bufferSize / 2) { int read = BufferUtil.readFrom(input, buffer); @@ -136,7 +138,7 @@ public class ServletMultiPartFormData } } - formData.parse(Content.Chunk.from(buffer, false, byteBuffer -> byteBufferPool.release(buffer))); + formData.parse(Content.Chunk.from(buffer, false, byteBuffer -> retainable.release())); if (readEof) { formData.parse(Content.Chunk.EOF); From d66e36b7462b2b40a8b36fe6327b33cbfa9a6190 Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Wed, 1 Feb 2023 14:59:29 +1100 Subject: [PATCH 15/22] consumeAvailable should use number of reads instead of bytes Signed-off-by: Lachlan Roberts --- .../jetty/server/HttpConfiguration.java | 22 +++++++++---------- .../org/eclipse/jetty/server/HttpStream.java | 8 +++---- 2 files changed, 15 insertions(+), 15 deletions(-) diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConfiguration.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConfiguration.java index d358101e1ae..6797c807809 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConfiguration.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConfiguration.java @@ -79,7 +79,7 @@ public class HttpConfiguration implements Dumpable private boolean _relativeRedirectAllowed; private HostPort _serverAuthority; private SocketAddress _localAddress; - private long _maxUnconsumedRequestContentBytes = 1024 * 1024; + private int _maxUnconsumedRequestContentReads = 16; /** *

An interface that allows a request object to be customized @@ -718,25 +718,25 @@ public class HttpConfiguration implements Dumpable } /** - * Sets the maximum amount of bytes that will be read from the HttpStream if the content is not fully consumed by the - * application. If this is unable to consume to EOF then the connection will be made non-persistent. + * Sets the maximum amount of {@link HttpStream#read()}s that can be done by the {@link HttpStream} if the content is not + * fully consumed by the application. If this is unable to consume to EOF then the connection will be made non-persistent. * - * @param maxUnconsumedRequestContentBytes the maximum amount of unconsumed bytes that will be read from the HttpStream. + * @param maxUnconsumedRequestContentReads the maximum amount of reads for unconsumed content or -1 for unlimited. */ - public void setMaxUnconsumedRequestContentBytes(long maxUnconsumedRequestContentBytes) + public void setMaxUnconsumedRequestContentReads(int maxUnconsumedRequestContentReads) { - _maxUnconsumedRequestContentBytes = maxUnconsumedRequestContentBytes; + _maxUnconsumedRequestContentReads = maxUnconsumedRequestContentReads; } /** - * Gets the maximum amount of bytes that will be read from the HttpStream if the content is not fully consumed by the - * application. If this is unable to consume to EOF then the connection will be made non-persistent. + * Gets the maximum amount of {@link HttpStream#read()}s that can be done by the {@link HttpStream} if the content is not + * fully consumed by the application. If this is unable to consume to EOF then the connection will be made non-persistent. * - * @return the maximum amount of unconsumed bytes that will be read from the HttpStream. + * @return the maximum amount of reads for unconsumed content or -1 for unlimited. */ - public long getMaxUnconsumedRequestContentBytes() + public int getMaxUnconsumedRequestContentReads() { - return _maxUnconsumedRequestContentBytes; + return _maxUnconsumedRequestContentReads; } @Override diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpStream.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpStream.java index 6946df400d3..546c488f0de 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpStream.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpStream.java @@ -108,19 +108,19 @@ public interface HttpStream extends Callback static Throwable consumeAvailable(HttpStream stream, HttpConfiguration httpConfig) { - long consumedRequestContentBytes = 0; - long maxUnconsumedRequestContentBytes = httpConfig.getMaxUnconsumedRequestContentBytes(); - while (maxUnconsumedRequestContentBytes < 0 || consumedRequestContentBytes < maxUnconsumedRequestContentBytes) + int numReads = 0; + int maxReads = httpConfig.getMaxUnconsumedRequestContentReads(); + while (maxReads < 0 || numReads < maxReads) { // We can always just read again here as EOF and Error content will be persistently returned. Chunk content = stream.read(); + numReads++; // if we cannot read to EOF then fail the stream rather than wait for unconsumed content if (content == null) return new IOException("Content not consumed"); // Always release any returned content. This is a noop for EOF and Error content. - consumedRequestContentBytes += content.remaining(); content.release(); // if the input failed, then fail the stream for same reason From cd47a07463a656b685d5fb2bf2e99fdde6d11352 Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Sat, 4 Feb 2023 00:48:12 +1100 Subject: [PATCH 16/22] use non-pooling RetainableByteBufferPool to work around performance bug Signed-off-by: Lachlan Roberts --- .../main/java/org/eclipse/jetty/http/MultiPart.java | 7 ++++--- .../ee10/servlet/ServletMultiPartFormData.java | 2 +- .../jetty/ee10/servlet/MultiPartServletTest.java | 13 ++++++++++--- 3 files changed, 15 insertions(+), 7 deletions(-) diff --git a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPart.java b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPart.java index 30d442c0b3b..6ef5b7d9243 100644 --- a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPart.java +++ b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPart.java @@ -125,7 +125,7 @@ public class MultiPart private final String name; private final String fileName; private final HttpFields fields; - private final Content.Source contentSource; + private Content.Source contentSource; private Path path; private boolean temporary = true; @@ -140,7 +140,6 @@ public class MultiPart this.fileName = fileName; this.fields = fields != null ? fields : HttpFields.EMPTY; this.path = path; - this.contentSource = newContentSource(); } private Path getPath() @@ -189,6 +188,8 @@ public class MultiPart */ public Content.Source getContentSource() { + if (contentSource == null) + contentSource = newContentSource(); return contentSource; } @@ -350,8 +351,8 @@ public class MultiPart public ChunksPart(String name, String fileName, HttpFields fields, List content) { super(name, fileName, fields); + this.content = Objects.requireNonNull(content); content.forEach(Content.Chunk::retain); - this.content = content; } @Override diff --git a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletMultiPartFormData.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletMultiPartFormData.java index 55087110208..7301b4f1417 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletMultiPartFormData.java +++ b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletMultiPartFormData.java @@ -138,7 +138,7 @@ public class ServletMultiPartFormData } } - formData.parse(Content.Chunk.from(buffer, false, byteBuffer -> retainable.release())); + formData.parse(Content.Chunk.from(buffer, false, retainable::release)); if (readEof) { formData.parse(Content.Chunk.EOF); diff --git a/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/MultiPartServletTest.java b/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/MultiPartServletTest.java index 5bbccf73da8..20d71007c4f 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/MultiPartServletTest.java +++ b/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/MultiPartServletTest.java @@ -50,6 +50,7 @@ import org.eclipse.jetty.http.MultiPart; import org.eclipse.jetty.http.MultiPartFormData; import org.eclipse.jetty.io.Content; import org.eclipse.jetty.io.EofException; +import org.eclipse.jetty.io.RetainableByteBufferPool; import org.eclipse.jetty.logging.StacklessLogging; import org.eclipse.jetty.server.Server; import org.eclipse.jetty.server.ServerConnector; @@ -96,7 +97,12 @@ public class MultiPartServletTest private void start(HttpServlet servlet, MultipartConfigElement config) throws Exception { - server = new Server(); + start(servlet, config, null); + } + + private void start(HttpServlet servlet, MultipartConfigElement config, RetainableByteBufferPool bufferPool) throws Exception + { + server = new Server(null, null, bufferPool); connector = new ServerConnector(server); server.addConnector(connector); @@ -128,6 +134,7 @@ public class MultiPartServletTest @Test public void testLargePart() throws Exception { + RetainableByteBufferPool bufferPool = new RetainableByteBufferPool.NonPooling(); start(new HttpServlet() { @Override @@ -135,7 +142,7 @@ public class MultiPartServletTest { req.getParameterMap(); } - }, new MultipartConfigElement(tmpDirString)); + }, new MultipartConfigElement(tmpDirString), bufferPool); OutputStreamRequestContent content = new OutputStreamRequestContent(); MultiPartRequestContent multiPart = new MultiPartRequestContent(); @@ -159,7 +166,7 @@ public class MultiPartServletTest } content.close(); - Response response = listener.get(2, TimeUnit.MINUTES); + Response response = listener.get(30, TimeUnit.MINUTES); assertThat(response.getStatus(), equalTo(HttpStatus.BAD_REQUEST_400)); String responseContent = IO.toString(listener.getInputStream()); assertThat(responseContent, containsString("Unable to parse form content")); From 1610cd36924a8ec092f0cc4a30422a5ba0235686 Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Mon, 6 Feb 2023 11:10:21 +1100 Subject: [PATCH 17/22] add todo to revert to normal pool after fix for #9311 Signed-off-by: Lachlan Roberts --- .../org/eclipse/jetty/ee10/servlet/MultiPartServletTest.java | 1 + 1 file changed, 1 insertion(+) diff --git a/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/MultiPartServletTest.java b/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/MultiPartServletTest.java index 20d71007c4f..b6cfdb22a74 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/MultiPartServletTest.java +++ b/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/MultiPartServletTest.java @@ -134,6 +134,7 @@ public class MultiPartServletTest @Test public void testLargePart() throws Exception { + // TODO: Use normal pool when a fix for https://github.com/eclipse/jetty.project/issues/9311 is merged. RetainableByteBufferPool bufferPool = new RetainableByteBufferPool.NonPooling(); start(new HttpServlet() { From d005e975b03e8474303583816d9da162d755d374 Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Tue, 7 Feb 2023 15:47:17 +1100 Subject: [PATCH 18/22] changes f rom review Signed-off-by: Lachlan Roberts --- .../jetty/http/MultiPartByteRanges.java | 3 +- .../eclipse/jetty/http/MultiPartFormData.java | 53 ++++++++++--------- .../jetty/io/content/ChunksContentSource.java | 1 - .../org/eclipse/jetty/server/HttpStream.java | 8 +-- .../org/eclipse/jetty/server/Request.java | 8 +++ .../servlet/MultiPartFormInputStream.java | 32 ----------- .../jetty/ee10/servlet/ServletApiRequest.java | 28 +++++----- .../ee10/servlet/ServletContextRequest.java | 2 +- .../jetty/ee10/servlet/ServletHolder.java | 2 +- .../servlet/ServletMultiPartFormData.java | 2 +- .../ee9/nested/MultiPartFormInputStream.java | 2 +- .../org/eclipse/jetty/ee9/nested/Request.java | 2 +- 12 files changed, 58 insertions(+), 85 deletions(-) delete mode 100644 jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/MultiPartFormInputStream.java diff --git a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartByteRanges.java b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartByteRanges.java index edb69378740..a0918c9bd22 100644 --- a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartByteRanges.java +++ b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartByteRanges.java @@ -23,7 +23,6 @@ import java.util.List; import java.util.concurrent.CompletableFuture; import org.eclipse.jetty.io.Content; -import org.eclipse.jetty.io.Retainable; import org.eclipse.jetty.util.thread.AutoLock; /** @@ -328,7 +327,7 @@ public class MultiPartByteRanges extends CompletableFuture p.fail(cause)); - partChunksToFail.forEach(Retainable::release); + partChunksToFail.forEach(Content.Chunk::release); } } } diff --git a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartFormData.java b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartFormData.java index abc69a2c880..9007278c6f8 100644 --- a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartFormData.java +++ b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartFormData.java @@ -308,7 +308,7 @@ public class MultiPartFormData extends CompletableFutureAn ordered list of {@link MultiPart.Part}s that can * be accessed by index or by name, or iterated over.

*/ - public class Parts implements Iterable + public class Parts implements Iterable, Closeable { private final List parts; @@ -378,6 +378,7 @@ public class MultiPartFormData extends CompletableFuture maxMemoryFileSize) { - // Must save to disk. - if (ensureFileChannel()) + try { - // Write existing memory chunks. - for (Content.Chunk c : partChunks) + // Must save to disk. + if (ensureFileChannel()) { - if (!write(c.getByteBuffer())) - return; + // Write existing memory chunks. + for (Content.Chunk c : partChunks) + { + write(c.getByteBuffer()); + } } + write(buffer); + if (chunk.isLast()) + close(); } - write(buffer); - if (chunk.isLast()) - close(); + catch (Throwable x) + { + onFailure(x); + } + + partChunks.forEach(Content.Chunk::release); + chunk.release(); return; } } @@ -469,24 +479,15 @@ public class MultiPartFormData extends CompletableFuture 0) { - int remaining = buffer.remaining(); - while (remaining > 0) - { - int written = fileChannel.write(buffer); - if (written == 0) - throw new NonWritableChannelException(); - remaining -= written; - } - return true; - } - catch (Throwable x) - { - onFailure(x); - return false; + int written = fileChannel.write(buffer); + if (written == 0) + throw new NonWritableChannelException(); + remaining -= written; } } diff --git a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/content/ChunksContentSource.java b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/content/ChunksContentSource.java index 074e4a87d96..8b88e4ddce8 100644 --- a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/content/ChunksContentSource.java +++ b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/content/ChunksContentSource.java @@ -132,7 +132,6 @@ public class ChunksContentSource implements Content.Source chunksToRelease = List.copyOf(chunks); } } - chunksToRelease.forEach(Content.Chunk::release); } } diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpStream.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpStream.java index 546c488f0de..fd5032c5bee 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpStream.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpStream.java @@ -13,7 +13,6 @@ package org.eclipse.jetty.server; -import java.io.IOException; import java.nio.ByteBuffer; import org.eclipse.jetty.http.HttpFields; @@ -22,6 +21,7 @@ import org.eclipse.jetty.io.Connection; import org.eclipse.jetty.io.Content; import org.eclipse.jetty.io.Content.Chunk; import org.eclipse.jetty.util.Callback; +import org.eclipse.jetty.util.StaticException; /** * A HttpStream is an abstraction that together with {@link MetaData.Request}, represents the @@ -31,6 +31,8 @@ import org.eclipse.jetty.util.Callback; */ public interface HttpStream extends Callback { + Exception CONTENT_NOT_CONSUMED = new StaticException("Content Not Consumed"); + /** *

Attribute name to be used as a {@link Request} attribute to store/retrieve * the {@link Connection} created during the HTTP/1.1 upgrade mechanism or the @@ -118,7 +120,7 @@ public interface HttpStream extends Callback // if we cannot read to EOF then fail the stream rather than wait for unconsumed content if (content == null) - return new IOException("Content not consumed"); + return CONTENT_NOT_CONSUMED; // Always release any returned content. This is a noop for EOF and Error content. content.release(); @@ -131,7 +133,7 @@ public interface HttpStream extends Callback return null; } - return new IOException("Content not consumed"); + return CONTENT_NOT_CONSUMED; } class Wrapper implements HttpStream diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java index 5973a949342..0f62e099deb 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java @@ -228,6 +228,14 @@ public interface Request extends Attributes, Content.Source @Override Content.Chunk read(); + /** + * Consume any available content. This bypasses any request wrappers to process the content in + * {@link Request#read()} and reads directly from the {@link HttpStream}. This reads until + * there is no content currently available or it reaches EOF. + * The {@link HttpConfiguration#setMaxUnconsumedRequestContentReads(int)} configuration can be used + * to configure how many reads will be attempted by this method. + * @return true if the content was fully consumed. + */ boolean consumeAvailable(); /** diff --git a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/MultiPartFormInputStream.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/MultiPartFormInputStream.java deleted file mode 100644 index 3d019334e4a..00000000000 --- a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/MultiPartFormInputStream.java +++ /dev/null @@ -1,32 +0,0 @@ -// -// ======================================================================== -// Copyright (c) 1995-2022 Mort Bay Consulting Pty Ltd and others. -// -// This program and the accompanying materials are made available under the -// terms of the Eclipse Public License v. 2.0 which is available at -// https://www.eclipse.org/legal/epl-2.0, or the Apache License, Version 2.0 -// which is available at https://www.apache.org/licenses/LICENSE-2.0. -// -// SPDX-License-Identifier: EPL-2.0 OR Apache-2.0 -// ======================================================================== -// - -package org.eclipse.jetty.ee10.servlet; - -import java.io.InputStream; -import java.util.List; - -import jakarta.servlet.http.Part; - -public class MultiPartFormInputStream -{ - public MultiPartFormInputStream(InputStream inputStream, String contentType, Object o, Object o1) - { - - } - - public List getParts() - { - return null; - } -} diff --git a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletApiRequest.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletApiRequest.java index 23fc201f9d6..ce41fee0773 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletApiRequest.java +++ b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletApiRequest.java @@ -83,10 +83,6 @@ import org.eclipse.jetty.util.URIUtil; import org.slf4j.Logger; import org.slf4j.LoggerFactory; -import static org.eclipse.jetty.ee10.servlet.ServletContextRequest.INPUT_NONE; -import static org.eclipse.jetty.ee10.servlet.ServletContextRequest.NO_PARAMS; -import static org.eclipse.jetty.ee10.servlet.ServletContextRequest.__MULTIPART_CONFIG_ELEMENT; - /** * The Jetty low level implementation of the ee10 {@link HttpServletRequest} object. * @@ -101,7 +97,7 @@ public class ServletApiRequest implements HttpServletRequest //TODO review which fields should be in ServletContextRequest private AsyncContextState _async; private String _characterEncoding; - private int _inputState = INPUT_NONE; + private int _inputState = ServletContextRequest.INPUT_NONE; private BufferedReader _reader; private String _readerEncoding; private String _contentType; @@ -487,7 +483,7 @@ public class ServletApiRequest implements HttpServletRequest if (contentType == null || !MimeTypes.Type.MULTIPART_FORM_DATA.is(HttpField.valueParameters(contentType, null))) throw new ServletException("Unsupported Content-Type [%s], expected [%s]".formatted(contentType, MimeTypes.Type.MULTIPART_FORM_DATA.asString())); - MultipartConfigElement config = (MultipartConfigElement)getAttribute(__MULTIPART_CONFIG_ELEMENT); + MultipartConfigElement config = (MultipartConfigElement)getAttribute(ServletContextRequest.MULTIPART_CONFIG_ELEMENT); if (config == null) throw new IllegalStateException("No multipart config for servlet"); @@ -532,7 +528,7 @@ public class ServletApiRequest implements HttpServletRequest if (p.getSubmittedFileName() == null) { formContentSize = Math.addExact(formContentSize, p.getSize()); - if (formContentSize > maxFormContentSize) + if (maxFormContentSize >= 0 && formContentSize > maxFormContentSize) throw new IllegalStateException("Form is larger than max length " + maxFormContentSize); // Servlet Spec 3.0 pg 23, parts without filename must be put into params. @@ -714,7 +710,7 @@ public class ServletApiRequest implements HttpServletRequest @Override public void setCharacterEncoding(String encoding) throws UnsupportedEncodingException { - if (_inputState != INPUT_NONE) + if (_inputState != ServletContextRequest.INPUT_NONE) return; _characterEncoding = encoding; @@ -765,7 +761,7 @@ public class ServletApiRequest implements HttpServletRequest @Override public ServletInputStream getInputStream() throws IOException { - if (_inputState != INPUT_NONE && _inputState != ServletContextRequest.INPUT_STREAM) + if (_inputState != ServletContextRequest.INPUT_NONE && _inputState != ServletContextRequest.INPUT_STREAM) throw new IllegalStateException("READER"); _inputState = ServletContextRequest.INPUT_STREAM; @@ -811,7 +807,7 @@ public class ServletApiRequest implements HttpServletRequest public void setContentParameters(Fields params) { if (params == null || params.getSize() == 0) - _contentParameters = NO_PARAMS; + _contentParameters = ServletContextRequest.NO_PARAMS; else _contentParameters = params; } @@ -835,7 +831,7 @@ public class ServletApiRequest implements HttpServletRequest // protect against calls to recycled requests (which is illegal, but // this gives better failures Fields parameters = _parameters; - return parameters == null ? NO_PARAMS : parameters; + return parameters == null ? ServletContextRequest.NO_PARAMS : parameters; } private void extractContentParameters() throws BadMessageException @@ -854,7 +850,7 @@ public class ServletApiRequest implements HttpServletRequest try { int contentLength = getContentLength(); - if (contentLength != 0 && _inputState == INPUT_NONE) + if (contentLength != 0 && _inputState == ServletContextRequest.INPUT_NONE) { String baseType = HttpField.valueParameters(getContentType(), null); if (MimeTypes.Type.FORM_ENCODED.is(baseType) && @@ -874,7 +870,7 @@ public class ServletApiRequest implements HttpServletRequest } } else if (MimeTypes.Type.MULTIPART_FORM_DATA.is(baseType) && - getAttribute(__MULTIPART_CONFIG_ELEMENT) != null) + getAttribute(ServletContextRequest.MULTIPART_CONFIG_ELEMENT) != null) { try { @@ -891,7 +887,7 @@ public class ServletApiRequest implements HttpServletRequest } if (_contentParameters == null || _contentParameters.isEmpty()) - _contentParameters = NO_PARAMS; + _contentParameters = ServletContextRequest.NO_PARAMS; } catch (IllegalStateException | IllegalArgumentException e) { @@ -910,7 +906,7 @@ public class ServletApiRequest implements HttpServletRequest { HttpURI httpURI = _request.getHttpURI(); if (httpURI == null || StringUtil.isEmpty(httpURI.getQuery())) - _queryParameters = NO_PARAMS; + _queryParameters = ServletContextRequest.NO_PARAMS; else { try @@ -1008,7 +1004,7 @@ public class ServletApiRequest implements HttpServletRequest @Override public BufferedReader getReader() throws IOException { - if (_inputState != INPUT_NONE && _inputState != ServletContextRequest.INPUT_READER) + if (_inputState != ServletContextRequest.INPUT_NONE && _inputState != ServletContextRequest.INPUT_READER) throw new IllegalStateException("STREAMED"); if (_inputState == ServletContextRequest.INPUT_READER) diff --git a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletContextRequest.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletContextRequest.java index 616c2697fe7..0530b0b984b 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletContextRequest.java +++ b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletContextRequest.java @@ -44,7 +44,7 @@ import org.slf4j.LoggerFactory; public class ServletContextRequest extends ContextRequest { - public static final String __MULTIPART_CONFIG_ELEMENT = "org.eclipse.jetty.multipartConfig"; + public static final String MULTIPART_CONFIG_ELEMENT = "org.eclipse.jetty.multipartConfig"; private static final Logger LOG = LoggerFactory.getLogger(ServletContextRequest.class); static final int INPUT_NONE = 0; static final int INPUT_STREAM = 1; diff --git a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletHolder.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletHolder.java index 7b1a797f976..f5d5865aada 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletHolder.java +++ b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletHolder.java @@ -715,7 +715,7 @@ public class ServletHolder extends Holder implements Comparable= _maxParts) + if (_maxParts >= 0 && _numParts > _maxParts) throw new IllegalStateException(String.format("Form with too many keys [%d > %d]", _numParts, _maxParts)); } diff --git a/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/Request.java b/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/Request.java index 57cf0aa554b..901cd8e5620 100644 --- a/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/Request.java +++ b/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/Request.java @@ -2008,7 +2008,7 @@ public class Request implements HttpServletRequest if (p.getSubmittedFileName() == null) { formContentSize = Math.addExact(formContentSize, p.getSize()); - if (formContentSize > maxFormContentSize) + if (maxFormContentSize >= 0 && formContentSize > maxFormContentSize) throw new IllegalStateException("Form is larger than max length " + maxFormContentSize); // Servlet Spec 3.0 pg 23, parts without filename must be put into params. From cb511bed8a00b84534d92eb6ca3d36f5f0aa88a5 Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Wed, 8 Feb 2023 13:23:35 +1100 Subject: [PATCH 19/22] Issue #9287 - fix failing tests Signed-off-by: Lachlan Roberts --- .../client/InputStreamResponseListener.java | 11 ++-------- .../org/eclipse/jetty/server/HttpStream.java | 2 +- .../jetty/server/handler/DelayedHandler.java | 21 ++++++++++++++++--- .../server/internal/HttpChannelState.java | 2 +- .../eclipse/jetty/server/MockHttpStream.java | 2 +- .../transport/HttpClientStreamTest.java | 4 ++-- .../servlet/ServletMultiPartFormData.java | 4 ++-- .../ee10/servlet/MultiPartServletTest.java | 6 +++--- .../org/eclipse/jetty/ee9/nested/Request.java | 1 + .../jetty/ee9/proxy/ProxyServletTest.java | 3 +-- 10 files changed, 32 insertions(+), 24 deletions(-) diff --git a/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/InputStreamResponseListener.java b/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/InputStreamResponseListener.java index 5c696fb466d..e0dc2c2b28f 100644 --- a/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/InputStreamResponseListener.java +++ b/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/InputStreamResponseListener.java @@ -33,6 +33,7 @@ import java.util.function.Consumer; import org.eclipse.jetty.client.Response.Listener; import org.eclipse.jetty.io.Content; import org.eclipse.jetty.util.IO; +import org.eclipse.jetty.util.StaticException; import org.eclipse.jetty.util.thread.AutoLock; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -301,7 +302,7 @@ public class InputStreamResponseListener extends Listener.Adapter break; if (failure != null) - throw new IOException(toIOException(failure)); + throw new IOException(failure); if (closed) throw new AsynchronousCloseException(); @@ -327,14 +328,6 @@ public class InputStreamResponseListener extends Listener.Adapter } } - private IOException toIOException(Throwable failure) - { - if (failure instanceof IOException) - return (IOException)failure; - else - return new IOException(failure); - } - @Override public void close() throws IOException { diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpStream.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpStream.java index fd5032c5bee..2d784b71e51 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpStream.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpStream.java @@ -31,7 +31,7 @@ import org.eclipse.jetty.util.StaticException; */ public interface HttpStream extends Callback { - Exception CONTENT_NOT_CONSUMED = new StaticException("Content Not Consumed"); + Exception CONTENT_NOT_CONSUMED = new StaticException("Content not consumed"); /** *

Attribute name to be used as a {@link Request} attribute to store/retrieve diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/DelayedHandler.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/DelayedHandler.java index f56b909701f..c695f867bcb 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/DelayedHandler.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/DelayedHandler.java @@ -291,7 +291,7 @@ public class DelayedHandler extends Handler.Wrapper { // We must execute here as even though we have consumed all the input, we are probably // invoked in a demand runnable that is serialized with any write callbacks that might be done in process - getRequest().getContext().execute(this::process); + getRequest().getContext().execute(() -> process(parts, x)); } else { @@ -304,7 +304,7 @@ public class DelayedHandler extends Handler.Wrapper { if (_formData == null) { - super.process(); + this.process(); } else { @@ -313,7 +313,22 @@ public class DelayedHandler extends Handler.Wrapper // if we are done already, then we are still in the scope of the original process call and can // process directly, otherwise we must execute a call to process as we are within a serialized // demand callback. - _formData.whenComplete(_formData.isDone() ? this::process : this::executeProcess); + if (_formData.isDone()) + { + try + { + MultiPartFormData.Parts parts = _formData.join(); + process(parts, null); + } + catch (Throwable t) + { + process(null, t); + } + } + else + { + _formData.whenComplete(this::executeProcess); + } } } diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java index babd7089523..93664d28cc1 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java @@ -890,7 +890,7 @@ public class HttpChannelState implements HttpChannel, Components stream = httpChannel._stream; } - return stream.consumeAvailable() != null; + return stream.consumeAvailable() == null; } @Override diff --git a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/MockHttpStream.java b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/MockHttpStream.java index 4742069f882..d183dbde981 100644 --- a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/MockHttpStream.java +++ b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/MockHttpStream.java @@ -228,7 +228,7 @@ public class MockHttpStream implements HttpStream @Override public Throwable consumeAvailable() { - return null; + return HttpStream.consumeAvailable(this, new HttpConfiguration()); } public boolean isComplete() diff --git a/jetty-core/jetty-tests/jetty-test-client-transports/src/test/java/org/eclipse/jetty/test/client/transport/HttpClientStreamTest.java b/jetty-core/jetty-tests/jetty-test-client-transports/src/test/java/org/eclipse/jetty/test/client/transport/HttpClientStreamTest.java index c6470f0bfd3..cbb8274bb6a 100644 --- a/jetty-core/jetty-tests/jetty-test-client-transports/src/test/java/org/eclipse/jetty/test/client/transport/HttpClientStreamTest.java +++ b/jetty-core/jetty-tests/jetty-test-client-transports/src/test/java/org/eclipse/jetty/test/client/transport/HttpClientStreamTest.java @@ -286,7 +286,7 @@ public class HttpClientStreamTest extends AbstractTest Response response = listener.get(5, TimeUnit.SECONDS); assertEquals(200, response.getStatus()); - assertThrows(AsynchronousCloseException.class, stream::read); + assertThrows(IOException.class, stream::read); } @ParameterizedTest @@ -329,7 +329,7 @@ public class HttpClientStreamTest extends AbstractTest assertTrue(latch.await(5, TimeUnit.SECONDS)); - assertThrows(AsynchronousCloseException.class, input::read); + assertThrows(IOException.class, input::read); } @ParameterizedTest diff --git a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletMultiPartFormData.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletMultiPartFormData.java index a6e06a94d8f..1550be555e0 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletMultiPartFormData.java +++ b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletMultiPartFormData.java @@ -29,10 +29,10 @@ import org.eclipse.jetty.http.HttpHeader; import org.eclipse.jetty.http.MultiPart; import org.eclipse.jetty.http.MultiPartFormData; import org.eclipse.jetty.io.AbstractConnection; +import org.eclipse.jetty.io.ByteBufferPool; import org.eclipse.jetty.io.Connection; import org.eclipse.jetty.io.Content; import org.eclipse.jetty.io.RetainableByteBuffer; -import org.eclipse.jetty.io.RetainableByteBufferPool; import org.eclipse.jetty.server.ConnectionMetaData; import org.eclipse.jetty.util.BufferUtil; import org.eclipse.jetty.util.IO; @@ -119,7 +119,7 @@ public class ServletMultiPartFormData ConnectionMetaData connectionMetaData = request.getServletContextRequest().getConnectionMetaData(); formData.setPartHeadersMaxLength(connectionMetaData.getHttpConfiguration().getRequestHeaderSize()); - RetainableByteBufferPool byteBufferPool = request.getServletContextRequest().getComponents().getRetainableByteBufferPool(); + ByteBufferPool byteBufferPool = request.getServletContextRequest().getComponents().getByteBufferPool(); Connection connection = connectionMetaData.getConnection(); int bufferSize = connection instanceof AbstractConnection c ? c.getInputBufferSize() : 2048; InputStream input = request.getInputStream(); diff --git a/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/MultiPartServletTest.java b/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/MultiPartServletTest.java index b6cfdb22a74..2c06e132c17 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/MultiPartServletTest.java +++ b/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/MultiPartServletTest.java @@ -48,9 +48,9 @@ import org.eclipse.jetty.http.HttpStatus; import org.eclipse.jetty.http.HttpTester; import org.eclipse.jetty.http.MultiPart; import org.eclipse.jetty.http.MultiPartFormData; +import org.eclipse.jetty.io.ByteBufferPool; import org.eclipse.jetty.io.Content; import org.eclipse.jetty.io.EofException; -import org.eclipse.jetty.io.RetainableByteBufferPool; import org.eclipse.jetty.logging.StacklessLogging; import org.eclipse.jetty.server.Server; import org.eclipse.jetty.server.ServerConnector; @@ -100,7 +100,7 @@ public class MultiPartServletTest start(servlet, config, null); } - private void start(HttpServlet servlet, MultipartConfigElement config, RetainableByteBufferPool bufferPool) throws Exception + private void start(HttpServlet servlet, MultipartConfigElement config, ByteBufferPool bufferPool) throws Exception { server = new Server(null, null, bufferPool); connector = new ServerConnector(server); @@ -135,7 +135,7 @@ public class MultiPartServletTest public void testLargePart() throws Exception { // TODO: Use normal pool when a fix for https://github.com/eclipse/jetty.project/issues/9311 is merged. - RetainableByteBufferPool bufferPool = new RetainableByteBufferPool.NonPooling(); + ByteBufferPool bufferPool = new ByteBufferPool.NonPooling(); start(new HttpServlet() { @Override diff --git a/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/Request.java b/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/Request.java index 1aec350b993..95afc0d668c 100644 --- a/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/Request.java +++ b/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/Request.java @@ -1969,6 +1969,7 @@ public class Request implements HttpServletRequest _multiParts = newMultiParts(config, maxFormKeys); Collection parts = _multiParts.getParts(); + setNonComplianceViolationsOnRequest(); String formCharset = null; Part charsetPart = _multiParts.getPart("_charset_"); diff --git a/jetty-ee9/jetty-ee9-proxy/src/test/java/org/eclipse/jetty/ee9/proxy/ProxyServletTest.java b/jetty-ee9/jetty-ee9-proxy/src/test/java/org/eclipse/jetty/ee9/proxy/ProxyServletTest.java index c6d41f538c7..db00f9c4f0b 100644 --- a/jetty-ee9/jetty-ee9-proxy/src/test/java/org/eclipse/jetty/ee9/proxy/ProxyServletTest.java +++ b/jetty-ee9/jetty-ee9-proxy/src/test/java/org/eclipse/jetty/ee9/proxy/ProxyServletTest.java @@ -13,7 +13,6 @@ package org.eclipse.jetty.ee9.proxy; -import java.io.EOFException; import java.io.IOException; import java.io.InputStream; import java.io.InterruptedIOException; @@ -1359,7 +1358,7 @@ public class ProxyServletTest chunk1Latch.countDown(); - assertThrows(EOFException.class, () -> + assertThrows(IOException.class, () -> { // Make sure the proxy does not receive chunk2. input.read(); From 1e92759578240b9647da394167fc54fbe2697f26 Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Wed, 8 Feb 2023 16:51:39 +1100 Subject: [PATCH 20/22] #9287 - remove unpaired release of Content.Chunk Signed-off-by: Lachlan Roberts --- .../src/main/java/org/eclipse/jetty/http/MultiPartFormData.java | 1 - 1 file changed, 1 deletion(-) diff --git a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartFormData.java b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartFormData.java index 9007278c6f8..0806f446419 100644 --- a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartFormData.java +++ b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartFormData.java @@ -469,7 +469,6 @@ public class MultiPartFormData extends CompletableFuture Date: Thu, 9 Feb 2023 08:37:00 +1100 Subject: [PATCH 21/22] #9287 - fix further test failures Signed-off-by: Lachlan Roberts --- .../eclipse/jetty/ee10/proxy/ProxyServletTest.java | 3 +-- .../jetty/ee9/nested/AsyncContentProducer.java | 14 ++------------ 2 files changed, 3 insertions(+), 14 deletions(-) diff --git a/jetty-ee10/jetty-ee10-proxy/src/test/java/org/eclipse/jetty/ee10/proxy/ProxyServletTest.java b/jetty-ee10/jetty-ee10-proxy/src/test/java/org/eclipse/jetty/ee10/proxy/ProxyServletTest.java index 255b09601b7..11e71990c03 100644 --- a/jetty-ee10/jetty-ee10-proxy/src/test/java/org/eclipse/jetty/ee10/proxy/ProxyServletTest.java +++ b/jetty-ee10/jetty-ee10-proxy/src/test/java/org/eclipse/jetty/ee10/proxy/ProxyServletTest.java @@ -13,7 +13,6 @@ package org.eclipse.jetty.ee10.proxy; -import java.io.EOFException; import java.io.IOException; import java.io.InputStream; import java.io.InterruptedIOException; @@ -1360,7 +1359,7 @@ public class ProxyServletTest chunk1Latch.countDown(); - assertThrows(EOFException.class, () -> + assertThrows(IOException.class, () -> { // Make sure the proxy does not receive chunk2. input.read(); diff --git a/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/AsyncContentProducer.java b/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/AsyncContentProducer.java index 0107299b0e4..14715eeebae 100644 --- a/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/AsyncContentProducer.java +++ b/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/AsyncContentProducer.java @@ -189,18 +189,8 @@ class AsyncContentProducer implements ContentProducer LOG.trace("consumeAll {}", this, x); } failCurrentContent(x); - // A specific HttpChannel mechanism must be used as the following code - // does not guarantee that the channel will synchronously deliver all - // content it already contains: - // while (true) - // { - // HttpInput.Content content = _httpChannel.produceContent(); - // ... - // } - // as the HttpChannel's produceContent() contract makes no such promise; - // for instance the H2 implementation calls Stream.demand() that may - // deliver the content asynchronously. Tests in StreamResetTest cover this. - boolean atEof = _httpChannel.failAllContent(x); + + boolean atEof = _httpChannel.getRequest().getCoreRequest().consumeAvailable(); if (LOG.isDebugEnabled()) LOG.debug("failed all content of http channel EOF={} {}", atEof, this); return atEof; From 820fbb7331a7cef4f5e823c7717f7e916bd76f77 Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Thu, 9 Feb 2023 15:57:50 +1100 Subject: [PATCH 22/22] #9287 - catch error in ee9 maxRequestSize MultiPart test Signed-off-by: Lachlan Roberts --- .../ee9/servlet/MultiPartServletTest.java | 29 +++++++++++++------ 1 file changed, 20 insertions(+), 9 deletions(-) diff --git a/jetty-ee9/jetty-ee9-servlet/src/test/java/org/eclipse/jetty/ee9/servlet/MultiPartServletTest.java b/jetty-ee9/jetty-ee9-servlet/src/test/java/org/eclipse/jetty/ee9/servlet/MultiPartServletTest.java index 6cc9a3993e4..5f988e44ebf 100644 --- a/jetty-ee9/jetty-ee9-servlet/src/test/java/org/eclipse/jetty/ee9/servlet/MultiPartServletTest.java +++ b/jetty-ee9/jetty-ee9-servlet/src/test/java/org/eclipse/jetty/ee9/servlet/MultiPartServletTest.java @@ -46,6 +46,7 @@ import org.eclipse.jetty.http.HttpScheme; import org.eclipse.jetty.http.HttpStatus; import org.eclipse.jetty.http.MimeTypes; import org.eclipse.jetty.http.MultiPart; +import org.eclipse.jetty.io.EofException; import org.eclipse.jetty.logging.StacklessLogging; import org.eclipse.jetty.server.Server; import org.eclipse.jetty.server.ServerConnector; @@ -58,10 +59,12 @@ import org.junit.jupiter.api.Test; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.instanceOf; import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.startsWith; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.fail; public class MultiPartServletTest { @@ -251,20 +254,28 @@ public class MultiPartServletTest .body(multiPart) .send(listener); - // Write large amount of content to the part. - byte[] byteArray = new byte[1024 * 1024]; - Arrays.fill(byteArray, (byte)1); - for (int i = 0; i < 1024 * 2; i++) + Throwable writeError = null; + try { - content.getOutputStream().write(byteArray); + // Write large amount of content to the part. + byte[] byteArray = new byte[1024 * 1024]; + Arrays.fill(byteArray, (byte)1); + for (int i = 0; i < 1024 * 1024; i++) + { + content.getOutputStream().write(byteArray); + } + fail("We should never be able to write all the content."); + } + catch (Exception e) + { + writeError = e; } - content.close(); + assertThat(writeError, instanceOf(EofException.class)); + + // We should get 400 response, for some reason reading the content throws EofException. Response response = listener.get(30, TimeUnit.SECONDS); assertThat(response.getStatus(), equalTo(HttpStatus.BAD_REQUEST_400)); - String responseContent = IO.toString(listener.getInputStream()); - assertThat(responseContent, containsString("Unable to parse form content")); - assertThat(responseContent, containsString("Request exceeds maxRequestSize")); } @Test