IKVM Loads classes in branches not taken

[TheLastRar]

IKVM version : image-netcoreapp3.1-win7-x64 commit a685641e85d626844b1cb3671c472f1ffd73a80a, self built

With #364 now resolved Minecraft forge modloader progresses a little further I now run into the issue I encountered when I tried this with IKVM back in 2015, except now I'm better at debugging

java.lang.reflect.InvocationTargetException
    at java.lang.reflect.Method.invoke(Method.java:486) ~[?:?]
    at net.minecraft.launchwrapper.Launch.launch(Launch.java:135) [launchwrapper-1.12.jar:?]
    at net.minecraft.launchwrapper.Launch.main(Launch.java:28) [launchwrapper-1.12.jar:?]
    at org.prismlauncher.launcher.impl.StandardLauncher.launch(StandardLauncher.java:89) [NewLaunch.jar:?]
    at org.prismlauncher.EntryPoint.listen(EntryPoint.java:128) [NewLaunch.jar:?]
    at org.prismlauncher.EntryPoint.main(EntryPoint.java:71) [NewLaunch.jar:?]
    at java.lang.reflect.Method.invoke(Method.java:486) ~[?:?]
    at IKVM.Runtime.Accessors.Java.Lang.Reflect.MethodAccessor.InvokeInvoke(Unknown Source) ~[?:?]
Caused by: java.lang.NoSuchMethodError: net.minecraft.server.MinecraftServer.func_130071_aq()J
    at net.minecraft.client.Minecraft.<init>(Minecraft.java:197) ~[bao.class:?]
    at net.minecraft.client.main.Main.main(SourceFile:129) ~[Main.class:?]
    ... 8 more

As it turns out, forge is very picky about when classes loads, as it de-obfuscates Minecraft via a custom class loader. The stack trace above is from a de-obfuscated class calling into a still obfuscated class as it was loaded before the custom class loader was setup.

This is due to IKVM loading a class (FMLCommonHandler which references a Minecraft class in a field) which OpenJDK skips until later.

Take example code below, which demonstrates the difference in class loading behaviour.

Main.java

package classloadtest;

public class Main {
    public static void main(String[] args) throws Exception {
        foo(args.length > 1);
        checkClassLoaded("classloadtest.bar");
    }

    public static void foo(boolean callBar) {
        if (callBar)
            bar.dummy();
    }

    public static void checkClassLoaded(String clazz) throws Exception {
        java.lang.reflect.Method m = ClassLoader.class.getDeclaredMethod("findLoadedClass",
                new Class[] { String.class });
        m.setAccessible(true);
        ClassLoader cl = ClassLoader.getSystemClassLoader();
        Object test1 = m.invoke(cl, clazz);
        System.out.println(test1 != null);
    }
}

bar.java

package classloadtest;

public class bar {
    public static void dummy() {}
}

OpenJDK prints false IKVM prints true

OpenJDK only loads the class when foo is called with true.

In forge's situation, this causes issues as forge's bar (FMLCommonHandler) has a singleton pattern private static final FMLCommonHandler INSTANCE = new FMLCommonHandler() and a non-static field referencing a Minecraft class. This would load said Minecraft class before forge has de-obfuscated it.

In forges case, the code that would call to FMLCommonHandler does so only in an error state to exit the program.

[wasabii]

Not exactly sure I undestand this one.

Can you explain more about this deobfuscation thing? I'd think that there would be no classloader in existance that had loaded an obfuscated class, if the point was to run it through a custom classloader?

[wasabii]

Well, I don't realy get the Minecraft thing. But looking at your sample, I see what's going on. When we emit IL for the foo method, we need to emit the call to bar.dummy(), whether it's hit or not, because we're generating the code to run it. We need the System.Type for bar in order to build the IL body for foo(). And so we have to build it. And the act of building it of course causes a call to the classloader to obtain the byte code, and at least preceeds bar through the loading step.

I don't think there's anything we can do about this.

The JVM spec makes it clear that linkage is pretty flexible:

5.4. Linking Linking a class or interface involves verifying and preparing that class or interface, its direct superclass, its direct superinterfaces, and its element type (if it is an array type), if necessary. Resolution of symbolic references in the class or interface is an optional part of linking.

This specification allows an implementation flexibility as to when linking activities (and, because of recursion, loading) take place, provided that all of the following properties are maintained:

A class or interface is completely loaded before it is linked.

A class or interface is completely verified and prepared before it is initialized.

Errors detected during linkage are thrown at a point in the program where some action is taken by the program that might, directly or indirectly, require linkage to the class or interface involved in the error.

So I think we maintain all these properties. Basically, we operate in a less lazy-mode than OpenJDK does, because we have to produce verifiable IL, and doing that requires us to have the bar type on hand, and so we must load it.

