Skip to content

Commit

Permalink
Merge pull request #161 from svanoort/memory-cleanup
Browse files Browse the repository at this point in the history 
[JENKINS-47758] Script security provides automatic memory leak protection to many groovy scripts
  • Loading branch information
@svanoort
svanoort committed Nov 2, 2017
2 parents 763a5db + 231a887 commit 2699d5d
Show file tree
Hide file tree
Showing 3 changed files with 310 additions and 5 deletions.
22 changes: 20 additions & 2 deletions pom.xml
View file
Expand Up @@ -3,7 +3,7 @@
<parent>
<groupId>org.jenkins-ci.plugins</groupId>
<artifactId>plugin</artifactId>
<version>2.30</version>
<version>2.37</version>
<relativePath />
</parent>

Expand Down Expand Up @@ -55,5 +55,23 @@
</exclusion>
</exclusions>
</dependency>
</dependencies>
<dependency> <!-- For memory leak testing. -->
<groupId>org.jvnet.hudson.plugins</groupId>
<artifactId>groovy-postbuild</artifactId>
<version>2.3.1</version>
<scope>test</scope>
</dependency>
<dependency> <!-- Required for groovy postbuild. -->
<groupId>org.jenkins-ci.plugins</groupId>
<artifactId>matrix-project</artifactId>
<version>1.4.1</version>
<scope>test</scope>
</dependency>
<dependency> <!-- Required for groovy postbuild, because needed for matrix-project after unbundling. -->
<groupId>org.jenkins-ci.plugins</groupId>
<artifactId>junit</artifactId>
<version>1.15</version>
<scope>test</scope>
</dependency>
</dependencies>
</project>
225 changes: 222 additions & 3 deletions src/main/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SecureGroovyScript.java
View file
Expand Up @@ -26,22 +26,40 @@

import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
import groovy.lang.Binding;
import groovy.lang.GroovyClassLoader;
import groovy.lang.GroovyShell;
import hudson.Extension;
import hudson.PluginManager;
import hudson.model.AbstractDescribableImpl;
import hudson.model.Descriptor;
import hudson.model.Item;
import hudson.util.FormValidation;

import java.beans.Introspector;
import java.lang.ref.Reference;
import java.lang.ref.WeakReference;
import java.lang.reflect.Field;
import java.lang.reflect.Method;
import java.net.URL;
import java.net.URLClassLoader;
import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
import java.util.HashSet;
import java.util.Iterator;
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.concurrent.ConcurrentMap;
import java.util.logging.Level;
import java.util.logging.Logger;
import javax.annotation.CheckForNull;
import javax.annotation.Nonnull;
import jenkins.model.Jenkins;
import org.codehaus.groovy.control.CompilationFailedException;
import org.codehaus.groovy.control.CompilationUnit;
import org.codehaus.groovy.control.CompilerConfiguration;
import org.codehaus.groovy.control.SourceUnit;
import org.jenkinsci.plugins.scriptsecurity.sandbox.RejectedAccessException;
import org.jenkinsci.plugins.scriptsecurity.sandbox.Whitelist;
import org.jenkinsci.plugins.scriptsecurity.scripts.ApprovalContext;
Expand All @@ -68,6 +86,8 @@ public final class SecureGroovyScript extends AbstractDescribableImpl<SecureGroo
private final @CheckForNull List<ClasspathEntry> classpath;
private transient boolean calledConfiguring;

static final Logger LOGGER = Logger.getLogger(SecureGroovyScript.class.getName());

@DataBoundConstructor public SecureGroovyScript(@Nonnull String script, boolean sandbox, @CheckForNull List<ClasspathEntry> classpath) {
this.script = script;
this.sandbox = sandbox;
Expand Down Expand Up @@ -130,6 +150,133 @@ public SecureGroovyScript configuringWithNonKeyItem() {
return configuring(context);
}

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);
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);
}
}
gcl.clearCache();
}

