diff --git a/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartFormInputStream.java b/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartFormInputStream.java index 0ea372fa3fc..10386218023 100644 --- a/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartFormInputStream.java +++ b/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartFormInputStream.java @@ -298,9 +298,8 @@ public class MultiPartFormInputStream */ public void cleanUp() throws IOException { - if (_temporary && _file != null && _file.exists()) - if (!_file.delete()) - throw new IOException("Could Not Delete File"); + if (_temporary) + delete(); } /** @@ -376,31 +375,21 @@ public class MultiPartFormInputStream */ public void deleteParts() { - if (!_parsed) - return; - - Collection parts; - try - { - parts = getParts(); - } - catch (IOException e) - { - throw new RuntimeException(e); - } - MultiException err = null; - for (Part p : parts) + for (List parts : _parts.values()) { - try + for (Part p : parts) { - ((MultiPart)p).cleanUp(); - } - catch (Exception e) - { - if (err == null) - err = new MultiException(); - err.add(e); + try + { + ((MultiPart)p).cleanUp(); + } + catch (Exception e) + { + if (err == null) + err = new MultiException(); + err.add(e); + } } } _parts.clear(); @@ -455,6 +444,9 @@ public class MultiPartFormInputStream { if (_err != null) { + if (LOG.isDebugEnabled()) + LOG.debug("MultiPart parsing failure ", _err); + _err.addSuppressed(new Throwable()); if (_err instanceof IOException) throw (IOException)_err; @@ -473,7 +465,9 @@ public class MultiPartFormInputStream if (_parsed) return; _parsed = true; - + + MultiPartParser parser = null; + Handler handler = new Handler(); try { // initialize @@ -509,9 +503,7 @@ public class MultiPartFormInputStream contentTypeBoundary = QuotedStringTokenizer.unquote(value(_contentType.substring(bstart, bend)).trim()); } - Handler handler = new Handler(); - MultiPartParser parser = new MultiPartParser(handler, contentTypeBoundary); - + parser = new MultiPartParser(handler, contentTypeBoundary); byte[] data = new byte[_bufferSize]; int len; long total = 0; @@ -523,7 +515,6 @@ public class MultiPartFormInputStream if (len > 0) { - // keep running total of size of bytes read from input and throw an exception if exceeds MultipartConfigElement._maxRequestSize total += len; if (_config.getMaxRequestSize() > 0 && total > _config.getMaxRequestSize()) @@ -573,6 +564,10 @@ public class MultiPartFormInputStream catch (Throwable e) { _err = e; + + // Notify parser if failure occurs + if (parser != null) + parser.parse(BufferUtil.EMPTY_BUFFER, true); } } @@ -720,6 +715,16 @@ public class MultiPartFormInputStream { if (LOG.isDebugEnabled()) LOG.debug("Early EOF {}", MultiPartFormInputStream.this); + + try + { + if (_part != null) + _part.close(); + } + catch (IOException e) + { + LOG.warn("part could not be closed", e); + } } public void reset() diff --git a/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartParser.java b/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartParser.java index 0832e0f8641..7e4b12da9d8 100644 --- a/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartParser.java +++ b/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartParser.java @@ -131,12 +131,8 @@ public class MultiPartParser private HttpTokens.Token next(ByteBuffer buffer) { byte ch = buffer.get(); - HttpTokens.Token t = HttpTokens.TOKENS[0xff & ch]; - if (DEBUG) - LOG.debug("token={}",t); - switch(t.getType()) { case CNTL: @@ -271,6 +267,9 @@ public class MultiPartParser /* ------------------------------------------------------------------------------- */ private void parsePreamble(ByteBuffer buffer) { + if (LOG.isDebugEnabled()) + LOG.debug("parsePreamble({})", BufferUtil.toDetailString(buffer)); + if (_partialBoundary > 0) { int partial = _delimiterSearch.startsWith(buffer.array(), buffer.arrayOffset() + buffer.position(), buffer.remaining(), _partialBoundary); @@ -307,6 +306,9 @@ public class MultiPartParser /* ------------------------------------------------------------------------------- */ private void parseDelimiter(ByteBuffer buffer) { + if (LOG.isDebugEnabled()) + LOG.debug("parseDelimiter({})", BufferUtil.toDetailString(buffer)); + while (__delimiterStates.contains(_state) && hasNextByte(buffer)) { HttpTokens.Token t = next(buffer); @@ -354,6 +356,9 @@ public class MultiPartParser */ protected boolean parseMimePartHeaders(ByteBuffer buffer) { + if (LOG.isDebugEnabled()) + LOG.debug("parseMimePartHeaders({})", BufferUtil.toDetailString(buffer)); + // Process headers while (_state == State.BODY_PART && hasNextByte(buffer)) { @@ -575,6 +580,8 @@ public class MultiPartParser protected boolean parseOctetContent(ByteBuffer buffer) { + if (LOG.isDebugEnabled()) + LOG.debug("parseOctetContent({})", BufferUtil.toDetailString(buffer)); // Starts With if (_partialBoundary > 0) diff --git a/jetty-http/src/test/java/org/eclipse/jetty/http/MultiPartFormInputStreamTest.java b/jetty-http/src/test/java/org/eclipse/jetty/http/MultiPartFormInputStreamTest.java index a2cc013371a..7fcfdc5a62b 100644 --- a/jetty-http/src/test/java/org/eclipse/jetty/http/MultiPartFormInputStreamTest.java +++ b/jetty-http/src/test/java/org/eclipse/jetty/http/MultiPartFormInputStreamTest.java @@ -217,6 +217,7 @@ public class MultiPartFormInputStreamTest @Test public void testNoBody() + throws Exception { String body = ""; @@ -277,6 +278,7 @@ public class MultiPartFormInputStreamTest @Test public void testWhitespaceBodyWithCRLF() + throws Exception { String whitespace = " \n\n\n\r\n\r\n\r\n\r\n"; @@ -292,6 +294,7 @@ public class MultiPartFormInputStreamTest @Test public void testWhitespaceBody() + throws Exception { String whitespace = " "; @@ -400,6 +403,7 @@ public class MultiPartFormInputStreamTest @Test public void testRequestTooBig () + throws Exception { MultipartConfigElement config = new MultipartConfigElement(_dirname, 60, 100, 50); MultiPartFormInputStream mpis = new MultiPartFormInputStream(new ByteArrayInputStream(_multi.getBytes()), @@ -415,6 +419,7 @@ public class MultiPartFormInputStreamTest @Test public void testRequestTooBigThrowsErrorOnGetParts () + throws Exception { MultipartConfigElement config = new MultipartConfigElement(_dirname, 60, 100, 50); MultiPartFormInputStream mpis = new MultiPartFormInputStream(new ByteArrayInputStream(_multi.getBytes()), @@ -434,6 +439,7 @@ public class MultiPartFormInputStreamTest @Test public void testFileTooBig() + throws Exception { MultipartConfigElement config = new MultipartConfigElement(_dirname, 40, 1024, 30); MultiPartFormInputStream mpis = new MultiPartFormInputStream(new ByteArrayInputStream(_multi.getBytes()), @@ -449,6 +455,7 @@ public class MultiPartFormInputStreamTest @Test public void testFileTooBigThrowsErrorOnGetParts() + throws Exception { MultipartConfigElement config = new MultipartConfigElement(_dirname, 40, 1024, 30); MultiPartFormInputStream mpis = new MultiPartFormInputStream(new ByteArrayInputStream(_multi.getBytes()), @@ -550,6 +557,7 @@ public class MultiPartFormInputStreamTest @Test public void testCROnlyRequest() + throws Exception { String str = "--AaB03x\r" + "content-disposition: form-data; name=\"field1\"\r" + @@ -576,6 +584,7 @@ public class MultiPartFormInputStreamTest @Test public void testCRandLFMixRequest() + throws Exception { String str = "--AaB03x\r" + "content-disposition: form-data; name=\"field1\"\r" + diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/MultiParts.java b/jetty-server/src/main/java/org/eclipse/jetty/server/MultiParts.java index 375b94abb14..cd4566813cb 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/MultiParts.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/MultiParts.java @@ -37,8 +37,8 @@ import org.eclipse.jetty.server.handler.ContextHandler.Context; */ public interface MultiParts extends Closeable { - Collection getParts(); - Part getPart(String name); + Collection getParts() throws IOException; + Part getPart(String name) throws IOException; boolean isEmpty(); ContextHandler.Context getContext(); @@ -51,32 +51,18 @@ public interface MultiParts extends Closeable { _httpParser = new MultiPartFormInputStream(in, contentType, config, contextTmpDir); _context = request.getContext(); - _httpParser.getParts(); } @Override - public Collection getParts() + public Collection getParts() throws IOException { - try - { - return _httpParser.getParts(); - } - catch (IOException e) - { - throw new RuntimeException(e); - } + return _httpParser.getParts(); } @Override - public Part getPart(String name) { - try - { - return _httpParser.getPart(name); - } - catch (IOException e) - { - throw new RuntimeException(e); - } + public Part getPart(String name) throws IOException + { + return _httpParser.getPart(name); } @Override @@ -96,6 +82,5 @@ public interface MultiParts extends Closeable { return _context; } - } } diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java b/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java index 8fa630ec16a..860891421a9 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java @@ -477,7 +477,7 @@ public class Request implements HttpServletRequest throw new BadMessageException(HttpStatus.NOT_IMPLEMENTED_501,"Unsupported Content-Encoding"); getParts(_contentParameters); } - catch (IOException | ServletException e) + catch (IOException e) { LOG.debug(e); throw new RuntimeIOException(e); @@ -2317,7 +2317,6 @@ public class Request implements HttpServletRequest public Part getPart(String name) throws IOException, ServletException { getParts(); - return _multiParts.getPart(name); } @@ -2331,7 +2330,7 @@ public class Request implements HttpServletRequest return getParts(null); } - private Collection getParts(MultiMap params) throws IOException, ServletException + private Collection getParts(MultiMap params) throws IOException { if (_multiParts == null) _multiParts = (MultiParts)getAttribute(__MULTIPARTS); @@ -2342,12 +2341,9 @@ public class Request implements HttpServletRequest if (config == null) throw new IllegalStateException("No multipart config for servlet"); - _multiParts = newMultiParts(getInputStream(), - getContentType(), config, - (_context != null?(File)_context.getAttribute("javax.servlet.context.tempdir"):null)); - + _multiParts = newMultiParts(config); setAttribute(__MULTIPARTS, _multiParts); - Collection parts = _multiParts.getParts(); //causes parsing + Collection parts = _multiParts.getParts(); String _charset_ = null; Part charsetPart = _multiParts.getPart("_charset_"); @@ -2409,9 +2405,9 @@ public class Request implements HttpServletRequest } - private MultiParts newMultiParts(ServletInputStream inputStream, String contentType, MultipartConfigElement config, Object object) throws IOException + private MultiParts newMultiParts(MultipartConfigElement config) throws IOException { - return new MultiParts.MultiPartsHttpParser(getInputStream(), contentType, config, + return new MultiParts.MultiPartsHttpParser(getInputStream(), getContentType(), config, (_context != null ? (File) _context.getAttribute("javax.servlet.context.tempdir") : null), this); } diff --git a/jetty-servlet/pom.xml b/jetty-servlet/pom.xml index 795784d0c78..bf018c32c37 100644 --- a/jetty-servlet/pom.xml +++ b/jetty-servlet/pom.xml @@ -65,6 +65,12 @@ ${project.version} test + + org.eclipse.jetty + jetty-client + ${project.version} + test + diff --git a/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/ServletHolder.java b/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/ServletHolder.java index 87a13e21667..1b192237ea2 100644 --- a/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/ServletHolder.java +++ b/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/ServletHolder.java @@ -748,11 +748,14 @@ public class ServletHolder extends Holder implements UserIdentity.Scope //cleaned up correctly if (((Registration)getRegistration()).getMultipartConfig() != null) { + if (LOG.isDebugEnabled()) + LOG.debug("multipart cleanup listener added for {}", this); + //Register a listener to delete tmp files that are created as a result of this //servlet calling Request.getPart() or Request.getParts() - ContextHandler ch = ContextHandler.getContextHandler(getServletHandler().getServletContext()); - ch.addEventListener(MultiPartCleanerListener.INSTANCE); + if(!Arrays.asList(ch.getEventListeners()).contains(MultiPartCleanerListener.INSTANCE)) + ch.addEventListener(MultiPartCleanerListener.INSTANCE); } } diff --git a/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/MultiPartServletTest.java b/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/MultiPartServletTest.java new file mode 100644 index 00000000000..dd379bd2be3 --- /dev/null +++ b/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/MultiPartServletTest.java @@ -0,0 +1,164 @@ +// +// ======================================================================== +// Copyright (c) 1995-2019 Mort Bay Consulting Pty. Ltd. +// ------------------------------------------------------------------------ +// All rights reserved. This program and the accompanying materials +// are made available under the terms of the Eclipse Public License v1.0 +// and Apache License v2.0 which accompanies this distribution. +// +// The Eclipse Public License is available at +// http://www.eclipse.org/legal/epl-v10.html +// +// The Apache License v2.0 is available at +// http://www.opensource.org/licenses/apache2.0.php +// +// You may elect to redistribute this code under either of these licenses. +// ======================================================================== +// + +package org.eclipse.jetty.servlet; + +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.Arrays; +import java.util.stream.Stream; +import javax.servlet.MultipartConfigElement; +import javax.servlet.ServletException; +import javax.servlet.http.HttpServlet; +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; +import javax.servlet.http.Part; + +import org.eclipse.jetty.client.HttpClient; +import org.eclipse.jetty.client.api.ContentResponse; +import org.eclipse.jetty.client.util.BytesContentProvider; +import org.eclipse.jetty.client.util.MultiPartContentProvider; +import org.eclipse.jetty.http.HttpMethod; +import org.eclipse.jetty.http.HttpScheme; +import org.eclipse.jetty.http.MimeTypes; +import org.eclipse.jetty.http.MultiPartFormInputStream; +import org.eclipse.jetty.server.HttpChannel; +import org.eclipse.jetty.server.HttpConnectionFactory; +import org.eclipse.jetty.server.MultiPartFormDataCompliance; +import org.eclipse.jetty.server.Server; +import org.eclipse.jetty.server.ServerConnector; +import org.eclipse.jetty.util.IO; +import org.eclipse.jetty.util.log.Log; +import org.eclipse.jetty.util.log.Logger; +import org.eclipse.jetty.util.log.StacklessLogging; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.is; +import static org.junit.jupiter.api.Assertions.assertEquals; + +public class MultiPartServletTest +{ + private static final Logger LOG = Log.getLogger(MultiPartServletTest.class); + + private Server server; + private ServerConnector connector; + private HttpClient client; + private Path tmpDir; + + private static final int MAX_FILE_SIZE = 512 * 1024; + private static final int LARGE_MESSAGE_SIZE = 1024 * 1024; + + public static Stream data() + { + return Arrays.asList(MultiPartFormDataCompliance.values()).stream().map(Arguments::of); + } + + public static class MultiPartServlet extends HttpServlet + { + @Override + protected void doPost(HttpServletRequest req, HttpServletResponse resp) throws ServletException, IOException + { + if (!req.getContentType().contains(MimeTypes.Type.MULTIPART_FORM_DATA.asString())) + { + resp.setContentType("text/plain"); + resp.getWriter().println("not content type " + MimeTypes.Type.MULTIPART_FORM_DATA); + resp.getWriter().println("contentType: " + req.getContentType()); + return; + } + + resp.setContentType("text/plain"); + for (Part part : req.getParts()) + { + resp.getWriter().println("Part: name=" + part.getName() + ", size=" + part.getSize()); + } + } + } + + @BeforeEach + public void start() throws Exception + { + tmpDir = Files.createTempDirectory(MultiPartServletTest.class.getSimpleName()); + + server = new Server(); + connector = new ServerConnector(server); + server.addConnector(connector); + + ServletContextHandler contextHandler = new ServletContextHandler(ServletContextHandler.SESSIONS); + contextHandler.setContextPath("/"); + ServletHolder servletHolder = contextHandler.addServlet(MultiPartServlet.class, "/"); + + MultipartConfigElement config = new MultipartConfigElement(tmpDir.toAbsolutePath().toString(), + MAX_FILE_SIZE, -1, 1); + servletHolder.getRegistration().setMultipartConfig(config); + + server.setHandler(contextHandler); + + server.start(); + + client = new HttpClient(); + client.start(); + } + + @AfterEach + public void stop() throws Exception + { + client.stop(); + server.stop(); + + IO.delete(tmpDir.toFile()); + } + + @ParameterizedTest + @MethodSource("data") + public void testTempFilesDeletedOnError(MultiPartFormDataCompliance compliance) throws Exception + { + connector.getConnectionFactory(HttpConnectionFactory.class).getHttpConfiguration() + .setMultiPartFormDataCompliance(compliance); + + byte[] byteArray = new byte[LARGE_MESSAGE_SIZE]; + for (int i=0; i