[wasabii]

The stack trace above is from a de-obfuscated class calling into a still obfuscated class as it was loaded before the custom class loader was setup.

This is what my first question was about. Why is it even possible to load a obfuscated class, if the goal is to set up a custom class loader? Why is the obfusicated code available to any class loader to load as a class at all?

[TheLastRar]

The custom classloader doesn't have deobfuscation built into it. (and it's not specific to forge, but part of Minecraft's launchwrapper) It instead, has a function to pass in classes via registerTransformer() which are to be used to modify classes on load, and can handle multiple registered transformer classes.

The code which inadvertently loads an obfuscated class on IKVM does so while setting up a transformer class to pass to the classloader. The de-obfuscation transformer has yet to be registered to the classloader, it is in fact next in line to be setup and registered.

[wasabii]

Why though is there a ClassLoader with that class besides the one with the transformer?

[TheLastRar]

Why though is there a ClassLoader with that class besides the one with the transformer?

I'm don't quite understand that question, so I'm guessing I was too brief in my explanation.

The classloader belongs to Minecraft's launchwrapper, and is set as the ContextClassLoader almost immediately. This henceforth will be referred to as LaunchClassLoader This launchwrapper gets passed a list of classes to use for setting up transformers, called tweak classes These tweak classes implement the method injectIntoClassLoader(LaunchClassLoader classLoader), which they use to setup transformers with the aforementioned LaunchClassLoader.registerTransformer() function

Forge provides a number of tweak classes Forge uses FMLPluginWrapper for loading core mods/plugins, Forge has two built in core plugins. Forge then adds de-obsfication tweak class to register its transformer.

The class at fault is one of the builtin coremods FMLCorePlugin, which commands FMLPluginWrapper to call into FMLSanityChecker FMLSanityChecker is a class that is intended to verify the version of Minecraft it is loading into is correct, and contains the following code

        if (!goodMC)
        {
            FMLRelaunchLog.severe("The minecraft jar %s appears to be corrupt! There has been CRITICAL TAMPERING WITH MINECRAFT, it is highly unlikely minecraft will work! STOP NOW, get a clean copy and try again!",codeSource.getLocation().getFile());
            if (!Boolean.parseBoolean(System.getProperty("fml.ignoreInvalidMinecraftCertificates","false")))
            {
                ...
                FMLCommonHandler.instance().exitJava(1, false);
            }
            else
            {
                ...
            }
        }

FMLCommonHandler is a singleton class that has a field whos type is one belonging to minecraft and would need to be de-obfuscated.

When FMLSanityChecker is ran, the transformer for de-obsfication isn't registered, so when IKVM triggers a load of FMLCommonHandler and thus the class of its fields, LaunchClassLoader won't know to de-obsficate any minecraft classes and thus load it in it's obfuscated form.

[wasabii]

So, they're not actually using two class loaders, but instead like, setting the transformers on LaunchClassLoader, and like... rewriting the byte code during defineClass by looping over the transformers? So, LaunchClassLoader can invoke defineClass with no transformers registered. And then invoke it again for the same class WITH tranformers registered?

Essentially the behavior of the class loader is expected to change after it's set as the context class loader. That's bad design. It should be frozen at that moment.

I'm going to have to consider this a bug on their part then. They are relying on an internal implementation detail of the OpenJDK class loader which isn't guarenteed by the spec. And one I don't think we can do anything about. Short of making every method call dynamic anyways, which we aren't going to do.

There isn't any guarentee about when in the usage of a method loading will occur for any unresolved symbols that may be used by that method. OpenJDK seems to favor lazy: it doesn't load or verify the branch until the instruction is first encountered.

My guess is what's happening here is the method is first running interpreted, and isn't considered a candidate for JIT in Hotspot. And thus the method is functionally lazy for it's entire life. We have a number of problems duplicating that behavior in .NET: there is no such thing as intepretation, and we need the type information to generate the IL.

[TheLastRar]

So, LaunchClassLoader can invoke defineClass with no transformers registered. And then invoke it again for the same class WITH tranformers registered?

No, it seems that it only loads classes once. In OpenJDK, FMLCommonHandler wouldn't get loaded due to OpenJDK's lazy loading, which would mean that it and nmy class referenced in its fields stay unloaded until sometime after the de-obsfication transformer has been registered. Other than that your summery is correct

The requirement of complete type information, however, does seem to prevent IKVM from supporting this

[wasabii]

It should only load classes once. That's stated in the spec. Chapter 5. A well behaved class loader should always return the same class for each invocation of the same class name.

What they're doing I think is altering how they expect the losing to work after the class loader is available to load. And just lucking out that it was never asked to load something before they alter it's behavior.

[wasabii]

