From 0e98c370fd00492dc6973e017b9b7cc3315280be Mon Sep 17 00:00:00 2001 From: Joakim Erdfelt Date: Tue, 20 Oct 2020 16:28:36 -0500 Subject: [PATCH 1/5] Issue #5480 - NPE protection on IO.delete(File) Signed-off-by: Joakim Erdfelt --- jetty-util/src/main/java/org/eclipse/jetty/util/IO.java | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/jetty-util/src/main/java/org/eclipse/jetty/util/IO.java b/jetty-util/src/main/java/org/eclipse/jetty/util/IO.java index 0ccd1ec969d..7937481be62 100644 --- a/jetty-util/src/main/java/org/eclipse/jetty/util/IO.java +++ b/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()) From 227dd47157c6839d137b49a14e6085f42787f444 Mon Sep 17 00:00:00 2001 From: Joakim Erdfelt Date: Tue, 20 Oct 2020 16:29:22 -0500 Subject: [PATCH 2/5] Issue #5480 - Fail in jetty-maven-plugin if unable to create temp directory. Signed-off-by: Joakim Erdfelt --- .../org/eclipse/jetty/maven/plugin/AbstractJettyMojo.java | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/jetty-maven-plugin/src/main/java/org/eclipse/jetty/maven/plugin/AbstractJettyMojo.java b/jetty-maven-plugin/src/main/java/org/eclipse/jetty/maven/plugin/AbstractJettyMojo.java index fd84f89d100..c4639d41f9a 100644 --- a/jetty-maven-plugin/src/main/java/org/eclipse/jetty/maven/plugin/AbstractJettyMojo.java +++ b/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); } From 6be6fae291eff4ff8d7449d7f222812e02c76645 Mon Sep 17 00:00:00 2001 From: Joakim Erdfelt Date: Tue, 20 Oct 2020 16:30:39 -0500 Subject: [PATCH 3/5] Issue #5480 - NPE Check in WebInfConfiguration.deconfigure() + Removing Temp dir deletion in configureTempDirectory() Signed-off-by: Joakim Erdfelt --- .../jetty/webapp/WebInfConfiguration.java | 21 +++++++------------ 1 file changed, 8 insertions(+), 13 deletions(-) diff --git a/jetty-webapp/src/main/java/org/eclipse/jetty/webapp/WebInfConfiguration.java b/jetty-webapp/src/main/java/org/eclipse/jetty/webapp/WebInfConfiguration.java index c91bc65aa50..53ec88c942a 100644 --- a/jetty-webapp/src/main/java/org/eclipse/jetty/webapp/WebInfConfiguration.java +++ b/jetty-webapp/src/main/java/org/eclipse/jetty/webapp/WebInfConfiguration.java @@ -355,7 +355,7 @@ public class WebInfConfiguration extends AbstractConfiguration public void deconfigure(WebAppContext context) throws Exception { //if we're not persisting the temp dir contents delete it - if (!context.isPersistTempDirectory()) + if (!context.isPersistTempDirectory() && context.getTempDirectory() != null && context.getTempDirectory().exists()) { IO.delete(context.getTempDirectory()); } @@ -522,21 +522,16 @@ 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() && !context.isPersistTempDirectory()) + // if it doesn't exist make it + if (!dir.exists()) { - if (!IO.delete(dir)) - throw new IllegalStateException("Failed to delete temp dir " + dir); + if (!dir.mkdirs()) + { + throw new IllegalStateException("Unable to create temp dir " + dir); + } } - //if it doesn't exist make it - if (!dir.exists()) - dir.mkdirs(); - - if (!context.isPersistTempDirectory()) - dir.deleteOnExit(); - - //is it useable + // is it useable if (!dir.canWrite() || !dir.isDirectory()) throw new IllegalStateException("Temp dir " + dir + " not useable: writeable=" + dir.canWrite() + ", dir=" + dir.isDirectory()); } From c7ae5b50ab8bfffe46aca5cc8cf1ceef305fa316 Mon Sep 17 00:00:00 2001 From: Joakim Erdfelt Date: Tue, 20 Oct 2020 16:53:09 -0500 Subject: [PATCH 4/5] Issue #5480 - Safer temp directory management Signed-off-by: Joakim Erdfelt --- .../jetty/webapp/WebInfConfiguration.java | 26 ++++++++++++++++--- 1 file changed, 22 insertions(+), 4 deletions(-) diff --git a/jetty-webapp/src/main/java/org/eclipse/jetty/webapp/WebInfConfiguration.java b/jetty-webapp/src/main/java/org/eclipse/jetty/webapp/WebInfConfiguration.java index 53ec88c942a..6fbe03080dd 100644 --- a/jetty-webapp/src/main/java/org/eclipse/jetty/webapp/WebInfConfiguration.java +++ b/jetty-webapp/src/main/java/org/eclipse/jetty/webapp/WebInfConfiguration.java @@ -354,8 +354,10 @@ public class WebInfConfiguration extends AbstractConfiguration @Override public void deconfigure(WebAppContext context) throws Exception { - //if we're not persisting the temp dir contents delete it - if (!context.isPersistTempDirectory() && context.getTempDirectory() != null && context.getTempDirectory().exists()) + File tempDirectory = context.getTempDirectory(); + + // if we're not persisting the temp dir contents delete it + if (!context.isPersistTempDirectory() && tempDirectory != null && tempDirectory.exists()) { IO.delete(context.getTempDirectory()); } @@ -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,6 +526,12 @@ 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 (!context.isPersistTempDirectory() && dir.exists() && !IO.delete(dir)) + { + throw new IllegalStateException("Failed to delete temp dir " + dir); + } + // if it doesn't exist make it if (!dir.exists()) { @@ -531,6 +541,14 @@ public class WebInfConfiguration extends AbstractConfiguration } } + if (!context.isPersistTempDirectory()) + dir.deleteOnExit(); + + 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()); From fa6c8f7decf89ce00774f8aa34c2fd1efb23a156 Mon Sep 17 00:00:00 2001 From: Joakim Erdfelt Date: Tue, 20 Oct 2020 17:19:54 -0500 Subject: [PATCH 5/5] Issue #5480 - Only delete non-persist temp dir if not-empty Signed-off-by: Joakim Erdfelt --- .../java/org/eclipse/jetty/io/IOTest.java | 62 +++++++++++++++++++ .../main/java/org/eclipse/jetty/util/IO.java | 21 +++++++ .../jetty/webapp/WebInfConfiguration.java | 4 +- 3 files changed, 85 insertions(+), 2 deletions(-) diff --git a/jetty-io/src/test/java/org/eclipse/jetty/io/IOTest.java b/jetty-io/src/test/java/org/eclipse/jetty/io/IOTest.java index abe2f523a6e..fecc78aca89 100644 --- a/jetty-io/src/test/java/org/eclipse/jetty/io/IOTest.java +++ b/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 { diff --git a/jetty-util/src/main/java/org/eclipse/jetty/util/IO.java b/jetty-util/src/main/java/org/eclipse/jetty/util/IO.java index 7937481be62..412977ebcd6 100644 --- a/jetty-util/src/main/java/org/eclipse/jetty/util/IO.java +++ b/jetty-util/src/main/java/org/eclipse/jetty/util/IO.java @@ -381,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 * diff --git a/jetty-webapp/src/main/java/org/eclipse/jetty/webapp/WebInfConfiguration.java b/jetty-webapp/src/main/java/org/eclipse/jetty/webapp/WebInfConfiguration.java index 6fbe03080dd..34a3cef407a 100644 --- a/jetty-webapp/src/main/java/org/eclipse/jetty/webapp/WebInfConfiguration.java +++ b/jetty-webapp/src/main/java/org/eclipse/jetty/webapp/WebInfConfiguration.java @@ -357,9 +357,9 @@ public class WebInfConfiguration extends AbstractConfiguration File tempDirectory = context.getTempDirectory(); // if we're not persisting the temp dir contents delete it - if (!context.isPersistTempDirectory() && tempDirectory != null && tempDirectory.exists()) + 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