private static void cleanUpGlobalClassValue(@Nonnull ClassLoader loader) throws Exception {
Class<?> classInfoC = Class.forName("org.codehaus.groovy.reflection.ClassInfo");
// TODO switch to MethodHandle for speed
Field globalClassValueF = classInfoC.getDeclaredField("globalClassValue");
globalClassValueF.setAccessible(true);
Object globalClassValue = globalClassValueF.get(null);
Class<?> groovyClassValuePreJava7C = Class.forName("org.codehaus.groovy.reflection.GroovyClassValuePreJava7");
if (!groovyClassValuePreJava7C.isInstance(globalClassValue)) {
return; // using GroovyClassValueJava7 due to -Dgroovy.use.classvalue or on IBM J9, fine
}
Field mapF = groovyClassValuePreJava7C.getDeclaredField("map");
mapF.setAccessible(true);
Object map = mapF.get(globalClassValue);
Class<?> groovyClassValuePreJava7Map = Class.forName("org.codehaus.groovy.reflection.GroovyClassValuePreJava7$GroovyClassValuePreJava7Map");
Collection entries = (Collection) groovyClassValuePreJava7Map.getMethod("values").invoke(map);
Method removeM = groovyClassValuePreJava7Map.getMethod("remove", Object.class);
Class<?> entryC = Class.forName("org.codehaus.groovy.util.AbstractConcurrentMapBase$Entry");
Method getValueM = entryC.getMethod("getValue");
List<Class<?>> toRemove = new ArrayList<>(); // not sure if it is safe against ConcurrentModificationException or not
try {
Field classRefF = classInfoC.getDeclaredField("classRef"); // 2.4.8+
classRefF.setAccessible(true);
for (Object entry : entries) {
Object value = getValueM.invoke(entry);
toRemove.add(((WeakReference<Class<?>>) classRefF.get(value)).get());
}
} catch (NoSuchFieldException x) {
Field klazzF = classInfoC.getDeclaredField("klazz"); // 2.4.7-
klazzF.setAccessible(true);
for (Object entry : entries) {
Object value = getValueM.invoke(entry);
toRemove.add((Class) klazzF.get(value));
}
}
Iterator<Class<?>> it = toRemove.iterator();
while (it.hasNext()) {
Class<?> klazz = it.next();
ClassLoader encounteredLoader = klazz.getClassLoader();
if (encounteredLoader != loader) {
it.remove();
LOGGER.log(Level.FINEST, "ignoring {0} with loader {1}", new Object[] {klazz, /* do not hold from LogRecord */String.valueOf(encounteredLoader)});
}
}
LOGGER.log(Level.FINE, "cleaning up {0} associated with {1}", new Object[] {toRemove.toString(), loader.toString()});
for (Class<?> klazz : toRemove) {
removeM.invoke(map, klazz);
}
}

private static void cleanUpGlobalClassSet(@Nonnull Class<?> clazz) throws Exception {
Class<?> classInfoC = Class.forName("org.codehaus.groovy.reflection.ClassInfo"); // or just ClassInfo.class, but unclear whether this will always be there
Field globalClassSetF = classInfoC.getDeclaredField("globalClassSet");
globalClassSetF.setAccessible(true);
Object globalClassSet = globalClassSetF.get(null);
try {
classInfoC.getDeclaredField("classRef");
return; // 2.4.8+, nothing to do here (classRef is weak anyway)
} catch (NoSuchFieldException x2) {} // 2.4.7-
// Cannot just call .values() since that returns a copy.
Field itemsF = globalClassSet.getClass().getDeclaredField("items");
itemsF.setAccessible(true);
Object items = itemsF.get(globalClassSet);
Method iteratorM = items.getClass().getMethod("iterator");
Field klazzF = classInfoC.getDeclaredField("klazz");
klazzF.setAccessible(true);
synchronized (items) {
Iterator<?> iterator = (Iterator) iteratorM.invoke(items);
while (iterator.hasNext()) {
Object classInfo = iterator.next();
if (classInfo == null) {
LOGGER.finer("JENKINS-41945: ignoring null ClassInfo from ManagedLinkedList.Iter.next");
continue;
}
if (klazzF.get(classInfo) == clazz) {
iterator.remove();
LOGGER.log(Level.FINER, "cleaning up {0} from GlobalClassSet", clazz.getName());
}
}
}
}

