From 547c7800e478ae69900c42d955262da5f4350a60 Mon Sep 17 00:00:00 2001 From: Ryan Ernst Date: Wed, 18 Dec 2024 15:10:29 -0800 Subject: [PATCH] Improve error message when whitelist resource file is not found (#119012) This commit replaces a NullPointerException that occurs if a whitelist resource is not found with a customized message. Additionally it augments the message with specific actions, especially in the case the owning class is modularized which requies additional work. --- .../painless/spi/WhitelistLoader.java | 32 ++++++++++- .../painless/WhitelistLoaderTests.java | 57 +++++++++++++++++++ .../bootstrap/test-framework.policy | 2 + .../org/elasticsearch/test/jar/JarUtils.java | 33 +++++++++++ 4 files changed, 121 insertions(+), 3 deletions(-) diff --git a/modules/lang-painless/spi/src/main/java/org/elasticsearch/painless/spi/WhitelistLoader.java b/modules/lang-painless/spi/src/main/java/org/elasticsearch/painless/spi/WhitelistLoader.java index 2e7f0de027de7..37bff97a07ae2 100644 --- a/modules/lang-painless/spi/src/main/java/org/elasticsearch/painless/spi/WhitelistLoader.java +++ b/modules/lang-painless/spi/src/main/java/org/elasticsearch/painless/spi/WhitelistLoader.java @@ -9,8 +9,10 @@ package org.elasticsearch.painless.spi; +import org.elasticsearch.ResourceNotFoundException; import org.elasticsearch.painless.spi.annotation.WhitelistAnnotationParser; +import java.io.InputStream; import java.io.InputStreamReader; import java.io.LineNumberReader; import java.lang.reflect.Constructor; @@ -140,7 +142,7 @@ public static Whitelist loadFromResourceFiles(Class resource, String... filep * } * } */ - public static Whitelist loadFromResourceFiles(Class resource, Map parsers, String... filepaths) { + public static Whitelist loadFromResourceFiles(Class owner, Map parsers, String... filepaths) { List whitelistClasses = new ArrayList<>(); List whitelistStatics = new ArrayList<>(); List whitelistClassBindings = new ArrayList<>(); @@ -153,7 +155,7 @@ public static Whitelist loadFromResourceFiles(Class resource, Map resource, Map) resource::getClassLoader); + ClassLoader loader = AccessController.doPrivileged((PrivilegedAction) owner::getClassLoader); return new Whitelist(loader, whitelistClasses, whitelistStatics, whitelistClassBindings, Collections.emptyList()); } + private static InputStream getResourceAsStream(Class owner, String name) { + InputStream stream = owner.getResourceAsStream(name); + if (stream == null) { + String msg = "Whitelist file [" + + owner.getPackageName().replace(".", "/") + + "/" + + name + + "] not found from owning class [" + + owner.getName() + + "]."; + if (owner.getModule().isNamed()) { + msg += " Check that the file exists and the package [" + + owner.getPackageName() + + "] is opened " + + "to module " + + WhitelistLoader.class.getModule().getName(); + } + throw new ResourceNotFoundException(msg); + } + return stream; + } + private static List parseWhitelistAnnotations(Map parsers, String line) { List annotations; diff --git a/modules/lang-painless/spi/src/test/java/org/elasticsearch/painless/WhitelistLoaderTests.java b/modules/lang-painless/spi/src/test/java/org/elasticsearch/painless/WhitelistLoaderTests.java index e62d0b438b098..b46bc118e0913 100644 --- a/modules/lang-painless/spi/src/test/java/org/elasticsearch/painless/WhitelistLoaderTests.java +++ b/modules/lang-painless/spi/src/test/java/org/elasticsearch/painless/WhitelistLoaderTests.java @@ -9,6 +9,7 @@ package org.elasticsearch.painless; +import org.elasticsearch.ResourceNotFoundException; import org.elasticsearch.painless.spi.Whitelist; import org.elasticsearch.painless.spi.WhitelistClass; import org.elasticsearch.painless.spi.WhitelistLoader; @@ -17,10 +18,18 @@ import org.elasticsearch.painless.spi.annotation.NoImportAnnotation; import org.elasticsearch.painless.spi.annotation.WhitelistAnnotationParser; import org.elasticsearch.test.ESTestCase; +import org.elasticsearch.test.compiler.InMemoryJavaCompiler; +import org.elasticsearch.test.jar.JarUtils; +import java.lang.ModuleLayer.Controller; +import java.nio.charset.StandardCharsets; +import java.nio.file.Path; import java.util.HashMap; import java.util.Map; +import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.notNullValue; + public class WhitelistLoaderTests extends ESTestCase { public void testUnknownAnnotations() { @@ -96,4 +105,52 @@ public void testAnnotations() { assertEquals(3, count); } + + public void testMissingWhitelistResource() { + var e = expectThrows(ResourceNotFoundException.class, () -> WhitelistLoader.loadFromResourceFiles(Whitelist.class, "missing.txt")); + assertThat( + e.getMessage(), + equalTo( + "Whitelist file [org/elasticsearch/painless/spi/missing.txt] not found" + + " from owning class [org.elasticsearch.painless.spi.Whitelist]." + ) + ); + } + + public void testMissingWhitelistResourceInModule() throws Exception { + Map sources = new HashMap<>(); + sources.put("module-info", "module m {}"); + sources.put("p.TestOwner", "package p; public class TestOwner { }"); + var classToBytes = InMemoryJavaCompiler.compile(sources); + + Path dir = createTempDir(getTestName()); + Path jar = dir.resolve("m.jar"); + Map jarEntries = new HashMap<>(); + jarEntries.put("module-info.class", classToBytes.get("module-info")); + jarEntries.put("p/TestOwner.class", classToBytes.get("p.TestOwner")); + jarEntries.put("p/resource.txt", "# test resource".getBytes(StandardCharsets.UTF_8)); + JarUtils.createJarWithEntries(jar, jarEntries); + + try (var loader = JarUtils.loadJar(jar)) { + Controller controller = JarUtils.loadModule(jar, loader.classloader(), "m"); + Module module = controller.layer().findModule("m").orElseThrow(); + + Class ownerClass = module.getClassLoader().loadClass("p.TestOwner"); + + // first check we get a nice error message when accessing the resource + var e = expectThrows(ResourceNotFoundException.class, () -> WhitelistLoader.loadFromResourceFiles(ownerClass, "resource.txt")); + assertThat( + e.getMessage(), + equalTo( + "Whitelist file [p/resource.txt] not found from owning class [p.TestOwner]." + + " Check that the file exists and the package [p] is opened to module null" + ) + ); + + // now check we can actually read it once the package is opened to us + controller.addOpens(module, "p", WhitelistLoader.class.getModule()); + var whitelist = WhitelistLoader.loadFromResourceFiles(ownerClass, "resource.txt"); + assertThat(whitelist, notNullValue()); + } + } } diff --git a/server/src/main/resources/org/elasticsearch/bootstrap/test-framework.policy b/server/src/main/resources/org/elasticsearch/bootstrap/test-framework.policy index 040a7a6205f9c..462fab651c211 100644 --- a/server/src/main/resources/org/elasticsearch/bootstrap/test-framework.policy +++ b/server/src/main/resources/org/elasticsearch/bootstrap/test-framework.policy @@ -88,6 +88,7 @@ grant codeBase "${codebase.elasticsearch}" { // this is the test-framework, but the jar is horribly named grant codeBase "${codebase.framework}" { permission java.lang.RuntimePermission "setSecurityManager"; + permission java.lang.RuntimePermission "createClassLoader"; }; grant codeBase "${codebase.elasticsearch-rest-client}" { @@ -129,4 +130,5 @@ grant { permission java.nio.file.LinkPermission "symbolic"; // needed for keystore tests permission java.lang.RuntimePermission "accessUserInformation"; + permission java.lang.RuntimePermission "getClassLoader"; }; diff --git a/test/framework/src/main/java/org/elasticsearch/test/jar/JarUtils.java b/test/framework/src/main/java/org/elasticsearch/test/jar/JarUtils.java index e5bdd66e949f7..0da392cb7fb01 100644 --- a/test/framework/src/main/java/org/elasticsearch/test/jar/JarUtils.java +++ b/test/framework/src/main/java/org/elasticsearch/test/jar/JarUtils.java @@ -9,13 +9,24 @@ package org.elasticsearch.test.jar; +import org.elasticsearch.test.PrivilegedOperations.ClosableURLClassLoader; + import java.io.ByteArrayInputStream; import java.io.IOException; import java.io.OutputStream; +import java.lang.module.Configuration; +import java.lang.module.ModuleFinder; +import java.net.MalformedURLException; +import java.net.URL; +import java.net.URLClassLoader; import java.nio.file.Files; import java.nio.file.Path; import java.nio.file.StandardOpenOption; +import java.security.AccessController; +import java.security.PrivilegedAction; +import java.util.List; import java.util.Map; +import java.util.Set; import java.util.jar.JarEntry; import java.util.jar.JarOutputStream; import java.util.jar.Manifest; @@ -85,6 +96,28 @@ public static void createJarWithEntriesUTF(Path jarfile, Map ent createJarWithEntries(jarfile, map); } + /** + * Creates a class loader for the given jar file. + * @param path Path to the jar file to load + * @return A URLClassLoader that will load classes from the jar. It should be closed when no longer needed. + */ + public static ClosableURLClassLoader loadJar(Path path) { + try { + URL[] urls = new URL[] { path.toUri().toURL() }; + return new ClosableURLClassLoader(URLClassLoader.newInstance(urls, JarUtils.class.getClassLoader())); + } catch (MalformedURLException e) { + throw new RuntimeException(e); + } + } + + public static ModuleLayer.Controller loadModule(Path path, ClassLoader loader, String name) { + var finder = ModuleFinder.of(path.getParent()); + var cf = Configuration.resolveAndBind(finder, List.of(ModuleLayer.boot().configuration()), ModuleFinder.of(), Set.of(name)); + return AccessController.doPrivileged( + (PrivilegedAction) () -> ModuleLayer.defineModulesWithOneLoader(cf, List.of(ModuleLayer.boot()), loader) + ); + } + @FunctionalInterface interface UncheckedIOFunction { R apply(T t) throws IOException;