From 1beb95115f8309d3f472b870064ea00138ab01b7 Mon Sep 17 00:00:00 2001 From: Pasqual Koschmieder Date: Fri, 12 Aug 2022 04:07:38 +0200 Subject: [PATCH] Improve cloning tests (#1822) --- .../protocol/injector/StructureCache.java | 60 ++++++++++--------- .../protocol/reflect/StructureModifier.java | 17 ++---- .../reflect/cloning/ImmutableDetector.java | 3 + .../reflect/instances/PrimitiveGenerator.java | 52 +++++++++------- .../protocol/utility/ZeroBuffer.java | 2 +- .../protocol/events/PacketContainerTest.java | 53 ++++++++++++---- 6 files changed, 113 insertions(+), 74 deletions(-) diff --git a/src/main/java/com/comphenix/protocol/injector/StructureCache.java b/src/main/java/com/comphenix/protocol/injector/StructureCache.java index f3fc37a8..438bf8ae 100644 --- a/src/main/java/com/comphenix/protocol/injector/StructureCache.java +++ b/src/main/java/com/comphenix/protocol/injector/StructureCache.java @@ -17,6 +17,11 @@ package com.comphenix.protocol.injector; +import java.util.Map; +import java.util.Objects; +import java.util.concurrent.ConcurrentHashMap; +import java.util.function.Supplier; + import com.comphenix.protocol.PacketType; import com.comphenix.protocol.injector.packet.PacketRegistry; import com.comphenix.protocol.reflect.StructureModifier; @@ -26,13 +31,11 @@ import com.comphenix.protocol.reflect.instances.DefaultInstances; import com.comphenix.protocol.utility.ByteBuddyFactory; import com.comphenix.protocol.utility.MinecraftMethods; import com.comphenix.protocol.utility.MinecraftReflection; +import com.comphenix.protocol.utility.MinecraftVersion; import com.comphenix.protocol.utility.ZeroBuffer; import com.comphenix.protocol.wrappers.WrappedChatComponent; import com.google.common.base.Preconditions; import io.netty.buffer.ByteBuf; -import java.util.Map; -import java.util.concurrent.ConcurrentHashMap; -import java.util.function.Supplier; import net.bytebuddy.dynamic.loading.ClassLoadingStrategy; import net.bytebuddy.implementation.FixedValue; import net.bytebuddy.matcher.ElementMatchers; @@ -52,33 +55,34 @@ public class StructureCache { private static boolean TRICK_TRIED = false; private static ConstructorAccessor TRICKED_DATA_SERIALIZER; - public static Object newPacket(Class clazz) { - Object result = DefaultInstances.DEFAULT.create(clazz); - - if (result == null) { - return PACKET_INSTANCE_CREATORS.computeIfAbsent(clazz, $ -> { - ConstructorAccessor accessor = Accessors.getConstructorAccessorOrNull(clazz, + public static Object newPacket(Class packetClass) { + Supplier packetConstructor = PACKET_INSTANCE_CREATORS.computeIfAbsent(packetClass, clazz -> { + // prefer construction via PacketDataSerializer constructor on 1.17 and above + if (MinecraftVersion.CAVES_CLIFFS_1.atOrAbove()) { + ConstructorAccessor serializerAccessor = Accessors.getConstructorAccessorOrNull( + clazz, MinecraftReflection.getPacketDataSerializerClass()); - if (accessor != null) { + if (serializerAccessor != null) { return () -> { - try { - return accessor.invoke(MinecraftReflection.getPacketDataSerializer(new ZeroBuffer())); - } catch (Exception exception) { - // try trick nms around as they want a non-null compound in the map_chunk packet constructor - Object trickyDataSerializer = getTrickDataSerializerOrNull(); - if (trickyDataSerializer != null) { - return accessor.invoke(trickyDataSerializer); - } - // the tricks are over - throw new IllegalArgumentException("Unable to create packet " + clazz, exception); + // the tricked serializer should always be present... If not try using a normal buffer + Object dataSerializer = getTrickDataSerializerOrNull(); + if (dataSerializer == null) { + dataSerializer = MinecraftReflection.getPacketDataSerializer(new ZeroBuffer()); } + + return serializerAccessor.invoke(dataSerializer); }; } - throw new IllegalArgumentException("No matching constructor to create packet in class " + clazz); - }).get(); - } + } - return result; + // try via DefaultInstances as fallback + return () -> { + Object packetInstance = DefaultInstances.DEFAULT.create(clazz); + Objects.requireNonNull(packetInstance, "Unable to create packet instance for class " + clazz); + return packetInstance; + }; + }); + return packetConstructor.get(); } /** @@ -113,17 +117,15 @@ public class StructureCache { public static StructureModifier getStructure(final PacketType packetType) { Preconditions.checkNotNull(packetType, "type cannot be null"); - StructureModifier modifier = STRUCTURE_MODIFIER_CACHE.computeIfAbsent(packetType, type -> { + return STRUCTURE_MODIFIER_CACHE.computeIfAbsent(packetType, type -> { Class packetClass = PacketRegistry.getPacketClassFromType(type); return new StructureModifier<>(packetClass, MinecraftReflection.getPacketClass(), true); }); - - return modifier; } /** - * Creates a packet data serializer sub-class if needed to allow the fixed read of a NbtTagCompound because of a null - * check in the MapChunk packet constructor. + * Creates a packet data serializer sub-class if needed to allow the fixed read of a NbtTagCompound because of a + * null check in the MapChunk packet constructor. * * @return an accessor to a constructor which creates a data serializer. */ diff --git a/src/main/java/com/comphenix/protocol/reflect/StructureModifier.java b/src/main/java/com/comphenix/protocol/reflect/StructureModifier.java index 1f5def03..a1967565 100644 --- a/src/main/java/com/comphenix/protocol/reflect/StructureModifier.java +++ b/src/main/java/com/comphenix/protocol/reflect/StructureModifier.java @@ -269,13 +269,8 @@ public class StructureModifier { return null; } - // don't try to convert null, just return null + // get the field value and convert it if needed Object fieldValue = accessor.get(this.target); - if (fieldValue == null) { - return null; - } - - // check if we need to convert the field value and do so if needed return this.needConversion() ? this.converter.getSpecific(fieldValue) : (T) fieldValue; } @@ -332,14 +327,10 @@ public class StructureModifier { return this; } - // just write the value if the specific given one is null or no conversion is needed - if (value == null || !this.needConversion()) { - accessor.set(this.target, value); - return this; - } - // convert and write - accessor.set(this.target, this.converter.getGeneric(value)); + Object fieldValue = this.needConversion() ? this.converter.getGeneric(value) : value; + accessor.set(this.target, fieldValue); + return this; } diff --git a/src/main/java/com/comphenix/protocol/reflect/cloning/ImmutableDetector.java b/src/main/java/com/comphenix/protocol/reflect/cloning/ImmutableDetector.java index 75870e8c..f8378dfb 100644 --- a/src/main/java/com/comphenix/protocol/reflect/cloning/ImmutableDetector.java +++ b/src/main/java/com/comphenix/protocol/reflect/cloning/ImmutableDetector.java @@ -76,6 +76,9 @@ public class ImmutableDetector implements Cloner { add("world.entity.npc.VillagerProfession", "VillagerProfession"); } + add("world.entity.animal.CatVariant"); + add("world.entity.animal.FrogVariant"); + // TODO automatically detect the technically-not-an-enum enums that Mojang is so fond of // Would also probably go in tandem with having the FieldCloner use this diff --git a/src/main/java/com/comphenix/protocol/reflect/instances/PrimitiveGenerator.java b/src/main/java/com/comphenix/protocol/reflect/instances/PrimitiveGenerator.java index c00c6c0a..0f91de7b 100644 --- a/src/main/java/com/comphenix/protocol/reflect/instances/PrimitiveGenerator.java +++ b/src/main/java/com/comphenix/protocol/reflect/instances/PrimitiveGenerator.java @@ -2,55 +2,64 @@ * 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.instances; import java.lang.reflect.Array; - -import javax.annotation.Nullable; +import java.util.Optional; import com.google.common.base.Defaults; import com.google.common.primitives.Primitives; +import javax.annotation.Nullable; /** - * Provides constructors for primtive types, wrappers, arrays and strings. + * Provides constructors for primitive java types and wrappers. + * * @author Kristian */ public class PrimitiveGenerator implements InstanceProvider { - + /** * Default value for Strings. */ + @Deprecated public static final String STRING_DEFAULT = ""; - + /** * Shared instance of this generator. */ - public static PrimitiveGenerator INSTANCE = new PrimitiveGenerator(STRING_DEFAULT); + public static PrimitiveGenerator INSTANCE = new PrimitiveGenerator(); // Our default string value - private String stringDefault; - + private final String stringDefault; + + public PrimitiveGenerator() { + this.stringDefault = ""; + } + + @Deprecated public PrimitiveGenerator(String stringDefault) { this.stringDefault = stringDefault; } - + /** * Retrieve the string default. + * * @return Default instance of a string. */ + @Deprecated public String getStringDefault() { return stringDefault; } @@ -68,13 +77,16 @@ public class PrimitiveGenerator implements InstanceProvider { return Array.newInstance(arrayType, 0); } else if (type.isEnum()) { Object[] values = type.getEnumConstants(); - if (values != null && values.length > 0) + if (values != null && values.length > 0) { return values[0]; + } } else if (type.equals(String.class)) { - return stringDefault; - } - + return this.stringDefault; + } else if (type.equals(Optional.class)) { + return Optional.empty(); + } + // Cannot handle this type return null; - } + } } diff --git a/src/main/java/com/comphenix/protocol/utility/ZeroBuffer.java b/src/main/java/com/comphenix/protocol/utility/ZeroBuffer.java index d24b468b..1d8cff35 100644 --- a/src/main/java/com/comphenix/protocol/utility/ZeroBuffer.java +++ b/src/main/java/com/comphenix/protocol/utility/ZeroBuffer.java @@ -366,7 +366,7 @@ public class ZeroBuffer extends ByteBuf { @Override public short readUnsignedByte() { - return 0; + return 255; } @Override diff --git a/src/test/java/com/comphenix/protocol/events/PacketContainerTest.java b/src/test/java/com/comphenix/protocol/events/PacketContainerTest.java index 7f3dd5c7..4f7bd019 100644 --- a/src/test/java/com/comphenix/protocol/events/PacketContainerTest.java +++ b/src/test/java/com/comphenix/protocol/events/PacketContainerTest.java @@ -35,6 +35,7 @@ import com.comphenix.protocol.reflect.StructureModifier; import com.comphenix.protocol.reflect.accessors.Accessors; import com.comphenix.protocol.reflect.accessors.FieldAccessor; import com.comphenix.protocol.reflect.cloning.SerializableCloner; +import com.comphenix.protocol.utility.MinecraftMethods; import com.comphenix.protocol.utility.MinecraftReflection; import com.comphenix.protocol.wrappers.BlockPosition; import com.comphenix.protocol.wrappers.BukkitConverters; @@ -67,6 +68,7 @@ import net.md_5.bungee.api.chat.ComponentBuilder; import net.md_5.bungee.api.chat.HoverEvent; import net.md_5.bungee.api.chat.hover.content.Text; import net.minecraft.core.IRegistry; +import net.minecraft.network.protocol.game.PacketPlayOutGameStateChange; import net.minecraft.network.protocol.game.PacketPlayOutUpdateAttributes; import net.minecraft.network.protocol.game.PacketPlayOutUpdateAttributes.AttributeSnapshot; import net.minecraft.resources.MinecraftKey; @@ -74,9 +76,8 @@ import net.minecraft.world.effect.MobEffect; import net.minecraft.world.effect.MobEffectList; import net.minecraft.world.entity.ai.attributes.AttributeBase; import net.minecraft.world.entity.ai.attributes.AttributeModifier; -import net.minecraft.world.entity.npc.VillagerData; -import net.minecraft.world.entity.npc.VillagerProfession; -import net.minecraft.world.entity.npc.VillagerType; +import net.minecraft.world.entity.animal.CatVariant; +import net.minecraft.world.entity.animal.FrogVariant; import org.apache.commons.lang.SerializationUtils; import org.bukkit.ChatColor; import org.bukkit.Material; @@ -794,7 +795,7 @@ public class PacketContainerTest { assertArrayEquals(signature, read.getSignature()); }*/ - private void assertPacketsEqual(PacketContainer constructed, PacketContainer cloned) { + private void assertPacketsEqualAndSerializable(PacketContainer constructed, PacketContainer cloned) { StructureModifier firstMod = constructed.getModifier(), secondMod = cloned.getModifier(); assertEquals(firstMod.size(), secondMod.size()); @@ -810,6 +811,9 @@ public class PacketContainerTest { } } } + + Object buffer = MinecraftReflection.createPacketDataSerializer(0); + MinecraftMethods.getPacketWriteByteBufMethod().invoke(cloned.getHandle(), buffer); } @Test @@ -833,32 +837,59 @@ public class PacketContainerTest { new WrappedWatchableObject( new WrappedDataWatcherObject(0, Registry.get(Byte.class)), (byte) 1), + new WrappedWatchableObject( + new WrappedDataWatcherObject(0, Registry.get(Integer.class)), + 1), + new WrappedWatchableObject( + new WrappedDataWatcherObject(0, Registry.get(Float.class)), + 1F), new WrappedWatchableObject( new WrappedDataWatcherObject(0, Registry.get(String.class)), "String"), new WrappedWatchableObject( - new WrappedDataWatcherObject(0, Registry.get(Float.class)), - 1.0F), + new WrappedDataWatcherObject(0, Registry.get(Boolean.class)), + true), new WrappedWatchableObject( new WrappedDataWatcherObject(0, Registry.getChatComponentSerializer(true)), Optional.of(ComponentConverter.fromBaseComponent(TEST_COMPONENT).getHandle())), new WrappedWatchableObject( - new WrappedDataWatcherObject(0, Registry.get(VillagerData.class)), - new VillagerData(VillagerType.b, VillagerProfession.c, 69)) + new WrappedDataWatcherObject(0, Registry.getItemStackSerializer(false)), + BukkitConverters.getItemStackConverter().getGeneric(new ItemStack(Material.WOODEN_AXE))), + new WrappedWatchableObject( + new WrappedDataWatcherObject(0, Registry.get(CatVariant.class)), + CatVariant.a), + new WrappedWatchableObject( + new WrappedDataWatcherObject(0, Registry.get(FrogVariant.class)), + FrogVariant.c) )); } else if (type == PacketType.Play.Server.CHAT) { constructed.getChatComponents().write(0, ComponentConverter.fromBaseComponent(TEST_COMPONENT)); + } else if (type == PacketType.Play.Server.REMOVE_ENTITY_EFFECT || type == PacketType.Play.Server.ENTITY_EFFECT) { + constructed.getEffectTypes().write(0, PotionEffectType.GLOWING); + } else if (type == PacketType.Play.Server.GAME_STATE_CHANGE) { + constructed.getStructures().write( + 0, + InternalStructure.getConverter().getSpecific(PacketPlayOutGameStateChange.a)); + } else if (type == PacketType.Play.Client.USE_ITEM || type == PacketType.Play.Client.BLOCK_PLACE) { + constructed.getLongs().write(0, 0L); // timestamp of the packet, not sent over the network } // gives some indication which cloning process fails as the checks itself are happening outside this method System.out.println("Cloning " + type); - // Clone the packet both ways + // Clone the packet all three ways PacketContainer shallowCloned = constructed.shallowClone(); - this.assertPacketsEqual(constructed, shallowCloned); + this.assertPacketsEqualAndSerializable(constructed, shallowCloned); PacketContainer deepCloned = constructed.deepClone(); - this.assertPacketsEqual(constructed, deepCloned); + this.assertPacketsEqualAndSerializable(constructed, deepCloned); + + PacketContainer serializedCloned = SerializableCloner.clone(constructed); + if (type == PacketType.Play.Client.USE_ITEM || type == PacketType.Play.Client.BLOCK_PLACE) { + // shit fix - but what are we supposed to do :/ + serializedCloned.getLongs().write(0, 0L); + } + this.assertPacketsEqualAndSerializable(constructed, serializedCloned); } catch (Exception ex) { Assertions.fail("Unable to clone " + type, ex); }