private static void cleanUpObjectStreamClassCaches(@Nonnull Class<?> clazz) throws Exception {
Class<?> cachesC = Class.forName("java.io.ObjectStreamClass$Caches");
for (String cacheFName : new String[] {"localDescs", "reflectors"}) {
Field cacheF = cachesC.getDeclaredField(cacheFName);
cacheF.setAccessible(true);
ConcurrentMap<Reference<Class<?>>, ?> cache = (ConcurrentMap) cacheF.get(null);
Iterator<? extends Map.Entry<Reference<Class<?>>, ?>> iterator = cache.entrySet().iterator();
while (iterator.hasNext()) {
if (iterator.next().getKey().get() == clazz) {
iterator.remove();
LOGGER.log(Level.FINER, "cleaning up {0} from ObjectStreamClass.Caches.{1}", new Object[] {clazz.getName(), cacheFName});
break;
}
}
}
}

/**
* Runs the Groovy script, using the sandbox if so configured.
* @param loader a class loader for constructing the shell, such as {@link PluginManager#uberClassLoader} (will be augmented by {@link #getClasspath} if nonempty)
Expand All @@ -146,6 +293,7 @@ public Object evaluate(ClassLoader loader, Binding binding) throws Exception {
throw new IllegalStateException("you need to call configuring or a related method before using GroovyScript");
}
URLClassLoader urlcl = null;
ClassLoader memoryProtectedLoader = null;
List<ClasspathEntry> cp = getClasspath();
if (!cp.isEmpty()) {
List<URL> urlList = new ArrayList<URL>(cp.size());
Expand All @@ -157,25 +305,96 @@ public Object evaluate(ClassLoader loader, Binding binding) throws Exception {

loader = urlcl = new URLClassLoader(urlList.toArray(new URL[urlList.size()]), loader);
}
boolean canDoCleanup = false;

try {
loader = GroovySandbox.createSecureClassLoader(loader);

Field loaderF = null;
try {
loaderF = GroovyShell.class.getDeclaredField("loader");
loaderF.setAccessible(true);
canDoCleanup = true;
} catch (NoSuchFieldException nsme) {
LOGGER.log(Level.FINE, "GroovyShell fields have changed, field loader no longer exists -- memory leak fixes won't work");
}

GroovyShell sh;
if (sandbox) {
GroovyShell shell = new GroovyShell(loader, binding, GroovySandbox.createSecureCompilerConfiguration());
CompilerConfiguration cc = GroovySandbox.createSecureCompilerConfiguration();
sh = new GroovyShell(loader, binding, cc);

if (canDoCleanup) {
memoryProtectedLoader = new CleanGroovyClassLoader(loader, cc);
loaderF.set(sh, memoryProtectedLoader);
}

try {
return GroovySandbox.run(shell.parse(script), Whitelist.all());
return GroovySandbox.run(sh.parse(script), Whitelist.all());
} catch (RejectedAccessException x) {
throw ScriptApproval.get().accessRejected(x, ApprovalContext.create());
}
} else {
return new GroovyShell(loader, binding).evaluate(ScriptApproval.get().using(script, GroovyLanguage.get()));
sh = new GroovyShell(loader, binding);
if (canDoCleanup) {
memoryProtectedLoader = new CleanGroovyClassLoader(loader);
loaderF.set(sh, memoryProtectedLoader);
}
return sh.evaluate(ScriptApproval.get().using(script, GroovyLanguage.get()));
}

} finally {
try {
if (canDoCleanup) {
cleanUpLoader(memoryProtectedLoader, new HashSet<ClassLoader>(), new HashSet<Class<?>>());
}
} catch (Exception x) {
LOGGER.log(Level.WARNING, "failed to clean up memory " , x);
}

if (urlcl != null) {
urlcl.close();
}
}
}


/**
* Disables the weird and unreliable {@link groovy.lang.GroovyClassLoader.InnerLoader}.
* This is apparently only necessary when you are using class recompilation, which we are not.
* We want the {@linkplain Class#getClassLoader defining loader} of {@code *.groovy} to be this one.
* Otherwise the defining loader will be an {@code InnerLoader}, and not necessarily the same instance from load to load.
* @see GroovyClassLoader#getTimeStamp
*/
private static final class CleanGroovyClassLoader extends GroovyClassLoader {

CleanGroovyClassLoader(ClassLoader loader, CompilerConfiguration config) {
super(loader, config);
}

CleanGroovyClassLoader(ClassLoader loader) {
super(loader);
}

@Override protected ClassCollector createCollector(CompilationUnit unit, SourceUnit su) {
// Super implementation is what creates the InnerLoader.
return new CleanClassCollector(unit, su);
}

private final class CleanClassCollector extends ClassCollector {

CleanClassCollector(CompilationUnit unit, SourceUnit su) {
// Cannot override {@code final cl} field so have to do it this way.
super(null, unit, su);
}

@Override public GroovyClassLoader getDefiningClassLoader() {
return CleanGroovyClassLoader.this;
}

}
}

