391188 Files written with Request.getPart().write(filename) should not be auto-deleted

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
+                ContextHandler.Context context = (ContextHandler.Context)sre.getServletRequest().getAttribute(__MULTIPART_CONTEXT);
-              {
+
-                  mpis.deleteParts();
+                //Only do the cleanup if we are exiting from the context in which a servlet parsed the multipart files
-              }
+                if (context == sre.getServletContext())
-              catch (MultiException e)
+                {
-              {
+                    try
-                  sre.getServletContext().log("Errors deleting multipart tmp files", e);
+                    {
-              }
+                        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<Part> 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<Part> 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<Part> parts = _multiPartInputStream.getParts(); //causes parsing 
             for (Part p:parts)
             {

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);
+                    return false;
-                    String value = request.getParameter("stuff");
+                }
-                    byte[] expected = "000000000000000000000000000000000000000000000000000".getBytes("ISO-8859-1");
+                catch (IllegalStateException e)
-                    return value.equals(new String(expected, "ISO-8859-1"));
+                {
+                    //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);
+            }
+        }
+    }
 }

jetty-util/src/main/java/org/eclipse/jetty/util/MultiPartInputStream.java

@@ -74,6 +74,7 @@ public class MultiPartInputStream
         protected String _contentType;
         protected MultiMap<String> _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<Part> 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<Part> 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
     {

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<Part> 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<Part> 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<Part> 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();
+        File tmpfile = ((MultiPartInputStream.MultiPart)stuff).getFile();
-        assertThat(f,notNullValue()); // longer than 100 bytes, should already be a file
+        assertThat(tmpfile,notNullValue()); // longer than 100 bytes, should already be a tmp file
-        assertThat(((MultiPartInputStream.MultiPart)stuff).getBytes(),nullValue()); //not in internal buffer any more
+        assertThat(((MultiPartInputStream.MultiPart)stuff).getBytes(),nullValue()); //not in an internal buffer
-        assertThat(f.exists(),is(true));
+        assertThat(tmpfile.exists(),is(true));
-        assertThat(f.getName(),is(not("stuff with space.txt")));
+        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 ()