Eclipse
/
jetty.project
mirror of https://github.com/jetty/jetty.project.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Issue #4368 directly use MultiPartFormInputStream instead of MultiParts 

MultiPart cleanup is now done in Request.onCompleted()

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
This commit is contained in:
Lachlan Roberts 2019-11-27 17:20:01 +11:00
parent 6988c4feaa
commit 76bb8d2327
5 changed files with 55 additions and 246 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

63
jetty-server/src/main/java/org/eclipse/jetty/server/MultiPartCleanerListener.java
View File

@ -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()
}
}

69
jetty-server/src/main/java/org/eclipse/jetty/server/MultiParts.java
View File

@ -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<Part> 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;
}
}

27
jetty-server/src/main/java/org/eclipse/jetty/server/Request.java
View File

@ -81,6 +81,7 @@ import org.eclipse.jetty.http.HttpURI;
import org.eclipse.jetty.http.HttpVersion; import org.eclipse.jetty.http.HttpVersion;
import org.eclipse.jetty.http.MetaData; import org.eclipse.jetty.http.MetaData;
import org.eclipse.jetty.http.MimeTypes; 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.PathSpec;
import org.eclipse.jetty.http.pathmap.ServletPathSpec; import org.eclipse.jetty.http.pathmap.ServletPathSpec;
import org.eclipse.jetty.io.RuntimeIOException; import org.eclipse.jetty.io.RuntimeIOException;
@ -141,7 +142,6 @@ import org.eclipse.jetty.util.log.Logger;
public class Request implements HttpServletRequest public class Request implements HttpServletRequest
{ {
public static final String __MULTIPART_CONFIG_ELEMENT = "org.eclipse.jetty.multipartConfig"; 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 Logger LOG = Log.getLogger(Request.class);
private static final Collection<Locale> __defaultLocale = Collections.singleton(Locale.getDefault()); private static final Collection<Locale> __defaultLocale = Collections.singleton(Locale.getDefault());
@ -228,7 +228,7 @@ public class Request implements HttpServletRequest
private HttpSession _session; private HttpSession _session;
private SessionHandler _sessionHandler; private SessionHandler _sessionHandler;
private long _timeStamp; 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 AsyncContextState _async;
private List<Session> _sessions; //list of sessions used during lifetime of request private List<Session> _sessions; //list of sessions used during lifetime of request
@ -1504,6 +1504,19 @@ public class Request implements HttpServletRequest
for (Session s:_sessions) for (Session s:_sessions)
leaveSession(s); 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<Part> getParts(MultiMap<String> params) throws IOException private Collection<Part> getParts(MultiMap<String> params) throws IOException
{ {
if (_multiParts == null)
_multiParts = (MultiParts)getAttribute(__MULTIPARTS);
if (_multiParts == null) if (_multiParts == null)
{ {
MultipartConfigElement config = (MultipartConfigElement)getAttribute(__MULTIPART_CONFIG_ELEMENT); MultipartConfigElement config = (MultipartConfigElement)getAttribute(__MULTIPART_CONFIG_ELEMENT);
@ -2315,7 +2325,6 @@ public class Request implements HttpServletRequest
throw new IllegalStateException("No multipart config for servlet"); throw new IllegalStateException("No multipart config for servlet");
_multiParts = newMultiParts(config); _multiParts = newMultiParts(config);
setAttribute(__MULTIPARTS, _multiParts);
Collection<Part> parts = _multiParts.getParts(); Collection<Part> parts = _multiParts.getParts();
String formCharset = null; String formCharset = null;
@ -2377,10 +2386,10 @@ public class Request implements HttpServletRequest
return _multiParts.getParts(); return _multiParts.getParts();
} }
private MultiParts newMultiParts(MultipartConfigElement config) throws IOException private MultiPartFormInputStream newMultiParts(MultipartConfigElement config) throws IOException
{ {
return new MultiParts(getInputStream(), getContentType(), config, return new MultiPartFormInputStream(getInputStream(), getContentType(), config,
(_context != null ? (File)_context.getAttribute("javax.servlet.context.tempdir") : null), this); (_context != null ? (File)_context.getAttribute("javax.servlet.context.tempdir") : null));
} }
@Override @Override

116
jetty-server/src/test/java/org/eclipse/jetty/server/RequestTest.java
View File

@ -38,7 +38,6 @@ import javax.servlet.DispatcherType;
import javax.servlet.MultipartConfigElement; import javax.servlet.MultipartConfigElement;
import javax.servlet.ServletException; import javax.servlet.ServletException;
import javax.servlet.ServletInputStream; import javax.servlet.ServletInputStream;
import javax.servlet.ServletRequestEvent;
import javax.servlet.http.Cookie; import javax.servlet.http.Cookie;
import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse; import javax.servlet.http.HttpServletResponse;
@ -341,28 +340,13 @@ public class RequestTest
testTmpDir.deleteOnExit(); testTmpDir.deleteOnExit();
assertTrue(testTmpDir.list().length == 0); 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 contextHandler = new ContextHandler();
contextHandler.setContextPath("/foo"); contextHandler.setContextPath("/foo");
contextHandler.setResourceBase("."); contextHandler.setResourceBase(".");
contextHandler.setHandler(new MultiPartRequestHandler(testTmpDir)); contextHandler.setHandler(new MultiPartRequestHandler(testTmpDir, tester));
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);
}
});
_server.stop(); _server.stop();
_server.setHandler(contextHandler); _server.setHandler(contextHandler);
_server.start(); _server.start();
@ -387,51 +371,16 @@ public class RequestTest
multipart; multipart;
String responses = _connector.getResponse(request); String responses = _connector.getResponse(request);
//System.err.println(responses);
assertTrue(responses.startsWith("HTTP/1.1 200")); assertTrue(responses.startsWith("HTTP/1.1 200"));
}
@Test // We know the previous request has completed if another request can be processed.
public void testHttpMultiPart() throws Exception String cleanupRequest = "GET /foo/cleanup HTTP/1.1\r\n" +
{
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" +
"Host: whatever\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" + "Connection: close\r\n" +
"\r\n" + "\r\n";
multipart; String cleanupResponse = _connector.getResponse(cleanupRequest);
assertTrue(cleanupResponse.startsWith("HTTP/1.1 200"));
String responses = _connector.getResponse(request); assertThat(testTmpDir.list().length, is(0));
//System.err.println(responses);
assertThat(responses, Matchers.startsWith("HTTP/1.1 500"));
} }
@Test @Test
@ -449,22 +398,6 @@ public class RequestTest
contextHandler.setContextPath("/foo"); contextHandler.setContextPath("/foo");
contextHandler.setResourceBase("."); contextHandler.setResourceBase(".");
contextHandler.setHandler(new BadMultiPartRequestHandler(testTmpDir)); 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.stop();
_server.setHandler(contextHandler); _server.setHandler(contextHandler);
_server.start(); _server.start();
@ -494,6 +427,15 @@ public class RequestTest
//System.err.println(responses); //System.err.println(responses);
assertTrue(responses.startsWith("HTTP/1.1 500")); 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 @Test
@ -1818,20 +1760,27 @@ public class RequestTest
private class MultiPartRequestHandler extends AbstractHandler private class MultiPartRequestHandler extends AbstractHandler
{ {
RequestTester checker;
File tmpDir; File tmpDir;
public MultiPartRequestHandler(File tmpDir) public MultiPartRequestHandler(File tmpDir, RequestTester checker)
{ {
this.tmpDir = tmpDir; this.tmpDir = tmpDir;
this.checker = checker;
} }
@Override @Override
public void handle(String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException public void handle(String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException
{ {
((Request)request).setHandled(true); ((Request)request).setHandled(true);
if ("/cleanup".equals(target))
{
response.setStatus(200);
return;
}
try try
{ {
MultipartConfigElement mpce = new MultipartConfigElement(tmpDir.getAbsolutePath(), -1, -1, 2); MultipartConfigElement mpce = new MultipartConfigElement(tmpDir.getAbsolutePath(), -1, -1, 2);
request.setAttribute(Request.__MULTIPART_CONFIG_ELEMENT, mpce); request.setAttribute(Request.__MULTIPART_CONFIG_ELEMENT, mpce);
@ -1850,6 +1799,9 @@ public class RequestTest
response.addHeader("Violation", v); response.addHeader("Violation", v);
} }
} }
if (checker != null && !checker.check(request, response))
response.sendError(500);
} }
catch (IllegalStateException e) catch (IllegalStateException e)
{ {
@ -1877,6 +1829,12 @@ public class RequestTest
public void handle(String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException public void handle(String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException
{ {
((Request)request).setHandled(true); ((Request)request).setHandled(true);
if ("/cleanup".equals(target))
{
response.setStatus(200);
return;
}
try try
{ {

26
jetty-servlet/src/main/java/org/eclipse/jetty/servlet/ServletHolder.java
View File

@ -49,7 +49,6 @@ import javax.servlet.http.HttpServletResponse;
import org.eclipse.jetty.security.IdentityService; import org.eclipse.jetty.security.IdentityService;
import org.eclipse.jetty.security.RunAsToken; import org.eclipse.jetty.security.RunAsToken;
import org.eclipse.jetty.server.MultiPartCleanerListener;
import org.eclipse.jetty.server.Request; import org.eclipse.jetty.server.Request;
import org.eclipse.jetty.server.UserIdentity; import org.eclipse.jetty.server.UserIdentity;
import org.eclipse.jetty.server.handler.ContextHandler; import org.eclipse.jetty.server.handler.ContextHandler;
@ -592,8 +591,6 @@ public class ServletHolder extends Holder<Servlet> implements UserIdentity.Scope
else if (_forcedPath != null) else if (_forcedPath != null)
detectJspContainer(); detectJspContainer();
initMultiPart();
if (LOG.isDebugEnabled()) if (LOG.isDebugEnabled())
LOG.debug("Servlet.init {} for {}", _servlet, getName()); LOG.debug("Servlet.init {} for {}", _servlet, getName());
_servlet.init(_config); _servlet.init(_config);
@ -652,29 +649,6 @@ public class ServletHolder extends Holder<Servlet> implements UserIdentity.Scope
scratch.mkdir(); 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 @Override
public ContextHandler getContextHandler() public ContextHandler getContextHandler()
{ {