Compile each Groovy script in its own classloader

closes #18572

core/src/main/java/org/elasticsearch/script/CompiledScript.java

@@ -39,9 +39,9 @@ public class CompiledScript {
     public CompiledScript(ScriptService.ScriptType type, String name, String lang, Object compiled) {
         this.type = type;
         this.name = name;
-            this.lang = lang;
+        this.lang = lang;
-            this.compiled = compiled;
+        this.compiled = compiled;
-        }
+    }
 
     /**
      * Method to get the type of language.

core/src/main/java/org/elasticsearch/script/NativeScriptEngineService.java

@@ -90,11 +90,6 @@ public class NativeScriptEngineService extends AbstractComponent implements Scri
     public void close() {
     }
 
-    @Override
-    public void scriptRemoved(CompiledScript script) {
-        // Nothing to do here
-    }
-
     @Override
     public boolean isInlineScriptEnabled() {
         return true;

core/src/main/java/org/elasticsearch/script/ScriptEngineService.java

@@ -23,7 +23,6 @@ import org.elasticsearch.common.Nullable;
 import org.elasticsearch.search.lookup.SearchLookup;
 
 import java.io.Closeable;
-import java.util.List;
 import java.util.Map;
 
 /**
@@ -49,13 +48,6 @@ public interface ScriptEngineService extends Closeable {
 
     SearchScript search(CompiledScript compiledScript, SearchLookup lookup, @Nullable Map<String, Object> vars);
 
-    /**
-     * Handler method called when a script is removed from the Guava cache.
-     *
-     * The passed script may be null if it has already been garbage collected.
-     * */
-    void scriptRemoved(@Nullable CompiledScript script);
-
     /**
      * Returns <code>true</code> if this scripting engine can safely accept inline scripts by default. The default is <code>false</code>
      */

core/src/main/java/org/elasticsearch/script/ScriptService.java

@@ -507,16 +507,10 @@ public class ScriptService extends AbstractComponent implements Closeable {
     private class ScriptCacheRemovalListener implements RemovalListener<CacheKey, CompiledScript> {
         @Override
         public void onRemoval(RemovalNotification<CacheKey, CompiledScript> notification) {
-            scriptMetrics.onCacheEviction();
+            if (logger.isDebugEnabled()) {
-            for (ScriptEngineService service : scriptEngines) {
+                logger.debug("removed {} from cache, reason: {}", notification.getValue(), notification.getRemovalReason());
-                try {
-                    service.scriptRemoved(notification.getValue());
-                } catch (Exception e) {
-                    logger.warn("exception calling script removal listener for script service", e);
-                    // We don't rethrow because Guava would just catch the
-                    // exception and log it, which we have already done
-                }
             }
+            scriptMetrics.onCacheEviction();
         }
     }
 

core/src/test/java/org/elasticsearch/script/ScriptModesTests.java

@@ -257,12 +257,6 @@ public class ScriptModesTests extends ESTestCase {
 
         @Override
         public void close() {
-
-        }
-
-        @Override
-        public void scriptRemoved(@Nullable CompiledScript script) {
-
         }
     }
 }

core/src/test/java/org/elasticsearch/script/ScriptServiceTests.java

@@ -518,11 +518,6 @@ public class ScriptServiceTests extends ESTestCase {
 
         }
 
-        @Override
-        public void scriptRemoved(CompiledScript script) {
-            // Nothing to do here
-        }
-
         @Override
         public boolean isInlineScriptEnabled() {
             return true;
@@ -562,12 +557,6 @@ public class ScriptServiceTests extends ESTestCase {
 
         @Override
         public void close() {
-
-        }
-
-        @Override
-        public void scriptRemoved(CompiledScript script) {
-            // Nothing to do here
         }
     }
 

core/src/test/java/org/elasticsearch/script/ScriptSettingsTests.java

@@ -95,12 +95,6 @@ public class ScriptSettingsTests extends ESTestCase {
 
         @Override
         public void close() {
-
-        }
-
-        @Override
-        public void scriptRemoved(@Nullable CompiledScript script) {
-
         }
     }
 

core/src/test/java/org/elasticsearch/search/aggregations/metrics/AvgIT.java

@@ -456,10 +456,6 @@ public class AvgIT extends AbstractNumericTestCase {
                 }
             };
         }
