From 2dbdaa6b88621209579671d6923accb36dca5b94 Mon Sep 17 00:00:00 2001 From: SoSeDiK Date: Sun, 17 Sep 2023 01:54:33 +0300 Subject: [PATCH] Cleanup old async commands patch (#8895) --- .../server/Add-PlayerKickEvent-causes.patch | 4 +- ...-debug-information-to-chat-packet-er.patch | 2 +- .../Ensure-commands-are-not-ran-async.patch | 170 ------------------ .../Improve-Player-chat-API-handling.patch | 80 +++++++++ .../server/Improved-Watchdog-Support.patch | 2 - .../Kick-on-main-for-illegal-chat.patch | 2 +- ...le-async-calls-to-restart-the-server.patch | 2 +- 7 files changed, 85 insertions(+), 177 deletions(-) delete mode 100644 patches/server/Ensure-commands-are-not-ran-async.patch create mode 100644 patches/server/Improve-Player-chat-API-handling.patch diff --git a/patches/server/Add-PlayerKickEvent-causes.patch b/patches/server/Add-PlayerKickEvent-causes.patch index 077086e6ce..c40118aaca 100644 --- a/patches/server/Add-PlayerKickEvent-causes.patch +++ b/patches/server/Add-PlayerKickEvent-causes.patch @@ -312,7 +312,7 @@ index 0000000000000000000000000000000000000000..00000000000000000000000000000000 if (!this.updateChatOrder(timestamp)) { ServerGamePacketListenerImpl.LOGGER.warn("{} sent out-of-order chat: '{}'", this.player.getName().getString(), message); - this.disconnect(Component.translatable("multiplayer.disconnect.out_of_order_chat")); -+ this.disconnect(Component.translatable("multiplayer.disconnect.out_of_order_chat"), org.bukkit.event.player.PlayerKickEvent.Cause.OUT_OF_ORDER_CHAT); // Paper - kick event ca ++ this.disconnect(Component.translatable("multiplayer.disconnect.out_of_order_chat"), org.bukkit.event.player.PlayerKickEvent.Cause.OUT_OF_ORDER_CHAT); // Paper - kick event causes return Optional.empty(); } else { Optional optional = this.unpackAndApplyLastSeen(acknowledgment); @@ -522,7 +522,7 @@ index 0000000000000000000000000000000000000000..00000000000000000000000000000000 @@ -0,0 +0,0 @@ public class CraftPlayer extends CraftHumanEntity implements Player { - // Paper start - improve chat handling + // Paper start - Improve chat handling if (ServerGamePacketListenerImpl.isChatMessageIllegal(msg)) { - this.getHandle().connection.disconnect(Component.translatable("multiplayer.disconnect.illegal_characters")); + this.getHandle().connection.disconnect(Component.translatable("multiplayer.disconnect.illegal_characters"), org.bukkit.event.player.PlayerKickEvent.Cause.ILLEGAL_CHARACTERS); // Paper - kick event causes diff --git a/patches/server/Add-some-minimal-debug-information-to-chat-packet-er.patch b/patches/server/Add-some-minimal-debug-information-to-chat-packet-er.patch index e3de2081cf..586111a963 100644 --- a/patches/server/Add-some-minimal-debug-information-to-chat-packet-er.patch +++ b/patches/server/Add-some-minimal-debug-information-to-chat-packet-er.patch @@ -16,5 +16,5 @@ index 0000000000000000000000000000000000000000..00000000000000000000000000000000 - ServerGamePacketListenerImpl.LOGGER.warn("{} sent out-of-order chat: '{}'", this.player.getName().getString(), message); + ServerGamePacketListenerImpl.LOGGER.warn("{} sent out-of-order chat: '{}': {} > {}", this.player.getName().getString(), message, this.lastChatTimeStamp.get().getEpochSecond(), timestamp.getEpochSecond()); // Paper this.server.scheduleOnMain(() -> { // Paper - push to main - this.disconnect(Component.translatable("multiplayer.disconnect.out_of_order_chat"), org.bukkit.event.player.PlayerKickEvent.Cause.OUT_OF_ORDER_CHAT); // Paper - kick event ca + this.disconnect(Component.translatable("multiplayer.disconnect.out_of_order_chat"), org.bukkit.event.player.PlayerKickEvent.Cause.OUT_OF_ORDER_CHAT); // Paper - kick event causes }); // Paper - push to main diff --git a/patches/server/Ensure-commands-are-not-ran-async.patch b/patches/server/Ensure-commands-are-not-ran-async.patch deleted file mode 100644 index b2ed462c30..0000000000 --- a/patches/server/Ensure-commands-are-not-ran-async.patch +++ /dev/null @@ -1,170 +0,0 @@ -From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 -From: Aikar -Date: Thu, 3 Mar 2016 01:17:12 -0600 -Subject: [PATCH] Ensure commands are not ran async - -Plugins calling Player.chat("/foo") or Server.dispatchCommand() could -trigger the server to execute a command while on another thread. - -These commands would then process EXPECTING to be on the main thread, leaving to -very hard to trace concurrency issues. - -This change will synchronize the command execution back to the main thread, causing a -big slowdown in execution but throwing an exception at same time to raise awareness -that it is happening so that plugin authors can fix their code to stop executing commands async. - -This also properly splits up the chat and command handling to reflect the server now -having separate packets for both, and the client always using the correct packet. Text -from a chat packet should never be parsed into a command, even if it starts with the `/` -character. - -Co-authored-by: Jake Potrebic - -diff --git a/src/main/java/net/minecraft/server/network/ServerGamePacketListenerImpl.java b/src/main/java/net/minecraft/server/network/ServerGamePacketListenerImpl.java -index 0000000000000000000000000000000000000000..0000000000000000000000000000000000000000 100644 ---- a/src/main/java/net/minecraft/server/network/ServerGamePacketListenerImpl.java -+++ b/src/main/java/net/minecraft/server/network/ServerGamePacketListenerImpl.java -@@ -0,0 +0,0 @@ public class ServerGamePacketListenerImpl implements ServerPlayerConnection, Tic - return true; - } - -- private static boolean isChatMessageIllegal(String message) { -+ public static boolean isChatMessageIllegal(String message) { // Paper - private -> public - for (int i = 0; i < message.length(); ++i) { - if (!SharedConstants.isAllowedChatCharacter(message.charAt(i))) { - return true; -@@ -0,0 +0,0 @@ public class ServerGamePacketListenerImpl implements ServerPlayerConnection, Tic - } - OutgoingChatMessage outgoing = OutgoingChatMessage.create(original); - -- if (!async && s.startsWith("/")) { -+ if (false && !async && s.startsWith("/")) { // Paper - don't handle commands in chat logic - this.handleCommand(s); - } else if (this.player.getChatVisibility() == ChatVisiblity.SYSTEM) { - // Do nothing, this is coming from a plugin -@@ -0,0 +0,0 @@ public class ServerGamePacketListenerImpl implements ServerPlayerConnection, Tic - } - } - -- private void handleCommand(String s) { -+ public void handleCommand(String s) { // Paper - private -> public -+ // Paper Start -+ if (!org.spigotmc.AsyncCatcher.shuttingDown && !org.bukkit.Bukkit.isPrimaryThread()) { -+ LOGGER.error("Command Dispatched Async: " + s); -+ LOGGER.error("Please notify author of plugin causing this execution to fix this bug! see: http://bit.ly/1oSiM6C", new Throwable()); -+ Waitable wait = new Waitable<>() { -+ @Override -+ protected Void evaluate() { -+ ServerGamePacketListenerImpl.this.handleCommand(s); -+ return null; -+ } -+ }; -+ server.processQueue.add(wait); -+ try { -+ wait.get(); -+ return; -+ } catch (InterruptedException e) { -+ Thread.currentThread().interrupt(); // This is proper habit for java. If we aren't handling it, pass it on! -+ } catch (Exception e) { -+ throw new RuntimeException("Exception processing chat command", e.getCause()); -+ } -+ } -+ // Paper End - co.aikar.timings.MinecraftTimings.playerCommandTimer.startTiming(); // Paper - if ( org.spigotmc.SpigotConfig.logCommands ) // Spigot - this.LOGGER.info(this.player.getScoreboardName() + " issued server command: " + s); -diff --git a/src/main/java/org/bukkit/craftbukkit/CraftServer.java b/src/main/java/org/bukkit/craftbukkit/CraftServer.java -index 0000000000000000000000000000000000000000..0000000000000000000000000000000000000000 100644 ---- a/src/main/java/org/bukkit/craftbukkit/CraftServer.java -+++ b/src/main/java/org/bukkit/craftbukkit/CraftServer.java -@@ -0,0 +0,0 @@ public final class CraftServer implements Server { - Preconditions.checkArgument(commandLine != null, "commandLine cannot be null"); - org.spigotmc.AsyncCatcher.catchOp("command dispatch"); // Spigot - -+ // Paper Start -+ if (!org.spigotmc.AsyncCatcher.shuttingDown && !Bukkit.isPrimaryThread()) { -+ final CommandSender fSender = sender; -+ final String fCommandLine = commandLine; -+ Bukkit.getLogger().log(Level.SEVERE, "Command Dispatched Async: " + commandLine); -+ Bukkit.getLogger().log(Level.SEVERE, "Please notify author of plugin causing this execution to fix this bug! see: http://bit.ly/1oSiM6C", new Throwable()); -+ org.bukkit.craftbukkit.util.Waitable wait = new org.bukkit.craftbukkit.util.Waitable() { -+ @Override -+ protected Boolean evaluate() { -+ return dispatchCommand(fSender, fCommandLine); -+ } -+ }; -+ net.minecraft.server.MinecraftServer.getServer().processQueue.add(wait); -+ try { -+ return wait.get(); -+ } catch (InterruptedException e) { -+ Thread.currentThread().interrupt(); // This is proper habit for java. If we aren't handling it, pass it on! -+ } catch (Exception e) { -+ throw new RuntimeException("Exception processing dispatch command", e.getCause()); -+ } -+ } -+ // Paper End - if (this.commandMap.dispatch(sender, commandLine)) { - return true; - } -diff --git a/src/main/java/org/bukkit/craftbukkit/entity/CraftPlayer.java b/src/main/java/org/bukkit/craftbukkit/entity/CraftPlayer.java -index 0000000000000000000000000000000000000000..0000000000000000000000000000000000000000 100644 ---- a/src/main/java/org/bukkit/craftbukkit/entity/CraftPlayer.java -+++ b/src/main/java/org/bukkit/craftbukkit/entity/CraftPlayer.java -@@ -0,0 +0,0 @@ public class CraftPlayer extends CraftHumanEntity implements Player { - - if (this.getHandle().connection == null) return; - -- this.getHandle().connection.chat(msg, PlayerChatMessage.system(msg), false); -+ // Paper start - improve chat handling -+ if (ServerGamePacketListenerImpl.isChatMessageIllegal(msg)) { -+ this.getHandle().connection.disconnect(Component.translatable("multiplayer.disconnect.illegal_characters")); -+ } else { -+ if (msg.startsWith("/")) { -+ this.getHandle().connection.handleCommand(msg); -+ } else { -+ final PlayerChatMessage playerChatMessage = PlayerChatMessage.system(msg).withResult(new net.minecraft.network.chat.ChatDecorator.ModernResult(Component.literal(msg), true, false)); -+ // TODO chat decorating -+ // TODO text filtering -+ this.getHandle().connection.chat(msg, playerChatMessage, false); -+ } -+ } -+ // Paper end - } - - @Override -diff --git a/src/main/java/org/bukkit/craftbukkit/util/ServerShutdownThread.java b/src/main/java/org/bukkit/craftbukkit/util/ServerShutdownThread.java -index 0000000000000000000000000000000000000000..0000000000000000000000000000000000000000 100644 ---- a/src/main/java/org/bukkit/craftbukkit/util/ServerShutdownThread.java -+++ b/src/main/java/org/bukkit/craftbukkit/util/ServerShutdownThread.java -@@ -0,0 +0,0 @@ public class ServerShutdownThread extends Thread { - public void run() { - try { - org.spigotmc.AsyncCatcher.enabled = false; // Spigot -+ org.spigotmc.AsyncCatcher.shuttingDown = true; // Paper - this.server.close(); - } finally { - try { -diff --git a/src/main/java/org/spigotmc/AsyncCatcher.java b/src/main/java/org/spigotmc/AsyncCatcher.java -index 0000000000000000000000000000000000000000..0000000000000000000000000000000000000000 100644 ---- a/src/main/java/org/spigotmc/AsyncCatcher.java -+++ b/src/main/java/org/spigotmc/AsyncCatcher.java -@@ -0,0 +0,0 @@ public class AsyncCatcher - { - - public static boolean enabled = true; -+ public static boolean shuttingDown = false; // Paper - - public static void catchOp(String reason) - { -diff --git a/src/main/java/org/spigotmc/RestartCommand.java b/src/main/java/org/spigotmc/RestartCommand.java -index 0000000000000000000000000000000000000000..0000000000000000000000000000000000000000 100644 ---- a/src/main/java/org/spigotmc/RestartCommand.java -+++ b/src/main/java/org/spigotmc/RestartCommand.java -@@ -0,0 +0,0 @@ public class RestartCommand extends Command - private static void restart(final String restartScript) - { - AsyncCatcher.enabled = false; // Disable async catcher incase it interferes with us -+ org.spigotmc.AsyncCatcher.shuttingDown = true; // Paper - try - { - String[] split = restartScript.split( " " ); diff --git a/patches/server/Improve-Player-chat-API-handling.patch b/patches/server/Improve-Player-chat-API-handling.patch new file mode 100644 index 0000000000..2215c60c04 --- /dev/null +++ b/patches/server/Improve-Player-chat-API-handling.patch @@ -0,0 +1,80 @@ +From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 +From: Aikar +Date: Thu, 3 Mar 2016 01:17:12 -0600 +Subject: [PATCH] Improve Player chat API handling + +Properly split up the chat and command handling to reflect the server now +having separate packets for both, and the client always using the correct packet. Text +from a chat packet should never be parsed into a command, even if it starts with the `/` +character. + +Add a missing async catcher and improve Spigot's async catcher error message. + +== AT == +public net.minecraft.server.network.ServerGamePacketListenerImpl isChatMessageIllegal(Ljava/lang/String;)Z + +Co-authored-by: Jake Potrebic +Co-authored-by: SoSeDiK + +diff --git a/src/main/java/net/minecraft/server/network/ServerGamePacketListenerImpl.java b/src/main/java/net/minecraft/server/network/ServerGamePacketListenerImpl.java +index 0000000000000000000000000000000000000000..0000000000000000000000000000000000000000 100644 +--- a/src/main/java/net/minecraft/server/network/ServerGamePacketListenerImpl.java ++++ b/src/main/java/net/minecraft/server/network/ServerGamePacketListenerImpl.java +@@ -0,0 +0,0 @@ public class ServerGamePacketListenerImpl implements ServerPlayerConnection, Tic + } + OutgoingChatMessage outgoing = OutgoingChatMessage.create(original); + +- if (!async && s.startsWith("/")) { ++ if (false && !async && s.startsWith("/")) { // Paper - Don't handle commands in chat logic + this.handleCommand(s); + } else if (this.player.getChatVisibility() == ChatVisiblity.SYSTEM) { + // Do nothing, this is coming from a plugin +@@ -0,0 +0,0 @@ public class ServerGamePacketListenerImpl implements ServerPlayerConnection, Tic + } + } + +- private void handleCommand(String s) { ++ public void handleCommand(String s) { // Paper - private -> public ++ org.spigotmc.AsyncCatcher.catchOp("Command Dispatched Async: " + s); // Paper - Add async catcher + co.aikar.timings.MinecraftTimings.playerCommandTimer.startTiming(); // Paper + if ( org.spigotmc.SpigotConfig.logCommands ) // Spigot + this.LOGGER.info(this.player.getScoreboardName() + " issued server command: " + s); +diff --git a/src/main/java/org/bukkit/craftbukkit/CraftServer.java b/src/main/java/org/bukkit/craftbukkit/CraftServer.java +index 0000000000000000000000000000000000000000..0000000000000000000000000000000000000000 100644 +--- a/src/main/java/org/bukkit/craftbukkit/CraftServer.java ++++ b/src/main/java/org/bukkit/craftbukkit/CraftServer.java +@@ -0,0 +0,0 @@ public final class CraftServer implements Server { + public boolean dispatchCommand(CommandSender sender, String commandLine) { + Preconditions.checkArgument(sender != null, "sender cannot be null"); + Preconditions.checkArgument(commandLine != null, "commandLine cannot be null"); +- org.spigotmc.AsyncCatcher.catchOp("command dispatch"); // Spigot ++ org.spigotmc.AsyncCatcher.catchOp("Command Dispatched Async: " + commandLine); // Spigot // Paper - Include command in error message + + if (this.commandMap.dispatch(sender, commandLine)) { + return true; +diff --git a/src/main/java/org/bukkit/craftbukkit/entity/CraftPlayer.java b/src/main/java/org/bukkit/craftbukkit/entity/CraftPlayer.java +index 0000000000000000000000000000000000000000..0000000000000000000000000000000000000000 100644 +--- a/src/main/java/org/bukkit/craftbukkit/entity/CraftPlayer.java ++++ b/src/main/java/org/bukkit/craftbukkit/entity/CraftPlayer.java +@@ -0,0 +0,0 @@ public class CraftPlayer extends CraftHumanEntity implements Player { + + if (this.getHandle().connection == null) return; + +- this.getHandle().connection.chat(msg, PlayerChatMessage.system(msg), false); ++ // Paper start - Improve chat handling ++ if (ServerGamePacketListenerImpl.isChatMessageIllegal(msg)) { ++ this.getHandle().connection.disconnect(Component.translatable("multiplayer.disconnect.illegal_characters")); ++ } else { ++ if (msg.startsWith("/")) { ++ this.getHandle().connection.handleCommand(msg); ++ } else { ++ final PlayerChatMessage playerChatMessage = PlayerChatMessage.system(msg).withResult(new net.minecraft.network.chat.ChatDecorator.ModernResult(Component.literal(msg), true, false)); ++ // TODO chat decorating ++ // TODO text filtering ++ this.getHandle().connection.chat(msg, playerChatMessage, false); ++ } ++ } ++ // Paper end + } + + @Override diff --git a/patches/server/Improved-Watchdog-Support.patch b/patches/server/Improved-Watchdog-Support.patch index aaa324468f..02ed572868 100644 --- a/patches/server/Improved-Watchdog-Support.patch +++ b/patches/server/Improved-Watchdog-Support.patch @@ -443,7 +443,6 @@ index 0000000000000000000000000000000000000000..00000000000000000000000000000000 + } + // Looks stalled, close async org.spigotmc.AsyncCatcher.enabled = false; // Spigot - org.spigotmc.AsyncCatcher.shuttingDown = true; // Paper + server.forceTicks = true; this.server.close(); + while (!server.hasFullyShutdown) Thread.sleep(1000); @@ -517,7 +516,6 @@ index 0000000000000000000000000000000000000000..00000000000000000000000000000000 { - RestartCommand.restart(); + AsyncCatcher.enabled = false; // Disable async catcher incase it interferes with us -+ AsyncCatcher.shuttingDown = true; + server.forceTicks = true; + if (restart) { + RestartCommand.addShutdownHook( SpigotConfig.restartScript ); diff --git a/patches/server/Kick-on-main-for-illegal-chat.patch b/patches/server/Kick-on-main-for-illegal-chat.patch index 201904a088..07dc400607 100644 --- a/patches/server/Kick-on-main-for-illegal-chat.patch +++ b/patches/server/Kick-on-main-for-illegal-chat.patch @@ -35,7 +35,7 @@ index 0000000000000000000000000000000000000000..00000000000000000000000000000000 if (!this.updateChatOrder(timestamp)) { ServerGamePacketListenerImpl.LOGGER.warn("{} sent out-of-order chat: '{}'", this.player.getName().getString(), message); + this.server.scheduleOnMain(() -> { // Paper - push to main - this.disconnect(Component.translatable("multiplayer.disconnect.out_of_order_chat"), org.bukkit.event.player.PlayerKickEvent.Cause.OUT_OF_ORDER_CHAT); // Paper - kick event ca + this.disconnect(Component.translatable("multiplayer.disconnect.out_of_order_chat"), org.bukkit.event.player.PlayerKickEvent.Cause.OUT_OF_ORDER_CHAT); // Paper - kick event causes + }); // Paper - push to main return Optional.empty(); } else { diff --git a/patches/server/Properly-handle-async-calls-to-restart-the-server.patch b/patches/server/Properly-handle-async-calls-to-restart-the-server.patch index 545abd0049..e5108ef83b 100644 --- a/patches/server/Properly-handle-async-calls-to-restart-the-server.patch +++ b/patches/server/Properly-handle-async-calls-to-restart-the-server.patch @@ -88,7 +88,7 @@ index 0000000000000000000000000000000000000000..00000000000000000000000000000000 --- a/src/main/java/org/spigotmc/RestartCommand.java +++ b/src/main/java/org/spigotmc/RestartCommand.java @@ -0,0 +0,0 @@ public class RestartCommand extends Command - org.spigotmc.AsyncCatcher.shuttingDown = true; // Paper + AsyncCatcher.enabled = false; // Disable async catcher incase it interferes with us try { - String[] split = restartScript.split( " " );