From cbf4def71bc63c591976b9716cc970f293588e09 Mon Sep 17 00:00:00 2001 From: "Kristian S. Stangeland" Date: Mon, 29 Oct 2012 16:54:53 +0100 Subject: [PATCH] Prevented map chunk listeners from crashing the server. If the NetServerHandler injector fails and we revert to the network field injector instead, any map chunk listener will crash the server due to complications with Bukkit's ChunkCompressionThread. We avoid this by disabling map chunk listening entirely. --- .../protocol/CleanupStaticMembers.java | 2 +- .../protocol/injector/ListenerInvoker.java | 21 ++++++++ .../protocol/injector/MinecraftRegistry.java | 6 ++- .../injector/PacketFilterManager.java | 20 +++++-- .../protocol/injector/PacketInjector.java | 2 +- .../injector/player/InjectedArrayList.java | 52 +++++++++++++------ .../injector/player/NetworkFieldInjector.java | 11 ++-- .../player/NetworkObjectInjector.java | 11 ++-- .../player/NetworkServerInjector.java | 3 +- .../player/PlayerInjectionHandler.java | 35 ++++++++----- .../injector/player/PlayerInjector.java | 14 ++++- .../injector/player/UnsupportedListener.java | 47 +++++++++++++++++ 12 files changed, 177 insertions(+), 47 deletions(-) create mode 100644 ProtocolLib/src/main/java/com/comphenix/protocol/injector/player/UnsupportedListener.java diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/CleanupStaticMembers.java b/ProtocolLib/src/main/java/com/comphenix/protocol/CleanupStaticMembers.java index 269345f9..60bbbcfa 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/CleanupStaticMembers.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/CleanupStaticMembers.java @@ -43,7 +43,7 @@ class CleanupStaticMembers { // This list must always be updated Class[] publicClasses = { AsyncListenerHandler.class, ListeningWhitelist.class, PacketContainer.class, - BukkitUnwrapper.class, CollectionGenerator.class, DefaultInstances.class, + BukkitUnwrapper.class, DefaultInstances.class, CollectionGenerator.class, PrimitiveGenerator.class, FuzzyReflection.class, MethodUtils.class, BackgroundCompiler.class, StructureCompiler.class, ObjectCloner.class, PrimitiveUtils.class, Packets.Server.class, diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/injector/ListenerInvoker.java b/ProtocolLib/src/main/java/com/comphenix/protocol/injector/ListenerInvoker.java index fd0a8391..ae48ea6d 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/injector/ListenerInvoker.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/injector/ListenerInvoker.java @@ -46,4 +46,25 @@ public interface ListenerInvoker { * @return The packet ID. */ public abstract int getPacketID(Packet packet); + + /** + * Associate a given class with the given packet ID. Internal method. + * @param clazz - class to associate. + * @param packetID - the packet ID. + */ + public abstract void unregisterPacketClass(Class clazz); + + /** + * Remove a given class from the packet registry. Internal method. + * @param clazz - class to remove. + */ + public abstract void registerPacketClass(Class clazz, int packetID); + + /** + * Retrieves the correct packet class from a given packet ID. + * @param packetID - the packet ID. + * @param forceVanilla - whether or not to look for vanilla classes, not injected classes. + * @return The associated class. + */ + public abstract Class getPacketClassFromID(int packetID, boolean forceVanilla); } \ No newline at end of file diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/injector/MinecraftRegistry.java b/ProtocolLib/src/main/java/com/comphenix/protocol/injector/MinecraftRegistry.java index 7b171e0d..34f7a341 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/injector/MinecraftRegistry.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/injector/MinecraftRegistry.java @@ -157,7 +157,7 @@ class MinecraftRegistry { /** * Retrieves the correct packet class from a given packet ID. * @param packetID - the packet ID. - * @param vanilla - whether or not to look for vanilla classes, not injected classes. + * @param forceVanilla - whether or not to look for vanilla classes, not injected classes. * @return The associated class. */ public static Class getPacketClassFromID(int packetID, boolean forceVanilla) { @@ -172,7 +172,9 @@ class MinecraftRegistry { // Will most likely not be used for (Map.Entry entry : getPacketToID().entrySet()) { if (Objects.equal(entry.getValue(), packetID)) { - return entry.getKey(); + // Attempt to get the vanilla class here too + if (!forceVanilla || entry.getKey().getName().startsWith("net.minecraft.server")) + return entry.getKey(); } } diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/injector/PacketFilterManager.java b/ProtocolLib/src/main/java/com/comphenix/protocol/injector/PacketFilterManager.java index f4d86ae9..4284b566 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/injector/PacketFilterManager.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/injector/PacketFilterManager.java @@ -174,7 +174,7 @@ public final class PacketFilterManager implements ProtocolManager, ListenerInvok try { // Initialize injection mangers - this.playerInjection = new PlayerInjectionHandler(classLoader, reporter, isInjectionNecessary, this, server); + this.playerInjection = new PlayerInjectionHandler(classLoader, reporter, isInjectionNecessary, this, packetListeners, server); this.packetInjector = new PacketInjector(classLoader, this, playerInjection); this.asyncFilterManager = new AsyncFilterManager(reporter, server.getScheduler(), this); @@ -210,9 +210,6 @@ public final class PacketFilterManager implements ProtocolManager, ListenerInvok */ public void setPlayerHook(PlayerInjectHooks playerHook) { playerInjection.setPlayerHook(playerHook); - - // Make sure the current listeners are compatible - playerInjection.checkListener(packetListeners); } @Override @@ -662,6 +659,21 @@ public final class PacketFilterManager implements ProtocolManager, ListenerInvok return MinecraftRegistry.getPacketToID().get(packet.getClass()); } + @Override + public void registerPacketClass(Class clazz, int packetID) { + MinecraftRegistry.getPacketToID().put(clazz, packetID); + } + + @Override + public void unregisterPacketClass(Class clazz) { + MinecraftRegistry.getPacketToID().remove(clazz); + } + + @Override + public Class getPacketClassFromID(int packetID, boolean forceVanilla) { + return MinecraftRegistry.getPacketClassFromID(packetID, forceVanilla); + } + // Yes, this is crazy. @SuppressWarnings({ "unchecked", "rawtypes" }) private void registerOld(PluginManager manager, Plugin plugin) { diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/injector/PacketInjector.java b/ProtocolLib/src/main/java/com/comphenix/protocol/injector/PacketInjector.java index 82bcb303..625093da 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/injector/PacketInjector.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/injector/PacketInjector.java @@ -141,10 +141,10 @@ class PacketInjector { try { // Override values - putMethod.invoke(intHashMap, packetID, proxy); previous.put(packetID, old); registry.put(proxy, packetID); overwritten.put(packetID, proxy); + putMethod.invoke(intHashMap, packetID, proxy); return true; } catch (IllegalArgumentException e) { diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/injector/player/InjectedArrayList.java b/ProtocolLib/src/main/java/com/comphenix/protocol/injector/player/InjectedArrayList.java index 8b2f1471..f75b0718 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/injector/player/InjectedArrayList.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/injector/player/InjectedArrayList.java @@ -22,6 +22,8 @@ import java.lang.reflect.Method; import java.util.ArrayList; import java.util.Set; +import com.comphenix.protocol.Packets; +import com.comphenix.protocol.injector.ListenerInvoker; import com.comphenix.protocol.injector.player.NetworkFieldInjector.FakePacket; import net.minecraft.server.Packet; @@ -73,7 +75,7 @@ class InjectedArrayList extends ArrayList { // We'll use the FakePacket marker instead of preventing the filters injector.sendServerPacket(createNegativePacket(packet), true); } - + // Collection.add contract return true; @@ -90,8 +92,10 @@ class InjectedArrayList extends ArrayList { * @return The inverted packet. */ Packet createNegativePacket(Packet source) { - Enhancer ex = new Enhancer(); - Class type = source.getClass(); + ListenerInvoker invoker = injector.getInvoker(); + + int packetID = invoker.getPacketID(source); + Class type = invoker.getPacketClassFromID(packetID, true); // We want to subtract the byte amount that were added to the running // total of outstanding packets. Otherwise, cancelling too many packets @@ -111,22 +115,38 @@ class InjectedArrayList extends ArrayList { // } // ect. // } + Enhancer ex = new Enhancer(); + ex.setSuperclass(type); ex.setInterfaces(new Class[] { FakePacket.class } ); ex.setUseCache(true); ex.setClassLoader(classLoader); - ex.setSuperclass(type); - ex.setCallback(new MethodInterceptor() { - @Override - public Object intercept(Object obj, Method method, Object[] args, MethodProxy proxy) throws Throwable { - if (method.getReturnType().equals(int.class) && args.length == 0) { - Integer result = (Integer) proxy.invokeSuper(obj, args); - return -result; - } else { - return proxy.invokeSuper(obj, args); - } - } - }); + ex.setCallbackType(InvertedIntegerCallback.class); + + Class proxyClass = ex.createClass(); + + // Temporarily associate the fake packet class + invoker.registerPacketClass(proxyClass, packetID); + + Packet fake = (Packet) Enhancer.create(proxyClass, new InvertedIntegerCallback()); - return (Packet) ex.create(); + // Remove this association + invoker.unregisterPacketClass(proxyClass); + return fake; + } + + /** + * Inverts the integer result of every integer method. + * @author Kristian + */ + private class InvertedIntegerCallback implements MethodInterceptor { + @Override + public Object intercept(Object obj, Method method, Object[] args, MethodProxy proxy) throws Throwable { + if (method.getReturnType().equals(int.class) && args.length == 0) { + Integer result = (Integer) proxy.invokeSuper(obj, args); + return -result; + } else { + return proxy.invokeSuper(obj, args); + } + } } } diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/injector/player/NetworkFieldInjector.java b/ProtocolLib/src/main/java/com/comphenix/protocol/injector/player/NetworkFieldInjector.java index 2f414f2c..dd65fa17 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/injector/player/NetworkFieldInjector.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/injector/player/NetworkFieldInjector.java @@ -123,11 +123,14 @@ class NetworkFieldInjector extends PlayerInjector { } @Override - public void checkListener(PacketListener listener) { + public UnsupportedListener checkListener(PacketListener listener) { + int[] unsupported = { Packets.Server.MAP_CHUNK, Packets.Server.MAP_CHUNK_BULK }; + // Unfortunately, we don't support chunk packets - if (ListeningWhitelist.containsAny(listener.getSendingWhitelist(), - Packets.Server.MAP_CHUNK, Packets.Server.MAP_CHUNK_BULK)) { - throw new IllegalStateException("The NETWORK_FIELD_INJECTOR hook doesn't support map chunk listeners."); + if (ListeningWhitelist.containsAny(listener.getSendingWhitelist(), unsupported)) { + return new UnsupportedListener("The NETWORK_FIELD_INJECTOR hook doesn't support map chunk listeners.", unsupported); + } else { + return null; } } diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/injector/player/NetworkObjectInjector.java b/ProtocolLib/src/main/java/com/comphenix/protocol/injector/player/NetworkObjectInjector.java index 6635744d..e391d832 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/injector/player/NetworkObjectInjector.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/injector/player/NetworkObjectInjector.java @@ -88,11 +88,14 @@ class NetworkObjectInjector extends PlayerInjector { } @Override - public void checkListener(PacketListener listener) { + public UnsupportedListener checkListener(PacketListener listener) { + int[] unsupported = { Packets.Server.MAP_CHUNK, Packets.Server.MAP_CHUNK_BULK }; + // Unfortunately, we don't support chunk packets - if (ListeningWhitelist.containsAny(listener.getSendingWhitelist(), - Packets.Server.MAP_CHUNK, Packets.Server.MAP_CHUNK_BULK)) { - throw new IllegalStateException("The NETWORK_FIELD_INJECTOR hook doesn't support map chunk listeners."); + if (ListeningWhitelist.containsAny(listener.getSendingWhitelist(), unsupported)) { + return new UnsupportedListener("The NETWORK_OBJECT_INJECTOR hook doesn't support map chunk listeners.", unsupported); + } else { + return null; } } diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/injector/player/NetworkServerInjector.java b/ProtocolLib/src/main/java/com/comphenix/protocol/injector/player/NetworkServerInjector.java index 70ed7a3f..c97d9f50 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/injector/player/NetworkServerInjector.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/injector/player/NetworkServerInjector.java @@ -301,8 +301,9 @@ public class NetworkServerInjector extends PlayerInjector { } @Override - public void checkListener(PacketListener listener) { + public UnsupportedListener checkListener(PacketListener listener) { // We support everything + return null; } @Override diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/injector/player/PlayerInjectionHandler.java b/ProtocolLib/src/main/java/com/comphenix/protocol/injector/player/PlayerInjectionHandler.java index 215fc3c7..2b2b86a6 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/injector/player/PlayerInjectionHandler.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/injector/player/PlayerInjectionHandler.java @@ -83,6 +83,9 @@ public class PlayerInjectionHandler { // Enabled packet filters private IntegerSet sendingFilters = new IntegerSet(MAXIMUM_PACKET_ID + 1); + // List of packet listeners + private Set packetListeners; + // The class loader we're using private ClassLoader classLoader; @@ -90,12 +93,13 @@ public class PlayerInjectionHandler { private Predicate injectionFilter; public PlayerInjectionHandler(ClassLoader classLoader, ErrorReporter reporter, Predicate injectionFilter, - ListenerInvoker invoker, Server server) { + ListenerInvoker invoker, Set packetListeners, Server server) { this.classLoader = classLoader; this.reporter = reporter; this.invoker = invoker; this.injectionFilter = injectionFilter; + this.packetListeners = packetListeners; this.netLoginInjector = new NetLoginInjector(reporter, this, server); this.serverInjection = new InjectedServerConnection(reporter, server, netLoginInjector); serverInjection.injectList(); @@ -143,6 +147,9 @@ public class PlayerInjectionHandler { loginPlayerHook = playerHook; if (phase.hasPlaying()) playingPlayerHook = playerHook; + + // Make sure the current listeners are compatible + checkListener(packetListeners); } /** @@ -534,26 +541,30 @@ public class PlayerInjectionHandler { // Make sure the current listeners are compatible if (lastSuccessfulHook != null) { for (PacketListener listener : listeners) { - try { - checkListener(listener); - } catch (IllegalStateException e) { - reporter.reportWarning(this, "Unsupported listener.", e); - } + checkListener(listener); } } } /** * Determine if a listener is valid or not. + *

