From 0dc2bfef0ca81eaad87fde03d0ecd955f3636fea Mon Sep 17 00:00:00 2001 From: "Kristian S. Stangeland" Date: Mon, 29 Oct 2012 17:20:04 +0100 Subject: [PATCH] Handle exceptions in injected code. This is very important, otherwise the server may crash unnecessarily. --- .../injector/PacketFilterManager.java | 5 +- .../protocol/injector/PacketInjector.java | 9 ++- .../protocol/injector/ReadPacketModifier.java | 50 +++++++------ .../player/InjectedServerConnection.java | 7 +- .../injector/player/NetLoginInjector.java | 48 +++++++------ .../injector/player/PlayerInjector.java | 71 ++++++++++--------- 6 files changed, 109 insertions(+), 81 deletions(-) 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 4284b566..e84998eb 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/injector/PacketFilterManager.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/injector/PacketFilterManager.java @@ -49,7 +49,6 @@ import com.comphenix.protocol.AsynchronousManager; import com.comphenix.protocol.ProtocolManager; import com.comphenix.protocol.async.AsyncFilterManager; import com.comphenix.protocol.async.AsyncMarker; -import com.comphenix.protocol.error.DetailedErrorReporter; import com.comphenix.protocol.error.ErrorReporter; import com.comphenix.protocol.events.*; import com.comphenix.protocol.injector.player.PlayerInjectionHandler; @@ -142,7 +141,7 @@ public final class PacketFilterManager implements ProtocolManager, ListenerInvok * Only create instances of this class if protocol lib is disabled. * @param unhookTask */ - public PacketFilterManager(ClassLoader classLoader, Server server, DelayedSingleTask unhookTask, DetailedErrorReporter reporter) { + public PacketFilterManager(ClassLoader classLoader, Server server, DelayedSingleTask unhookTask, ErrorReporter reporter) { if (reporter == null) throw new IllegalArgumentException("reporter cannot be NULL."); if (classLoader == null) @@ -175,7 +174,7 @@ public final class PacketFilterManager implements ProtocolManager, ListenerInvok try { // Initialize injection mangers this.playerInjection = new PlayerInjectionHandler(classLoader, reporter, isInjectionNecessary, this, packetListeners, server); - this.packetInjector = new PacketInjector(classLoader, this, playerInjection); + this.packetInjector = new PacketInjector(classLoader, this, playerInjection, reporter); this.asyncFilterManager = new AsyncFilterManager(reporter, server.getScheduler(), this); // Attempt to load the list of server and client packets 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 625093da..951e4d79 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/injector/PacketInjector.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/injector/PacketInjector.java @@ -31,6 +31,7 @@ import net.minecraft.server.Packet; import net.sf.cglib.proxy.Callback; import net.sf.cglib.proxy.Enhancer; +import com.comphenix.protocol.error.ErrorReporter; import com.comphenix.protocol.events.PacketContainer; import com.comphenix.protocol.events.PacketEvent; import com.comphenix.protocol.injector.player.PlayerInjectionHandler; @@ -51,6 +52,9 @@ class PacketInjector { // The packet filter manager private ListenerInvoker manager; + // Error reporter + private ErrorReporter reporter; + // Allows us to determine the sender private PlayerInjectionHandler playerInjection; @@ -61,11 +65,12 @@ class PacketInjector { private ClassLoader classLoader; public PacketInjector(ClassLoader classLoader, ListenerInvoker manager, - PlayerInjectionHandler playerInjection) throws IllegalAccessException { + PlayerInjectionHandler playerInjection, ErrorReporter reporter) throws IllegalAccessException { this.classLoader = classLoader; this.manager = manager; this.playerInjection = playerInjection; + this.reporter = reporter; this.readModifier = new ConcurrentHashMap(); initialize(); } @@ -133,7 +138,7 @@ class PacketInjector { Class proxy = ex.createClass(); // Create the proxy handler - ReadPacketModifier modifier = new ReadPacketModifier(packetID, this); + ReadPacketModifier modifier = new ReadPacketModifier(packetID, this, reporter); readModifier.put(packetID, modifier); // Add a static reference diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/injector/ReadPacketModifier.java b/ProtocolLib/src/main/java/com/comphenix/protocol/injector/ReadPacketModifier.java index 67b3fae5..6e1967e1 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/injector/ReadPacketModifier.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/injector/ReadPacketModifier.java @@ -25,6 +25,7 @@ import java.util.Map; import java.util.WeakHashMap; import com.comphenix.protocol.Packets; +import com.comphenix.protocol.error.ErrorReporter; import com.comphenix.protocol.events.PacketContainer; import com.comphenix.protocol.events.PacketEvent; @@ -44,12 +45,16 @@ class ReadPacketModifier implements MethodInterceptor { private PacketInjector packetInjector; private int packetID; + // Report errors + private ErrorReporter reporter; + // Whether or not a packet has been cancelled private static Map override = Collections.synchronizedMap(new WeakHashMap()); - public ReadPacketModifier(int packetID, PacketInjector packetInjector) { + public ReadPacketModifier(int packetID, PacketInjector packetInjector, ErrorReporter reporter) { this.packetID = packetID; this.packetInjector = packetInjector; + this.reporter = reporter; } /** @@ -102,27 +107,32 @@ class ReadPacketModifier implements MethodInterceptor { if (returnValue == null && Arrays.equals(method.getParameterTypes(), parameters)) { - // We need this in order to get the correct player - DataInputStream input = (DataInputStream) args[0]; - - // Let the people know - PacketContainer container = new PacketContainer(packetID, (Packet) thisObj); - PacketEvent event = packetInjector.packetRecieved(container, input); - - // Handle override - if (event != null) { - Packet result = event.getPacket().getHandle(); + try { + // We need this in order to get the correct player + DataInputStream input = (DataInputStream) args[0]; + + // Let the people know + PacketContainer container = new PacketContainer(packetID, (Packet) thisObj); + PacketEvent event = packetInjector.packetRecieved(container, input); - if (event.isCancelled()) { - override.put(thisObj, CANCEL_MARKER); - } else if (!objectEquals(thisObj, result)) { - override.put(thisObj, result); - } - - // Update DataInputStream next time - if (!event.isCancelled() && packetID == Packets.Server.KEY_RESPONSE) { - packetInjector.scheduleDataInputRefresh(event.getPlayer()); + // Handle override + if (event != null) { + Packet result = event.getPacket().getHandle(); + + if (event.isCancelled()) { + override.put(thisObj, CANCEL_MARKER); + } else if (!objectEquals(thisObj, result)) { + override.put(thisObj, result); + } + + // Update DataInputStream next time + if (!event.isCancelled() && packetID == Packets.Server.KEY_RESPONSE) { + packetInjector.scheduleDataInputRefresh(event.getPlayer()); + } } + } catch (Throwable e) { + // Minecraft cannot handle this error + reporter.reportDetailed(this, "Cannot handle clienet packet.", e, args[0]); } } diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/injector/player/InjectedServerConnection.java b/ProtocolLib/src/main/java/com/comphenix/protocol/injector/player/InjectedServerConnection.java index e7b20d58..c9ae47ff 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/injector/player/InjectedServerConnection.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/injector/player/InjectedServerConnection.java @@ -217,7 +217,12 @@ class InjectedServerConnection { // Is this a normal Minecraft object? if (!(inserting instanceof Factory)) { // If so, copy the content of the old element to the new - ObjectCloner.copyTo(inserting, replacement, inserting.getClass()); + try { + ObjectCloner.copyTo(inserting, replacement, inserting.getClass()); + } catch (Throwable e) { + reporter.reportDetailed(InjectedServerConnection.this, "Cannot copy old " + inserting + + " to new.", e, inserting, replacement); + } } } diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/injector/player/NetLoginInjector.java b/ProtocolLib/src/main/java/com/comphenix/protocol/injector/player/NetLoginInjector.java index b5fc924b..ac1c16d6 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/injector/player/NetLoginInjector.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/injector/player/NetLoginInjector.java @@ -95,30 +95,34 @@ class NetLoginInjector { PlayerInjector injected = injectedLogins.get(removing); if (injected != null) { - PlayerInjector newInjector = null; - Player player = injected.getPlayer(); - - // Clean up list - injectedLogins.remove(removing); - - // No need to clean up twice - if (injected.isClean()) - return; - - // Hack to clean up other references - newInjector = injectionHandler.getInjectorByNetworkHandler(injected.getNetworkManager()); - - // Update NetworkManager - if (newInjector == null) { - injectionHandler.uninjectPlayer(player); - } else { - injectionHandler.uninjectPlayer(player, false); + try { + PlayerInjector newInjector = null; + Player player = injected.getPlayer(); - if (injected instanceof NetworkObjectInjector) - newInjector.setNetworkManager(injected.getNetworkManager(), true); + // Clean up list + injectedLogins.remove(removing); + + // No need to clean up twice + if (injected.isClean()) + return; + + // Hack to clean up other references + newInjector = injectionHandler.getInjectorByNetworkHandler(injected.getNetworkManager()); + + // Update NetworkManager + if (newInjector == null) { + injectionHandler.uninjectPlayer(player); + } else { + injectionHandler.uninjectPlayer(player, false); + + if (injected instanceof NetworkObjectInjector) + newInjector.setNetworkManager(injected.getNetworkManager(), true); + } + + } catch (Throwable e) { + // Don't leak this to Minecraft + reporter.reportDetailed(this, "Cannot cleanup NetLoginHandler.", e, removing); } - - //logger.warning("Using alternative cleanup method."); } } 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 5508d235..6eda9848 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 @@ -502,43 +502,48 @@ abstract class PlayerInjector { * @return The given packet, or the packet replaced by the listeners. */ public Packet handlePacketSending(Packet packet) { - // Get the packet ID too - Integer id = invoker.getPacketID(packet); - Player currentPlayer = player; - - // Hack #1: Handle a single scheduled action - if (scheduledAction != null) { - scheduledAction.run(); - scheduledAction = null; - } - // Hack #2 - if (updateOnLogin) { - if (id == Packets.Server.LOGIN) { - try { - updatedPlayer = getEntityPlayer(getNetHandler()).getBukkitEntity(); - } catch (IllegalAccessException e) { - reporter.reportDetailed(this, "Cannot update player in PlayerEvent.", e, packet); + try { + // Get the packet ID too + Integer id = invoker.getPacketID(packet); + Player currentPlayer = player; + + // Hack #1: Handle a single scheduled action + if (scheduledAction != null) { + scheduledAction.run(); + scheduledAction = null; + } + // Hack #2 + if (updateOnLogin) { + if (id == Packets.Server.LOGIN) { + try { + updatedPlayer = getEntityPlayer(getNetHandler()).getBukkitEntity(); + } catch (IllegalAccessException e) { + reporter.reportDetailed(this, "Cannot update player in PlayerEvent.", e, packet); + } } + + // This will only occur in the NetLoginHandler injection + if (updatedPlayer != null) + currentPlayer = updatedPlayer; } - // This will only occur in the NetLoginHandler injection - if (updatedPlayer != null) - currentPlayer = updatedPlayer; - } - - // Make sure we're listening - if (id != null && hasListener(id)) { - // A packet has been sent guys! - PacketContainer container = new PacketContainer(id, packet); - PacketEvent event = PacketEvent.fromServer(invoker, container, currentPlayer); - invoker.invokePacketSending(event); + // Make sure we're listening + if (id != null && hasListener(id)) { + // A packet has been sent guys! + PacketContainer container = new PacketContainer(id, packet); + PacketEvent event = PacketEvent.fromServer(invoker, container, currentPlayer); + invoker.invokePacketSending(event); + + // Cancelling is pretty simple. Just ignore the packet. + if (event.isCancelled()) + return null; + + // Right, remember to replace the packet again + return event.getPacket().getHandle(); + } - // Cancelling is pretty simple. Just ignore the packet. - if (event.isCancelled()) - return null; - - // Right, remember to replace the packet again - return event.getPacket().getHandle(); + } catch (Throwable e) { + reporter.reportDetailed(this, "Cannot handle server packet.", e, packet); } return packet;