Merge pull request #5481 from eclipse/jetty-9.4.x-5480-npe-webinfconfiguration

NPE protection on WebInfConfiguration.deconfigure()

jetty-io/src/test/java/org/eclipse/jetty/io/IOTest.java

@@ -20,6 +20,7 @@ package org.eclipse.jetty.io;
 
 import java.io.ByteArrayInputStream;
 import java.io.ByteArrayOutputStream;
+import java.io.File;
 import java.io.IOException;
 import java.net.InetSocketAddress;
 import java.net.ServerSocket;
@@ -41,12 +42,15 @@ import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.Future;
 import java.util.concurrent.TimeUnit;
 
+import org.eclipse.jetty.toolchain.test.FS;
+import org.eclipse.jetty.toolchain.test.MavenTestingUtils;
 import org.eclipse.jetty.toolchain.test.jupiter.WorkDir;
 import org.eclipse.jetty.toolchain.test.jupiter.WorkDirExtension;
 import org.eclipse.jetty.util.BufferUtil;
 import org.eclipse.jetty.util.IO;
 import org.junit.jupiter.api.Assertions;
 import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.TestInfo;
 import org.junit.jupiter.api.condition.OS;
 import org.junit.jupiter.api.extension.ExtendWith;
 
@@ -449,6 +453,64 @@ public class IOTest
         }
     }
 
+    @Test
+    public void testDeleteNull()
+    {
+        assertFalse(IO.delete(null));
+    }
+
+    @Test
+    public void testDeleteNonExistentFile(TestInfo testInfo)
+    {
+        File dir = MavenTestingUtils.getTargetTestingDir(testInfo.getDisplayName());
+        FS.ensureEmpty(dir);
+        File noFile = new File(dir, "nada");
+        assertFalse(IO.delete(noFile));
+    }
+
+    @Test
+    public void testIsEmptyNull()
+    {
+        assertTrue(IO.isEmptyDir(null));
+    }
+
+    @Test
+    public void testIsEmptyDoesNotExist(TestInfo testInfo)
+    {
+        File dir = MavenTestingUtils.getTargetTestingDir(testInfo.getDisplayName());
+        FS.ensureEmpty(dir);
+        File noFile = new File(dir, "nada");
+        assertTrue(IO.isEmptyDir(noFile));
+    }
+
+    @Test
+    public void testIsEmptyExistButAsFile(TestInfo testInfo) throws IOException
+    {
+        File dir = MavenTestingUtils.getTargetTestingDir(testInfo.getDisplayName());
+        FS.ensureEmpty(dir);
+        File file = new File(dir, "nada");
+        FS.touch(file);
+        assertFalse(IO.isEmptyDir(file));
+    }
+
+    @Test
+    public void testIsEmptyExistAndIsEmpty(TestInfo testInfo)
+    {
+        File dir = MavenTestingUtils.getTargetTestingDir(testInfo.getDisplayName());
+        FS.ensureEmpty(dir);
+        assertTrue(IO.isEmptyDir(dir));
+    }
+
+    @Test
+    public void testIsEmptyExistAndHasContent(TestInfo testInfo) throws IOException
+    {
+        File dir = MavenTestingUtils.getTargetTestingDir(testInfo.getDisplayName());
+        FS.ensureEmpty(dir);
+        File file = new File(dir, "nada");
+        FS.touch(file);
+        assertFalse(IO.isEmptyDir(dir));
+    }
+
     @Test
     public void testSelectorWakeup() throws Exception
     {

jetty-maven-plugin/src/main/java/org/eclipse/jetty/maven/plugin/AbstractJettyMojo.java

@@ -49,7 +49,6 @@ import org.eclipse.jetty.server.ShutdownMonitor;
 import org.eclipse.jetty.server.handler.ContextHandler;
 import org.eclipse.jetty.server.handler.ContextHandlerCollection;
 import org.eclipse.jetty.server.handler.HandlerCollection;
-import org.eclipse.jetty.util.PathWatcher;
 import org.eclipse.jetty.util.Scanner;
 import org.eclipse.jetty.util.StringUtil;
 import org.eclipse.jetty.util.resource.Resource;
@@ -559,7 +558,12 @@ public abstract class AbstractJettyMojo extends AbstractMojo
             File target = new File(project.getBuild().getDirectory());
             File tmp = new File(target, "tmp");
             if (!tmp.exists())
-                tmp.mkdirs();
+            {
+                if (!tmp.mkdirs())
+                {
+                    throw new MojoFailureException("Unable to create temp directory: " + tmp);
+                }
+            }
             webApp.setTempDirectory(tmp);
         }
 

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