+ * If not, a warning will be printed to the console. * @param listener - listener to check. - * @throws IllegalStateException If the given listener's whitelist cannot be fulfilled. */ public void checkListener(PacketListener listener) { - try { - if (lastSuccessfulHook != null) - lastSuccessfulHook.checkListener(listener); - } catch (Exception e) { - throw new IllegalStateException("Registering listener " + PacketAdapter.getPluginName(listener) + " failed", e); + if (lastSuccessfulHook != null) { + UnsupportedListener result = lastSuccessfulHook.checkListener(listener); + + // We won't prevent the listener, as it may still have valid packets + if (result != null) { + reporter.reportWarning(this, "Cannot fully register listener for " + + PacketAdapter.getPluginName(listener) + ": " + result.toString()); + + // These are illegal + for (int packetID : result.getPackets()) + removePacketHandler(packetID); + } } } diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/injector/player/PlayerInjector.java b/ProtocolLib/src/main/java/com/comphenix/protocol/injector/player/PlayerInjector.java index 31b2eeda..5508d235 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/injector/player/PlayerInjector.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/injector/player/PlayerInjector.java @@ -489,10 +489,12 @@ abstract class PlayerInjector { /** * Invoked before a new listener is registered. *

- * The player injector should throw an exception if this listener cannot be properly supplied with packet events. + * The player injector should only return a non-null value if some or all of the packet IDs are unsupported. + * * @param listener - the listener that is about to be registered. + * @return A error message with the unsupported packet IDs, or NULL if this listener is valid. */ - public abstract void checkListener(PacketListener listener); + public abstract UnsupportedListener checkListener(PacketListener listener); /** * Allows a packet to be sent by the listeners. @@ -589,6 +591,14 @@ abstract class PlayerInjector { return player; } + /** + * Object that can invoke the packet events. + * @return Packet event invoker. + */ + public ListenerInvoker getInvoker() { + return invoker; + } + /** * Retrieve the hooked player object OR the more up-to-date player instance. * @return The hooked player, or a more up-to-date instance. diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/injector/player/UnsupportedListener.java b/ProtocolLib/src/main/java/com/comphenix/protocol/injector/player/UnsupportedListener.java new file mode 100644 index 00000000..c00f1088 --- /dev/null +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/injector/player/UnsupportedListener.java @@ -0,0 +1,47 @@ +package com.comphenix.protocol.injector.player; + +import java.util.Arrays; + +import com.google.common.base.Joiner; + +/** + * Represents an error message from a player injector. + * + * @author Kristian + */ +public class UnsupportedListener { + private String message; + private int[] packets; + + /** + * Create a new error message. + * @param message - the message. + * @param packets - unsupported packets. + */ + public UnsupportedListener(String message, int[] packets) { + super(); + this.message = message; + this.packets = packets; + } + + /** + * Retrieve the error message. + * @return Error message. + */ + public String getMessage() { + return message; + } + + /** + * Retrieve all unsupported packets. + * @return Unsupported packets. + */ + public int[] getPackets() { + return packets; + } + + @Override + public String toString() { + return String.format("%s (%s)", message, Joiner.on(", ").join(Arrays.asList(packets))); + } +}