From 31807b7892ea4a37e4dcdb2e101b7c6a89a0a2e8 Mon Sep 17 00:00:00 2001 From: Jan Bartel Date: Mon, 8 Oct 2012 11:09:41 +1100 Subject: [PATCH] 391188 Files written with Request.getPart().write(filename) should not be auto-deleted --- .../org/eclipse/jetty/server/Request.java | 59 ++++----- .../org/eclipse/jetty/server/RequestTest.java | 116 +++++++++++++++++- .../jetty/util/MultiPartInputStream.java | 64 ++++++++-- .../jetty/util/MultiPartInputStreamTest.java | 74 +++++++++-- 4 files changed, 260 insertions(+), 53 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 ef57580220f..d0ca3771e06 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 @@ -127,7 +127,7 @@ public class Request implements HttpServletRequest { public static final String __MULTIPART_CONFIG_ELEMENT = "org.eclipse.multipartConfig"; public static final String __MULTIPART_INPUT_STREAM = "org.eclipse.multiPartInputStream"; - + public static final String __MULTIPART_CONTEXT = "org.eclipse.multiPartContext"; private static final Logger LOG = Log.getLogger(Request.class); private static final String __ASYNC_FWD = "org.eclipse.asyncfwd"; @@ -144,14 +144,20 @@ public class Request implements HttpServletRequest MultiPartInputStream mpis = (MultiPartInputStream)sre.getServletRequest().getAttribute(__MULTIPART_INPUT_STREAM); if (mpis != null) { - try - { - mpis.deleteParts(); - } - catch (MultiException e) - { - sre.getServletContext().log("Errors deleting multipart tmp files", e); - } + ContextHandler.Context context = (ContextHandler.Context)sre.getServletRequest().getAttribute(__MULTIPART_CONTEXT); + + //Only do the cleanup if we are exiting from the context in which a servlet parsed the multipart files + if (context == sre.getServletContext()) + { + try + { + mpis.deleteParts(); + } + catch (MultiException e) + { + sre.getServletContext().log("Errors deleting multipart tmp files", e); + } + } } } @@ -1468,24 +1474,6 @@ 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; } @@ -2007,10 +1995,16 @@ public class Request implements HttpServletRequest if (_multiPartInputStream == null) { + MultipartConfigElement config = (MultipartConfigElement)getAttribute(__MULTIPART_CONFIG_ELEMENT); + + if (config == null) + throw new IllegalStateException("No multipart config for servlet"); + _multiPartInputStream = new MultiPartInputStream(getInputStream(), - getContentType(),(MultipartConfigElement)getAttribute(__MULTIPART_CONFIG_ELEMENT), + getContentType(),config, (_context != null?(File)_context.getAttribute("javax.servlet.context.tempdir"):null)); setAttribute(__MULTIPART_INPUT_STREAM, _multiPartInputStream); + setAttribute(__MULTIPART_CONTEXT, _context); Collection parts = _multiPartInputStream.getParts(); //causes parsing for (Part p:parts) { @@ -2039,10 +2033,17 @@ public class Request implements HttpServletRequest if (_multiPartInputStream == null) { + MultipartConfigElement config = (MultipartConfigElement)getAttribute(__MULTIPART_CONFIG_ELEMENT); + + if (config == null) + throw new IllegalStateException("No multipart config for servlet"); + _multiPartInputStream = new MultiPartInputStream(getInputStream(), - getContentType(),(MultipartConfigElement)getAttribute(__MULTIPART_CONFIG_ELEMENT), + getContentType(), config, (_context != null?(File)_context.getAttribute("javax.servlet.context.tempdir"):null)); + setAttribute(__MULTIPART_INPUT_STREAM, _multiPartInputStream); + setAttribute(__MULTIPART_CONTEXT, _context); Collection parts = _multiPartInputStream.getParts(); //causes parsing for (Part p:parts) { 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 355264fab66..408feefa628 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 @@ -37,7 +37,9 @@ import java.util.Enumeration; import java.util.HashMap; import java.util.Map; +import javax.servlet.MultipartConfigElement; import javax.servlet.ServletException; +import javax.servlet.ServletRequestEvent; import javax.servlet.http.Cookie; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; @@ -49,6 +51,7 @@ import org.eclipse.jetty.http.MimeTypes; import org.eclipse.jetty.server.handler.AbstractHandler; import org.eclipse.jetty.server.handler.ContextHandler; import org.eclipse.jetty.util.IO; +import org.eclipse.jetty.util.MultiPartInputStream; import org.eclipse.jetty.util.StringUtil; import org.eclipse.jetty.util.log.Log; import org.junit.After; @@ -131,7 +134,7 @@ public class RequestTest } @Test - public void testMultiPart() throws Exception + public void testMultiPartNoConfig() throws Exception { _handler._checker = new RequestTester() { @@ -140,14 +143,16 @@ public class RequestTest try { Part foo = request.getPart("stuff"); - assertNotNull(foo); - String value = request.getParameter("stuff"); - byte[] expected = "000000000000000000000000000000000000000000000000000".getBytes("ISO-8859-1"); - return value.equals(new String(expected, "ISO-8859-1")); + return false; + } + catch (IllegalStateException e) + { + //expected exception because no multipart config is set up + assertTrue(e.getMessage().startsWith("No multipart config")); + return true; } catch (Exception e) { - e.printStackTrace(); return false; } } @@ -174,6 +179,66 @@ public class RequestTest String responses=_connector.getResponses(request); assertTrue(responses.startsWith("HTTP/1.1 200")); } + + + @Test + public void testMultiPart() throws Exception + { + final File tmpDir = new File (System.getProperty("java.io.tmpdir")); + final File testTmpDir = new File (tmpDir, "reqtest"); + testTmpDir.deleteOnExit(); + assertTrue(testTmpDir.mkdirs()); + assertTrue(testTmpDir.list().length == 0); + + ContextHandler contextHandler = new ContextHandler(); + contextHandler.setContextPath("/foo"); + contextHandler.setResourceBase("."); + contextHandler.setHandler(new MultiPartRequestHandler(testTmpDir)); + contextHandler.addEventListener(new Request.MultiPartCleanerListener() + { + + @Override + public void requestDestroyed(ServletRequestEvent sre) + { + MultiPartInputStream m = (MultiPartInputStream)sre.getServletRequest().getAttribute(Request.__MULTIPART_INPUT_STREAM); + ContextHandler.Context c = (ContextHandler.Context)sre.getServletRequest().getAttribute(Request.__MULTIPART_CONTEXT); + assertNotNull (m); + assertNotNull (c); + assertTrue(c == sre.getServletContext()); + assertTrue(!m.getParsedParts().isEmpty()); + assertTrue(testTmpDir.list().length == 2); + super.requestDestroyed(sre); + String[] files = testTmpDir.list(); + assertTrue(files.length == 0); + } + + }); + _server.stop(); + _server.setHandler(contextHandler); + _server.start(); + + String multipart = "--AaB03x\r\n"+ + "content-disposition: form-data; name=\"field1\"\r\n"+ + "\r\n"+ + "Joe Blow\r\n"+ + "--AaB03x\r\n"+ + "content-disposition: form-data; name=\"stuff\"; filename=\"foo.upload\"\r\n"+ + "Content-Type: text/plain;charset=ISO-8859-1\r\n"+ + "\r\n"+ + "000000000000000000000000000000000000000000000000000\r\n"+ + "--AaB03x--\r\n"; + + String request="GET /foo/x.html 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"+ + "\r\n"+ + multipart; + + String responses=_connector.getResponses(request); + System.err.println(responses); + assertTrue(responses.startsWith("HTTP/1.1 200")); + } @Test public void testBadUtf8ParamExtraction() throws Exception @@ -912,4 +977,43 @@ public class RequestTest } } + + private class MultiPartRequestHandler extends AbstractHandler + { + File tmpDir; + + public MultiPartRequestHandler(File tmpDir) + { + this.tmpDir = tmpDir; + } + + + @Override + public void handle(String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException + { + ((Request)request).setHandled(true); + try + { + + MultipartConfigElement mpce = new MultipartConfigElement(tmpDir.getAbsolutePath(),-1, -1, 2); + request.setAttribute(Request.__MULTIPART_CONFIG_ELEMENT, mpce); + + Part foo = request.getPart("stuff"); + assertNotNull(foo); + assertTrue(foo.getSize() > 0); + + response.setStatus(200); + } + catch (IllegalStateException e) + { + //expected exception because no multipart config is set up + assertTrue(e.getMessage().startsWith("No multipart config")); + response.setStatus(200); + } + catch (Exception e) + { + response.sendError(500); + } + } + } } 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 a201a2d10bd..4077ed28528 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 @@ -74,6 +74,7 @@ public class MultiPartInputStream protected String _contentType; protected MultiMap _headers; protected long _size = 0; + protected boolean _temporary = true; public MultiPart (String name, String filename) throws IOException @@ -144,7 +145,6 @@ public class MultiPartInputStream _file = File.createTempFile("MultiPart", "", MultiPartInputStream.this._tmpDir); if (_deleteOnExit) _file.deleteOnExit(); - FileOutputStream fos = new FileOutputStream(_file); BufferedOutputStream bos = new BufferedOutputStream(fos); @@ -207,11 +207,12 @@ public class MultiPartInputStream { if (_file != null) { + //written to a file, whether temporary or not return new BufferedInputStream (new FileInputStream(_file)); } else { - //part content is in a ByteArrayOutputStream + //part content is in memory return new ByteArrayInputStream(_bout.getBuf(),0,_bout.size()); } } @@ -246,10 +247,11 @@ public class MultiPartInputStream { if (_file == null) { + _temporary = false; + //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 { @@ -267,23 +269,36 @@ public class MultiPartInputStream else { //the part data is already written to a temporary file, just rename it + _temporary = false; + File f = new File(_tmpDir, fileName); - if (_deleteOnExit) - f.deleteOnExit(); if (_file.renameTo(f)) _file = f; } } /** + * Remove the file, whether or not Part.write() was called on it + * (ie no longer temporary) * @see javax.servlet.http.Part#delete() */ public void delete() throws IOException { - if (_file != null) + if (_file != null && _file.exists()) _file.delete(); } + /** + * Only remove tmp files. + * + * @throws IOException + */ + public void cleanUp() throws IOException + { + if (_temporary && _file != null && _file.exists()) + _file.delete(); + } + /** * Get the file, if any, the data has been written to. @@ -327,6 +342,11 @@ public class MultiPartInputStream _config = new MultipartConfigElement(_contextTmpDir.getAbsolutePath()); } + /** + * Get the already parsed parts. + * + * @return + */ public Collection getParsedParts() { if (_parts == null) @@ -342,6 +362,11 @@ public class MultiPartInputStream return parts; } + /** + * Delete any tmp storage for parts, and clear out the parts list. + * + * @throws MultiException + */ public void deleteParts () throws MultiException { @@ -351,17 +376,26 @@ public class MultiPartInputStream { try { - p.delete(); + ((MultiPartInputStream.MultiPart)p).cleanUp(); } catch(Exception e) { err.add(e); } } + _parts.clear(); + err.ifExceptionThrowMulti(); } + /** + * Parse, if necessary, the multipart data and return the list of Parts. + * + * @return + * @throws IOException + * @throws ServletException + */ public Collection getParts() throws IOException, ServletException { @@ -377,6 +411,14 @@ public class MultiPartInputStream } + /** + * Get the named Part. + * + * @param name + * @return + * @throws IOException + * @throws ServletException + */ public Part getPart(String name) throws IOException, ServletException { @@ -385,6 +427,12 @@ public class MultiPartInputStream } + /** + * Parse, if necessary, the multipart stream. + * + * @throws IOException + * @throws ServletException + */ protected void parse () throws IOException, ServletException { 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 0407ccae964..fffda46b325 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 @@ -27,6 +27,7 @@ import static org.junit.Assert.assertThat; import java.io.ByteArrayInputStream; import java.io.ByteArrayOutputStream; import java.io.File; +import java.io.FileInputStream; import java.io.IOException; import java.io.InputStream; import java.util.Collection; @@ -153,6 +154,48 @@ public class MultiPartInputStreamTest extends TestCase } } + public void testPartFileNotDeleted () throws Exception + { + MultipartConfigElement config = new MultipartConfigElement(_dirname, 1024, 3072, 50); + MultiPartInputStream mpis = new MultiPartInputStream(new ByteArrayInputStream(createMultipartRequestString("tptfd").getBytes()), + _contentType, + config, + _tmpDir); + mpis.setDeleteOnExit(true); + Collection parts = mpis.getParts(); + + MultiPart part = (MultiPart)mpis.getPart("stuff"); + File stuff = ((MultiPartInputStream.MultiPart)part).getFile(); + assertThat(stuff,notNullValue()); // longer than 100 bytes, should already be a tmp file + part.write("tptfd.txt"); + File tptfd = new File (_dirname+File.separator+"tptfd.txt"); + assertThat(tptfd.exists(), is(true)); + assertThat(stuff.exists(), is(false)); //got renamed + part.cleanUp(); + assertThat(tptfd.exists(), is(true)); //explicitly written file did not get removed after cleanup + tptfd.deleteOnExit(); //clean up test + } + + + public void testPartTmpFileDeletion () throws Exception + { + MultipartConfigElement config = new MultipartConfigElement(_dirname, 1024, 3072, 50); + MultiPartInputStream mpis = new MultiPartInputStream(new ByteArrayInputStream(createMultipartRequestString("tptfd").getBytes()), + _contentType, + config, + _tmpDir); + mpis.setDeleteOnExit(true); + Collection parts = mpis.getParts(); + + MultiPart part = (MultiPart)mpis.getPart("stuff"); + File stuff = ((MultiPartInputStream.MultiPart)part).getFile(); + assertThat(stuff,notNullValue()); // longer than 100 bytes, should already be a tmp file + assertThat (stuff.exists(), is(true)); + part.cleanUp(); + assertThat(stuff.exists(), is(false)); //tmp file was removed after cleanup + } + + public void testMulti () throws Exception @@ -175,7 +218,7 @@ public class MultiPartInputStreamTest extends TestCase mpis.setDeleteOnExit(true); Collection parts = mpis.getParts(); assertThat(parts.size(), is(2)); - Part field1 = mpis.getPart("field1"); + Part field1 = mpis.getPart("field1"); //field 1 too small to go into tmp file, should be in internal buffer assertThat(field1,notNullValue()); assertThat(field1.getName(),is("field1")); InputStream is = field1.getInputStream(); @@ -184,17 +227,18 @@ public class MultiPartInputStreamTest extends TestCase assertEquals("Joe Blow", new String(os.toByteArray())); assertEquals(8, field1.getSize()); - assertNotNull(((MultiPartInputStream.MultiPart)field1).getBytes()); //in internal buffer + assertNotNull(((MultiPartInputStream.MultiPart)field1).getBytes());//in internal buffer field1.write("field1.txt"); - assertNull(((MultiPartInputStream.MultiPart)field1).getBytes()); //no longer in internal buffer + assertNull(((MultiPartInputStream.MultiPart)field1).getBytes());//no longer in internal buffer File f = new File (_dirname+File.separator+"field1.txt"); assertTrue(f.exists()); - field1.write("another_field1.txt"); + field1.write("another_field1.txt"); //write after having already written File f2 = new File(_dirname+File.separator+"another_field1.txt"); assertTrue(f2.exists()); assertFalse(f.exists()); //should have been renamed field1.delete(); //file should be deleted - assertFalse(f2.exists()); + assertFalse(f.exists()); //original file was renamed + assertFalse(f2.exists()); //2nd written file was explicitly deleted MultiPart stuff = (MultiPart)mpis.getPart("stuff"); assertThat(stuff.getContentDispositionFilename(), is(filename)); @@ -204,14 +248,24 @@ public class MultiPartInputStreamTest extends TestCase assertThat(stuff.getHeader("content-disposition"),is("form-data; name=\"stuff\"; filename=\"" + filename + "\"")); assertThat(stuff.getHeaderNames().size(),is(2)); assertThat(stuff.getSize(),is(51L)); - f = ((MultiPartInputStream.MultiPart)stuff).getFile(); - assertThat(f,notNullValue()); // longer than 100 bytes, should already be a file - assertThat(((MultiPartInputStream.MultiPart)stuff).getBytes(),nullValue()); //not in internal buffer any more - assertThat(f.exists(),is(true)); - assertThat(f.getName(),is(not("stuff with space.txt"))); + File tmpfile = ((MultiPartInputStream.MultiPart)stuff).getFile(); + assertThat(tmpfile,notNullValue()); // longer than 100 bytes, should already be a tmp file + assertThat(((MultiPartInputStream.MultiPart)stuff).getBytes(),nullValue()); //not in an internal buffer + assertThat(tmpfile.exists(),is(true)); + assertThat(tmpfile.getName(),is(not("stuff with space.txt"))); stuff.write(filename); f = new File(_dirname+File.separator+filename); assertThat(f.exists(),is(true)); + assertThat(tmpfile.exists(), is(false)); + try + { + stuff.getInputStream(); + } + catch (Exception e) + { + fail("Part.getInputStream() after file rename operation"); + } + f.deleteOnExit(); //clean up after test } public void testMultiSameNames ()