-
-        @Override
-        public void scriptRemoved(CompiledScript script) {
-        }
     }
 
     /**
@@ -564,10 +560,6 @@ public class AvgIT extends AbstractNumericTestCase {
             };
         }
 
-        @Override
-        public void scriptRemoved(CompiledScript script) {
-        }
-
         @Override
         public boolean isInlineScriptEnabled() {
             return true;

core/src/test/java/org/elasticsearch/search/aggregations/metrics/SumIT.java

@@ -453,10 +453,6 @@ public class SumIT extends AbstractNumericTestCase {
             };
         }
 
-        @Override
-        public void scriptRemoved(CompiledScript script) {
-        }
-
         @Override
         public boolean isInlineScriptEnabled() {
             return true;
@@ -573,10 +569,6 @@ public class SumIT extends AbstractNumericTestCase {
             };
         }
 
-        @Override
-        public void scriptRemoved(CompiledScript script) {
-        }
-
         @Override
         public boolean isInlineScriptEnabled() {
             return true;

core/src/test/java/org/elasticsearch/search/aggregations/metrics/ValueCountIT.java

@@ -318,10 +318,6 @@ public class ValueCountIT extends ESIntegTestCase {
             };
         }
 
-        @Override
-        public void scriptRemoved(CompiledScript script) {
-        }
-
         @Override
         public boolean isInlineScriptEnabled() {
             return true;

core/src/test/java/org/elasticsearch/update/UpdateIT.java

@@ -144,10 +144,6 @@ public class UpdateIT extends ESIntegTestCase {
             throw new UnsupportedOperationException();
         }
 
-        @Override
-        public void scriptRemoved(CompiledScript script) {
-        }
-
         @Override
         public boolean isInlineScriptEnabled() {
             return true;
@@ -214,15 +210,10 @@ public class UpdateIT extends ESIntegTestCase {
             throw new UnsupportedOperationException();
         }
 
-        @Override
-        public void scriptRemoved(CompiledScript script) {
-        }
-
         @Override
         public boolean isInlineScriptEnabled() {
             return true;
         }
-
     }
 
     public static class ScriptedUpsertScriptPlugin extends Plugin implements ScriptPlugin {
@@ -285,10 +276,6 @@ public class UpdateIT extends ESIntegTestCase {
             throw new UnsupportedOperationException();
         }
 
-        @Override
-        public void scriptRemoved(CompiledScript script) {
-        }
-
         @Override
         public boolean isInlineScriptEnabled() {
             return true;
@@ -357,10 +344,6 @@ public class UpdateIT extends ESIntegTestCase {
             throw new UnsupportedOperationException();
         }
 
-        @Override
-        public void scriptRemoved(CompiledScript script) {
-        }
-
         @Override
         public boolean isInlineScriptEnabled() {
             return true;

modules/lang-expression/src/main/java/org/elasticsearch/script/expression/ExpressionScriptEngineService.java

@@ -266,11 +266,6 @@ public class ExpressionScriptEngineService extends AbstractComponent implements
     @Override
     public void close() {}
 
-    @Override
-    public void scriptRemoved(CompiledScript script) {
-        // Nothing to do
-    }
-
     @Override
     public boolean isInlineScriptEnabled() {
         return true;

modules/lang-groovy/src/main/java/org/elasticsearch/script/groovy/GroovyScriptEngineService.java

@@ -33,9 +33,11 @@ import org.codehaus.groovy.classgen.GeneratorContext;
 import org.codehaus.groovy.control.CompilationFailedException;
 import org.codehaus.groovy.control.CompilePhase;
 import org.codehaus.groovy.control.CompilerConfiguration;
+import org.codehaus.groovy.control.MultipleCompilationErrorsException;
 import org.codehaus.groovy.control.SourceUnit;
 import org.codehaus.groovy.control.customizers.CompilationCustomizer;
 import org.codehaus.groovy.control.customizers.ImportCustomizer;
+import org.codehaus.groovy.control.messages.Message;
 import org.elasticsearch.SpecialPermission;
 import org.elasticsearch.bootstrap.BootstrapInfo;
 import org.elasticsearch.common.Nullable;
@@ -49,20 +51,26 @@ import org.elasticsearch.script.ExecutableScript;
 import org.elasticsearch.script.LeafSearchScript;
 import org.elasticsearch.script.ScoreAccessor;
 import org.elasticsearch.script.ScriptEngineService;
-import org.elasticsearch.script.GeneralScriptException;
+import org.elasticsearch.script.ScriptException;
 import org.elasticsearch.script.SearchScript;
 import org.elasticsearch.search.lookup.LeafSearchLookup;
 import org.elasticsearch.search.lookup.SearchLookup;
 
 import java.io.IOException;
+import java.io.PrintWriter;
+import java.io.StringWriter;
 import java.math.BigDecimal;
 import java.nio.charset.StandardCharsets;
 import java.security.AccessControlContext;
 import java.security.AccessController;
 import java.security.PrivilegedAction;
+import java.util.ArrayList;
 import java.util.HashMap;
+import java.util.List;
 import java.util.Map;
 
+import static java.util.Collections.emptyList;
+
 /**
  * Provides the infrastructure for Groovy as a scripting language for Elasticsearch
  */
