From 79da4f92a1c196091bf4744635e5aeef1e7cb207 Mon Sep 17 00:00:00 2001 From: Dan Mulloy Date: Thu, 1 Jan 2015 12:48:05 -0500 Subject: [PATCH] Don't attempt to process packets after disconnect Addresses #28 --- .../comphenix/protocol/ProtocolConfig.java | 24 +++++------ .../comphenix/protocol/ProtocolLibrary.java | 13 +++++- .../comphenix/protocol/async/AsyncMarker.java | 6 ++- .../protocol/error/DetailedErrorReporter.java | 3 +- .../injector/netty/ChannelInjector.java | 42 +++++++++++-------- .../protocol/reflect/VolatileField.java | 18 ++++---- .../protocol/utility/MinecraftReflection.java | 3 +- 7 files changed, 67 insertions(+), 42 deletions(-) diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/ProtocolConfig.java b/ProtocolLib/src/main/java/com/comphenix/protocol/ProtocolConfig.java index 76bf8ebc..a2cec719 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/ProtocolConfig.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/ProtocolConfig.java @@ -2,16 +2,16 @@ * ProtocolLib - Bukkit server library that allows access to the Minecraft protocol. * Copyright (C) 2012 Kristian S. Stangeland * - * This program is free software; you can redistribute it and/or modify it under the terms of the - * GNU General Public License as published by the Free Software Foundation; either version 2 of + * This program is free software; you can redistribute it and/or modify it under the terms of the + * GNU General Public License as published by the Free Software Foundation; either version 2 of * the License, or (at your option) any later version. * - * This program is distributed in the hope that it will be useful, but WITHOUT ANY WARRANTY; - * without even the implied warranty of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. + * This program is distributed in the hope that it will be useful, but WITHOUT ANY WARRANTY; + * without even the implied warranty of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. * See the GNU General Public License for more details. * - * You should have received a copy of the GNU General Public License along with this program; - * if not, write to the Free Software Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA + * You should have received a copy of the GNU General Public License along with this program; + * if not, write to the Free Software Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA * 02111-1307 USA */ @@ -44,7 +44,7 @@ public class ProtocolConfig { private static final String METRICS_ENABLED = "metrics"; - private static final String IGNORE_VERSION_CHECK = "ignore version check"; + private static final String IGNORE_VERSION_CHECK = "ignore version check"; private static final String BACKGROUND_COMPILER_ENABLED = "background compiler"; private static final String DEBUG_MODE_ENABLED = "debug"; @@ -167,10 +167,10 @@ public class ProtocolConfig { config.options().copyDefaults(true); plugin.saveDefaultConfig(); plugin.reloadConfig(); - loadingSections = false; + loadingSections = false; // Inform the user - System.out.println("[ProtocolLib] Created default configuration."); + ProtocolLibrary.log("Created default configuration."); } } @@ -373,11 +373,11 @@ public class ProtocolConfig { /** * Set the last time we updated, in seconds since 1970.01.01 00:00. *

