Fix ServiceLoader queries using ModuleClassLoader & add related tests (#52)

The fix is just to call the package-private
`ModuleLayer.bindToLoader(ClassLoader)` method on the parent layers when
creating the `ModuleClassLoader`.

The rest of the PR is a complicated testing setup, to make sure that we
can test with both CP-loaded and SJH-loaded source sets.

Closes McModLauncher/modlauncher#100.

Author: Technici4n

build.gradle

@@ -84,6 +84,17 @@ dependencyUpdates {
     }
 }
 
+sourceSets {
+    // Additional test JARs, to be loaded via SecureJar.from(...)
+    testjar1 {}
+    testjar2 {}
+    // Test classpath code, to make sure that ModuleClassLoader is properly isolated from the classpath
+    testjar_cp {}
+    test {
+        runtimeClasspath += sourceSets.testjar_cp.output
+    }
+}
+
 // We can't use toolchains because we need --add-export
 //java.toolchain.languageVersion = JavaLanguageVersion.of(16)
 compileJava {
@@ -95,20 +106,12 @@ compileJava {
     ]
 }
 
-test {
-    //exclude '**/*'
-    useJUnitPlatform()
-    jvmArgs += [
-            '--add-opens=java.base/java.lang.invoke=ALL-UNNAMED'
-    ]
-}
-
 compileTestJava {
     sourceCompatibility = JavaVersion.VERSION_16
     targetCompatibility = JavaVersion.VERSION_16
     options.compilerArgs += [
             '--add-modules=jdk.zipfs',
-            '--add-exports=jdk.zipfs/jdk.nio.zipfs=ALL-UNNAMED'
+            '--add-exports=jdk.zipfs/jdk.nio.zipfs=cpw.mods.securejarhandler'
     ]
 }
 
@@ -126,6 +129,49 @@ dependencies {
     testRuntimeOnly('org.junit.jupiter:junit-jupiter-engine:5.8.+')
 }
 
+test {
+    //exclude '**/*'
+    useJUnitPlatform()
+    jvmArgs += [
+            // Add test sourceset to the SJH module
+            '--patch-module=cpw.mods.securejarhandler=' + sourceSets.test.output.classesDirs.asPath,
+            // SJH needs this for UnionFileSystem
+            '--add-opens=java.base/java.lang.invoke=cpw.mods.securejarhandler',
+            // Allow JUnit to access the tests
+            '--add-opens=cpw.mods.securejarhandler/cpw.mods.cl.test=ALL-UNNAMED',
+            '--add-opens=cpw.mods.securejarhandler/cpw.mods.jarhandling.impl=ALL-UNNAMED',
+            '--add-opens=cpw.mods.securejarhandler/cpw.mods.niofs.union=ALL-UNNAMED',
+            // To test reading from the classpath
+            '--add-reads=cpw.mods.securejarhandler=ALL-UNNAMED',
+    ]
+
+    dependsOn tasks.compileTestjar1Java
+    dependsOn tasks.processTestjar1Resources
+    dependsOn tasks.compileTestjar2Java
+    dependsOn tasks.processTestjar2Resources
+    environment "sjh.testjar1", sourceSets.testjar1.output.classesDirs.asPath + File.pathSeparator + sourceSets.testjar1.output.resourcesDir.absolutePath
+    environment "sjh.testjar2", sourceSets.testjar2.output.classesDirs.asPath + File.pathSeparator + sourceSets.testjar2.output.resourcesDir.absolutePath
+
+    // Add SJH and its dependencies to the module path
+    jvmArgumentProviders.add(new CommandLineArgumentProvider() {
+        @Override
+        Iterable<String> asArguments() {
+            // Dependencies
+            def modulePath = test.classpath.filter {
+                it.name.contains("asm")
+            }.join(File.pathSeparator)
+            // SJH itself
+            modulePath += File.pathSeparator + sourceSets.main.output.classesDirs.asPath
+
+            return [
+                    "--module-path",
+                    modulePath,
+                    "--add-modules=ALL-MODULE-PATH",
+            ]
+        }
+    })
+}
+
 changelog {
     from '0.9'
 }

src/main/java/cpw/mods/cl/ModuleClassLoader.java

@@ -6,14 +6,17 @@
 import java.io.IOException;
 import java.io.InputStream;
 import java.io.UncheckedIOException;
