search

PaperMC / Paper

The most widely used, high performance Minecraft server that aims to fix gameplay and mechanics inconsistencies
https://papermc.io/
Other
9.44k stars 2.21k forks source link

Bug in the ASM EventExecutor bytecode generator #7311

Closed Jannyboy11 closed 2 years ago

Jannyboy11 commented 2 years ago

Stack trace

java.lang.VerifyError: Bad type on operand stack
Exception Details:
  Location:
    com/destroystokyo/paper/event/executor/asm/generated/GeneratedEventExecutor1.execute(Lorg/bukkit/event/Listener;Lorg/bukkit/event/Event;)V @11: pop
  Reason:
    Type long_2nd (current frame, stack[1]) is not assignable to category1 type
  Current Frame:
    bci: @11
    flags: { }
    locals: { 'com/destroystokyo/paper/event/executor/asm/generated/GeneratedEventExecutor1', 'org/bukkit/event/Listener', 'org/bukkit/event/Event' }
    stack: { long, long_2nd }
  Bytecode:
    0000000: 2bc0 000e 2cc0 0010 b600 1457 b1       

    at java.lang.Class.getDeclaredConstructors0(Native Method) ~[?:?]
    at java.lang.Class.privateGetDeclaredConstructors(Class.java:3373) ~[?:?]
    at java.lang.Class.getConstructor0(Class.java:3578) ~[?:?]
    at java.lang.Class.newInstance(Class.java:626) ~[?:?]
    at org.bukkit.plugin.EventExecutor.create(EventExecutor.java:65) ~[paper-api-1.18.1-R0.1-SNAPSHOT.jar:?]
    at org.bukkit.plugin.java.JavaPluginLoader.createRegisteredListeners(JavaPluginLoader.java:336) ~[paper-api-1.18.1-R0.1-SNAPSHOT.jar:?]
    at org.bukkit.plugin.SimplePluginManager.registerEvents(SimplePluginManager.java:660) ~[paper-api-1.18.1-R0.1-SNAPSHOT.jar:?]
    at com.janboerman.repro.invalidgeneratedeventexecutor.Demo.onEnable(Demo.java:9) ~[InvalidGeneratedEventExecutor-1.0-SNAPSHOT.jar:?]
    at org.bukkit.plugin.java.JavaPlugin.setEnabled(JavaPlugin.java:264) ~[paper-api-1.18.1-R0.1-SNAPSHOT.jar:?]
    at org.bukkit.plugin.java.JavaPluginLoader.enablePlugin(JavaPluginLoader.java:370) ~[paper-api-1.18.1-R0.1-SNAPSHOT.jar:?]
    at org.bukkit.plugin.SimplePluginManager.enablePlugin(SimplePluginManager.java:500) ~[paper-api-1.18.1-R0.1-SNAPSHOT.jar:?]
    at org.bukkit.craftbukkit.v1_18_R1.CraftServer.enablePlugin(CraftServer.java:564) ~[paper-1.18.1.jar:git-Paper-"87d8ef9"]
    at org.bukkit.craftbukkit.v1_18_R1.CraftServer.enablePlugins(CraftServer.java:478) ~[paper-1.18.1.jar:git-Paper-"87d8ef9"]
    at net.minecraft.server.MinecraftServer.loadWorld0(MinecraftServer.java:727) ~[paper-1.18.1.jar:git-Paper-"87d8ef9"]
    at net.minecraft.server.MinecraftServer.loadLevel(MinecraftServer.java:503) ~[paper-1.18.1.jar:git-Paper-"87d8ef9"]
    at net.minecraft.server.dedicated.DedicatedServer.initServer(DedicatedServer.java:313) ~[paper-1.18.1.jar:git-Paper-"87d8ef9"]
    at net.minecraft.server.MinecraftServer.runServer(MinecraftServer.java:1202) ~[paper-1.18.1.jar:git-Paper-"87d8ef9"]
    at net.minecraft.server.MinecraftServer.lambda$spin$0(MinecraftServer.java:317) ~[paper-1.18.1.jar:git-Paper-"87d8ef9"]
    at java.lang.Thread.run(Thread.java:833) ~[?:?]

Plugin and Datapack List

just my repro plugin: https://github.com/Jannyboy11/GeneratedEventExecutorBug

Actions to reproduce (if known)

Install a plugin that registers a Listener with a method annotated with @EventHandler that returns either a double or long.

Paper version

This server is running Paper version git-Paper-"87d8ef9" (MC: 1.18.1) (Implementing API version 1.18.1-R0.1-SNAPSHOT) (Git: 87d8ef9) You are running the latest version

Other

The JVM spec specifies that long and double (unboxed) are so called 'category 2 types' which occupy twice as much space on the operand stack. See: https://docs.oracle.com/javase/specs/jvms/se17/html/jvms-2.html#jvms-2.11.1 Paper's EventExecutor generator however only check whether the return type of the event handler is not void, and generates a POP instruction in that case. However, if the type is long "J" or double "D", then a POP2 instruction should be generated instead. (https://github.com/PaperMC/Paper/blob/master/patches/api/0026-Use-ASM-for-event-executors.patch#L156)