An interesting test would be to disable the interpreter in your JVM and see if the first call to foo() results in it loading bar too early. I suspect a JIT would hit the issue, since the JIT is likely to emit all branches, and will need to know information about the bar type.

[wasabii]

Try -XX:CompileThreshold=0. Not sure if that's the right way to do it.

[TheLastRar]

Try -XX:CompileThreshold=0. Not sure if that's the right way to do it.

With this command line argument, my test case still reports false

Edit Minecraft forge also still loads without issue

[wasabii]

Hmm. Bummer. Would probably need to get deeper into Hotspot to figure it out then.

[TheLastRar]

I took another look over this, and it seems the class loading is more nuanced here than I thought

Creating a test case that more closely mimics what forge is doing;

ClassToNotLoad .java

package classloadtest;

public class ClassToNotLoad {
}

bar.java

package classloadtest;

public class bar {
    private ClassToNotLoad obj;

    private static bar instance = new bar();

    public static bar getInstance()
    {
        return instance;
    }

    public void dummy() {}
}

Ifoo.java

package classloadtest;

public interface Ifoo {
    public void hello();
    public void foo(boolean b);
}

foo.java

package classloadtest;

public class foo implements Ifoo {

    @Override
    public void hello() {
        System.out.println("Hello World");
    }

    @Override
    public void foo(boolean callBar) {
        if (callBar)
            bar.getInstance().dummy();
    }
}

Main.java

package classloadtest;

public class Main {
    public static void main(String[] args) throws Exception {
        System.out.println("Startup");
        checkClassLoaded("classloadtest.bar");
        checkClassLoaded("classloadtest.ClassToNotLoad");
        //In Forge both is false;
        System.out.println("Load class foo");
        Ifoo obj = (Ifoo) Class.forName("classloadtest.foo", true, ClassLoader.getSystemClassLoader()).newInstance();
        System.out.println("class foo loaded");

        checkClassLoaded("classloadtest.bar");
        checkClassLoaded("classloadtest.ClassToNotLoad");

        System.out.println("Calling foo.hello");
        obj.hello();
        checkClassLoaded("classloadtest.bar");
        checkClassLoaded("classloadtest.ClassToNotLoad");

        System.out.println("Calling foo.foo");
        obj.foo(false);
        checkClassLoaded("classloadtest.bar");
        checkClassLoaded("classloadtest.ClassToNotLoad");
    }

    public static void checkClassLoaded(String clazz) throws Exception {
        java.lang.reflect.Method m = ClassLoader.class.getDeclaredMethod("findLoadedClass",
                new Class[] { String.class });
        m.setAccessible(true);
        ClassLoader cl = ClassLoader.getSystemClassLoader();
        Object test1 = m.invoke(cl, clazz);
        System.out.println("is " + clazz + " loaded: " + (test1 != null));
    }
}

IKVM logs the following

Startup
is classloadtest.bar loaded: false
is classloadtest.ClassToNotLoad loaded: false
Load class foo
class foo loaded
is classloadtest.bar loaded: true
is classloadtest.ClassToNotLoad loaded: false
Calling foo.hello
Hello World
is classloadtest.bar loaded: true
is classloadtest.ClassToNotLoad loaded: false
Calling foo.foo
is classloadtest.bar loaded: true
is classloadtest.ClassToNotLoad loaded: true

I see that while bar is loaded with foo, bar's dependent classes aren't loaded until we enter foo.foo() Forge just requires that its equivalent to ClassToNotLoad remains unloaded,

Probably doesn't change anything of note, but I figured I'd at least mention this observation.

[wasabii]

Likely do to our ability to to use AppDomain.TypeResolve to defer finishing of certain types.

The idea here is that when we build the IL for foo, we can build it against the TypeBuilder for bar. But, that TypeBuilder doesn't need to actually be populated. So while we need to load the bar class data in order to define the bar TypeBuilder (need name, visibility, base class, top level type stuff), we don't have to actually Finish the type at that point. Finish is where we write out the fields and methods, and then generate the final Type. So, .NET can create the foo type on the dynamic assembly, and has to load foo and bar. To do it. But doesn't have to do anything with ClassToNotLoad until we need a real type with the actual field.

And when .NET needs to do that, it will fire off AppDomain.TypeResolve. We hook into AppDomain.TypeResolve, and finish the type then, which would cause the loading of ClassToNotLoad.

So yeah, we try to defer as much as possible. But, not what this code relies on.

I am in constant fear .NET Core will remove AppDomain.TypeResolve. It's not a thing many people know about, and it's never been moved off of the AppDomain class.

[wasabii]

Going to go ahead and close this one as not a bug. That said, it is something I'd be willing to work around if some idea occurs to somebody, where said idea doesn't cause more problems than it solves.