diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/MultiPartCleanerListener.java b/jetty-server/src/main/java/org/eclipse/jetty/server/MultiPartCleanerListener.java deleted file mode 100644 index 4340ffbc842..00000000000 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/MultiPartCleanerListener.java +++ /dev/null @@ -1,63 +0,0 @@ -// -// ======================================================================== -// 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.server; - -import javax.servlet.ServletRequestEvent; -import javax.servlet.ServletRequestListener; - -import org.eclipse.jetty.server.handler.ContextHandler; - -public class MultiPartCleanerListener implements ServletRequestListener -{ - public static final MultiPartCleanerListener INSTANCE = new MultiPartCleanerListener(); - - protected MultiPartCleanerListener() - { - } - - @Override - public void requestDestroyed(ServletRequestEvent sre) - { - //Clean up any tmp files created by MultiPartInputStream - MultiParts parts = (MultiParts)sre.getServletRequest().getAttribute(Request.__MULTIPARTS); - if (parts != null) - { - ContextHandler.Context context = parts.getContext(); - - //Only do the cleanup if we are exiting from the context in which a servlet parsed the multipart files - if (context == sre.getServletContext()) - { - try - { - parts.close(); - } - catch (Throwable e) - { - sre.getServletContext().log("Errors deleting multipart tmp files", e); - } - } - } - } - - @Override - public void requestInitialized(ServletRequestEvent sre) - { - //nothing to do, multipart config set up by ServletHolder.handle() - } -} 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 deleted file mode 100644 index b6deb13c132..00000000000 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/MultiParts.java +++ /dev/null @@ -1,69 +0,0 @@ -// -// ======================================================================== -// 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.server; - -import java.io.Closeable; -import java.io.File; -import java.io.IOException; -import java.io.InputStream; -import java.util.Collection; -import javax.servlet.MultipartConfigElement; -import javax.servlet.http.Part; - -import org.eclipse.jetty.http.MultiPartFormInputStream; -import org.eclipse.jetty.server.handler.ContextHandler; -import org.eclipse.jetty.server.handler.ContextHandler.Context; - -public class MultiParts implements Closeable -{ - private final MultiPartFormInputStream _httpParser; - private final ContextHandler.Context _context; - - public MultiParts(InputStream in, String contentType, MultipartConfigElement config, File contextTmpDir, Request request) - { - _httpParser = new MultiPartFormInputStream(in, contentType, config, contextTmpDir); - _context = request.getContext(); - } - - public Collection getParts() throws IOException - { - return _httpParser.getParts(); - } - - public Part getPart(String name) throws IOException - { - return _httpParser.getPart(name); - } - - @Override - public void close() - { - _httpParser.deleteParts(); - } - - public boolean isEmpty() - { - return _httpParser.isEmpty(); - } - - public Context getContext() - { - 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 bf198f95791..21d5339184e 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 @@ -81,6 +81,7 @@ import org.eclipse.jetty.http.HttpURI; import org.eclipse.jetty.http.HttpVersion; import org.eclipse.jetty.http.MetaData; import org.eclipse.jetty.http.MimeTypes; +import org.eclipse.jetty.http.MultiPartFormInputStream; import org.eclipse.jetty.http.pathmap.PathSpec; import org.eclipse.jetty.http.pathmap.ServletPathSpec; import org.eclipse.jetty.io.RuntimeIOException; @@ -141,7 +142,6 @@ import org.eclipse.jetty.util.log.Logger; public class Request implements HttpServletRequest { public static final String __MULTIPART_CONFIG_ELEMENT = "org.eclipse.jetty.multipartConfig"; - public static final String __MULTIPARTS = "org.eclipse.jetty.multiParts"; private static final Logger LOG = Log.getLogger(Request.class); private static final Collection __defaultLocale = Collections.singleton(Locale.getDefault()); @@ -228,7 +228,7 @@ public class Request implements HttpServletRequest private HttpSession _session; private SessionHandler _sessionHandler; private long _timeStamp; - private MultiParts _multiParts; //if the request is a multi-part mime + private MultiPartFormInputStream _multiParts; //if the request is a multi-part mime private AsyncContextState _async; private List _sessions; //list of sessions used during lifetime of request @@ -1504,6 +1504,19 @@ public class Request implements HttpServletRequest for (Session s:_sessions) leaveSession(s); } + + //Clean up any tmp files created by MultiPartInputStream + if (_multiParts != null) + { + try + { + _multiParts.deleteParts(); + } + catch (Throwable e) + { + LOG.warn("Errors deleting multipart tmp files", e); + } + } } /** @@ -2305,9 +2318,6 @@ public class Request implements HttpServletRequest private Collection getParts(MultiMap params) throws IOException { - if (_multiParts == null) - _multiParts = (MultiParts)getAttribute(__MULTIPARTS); - if (_multiParts == null) { MultipartConfigElement config = (MultipartConfigElement)getAttribute(__MULTIPART_CONFIG_ELEMENT); @@ -2315,7 +2325,6 @@ public class Request implements HttpServletRequest throw new IllegalStateException("No multipart config for servlet"); _multiParts = newMultiParts(config); - setAttribute(__MULTIPARTS, _multiParts); Collection parts = _multiParts.getParts(); String formCharset = null; @@ -2377,10 +2386,10 @@ public class Request implements HttpServletRequest return _multiParts.getParts(); } - private MultiParts newMultiParts(MultipartConfigElement config) throws IOException + private MultiPartFormInputStream newMultiParts(MultipartConfigElement config) throws IOException { - return new MultiParts(getInputStream(), getContentType(), config, - (_context != null ? (File)_context.getAttribute("javax.servlet.context.tempdir") : null), this); + return new MultiPartFormInputStream(getInputStream(), getContentType(), config, + (_context != null ? (File)_context.getAttribute("javax.servlet.context.tempdir") : null)); } @Override diff --git a/jetty-server/src/test/java/org/eclipse/jetty/server/RequestTest.java b/jetty-server/src/test/java/org/eclipse/jetty/server/RequestTest.java index 88e3225bf14..841360cc397 100644 --- a/jetty-server/src/test/java/org/eclipse/jetty/server/RequestTest.java +++ b/jetty-server/src/test/java/org/eclipse/jetty/server/RequestTest.java @@ -38,7 +38,6 @@ import javax.servlet.DispatcherType; import javax.servlet.MultipartConfigElement; import javax.servlet.ServletException; import javax.servlet.ServletInputStream; -import javax.servlet.ServletRequestEvent; import javax.servlet.http.Cookie; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; @@ -341,28 +340,13 @@ public class RequestTest testTmpDir.deleteOnExit(); assertTrue(testTmpDir.list().length == 0); + // We should have two tmp files after parsing the multipart form. + RequestTester tester = (request, response) -> testTmpDir.list().length == 2; + ContextHandler contextHandler = new ContextHandler(); contextHandler.setContextPath("/foo"); contextHandler.setResourceBase("."); - contextHandler.setHandler(new MultiPartRequestHandler(testTmpDir)); - contextHandler.addEventListener(new MultiPartCleanerListener() - { - - @Override - public void requestDestroyed(ServletRequestEvent sre) - { - MultiParts m = (MultiParts)sre.getServletRequest().getAttribute(Request.__MULTIPARTS); - assertNotNull(m); - ContextHandler.Context c = m.getContext(); - assertNotNull(c); - assertTrue(c == sre.getServletContext()); - assertTrue(!m.isEmpty()); - assertTrue(testTmpDir.list().length == 2); - super.requestDestroyed(sre); - String[] files = testTmpDir.list(); - assertTrue(files.length == 0); - } - }); + contextHandler.setHandler(new MultiPartRequestHandler(testTmpDir, tester)); _server.stop(); _server.setHandler(contextHandler); _server.start(); @@ -387,51 +371,16 @@ public class RequestTest multipart; String responses = _connector.getResponse(request); - //System.err.println(responses); assertTrue(responses.startsWith("HTTP/1.1 200")); - } - @Test - public void testHttpMultiPart() throws Exception - { - final File testTmpDir = File.createTempFile("reqtest", null); - if (testTmpDir.exists()) - testTmpDir.delete(); - testTmpDir.mkdir(); - testTmpDir.deleteOnExit(); - assertTrue(testTmpDir.list().length == 0); - - ContextHandler contextHandler = new ContextHandler(); - contextHandler.setContextPath("/foo"); - contextHandler.setResourceBase("."); - contextHandler.setHandler(new MultiPartRequestHandler(testTmpDir)); - - _server.stop(); - _server.setHandler(contextHandler); - _server.start(); - - String multipart = " --AaB03x\r" + - "content-disposition: form-data; name=\"field1\"\r" + - "\r" + - "Joe Blow\r" + - "--AaB03x\r" + - "content-disposition: form-data; name=\"stuff\"; filename=\"foo.upload\"\r" + - "Content-Type: text/plain;charset=ISO-8859-1\r" + - "\r" + - "000000000000000000000000000000000000000000000000000\r" + - "--AaB03x--\r"; - - String request = "GET /foo/x.html HTTP/1.1\r\n" + + // We know the previous request has completed if another request can be processed. + String cleanupRequest = "GET /foo/cleanup HTTP/1.1\r\n" + "Host: whatever\r\n" + - "Content-Type: multipart/form-data; boundary=\"AaB03x\"\r\n" + - "Content-Length: " + multipart.getBytes().length + "\r\n" + "Connection: close\r\n" + - "\r\n" + - multipart; - - String responses = _connector.getResponse(request); - //System.err.println(responses); - assertThat(responses, Matchers.startsWith("HTTP/1.1 500")); + "\r\n"; + String cleanupResponse = _connector.getResponse(cleanupRequest); + assertTrue(cleanupResponse.startsWith("HTTP/1.1 200")); + assertThat(testTmpDir.list().length, is(0)); } @Test @@ -449,22 +398,6 @@ public class RequestTest contextHandler.setContextPath("/foo"); contextHandler.setResourceBase("."); contextHandler.setHandler(new BadMultiPartRequestHandler(testTmpDir)); - contextHandler.addEventListener(new MultiPartCleanerListener() - { - - @Override - public void requestDestroyed(ServletRequestEvent sre) - { - MultiParts m = (MultiParts)sre.getServletRequest().getAttribute(Request.__MULTIPARTS); - assertNotNull(m); - ContextHandler.Context c = m.getContext(); - assertNotNull(c); - assertTrue(c == sre.getServletContext()); - super.requestDestroyed(sre); - String[] files = testTmpDir.list(); - assertTrue(files.length == 0); - } - }); _server.stop(); _server.setHandler(contextHandler); _server.start(); @@ -494,6 +427,15 @@ public class RequestTest //System.err.println(responses); assertTrue(responses.startsWith("HTTP/1.1 500")); } + + // We know the previous request has completed if another request can be processed. + String cleanupRequest = "GET /foo/cleanup HTTP/1.1\r\n" + + "Host: whatever\r\n" + + "Connection: close\r\n" + + "\r\n"; + String cleanupResponse = _connector.getResponse(cleanupRequest); + assertTrue(cleanupResponse.startsWith("HTTP/1.1 200")); + assertThat(testTmpDir.list().length, is(0)); } @Test @@ -1818,20 +1760,27 @@ public class RequestTest private class MultiPartRequestHandler extends AbstractHandler { + RequestTester checker; File tmpDir; - public MultiPartRequestHandler(File tmpDir) + public MultiPartRequestHandler(File tmpDir, RequestTester checker) { this.tmpDir = tmpDir; + this.checker = checker; } @Override public void handle(String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException { ((Request)request).setHandled(true); + if ("/cleanup".equals(target)) + { + response.setStatus(200); + return; + } + try { - MultipartConfigElement mpce = new MultipartConfigElement(tmpDir.getAbsolutePath(), -1, -1, 2); request.setAttribute(Request.__MULTIPART_CONFIG_ELEMENT, mpce); @@ -1850,6 +1799,9 @@ public class RequestTest response.addHeader("Violation", v); } } + + if (checker != null && !checker.check(request, response)) + response.sendError(500); } catch (IllegalStateException e) { @@ -1877,6 +1829,12 @@ public class RequestTest public void handle(String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException { ((Request)request).setHandled(true); + if ("/cleanup".equals(target)) + { + response.setStatus(200); + return; + } + try { 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 2fe67f75b12..b3b4ae9b07f 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 @@ -49,7 +49,6 @@ import javax.servlet.http.HttpServletResponse; import org.eclipse.jetty.security.IdentityService; import org.eclipse.jetty.security.RunAsToken; -import org.eclipse.jetty.server.MultiPartCleanerListener; import org.eclipse.jetty.server.Request; import org.eclipse.jetty.server.UserIdentity; import org.eclipse.jetty.server.handler.ContextHandler; @@ -592,8 +591,6 @@ public class ServletHolder extends Holder implements UserIdentity.Scope else if (_forcedPath != null) detectJspContainer(); - initMultiPart(); - if (LOG.isDebugEnabled()) LOG.debug("Servlet.init {} for {}", _servlet, getName()); _servlet.init(_config); @@ -652,29 +649,6 @@ public class ServletHolder extends Holder implements UserIdentity.Scope scratch.mkdir(); } - /** - * Register a ServletRequestListener that will ensure tmp multipart - * files are deleted when the request goes out of scope. - * - * @throws Exception if unable to init the multipart - */ - protected void initMultiPart() throws Exception - { - //if this servlet can handle multipart requests, ensure tmp files will be - //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()); - if (!ch.getEventListeners().contains(MultiPartCleanerListener.INSTANCE)) - ch.addEventListener(MultiPartCleanerListener.INSTANCE); - } - } - @Override public ContextHandler getContextHandler() {