This bug does not occur on CraftBukkit/Spigot since they simply use reflection to invoke the method.

I can see two ways to fix this issue:

  1. Generate a pop2 instruction if the return type of the event handler is double or long.
  2. Require that event handlers always return void (and don't generate the pop instruction at all) and throw an exeption otherwise, because eventhandlers with a return value are nonsensical anyway.

What's more: ASM's [Type](https://asm.ow2.io/javadoc/org/objectweb/asm/Type.html#getSize()) api has a method which returns the size on the operand stack of a type.

Jannyboy11 commented 2 years ago

Additionally, if this bytecode stuff is going to be patched anyway, it is possible to use a hidden class for the generated EventExecutor, instead of a per-class classloader, which should simplify things by a lot. (Hidden classes got introduced in Java 15 so they weren't available to use in the past, but since we're on JDK17 anyway now, you might as well use them)

electronicboy commented 2 years ago

This is the first time we've seen such an issue in a long time, so I don't think that this is really an issue, I feel that we're far too downstream and far too many years in for setting such requirements

I also don't see what hidden classes solve for our use case given that we still need to retain them

Jannyboy11 commented 2 years ago

This is the first time we've seen such an issue in a long time

That is because nobody tried to return a double or long from their event handler yet, or nobody bothered to report on it before me. That does not mean that the bug does not exist.

so I don't think that this is really an issue

How is a VerifyError not an issue? if non-void return types are allowed, then this should just work for long and double as well - they are first-class citizens just like all the other types.

I also don't see what hidden classes solve for our use case given that we still need to retain them

I think it removes the need for the atomic integer that increments every time a new event executor is generated. There is no possibility of colliding class names, because classloaders cannot even find hidden classes in the first place. You can still keep Class<? extends EventExecutor> references in a collection in order to retain them manually, but the gc will keep them alive anyway as long as instances of the generated class are alive.

electronicboy commented 2 years ago

I more mean, I don't think it's an issue that the thing doesn't accept doubles as a return type, given that we've not seen this in a long time, and the fact that those methods should be returning void anyways as per literally every piece of documentation out there.

The issue here though is deciding what the real bug is, the notation of accepting anything bar void here is really just an annoyance factor given that it violates the typical expectations here, but, bukkit for years has never given single care to validate that the return type is a void, so, I guess we potentially need to err towards accepting such violations (but, do we commit to this? Noting that if we stand up now and patch this, we are basically committing to allow people to return whatever the heck they want in those methods)

only real pro I can see of the hidden classes would be if we started producing more useful event handler names, which, maybe; this in and of itself really seems to offer no gains here otherwise

Jannyboy11 commented 2 years ago

as per literally every piece of documentation out there

I disagree. Bukkit's original plugin development wiki didn't specify this. It's not specified at SpigotMC's wiki either, and it's not in the Javadocs of EventHandler, Listener or Event. All that is documented is that the parameter type must be a subtype of Event, but nowhere it is specified that the return type must be void (void is only used in the examples).

If Paper wants to support Spigot's api, then non-void return types should be supported, as Spigot supports this too currently. You can restrict it to void-only once the hard-fork happens.

electronicboy commented 2 years ago

supports and, we didn't expect nor care if people did this are two different aspects, and as I said, we should probably eer towards that

The point at which every single piece of documentation shows a void return type and the return types are meaningless in this context is where I stand on it not being officially supported, but, this is where you mostly start getting into pointless debates and semantics.

This is not a priority for us as far as I know, PRs welcome

zml2008 commented 2 years ago

hihi

  1. I implemented the event api, and tbh never considered someone doing non-void return types because why would you ever want to? if I had, non-voud return types would be explicitly forbidden. just don't be dumb and you'll be fine.

  2. there's a hidden classes reference (from SpongeAPI) pinned in #hydra, the team's known about the possibility for ages, but it's not something important enough for anyone to actually spend time on.

Jannyboy11 commented 2 years ago
  1. I implemented the event api, and tbh never considered someone doing non-void return types

The code seems to suggest otherwise, because a pop instruction is generated if the return type of the event handler is not void. (there is an explicit check for m.getReturnType() != void.class.)

This is not a priority for us as far as I know, PRs welcome

I'll create a PR that adresses the VerifyError when I have time by the end of the week.

If you guys let me, I'll create a separate PR to generate hidden classes for generated EventExecutors (it'll be a first time for me as well, I need to do some deep diving first)

electronicboy commented 2 years ago

zml is talking about the Event API inside of bukkit itself, not the ASM stuff which was added on years later and from my recollection that pop was only added after it was found out a plugin was for some reason doing this

Jannyboy11 commented 2 years ago

PR: https://github.com/PaperMC/Paper/pull/7317