From 27c4935e360a642c5179bcf0b5a92d655f04dd7c Mon Sep 17 00:00:00 2001 From: Michael Jumper Date: Thu, 20 Jan 2022 16:03:17 -0800 Subject: [PATCH] GUACAMOLE-1508: Automatically delete temporary files on webapp shutdown. --- .../GuacamoleServletContextListener.java | 56 ++++++++++++++++--- .../apache/guacamole/extension/Extension.java | 11 +++- .../extension/ExtensionClassLoader.java | 32 ++++++++--- .../guacamole/extension/ExtensionModule.java | 23 +++++++- 4 files changed, 103 insertions(+), 19 deletions(-) diff --git a/guacamole/src/main/java/org/apache/guacamole/GuacamoleServletContextListener.java b/guacamole/src/main/java/org/apache/guacamole/GuacamoleServletContextListener.java index cd81572ec..2b3f155d0 100644 --- a/guacamole/src/main/java/org/apache/guacamole/GuacamoleServletContextListener.java +++ b/guacamole/src/main/java/org/apache/guacamole/GuacamoleServletContextListener.java @@ -19,6 +19,7 @@ package org.apache.guacamole; +import com.google.common.collect.Lists; import org.apache.guacamole.tunnel.TunnelModule; import com.google.inject.Guice; import com.google.inject.Injector; @@ -117,6 +118,14 @@ public class GuacamoleServletContextListener extends GuiceServletContextListener @Inject private List authProviders; + /** + * All temporary files that should be deleted upon application shutdown, in + * reverse order of desired deletion. This will typically simply be the + * order that each file was created. + */ + @Inject + private List temporaryFiles; + /** * Internal reference to the Guice injector that was lazily created when * getInjector() was first invoked. @@ -194,20 +203,49 @@ public class GuacamoleServletContextListener extends GuiceServletContextListener }); } + /** + * Deletes the given temporary file/directory, if possible. If the deletion + * operation fails, a warning is logged noting the failure. If the given + * file is a directory, it will only be deleted if empty. + * + * @param temp + * The temporary file to delete. + */ + private void deleteTemporaryFile(File temp) { + if (!temp.delete()) { + logger.warn("Temporary file/directory \"{}\" could not be " + + "deleted. The file may remain until the JVM exits, or " + + "may need to be manually deleted.", temp); + } + else + logger.debug("Deleted temporary file/directory \"{}\".", temp); + } + @Override public void contextDestroyed(ServletContextEvent servletContextEvent) { + try { + + // Clean up reference to Guice injector + servletContextEvent.getServletContext().removeAttribute(GUICE_INJECTOR); - // Clean up reference to Guice injector - servletContextEvent.getServletContext().removeAttribute(GUICE_INJECTOR); + // Shutdown TokenSessionMap + if (sessionMap != null) + sessionMap.shutdown(); - // Shutdown TokenSessionMap - if (sessionMap != null) - sessionMap.shutdown(); + // Unload all extensions + if (authProviders != null) { + for (AuthenticationProvider authProvider : authProviders) + authProvider.shutdown(); + } + + } + finally { + + // Regardless of what may succeed/fail here, always attempt to + // clean up ALL temporary files + if (temporaryFiles != null) + Lists.reverse(temporaryFiles).stream().forEachOrdered(this::deleteTemporaryFile); - // Unload all extensions - if (authProviders != null) { - for (AuthenticationProvider authProvider : authProviders) - authProvider.shutdown(); } // Continue any Guice-specific cleanup diff --git a/guacamole/src/main/java/org/apache/guacamole/extension/Extension.java b/guacamole/src/main/java/org/apache/guacamole/extension/Extension.java index 5c74c897d..f6cf80df2 100644 --- a/guacamole/src/main/java/org/apache/guacamole/extension/Extension.java +++ b/guacamole/src/main/java/org/apache/guacamole/extension/Extension.java @@ -27,6 +27,7 @@ import java.util.ArrayList; import java.util.Collection; import java.util.Collections; import java.util.LinkedHashMap; +import java.util.List; import java.util.Map; import java.util.zip.ZipEntry; import java.util.zip.ZipException; @@ -355,12 +356,18 @@ public class Extension { * @param file * The file to load as an extension. * + * @param temporaryFiles + * A modifiable List that should be populated with all temporary files + * created for this extension. These files should be deleted on + * application shutdown in reverse order. + * * @throws GuacamoleException * If the provided file is not a .jar file, does not contain the * guac-manifest.json, or if guac-manifest.json is invalid and cannot * be parsed. */ - public Extension(final ClassLoader parent, final File file) throws GuacamoleException { + public Extension(final ClassLoader parent, final File file, + final List temporaryFiles) throws GuacamoleException { // Associate extension abstraction with original file this.file = file; @@ -390,7 +397,7 @@ public class Extension { } // Create isolated classloader for this extension - classLoader = ExtensionClassLoader.getInstance(file, parent); + classLoader = ExtensionClassLoader.getInstance(file, temporaryFiles, parent); } diff --git a/guacamole/src/main/java/org/apache/guacamole/extension/ExtensionClassLoader.java b/guacamole/src/main/java/org/apache/guacamole/extension/ExtensionClassLoader.java index 3297b7e89..c32fbde3a 100644 --- a/guacamole/src/main/java/org/apache/guacamole/extension/ExtensionClassLoader.java +++ b/guacamole/src/main/java/org/apache/guacamole/extension/ExtensionClassLoader.java @@ -84,6 +84,11 @@ public class ExtensionClassLoader extends URLClassLoader { * @param extension * The extension .jar file from which classes should be loaded. * + * @param temporaryFiles + * A modifiable List that should be populated with all temporary files + * created for the given extension. These files should be deleted on + * application shutdown in reverse order. + * * @param parent * The ClassLoader to use if class resolution through the extension * .jar fails. @@ -97,7 +102,8 @@ public class ExtensionClassLoader extends URLClassLoader { * file cannot be read. */ public static ExtensionClassLoader getInstance(final File extension, - final ClassLoader parent) throws GuacamoleException { + final List temporaryFiles, final ClassLoader parent) + throws GuacamoleException { try { // Attempt to create classloader which loads classes from the given @@ -106,7 +112,7 @@ public class ExtensionClassLoader extends URLClassLoader { @Override public ExtensionClassLoader run() throws GuacamoleException { - return new ExtensionClassLoader(extension, parent); + return new ExtensionClassLoader(extension, temporaryFiles, parent); } }); @@ -194,6 +200,11 @@ public class ExtensionClassLoader extends URLClassLoader { * @param extension * The extension .jar file to generate URLs for. * + * @param temporaryFiles + * A modifiable List that should be populated with all temporary files + * created for the given extension. These files should be deleted on + * application shutdown in reverse order. + * * @return * An array of all URLs relevant to the given extension .jar. * @@ -202,8 +213,8 @@ public class ExtensionClassLoader extends URLClassLoader { * cannot be read, or any necessary temporary files/directories cannot * be created. */ - private static URL[] getExtensionURLs(File extension) - throws GuacamoleException { + private static URL[] getExtensionURLs(File extension, + List temporaryFiles) throws GuacamoleException { JarFile extensionJar; try { @@ -238,6 +249,7 @@ public class ExtensionClassLoader extends URLClassLoader { try { if (extensionTempLibDir == null) { extensionTempLibDir = Files.createTempDirectory(EXTENSION_TEMP_DIR_PREFIX); + temporaryFiles.add(extensionTempLibDir.toFile()); extensionTempLibDir.toFile().deleteOnExit(); } } @@ -252,6 +264,7 @@ public class ExtensionClassLoader extends URLClassLoader { File tempLibrary; try { tempLibrary = Files.createTempFile(extensionTempLibDir, EXTENSION_TEMP_LIB_PREFIX, ".jar").toFile(); + temporaryFiles.add(tempLibrary); tempLibrary.deleteOnExit(); } catch (IOException e) { @@ -295,6 +308,11 @@ public class ExtensionClassLoader extends URLClassLoader { * @param extension * The extension .jar file from which classes should be loaded. * + * @param temporaryFiles + * A modifiable List that should be populated with all temporary files + * created for the given extension. These files should be deleted on + * application shutdown in reverse order. + * * @param parent * The ClassLoader to use if class resolution through the extension * .jar fails. @@ -303,9 +321,9 @@ public class ExtensionClassLoader extends URLClassLoader { * If the given file is not actually a file, or the contents of the * file cannot be read. */ - private ExtensionClassLoader(File extension, ClassLoader parent) - throws GuacamoleException { - super(getExtensionURLs(extension), null); + private ExtensionClassLoader(File extension, List temporaryFiles, + ClassLoader parent) throws GuacamoleException { + super(getExtensionURLs(extension, temporaryFiles), null); this.parent = parent; } diff --git a/guacamole/src/main/java/org/apache/guacamole/extension/ExtensionModule.java b/guacamole/src/main/java/org/apache/guacamole/extension/ExtensionModule.java index d82552dad..0fcff57dd 100644 --- a/guacamole/src/main/java/org/apache/guacamole/extension/ExtensionModule.java +++ b/guacamole/src/main/java/org/apache/guacamole/extension/ExtensionModule.java @@ -140,6 +140,13 @@ public class ExtensionModule extends ServletModule { private final List boundListeners = new ArrayList(); + /** + * All temporary files that should be deleted upon application shutdown, in + * reverse order of desired deletion. This will typically simply be the + * order that each file was created. + */ + private final List temporaryFiles = new ArrayList<>(); + /** * Service for adding and retrieving language resources. */ @@ -261,6 +268,20 @@ public class ExtensionModule extends ServletModule { return Collections.unmodifiableList(boundAuthenticationProviders); } + /** + * Returns a list of all temporary files that should be deleted upon + * application shutdown, in reverse order of desired deletion. This will + * typically simply be the order that each file was created. + * + * @return + * A List of all temporary files that should be deleted upon + * application shutdown. The List is not modifiable. + */ + @Provides + public List getTemporaryFiles() { + return Collections.unmodifiableList(temporaryFiles); + } + /** * Binds the given provider class such that a listener is bound for each * listener interface implemented by the provider and such that all bound @@ -479,7 +500,7 @@ public class ExtensionModule extends ServletModule { try { // Load extension from file - Extension extension = new Extension(getParentClassLoader(), extensionFile); + Extension extension = new Extension(getParentClassLoader(), extensionFile, temporaryFiles); // Validate Guacamole version of extension if (!isCompatible(extension.getGuacamoleVersion())) {