Packet filtering for bundled packets in 1.19.4 (#2258)

Since Minecraft 1.19.4, the protocol supports bundling consecutive packets to ensure the client processes them in one tick. However, Packet Events are not called for the individual packets in such a bundle in the current dev build of ProtocolLib. For example, no packet events are currently sent for the ENTITY_METADATA packet when an entity is first spawned as the packet is bundled with the ENTITY_SPAWN packet. However, if the entity metadata is changed later on, the event will be called.
This PR proposes to fix this by unpacking the bundled packets and invoking the packet filtering for each packet.

I also want to briefly explain how the bundling works. A bundle starts with a PACKET_DELIMITER (0x00, net.minecraft.network.protocol.BundleDelimiterPacket) packet followed by all packets that should be bundled and finished with another PACKET_DELIMITER (0x00). Within the Netty pipeline, this sequence is transformed into one synthesized packet found in net.minecraft.network.protocol.game.ClientboundBundlePacket, which is essentially just a list of packets. At the stage at which ProtocolLib injects into the clientbound netty pipeline, this packet has not been unpacked yet. Thus, we need to handle the ClientboundBundlePacket, which unfortunately is not registered in ProtocolLib. The fact that two different classes map to the same packet currently requires a dirty remapping in the packet structure modifier.
This commit is contained in:
Lukas Alt 2023-03-26 04:08:31 +02:00 committed by GitHub
parent 64e1e7de24
commit aebefded86
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
15 changed files with 231 additions and 57 deletions

View File

@ -125,7 +125,7 @@
<plugin>
<artifactId>maven-javadoc-plugin</artifactId>
<version>3.4.0</version>
<version>3.5.0</version>
<configuration>
<failOnError>false</failOnError>
<encoding>ISO-8859-1</encoding>

View File

@ -1121,6 +1121,12 @@ public abstract class AbstractStructure {
return structureModifier.withType(Optional.class, Converters.optional(converter));
}
public StructureModifier<Iterable<PacketContainer>> getPacketBundles() {
return structureModifier.withType(Iterable.class, Converters.iterable(
BukkitConverters.getPacketContainerConverter(), ArrayList::new, ArrayList::new
));
}
/**
* Represents an equivalent converter for ItemStack arrays.
* @author Kristian

View File

@ -40,6 +40,8 @@ import org.bukkit.Bukkit;
import org.bukkit.entity.Player;
import org.bukkit.event.Cancellable;
import javax.annotation.Nullable;
/**
* Represents a packet sending or receiving event. Changes to the packet will be reflected in the final version to be
* sent or received. It is also possible to cancel an event.
@ -70,6 +72,9 @@ public class PacketEvent extends EventObject implements Cancellable {
private boolean readOnly;
private boolean filtered;
@Nullable
private PacketEvent bundle;
/**
* Use the static constructors to create instances of this event.
*
@ -81,27 +86,28 @@ public class PacketEvent extends EventObject implements Cancellable {
}
private PacketEvent(Object source, PacketContainer packet, Player player, boolean serverPacket) {
this(source, packet, null, player, serverPacket, true);
this(source, packet, null, player, serverPacket, true, null);
}
private PacketEvent(Object source, PacketContainer packet, NetworkMarker marker, Player player, boolean serverPacket,
boolean filtered) {
boolean filtered, @Nullable PacketEvent bundleEvent) {
super(source);
this.packet = packet;
this.playerReference = new WeakReference<>(player);
this.networkMarker = marker;
this.serverPacket = serverPacket;
this.filtered = filtered;
this.bundle = bundleEvent;
}
private PacketEvent(PacketEvent origial, AsyncMarker asyncMarker) {
super(origial.source);
this.packet = origial.packet;
this.playerReference = origial.getPlayerReference();
this.cancel = origial.cancel;
this.serverPacket = origial.serverPacket;
this.filtered = origial.filtered;
this.networkMarker = origial.networkMarker;
private PacketEvent(PacketEvent original, AsyncMarker asyncMarker) {
super(original.source);
this.packet = original.packet;
this.playerReference = original.getPlayerReference();
this.cancel = original.cancel;
this.serverPacket = original.serverPacket;
this.filtered = original.filtered;
this.networkMarker = original.networkMarker;
this.asyncMarker = asyncMarker;
this.asynchronous = true;
}
@ -128,7 +134,7 @@ public class PacketEvent extends EventObject implements Cancellable {
* @return The event.
*/
public static PacketEvent fromClient(Object source, PacketContainer packet, NetworkMarker marker, Player client) {
return new PacketEvent(source, packet, marker, client, false, true);
return new PacketEvent(source, packet, marker, client, false, true, null);
}
/**
@ -145,7 +151,7 @@ public class PacketEvent extends EventObject implements Cancellable {
*/
public static PacketEvent fromClient(Object source, PacketContainer packet, NetworkMarker marker, Player client,
boolean filtered) {
return new PacketEvent(source, packet, marker, client, false, filtered);
return new PacketEvent(source, packet, marker, client, false, filtered, null);
}
/**
@ -170,7 +176,7 @@ public class PacketEvent extends EventObject implements Cancellable {
* @return The event.
*/
public static PacketEvent fromServer(Object source, PacketContainer packet, NetworkMarker marker, Player recipient) {
return new PacketEvent(source, packet, marker, recipient, true, true);
return new PacketEvent(source, packet, marker, recipient, true, true, null);
}
/**
@ -187,7 +193,25 @@ public class PacketEvent extends EventObject implements Cancellable {
*/
public static PacketEvent fromServer(Object source, PacketContainer packet, NetworkMarker marker, Player recipient,
boolean filtered) {
return new PacketEvent(source, packet, marker, recipient, true, filtered);
return fromServer(source, packet, marker, recipient, filtered, null);
}
/**
* Creates an event representing a server packet transmission.
* <p>
* If <i>filtered</i> is FALSE, then this event is only processed by packet monitors.
*
* @param source - the event source.
* @param packet - the packet.
* @param marker - the network marker.
* @param recipient - the client that will receieve the packet.
* @param filtered - whether this packet event is processed by every packet listener.
* @param bundle - The corresponding packet event of the bundle if this packet is part of a bundle. Otherwise, null.
* @return The event.
*/
public static PacketEvent fromServer(Object source, PacketContainer packet, NetworkMarker marker, Player recipient,
boolean filtered, @Nullable PacketEvent bundle) {
return new PacketEvent(source, packet, marker, recipient, true, filtered, bundle);
}
/**
@ -517,6 +541,15 @@ public class PacketEvent extends EventObject implements Cancellable {
}
}
/**
* Returns the packet event corresponding to the bundle if this packet is sent as a part of a bundle, t. Otherwise, null.
* @return Corresponding packet event or null.
*/
@Nullable
public PacketEvent getBundle() {
return bundle;
}
@Override
public String toString() {
return "PacketEvent[player=" + getPlayer() + ", packet=" + packet + "]";

View File

@ -17,17 +17,25 @@
package com.comphenix.protocol.injector;
import java.util.ArrayList;
import java.util.Collection;
import java.util.List;
import com.comphenix.protocol.PacketType;
import com.comphenix.protocol.concurrency.AbstractConcurrentListenerMultimap;
import com.comphenix.protocol.error.ErrorReporter;
import com.comphenix.protocol.events.ListenerPriority;
import com.comphenix.protocol.events.PacketContainer;
import com.comphenix.protocol.events.PacketEvent;
import com.comphenix.protocol.events.PacketListener;
import com.comphenix.protocol.injector.packet.PacketRegistry;
import com.comphenix.protocol.reflect.StructureModifier;
import com.comphenix.protocol.timing.TimedListenerManager;
import com.comphenix.protocol.timing.TimedListenerManager.ListenerType;
import com.comphenix.protocol.timing.TimedTracker;
import javax.annotation.Nullable;
/**
* Registry of synchronous packet listeners.
*
@ -108,7 +116,7 @@ public final class SortedPacketListenerList extends AbstractConcurrentListenerMu
* @param event - the related packet event.
* @param element - the listener to invoke.
*/
private final void invokeReceivingListener(ErrorReporter reporter, PacketEvent event, PrioritizedListener<PacketListener> element) {
private void invokeReceivingListener(ErrorReporter reporter, PacketEvent event, PrioritizedListener<PacketListener> element) {
try {
event.setReadOnly(element.getPriority() == ListenerPriority.MONITOR);
element.getListener().onPacketReceiving(event);
@ -130,59 +138,63 @@ public final class SortedPacketListenerList extends AbstractConcurrentListenerMu
* @param event - the packet event to invoke.
*/
public void invokePacketSending(ErrorReporter reporter, PacketEvent event) {
Collection<PrioritizedListener<PacketListener>> list = getListener(event.getPacketType());
if (list == null)
return;
if (timedManager.isTiming()) {
for (PrioritizedListener<PacketListener> element : list) {
TimedTracker tracker = timedManager.getTracker(element.getListener(), ListenerType.SYNC_SERVER_SIDE);
long token = tracker.beginTracking();
// Measure and record the execution time
invokeSendingListener(reporter, event, element);
tracker.endTracking(token, event.getPacketType());
}
} else {
for (PrioritizedListener<PacketListener> element : list) {
invokeSendingListener(reporter, event, element);
}
}
invokePacketSending(reporter, event, null);
}
/**
* Invokes the given packet event for every registered listener of the given priority.
* @param reporter - the error reporter that will be used to inform about listener exceptions.
* @param event - the packet event to invoke.
* @param priorityFilter - the required priority for a listener to be invoked.
* @param priorityFilter - the priority for a listener to be invoked. If null is provided, every registered listener will be invoked
*/
public void invokePacketSending(ErrorReporter reporter, PacketEvent event, ListenerPriority priorityFilter) {
public void invokePacketSending(ErrorReporter reporter, PacketEvent event, @Nullable ListenerPriority priorityFilter) {
invokeUnpackedPacketSending(reporter, event, priorityFilter);
if(event.getPacketType() == PacketType.Play.Server.DELIMITER && !event.isCancelled()) {
// unpack the bundle and invoke for each packet in the bundle
Iterable<PacketContainer> packets = event.getPacket().getPacketBundles().read(0);
List<PacketContainer> outPackets = new ArrayList<>();
for(PacketContainer subPacket : packets) {
PacketEvent subPacketEvent = PacketEvent.fromServer(this, subPacket, event.getNetworkMarker(), event.getPlayer());
invokeUnpackedPacketSending(reporter, subPacketEvent, priorityFilter);
if(!subPacketEvent.isCancelled()) {
outPackets.add(subPacketEvent.getPacket()); // if the packet event has been cancelled, the packet will be removed from the bundle.
}
}
if(packets.iterator().hasNext()) { // are there still packets in this bundle?
event.getPacket().getPacketBundles().write(0, outPackets);
} else {
event.setCancelled(true); // cancel packet if each individual packet has been canceled
}
}
}
private void invokeUnpackedPacketSending(ErrorReporter reporter, PacketEvent event, @org.jetbrains.annotations.Nullable ListenerPriority priorityFilter) {
Collection<PrioritizedListener<PacketListener>> list = getListener(event.getPacketType());
if (list == null)
return;
if (timedManager.isTiming()) {
for (PrioritizedListener<PacketListener> element : list) {
if (element.getPriority() == priorityFilter) {
TimedTracker tracker = timedManager.getTracker(element.getListener(), ListenerType.SYNC_SERVER_SIDE);
long token = tracker.beginTracking();
// Measure and record the execution time
invokeSendingListener(reporter, event, element);
tracker.endTracking(token, event.getPacketType());
if (priorityFilter == null || element.getPriority() == priorityFilter) {
TimedTracker tracker = timedManager.getTracker(element.getListener(), ListenerType.SYNC_SERVER_SIDE);
long token = tracker.beginTracking();
// Measure and record the execution time
invokeSendingListener(reporter, event, element);
tracker.endTracking(token, event.getPacketType());
}
}
} else {
for (PrioritizedListener<PacketListener> element : list) {
if (element.getPriority() == priorityFilter) {
if (priorityFilter == null || element.getPriority() == priorityFilter) {
invokeSendingListener(reporter, event, element);
}
}
}
}
/**
* Invoke a particular sending listener.
* @param reporter - the error reporter.

View File

@ -135,6 +135,11 @@ public class StructureCache {
return STRUCTURE_MODIFIER_CACHE.computeIfAbsent(packetType, type -> {
Class<?> packetClass = PacketRegistry.getPacketClassFromType(type);
// We need to map the Bundle Delimiter to the synthetic bundle packet which contains a list of all packets in a bundle
if(MinecraftVersion.atOrAbove(MinecraftVersion.FEATURE_PREVIEW_2) && packetClass.equals(MinecraftReflection.getBundleDelimiterClass())) {
packetClass = MinecraftReflection.getPackedBundlePacketClass();
}
return new StructureModifier<>(packetClass, MinecraftReflection.getPacketClass(), true);
});
}

View File

@ -27,12 +27,7 @@ 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.ByteBuddyGenerated;
import com.comphenix.protocol.utility.MinecraftFields;
import com.comphenix.protocol.utility.MinecraftMethods;
import com.comphenix.protocol.utility.MinecraftProtocolVersion;
import com.comphenix.protocol.utility.MinecraftReflection;
import com.comphenix.protocol.utility.MinecraftVersion;
import com.comphenix.protocol.utility.*;
import com.comphenix.protocol.wrappers.WrappedGameProfile;
import io.netty.channel.Channel;
import io.netty.channel.ChannelHandler;
@ -558,7 +553,7 @@ public class NettyChannelInjector implements Injector {
}
// no listener and no marker - no magic :)
if (!this.channelListener.hasListener(packet.getClass()) && marker == null) {
if (!this.channelListener.hasListener(packet.getClass()) && marker == null && !Util.isBundlePacket(packet.getClass())) {
return action;
}

View File

@ -28,6 +28,7 @@ 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;
@ -90,7 +91,7 @@ public class NetworkManagerInjector implements ChannelListener {
public PacketEvent onPacketSending(Injector injector, Object packet, NetworkMarker marker) {
// check if we need to intercept the packet
Class<?> packetClass = packet.getClass();
if (this.outboundListeners.contains(packetClass) || marker != null) {
if (this.outboundListeners.contains(packetClass) || marker != null || Util.isBundlePacket(packetClass)) {
// wrap packet and construct the event
PacketContainer container = new PacketContainer(PacketRegistry.getPacketType(packetClass), packet);
PacketEvent packetEvent = PacketEvent.fromServer(this, container, marker, injector.getPlayer());

View File

@ -30,6 +30,7 @@ import com.comphenix.protocol.reflect.StructureModifier;
import com.comphenix.protocol.reflect.fuzzy.FuzzyFieldContract;
import com.comphenix.protocol.utility.MinecraftReflection;
import com.comphenix.protocol.utility.MinecraftVersion;
import com.comphenix.protocol.utility.Util;
/**
* Static packet registry in Minecraft.
@ -382,6 +383,9 @@ public class PacketRegistry {
* @return The packet type, or NULL if not found.
*/
public static PacketType getPacketType(Class<?> packet) {
if(Util.isBundlePacket(packet)) {
return PacketType.Play.Server.DELIMITER;
}
initialize();
return REGISTER.classToType.get(packet);
}

View File

@ -628,6 +628,14 @@ public final class MinecraftReflection {
return getMinecraftClass("network.chat.IChatBaseComponent", "network.chat.IChatbaseComponent", "network.chat.Component", "IChatBaseComponent");
}
public static Class<?> getPackedBundlePacketClass() {
return getMinecraftClass("network.protocol.game.ClientboundBundlePacket", "ClientboundBundlePacket");
}
public static Class<?> getBundleDelimiterClass() {
return getMinecraftClass("network.protocol.BundleDelimiterPacket","BundleDelimiterPacket");
}
public static Class<?> getIChatBaseComponentArrayClass() {
return getArrayClass(getIChatBaseComponentClass());
}

View File

@ -22,6 +22,7 @@ package com.comphenix.protocol.utility;
public final class Util {
private static final boolean SPIGOT = classExists("org.spigotmc.SpigotConfig");
private static Class<?> cachedBundleClass;
public static boolean classExists(String className) {
try {
@ -59,4 +60,14 @@ public final class Util {
}
return false;
}
public static boolean isBundlePacket(Class<?> packetClass) {
if(!MinecraftVersion.atOrAbove(MinecraftVersion.FEATURE_PREVIEW_2)) {
return false;
}
if(cachedBundleClass == null) {
cachedBundleClass = MinecraftReflection.getPackedBundlePacketClass();
}
return packetClass.equals(cachedBundleClass);
}
}

View File

@ -40,7 +40,7 @@ public class AdventureComponentConverter {
/**
* Converts a {@link Component} into a ProtocolLib wrapper
* @param components Component
* @param component Component
* @return ProtocolLib wrapper
*/
public static WrappedChatComponent fromComponent(Component component) {

View File

@ -16,6 +16,7 @@
*/
package com.comphenix.protocol.wrappers;
import com.comphenix.protocol.events.PacketContainer;
import com.comphenix.protocol.wrappers.Either.Left;
import com.comphenix.protocol.wrappers.Either.Right;
import com.comphenix.protocol.wrappers.WrappedProfilePublicKey.WrappedProfileKeyData;
@ -618,6 +619,10 @@ public class BukkitConverters {
return ignoreNull(handle(WrappedLevelChunkData.LightData::getHandle, WrappedLevelChunkData.LightData::new, WrappedLevelChunkData.LightData.class));
}
public static EquivalentConverter<PacketContainer> getPacketContainerConverter() {
return handle(PacketContainer::getHandle, PacketContainer::fromPacket, PacketContainer.class);
}
/**
* Retrieve a converter for watchable objects and the respective wrapper.
* @return A watchable object converter.

View File

@ -18,10 +18,12 @@ import java.lang.reflect.Array;
import java.lang.reflect.Method;
import java.lang.reflect.Modifier;
import java.util.Collection;
import java.util.List;
import java.util.Optional;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.ConcurrentMap;
import java.util.function.Function;
import java.util.function.Supplier;
import com.comphenix.protocol.reflect.EquivalentConverter;
import com.comphenix.protocol.reflect.FuzzyReflection;
@ -29,6 +31,7 @@ import com.comphenix.protocol.reflect.accessors.Accessors;
import com.comphenix.protocol.reflect.accessors.MethodAccessor;
import com.comphenix.protocol.reflect.fuzzy.FuzzyMethodContract;
import com.comphenix.protocol.utility.MinecraftReflection;
import org.checkerframework.checker.units.qual.C;
/**
* Utility class for converters
@ -226,6 +229,53 @@ public class Converters {
});
}
public static <T> EquivalentConverter<Iterable<T>> iterable(
final EquivalentConverter<T> elementConverter,
final Supplier<List<T>> specificCollectionFactory,
final Supplier<List<?>> genericCollectionFactory
) {
return ignoreNull(new EquivalentConverter<Iterable<T>>() {
@Override
public Object getGeneric(Iterable<T> specific) {
// generics are very cool, thank you java
List<Object> targetCollection = (List<Object>) genericCollectionFactory.get();
for (T element : specific) {
Object generic = elementConverter.getGeneric(element);
if (generic != null) {
targetCollection.add(generic);
}
}
return targetCollection;
}
@Override
public Iterable<T> getSpecific(Object generic) {
if (generic instanceof Iterable<?>) {
Iterable<Object> sourceCollection = (Iterable<Object>) generic;
List<T> targetCollection = specificCollectionFactory.get();
// copy over all elements into a new collection
for (Object element : sourceCollection) {
T specific = elementConverter.getSpecific(element);
if (specific != null) {
targetCollection.add(specific);
}
}
return targetCollection;
}
// not valid
return null;
}
@Override
public Class<Iterable<T>> getSpecificType() {
return (Class) Iterable.class;
}
});
}
private static MethodAccessor holderGetValue;
public static <T> EquivalentConverter<T> holder(final EquivalentConverter<T> converter,

View File

@ -4,9 +4,13 @@ import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertTrue;
import com.comphenix.protocol.BukkitInitialization;
import com.comphenix.protocol.PacketType;
import com.comphenix.protocol.ProtocolLibrary;
import com.comphenix.protocol.events.PacketContainer;
import com.comphenix.protocol.reflect.EquivalentConverter;
import com.comphenix.protocol.wrappers.Either.Left;
import org.apache.commons.lang.builder.EqualsBuilder;
import org.bukkit.Bukkit;
import org.bukkit.ChatColor;
import org.bukkit.Material;
@ -55,4 +59,17 @@ public class BukkitConvertersTest {
assertEquals(wrapped.left(), nmsEither.left());
assertEquals(wrapped.right(), nmsEither.right());
}
@Test
public void testPacketContainerConverter() {
for (PacketType type : PacketType.values()) {
if(!type.isSupported()) {
continue;
}
PacketContainer container = new PacketContainer(type);
Object generic = BukkitConverters.getPacketContainerConverter().getGeneric(container);
Object specific = BukkitConverters.getPacketContainerConverter().getSpecific(generic);
assertTrue(EqualsBuilder.reflectionEquals(container, specific)); // PacketContainer does not properly implement equals(.)
}
}
}

View File

@ -0,0 +1,27 @@
package com.comphenix.protocol.wrappers;
import com.comphenix.protocol.reflect.EquivalentConverter;
import org.apache.commons.lang.builder.EqualsBuilder;
import org.junit.jupiter.api.Test;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertTrue;
/**
* @author Lukas Alt
* @since 26.03.2023
*/
public class ConverterTest {
@Test
public void testIterableConverter() {
EquivalentConverter<Iterable<String>> converter = Converters.iterable(Converters.passthrough(String.class), ArrayList::new, ArrayList::new);
List<String> l = Arrays.asList("a", "b", "c");
Object generic = converter.getGeneric(l);
Object specific = converter.getSpecific(generic);
assertEquals(l, specific);
}
}