Solved Disable commodore/craftmagicnumbers 1.13 conversion

Discussion in 'Spigot Plugin Development' started by TeamBergerhealer, Jul 29, 2018.

  1. TeamBergerhealer

    Supporter

    For the first time in a couple weekends worth of work, BKCommonLib is ready for some first testing. The problem is that Spigot is trying to convert the plugin to use legacy material logic, and I want it to NOT do that. Its choking on the plugin currently:

    ScreenShot 2018-07-29 13.36.33.png
    What do I put in the plugin.yml to disable this? I already tried adding this to my plugin.yml to no effect:
    Code (Text):
    api-version: 1.13
    Come to think of it, it may also be related to at-runtime code generation. I might have to hack some stuff in to stop Spigot from interfering.

    ScreenShot 2018-07-29 13.38.23.png
     
    #1 TeamBergerhealer, Jul 29, 2018
    Last edited: Jul 29, 2018
  2. md_5

    Administrator Developer

    api-version: 1.13 does disable legacy material conversion.
    You are probably seeing something different, that is api-version: 1.13 will rewrite Material.values() to not return legacy materials (and also throw a NoSuchFieldException on .ordinal for legacy materials because the eclipse compiler some devs use will try and include them in switch statements).
     
  3. TeamBergerhealer

    Supporter

    Ah I did not know about this. Does this mean it is always processing classes through that filter? Is there any means to disable this at runtime? I'm assuming the processor can't handle classes that are generated at runtime if that's the case.
     
  4. md_5

    Administrator Developer

    From that full stack trace, looks to me like you are generating invalid classes (or ASM is bugged, which seems less likely).
     
  5. md_5

    Administrator Developer

    Also worth noting that it only applies the transformations to class files INSIDE the jar..... so unless you're runtime generating classes into your plugin jar, its probably some classes you already compiled being bad
     
  6. TeamBergerhealer

    Supporter

    Ah I see, if it can't find the file from the plugin class loader (so in the jar) it can't process it. Shame I can't see what file it's choking on, with classloaders it could be anything. It is quite possible that the generated code has references to types inside the jar its trying to load.

    Do you think it's possible to add some try-catch handling and log that class file it tried to process in the error?

    No idea what kind of code its having problems with, but I do note that all errors seem to have to do with the NMS ItemStack handle. That cant be a coincidence.

    I will do a full preload of all classes at startup and see where that goes.
     
  7. md_5

    Administrator Developer

    I'll make it throw a CFNE, dunno if that will give you more useful info because you haven't given me a test case

    https://hub.spigotmc.org/stash/proj...mits/235aa19c407f548f8f9b75394b5012dc5b861820
     
    • Friendly Friendly x 1
  8. TeamBergerhealer

    Supporter

    Oh I do note that even when errors occur, the class is loaded. Theres a trycatch in CraftMagicNumbers. I guess I can ignore the errors for now...

    let me give that a try!

    EDIT

    Actually, doesnt it now actually-actually error? I might actually be worse off now, lol. But let me inspect the errors im getting at least. Once I got something I can track down I'll try to write up a small test case. I dont think sending the entire 3.5mb blob of a jar will help you, so its up to me to minify the problem.

    @md_5
    Unfortunately what you added made no effect, because the error was thrown within a try-catch inside the one you try-catched with the ClassNotFound exception. This is because of the try-catch in CraftMagicNumbers processClass. (which only logs the plugin name, not the individual class/file)

    I still would like to see a plugin.yml option to disable this entirely. Its likely churning through all those libraries too. Considering the generated code its choking up on is a simple proxy method that only has Object types, Im suspecting its clashing with one of the classes of javaassist. Plus, I wrote all in BKCommonLib keeping in mind the legacy/non-legacy material constants, and it can safely work with both.
     
    #8 TeamBergerhealer, Jul 29, 2018
    Last edited: Jul 29, 2018
  9. TeamBergerhealer

    Supporter

    @md_5
    I byte-edited the code to remove the try-catch handler with dirtyJOE (nice tool, btw!) so it would produce a proper error. Unfortunately I was right, and its choking up on a class in javaassist.

    Code (Text):
    [15:09:47] [Server thread/ERROR]: com/bergerkiller/mountiplex/dep/javassist/ClassPool initializing BKCommonLib v1.13-v1-SNAPSHOT (Is it up to date?)
    java.lang.NoClassDefFoundError: com/bergerkiller/mountiplex/dep/javassist/ClassPool
        at java.lang.ClassLoader.defineClass1(Native Method) ~[?:1.8.0_151]
        at java.lang.ClassLoader.defineClass(Unknown Source) ~[?:1.8.0_151]
        at java.security.SecureClassLoader.defineClass(Unknown Source) ~[?:1.8.0_151]
        at org.bukkit.plugin.java.PluginClassLoader.findClass(PluginClassLoader.java:154) ~[spigot-1.13.jar:git-Spigot-69774b3-3b8f5be]
        at org.bukkit.plugin.java.JavaPluginLoader.getClassByName(JavaPluginLoader.java:193) ~[spigot-1.13.jar:git-Spigot-69774b3-3b8f5be]
        at org.bukkit.plugin.java.PluginClassLoader.findClass(PluginClassLoader.java:111) ~[spigot-1.13.jar:git-Spigot-69774b3-3b8f5be]
        at org.bukkit.plugin.java.PluginClassLoader.findClass(PluginClassLoader.java:100) ~[spigot-1.13.jar:git-Spigot-69774b3-3b8f5be]
        at java.lang.ClassLoader.loadClass(Unknown Source) ~[?:1.8.0_151]
        at java.lang.ClassLoader.loadClass(Unknown Source) ~[?:1.8.0_151]
        at com.bergerkiller.mountiplex.reflection.util.fast.GeneratedCodeInvoker.create(GeneratedCodeInvoker.java:136) ~[?:?]
        at com.bergerkiller.mountiplex.reflection.util.FastMethod$FastMethodInitProxy.init(FastMethod.java:168) ~[?:?]
        at com.bergerkiller.mountiplex.reflection.util.FastMethod$FastMethodInitProxy.invoke(FastMethod.java:186) ~[?:?]
        at com.bergerkiller.mountiplex.reflection.util.FastMethod.invoke(FastMethod.java:117) ~[?:?]
        at com.bergerkiller.mountiplex.reflection.declarations.Template$StaticMethod.invoke(Template.java:1194) ~[?:?]
        at com.bergerkiller.mountiplex.reflection.declarations.Template$StaticMethod$Converted.invoke(Template.java:1302) ~[?:?]
        at com.bergerkiller.generated.net.minecraft.server.ItemStackHandle.newInstance(ItemStackHandle.java:28) ~[?:?]
        at com.bergerkiller.reflection.net.minecraft.server.NMSItemStack.newInstance(NMSItemStack.java:17) ~[?:?]
        at com.bergerkiller.bukkit.common.utils.RecipeUtil.<clinit>(RecipeUtil.java:28) ~[?:?]
        at java.lang.Class.forName0(Native Method) ~[?:1.8.0_151]
        at java.lang.Class.forName(Unknown Source) ~[?:1.8.0_151]
        at com.bergerkiller.bukkit.common.Common.loadClasses(Common.java:307) ~[?:?]
        at com.bergerkiller.bukkit.common.internal.CommonClasses.loadCommon(CommonClasses.java:47) ~[?:?]
        at com.bergerkiller.bukkit.common.internal.CommonClasses.loadUtil(CommonClasses.java:40) ~[?:?]
        at com.bergerkiller.bukkit.common.internal.CommonClasses.init(CommonClasses.java:29) ~[?:?]
        at com.bergerkiller.bukkit.common.internal.CommonPlugin.onLoad(CommonPlugin.java:347) ~[?:?]
        at org.bukkit.craftbukkit.v1_13_R1.CraftServer.loadPlugins(CraftServer.java:319) [spigot-1.13.jar:git-Spigot-69774b3-3b8f5be]
        at net.minecraft.server.v1_13_R1.DedicatedServer.init(DedicatedServer.java:213) [spigot-1.13.jar:git-Spigot-69774b3-3b8f5be]
        at net.minecraft.server.v1_13_R1.MinecraftServer.run(MinecraftServer.java:686) [spigot-1.13.jar:git-Spigot-69774b3-3b8f5be]
        at java.lang.Thread.run(Unknown Source) [?:1.8.0_151]
    Caused by: java.lang.ClassNotFoundException: com.bergerkiller.mountiplex.dep.javassist.ClassPool
        at org.bukkit.plugin.java.PluginClassLoader.findClass(PluginClassLoader.java:130) ~[spigot-1.13.jar:git-Spigot-69774b3-3b8f5be]
        at org.bukkit.plugin.java.PluginClassLoader.findClass(PluginClassLoader.java:100) ~[spigot-1.13.jar:git-Spigot-69774b3-3b8f5be]
        at java.lang.ClassLoader.loadClass(Unknown Source) ~[?:1.8.0_151]
        at java.lang.ClassLoader.loadClass(Unknown Source) ~[?:1.8.0_151]
        ... 29 more
    Caused by: java.lang.ArrayIndexOutOfBoundsException: 129
        at org.objectweb.asm.ClassReader.readUTF(ClassReader.java:3414) ~[spigot-1.13.jar:git-Spigot-69774b3-3b8f5be]
        at org.objectweb.asm.ClassReader.readUTF(ClassReader.java:3392) ~[spigot-1.13.jar:git-Spigot-69774b3-3b8f5be]
        at org.objectweb.asm.ClassReader.readUTF8(ClassReader.java:3374) ~[spigot-1.13.jar:git-Spigot-69774b3-3b8f5be]
        at org.objectweb.asm.ClassReader.readMethod(ClassReader.java:1148) ~[spigot-1.13.jar:git-Spigot-69774b3-3b8f5be]
        at org.objectweb.asm.ClassReader.accept(ClassReader.java:679) ~[spigot-1.13.jar:git-Spigot-69774b3-3b8f5be]
        at org.objectweb.asm.ClassReader.accept(ClassReader.java:391) ~[spigot-1.13.jar:git-Spigot-69774b3-3b8f5be]
        at org.bukkit.craftbukkit.v1_13_R1.util.Commodore.convert(Commodore.java:130) ~[spigot-1.13.jar:git-Spigot-69774b3-3b8f5be]
        at org.bukkit.craftbukkit.v1_13_R1.util.CraftMagicNumbers.processClass(CraftMagicNumbers.java:215) ~[spigot-1.13.jar:git-Spigot-69774b3-3b8f5be]
        at org.bukkit.plugin.java.PluginClassLoader.findClass(PluginClassLoader.java:128) ~[spigot-1.13.jar:git-Spigot-69774b3-3b8f5be]
        at org.bukkit.plugin.java.PluginClassLoader.findClass(PluginClassLoader.java:100) ~[spigot-1.13.jar:git-Spigot-69774b3-3b8f5be]
        at java.lang.ClassLoader.loadClass(Unknown Source) ~[?:1.8.0_151]
        at java.lang.ClassLoader.loadClass(Unknown Source) ~[?:1.8.0_151]
        ... 29 more
    Maybe there can be some way to blacklist classes from being converted? I think you can expect more ASM class parsing errors like these in the future, and it shouldn't be processing these libraries in the first place!

    Ive included the Classpool and internal anonymous class files in the attachments, if it can help you.
     

    Attached Files:

    #9 TeamBergerhealer, Jul 29, 2018
    Last edited: Jul 29, 2018
  10. md_5

    Administrator Developer

    You should open a bug report with asm and/or javassist.
     
  11. TeamBergerhealer

    Supporter

    @md_5 For now its fine, it doesnt actually error thanks to the try-catch and everything is working. I'll figure out a way to hack into Commodore or the plugin class loader some other time to mask the error when it does get to a release.

    Note that the error isnt my own code, but the ASM library Spigot is using the analyze the class of javaassist. Something it shouldn't be touching in the first place, but eh. I can try posting an issue ticket with the ObjectWeb ASM project, but it'll probably take a while until any fixes occur and spigot can pull such fixes. I noticed that Spigot is already using one of the latest versions (6.2), so it's surprising.

    Are you willing to accept a PR in the future to add a plugin.yml configuration section that disables class rewriting of one or more class path patterns? Because that's something I could easily add for you.

    A simple boolean option to turn off this rewriting for my plugin would work too, but that's probably not a good idea for future support.
     
    #11 TeamBergerhealer, Jul 30, 2018
    Last edited: Jul 30, 2018
  12. md_5

    Administrator Developer

    I don't think there should be exemptions.

    It also may not be ASM, the bytecode it is getting could be invalid in the first place.
     
  13. TeamBergerhealer

    Supporter

    How can it be invalid, when it functionally works just fine, and has worked all the way since 1.11.2? I doubt javaassist devs are going to take me seriously when I say there is a bug in their bytecode that causes a third party interpreter to choke up, but doesn't cause any actual malfunction.

    I do disagree with your rule of 'no exemptions', but that's because I'm a lowly user. My needs are different from most plugins. Most of that has to do with how BKCommonLib has to interface with other plugins, both legacy and new, and to do so it needs the full Material enum information. It's just a different situation from a standard plugin. And I can't expect something so niche to be taken into account.

    Don't worry, I'll find my way to hack around these limitations like I always am forced to do.

    I think the only real argument I can make for exemptions is that its a waste of CPU time to be churning through 2MB worth of libraries for no reason. But in my testing, this didn't have too big of an effect, so I digress.

    Thanks for helping out understanding the problem though! I understand a bit better now what Spigot does for backwards support, and I'm liking it from the POV of Traincarts and the like. That will help me for sure. :)

    edit

    Looks like ObjectWeb ASM is still actively maintained! I'll post an issue ticket there the moment I have a minified test case. https://gitlab.ow2.org/asm/asm/issues?scope=all&utf8=✓&state=opened
     
    #13 TeamBergerhealer, Jul 30, 2018
    Last edited: Jul 30, 2018
  14. md_5

    Administrator Developer

    I'd avoid making statements like that until you know for sure.

    For all you know ASM is correct, and JavaAssist and OpenJDK are wrong and violating the JLS.

    EDIT: This is not as uncommon as you may think, e.g. just browsing recent ASM tickets someone tried to report a similar issue where an obfuscator was throwing invalid stuff in the constant pool - https://gitlab.ow2.org/asm/asm/issues/317838
     
    #14 md_5, Jul 30, 2018
    Last edited: Jul 30, 2018
  15. TeamBergerhealer

    Supporter

    Hmmn, fair enough. I'll keep that in mind as a possible outcome.
     
  16. md_5

    Administrator Developer

    I looked at that class, and per https://docs.oracle.com/javase/specs/jvms/se10/html/jvms-4.html#jvms-4.7.24 it looks invalid to me.

    Code (Text):
      public com.bergerkiller.mountiplex.dep.javassist.ClassPool(boolean);
        descriptor: (Z)V
        flags: ACC_PUBLIC
        Code:
          stack=2, locals=2, args_size=2
             0: aload_0
             1: aconst_null
             2: invokespecial #36                 // Method "<init>":(Lcom/bergerkiller/mountiplex/dep/javassist/ClassPool;)V
             5: iload_1
             6: ifeq          14
             9: aload_0
            10: invokevirtual #42                 // Method appendSystemPath:()Lcom/bergerkiller/mountiplex/dep/javassist/ClassPath;
            13: pop
            14: return
          LocalVariableTable:
            Start  Length  Slot  Name   Signature
                0      15     0  this   Lcom/bergerkiller/mountiplex/dep/javassist/ClassPool;
                0      15     1 useDefaultPath   Z
          LineNumberTable:
            line 177: 0
            line 178: 5
            line 179: 9
            line 180: 14
          StackMapTable: number_of_entries = 1
            frame_type = 255 /* full_frame */
              offset_delta = 14
              locals = [ class com/bergerkiller/mountiplex/dep/javassist/ClassPool, int ]
              stack = []
        MethodParameters:
          Name                           Flags
          com/bergerkiller/mountiplex/dep/javassist/NotFoundException."<init>":(Ljava/lang/String;)V
     
    Code (Text):
      #168 = Methodref          #162.#167     // com/bergerkiller/mountiplex/dep/javassist/NotFoundException."<init>":(Ljava/lang/String;)V
     
    But look at the class file:

    Seems like the class is indeed invalid.

    EDIT: Also just downloaded Javassist 3.23.1-GA and it didn't appear to have the same issue. So your javassist is either old, modified, or your relocation broke it.
     
    #16 md_5, Jul 30, 2018
    Last edited: Jul 30, 2018
    • Friendly Friendly x 1
  17. TeamBergerhealer

    Supporter

    Excellent post! Thanks for diving into this, I had to go to bed before I could look into it further :)

    The current dependency is version 3.20.0-GA so it might be somewhat old indeed. I remember not going latest because of the Java7/Java8 things, as I wanted to keep Mountiplex java7 compatible. Its now already using some code that requires java8, so theres no harm in updating. Ill let you know whether problems are solved.

    A bug in relocating is of course possible if it's writing the wrong bytecode back to alter the package paths. Seems more likely to me too, in which case I'll have to look into updating the maven plugin responsible for this also.
     
  18. TeamBergerhealer

    Supporter

    @md_5
    Good news! After updating Javassist to version 3.22.0-GA the problem was completely resolved. Version 3.23.0-GA wasnt possible because it had some incompatibility with java 8. Consider the issue resolved, thanks for helping out :)

    (it will remain a mystery what change was made between the versions that fixed this issue, however)