- * Note that this is not considered to modify the configuration, so the modification count + * Note that this is not considered to modify the configuration, so the modification count * will not be incremented. * @param lastTimeSeconds - new last update time. */ - public void setAutoLastTime(long lastTimeSeconds) { + public void setAutoLastTime(long lastTimeSeconds) { this.valuesChanged = true; this.lastUpdateTime = lastTimeSeconds; } @@ -420,7 +420,7 @@ public class ProtocolConfig { // Default hook if nothing has been set PlayerInjectHooks hook = getDefaultMethod(); - if (text != null) + if (text != null) hook = PlayerInjectHooks.valueOf(text.toUpperCase().replace(" ", "_")); return hook; } diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/ProtocolLibrary.java b/ProtocolLib/src/main/java/com/comphenix/protocol/ProtocolLibrary.java index 6768a0fb..089b36cb 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/ProtocolLibrary.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/ProtocolLibrary.java @@ -19,6 +19,7 @@ package com.comphenix.protocol; import java.io.File; import java.io.IOException; +import java.text.MessageFormat; import java.util.Set; import java.util.logging.Handler; import java.util.logging.Level; @@ -140,7 +141,7 @@ public class ProtocolLibrary extends JavaPlugin { private int configExpectedMod = -1; // Logger - private Logger logger; + private static Logger logger; private Handler redirectHandler; // Commands @@ -652,4 +653,14 @@ public class ProtocolLibrary extends JavaPlugin { public static ListeningScheduledExecutorService getExecutorSync() { return executorSync; } + + // ---- Logging + + public static void log(Level level, String message, Object... args) { + logger.log(level, MessageFormat.format(message, args)); + } + + public static void log(String message, Object... args) { + log(Level.INFO, message, args); + } } diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/async/AsyncMarker.java b/ProtocolLib/src/main/java/com/comphenix/protocol/async/AsyncMarker.java index e919c5a1..5bcd077b 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/async/AsyncMarker.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/async/AsyncMarker.java @@ -24,9 +24,11 @@ import java.lang.reflect.Method; import java.util.Iterator; import java.util.List; import java.util.concurrent.atomic.AtomicInteger; +import java.util.logging.Level; import com.comphenix.protocol.PacketStream; import com.comphenix.protocol.PacketType; +import com.comphenix.protocol.ProtocolLibrary; import com.comphenix.protocol.events.NetworkMarker; import com.comphenix.protocol.events.PacketEvent; import com.comphenix.protocol.injector.PrioritizedListener; @@ -426,7 +428,7 @@ public class AsyncMarker implements Serializable, Comparable { // Incoming chat packets are async only if they aren't commands return ! content.startsWith("/"); } else { - System.err.println("[ProtocolLib] Failed to determine contents of incoming chat packet!"); + ProtocolLibrary.log(Level.WARNING, "Failed to determine contents of incoming chat packet!"); alwaysSync = true; } } else { @@ -434,7 +436,7 @@ public class AsyncMarker implements Serializable, Comparable { return false; } } else { - System.err.println("[ProtocolLib] Cannot determine asynchronous state of packets!"); + ProtocolLibrary.log(Level.WARNING, "Cannot determine asynchronous state of packets!"); alwaysSync = true; } } diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/error/DetailedErrorReporter.java b/ProtocolLib/src/main/java/com/comphenix/protocol/error/DetailedErrorReporter.java index 16eb4ada..9593c4f7 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/error/DetailedErrorReporter.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/error/DetailedErrorReporter.java @@ -35,6 +35,7 @@ import org.apache.commons.lang.builder.ToStringStyle; import org.bukkit.Bukkit; import org.bukkit.plugin.Plugin; +import com.comphenix.protocol.ProtocolLibrary; import com.comphenix.protocol.collections.ExpireHashMap; import com.comphenix.protocol.error.Report.ReportBuilder; import com.comphenix.protocol.events.PacketAdapter; @@ -456,7 +457,7 @@ public class DetailedErrorReporter implements ErrorReporter { apacheCommonsMissing = true; } catch (Exception e) { // Don't use the error logger to log errors in error logging (that could lead to infinite loops) - System.err.print("[ProtocolLib] Warning: Cannot convert to a String with Apache: " + e.getMessage()); + ProtocolLibrary.log(Level.WARNING, "Cannot convert to a String with Apache: " + e.getMessage()); } // Use our custom object printer instead diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/injector/netty/ChannelInjector.java b/ProtocolLib/src/main/java/com/comphenix/protocol/injector/netty/ChannelInjector.java index 93e680aa..8ab08bdb 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/injector/netty/ChannelInjector.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/injector/netty/ChannelInjector.java @@ -14,6 +14,7 @@ import io.netty.handler.codec.MessageToByteEncoder; import io.netty.util.concurrent.GenericFutureListener; import io.netty.util.internal.TypeParameterMatcher; +import java.lang.ref.WeakReference; import java.lang.reflect.InvocationTargetException; import java.net.Socket; import java.net.SocketAddress; @@ -84,9 +85,10 @@ class ChannelInjector extends ByteToMessageDecoder implements Injector { // The factory that created this injector private InjectionFactory factory; - // The player, or temporary player - private Player player; - private Player updated; + // The player + private WeakReference player; + private WeakReference updated; + private String playerName; // The player connection private Object playerConnection; @@ -149,7 +151,7 @@ class ChannelInjector extends ByteToMessageDecoder implements Injector { * @param factory - the factory that created this injector */ public ChannelInjector(Player player, Object networkManager, Channel channel, ChannelListener channelListener, InjectionFactory factory) { - this.player = Preconditions.checkNotNull(player, "player cannot be NULL"); + this.player = new WeakReference(Preconditions.checkNotNull(player, "player cannot be NULL")); this.networkManager = Preconditions.checkNotNull(networkManager, "networkMananger cannot be NULL"); this.originalChannel = Preconditions.checkNotNull(channel, "channel cannot be NULL"); this.channelListener = Preconditions.checkNotNull(channelListener, "channelListener cannot be NULL"); @@ -486,6 +488,11 @@ class ChannelInjector extends ByteToMessageDecoder implements Injector { // Reset queue finishQueue.clear(); + if (player.get() == null) { + ProtocolLibrary.log("Failed to intercept client packet for {0}: Invalid player.", playerName); + return; + } + for (ListIterator it = packets.listIterator(); it.hasNext(); ) { Object input = it.next(); Class packetClass = input.getClass(); @@ -498,6 +505,7 @@ class ChannelInjector extends ByteToMessageDecoder implements Injector { byteBuffer.resetReaderIndex(); marker = new NettyNetworkMarker(ConnectionSide.CLIENT_SIDE, getBytes(byteBuffer)); } + PacketEvent output = channelListener.onPacketReceiving(this, input, marker); // Handle packet changes @@ -674,7 +682,7 @@ class ChannelInjector extends ByteToMessageDecoder implements Injector { */ private Object getPlayerConnection() { if (playerConnection == null) { - playerConnection = MinecraftFields.getPlayerConnection(player); + playerConnection = MinecraftFields.getPlayerConnection(player.get()); } return playerConnection; } @@ -693,7 +701,7 @@ class ChannelInjector extends ByteToMessageDecoder implements Injector { @Override public Player getPlayer() { - return player; + return player.get(); } /** @@ -702,7 +710,7 @@ class ChannelInjector extends ByteToMessageDecoder implements Injector { */ @Override public void setPlayer(Player player) { - this.player = player; + this.player = new WeakReference(Preconditions.checkNotNull(player, "player cannot be null")); } /** @@ -711,7 +719,7 @@ class ChannelInjector extends ByteToMessageDecoder implements Injector { */ @Override public void setUpdatedPlayer(Player updated) { - this.updated = updated; + this.updated = new WeakReference(Preconditions.checkNotNull(updated, "updated cannot be null"));; } @Override @@ -752,7 +760,7 @@ class ChannelInjector extends ByteToMessageDecoder implements Injector { // we end up with a deadlock. The main thread is waiting for the worker thread to process the task, and // the worker thread is waiting for the main thread to finish executing PlayerQuitEvent. // - // TLDR: Concurrenty is hard. + // TLDR: Concurrency is hard. executeInChannelThread(new Runnable() { @Override public void run() { @@ -767,14 +775,14 @@ class ChannelInjector extends ByteToMessageDecoder implements Injector { }); // Clear cache - factory.invalidate(player); + factory.invalidate(player.get()); } } - // dmulloy2 - clear player instances - // Should fix memory leaks - this.player = null; - this.updated = null; + // Clear player references + // Should help fix memory leaks + player.clear(); + updated.clear(); } /** @@ -845,12 +853,12 @@ class ChannelInjector extends ByteToMessageDecoder implements Injector { @Override public Player getPlayer() { - return injector.player; + return injector.player.get(); } @Override public Player getUpdatedPlayer() { - return injector.updated; + return injector.updated.get(); } @Override @@ -860,7 +868,7 @@ class ChannelInjector extends ByteToMessageDecoder implements Injector { @Override public void setUpdatedPlayer(Player updatedPlayer) { - injector.player = updatedPlayer; + injector.player = new WeakReference(updatedPlayer); } public ChannelInjector getChannelInjector() { diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/reflect/VolatileField.java b/ProtocolLib/src/main/java/com/comphenix/protocol/reflect/VolatileField.java index a3f8ea84..b78dc520 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/reflect/VolatileField.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/reflect/VolatileField.java @@ -2,23 +2,25 @@ * ProtocolLib - Bukkit server library that allows access to the Minecraft protocol. * Copyright (C) 2012 Kristian S. Stangeland * - * This program is free software; you can redistribute it and/or modify it under the terms of the - * GNU General Public License as published by the Free Software Foundation; either version 2 of + * This program is free software; you can redistribute it and/or modify it under the terms of the + * GNU General Public License as published by the Free Software Foundation; either version 2 of * the License, or (at your option) any later version. * - * This program is distributed in the hope that it will be useful, but WITHOUT ANY WARRANTY; - * without even the implied warranty of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. + * This program is distributed in the hope that it will be useful, but WITHOUT ANY WARRANTY; + * without even the implied warranty of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. * See the GNU General Public License for more details. * - * You should have received a copy of the GNU General Public License along with this program; - * if not, write to the Free Software Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA + * You should have received a copy of the GNU General Public License along with this program; + * if not, write to the Free Software Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA * 02111-1307 USA */ package com.comphenix.protocol.reflect; import java.lang.reflect.Field; +import java.util.logging.Level; +import com.comphenix.protocol.ProtocolLibrary; import com.comphenix.protocol.reflect.accessors.Accessors; import com.comphenix.protocol.reflect.accessors.FieldAccessor; import com.google.common.base.Objects; @@ -183,8 +185,8 @@ public class VolatileField { currentSet = false; } else { // This can be a bad sign - System.out.println(String.format("[ProtocolLib] Unable to switch %s to %s. Expected %s but got %s.", - getField().toGenericString(), previous, current, getValue())); + ProtocolLibrary.log(Level.WARNING, "Unable to switch %s to %s. Expected %s but got %s.", + getField().toGenericString(), previous, current, getValue()); } } } diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/utility/MinecraftReflection.java b/ProtocolLib/src/main/java/com/comphenix/protocol/utility/MinecraftReflection.java index a41a51e7..fb5af3b9 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/utility/MinecraftReflection.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/utility/MinecraftReflection.java @@ -34,6 +34,7 @@ import java.util.List; import java.util.Map; import java.util.Set; import java.util.concurrent.ConcurrentMap; +import java.util.logging.Level; import java.util.regex.Matcher; import java.util.regex.Pattern; @@ -216,7 +217,7 @@ public class MinecraftReflection { if (MinecraftVersion.SCARY_UPDATE.compareTo(version) <= 0) { // Just assume R1 - it's probably fine packageVersion = "v" + version.getMajor() + "_" + version.getMinor() + "_R1"; - System.err.println("[ProtocolLib] Assuming package version: " + packageVersion); + ProtocolLibrary.log(Level.WARNING, "Assuming package version: " + packageVersion); } }