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

Merge pull request #3707 from lachlan-roberts/jetty-9.4.x-3683-multipart 

Issue #3683 - ensure multipart files are still cleaned up if error occurs
This commit is contained in:
Lachlan 2019-06-06 07:38:32 +10:00 committed by GitHub
parent b96b4bfffe e75ca325fd
commit 3651acc5c9
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
8 changed files with 274 additions and 116 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

66
jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartFormInputStream.java
View File

@ -35,7 +35,6 @@ import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
import java.util.List;
import javax.servlet.MultipartConfigElement;
import javax.servlet.ServletInputStream;
import javax.servlet.http.Part;
@ -299,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();
}
/**
@ -398,31 +396,21 @@ public class MultiPartFormInputStream
*/
public void deleteParts()
{
if (!_parsed)
return;
Collection<Part> parts;
try
{
parts = getParts();
}
catch (IOException e)
{
throw new RuntimeException(e);
}
MultiException err = null;
for (Part p : parts)
for (List<Part> 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();
@ -477,6 +465,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;
@ -495,7 +486,9 @@ public class MultiPartFormInputStream
if (_parsed)
return;
_parsed = true;
MultiPartParser parser = null;
Handler handler = new Handler();
try
{
// initialize
@ -531,9 +524,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;
@ -545,7 +536,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())
@ -595,6 +585,10 @@ public class MultiPartFormInputStream
catch (Throwable e)
{
_err = e;
// Notify parser if failure occurs
if (parser != null)
parser.parse(BufferUtil.EMPTY_BUFFER, true);
}
}
@ -742,6 +736,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()

15
jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartParser.java
View File

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

9
jetty-http/src/test/java/org/eclipse/jetty/http/MultiPartFormInputStreamTest.java
View File

@ -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" +

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

@ -26,7 +26,6 @@ import java.util.ArrayList;
import java.util.Collection;
import java.util.EnumSet;
import java.util.List;
import javax.servlet.MultipartConfigElement;
import javax.servlet.http.Part;
@ -44,13 +43,12 @@ import org.eclipse.jetty.util.MultiPartInputStreamParser.NonCompliance;
*/
public interface MultiParts extends Closeable
{
public Collection<Part> getParts();
public Part getPart(String name);
public boolean isEmpty();
public ContextHandler.Context getContext();
public class MultiPartsHttpParser implements MultiParts
Collection<Part> getParts() throws IOException;
Part getPart(String name) throws IOException;
boolean isEmpty();
ContextHandler.Context getContext();
class MultiPartsHttpParser implements MultiParts
{
private final MultiPartFormInputStream _httpParser;
private final ContextHandler.Context _context;
@ -59,32 +57,18 @@ public interface MultiParts extends Closeable
{
_httpParser = new MultiPartFormInputStream(in, contentType, config, contextTmpDir);
_context = request.getContext();
_httpParser.getParts();
}
@Override
public Collection<Part> getParts()
public Collection<Part> 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
@ -104,65 +88,36 @@ public interface MultiParts extends Closeable
{
return _context;
}
}
@SuppressWarnings("deprecation")
public class MultiPartsUtilParser implements MultiParts
@SuppressWarnings("deprecation")
class MultiPartsUtilParser implements MultiParts
{
private final MultiPartInputStreamParser _utilParser;
private final ContextHandler.Context _context;
private final Request _request;
public MultiPartsUtilParser(InputStream in, String contentType, MultipartConfigElement config, File contextTmpDir, Request request) throws IOException
{
_utilParser = new MultiPartInputStreamParser(in, contentType, config, contextTmpDir);
_context = request.getContext();
_utilParser.getParts();
EnumSet<NonCompliance> nonComplianceWarnings = _utilParser.getNonComplianceWarnings();
if (!nonComplianceWarnings.isEmpty())
{
@SuppressWarnings("unchecked")
List<String> violations = (List<String>)request.getAttribute(HttpCompliance.VIOLATIONS_ATTR);
if (violations==null)
{
violations = new ArrayList<>();
request.setAttribute(HttpCompliance.VIOLATIONS_ATTR,violations);
}
for(NonCompliance nc : nonComplianceWarnings)
violations.add(nc.name()+": "+nc.getURL());
System.out.println(violations);
}
_request = request;
}
@Override
public Collection<Part> getParts()
public Collection<Part> getParts() throws IOException
{
try
{
return _utilParser.getParts();
}
catch (IOException e)
{
throw new RuntimeException(e);
}
Collection<Part> parts = _utilParser.getParts();
setNonComplianceViolationsOnRequest();
return parts;
}
@Override
public Part getPart(String name)
public Part getPart(String name) throws IOException
{
try
{
return _utilParser.getPart(name);
}
catch (IOException e)
{
throw new RuntimeException(e);
}
Part part = _utilParser.getPart(name);
setNonComplianceViolationsOnRequest();
return part;
}
@Override
@ -182,5 +137,19 @@ public interface MultiParts extends Closeable
{
return _context;
}
private void setNonComplianceViolationsOnRequest()
{
@SuppressWarnings("unchecked")
List<String> violations = (List<String>)_request.getAttribute(HttpCompliance.VIOLATIONS_ATTR);
if (violations != null)
return;
EnumSet<NonCompliance> nonComplianceWarnings = _utilParser.getNonComplianceWarnings();
violations = new ArrayList<>();
for (NonCompliance nc : nonComplianceWarnings)
violations.add(nc.name() + ": " + nc.getURL());
_request.setAttribute(HttpCompliance.VIOLATIONS_ATTR, violations);
}
}
}

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

@ -487,7 +487,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);
@ -2320,7 +2320,6 @@ public class Request implements HttpServletRequest
public Part getPart(String name) throws IOException, ServletException
{
getParts();
return _multiParts.getPart(name);
}
@ -2334,7 +2333,7 @@ public class Request implements HttpServletRequest
return getParts(null);
}
private Collection<Part> getParts(MultiMap<String> params) throws IOException, ServletException
private Collection<Part> getParts(MultiMap<String> params) throws IOException
{
if (_multiParts == null)
_multiParts = (MultiParts)getAttribute(__MULTIPARTS);
@ -2345,12 +2344,9 @@ public class Request implements HttpServletRequest
if (config == null)
throw new IllegalStateException("No multipart config for servlet");
_multiParts = newMultiParts(getInputStream(),
_contentType, config,
(_context != null?(File)_context.getAttribute("javax.servlet.context.tempdir"):null));
_multiParts = newMultiParts(config);
setAttribute(__MULTIPARTS, _multiParts);
Collection<Part> parts = _multiParts.getParts(); //causes parsing
Collection<Part> parts = _multiParts.getParts();
String _charset_ = null;
Part charsetPart = _multiParts.getPart("_charset_");
@ -2412,7 +2408,7 @@ 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
{
MultiPartFormDataCompliance compliance = getHttpChannel().getHttpConfiguration().getMultipartFormDataCompliance();
if(LOG.isDebugEnabled())
@ -2421,12 +2417,12 @@ public class Request implements HttpServletRequest
switch(compliance)
{
case RFC7578:
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);
case LEGACY:
default:
return new MultiParts.MultiPartsUtilParser(getInputStream(), contentType, config,
return new MultiParts.MultiPartsUtilParser(getInputStream(), getContentType(), config,
(_context != null?(File)_context.getAttribute("javax.servlet.context.tempdir"):null), this);
}

6
jetty-servlet/pom.xml
View File

@ -60,5 +60,11 @@
<classifier>tests</classifier>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.eclipse.jetty</groupId>
<artifactId>jetty-client</artifactId>
<version>${project.version}</version>
<scope>test</scope>
</dependency>
</dependencies>
</project>

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

@ -747,11 +747,14 @@ public class ServletHolder extends Holder<Servlet> 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);
}
}

164
jetty-servlet/src/test/java/org/eclipse/jetty/servlet/MultiPartServletTest.java Normal file
View File

@ -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<Arguments> 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<byteArray.length; i++)
byteArray[i] = 1;
BytesContentProvider contentProvider = new BytesContentProvider(byteArray);
MultiPartContentProvider multiPart = new MultiPartContentProvider();
multiPart.addFieldPart("largePart", contentProvider, null);
multiPart.close();
try (StacklessLogging stacklessLogging = new StacklessLogging(HttpChannel.class, MultiPartFormInputStream.class))
{
ContentResponse response = client.newRequest("localhost", connector.getLocalPort())
.scheme(HttpScheme.HTTP.asString())
.method(HttpMethod.POST)
.content(multiPart)
.send();
assertEquals(500, response.getStatus());
assertThat(response.getContentAsString(),
containsString("Multipart Mime part largePart exceeds max filesize"));
}
assertThat(tmpDir.toFile().list().length, is(0));
}
}