From 1be94aad784913aa8b85d7733491461b446d69b0 Mon Sep 17 00:00:00 2001 From: Dan Mulloy Date: Mon, 29 May 2017 22:14:08 -0400 Subject: [PATCH] Rework packet type deprecation to actually work properly Also fix compatibility with 1.8.0 --- .../com/comphenix/protocol/PacketType.java | 161 ++++++++++-------- .../ObjectEnum.java => PacketTypeEnum.java} | 98 ++++++----- .../comphenix/protocol/ProtocolLogger.java | 6 + .../injector/netty/NettyProtocolRegistry.java | 9 +- .../injector/packet/PacketRegistry.java | 22 +-- .../protocol/reflect/FuzzyReflection.java | 29 +++- .../injector/PacketFilterManager.java | 8 +- .../injector/netty/ProtocolInjector.java | 39 +++-- .../comphenix/protocol/PacketTypeTest.java | 11 ++ .../protocol/events/PacketContainerTest.java | 8 +- 10 files changed, 222 insertions(+), 169 deletions(-) rename modules/API/src/main/java/com/comphenix/protocol/{reflect/ObjectEnum.java => PacketTypeEnum.java} (56%) diff --git a/modules/API/src/main/java/com/comphenix/protocol/PacketType.java b/modules/API/src/main/java/com/comphenix/protocol/PacketType.java index b865862e..d6fceaaf 100644 --- a/modules/API/src/main/java/com/comphenix/protocol/PacketType.java +++ b/modules/API/src/main/java/com/comphenix/protocol/PacketType.java @@ -1,6 +1,10 @@ package com.comphenix.protocol; import java.io.Serializable; +import java.lang.annotation.ElementType; +import java.lang.annotation.Retention; +import java.lang.annotation.RetentionPolicy; +import java.lang.annotation.Target; import java.util.Arrays; import java.util.Collection; import java.util.List; @@ -16,7 +20,6 @@ import org.bukkit.Bukkit; import com.comphenix.protocol.PacketTypeLookup.ClassLookup; import com.comphenix.protocol.events.ConnectionSide; import com.comphenix.protocol.injector.packet.PacketRegistry; -import com.comphenix.protocol.reflect.ObjectEnum; import com.comphenix.protocol.utility.MinecraftReflection; import com.comphenix.protocol.utility.MinecraftVersion; import com.google.common.base.Objects; @@ -52,7 +55,7 @@ public class PacketType implements Serializable, Cloneable, Comparable { + public static class Client extends PacketTypeEnum { private final static Sender SENDER = Sender.CLIENT; public static final PacketType SET_PROTOCOL = new PacketType(PROTOCOL, SENDER, 0x00, 0x00, "SetProtocol"); @@ -60,7 +63,7 @@ public class PacketType implements Serializable, Cloneable, Comparable { + public static class Server extends PacketTypeEnum { private final static Sender SENDER = Sender.CLIENT; private final static Server INSTANCE = new Server(); - private Server() { super(PacketType.class); } + private Server() { super(); } public static Server getInstance() { return INSTANCE; @@ -103,7 +106,7 @@ public class PacketType implements Serializable, Cloneable, Comparable { + public static class Server extends PacketTypeEnum { private final static Sender SENDER = Sender.SERVER; public static final PacketType SPAWN_ENTITY = new PacketType(PROTOCOL, SENDER, 0x00, 0x0E, "SpawnEntity"); @@ -192,19 +195,19 @@ public class PacketType implements Serializable, Cloneable, Comparable { + public static class Client extends PacketTypeEnum { private final static Sender SENDER = Sender.CLIENT; public static final PacketType TELEPORT_ACCEPT = new PacketType(PROTOCOL, SENDER, 0x00, 0xF9, "TeleportAccept"); @@ -298,7 +301,7 @@ public class PacketType implements Serializable, Cloneable, Comparable { + public static class Server extends PacketTypeEnum { private final static Sender SENDER = Sender.SERVER; - public static final PacketType SERVER_INFO = new PacketType(PROTOCOL, SENDER, 0x00, 0x00, "ServerInfo").forceAsync(true); + @ForceAsync + public static final PacketType SERVER_INFO = new PacketType(PROTOCOL, SENDER, 0x00, 0x00, "ServerInfo"); public static final PacketType PONG = new PacketType(PROTOCOL, SENDER, 0x01, 0x01, "Pong"); /** - * @deprecated Replaced by {@link #SERVER_INFO} + * @deprecated Renamed to {@link #SERVER_INFO} */ @Deprecated - public static final PacketType OUT_SERVER_INFO = SERVER_INFO.deprecated(); + @ForceAsync + public static final PacketType OUT_SERVER_INFO = SERVER_INFO.clone(); private final static Server INSTANCE = new Server(); // Prevent accidental construction - private Server() { super(PacketType.class); } + private Server() { super(); } public static Sender getSender() { return SENDER; @@ -353,7 +358,7 @@ public class PacketType implements Serializable, Cloneable, Comparable { + public static class Client extends PacketTypeEnum { private final static Sender SENDER = Sender.CLIENT; public static final PacketType START = new PacketType(PROTOCOL, SENDER, 0x00, 0x00, "Start"); @@ -362,7 +367,7 @@ public class PacketType implements Serializable, Cloneable, Comparable { + public static class Server extends PacketTypeEnum { private final static Sender SENDER = Sender.SERVER; public static final PacketType DISCONNECT = new PacketType(PROTOCOL, SENDER, 0x00, 0x00, "Disconnect"); @@ -399,7 +404,7 @@ public class PacketType implements Serializable, Cloneable, Comparable { + public static class Client extends PacketTypeEnum { private final static Sender SENDER = Sender.CLIENT; public static final PacketType START = new PacketType(PROTOCOL, SENDER, 0x00, 0x00, "Start"); @@ -422,7 +427,7 @@ public class PacketType implements Serializable, Cloneable, Comparable { + public static class Server extends PacketTypeEnum { private final static Sender SENDER = Sender.SERVER; public static final PacketType PLAYER_FLYING = PacketType.newLegacy(SENDER, 10); @@ -473,7 +478,7 @@ public class PacketType implements Serializable, Cloneable, Comparable { + public static class Client extends PacketTypeEnum { private final static Sender SENDER = Sender.CLIENT; public static final PacketType LOGIN = PacketType.newLegacy(SENDER, 1); @@ -499,7 +504,7 @@ public class PacketType implements Serializable, Cloneable, Comparable callable = new Callable() { @Override public Boolean call() throws Exception { - ObjectEnum objEnum; + PacketTypeEnum objEnum; // A bit ugly, but performance is critical objEnum = getObjectEnum(type); @@ -920,7 +934,7 @@ public class PacketType implements Serializable, Cloneable, Comparable getObjectEnum(final PacketType type) { + public static PacketTypeEnum getObjectEnum(final PacketType type) { switch (type.getProtocol()) { case HANDSHAKING: return type.isClient() ? Handshake.Client.getInstance() : Handshake.Server.getInstance(); @@ -967,7 +981,6 @@ public class PacketType implements Serializable, Cloneable, Comparable implements Iterable { +public class PacketTypeEnum implements Iterable { // Used to convert between IDs and names - protected BiMap members = HashBiMap.create(); + protected Set members = Sets.newHashSet(); /** - * Registers every declared integer field. - * @param fieldType Field type + * Registers every declared PacketType field. */ - public ObjectEnum(Class fieldType) { - registerAll(fieldType); + public PacketTypeEnum() { + registerAll(); } /** * Registers every public assignable static field as a member. - * @param fieldType Field type */ @SuppressWarnings("unchecked") - protected void registerAll(Class fieldType) { + protected void registerAll() { try { // Register every non-deprecated field for (Field entry : this.getClass().getFields()) { - if (Modifier.isStatic(entry.getModifiers()) && fieldType.isAssignableFrom(entry.getType())) { - T value = (T) entry.get(null); - - if (value == null) - throw new IllegalArgumentException("Field " + entry + " was NULL. Remember to " + - "construct the object after the field has been declared."); - registerMember(value, entry.getName()); + if (Modifier.isStatic(entry.getModifiers()) && PacketType.class.isAssignableFrom(entry.getType())) { + PacketType value = (PacketType) entry.get(null); + if (value == null) { + throw new IllegalArgumentException("Field " + entry.getName() + " was null!"); + } + + value.setName(entry.getName()); + + if (entry.getAnnotation(PacketType.ForceAsync.class) != null) { + value.forceAsync(); + } + + boolean deprecated = entry.getAnnotation(Deprecated.class) != null; + if (deprecated) value.setDeprecated(); + + if (members.contains(value)) { + // Replace potentially deprecated packet types with non-deprecated ones + if (!deprecated) { + members.remove(value); + members.add(value); + } + } else { + members.add(value); + } } } - } catch (IllegalArgumentException e) { - e.printStackTrace(); - } catch (IllegalAccessException e) { - e.printStackTrace(); + } catch (Exception ex) { + ex.printStackTrace(); } } @@ -76,11 +91,14 @@ public class ObjectEnum implements Iterable { * @param name - name of member. * @return TRUE if the member was registered, FALSE otherwise. */ - public boolean registerMember(T instance, String name) { - if (!members.containsKey(instance)) { - members.put(instance, name); + public boolean registerMember(PacketType instance, String name) { + instance.setName(name); + + if (!members.contains(instance)) { + members.add(instance); return true; } + return false; } @@ -89,38 +107,36 @@ public class ObjectEnum implements Iterable { * @param member - the member to check. * @return TRUE if the given member has been registered, FALSE otherwise. */ - public boolean hasMember(T member) { - return members.containsKey(member); + public boolean hasMember(PacketType member) { + return members.contains(member); } /** * Retrieve a member by name, * @param name - name of member to retrieve. * @return The member, or NULL if not found. + * @deprecated Don't use this */ - public T valueOf(String name) { - return members.inverse().get(name); + @Deprecated + public PacketType valueOf(String name) { + for (PacketType member : members) { + if (member.name().equals(name)) + return member; + } + + return null; } - - /** - * Retrieve the name of the given member. - * @param member - the member to retrieve. - * @return Declared name of the member, or NULL if not found. - */ - public String getDeclaredName(T member) { - return members.get(member); - } - + /** * Retrieve every registered member. * @return Enumeration of every value. */ - public Set values() { - return new HashSet(members.keySet()); + public Set values() { + return new HashSet<>(members); } @Override - public Iterator iterator() { - return members.keySet().iterator(); + public Iterator iterator() { + return members.iterator(); } } diff --git a/modules/API/src/main/java/com/comphenix/protocol/ProtocolLogger.java b/modules/API/src/main/java/com/comphenix/protocol/ProtocolLogger.java index 6b33a72e..eea46d76 100644 --- a/modules/API/src/main/java/com/comphenix/protocol/ProtocolLogger.java +++ b/modules/API/src/main/java/com/comphenix/protocol/ProtocolLogger.java @@ -73,4 +73,10 @@ public class ProtocolLogger { log("[Debug] " + message, args); } } + + public static void debug(String message, Throwable ex) { + if (isDebugEnabled()) { + plugin.getLogger().log(Level.WARNING, "[Debug] " + message, ex); + } + } } diff --git a/modules/API/src/main/java/com/comphenix/protocol/injector/netty/NettyProtocolRegistry.java b/modules/API/src/main/java/com/comphenix/protocol/injector/netty/NettyProtocolRegistry.java index 01774a36..ee91f0dd 100644 --- a/modules/API/src/main/java/com/comphenix/protocol/injector/netty/NettyProtocolRegistry.java +++ b/modules/API/src/main/java/com/comphenix/protocol/injector/netty/NettyProtocolRegistry.java @@ -40,8 +40,6 @@ public class NettyProtocolRegistry extends ProtocolRegistry { @Override protected synchronized void initialize() { - ProtocolLogger.debug("Initializing the Netty protocol registry"); // Debug for issue #202 - Object[] protocols = enumProtocol.getEnumConstants(); // ID to Packet class maps @@ -75,8 +73,7 @@ public class NettyProtocolRegistry extends ProtocolRegistry { result.containers.add(new MapContainer(map)); } - for (int i = 0; i < protocols.length; i++) { - Object protocol = protocols[i]; + for (Object protocol : protocols) { Enum enumProtocol = (Enum) protocol; Protocol equivalent = Protocol.fromVanilla(enumProtocol); @@ -103,8 +100,8 @@ public class NettyProtocolRegistry extends ProtocolRegistry { register.serverPackets.add(type); if (sender == Sender.CLIENT) register.clientPackets.add(type); - } catch (IllegalArgumentException ex) { - // Sometimes this happens with fake packets, just ignore it + } catch (Exception ex) { + ProtocolLogger.debug("Encountered an exception associating packet " + type, ex); } } } diff --git a/modules/API/src/main/java/com/comphenix/protocol/injector/packet/PacketRegistry.java b/modules/API/src/main/java/com/comphenix/protocol/injector/packet/PacketRegistry.java index 24aa57d3..c93643ae 100644 --- a/modules/API/src/main/java/com/comphenix/protocol/injector/packet/PacketRegistry.java +++ b/modules/API/src/main/java/com/comphenix/protocol/injector/packet/PacketRegistry.java @@ -171,16 +171,7 @@ public class PacketRegistry { initialize(); NETTY.synchronize(); - Set types = new HashSet<>(); - - // Filter out unsupported packets - for (PacketType type : NETTY.getServerPackets()) { - if (!type.isDeprecated()) { - types.add(type); - } - } - - return types; + return NETTY.getServerPackets(); } /** @@ -207,16 +198,7 @@ public class PacketRegistry { initialize(); NETTY.synchronize(); - Set types = new HashSet<>(); - - // Filter out unsupported packets - for (PacketType type : NETTY.getClientPackets()) { - if (!type.isDeprecated()) { - types.add(type); - } - } - - return types; + return NETTY.getClientPackets(); } /** diff --git a/modules/API/src/main/java/com/comphenix/protocol/reflect/FuzzyReflection.java b/modules/API/src/main/java/com/comphenix/protocol/reflect/FuzzyReflection.java index df3ec791..bf9626ab 100644 --- a/modules/API/src/main/java/com/comphenix/protocol/reflect/FuzzyReflection.java +++ b/modules/API/src/main/java/com/comphenix/protocol/reflect/FuzzyReflection.java @@ -17,10 +17,7 @@ package com.comphenix.protocol.reflect; -import java.lang.reflect.Constructor; -import java.lang.reflect.Field; -import java.lang.reflect.Method; -import java.lang.reflect.Modifier; +import java.lang.reflect.*; import java.util.ArrayList; import java.util.Arrays; import java.util.LinkedHashSet; @@ -421,6 +418,30 @@ public class FuzzyReflection { return fields; } + + /** + * Retrieves a field with a given type and parameters. This is most useful + * when dealing with Collections. + * + * @param fieldType Type of the field + * @param params Variable length array of type parameters + * @return The field + * + * @throws IllegalArgumentException If the field cannot be found + */ + public Field getParameterizedField(Class fieldType, Class... params) { + for (Field field : getFields()) { + if (field.getType().equals(fieldType)) { + Type type = field.getGenericType(); + if (type instanceof ParameterizedType) { + if (Arrays.equals(((ParameterizedType) type).getActualTypeArguments(), params)) + return field; + } + } + } + + throw new IllegalArgumentException("Unable to find a field with type " + fieldType + " and params " + Arrays.toString(params)); + } /** * Retrieve the first field that matches. diff --git a/modules/ProtocolLib/src/main/java/com/comphenix/protocol/injector/PacketFilterManager.java b/modules/ProtocolLib/src/main/java/com/comphenix/protocol/injector/PacketFilterManager.java index 2102c7b3..dbfc4dc8 100644 --- a/modules/ProtocolLib/src/main/java/com/comphenix/protocol/injector/PacketFilterManager.java +++ b/modules/ProtocolLib/src/main/java/com/comphenix/protocol/injector/PacketFilterManager.java @@ -83,8 +83,8 @@ public final class PacketFilterManager implements ListenerInvoker, InternalManag new ReportType("%s doesn't depend on ProtocolLib. Check that its plugin.yml has a 'depend' directive."); // Registering packet IDs that are not supported - public static final ReportType REPORT_UNSUPPORTED_SERVER_PACKET_ID = new ReportType("[%s] Unsupported server packet ID in current Minecraft version: %s"); - public static final ReportType REPORT_UNSUPPORTED_CLIENT_PACKET_ID = new ReportType("[%s] Unsupported client packet ID in current Minecraft version: %s"); + public static final ReportType REPORT_UNSUPPORTED_SERVER_PACKET = new ReportType("[%s] Unsupported server packet in current Minecraft version: %s"); + public static final ReportType REPORT_UNSUPPORTED_CLIENT_PACKET = new ReportType("[%s] Unsupported client packet in current Minecraft version: %s"); // Problems injecting and uninjecting players public static final ReportType REPORT_CANNOT_UNINJECT_PLAYER = new ReportType("Unable to uninject net handler for player."); @@ -624,7 +624,7 @@ public final class PacketFilterManager implements ListenerInvoker, InternalManag playerInjection.addPacketHandler(type, listener.getSendingWhitelist().getOptions()); else reporter.reportWarning(this, - Report.newBuilder(REPORT_UNSUPPORTED_SERVER_PACKET_ID).messageParam(PacketAdapter.getPluginName(listener), type) + Report.newBuilder(REPORT_UNSUPPORTED_SERVER_PACKET).messageParam(PacketAdapter.getPluginName(listener), type) ); } @@ -634,7 +634,7 @@ public final class PacketFilterManager implements ListenerInvoker, InternalManag packetInjector.addPacketHandler(type, listener.getReceivingWhitelist().getOptions()); else reporter.reportWarning(this, - Report.newBuilder(REPORT_UNSUPPORTED_CLIENT_PACKET_ID).messageParam(PacketAdapter.getPluginName(listener), type) + Report.newBuilder(REPORT_UNSUPPORTED_CLIENT_PACKET).messageParam(PacketAdapter.getPluginName(listener), type) ); } } diff --git a/modules/ProtocolLib/src/main/java/com/comphenix/protocol/injector/netty/ProtocolInjector.java b/modules/ProtocolLib/src/main/java/com/comphenix/protocol/injector/netty/ProtocolInjector.java index 3c1c6984..f27ed241 100644 --- a/modules/ProtocolLib/src/main/java/com/comphenix/protocol/injector/netty/ProtocolInjector.java +++ b/modules/ProtocolLib/src/main/java/com/comphenix/protocol/injector/netty/ProtocolInjector.java @@ -29,6 +29,7 @@ import org.bukkit.entity.Player; import org.bukkit.plugin.Plugin; import com.comphenix.protocol.PacketType; +import com.comphenix.protocol.ProtocolLogger; import com.comphenix.protocol.concurrency.PacketTypeSet; import com.comphenix.protocol.error.ErrorReporter; import com.comphenix.protocol.error.Report; @@ -132,12 +133,15 @@ public class ProtocolInjector implements ChannelListener { if (serverConnection != null) { break; } - } catch (Exception e) { - // Try the next though - e.printStackTrace(); + } catch (Exception ex) { + ProtocolLogger.debug("Encountered an exception invoking " + method, ex); } } + if (serverConnection == null) { + throw new ReflectiveOperationException("Failed to obtain server connection"); + } + // Handle connected channels final ChannelInboundHandler endInitProtocol = new ChannelInitializer() { @Override @@ -153,8 +157,8 @@ public class ProtocolInjector implements ChannelListener { injectionFactory.fromChannel(channel, ProtocolInjector.this, playerFactory).inject(); } } - } catch (Exception e) { - reporter.reportDetailed(ProtocolInjector.this, Report.newBuilder(REPORT_CANNOT_INJECT_INCOMING_CHANNEL).messageParam(channel).error(e)); + } catch (Exception ex) { + reporter.reportDetailed(ProtocolInjector.this, Report.newBuilder(REPORT_CANNOT_INJECT_INCOMING_CHANNEL).messageParam(channel).error(ex)); } } }; @@ -183,21 +187,22 @@ public class ProtocolInjector implements ChannelListener { FuzzyReflection fuzzy = FuzzyReflection.fromObject(serverConnection, true); try { - List fields = fuzzy.getFieldListByType(List.class); - for (Field field : fields) { - ParameterizedType param = (ParameterizedType) field.getGenericType(); - if (param.getActualTypeArguments()[0].equals(MinecraftReflection.getNetworkManagerClass())) { - field.setAccessible(true); - networkManagers = (List) field.get(serverConnection); - } - } + Field field = fuzzy.getParameterizedField(List.class, MinecraftReflection.getNetworkManagerClass()); + field.setAccessible(true); + + networkManagers = (List) field.get(serverConnection); } catch (Exception ex) { - networkManagers = (List) fuzzy.getMethodByParameters("getNetworkManagers", List.class, serverConnection.getClass()) - .invoke(null, serverConnection); + ProtocolLogger.debug("Encountered an exception checking list fields", ex); + + Method method = fuzzy.getMethodByParameters("getNetworkManagers", List.class, + new Class[] { serverConnection.getClass() }); + method.setAccessible(true); + + networkManagers = (List) method.invoke(null, serverConnection); } if (networkManagers == null) { - throw new RuntimeException("Failed to obtain list of network managers."); + throw new ReflectiveOperationException("Failed to obtain list of network managers"); } // Insert ProtocolLib's connection interceptor @@ -376,7 +381,7 @@ public class ProtocolInjector implements ChannelListener { @Override public void addPacketHandler(PacketType type, Set options) { - if (options != null && !type.forceAsync() && !options.contains(ListenerOptions.ASYNC)) + if (!type.isAsyncForced() && (options == null || !options.contains(ListenerOptions.ASYNC))) mainThreadFilters.addType(type); super.addPacketHandler(type, options); } diff --git a/modules/ProtocolLib/src/test/java/com/comphenix/protocol/PacketTypeTest.java b/modules/ProtocolLib/src/test/java/com/comphenix/protocol/PacketTypeTest.java index eb8dc56f..ea8102cb 100644 --- a/modules/ProtocolLib/src/test/java/com/comphenix/protocol/PacketTypeTest.java +++ b/modules/ProtocolLib/src/test/java/com/comphenix/protocol/PacketTypeTest.java @@ -23,6 +23,7 @@ import java.util.TreeMap; import com.comphenix.protocol.PacketType.Protocol; import com.comphenix.protocol.PacketType.Sender; +import com.comphenix.protocol.injector.packet.PacketRegistry; import net.minecraft.server.v1_12_R1.EnumProtocol; import net.minecraft.server.v1_12_R1.EnumProtocolDirection; @@ -54,6 +55,16 @@ public class PacketTypeTest { assertEquals(PacketLoginInStart.class, PacketType.Login.Client.START.getPacketClass()); } + @Test + public void testDeprecation() { + assertTrue("Packet isn't properly deprecated", PacketType.Status.Server.OUT_SERVER_INFO.isDeprecated()); + assertTrue("Deprecated packet isn't properly included", + PacketRegistry.getServerPacketTypes().contains(PacketType.Status.Server.OUT_SERVER_INFO)); + assertFalse("Packet isn't properly deprecated", PacketType.Play.Server.CHAT.isDeprecated()); + assertEquals("Deprecated packets aren't equal", PacketType.Status.Server.OUT_SERVER_INFO, + PacketType.Status.Server.SERVER_INFO); + } + @Test @SuppressWarnings("unchecked") public void ensureTypesAreCorrect() throws Exception { diff --git a/modules/ProtocolLib/src/test/java/com/comphenix/protocol/events/PacketContainerTest.java b/modules/ProtocolLib/src/test/java/com/comphenix/protocol/events/PacketContainerTest.java index 69ef7dfe..65f56e12 100644 --- a/modules/ProtocolLib/src/test/java/com/comphenix/protocol/events/PacketContainerTest.java +++ b/modules/ProtocolLib/src/test/java/com/comphenix/protocol/events/PacketContainerTest.java @@ -527,7 +527,7 @@ public class PacketContainerTest { private static final List BLACKLISTED = Util.asList( PacketType.Play.Client.CUSTOM_PAYLOAD, PacketType.Play.Server.CUSTOM_PAYLOAD, - PacketType.Play.Server.SET_COOLDOWN, PacketType.Play.Server.REL_ENTITY_MOVE_LOOK + PacketType.Play.Server.SET_COOLDOWN ); @Test @@ -544,9 +544,9 @@ public class PacketContainerTest { try { PacketContainer constructed = new PacketContainer(type); -// if (!registered) { -// fail("Expected IllegalArgumentException(Packet " + type + " not registered)"); -// } + if (!registered) { + fail("Expected IllegalArgumentException(Packet " + type + " not registered)"); + } // Initialize default values constructed.getModifier().writeDefaults();