@@ -361,10 +361,13 @@ public class IO
      * This delete will recursively delete directories - BE CAREFUL
      *
      * @param file The file (or directory) to be deleted.
-     * @return true if anything was deleted. (note: this does not mean that all content in a directory was deleted)
+     * @return true if file was deleted, or directory referenced was deleted.
+     * false if file doesn't exist, or was null.
      */
     public static boolean delete(File file)
     {
+        if (file == null)
+            return false;
         if (!file.exists())
             return false;
         if (file.isDirectory())
@@ -378,6 +381,27 @@ public class IO
         return file.delete();
     }
 
+    /**
+     * Test if directory is empty.
+     *
+     * @param dir the directory
+     * @return true if directory is null, doesn't exist, or has no content.
+     * false if not a directory, or has contents
+     */
+    public static boolean isEmptyDir(File dir)
+    {
+        if (dir == null)
+            return true;
+        if (!dir.exists())
+            return true;
+        if (!dir.isDirectory())
+            return false;
+        String[] list = dir.list();
+        if (list == null)
+            return true;
+        return list.length <= 0;
+    }
+
     /**
      * Closes an arbitrary closable, and logs exceptions at ignore level
      *

jetty-webapp/src/main/java/org/eclipse/jetty/webapp/WebInfConfiguration.java

@@ -354,10 +354,12 @@ public class WebInfConfiguration extends AbstractConfiguration
     @Override
     public void deconfigure(WebAppContext context) throws Exception
     {
-        //if we're not persisting the temp dir contents delete it
+        File tempDirectory = context.getTempDirectory();
-        if (!context.isPersistTempDirectory())
+
+        // if we're not persisting the temp dir contents delete it
+        if (!context.isPersistTempDirectory() && !IO.isEmptyDir(tempDirectory))
         {
-            IO.delete(context.getTempDirectory());
+            IO.delete(tempDirectory);
         }
 
         //if it wasn't explicitly configured by the user, then unset it
@@ -504,13 +506,15 @@ public class WebInfConfiguration extends AbstractConfiguration
             //if it is to be persisted, make sure it will be the same name
             //by not using File.createTempFile, which appends random digits
             tmpDir = new File(parent, temp);
+            configureTempDirectory(tmpDir, context);
         }
         else
         {
-            //ensure file will always be unique by appending random digits
+            // ensure dir will always be unique by having classlib generate random path name
             tmpDir = Files.createTempDirectory(parent.toPath(), temp).toFile();
+            tmpDir.deleteOnExit();
+            ensureTempDirUsable(tmpDir);
         }
-        configureTempDirectory(tmpDir, context);
 
         if (LOG.isDebugEnabled())
             LOG.debug("Set temp dir " + tmpDir);
@@ -522,21 +526,30 @@ public class WebInfConfiguration extends AbstractConfiguration
         if (dir == null)
             throw new IllegalArgumentException("Null temp dir");
 
-        //if dir exists and we don't want it persisted, delete it
+        // if dir exists and we don't want it persisted, delete it
-        if (dir.exists() && !context.isPersistTempDirectory())
+        if (!context.isPersistTempDirectory() && dir.exists() && !IO.delete(dir))
         {
-            if (!IO.delete(dir))
+            throw new IllegalStateException("Failed to delete temp dir " + dir);
-                throw new IllegalStateException("Failed to delete temp dir " + dir);
         }
 
-        //if it doesn't exist make it
+        // if it doesn't exist make it
         if (!dir.exists())
-            dir.mkdirs();
+        {
+            if (!dir.mkdirs())
+            {
+                throw new IllegalStateException("Unable to create temp dir " + dir);
+            }
+        }
 
         if (!context.isPersistTempDirectory())
             dir.deleteOnExit();
 
-        //is it useable
+        ensureTempDirUsable(dir);
+    }
+
+    private void ensureTempDirUsable(File dir)
+    {
+        // is it useable
         if (!dir.canWrite() || !dir.isDirectory())
             throw new IllegalStateException("Temp dir " + dir + " not useable: writeable=" + dir.canWrite() + ", dir=" + dir.isDirectory());
     }