@@ -78,81 +86,40 @@ public class GroovyScriptEngineService extends AbstractComponent implements Scri
      */
     public static final String GROOVY_INDY_SETTING_NAME = "indy";
 
-    private final GroovyClassLoader loader;
+    /**
+     * Classloader used as a parent classloader for all Groovy scripts
+     */
+    private final ClassLoader loader;
 
     public GroovyScriptEngineService(Settings settings) {
         super(settings);
-
+        // Creates the classloader here in order to isolate Groovy-land code
-        ImportCustomizer imports = new ImportCustomizer();
-        imports.addStarImports("org.joda.time");
-        imports.addStaticStars("java.lang.Math");
-
-        CompilerConfiguration config = new CompilerConfiguration();
-
-        config.addCompilationCustomizers(imports);
-        // Add BigDecimal -> Double transformer
-        config.addCompilationCustomizers(new GroovyBigDecimalTransformer(CompilePhase.CONVERSION));
-
-        // always enable invokeDynamic, not the crazy softreference-based stuff
-        config.getOptimizationOptions().put(GROOVY_INDY_SETTING_NAME, true);
-
-        // Groovy class loader to isolate Groovy-land code
-        // classloader created here
         final SecurityManager sm = System.getSecurityManager();
         if (sm != null) {
             sm.checkPermission(new SpecialPermission());
         }
-        this.loader = AccessController.doPrivileged(new PrivilegedAction<GroovyClassLoader>() {
+        this.loader = AccessController.doPrivileged((PrivilegedAction<ClassLoader>) () -> {
-            @Override
+            // snapshot our context (which has permissions for classes), since the script has none
-            public GroovyClassLoader run() {
+            AccessControlContext context = AccessController.getContext();
-                // snapshot our context (which has permissions for classes), since the script has none
+            return new ClassLoader(getClass().getClassLoader()) {
-                final AccessControlContext engineContext = AccessController.getContext();
+                @Override
-                return new GroovyClassLoader(new ClassLoader(getClass().getClassLoader()) {
+                protected Class<?> loadClass(String name, boolean resolve) throws ClassNotFoundException {
-                    @Override
+                    if (sm != null) {
-                    protected Class<?> loadClass(String name, boolean resolve) throws ClassNotFoundException {
+                        try {
-                        if (sm != null) {
+                            context.checkPermission(new ClassPermission(name));
-                            try {
+                        } catch (SecurityException e) {
-                                engineContext.checkPermission(new ClassPermission(name));
+                            throw new ClassNotFoundException(name, e);
-                            } catch (SecurityException e) {
-                                throw new ClassNotFoundException(name, e);
-                            }
                         }
-                        return super.loadClass(name, resolve);
                     }
-                }, config);
+                    return super.loadClass(name, resolve);
-            }
-        });
-    }
-
-    @Override
-    public void close() {
-        loader.clearCache();
-        // close classloader here (why do we do this?)
-        SecurityManager sm = System.getSecurityManager();
-        if (sm != null) {
-            sm.checkPermission(new SpecialPermission());
-        }
-        AccessController.doPrivileged(new PrivilegedAction<Void>() {
-            @Override
-            public Void run() {
-                try {
-                    loader.close();
-                } catch (IOException e) {
-                    logger.warn("Unable to close Groovy loader", e);
                 }
-                return null;
+            };
-            }
         });
     }
 
     @Override
-    public void scriptRemoved(@Nullable CompiledScript script) {
+    public void close() throws IOException {
-        // script could be null, meaning the script has already been garbage collected
+        // Nothing to do here
-        if (script == null || NAME.equals(script.lang())) {
-            // Clear the cache, this removes old script versions from the
-            // cache to prevent running out of PermGen space
-            loader.clearCache();
-        }
     }
 
     @Override
@@ -167,30 +134,34 @@ public class GroovyScriptEngineService extends AbstractComponent implements Scri
 
     @Override
     public Object compile(String scriptName, String scriptSource, Map<String, String> params) {
-        try {
+        // Create the script class name
-            // we reuse classloader, so do a security check just in case.
+        String className = MessageDigests.toHexString(MessageDigests.sha1().digest(scriptSource.getBytes(StandardCharsets.UTF_8)));
-            SecurityManager sm = System.getSecurityManager();
+
-            if (sm != null) {
+        final SecurityManager sm = System.getSecurityManager();
-                sm.checkPermission(new SpecialPermission());
+        if (sm != null) {
-            }
+            sm.checkPermission(new SpecialPermission());
-            String fake = MessageDigests.toHexString(MessageDigests.sha1().digest(scriptSource.getBytes(StandardCharsets.UTF_8)));
-            // same logic as GroovyClassLoader.parseClass() but with a different codesource string:
-            return AccessController.doPrivileged(new PrivilegedAction<Object>() {
-                @Override
-                public Class<?> run() {
-                    GroovyCodeSource gcs = new GroovyCodeSource(scriptSource, fake, BootstrapInfo.UNTRUSTED_CODEBASE);
-                    gcs.setCachable(false);
-                    // TODO: we could be more complicated and paranoid, and move this to separate block, to
-                    // sandbox the compilation process itself better.
-                    return loader.parseClass(gcs);
-                }
-            });
-        } catch (Throwable e) {
-            if (logger.isTraceEnabled()) {
-                logger.trace("exception compiling Groovy script:", e);
-            }
-            throw new GeneralScriptException("failed to compile groovy script", e);
         }
+        return AccessController.doPrivileged((PrivilegedAction<Object>) () -> {
+            try {
+                GroovyCodeSource codeSource = new GroovyCodeSource(scriptSource, className, BootstrapInfo.UNTRUSTED_CODEBASE);
+                codeSource.setCachable(false);
+
+                CompilerConfiguration configuration = new CompilerConfiguration()
+                        .addCompilationCustomizers(new ImportCustomizer().addStarImports("org.joda.time").addStaticStars("java.lang.Math"))
+                        .addCompilationCustomizers(new GroovyBigDecimalTransformer(CompilePhase.CONVERSION));
+
+                // always enable invokeDynamic, not the crazy softreference-based stuff
+                configuration.getOptimizationOptions().put(GROOVY_INDY_SETTING_NAME, true);
+
+                GroovyClassLoader groovyClassLoader = new GroovyClassLoader(loader, configuration);
+                return groovyClassLoader.parseClass(codeSource);
+            } catch (Throwable e) {
+                if (logger.isTraceEnabled()) {
+                    logger.trace("Exception compiling Groovy script:", e);
+                }
+                throw convertToScriptException("Error compiling script " + className, scriptSource, e);
+            }
+        });
     }
 
     /**
@@ -215,7 +186,7 @@ public class GroovyScriptEngineService extends AbstractComponent implements Scri
             }
             return new GroovyScript(compiledScript, createScript(compiledScript.compiled(), allVars), this.logger);
         } catch (ReflectiveOperationException e) {
-            throw new GeneralScriptException("failed to build executable " + compiledScript, e);
+            throw convertToScriptException("Failed to build executable script", compiledScript.name(), e);
         }
     }
 
@@ -235,7 +206,7 @@ public class GroovyScriptEngineService extends AbstractComponent implements Scri
                 try {
                     scriptObject = createScript(compiledScript.compiled(), allVars);
                 } catch (ReflectiveOperationException e) {
-                    throw new GeneralScriptException("failed to build search " + compiledScript, e);
+                    throw convertToScriptException("Failed to build search script", compiledScript.name(), e);
                 }
                 return new GroovyScript(compiledScript, scriptObject, leafLookup, logger);
             }
@@ -248,6 +219,29 @@ public class GroovyScriptEngineService extends AbstractComponent implements Scri
         };
     }
 
+    /**
+     * Converts a {@link Throwable} to a {@link ScriptException}
+     */
+    private ScriptException convertToScriptException(String message, String source, Throwable cause) {
+        List<String> stack = new ArrayList<>();
+        if (cause instanceof MultipleCompilationErrorsException) {
+            @SuppressWarnings({"unchecked"})
+            List<Message> errors = (List<Message>) ((MultipleCompilationErrorsException) cause).getErrorCollector().getErrors();
+            for (Message error : errors) {
+                try (StringWriter writer = new StringWriter()) {
+                    error.write(new PrintWriter(writer));
+                    stack.add(writer.toString());
+                } catch (IOException e1) {
+                    logger.error("failed to write compilation error message to the stack", e1);
+                }
+            }
+        } else if (cause instanceof CompilationFailedException) {
+            CompilationFailedException error = (CompilationFailedException) cause;
+            stack.add(error.getMessage());
+        }
+        throw new ScriptException(message, cause, stack, source, NAME);
+    }
+
     public static final class GroovyScript implements ExecutableScript, LeafSearchScript {
 
         private final CompiledScript compiledScript;
@@ -298,17 +292,12 @@ public class GroovyScriptEngineService extends AbstractComponent implements Scri
             try {
                 // NOTE: we truncate the stack because IndyInterface has security issue (needs getClassLoader)
                 // we don't do a security check just as a tradeoff, it cannot really escalate to anything.
-                return AccessController.doPrivileged(new PrivilegedAction<Object>() {
+                return AccessController.doPrivileged((PrivilegedAction<Object>) script::run);
-                    @Override
-                    public Object run() {
-                        return script.run();
-                    }
-                });
             } catch (Throwable e) {
                 if (logger.isTraceEnabled()) {
                     logger.trace("failed to run {}", e, compiledScript);
                 }
-                throw new GeneralScriptException("failed to run " + compiledScript, e);
+                throw new ScriptException("Error evaluating " + compiledScript.name(), e, emptyList(), "", compiledScript.lang());
             }
         }
 

modules/lang-groovy/src/main/plugin-metadata/plugin-security.policy

@@ -26,8 +26,6 @@ grant {
   permission java.lang.RuntimePermission "accessDeclaredMembers";
   permission java.lang.RuntimePermission "accessClassInPackage.sun.reflect";
   permission java.lang.RuntimePermission "accessClassInPackage.jdk.internal.reflect";
-  // needed by GroovyScriptEngineService to close its classloader (why?)
-  permission java.lang.RuntimePermission "closeClassLoader";
   // Allow executing groovy scripts with codesource of /untrusted
   permission groovy.security.GroovyCodeSourcePermission "/untrusted";
 

modules/lang-groovy/src/test/java/org/elasticsearch/script/groovy/GroovySecurityTests.java

@@ -24,7 +24,7 @@ import org.apache.lucene.util.Constants;
 import org.codehaus.groovy.control.MultipleCompilationErrorsException;
 import org.elasticsearch.common.settings.Settings;
 import org.elasticsearch.script.CompiledScript;
-import org.elasticsearch.script.GeneralScriptException;
+import org.elasticsearch.script.ScriptException;
 import org.elasticsearch.script.ScriptService;
 import org.elasticsearch.test.ESTestCase;
 
@@ -151,7 +151,7 @@ public class GroovySecurityTests extends ESTestCase {
         try {
             doTest(script);
             fail("did not get expected exception");
-        } catch (GeneralScriptException expected) {
+        } catch (ScriptException expected) {
             Throwable cause = expected.getCause();
             assertNotNull(cause);
             if (exceptionClass.isAssignableFrom(cause.getClass()) == false) {

modules/lang-groovy/src/test/resources/rest-api-spec/test/lang_groovy/16_update2.yaml

@@ -51,7 +51,7 @@
 
 
   - do:
-      catch: /Unable.to.parse.*/
+      catch: /script_exception,.+Error.compiling.script.*/
       put_script:
         id: "1"
         lang: "groovy"

modules/lang-mustache/src/main/java/org/elasticsearch/script/mustache/MustacheScriptEngineService.java

@@ -133,11 +133,6 @@ public final class MustacheScriptEngineService extends AbstractComponent impleme
         // Nothing to do here
     }
 
-    @Override
-    public void scriptRemoved(CompiledScript script) {
-        // Nothing to do here
-    }
-
     // permission checked before doing crazy reflection
     static final SpecialPermission SPECIAL_PERMISSION = new SpecialPermission();
 

modules/lang-painless/src/main/java/org/elasticsearch/painless/PainlessScriptEngineService.java

@@ -202,15 +202,6 @@ public final class PainlessScriptEngineService extends AbstractComponent impleme
         };
     }
 
-    /**
-     * Action taken when a script is removed from the cache.
-     * @param script The removed script.
-     */
-    @Override
-    public void scriptRemoved(final CompiledScript script) {
-        // Nothing to do.
-    }
-
     /**
      * Action taken when the engine is closed.
      */

plugins/lang-javascript/src/main/java/org/elasticsearch/script/javascript/JavaScriptScriptEngineService.java

@@ -148,11 +148,6 @@ public class JavaScriptScriptEngineService extends AbstractComponent implements
 
     @Override
     public void close() {
-
-    }
-
-    @Override
-    public void scriptRemoved(@Nullable CompiledScript compiledScript) {
         // Nothing to do here
     }
 

plugins/lang-python/src/main/java/org/elasticsearch/script/python/PythonScriptEngineService.java

@@ -142,11 +142,6 @@ public class PythonScriptEngineService extends AbstractComponent implements Scri
         interp.cleanup();
     }
 
-    @Override
-    public void scriptRemoved(@Nullable CompiledScript compiledScript) {
-        // Nothing to do
-    }
-
     public class PythonExecutableScript implements ExecutableScript {
 
         private final PyCode code;

test/framework/src/main/java/org/elasticsearch/script/MockScriptEngine.java

@@ -110,10 +110,6 @@ public class MockScriptEngine implements ScriptEngineService {
         };
     }
 
-    @Override
-    public void scriptRemoved(@Nullable CompiledScript script) {
-    }
-
     @Override
     public void close() throws IOException {
     }