From a51ad1f50f9d9f225012e9d73b5b9b05854b7d25 Mon Sep 17 00:00:00 2001 From: "Kristian S. Stangeland" Date: Sun, 21 Oct 2012 20:31:20 +0200 Subject: [PATCH] Properly remove previous hook, even if socket is NULL. FIXES Ticket-1. --- ProtocolLib/dependency-reduced-pom.xml | 2 +- .../injector/player/NetLoginInjector.java | 27 ++++- .../injector/player/NetworkFieldInjector.java | 2 +- .../player/NetworkObjectInjector.java | 2 +- .../player/NetworkServerInjector.java | 2 +- .../player/PlayerInjectionHandler.java | 107 ++++++++++++++---- .../injector/player/PlayerInjector.java | 38 ++++++- .../player/TemporaryPlayerFactory.java | 5 +- .../reflect/instances/DefaultInstances.java | 1 - 9 files changed, 159 insertions(+), 27 deletions(-) diff --git a/ProtocolLib/dependency-reduced-pom.xml b/ProtocolLib/dependency-reduced-pom.xml index 09e31779..a9c613e4 100644 --- a/ProtocolLib/dependency-reduced-pom.xml +++ b/ProtocolLib/dependency-reduced-pom.xml @@ -4,7 +4,7 @@ com.comphenix.protocol ProtocolLib ProtocolLib - 1.4.1 + 1.4.2-SNAPSHOT Provides read/write access to the Minecraft protocol. http://dev.bukkit.org/server-mods/protocollib/ 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 7f753fbb..0bc8981a 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 @@ -63,6 +63,9 @@ class NetLoginInjector { InjectContainer container = (InjectContainer) fakePlayer; container.setInjector(injector); + // Save the login + injectedLogins.putIfAbsent(inserting, injector); + // NetServerInjector can never work (currently), so we don't need to replace the NetLoginHandler return inserting; @@ -93,8 +96,30 @@ class NetLoginInjector { PlayerInjector injected = injectedLogins.get(removing); if (injected != null) { - injected.cleanupAll(); + 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); + + if (injected instanceof NetworkObjectInjector) + newInjector.setNetworkManager(injected.getNetworkManager(), true); + } + + //logger.warning("Using alternative cleanup method."); } } 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 d6c21dbb..10bf4f1d 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 @@ -166,7 +166,7 @@ class NetworkFieldInjector extends PlayerInjector { } @SuppressWarnings("unchecked") - public void cleanupAll() { + protected void cleanHook() { // Clean up for (VolatileField overriden : overridenLists) { List minecraftList = (List) overriden.getOldValue(); 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 e8d07dd8..9cf12a21 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 @@ -152,7 +152,7 @@ class NetworkObjectInjector extends PlayerInjector { } @Override - public void cleanupAll() { + protected void cleanHook() { // Clean up if (networkManagerRef != null && networkManagerRef.isCurrentSet()) { networkManagerRef.revertValue(); 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 5f8330d0..9872947a 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 @@ -232,7 +232,7 @@ public class NetworkServerInjector extends PlayerInjector { } @Override - public void cleanupAll() { + protected void cleanHook() { if (serverHandlerRef != null && serverHandlerRef.isCurrentSet()) { ObjectCloner.copyTo(serverHandlerRef.getValue(), serverHandlerRef.getOldValue(), serverHandler.getClass()); serverHandlerRef.revertValue(); 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 25aecf27..14b3279a 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 @@ -281,28 +281,27 @@ public class PlayerInjectionHandler { DataInputStream inputStream = injector.getInputStream(false); Socket socket = injector.getSocket(); - SocketAddress address = socket.getRemoteSocketAddress(); + SocketAddress address = socket != null ? socket.getRemoteSocketAddress() : null; // Make sure the current player is not logged out - if (socket.isClosed()) { + if (socket != null && socket.isClosed()) { throw new PlayerLoggedOutException(); } - PlayerInjector previous = addressLookup.get(address); + // Guard against NPE here too + PlayerInjector previous = address != null ? addressLookup.get(address) : null; // Close any previously associated hooks before we proceed if (previous != null) { - uninjectPlayer(previous.getPlayer()); - - // Remove the "hooked" network manager in our instance as well - if (previous instanceof NetworkObjectInjector) { - injector.setNetworkManager(previous.getNetworkManager(), true); - } + uninjectPlayer(previous.getPlayer(), false, true); } - + injector.injectManager(); - dataInputLookup.put(inputStream, injector); - addressLookup.put(address, injector); + + if (inputStream != null) + dataInputLookup.put(inputStream, injector); + if (address != null) + addressLookup.put(address, injector); break; } @@ -362,8 +361,30 @@ public class PlayerInjectionHandler { /** * Unregisters the given player. * @param player - player to unregister. + * @return TRUE if a player has been uninjected, FALSE otherwise. */ - public void uninjectPlayer(Player player) { + public boolean uninjectPlayer(Player player) { + return uninjectPlayer(player, true, false); + } + + /** + * Unregisters the given player. + * @param player - player to unregister. + * @param removeAuxiliary - TRUE to remove auxiliary information, such as input stream and address. + * @return TRUE if a player has been uninjected, FALSE otherwise. + */ + public boolean uninjectPlayer(Player player, boolean removeAuxiliary) { + return uninjectPlayer(player, removeAuxiliary, false); + } + + /** + * Unregisters the given player. + * @param player - player to unregister. + * @param removeAuxiliary - TRUE to remove auxiliary information, such as input stream and address. + * @param prepareNextHook - whether or not we need to fix any lingering hooks. + * @return TRUE if a player has been uninjected, FALSE otherwise. + */ + private boolean uninjectPlayer(Player player, boolean removeAuxiliary, boolean prepareNextHook) { if (!hasClosed && player != null) { PlayerInjector injector = playerInjection.remove(player); @@ -373,26 +394,54 @@ public class PlayerInjectionHandler { InetSocketAddress address = player.getAddress(); injector.cleanupAll(); - dataInputLookup.remove(input); + // Remove the "hooked" network manager in our instance as well + if (prepareNextHook && injector instanceof NetworkObjectInjector) { + try { + PlayerInjector dummyInjector = getHookInstance(player, PlayerInjectHooks.NETWORK_SERVER_OBJECT); + dummyInjector.initializePlayer(player); + dummyInjector.setNetworkManager(injector.getNetworkManager(), true); + + } catch (IllegalAccessException e) { + // Let the user know + logger.log(Level.WARNING, "Unable to fully revert old injector. May cause conflicts.", e); + } + } - if (address != null) - addressLookup.remove(address); + // Clean up + if (removeAuxiliary) { + if (input != null) + dataInputLookup.remove(input); + if (address != null) + addressLookup.remove(address); + } + return true; } - } + } + + return false; } /** * Unregisters a player by the given address. + *