+import java.lang.invoke.MethodHandle;
+import java.lang.invoke.MethodHandles;
+import java.lang.invoke.MethodType;
 import java.lang.module.*;
 import java.net.MalformedURLException;
 import java.net.URI;
 import java.net.URL;
 import java.util.*;
 import java.util.function.BiFunction;
+import java.util.function.Consumer;
 import java.util.function.Function;
-import java.util.function.Supplier;
 import java.util.stream.Collectors;
 import java.util.stream.Stream;
 
@@ -24,6 +27,39 @@ public class ModuleClassLoader extends ClassLoader {
         ModularURLHandler.initFrom(ModuleClassLoader.class.getModule().getLayer());
     }
 
+    // Reflect into JVM internals to associate each ModuleClassLoader with all of its parent layers.
+    // This is necessary to let ServiceProvider find service implementations in parent module layers.
+    // At the moment, this does not work for providers in the bootstrap or platform class loaders,
+    // but any other provider (defined by the application class loader or child layers) should work.
+    //
+    // The only mechanism the JVM has for this is to also look for layers defined by the parent class loader.
+    // We don't want to set a parent because we explicitly do not want to delegate to a parent class loader,
+    // and that wouldn't even handle the case of multiple parent layers anyway.
+    private static final MethodHandle LAYER_BIND_TO_LOADER;
+
+    static {
+        try {
+            var hackfield = MethodHandles.Lookup.class.getDeclaredField("IMPL_LOOKUP");
+            hackfield.setAccessible(true);
+            MethodHandles.Lookup hack = (MethodHandles.Lookup) hackfield.get(null);
+
+            LAYER_BIND_TO_LOADER = hack.findSpecial(ModuleLayer.class, "bindToLoader", MethodType.methodType(void.class, ClassLoader.class), ModuleLayer.class);
+        } catch (NoSuchFieldException | IllegalAccessException | NoSuchMethodException e) {
+            throw new RuntimeException(e);
+        }
+    }
+
+    /**
+     * Invokes {@code ModuleLayer.bindToLoader(ClassLoader)}.
+     */
+    private static void bindToLayer(ModuleClassLoader classLoader, ModuleLayer layer) {
+        try {
+            LAYER_BIND_TO_LOADER.invokeExact(layer, (ClassLoader) classLoader);
+        } catch (Throwable t) {
+            throw new RuntimeException(t);
+        }
+    }
+
     private final Configuration configuration;
     private final Map<String, JarModuleFinder.JarModuleReference> resolvedRoots;
     private final Map<String, ResolvedModule> packageLookup;
@@ -78,6 +114,20 @@ public ModuleClassLoader(final String name, final Configuration configuration, f
                 }
             }
         }