@Extension public static final class DescriptorImpl extends Descriptor<SecureGroovyScript> {

@Override public String getDisplayName() {
Expand Down
68 changes: 68 additions & 0 deletions src/test/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/GroovyMemoryLeakTest.java
View file
@@ -0,0 +1,68 @@
package org.jenkinsci.plugins.scriptsecurity.sandbox.groovy;

import groovy.lang.MetaClass;
import hudson.model.FreeStyleProject;
import org.codehaus.groovy.reflection.ClassInfo;
import org.junit.After;
import org.junit.ClassRule;
import org.junit.Rule;
import org.junit.Test;
import org.jvnet.hudson.plugins.groovypostbuild.GroovyPostbuildRecorder;
import org.jvnet.hudson.test.BuildWatcher;
import org.jvnet.hudson.test.JenkinsRule;
import org.jvnet.hudson.test.LoggerRule;
import org.jvnet.hudson.test.MemoryAssert;

import java.lang.ref.WeakReference;
import java.lang.reflect.Method;
import java.util.ArrayList;
import java.util.List;
import java.util.logging.Level;

import static org.junit.Assert.assertFalse;

/**
* Tests for memory leak cleanup successfully purging the most common memory leak.
*/
public class GroovyMemoryLeakTest {
@ClassRule
public static BuildWatcher buildWatcher = new BuildWatcher();
@Rule
public JenkinsRule r = new JenkinsRule();
@Rule public LoggerRule logger = new LoggerRule().record(SecureGroovyScript.class, Level.FINER);

@After
public void clearLoaders() {
LOADERS.clear();
}
private static final List<WeakReference<ClassLoader>> LOADERS = new ArrayList<>();

public static void register(Object o) {
ClassLoader loader = o.getClass().getClassLoader();
System.err.println("registering " + o + " from " + loader);
LOADERS.add(new WeakReference<>(loader));
}

@Test
public void loaderReleased() throws Exception {
FreeStyleProject p = r.jenkins.createProject(FreeStyleProject.class, "p");
p.addPublisher(new GroovyPostbuildRecorder(
new SecureGroovyScript(GroovyMemoryLeakTest.class.getName()+".register(this)", false, null),
2, false
));
r.buildAndAssertSuccess(p);

assertFalse(LOADERS.isEmpty());
{ // TODO it seems that the call to GroovyMemoryLeakTest.register(Object) on a Script1 parameter creates a MetaMethodIndex.Entry.cachedStaticMethod.
// In other words any call to a foundational API might leak classes. Why does Groovy need to do this?
// Unclear whether this is a problem in a realistic environment; for the moment, suppressing it so the test can run with no SoftReference.
MetaClass metaClass = ClassInfo.getClassInfo(GroovyMemoryLeakTest.class).getMetaClass();
Method clearInvocationCaches = metaClass.getClass().getDeclaredMethod("clearInvocationCaches");
clearInvocationCaches.setAccessible(true);
clearInvocationCaches.invoke(metaClass);
}
for (WeakReference<ClassLoader> loaderRef : LOADERS) {
MemoryAssert.assertGC(loaderRef, false);
}
}
}

0 comments on commit 2699d5d

Please sign in to comment.