Merge pull request #253 from jglick/classpath-leak-redux

Classpath leak redux
jenkinsci · May 30, 2019 · 926841e · 926841e
2 parents 386607c + c1fc0b3
commit 926841e
Show file tree

Hide file tree

Showing 4 changed files with 90 additions and 33 deletions.
diff --git a/...java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SandboxResolvingClassLoader.java b/...java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SandboxResolvingClassLoader.java
@@ -26,27 +26,37 @@ class SandboxResolvingClassLoader extends ClassLoader {
 
     private static final Logger LOGGER = Logger.getLogger(SandboxResolvingClassLoader.class.getName());
 
-    private static final LoadingCache<ClassLoader, Cache<String, Optional<Class<?>>>> parentClassCache = makeParentCache();
+    private static final LoadingCache<ClassLoader, Cache<String, Class<?>>> parentClassCache = makeParentCache(true);
 
-    private static final LoadingCache<ClassLoader, Cache<String, Optional<URL>>> parentResourceCache = makeParentCache();
+    private static final LoadingCache<ClassLoader, Cache<String, Optional<URL>>> parentResourceCache = makeParentCache(false);
 
     SandboxResolvingClassLoader(ClassLoader parent) {
         super(parent);
     }
 
+    /**
+     * Marker value for a {@link ClassNotFoundException} negative cache hit.
+     * Cannot use null, since the cache API does not permit null values.
+     * Cannot use {@code Optional<Class<?>>} since weak values would mean this is always collected.
+     * This value is non-null, not a legitimate return value
+     * (no script should be trying to load this implementation detail), and strongly held.
+     */
+    private static final Class<?> CLASS_NOT_FOUND = Unused.class;
+    private static final class Unused {}
+
     @Override protected synchronized Class<?> loadClass(String name, boolean resolve) throws ClassNotFoundException {
         if (name.startsWith("org.kohsuke.groovy.sandbox.")) {
             return this.getClass().getClassLoader().loadClass(name);
         } else {
             ClassLoader parentLoader = getParent();
             Class<?> c = load(parentClassCache, name, parentLoader, () -> {
                 try {
-                    return Optional.of(parentLoader.loadClass(name));
+                    return parentLoader.loadClass(name);
                 } catch (ClassNotFoundException x) {
-                    return Optional.absent();
+                    return CLASS_NOT_FOUND;
                 }
-            }).orNull();
-            if (c != null) {
+            });
+            if (c != CLASS_NOT_FOUND) {
                 if (resolve) {
                     super.resolveClass(c);
                 }
@@ -89,8 +99,12 @@ private static <T> T load(LoadingCache<ClassLoader, Cache<String, T>> cache, Str
         }
     }
 
-    private static <T> LoadingCache<ClassLoader, Cache<String, T>> makeParentCache() {
-        return CacheBuilder.newBuilder().weakKeys().build(new CacheLoader<ClassLoader, Cache<String, T>>() {
+    private static <T> LoadingCache<ClassLoader, Cache<String, T>> makeParentCache(boolean weakValues) {
+        CacheBuilder<Object, Object> builder = CacheBuilder.newBuilder().weakKeys();
+        if (weakValues) {
+            builder = builder.weakValues();
+        }
+        return builder.build(new CacheLoader<ClassLoader, Cache<String, T>>() {
             @Override public Cache<String, T> load(ClassLoader parentLoader) {
                 return CacheBuilder.newBuilder()./* allow new plugins to be used, and clean up memory */expireAfterWrite(15, TimeUnit.MINUTES).build();
             }

diff --git a/src/main/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SecureGroovyScript.java b/src/main/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SecureGroovyScript.java
@@ -151,36 +151,49 @@ public SecureGroovyScript configuringWithNonKeyItem() {
     }
 
     private static void cleanUpLoader(ClassLoader loader, Set<ClassLoader> encounteredLoaders, Set<Class<?>> encounteredClasses) throws Exception {
-        /*if (loader instanceof CpsGroovyShell.TimingLoader) {
-            cleanUpLoader(loader.getParent(), encounteredLoaders, encounteredClasses);
-            return;
-        }*/
-        // Check me, am I cleaning up the right loader???
-        if (!(loader instanceof GroovyClassLoader)) {
-            LOGGER.log(Level.FINER, "ignoring {0}", loader);
-            return;
-        }
         if (!encounteredLoaders.add(loader)) {
             return;
         }
-        cleanUpLoader(loader.getParent(), encounteredLoaders, encounteredClasses);
         if (LOGGER.isLoggable(Level.FINER)) {
           LOGGER.log(Level.FINER, "found {0}", String.valueOf(loader));
         }
-        cleanUpGlobalClassValue(loader);
-        GroovyClassLoader gcl = (GroovyClassLoader) loader;
-        for (Class<?> clazz : gcl.getLoadedClasses()) {
-            if (encounteredClasses.add(clazz)) {
-                LOGGER.log(Level.FINER, "found {0}", clazz.getName());
-                Introspector.flushFromCaches(clazz);
-                cleanUpGlobalClassSet(clazz);
-                cleanUpObjectStreamClassCaches(clazz);
-                cleanUpLoader(clazz.getClassLoader(), encounteredLoaders, encounteredClasses);
+        if (loader instanceof GroovyClassLoader) {
+            GroovyClassLoader gcl = (GroovyClassLoader) loader;
+            for (Class<?> clazz : gcl.getLoadedClasses()) {
+                cleanUpClass(clazz, encounteredLoaders, encounteredClasses);
+            }
+            gcl.clearCache();
+        } else if (loader instanceof SandboxResolvingClassLoader) {
+            // OK, just check its parent
+        } else if (loader instanceof ClasspathURLClassLoader) {
+            Collection<Class<?>> loadedClasses = ((ClasspathURLClassLoader) loader).loadedClasses;
+            synchronized (loadedClasses) {
+                loadedClasses = new ArrayList<>(loadedClasses);
+            }
+            for (Class<?> clazz : loadedClasses) {
+                cleanUpClass(clazz, encounteredLoaders, encounteredClasses);
             }
+        } else {
+            LOGGER.log(Level.FINER, "ignoring {0}", loader);
+            return;
+        }
+        cleanUpGlobalClassValue(loader);
+        cleanUpLoader(loader.getParent(), encounteredLoaders, encounteredClasses);
+    }
+
+    private static void cleanUpClass(Class<?> clazz, Set<ClassLoader> encounteredLoaders, Set<Class<?>> encounteredClasses) throws Exception {
+        if (encounteredClasses.add(clazz)) {
+            LOGGER.log(Level.FINER, "found {0}", clazz.getName());
+            Introspector.flushFromCaches(clazz);
+            cleanUpGlobalClassSet(clazz);
+            cleanUpClassHelperCache(clazz);
+            cleanUpObjectStreamClassCaches(clazz);
+            cleanUpLoader(clazz.getClassLoader(), encounteredLoaders, encounteredClasses);
         }
-        gcl.clearCache();
     }
 
+    // TODO copied with modifications from CpsFlowExecution; need to find a way to share commonalities
+
     private static void cleanUpGlobalClassValue(@Nonnull ClassLoader loader) throws Exception {
         Class<?> classInfoC = Class.forName("org.codehaus.groovy.reflection.ClassInfo");
         // TODO switch to MethodHandle for speed
@@ -264,6 +277,16 @@ private static void cleanUpGlobalClassSet(@Nonnull Class<?> clazz) throws Except
         }
     }
 
+    private static void cleanUpClassHelperCache(@Nonnull Class<?> clazz) throws Exception {
+        Field classCacheF = Class.forName("org.codehaus.groovy.ast.ClassHelper$ClassHelperCache").getDeclaredField("classCache");
+        classCacheF.setAccessible(true);
+        Object classCache = classCacheF.get(null);
+        if (LOGGER.isLoggable(Level.FINER)) {
+            LOGGER.log(Level.FINER, "cleaning up {0} from ClassHelperCache? {1}", new Object[] {clazz.getName(), classCache.getClass().getMethod("get", Object.class).invoke(classCache, clazz) != null});
+        }
+        classCache.getClass().getMethod("remove", Object.class).invoke(classCache, clazz);
+    }
+
     private static void cleanUpObjectStreamClassCaches(@Nonnull Class<?> clazz) throws Exception {
         Class<?> cachesC = Class.forName("java.io.ObjectStreamClass$Caches");
         for (String cacheFName : new String[] {"localDescs", "reflectors"}) {
@@ -316,7 +339,7 @@ public Object evaluate(ClassLoader loader, Binding binding, @CheckForNull TaskLi
                 urlList.add(entry.getURL());
             }
 
-            loader = urlcl = new URLClassLoader(urlList.toArray(new URL[urlList.size()]), loader);
+            loader = urlcl = new ClasspathURLClassLoader(urlList.toArray(new URL[urlList.size()]), loader);
         }
         boolean canDoCleanup = false;
 
@@ -367,6 +390,26 @@ public Object evaluate(ClassLoader loader, Binding binding, @CheckForNull TaskLi
         }
     }
 
+    /**
+     * Both serves as a marker that we should clean classes from here, and tracks which classes were loaded.
+     */
+    private static final class ClasspathURLClassLoader extends URLClassLoader {
+
+        private final Collection<Class<?>> loadedClasses = new ArrayList<>();
+
+        ClasspathURLClassLoader(URL[] urls, ClassLoader parent) {
+            super(urls, parent);
+        }
+
+        @Override protected Class<?> findClass(String name) throws ClassNotFoundException {
+            Class<?> c = super.findClass(name);
+            synchronized (loadedClasses) {
+                loadedClasses.add(c);
+            }
+            return c;
+        }
+
+    }
 
     /**
      * Disables the weird and unreliable {@link groovy.lang.GroovyClassLoader.InnerLoader}.

diff --git a/src/test/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/GroovyMemoryLeakTest.java b/src/test/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/GroovyMemoryLeakTest.java
@@ -9,7 +9,6 @@
 import java.util.Collections;
 import java.util.List;
 import java.util.logging.Level;
-import org.apache.commons.text.CharacterPredicates;
 import org.codehaus.groovy.reflection.ClassInfo;
 import org.jenkinsci.plugins.scriptsecurity.scripts.ClasspathEntry;
 import org.junit.After;
@@ -49,9 +48,10 @@ public static void register(Object o) {
     @Test
     public void loaderReleased() throws Exception {
         FreeStyleProject p = r.jenkins.createProject(FreeStyleProject.class, "p");
-        String cp = CharacterPredicates.class.getProtectionDomain().getCodeSource().getLocation().toString(); // some JAR
-        p.getPublishersList().add(new TestGroovyRecorder(
-            new SecureGroovyScript(GroovyMemoryLeakTest.class.getName()+".register(this)", false, Collections.singletonList(new ClasspathEntry(cp)))));
+        String cp = GroovyMemoryLeakTest.class.getResource("somejar.jar").toString();
+        p.getPublishersList().add(new TestGroovyRecorder(new SecureGroovyScript(
+            GroovyMemoryLeakTest.class.getName() + ".register(this); new somepkg.SomeClass()",
+            false, Collections.singletonList(new ClasspathEntry(cp)))));
         r.buildAndAssertSuccess(p);
 
         assertFalse(LOADERS.isEmpty());

diff --git a/src/test/resources/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/somejar.jar b/src/test/resources/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/somejar.jar