From af33a2ab41199f7c564a5ca9f919e91822532cf4 Mon Sep 17 00:00:00 2001 From: Pasqual Koschmieder Date: Wed, 25 Oct 2023 02:56:38 +0200 Subject: [PATCH] fix invalid packet types due to state mismatch when calling packet events (#2568) --- .../protocol/injector/ListenerInvoker.java | 2 + .../protocol/injector/StructureCache.java | 2 + .../protocol/injector/netty/Injector.java | 16 ++- .../netty/channel/ChannelProtocolUtil.java | 133 ++++++++++++++++++ .../injector/netty/channel/EmptyInjector.java | 3 +- .../netty/channel/NettyChannelInjector.java | 16 +-- .../netty/manager/NetworkManagerInjector.java | 8 +- .../injector/packet/PacketRegistry.java | 26 +++- .../protocol/reflect/ObjectWriter.java | 17 --- .../channel/ChannelProtocolUtilTest.java | 33 +++++ 10 files changed, 218 insertions(+), 38 deletions(-) create mode 100644 src/main/java/com/comphenix/protocol/injector/netty/channel/ChannelProtocolUtil.java create mode 100644 src/test/java/com/comphenix/protocol/injector/netty/channel/ChannelProtocolUtilTest.java diff --git a/src/main/java/com/comphenix/protocol/injector/ListenerInvoker.java b/src/main/java/com/comphenix/protocol/injector/ListenerInvoker.java index f30863ba..ef50c278 100644 --- a/src/main/java/com/comphenix/protocol/injector/ListenerInvoker.java +++ b/src/main/java/com/comphenix/protocol/injector/ListenerInvoker.java @@ -46,6 +46,8 @@ public interface ListenerInvoker { * * @param packet - the packet. * @return The packet type. + * @deprecated use {@link com.comphenix.protocol.injector.packet.PacketRegistry#getPacketType(PacketType.Protocol, Class)} instead. */ + @Deprecated PacketType getPacketType(Object packet); } diff --git a/src/main/java/com/comphenix/protocol/injector/StructureCache.java b/src/main/java/com/comphenix/protocol/injector/StructureCache.java index 5b5f2a1e..c8c41fd2 100644 --- a/src/main/java/com/comphenix/protocol/injector/StructureCache.java +++ b/src/main/java/com/comphenix/protocol/injector/StructureCache.java @@ -118,7 +118,9 @@ public class StructureCache { * * @param packetType - packet type. * @return A structure modifier. + * @deprecated use {@link #getStructure(PacketType)} instead. */ + @Deprecated public static StructureModifier getStructure(Class packetType) { // Get the ID from the class PacketType type = PacketRegistry.getPacketType(packetType); diff --git a/src/main/java/com/comphenix/protocol/injector/netty/Injector.java b/src/main/java/com/comphenix/protocol/injector/netty/Injector.java index 07ee1a95..9614ccd8 100644 --- a/src/main/java/com/comphenix/protocol/injector/netty/Injector.java +++ b/src/main/java/com/comphenix/protocol/injector/netty/Injector.java @@ -1,5 +1,6 @@ package com.comphenix.protocol.injector.netty; +import com.comphenix.protocol.PacketType; import com.comphenix.protocol.PacketType.Protocol; import com.comphenix.protocol.events.NetworkMarker; import org.bukkit.entity.Player; @@ -49,8 +50,21 @@ public interface Injector { * Retrieve the current protocol state. * * @return The current protocol. + * @deprecated use {@link #getCurrentProtocol(PacketType.Sender)} instead. */ - Protocol getCurrentProtocol(); + @Deprecated + default Protocol getCurrentProtocol() { + return this.getCurrentProtocol(PacketType.Sender.SERVER); + } + + /** + * Retrieve the current protocol state. Note that since 1.20.2 the client and server direction can be in different + * protocol states. + * + * @param sender the side for which the state should be resolved. + * @return The current protocol. + */ + Protocol getCurrentProtocol(PacketType.Sender sender); /** * Retrieve the network marker associated with a given packet. diff --git a/src/main/java/com/comphenix/protocol/injector/netty/channel/ChannelProtocolUtil.java b/src/main/java/com/comphenix/protocol/injector/netty/channel/ChannelProtocolUtil.java new file mode 100644 index 00000000..3ab12265 --- /dev/null +++ b/src/main/java/com/comphenix/protocol/injector/netty/channel/ChannelProtocolUtil.java @@ -0,0 +1,133 @@ +package com.comphenix.protocol.injector.netty.channel; + +import com.comphenix.protocol.PacketType; +import com.comphenix.protocol.reflect.FuzzyReflection; +import com.comphenix.protocol.reflect.accessors.Accessors; +import com.comphenix.protocol.reflect.accessors.FieldAccessor; +import com.comphenix.protocol.reflect.fuzzy.FuzzyFieldContract; +import com.comphenix.protocol.utility.MinecraftReflection; +import io.netty.channel.Channel; +import io.netty.util.AttributeKey; + +import java.lang.reflect.Field; +import java.lang.reflect.Modifier; +import java.util.List; +import java.util.function.BiFunction; + +@SuppressWarnings("unchecked") +final class ChannelProtocolUtil { + + public static final BiFunction PROTOCOL_RESOLVER; + + static { + Class networkManagerClass = MinecraftReflection.getNetworkManagerClass(); + List attributeKeys = FuzzyReflection.fromClass(networkManagerClass, true).getFieldList(FuzzyFieldContract.newBuilder() + .typeExact(AttributeKey.class) + .requireModifier(Modifier.STATIC) + .declaringClassExactType(networkManagerClass) + .build()); + + BiFunction baseResolver = null; + if (attributeKeys.size() == 1) { + // if there is only one attribute key we can assume it's the correct one (1.8 - 1.20.1) + Object protocolKey = Accessors.getFieldAccessor(attributeKeys.get(0)).get(null); + baseResolver = new Pre1_20_2DirectResolver((AttributeKey) protocolKey); + } else if (attributeKeys.size() > 1) { + // most likely 1.20.2+: 1 protocol key per protocol direction + AttributeKey serverBoundKey = null; + AttributeKey clientBoundKey = null; + + for (Field keyField : attributeKeys) { + AttributeKey key = (AttributeKey) Accessors.getFieldAccessor(keyField).get(null); + if (key.name().equals("protocol")) { + // legacy (pre 1.20.2 name) - fall back to the old behaviour + baseResolver = new Pre1_20_2DirectResolver(key); + break; + } + + if (key.name().contains("protocol")) { + // one of the two protocol keys for 1.20.2 + if (key.name().contains("server")) { + serverBoundKey = key; + } else { + clientBoundKey = key; + } + } + } + + if (baseResolver == null) { + if ((serverBoundKey == null || clientBoundKey == null)) { + // neither pre 1.20.2 key nor 1.20.2+ keys are available + throw new ExceptionInInitializerError("Unable to resolve protocol state attribute keys"); + } else { + baseResolver = new Post1_20_2WrappedResolver(serverBoundKey, clientBoundKey); + } + } + } else { + throw new ExceptionInInitializerError("Unable to resolve protocol state attribute key(s)"); + } + + // decorate the base resolver by wrapping its return value into our packet type value + PROTOCOL_RESOLVER = baseResolver.andThen(nmsProtocol -> PacketType.Protocol.fromVanilla((Enum) nmsProtocol)); + } + + private static final class Pre1_20_2DirectResolver implements BiFunction { + + private final AttributeKey attributeKey; + + public Pre1_20_2DirectResolver(AttributeKey attributeKey) { + this.attributeKey = attributeKey; + } + + @Override + public Object apply(Channel channel, PacketType.Sender sender) { + return channel.attr(this.attributeKey).get(); + } + } + + private static final class Post1_20_2WrappedResolver implements BiFunction { + + private final AttributeKey serverBoundKey; + private final AttributeKey clientBoundKey; + + // lazy initialized when needed + private FieldAccessor protocolAccessor; + + public Post1_20_2WrappedResolver(AttributeKey serverBoundKey, AttributeKey clientBoundKey) { + this.serverBoundKey = serverBoundKey; + this.clientBoundKey = clientBoundKey; + } + + @Override + public Object apply(Channel channel, PacketType.Sender sender) { + AttributeKey key = this.getKeyForSender(sender); + Object codecData = channel.attr(key).get(); + if (codecData == null) { + return null; + } + + FieldAccessor protocolAccessor = this.getProtocolAccessor(codecData.getClass()); + return protocolAccessor.get(codecData); + } + + private AttributeKey getKeyForSender(PacketType.Sender sender) { + switch (sender) { + case SERVER: + return this.clientBoundKey; + case CLIENT: + return this.serverBoundKey; + default: + throw new IllegalArgumentException("Illegal packet sender " + sender.name()); + } + } + + private FieldAccessor getProtocolAccessor(Class codecClass) { + if (this.protocolAccessor == null) { + Class enumProtocolClass = MinecraftReflection.getEnumProtocolClass(); + this.protocolAccessor = Accessors.getFieldAccessor(codecClass, enumProtocolClass, true); + } + + return this.protocolAccessor; + } + } +} diff --git a/src/main/java/com/comphenix/protocol/injector/netty/channel/EmptyInjector.java b/src/main/java/com/comphenix/protocol/injector/netty/channel/EmptyInjector.java index 8a19188a..7b60d6f7 100644 --- a/src/main/java/com/comphenix/protocol/injector/netty/channel/EmptyInjector.java +++ b/src/main/java/com/comphenix/protocol/injector/netty/channel/EmptyInjector.java @@ -1,5 +1,6 @@ package com.comphenix.protocol.injector.netty.channel; +import com.comphenix.protocol.PacketType; import com.comphenix.protocol.PacketType.Protocol; import com.comphenix.protocol.events.NetworkMarker; import com.comphenix.protocol.injector.netty.Injector; @@ -42,7 +43,7 @@ final class EmptyInjector implements Injector { } @Override - public Protocol getCurrentProtocol() { + public Protocol getCurrentProtocol(PacketType.Sender sender) { return Protocol.HANDSHAKING; } diff --git a/src/main/java/com/comphenix/protocol/injector/netty/channel/NettyChannelInjector.java b/src/main/java/com/comphenix/protocol/injector/netty/channel/NettyChannelInjector.java index eaa1a898..7dab3461 100644 --- a/src/main/java/com/comphenix/protocol/injector/netty/channel/NettyChannelInjector.java +++ b/src/main/java/com/comphenix/protocol/injector/netty/channel/NettyChannelInjector.java @@ -2,8 +2,6 @@ package com.comphenix.protocol.injector.netty.channel; import java.lang.reflect.Field; import java.lang.reflect.Modifier; -import java.util.Collections; -import java.util.HashSet; import java.util.Map; import java.util.Map.Entry; import java.util.NoSuchElementException; @@ -110,7 +108,6 @@ public class NettyChannelInjector implements Injector { // lazy initialized fields, if we don't need them we don't bother about them private Object playerConnection; - private FieldAccessor protocolAccessor; public NettyChannelInjector( Player player, @@ -322,17 +319,8 @@ public class NettyChannelInjector implements Injector { } @Override - public Protocol getCurrentProtocol() { - // ensure that the accessor to the protocol field is available - if (this.protocolAccessor == null) { - this.protocolAccessor = Accessors.getFieldAccessor( - this.networkManager.getClass(), - MinecraftReflection.getEnumProtocolClass(), - true); - } - - Object nmsProtocol = this.protocolAccessor.get(this.networkManager); - return Protocol.fromVanilla((Enum) nmsProtocol); + public Protocol getCurrentProtocol(PacketType.Sender sender) { + return ChannelProtocolUtil.PROTOCOL_RESOLVER.apply(this.wrappedChannel, sender); } @Override diff --git a/src/main/java/com/comphenix/protocol/injector/netty/manager/NetworkManagerInjector.java b/src/main/java/com/comphenix/protocol/injector/netty/manager/NetworkManagerInjector.java index 68ed9762..f5758cde 100644 --- a/src/main/java/com/comphenix/protocol/injector/netty/manager/NetworkManagerInjector.java +++ b/src/main/java/com/comphenix/protocol/injector/netty/manager/NetworkManagerInjector.java @@ -28,7 +28,6 @@ import com.comphenix.protocol.reflect.accessors.FieldAccessor; import com.comphenix.protocol.reflect.fuzzy.FuzzyFieldContract; import com.comphenix.protocol.reflect.fuzzy.FuzzyMethodContract; import com.comphenix.protocol.utility.MinecraftReflection; -import com.comphenix.protocol.utility.Util; import com.comphenix.protocol.wrappers.Pair; import io.netty.channel.ChannelFuture; import org.bukkit.Server; @@ -93,7 +92,8 @@ public class NetworkManagerInjector implements ChannelListener { Class packetClass = packet.getClass(); if (marker != null || MinecraftReflection.isBundlePacket(packetClass) || outboundListeners.contains(packetClass)) { // wrap packet and construct the event - PacketContainer container = new PacketContainer(PacketRegistry.getPacketType(packetClass), packet); + PacketType.Protocol currentProtocol = injector.getCurrentProtocol(PacketType.Sender.SERVER); + PacketContainer container = new PacketContainer(PacketRegistry.getPacketType(currentProtocol, packetClass), packet); PacketEvent packetEvent = PacketEvent.fromServer(this, container, marker, injector.getPlayer()); // post to all listeners, then return the packet event we constructed @@ -111,7 +111,8 @@ public class NetworkManagerInjector implements ChannelListener { Class packetClass = packet.getClass(); if (marker != null || inboundListeners.contains(packetClass)) { // wrap the packet and construct the event - PacketContainer container = new PacketContainer(PacketRegistry.getPacketType(packetClass), packet); + PacketType.Protocol currentProtocol = injector.getCurrentProtocol(PacketType.Sender.CLIENT); + PacketContainer container = new PacketContainer(PacketRegistry.getPacketType(currentProtocol, packetClass), packet); PacketEvent packetEvent = PacketEvent.fromClient(this, container, marker, injector.getPlayer()); // post to all listeners, then return the packet event we constructed @@ -238,7 +239,6 @@ public class NetworkManagerInjector implements ChannelListener { // just reset to the list we wrapped originally ListeningList ourList = (ListeningList) currentFieldValue; List original = ourList.getOriginal(); - //noinspection SynchronizationOnLocalVariableOrMethodParameter synchronized (original) { // revert the injection from all values of the list ourList.unProcessAll(); diff --git a/src/main/java/com/comphenix/protocol/injector/packet/PacketRegistry.java b/src/main/java/com/comphenix/protocol/injector/packet/PacketRegistry.java index d87b655e..fa55ef85 100644 --- a/src/main/java/com/comphenix/protocol/injector/packet/PacketRegistry.java +++ b/src/main/java/com/comphenix/protocol/injector/packet/PacketRegistry.java @@ -48,7 +48,9 @@ public class PacketRegistry { protected static class Register { // The main lookup table final Map>> typeToClass = new ConcurrentHashMap<>(); + final Map, PacketType> classToType = new ConcurrentHashMap<>(); + final Map, PacketType>> protocolClassToType = new ConcurrentHashMap<>(); volatile Set serverPackets = new HashSet<>(); volatile Set clientPackets = new HashSet<>(); @@ -58,7 +60,10 @@ public class PacketRegistry { public void registerPacket(PacketType type, Class clazz, Sender sender) { typeToClass.put(type, Optional.of(clazz)); + classToType.put(clazz, type); + protocolClassToType.computeIfAbsent(type.getProtocol(), __ -> new ConcurrentHashMap<>()).put(clazz, type); + if (sender == Sender.CLIENT) { clientPackets.add(type); } else { @@ -430,7 +435,9 @@ public class PacketRegistry { * Retrieve the packet type of a given packet. * @param packet - the class of the packet. * @return The packet type, or NULL if not found. + * @deprecated major issues due to packets with shared classes being registered in multiple states. */ + @Deprecated public static PacketType getPacketType(Class packet) { initialize(); @@ -440,7 +447,24 @@ public class PacketRegistry { return REGISTER.classToType.get(packet); } - + + /** + * Retrieve the associated packet type for a packet class in the given protocol state. + * + * @param protocol the protocol state to retrieve the packet from. + * @param packet the class identifying the packet type. + * @return the packet type associated with the given class in the given protocol state, or null if not found. + */ + public static PacketType getPacketType(PacketType.Protocol protocol, Class packet) { + initialize(); + if (MinecraftReflection.isBundlePacket(packet)) { + return PacketType.Play.Server.BUNDLE; + } + + Map, PacketType> classToTypesForProtocol = REGISTER.protocolClassToType.get(protocol); + return classToTypesForProtocol == null ? null : classToTypesForProtocol.get(packet); + } + /** * Retrieve the packet type of a given packet. * @param packet - the class of the packet. diff --git a/src/main/java/com/comphenix/protocol/reflect/ObjectWriter.java b/src/main/java/com/comphenix/protocol/reflect/ObjectWriter.java index 0df5e37c..48d698ba 100644 --- a/src/main/java/com/comphenix/protocol/reflect/ObjectWriter.java +++ b/src/main/java/com/comphenix/protocol/reflect/ObjectWriter.java @@ -17,11 +17,6 @@ package com.comphenix.protocol.reflect; -import com.comphenix.protocol.PacketType; -import com.comphenix.protocol.injector.StructureCache; -import com.comphenix.protocol.injector.packet.PacketRegistry; -import com.comphenix.protocol.utility.MinecraftReflection; -import com.comphenix.protocol.utility.StreamSerializer; import java.lang.reflect.Field; import java.lang.reflect.Modifier; import java.util.HashMap; @@ -46,18 +41,6 @@ public class ObjectWriter { * @return A structure modifier for the given type. */ private StructureModifier getModifier(Class type) { - Class packetClass = MinecraftReflection.getPacketClass(); - - // Handle subclasses of the packet class with our custom structure cache, if possible - if (!type.equals(packetClass) && packetClass.isAssignableFrom(type)) { - // might be a packet, but some packets are not registered (for example PacketPlayInFlying, only the subtypes are present) - PacketType packetType = PacketRegistry.getPacketType(type); - if (packetType != null) { - // packet is present, delegate to the cache - return StructureCache.getStructure(packetType); - } - } - // Create the structure modifier if we haven't already StructureModifier modifier = CACHE.get(type); if (modifier == null) { diff --git a/src/test/java/com/comphenix/protocol/injector/netty/channel/ChannelProtocolUtilTest.java b/src/test/java/com/comphenix/protocol/injector/netty/channel/ChannelProtocolUtilTest.java new file mode 100644 index 00000000..29cb0ee4 --- /dev/null +++ b/src/test/java/com/comphenix/protocol/injector/netty/channel/ChannelProtocolUtilTest.java @@ -0,0 +1,33 @@ +package com.comphenix.protocol.injector.netty.channel; + +import com.comphenix.protocol.BukkitInitialization; +import com.comphenix.protocol.PacketType; +import io.netty.channel.Channel; +import io.netty.channel.local.LocalServerChannel; +import net.minecraft.network.EnumProtocol; +import net.minecraft.network.NetworkManager; +import net.minecraft.network.protocol.EnumProtocolDirection; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.Test; + +public class ChannelProtocolUtilTest { + + @BeforeAll + public static void beforeClass() { + BukkitInitialization.initializeAll(); + } + + @Test + public void testProtocolResolving() { + Channel channel = new LocalServerChannel(); + channel.attr(NetworkManager.e).set(EnumProtocol.e.b(EnumProtocolDirection.a)); // ATTRIBUTE_SERVERBOUND_PROTOCOL -> Protocol.CONFIG.codec(SERVERBOUND) + channel.attr(NetworkManager.f).set(EnumProtocol.b.b(EnumProtocolDirection.b)); // ATTRIBUTE_CLIENTBOUND_PROTOCOL -> Protocol.PLAY.codec(CLIENTBOUND) + + PacketType.Protocol serverBoundProtocol = ChannelProtocolUtil.PROTOCOL_RESOLVER.apply(channel, PacketType.Sender.CLIENT); + Assertions.assertEquals(PacketType.Protocol.CONFIGURATION, serverBoundProtocol); + + PacketType.Protocol clientBoundProtocol = ChannelProtocolUtil.PROTOCOL_RESOLVER.apply(channel, PacketType.Sender.SERVER); + Assertions.assertEquals(PacketType.Protocol.PLAY, clientBoundProtocol); + } +}