From 38f366874c54f2f65b54ad6b3e654bb2d165f5b3 Mon Sep 17 00:00:00 2001 From: Devin Nusbaum Date: Mon, 23 Sep 2019 12:08:59 -0400 Subject: [PATCH 1/4] Use weakValues on the inner caches in SandboxResolvingClassLoader.parentClassCache instead of on the outer caches --- .../groovy/SandboxResolvingClassLoader.java | 22 ++++--- .../SandboxResolvingClassLoaderTest.java | 59 +++++++++++++++++++ 2 files changed, 73 insertions(+), 8 deletions(-) create mode 100644 src/test/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SandboxResolvingClassLoaderTest.java diff --git a/src/main/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SandboxResolvingClassLoader.java b/src/main/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SandboxResolvingClassLoader.java index 76318148b..b7822d16f 100644 --- a/src/main/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SandboxResolvingClassLoader.java +++ b/src/main/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SandboxResolvingClassLoader.java @@ -23,9 +23,9 @@ class SandboxResolvingClassLoader extends ClassLoader { private static final Logger LOGGER = Logger.getLogger(SandboxResolvingClassLoader.class.getName()); - private static final LoadingCache>> parentClassCache = makeParentCache(true); + static final LoadingCache>> parentClassCache = makeParentCache(true); - private static final LoadingCache>> parentResourceCache = makeParentCache(false); + static final LoadingCache>> parentResourceCache = makeParentCache(false); SandboxResolvingClassLoader(ClassLoader parent) { super(parent); @@ -38,7 +38,7 @@ class SandboxResolvingClassLoader extends ClassLoader { * 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; + static final Class CLASS_NOT_FOUND = Unused.class; private static final class Unused {} @Override protected synchronized Class loadClass(String name, boolean resolve) throws ClassNotFoundException { @@ -93,12 +93,18 @@ private static T load(LoadingCache> cache, Str }); } - private static LoadingCache> makeParentCache(boolean weakValues) { - Caffeine builder = Caffeine.newBuilder().weakKeys(); - if (weakValues) { - builder = builder.weakValues(); + private static LoadingCache> makeParentCache(boolean weakValuesInnerCache) { + // The outer cache has weak keys, so that we do not leak class loaders, but strong values, because the + // inner caches are only referenced by the outer cache internally. + Caffeine outerBuilder = Caffeine.newBuilder().weakKeys(); + // The inner cache has strong keys, since they are just strings, and expires entries 15 minutes after they are + // added to the cache (TODO: should we use expireAfterAccess instead?). The values for the inner cache may + // be weak if needed, for example parentClassCache uses weak values to avoid leaking classes and their loaders. + Caffeine innerBuilder = Caffeine.newBuilder().expireAfterWrite(Duration.ofMinutes(15)); + if (weakValuesInnerCache) { + innerBuilder.weakValues(); } - return builder.build(parentLoader -> Caffeine.newBuilder()./* allow new plugins to be used, and clean up memory */expireAfterWrite(Duration.ofMinutes(15)).build()); + return outerBuilder.build(parentLoader -> innerBuilder.build()); } } diff --git a/src/test/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SandboxResolvingClassLoaderTest.java b/src/test/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SandboxResolvingClassLoaderTest.java new file mode 100644 index 000000000..e9854407e --- /dev/null +++ b/src/test/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SandboxResolvingClassLoaderTest.java @@ -0,0 +1,59 @@ +/* + * The MIT License + * + * Copyright 2019 CloudBees, Inc. + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN + * THE SOFTWARE. + */ + +package org.jenkinsci.plugins.scriptsecurity.sandbox.groovy; + +import org.junit.Test; + +import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.equalTo; +import static org.jenkinsci.plugins.scriptsecurity.sandbox.groovy.SandboxResolvingClassLoader.CLASS_NOT_FOUND; +import static org.jenkinsci.plugins.scriptsecurity.sandbox.groovy.SandboxResolvingClassLoader.parentClassCache; +import static org.junit.Assert.assertThat; +import static org.junit.Assert.fail; + +public class SandboxResolvingClassLoaderTest { + + private final ClassLoader parentLoader = SandboxResolvingClassLoaderTest.class.getClassLoader(); + private final SandboxResolvingClassLoader loader = new SandboxResolvingClassLoader(parentLoader); + + @Test public void classCacheDoesNotHoldClassValuesTooWeakly() throws Exception { + // Load a class that does exist. + assertThat(loader.loadClass("java.lang.String", false), equalTo(String.class)); + // Load a class that does not exist. + try { + loader.loadClass("this.does.not.Exist", false); + fail("Class should not exist"); + } catch (ClassNotFoundException e) { + assertThat(e.getMessage(), containsString("this.does.not.Exist")); + } + // The result of both of the class loading attempts should exist in the cache. + assertThat(parentClassCache.get(parentLoader).getIfPresent("java.lang.String"), equalTo(String.class)); + assertThat(parentClassCache.get(parentLoader).getIfPresent("this.does.not.Exist"), equalTo(CLASS_NOT_FOUND)); + // Make sure that both of the entries are still in the cache after a GC. + System.gc(); + assertThat(parentClassCache.get(parentLoader).getIfPresent("java.lang.String"), equalTo(String.class)); + assertThat(parentClassCache.get(parentLoader).getIfPresent("this.does.not.Exist"), equalTo(CLASS_NOT_FOUND)); + } +} From ec71846f5856a33503565e6aa8ae4f7da45ff771 Mon Sep 17 00:00:00 2001 From: Devin Nusbaum Date: Mon, 23 Sep 2019 16:02:28 -0400 Subject: [PATCH 2/4] Update comment to explain reasoning behind expiration --- .../sandbox/groovy/SandboxResolvingClassLoader.java | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/main/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SandboxResolvingClassLoader.java b/src/main/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SandboxResolvingClassLoader.java index b7822d16f..fb641a42a 100644 --- a/src/main/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SandboxResolvingClassLoader.java +++ b/src/main/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SandboxResolvingClassLoader.java @@ -98,8 +98,10 @@ private static LoadingCache> makeParentCache(b // inner caches are only referenced by the outer cache internally. Caffeine outerBuilder = Caffeine.newBuilder().weakKeys(); // The inner cache has strong keys, since they are just strings, and expires entries 15 minutes after they are - // added to the cache (TODO: should we use expireAfterAccess instead?). The values for the inner cache may - // be weak if needed, for example parentClassCache uses weak values to avoid leaking classes and their loaders. + // added to the cache, so that classes defined by dynamically installed plugins become available even if there + // were negative cache hits prior to the installation (ideally this would be done with a listener). The values + // for the inner cache may be weak if needed, for example parentClassCache uses weak values to avoid leaking + // classes and their loaders. Caffeine innerBuilder = Caffeine.newBuilder().expireAfterWrite(Duration.ofMinutes(15)); if (weakValuesInnerCache) { innerBuilder.weakValues(); From e46d4792f376fc298755ce35e0424b6e03b9f85a Mon Sep 17 00:00:00 2001 From: Devin Nusbaum Date: Mon, 30 Sep 2019 12:22:13 -0400 Subject: [PATCH 3/4] Enable statistics collection on caches --- .../sandbox/groovy/SandboxResolvingClassLoader.java | 4 ++-- .../sandbox/groovy/SandboxResolvingClassLoaderTest.java | 8 ++++++++ 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/src/main/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SandboxResolvingClassLoader.java b/src/main/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SandboxResolvingClassLoader.java index fb641a42a..f471a947f 100644 --- a/src/main/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SandboxResolvingClassLoader.java +++ b/src/main/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SandboxResolvingClassLoader.java @@ -96,13 +96,13 @@ private static T load(LoadingCache> cache, Str private static LoadingCache> makeParentCache(boolean weakValuesInnerCache) { // The outer cache has weak keys, so that we do not leak class loaders, but strong values, because the // inner caches are only referenced by the outer cache internally. - Caffeine outerBuilder = Caffeine.newBuilder().weakKeys(); + Caffeine outerBuilder = Caffeine.newBuilder().recordStats().weakKeys(); // The inner cache has strong keys, since they are just strings, and expires entries 15 minutes after they are // added to the cache, so that classes defined by dynamically installed plugins become available even if there // were negative cache hits prior to the installation (ideally this would be done with a listener). The values // for the inner cache may be weak if needed, for example parentClassCache uses weak values to avoid leaking // classes and their loaders. - Caffeine innerBuilder = Caffeine.newBuilder().expireAfterWrite(Duration.ofMinutes(15)); + Caffeine innerBuilder = Caffeine.newBuilder().recordStats().expireAfterWrite(Duration.ofMinutes(15)); if (weakValuesInnerCache) { innerBuilder.weakValues(); } diff --git a/src/test/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SandboxResolvingClassLoaderTest.java b/src/test/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SandboxResolvingClassLoaderTest.java index e9854407e..bc2c68915 100644 --- a/src/test/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SandboxResolvingClassLoaderTest.java +++ b/src/test/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SandboxResolvingClassLoaderTest.java @@ -24,7 +24,9 @@ package org.jenkinsci.plugins.scriptsecurity.sandbox.groovy; +import com.github.benmanes.caffeine.cache.stats.CacheStats; import org.junit.Test; +import org.jvnet.hudson.test.Issue; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.equalTo; @@ -38,6 +40,7 @@ public class SandboxResolvingClassLoaderTest { private final ClassLoader parentLoader = SandboxResolvingClassLoaderTest.class.getClassLoader(); private final SandboxResolvingClassLoader loader = new SandboxResolvingClassLoader(parentLoader); + @Issue("JENKINS-59587") @Test public void classCacheDoesNotHoldClassValuesTooWeakly() throws Exception { // Load a class that does exist. assertThat(loader.loadClass("java.lang.String", false), equalTo(String.class)); @@ -55,5 +58,10 @@ public class SandboxResolvingClassLoaderTest { System.gc(); assertThat(parentClassCache.get(parentLoader).getIfPresent("java.lang.String"), equalTo(String.class)); assertThat(parentClassCache.get(parentLoader).getIfPresent("this.does.not.Exist"), equalTo(CLASS_NOT_FOUND)); + CacheStats stats = parentClassCache.get(parentLoader).stats(); + // Before the fix for JENKINS-59587, the original inner cache was removed after the call to `System.gc()`, so + // the miss count was 2 and the hit count was 0. + assertThat(stats.missCount(), equalTo(2L)); // The two calls to `loadClass()` + assertThat(stats.hitCount(), equalTo(4L)); // The four calls to `getIfPresent()` } } From 0a222ce7dbecff8664ecef61e6119c8ee4a80161 Mon Sep 17 00:00:00 2001 From: Devin Nusbaum Date: Mon, 30 Sep 2019 13:04:03 -0400 Subject: [PATCH 4/4] Run each test class in a separate JVM process --- pom.xml | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/pom.xml b/pom.xml index 8b6b27b76..a91182189 100644 --- a/pom.xml +++ b/pom.xml @@ -47,6 +47,18 @@ + + + + org.apache.maven.plugins + maven-surefire-plugin + + false + + + + + org.kohsuke