From 25cdef966ce4082856b3c4e0d65120f4ccac3242 Mon Sep 17 00:00:00 2001 From: Jan Bartel Date: Thu, 4 Oct 2012 15:24:23 +1000 Subject: [PATCH] 391080 Multipart temp files can be left on disk with MultiPartFilter --- .../org/eclipse/jetty/server/Request.java | 18 +++++ .../jetty/servlets/MultiPartFilter.java | 33 +++----- .../jetty/servlets/MultipartFilterTest.java | 2 +- .../jetty/util/MultiPartInputStream.java | 39 ++++++++- .../jetty/util/MultiPartInputStreamTest.java | 80 +++++++++++++++---- 5 files changed, 131 insertions(+), 41 deletions(-) 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 47b5d3acd41..db32fdc258a 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 @@ -1433,6 +1433,24 @@ public class Request implements HttpServletRequest if (_savedNewSessions != null) _savedNewSessions.clear(); _savedNewSessions=null; + if (_multiPartInputStream != null) + { + Collection parts = _multiPartInputStream.getParsedParts(); + if (parts != null) + { + for (Part p:parts) + { + try + { + p.delete(); + } + catch (IOException e) + { + LOG.warn("Error deleting multipart file", e); + } + } + } + } _multiPartInputStream = null; } diff --git a/jetty-servlets/src/main/java/org/eclipse/jetty/servlets/MultiPartFilter.java b/jetty-servlets/src/main/java/org/eclipse/jetty/servlets/MultiPartFilter.java index c6f34a16ebe..15c1faaa17b 100644 --- a/jetty-servlets/src/main/java/org/eclipse/jetty/servlets/MultiPartFilter.java +++ b/jetty-servlets/src/main/java/org/eclipse/jetty/servlets/MultiPartFilter.java @@ -84,7 +84,7 @@ import org.eclipse.jetty.util.TypeUtil; public class MultiPartFilter implements Filter { public final static String CONTENT_TYPE_SUFFIX=".org.eclipse.jetty.servlet.contentType"; - private final static String FILES ="org.eclipse.jetty.servlet.MultiPartFilter.files"; + private final static String MULTIPART = "org.eclipse.jetty.servlet.MultiPartInputStream"; private File tempdir; private boolean _deleteFiles; private ServletContext _context; @@ -149,7 +149,8 @@ public class MultiPartFilter implements Filter MultipartConfigElement config = new MultipartConfigElement(tempdir.getCanonicalPath(), _maxFileSize, _maxRequestSize, _fileOutputBuffer); MultiPartInputStream mpis = new MultiPartInputStream(in, content_type, config, tempdir); - + mpis.setDeleteOnExit(_deleteFiles); + request.setAttribute(MULTIPART, mpis); try { @@ -170,18 +171,6 @@ public class MultiPartFilter implements Filter if (mp.getContentType() != null) params.add(mp.getName()+CONTENT_TYPE_SUFFIX, mp.getContentType()); } - if (_deleteFiles) - { - mp.getFile().deleteOnExit(); - - ArrayList files = (ArrayList)request.getAttribute(FILES); - if (files==null) - { - files=new ArrayList(); - request.setAttribute(FILES,files); - } - files.add(mp.getFile()); - } } else { @@ -205,23 +194,23 @@ public class MultiPartFilter implements Filter private void deleteFiles(ServletRequest request) { - ArrayList files = (ArrayList)request.getAttribute(FILES); - if (files!=null) + MultiPartInputStream mpis = (MultiPartInputStream)request.getAttribute(MULTIPART); + if (mpis != null) { - Iterator iter = files.iterator(); - while (iter.hasNext()) + Collection parts = mpis.getParsedParts(); + for (Part p:parts) { - File file=(File)iter.next(); try { - file.delete(); - } + p.delete(); + } catch(Exception e) { - _context.log("failed to delete "+file,e); + _context.log("Failed to delete "+p.getName(),e); } } } + request.removeAttribute(MULTIPART); } /* ------------------------------------------------------------ */ diff --git a/jetty-servlets/src/test/java/org/eclipse/jetty/servlets/MultipartFilterTest.java b/jetty-servlets/src/test/java/org/eclipse/jetty/servlets/MultipartFilterTest.java index 11bab2c3963..38e2c8bbdca 100644 --- a/jetty-servlets/src/test/java/org/eclipse/jetty/servlets/MultipartFilterTest.java +++ b/jetty-servlets/src/test/java/org/eclipse/jetty/servlets/MultipartFilterTest.java @@ -121,7 +121,7 @@ public class MultipartFilterTest response.parse(tester.getResponses(request.generate())); assertTrue(response.getMethod()==null); - assertEquals(HttpServletResponse.SC_OK,response.getStatus()); + assertEquals(HttpServletResponse.SC_INTERNAL_SERVER_ERROR,response.getStatus()); } diff --git a/jetty-util/src/main/java/org/eclipse/jetty/util/MultiPartInputStream.java b/jetty-util/src/main/java/org/eclipse/jetty/util/MultiPartInputStream.java index 1807bdde7b5..48069f38115 100644 --- a/jetty-util/src/main/java/org/eclipse/jetty/util/MultiPartInputStream.java +++ b/jetty-util/src/main/java/org/eclipse/jetty/util/MultiPartInputStream.java @@ -34,6 +34,7 @@ import java.io.InputStreamReader; import java.io.OutputStream; import java.util.ArrayList; import java.util.Collection; +import java.util.Collections; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -59,7 +60,7 @@ public class MultiPartInputStream protected MultiMap _parts; protected File _tmpDir; protected File _contextTmpDir; - + protected boolean _deleteOnExit; @@ -141,6 +142,9 @@ public class MultiPartInputStream throws IOException { _file = File.createTempFile("MultiPart", "", MultiPartInputStream.this._tmpDir); + if (_deleteOnExit) + _file.deleteOnExit(); + FileOutputStream fos = new FileOutputStream(_file); BufferedOutputStream bos = new BufferedOutputStream(fos); @@ -244,6 +248,8 @@ public class MultiPartInputStream { //part data is only in the ByteArrayOutputStream and never been written to disk _file = new File (_tmpDir, fileName); + if (_deleteOnExit) + _file.deleteOnExit(); BufferedOutputStream bos = null; try { @@ -262,6 +268,8 @@ public class MultiPartInputStream { //the part data is already written to a temporary file, just rename it File f = new File(_tmpDir, fileName); + if (_deleteOnExit) + f.deleteOnExit(); if (_file.renameTo(f)) _file = f; } @@ -318,7 +326,21 @@ public class MultiPartInputStream _config = new MultipartConfigElement(_contextTmpDir.getAbsolutePath()); } - + public Collection getParsedParts() + { + if (_parts == null) + return Collections.emptyList(); + + Collection values = _parts.values(); + List parts = new ArrayList(); + for (Object o: values) + { + List asList = LazyList.getList(o, false); + parts.addAll(asList); + } + return parts; + } + public Collection getParts() throws IOException, ServletException @@ -593,7 +615,18 @@ public class MultiPartInputStream throw new IOException("Incomplete parts"); } - + public void setDeleteOnExit(boolean deleteOnExit) + { + _deleteOnExit = deleteOnExit; + } + + + public boolean isDeleteOnExit() + { + return _deleteOnExit; + } + + /* ------------------------------------------------------------ */ private String value(String nameEqualsValue, boolean splitAfterSpace) { diff --git a/jetty-util/src/test/java/org/eclipse/jetty/util/MultiPartInputStreamTest.java b/jetty-util/src/test/java/org/eclipse/jetty/util/MultiPartInputStreamTest.java index ec8a95b02d4..0407ccae964 100644 --- a/jetty-util/src/test/java/org/eclipse/jetty/util/MultiPartInputStreamTest.java +++ b/jetty-util/src/test/java/org/eclipse/jetty/util/MultiPartInputStreamTest.java @@ -50,8 +50,42 @@ public class MultiPartInputStreamTest extends TestCase protected String _contentType = "multipart/form-data, boundary=AaB03x"; protected String _multi = createMultipartRequestString(FILENAME); protected String _dirname = System.getProperty("java.io.tmpdir")+File.separator+"myfiles-"+System.currentTimeMillis(); + protected File _tmpDir = new File(_dirname); + public MultiPartInputStreamTest () + { + _tmpDir.deleteOnExit(); + } + public void testBadMultiPartRequest() + throws Exception + { + String boundary = "X0Y0"; + String str = "--" + boundary + "\r\n"+ + "Content-Disposition: form-data; name=\"fileup\"; filename=\"test.upload\"\r\n"+ + "Content-Type: application/octet-stream\r\n\r\n"+ + "How now brown cow."+ + "\r\n--" + boundary + "-\r\n\r\n"; + + MultipartConfigElement config = new MultipartConfigElement(_dirname, 1024, 3072, 50); + MultiPartInputStream mpis = new MultiPartInputStream(new ByteArrayInputStream(str.getBytes()), + "multipart/form-data, boundary="+boundary, + config, + _tmpDir); + mpis.setDeleteOnExit(true); + try + { + mpis.getParts(); + fail ("Multipart incomplete"); + } + catch (IOException e) + { + assertTrue(e.getMessage().startsWith("Incomplete")); + } + } + + + public void testNonMultiPartRequest() throws Exception { @@ -59,7 +93,8 @@ public class MultiPartInputStreamTest extends TestCase MultiPartInputStream mpis = new MultiPartInputStream(new ByteArrayInputStream(_multi.getBytes()), "Content-type: text/plain", config, - new File(_dirname)); + _tmpDir); + mpis.setDeleteOnExit(true); assertTrue(mpis.getParts().isEmpty()); } @@ -70,7 +105,8 @@ public class MultiPartInputStreamTest extends TestCase MultiPartInputStream mpis = new MultiPartInputStream(new ByteArrayInputStream(_multi.getBytes()), _contentType, config, - new File(_dirname)); + _tmpDir); + mpis.setDeleteOnExit(true); Collection parts = mpis.getParts(); assertFalse(parts.isEmpty()); } @@ -82,11 +118,12 @@ public class MultiPartInputStreamTest extends TestCase MultiPartInputStream mpis = new MultiPartInputStream(new ByteArrayInputStream(_multi.getBytes()), _contentType, config, - new File(_dirname)); - + _tmpDir); + mpis.setDeleteOnExit(true); + Collection parts = null; try { - mpis.getParts(); + parts = mpis.getParts(); fail("Request should have exceeded maxRequestSize"); } catch (IllegalStateException e) @@ -102,11 +139,12 @@ public class MultiPartInputStreamTest extends TestCase MultiPartInputStream mpis = new MultiPartInputStream(new ByteArrayInputStream(_multi.getBytes()), _contentType, config, - new File(_dirname)); - + _tmpDir); + mpis.setDeleteOnExit(true); + Collection parts = null; try { - mpis.getParts(); + parts = mpis.getParts(); fail("stuff.txt should have been larger than maxFileSize"); } catch (IllegalStateException e) @@ -133,8 +171,8 @@ public class MultiPartInputStreamTest extends TestCase MultiPartInputStream mpis = new MultiPartInputStream(new ByteArrayInputStream(createMultipartRequestString(filename).getBytes()), _contentType, config, - new File(_dirname)); - + _tmpDir); + mpis.setDeleteOnExit(true); Collection parts = mpis.getParts(); assertThat(parts.size(), is(2)); Part field1 = mpis.getPart("field1"); @@ -188,15 +226,15 @@ public class MultiPartInputStreamTest extends TestCase "content-disposition: form-data; name=\"stuff\"; filename=\"stuff2.txt\"\r\n"+ "Content-Type: text/plain\r\n"+ "\r\n"+ - "000000000000000000000000000000000000000000000000000\r\n"+ + "110000000000000000000000000000000000000000000000000\r\n"+ "--AaB03x--\r\n"; MultipartConfigElement config = new MultipartConfigElement(_dirname, 1024, 3072, 50); MultiPartInputStream mpis = new MultiPartInputStream(new ByteArrayInputStream(sameNames.getBytes()), _contentType, config, - new File(_dirname)); - + _tmpDir); + mpis.setDeleteOnExit(true); Collection parts = mpis.getParts(); assertEquals(2, parts.size()); for (Part p:parts) @@ -210,6 +248,18 @@ public class MultiPartInputStreamTest extends TestCase private String createMultipartRequestString(String filename) { + int length = filename.length(); + String name = filename; + if (length > 10) + name = filename.substring(0,10); + StringBuffer filler = new StringBuffer(); + int i = name.length(); + while (i < 51) + { + filler.append("0"); + i++; + } + return "--AaB03x\r\n"+ "content-disposition: form-data; name=\"field1\"\r\n"+ "\r\n"+ @@ -217,8 +267,8 @@ public class MultiPartInputStreamTest extends TestCase "--AaB03x\r\n"+ "content-disposition: form-data; name=\"stuff\"; filename=\"" + filename + "\"\r\n"+ "Content-Type: text/plain\r\n"+ - "\r\n"+ - "000000000000000000000000000000000000000000000000000\r\n"+ + "\r\n"+name+ + filler.toString()+"\r\n" + "--AaB03x--\r\n"; } }