+ * If the server handler has been created before we've gotten a chance to unject the player, + * the method will try a workaround to remove the injected hook in the NetServerHandler. + * * @param address - address of the player to unregister. + * @param serverHandler - whether or not the net server handler has already been created. + * @return TRUE if a player has been uninjected, FALSE otherwise. */ - public void uninjectPlayer(InetSocketAddress address) { + public boolean uninjectPlayer(InetSocketAddress address) { if (!hasClosed && address != null) { PlayerInjector injector = addressLookup.get(address); // Clean up if (injector != null) - uninjectPlayer(injector.getPlayer()); + uninjectPlayer(injector.getPlayer(), false, true); + return true; } + + return false; } /** @@ -445,6 +494,26 @@ public class PlayerInjectionHandler { return playerInjection.get(player); } + /** + * Retrieve a player injector by looking for its NetworkManager. + * @param networkManager - current network manager. + * @return Related player injector. + */ + PlayerInjector getInjectorByNetworkHandler(Object networkManager) { + // That's not legal + if (networkManager == null) + return null; + + // O(n) is okay in this instance. This is only a backup solution. + for (PlayerInjector injector : playerInjection.values()) { + if (injector.getNetworkManager() == networkManager) + return injector; + } + + // None found + return null; + } + /** * Determine if the given listeners are valid. * @param listeners - listeners to check. 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 3e2457e9..739df11c 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 @@ -23,6 +23,7 @@ import java.lang.reflect.Field; import java.lang.reflect.InvocationTargetException; import java.lang.reflect.Method; import java.net.Socket; +import java.net.SocketAddress; import java.util.logging.Level; import java.util.logging.Logger; @@ -104,6 +105,9 @@ abstract class PlayerInjector { // Scheduled action on the next packet event protected Runnable scheduledAction; + // Whether or not the injector has been cleaned + private boolean clean; + // Whether or not to update the current player on the first Packet1Login boolean updateOnLogin; Player updatedPlayer; @@ -256,6 +260,21 @@ abstract class PlayerInjector { } } + /** + * Retrieve the associated address of this player. + * @return The associated address. + * @throws IllegalAccessException If we're unable to read the socket field. + */ + public SocketAddress getAddress() throws IllegalAccessException { + Socket socket = getSocket(); + + // Guard against NULL + if (socket != null) + return socket.getRemoteSocketAddress(); + else + return null; + } + /** * Attempt to disconnect the current client. * @param message - the message to display. @@ -427,7 +446,24 @@ abstract class PlayerInjector { /** * Remove all hooks and modifications. */ - public abstract void cleanupAll(); + public final void cleanupAll() { + if (!clean) + cleanHook(); + clean = true; + } + + /** + * Override to add custom cleanup behavior. + */ + protected abstract void cleanHook(); + + /** + * Determine whether or not this hook has already been cleaned. + * @return TRUE if it has, FALSE otherwise. + */ + public boolean isClean() { + return clean; + } /** * Determine if this inject method can even be attempted. diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/injector/player/TemporaryPlayerFactory.java b/ProtocolLib/src/main/java/com/comphenix/protocol/injector/player/TemporaryPlayerFactory.java index bb1d6a96..b6afb6a8 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/injector/player/TemporaryPlayerFactory.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/injector/player/TemporaryPlayerFactory.java @@ -76,13 +76,16 @@ class TemporaryPlayerFactory { String methodName = method.getName(); PlayerInjector injector = ((InjectContainer) obj).getInjector(); + if (injector == null) + throw new IllegalStateException("Unable to find injector."); + // Use the socket to get the address if (methodName.equalsIgnoreCase("getName")) return "UNKNOWN[" + injector.getSocket().getRemoteSocketAddress() + "]"; if (methodName.equalsIgnoreCase("getPlayer")) return injector.getUpdatedPlayer(); if (methodName.equalsIgnoreCase("getAddress")) - return injector.getSocket().getRemoteSocketAddress(); + return injector.getAddress(); if (methodName.equalsIgnoreCase("getServer")) return server; diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/reflect/instances/DefaultInstances.java b/ProtocolLib/src/main/java/com/comphenix/protocol/reflect/instances/DefaultInstances.java index 46505891..38066613 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/reflect/instances/DefaultInstances.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/reflect/instances/DefaultInstances.java @@ -206,7 +206,6 @@ public class DefaultInstances { // Just check if any of them are NULL for (Class type : types) { if (getDefaultInternal(type, providers, recursionLevel) == null) { - System.out.println(type.getName() + " is NULL!"); return true; } }