+        // Bind this classloader to all parent layers recursively,
+        // to make sure ServiceLoader can find providers defined in parent layers
+        Set<ModuleLayer> visitedLayers = new HashSet<>();
+        parentLayers.forEach(p -> forLayerAndParents(p, visitedLayers, l -> bindToLayer(this, l)));
+    }
+
+    private static void forLayerAndParents(ModuleLayer layer, Set<ModuleLayer> visited, Consumer<ModuleLayer> operation) {
+        if (visited.contains(layer)) return;
+        visited.add(layer);
+        operation.accept(layer);
+
+        if (layer != ModuleLayer.boot()) {
+            layer.parents().forEach(l -> forLayerAndParents(l, visited, operation));
+        }
     }
 
     private URL readerToURL(final ModuleReader reader, final ModuleReference ref, final String name) {

src/test/java/cpw/mods/cl/test/TestClassLoader.java

@@ -14,6 +14,9 @@
 import java.util.ServiceLoader;
 import java.util.function.Consumer;
 
+import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+
 public class TestClassLoader {
     public static void main(String[] args) {
         new TestClassLoader().testCL();
@@ -41,4 +44,17 @@ void testCL() {
         var c = sl.stream().map(ServiceLoader.Provider::get).toList();
         c.get(0).accept(new String[0]);
     }
+
+    @Test
+    public void testCpIsolation() throws Exception {
+        // Make sure that classes that would normally be accessible via classpath...
+        assertDoesNotThrow(() -> Class.forName("cpw.mods.testjar_cp.SomeClass"));
+
+        // ...cannot be loaded via a ModuleClassLoader
+        TestjarUtil.withTestjar1Setup(cl -> {
+            assertThrows(ClassNotFoundException.class, () -> {
+                Class.forName("cpw.mods.testjar_cp.SomeClass", true, cl);
+            });
+        });
+    }
 }

src/test/java/cpw/mods/cl/test/TestServiceLoader.java

@@ -0,0 +1,72 @@
+package cpw.mods.cl.test;
+
+import cpw.mods.cl.ModuleClassLoader;
+import org.junit.jupiter.api.Test;
+
+import java.net.spi.URLStreamHandlerProvider;
+import java.nio.file.spi.FileSystemProvider;
+import java.util.ServiceLoader;
+
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+public class TestServiceLoader {
+    /**
+     * Tests that we can load services from modules that are part of the boot layer.
+     * In principle this also tests that services correctly get loaded from parent module layers too.
+     */
+    @Test
+    public void testLoadServiceFromBootLayer() throws Exception {
+        TestjarUtil.withTestjar1Setup(cl -> {
+            // We expect to find at least the unionfs provider
+            ServiceLoader<FileSystemProvider> sl = TestjarUtil.loadTestjar1(cl, FileSystemProvider.class);
+            boolean foundUnionFsProvider = sl.stream().map(ServiceLoader.Provider::get).anyMatch(p -> p.getScheme().equals("union"));
+
+            assertTrue(foundUnionFsProvider, "Expected to be able to find the UFS provider");
+        });
+    }
+
+    @Test
+    public void testLoadServiceFromBootLayerNested() throws Exception {
+        TestjarUtil.withTestjar2Setup(cl -> {
+            // Try to find service from boot layer
+            // We expect to find at least the unionfs provider
+            ServiceLoader<FileSystemProvider> sl = TestjarUtil.loadTestjar2(cl, FileSystemProvider.class);
+            boolean foundUnionFsProvider = sl.stream().map(ServiceLoader.Provider::get).anyMatch(p -> p.getScheme().equals("union"));
+
+            assertTrue(foundUnionFsProvider, "Expected to be able to find the UFS provider");
+
+            // Try to find service from testjar1 layer
+            var foundService = TestjarUtil.loadTestjar2(cl, URLStreamHandlerProvider.class)
+                    .stream()
+                    .anyMatch(p -> p.type().getName().startsWith("cpw.mods.cl.testjar1"));
+
+            assertTrue(foundService, "Expected to be able to find the provider in testjar1 layer");
+        });
+    }
+
+    /**
+     * Tests that services that would normally be loaded from the classpath
+     * do not get loaded by {@link ModuleClassLoader}.
+     * In other words, test that our class loader isolation also works with services.
+     */
+    @Test
+    public void testClassPathServiceDoesNotLeak() throws Exception {
+        // Test that the DummyURLStreamHandlerProvider service provider can be loaded from the classpath
+        var foundService = TestjarUtil.loadClasspath(TestServiceLoader.class.getClassLoader(), URLStreamHandlerProvider.class)
+                        .stream()
+                        .anyMatch(p -> p.type().getName().startsWith("cpw.mods.testjar_cp"));
+
+        assertTrue(foundService, "Could not find service in classpath using application class loader!");
+
+        TestjarUtil.withTestjar1Setup(cl -> {
+            // Test that the DummyURLStreamHandlerProvider service provider cannot be loaded
+            // from the classpath via ModuleClassLoader
+            var foundServiceMCL = TestjarUtil.loadTestjar1(cl, URLStreamHandlerProvider.class)
+                    .stream()
+                    .anyMatch(p -> p.type().getName().startsWith("cpw.mods.testjar_cp"));
+
+            assertFalse(foundServiceMCL, "Could find service in classpath using application class loader!");
+        });
+    }
+}

src/test/java/cpw/mods/cl/test/TestjarUtil.java

@@ -0,0 +1,110 @@
+package cpw.mods.cl.test;
+
+import cpw.mods.cl.JarModuleFinder;
+import cpw.mods.cl.ModuleClassLoader;
+import cpw.mods.jarhandling.SecureJar;
+
+import java.io.File;
+import java.lang.module.Configuration;
+import java.lang.module.ModuleFinder;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.util.List;
+import java.util.ServiceLoader;
+import java.util.stream.Stream;
+
+public class TestjarUtil {
+    private record BuiltLayer(ModuleClassLoader cl, ModuleLayer layer) {}
+
+    /**
+     * Build a layer for a {@code testjarX} source set.
+     */
+    private static BuiltLayer buildTestjarLayer(int testjar, List<ModuleLayer> parentLayers) {
+        var paths = Stream.of(System.getenv("sjh.testjar" + testjar).split(File.pathSeparator))
+                .map(Paths::get)
+                .toArray(Path[]::new);
+        var jar = SecureJar.from(paths);
+
+        var roots = List.of(jar.name());
+        var jf = JarModuleFinder.of(jar);
+        var conf = Configuration.resolveAndBind(
+                jf,
+                parentLayers.stream().map(ModuleLayer::configuration).toList(),
+                ModuleFinder.of(),
+                roots);
+        var cl = new ModuleClassLoader("testjar2-layer", conf, parentLayers);
+        var layer = ModuleLayer.defineModules(conf, parentLayers, m -> cl).layer();
+        return new BuiltLayer(cl, layer);
+    }
+
+    private static void withClassLoader(ClassLoader cl, TestCallback callback) throws Exception {
+        // Replace context classloader during the callback
+        var previousCl = Thread.currentThread().getContextClassLoader();
+        Thread.currentThread().setContextClassLoader(cl);
+
+        try {
+            callback.test(cl);
+        } finally {
+            Thread.currentThread().setContextClassLoader(previousCl);
+        }
+    }
+
+    /**
+     * Load the {@code testjar1} source set as new module into a new layer,
+     * and run the callback with the new layer's classloader.
+     */
+    public static void withTestjar1Setup(TestCallback callback) throws Exception {
+        var built = buildTestjarLayer(1, List.of(ModuleLayer.boot()));
+
+        withClassLoader(built.cl, callback);
+    }
+
+    /**
+     * Load the {@code testjar2} source set as new module into a new layer,
+     * whose parent is a layer loaded from the {@code testjar1} source set.
+     */
+    public static void withTestjar2Setup(TestCallback callback) throws Exception {
+        var built1 = buildTestjarLayer(1, List.of(ModuleLayer.boot()));
+        var built2 = buildTestjarLayer(2, List.of(built1.layer));
+
+        withClassLoader(built2.cl, callback);
+    }
+
+    @FunctionalInterface
+    public interface TestCallback {
+        void test(ClassLoader cl) throws Exception;
+    }
+
+    /**
+     * Instantiates a {@link ServiceLoader} within the testjar1 module.
+     */
+    public static <S> ServiceLoader<S> loadTestjar1(ClassLoader cl, Class<S> clazz) throws Exception {
+        // Use the `load` method from the testjar sourceset.
+        var testClass = cl.loadClass("cpw.mods.cl.testjar1.ServiceLoaderTest");
+        var loadMethod = testClass.getMethod("load", Class.class);
+        //noinspection unchecked
+        return (ServiceLoader<S>) loadMethod.invoke(null, clazz);
+    }
+
+    /**
+     * Instantiates a {@link ServiceLoader} within the testjar2 module.
+     */
+    public static <S> ServiceLoader<S> loadTestjar2(ClassLoader cl, Class<S> clazz) throws Exception {
+        // Use the `load` method from the testjar sourceset.
+        var testClass = cl.loadClass("cpw.mods.cl.testjar2.ServiceLoaderTest");
+        var loadMethod = testClass.getMethod("load", Class.class);
+        //noinspection unchecked
+        return (ServiceLoader<S>) loadMethod.invoke(null, clazz);
+    }
+
+    /**
+     * Instantiates a {@link ServiceLoader} within the classpath source set.
+     */
+    public static <S> ServiceLoader<S> loadClasspath(ClassLoader cl, Class<S> clazz) throws Exception {
+        // Use the `load` method from the testjar sourceset.
+        var testClass = cl.loadClass("cpw.mods.testjar_cp.ServiceLoaderTest");
+        var loadMethod = testClass.getMethod("load", Class.class);
+        //noinspection unchecked
+        return (ServiceLoader<S>) loadMethod.invoke(null, clazz);
+    }
+}

src/testjar1/java/cpw/mods/cl/testjar1/DummyURLStreamHandlerProvider.java

@@ -0,0 +1,14 @@
+package cpw.mods.cl.testjar1;
+
+import java.net.URLStreamHandler;
+import java.net.spi.URLStreamHandlerProvider;
+
+/**
+ * Referenced by {@code TestServiceLoader}.
+ */
+public class DummyURLStreamHandlerProvider extends URLStreamHandlerProvider {
+    @Override
+    public URLStreamHandler createURLStreamHandler(String protocol) {
+        return null;
+    }
+}