From ea08696d105a5c5b93efe5dad8f4ed464d087a1d Mon Sep 17 00:00:00 2001 From: "Kristian S. Stangeland" Date: Thu, 31 Jan 2013 20:30:08 +0100 Subject: [PATCH] Fix a serious bug that caused Blocks to inadvertanly be created. Our custom IOC system (DefaultInstances) would call new Block(0) on a MCPC based CraftBukkit system, causing air blocks to be treated as solid blocks. We prevented this by explicitly forbidding it from creating instances of ItemStack or Block in our structure modifier's default field generator. A second bug was caused by mismatched methods in WrappedDataWatcher, which we solved by simply trying to use them, and then swapping them if we encounter any errors or weird state. --- .../protocol/reflect/FuzzyMatchers.java | 40 +++++++++++++++++- .../protocol/reflect/StructureModifier.java | 18 +++++++- .../protocol/reflect/cloning/FieldCloner.java | 7 +++- .../reflect/instances/BannedGenerator.java | 36 ++++++++++++++++ .../reflect/instances/DefaultInstances.java | 29 +++++++++---- .../reflect/instances/InstanceProvider.java | 2 +- .../instances/NotConstructableException.java | 42 +++++++++++++++++++ .../protocol/utility/MinecraftReflection.java | 31 ++++++++++++++ .../protocol/wrappers/WrappedDataWatcher.java | 37 +++++++++++----- 9 files changed, 219 insertions(+), 23 deletions(-) create mode 100644 ProtocolLib/src/main/java/com/comphenix/protocol/reflect/instances/BannedGenerator.java create mode 100644 ProtocolLib/src/main/java/com/comphenix/protocol/reflect/instances/NotConstructableException.java diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/reflect/FuzzyMatchers.java b/ProtocolLib/src/main/java/com/comphenix/protocol/reflect/FuzzyMatchers.java index 1fee2439..461c6df6 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/reflect/FuzzyMatchers.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/reflect/FuzzyMatchers.java @@ -1,8 +1,11 @@ package com.comphenix.protocol.reflect; import java.lang.reflect.Member; +import java.util.Set; import java.util.regex.Pattern; +import com.google.common.collect.Sets; + /** * Contains factory methods for matching classes. * @@ -16,10 +19,45 @@ public class FuzzyMatchers { /** * Construct a class matcher that matches types exactly. * @param matcher - the matching class. + * @return A new class mathcher. */ public static AbstractFuzzyMatcher> matchExact(Class matcher) { return new ExactClassMatcher(matcher, ExactClassMatcher.Options.MATCH_EXACT); } + + /** + * Construct a class matcher that matches any of the given classes exactly. + * @param classes - list of classes to match. + * @return A new class mathcher. + */ + public static AbstractFuzzyMatcher> matchAnyOf(Class... classes) { + return matchAnyOf(Sets.newHashSet(classes)); + } + + /** + * Construct a class matcher that matches any of the given classes exactly. + * @param classes - set of classes to match. + * @return A new class mathcher. + */ + public static AbstractFuzzyMatcher> matchAnyOf(final Set> classes) { + return new AbstractFuzzyMatcher>() { + @Override + public boolean isMatch(Class value, Object parent) { + return classes.contains(value); + } + + @Override + protected int calculateRoundNumber() { + int roundNumber = 0; + + // The highest round number (except zero). + for (Class clazz : classes) { + roundNumber = combineRounds(roundNumber, -ExactClassMatcher.getClassNumber(clazz)); + } + return roundNumber; + } + }; + } /** * Construct a class matcher that matches super types of the given class. @@ -38,7 +76,7 @@ public class FuzzyMatchers { public static AbstractFuzzyMatcher> matchDerived(Class matcher) { return new ExactClassMatcher(matcher, ExactClassMatcher.Options.MATCH_DERIVED); } - + /** * Construct a class matcher based on the canonical names of classes. * @param regex - regular expression pattern matching class names. diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/reflect/StructureModifier.java b/ProtocolLib/src/main/java/com/comphenix/protocol/reflect/StructureModifier.java index 3f2c98d3..752a14d9 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/reflect/StructureModifier.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/reflect/StructureModifier.java @@ -26,10 +26,14 @@ import java.util.Map; import java.util.concurrent.ConcurrentHashMap; import com.comphenix.protocol.reflect.compiler.BackgroundCompiler; +import com.comphenix.protocol.reflect.instances.BannedGenerator; import com.comphenix.protocol.reflect.instances.DefaultInstances; +import com.comphenix.protocol.reflect.instances.InstanceProvider; +import com.comphenix.protocol.utility.MinecraftReflection; import com.google.common.base.Function; import com.google.common.base.Objects; import com.google.common.collect.ImmutableList; +import com.google.common.collect.Lists; /** * Provides list-oriented access to the fields of a Minecraft packet. @@ -64,7 +68,19 @@ public class StructureModifier { // Whether or not to automatically compile the structure modifier protected boolean useStructureCompiler; + + // Instance generator we wil use + private static DefaultInstances DEFAULT_GENERATOR = getDefaultGenerator(); + private static DefaultInstances getDefaultGenerator() { + List providers = Lists.newArrayList(); + + // Prevent certain classes from being generated + providers.add(new BannedGenerator(MinecraftReflection.getItemStackClass(), MinecraftReflection.getBlockClass())); + providers.addAll(DefaultInstances.DEFAULT.getRegistered()); + return DefaultInstances.fromCollection(providers); + } + /** * Creates a structure modifier. * @param targetType - the structure to modify. @@ -528,7 +544,7 @@ public class StructureModifier { private static Map generateDefaultFields(List fields) { Map requireDefaults = new HashMap(); - DefaultInstances generator = DefaultInstances.DEFAULT; + DefaultInstances generator = DEFAULT_GENERATOR; int index = 0; for (Field field : fields) { diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/reflect/cloning/FieldCloner.java b/ProtocolLib/src/main/java/com/comphenix/protocol/reflect/cloning/FieldCloner.java index 62a07bc7..503fb752 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/reflect/cloning/FieldCloner.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/reflect/cloning/FieldCloner.java @@ -20,6 +20,7 @@ package com.comphenix.protocol.reflect.cloning; import com.comphenix.protocol.reflect.ObjectWriter; import com.comphenix.protocol.reflect.StructureModifier; import com.comphenix.protocol.reflect.instances.InstanceProvider; +import com.comphenix.protocol.reflect.instances.NotConstructableException; /** * Represents a class capable of cloning objects by deeply copying its fields. @@ -72,7 +73,11 @@ public class FieldCloner implements Cloner { return false; // Attempt to create the type - return instanceProvider.create(source.getClass()) != null; + try { + return instanceProvider.create(source.getClass()) != null; + } catch (NotConstructableException e) { + return false; + } } @Override diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/reflect/instances/BannedGenerator.java b/ProtocolLib/src/main/java/com/comphenix/protocol/reflect/instances/BannedGenerator.java new file mode 100644 index 00000000..778e4682 --- /dev/null +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/reflect/instances/BannedGenerator.java @@ -0,0 +1,36 @@ +package com.comphenix.protocol.reflect.instances; + +import javax.annotation.Nullable; + +import com.comphenix.protocol.reflect.AbstractFuzzyMatcher; +import com.comphenix.protocol.reflect.FuzzyMatchers; + +/** + * Generator that ensures certain types will never be created. + * + * @author Kristian + */ +public class BannedGenerator implements InstanceProvider { + private AbstractFuzzyMatcher> classMatcher; + + /** + * Construct a generator that ensures any class that matches the given matcher is never constructed. + * @param classMatcher - a class matcher. + */ + public BannedGenerator(AbstractFuzzyMatcher> classMatcher) { + this.classMatcher = classMatcher; + } + + public BannedGenerator(Class... classes) { + this.classMatcher = FuzzyMatchers.matchAnyOf(classes); + } + + @Override + public Object create(@Nullable Class type) { + // Prevent these types from being constructed + if (classMatcher.isMatch(type, null)) { + throw new NotConstructableException(); + } + return null; + } +} diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/reflect/instances/DefaultInstances.java b/ProtocolLib/src/main/java/com/comphenix/protocol/reflect/instances/DefaultInstances.java index fe7233da..826a4ecd 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/reflect/instances/DefaultInstances.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/reflect/instances/DefaultInstances.java @@ -86,8 +86,17 @@ public class DefaultInstances implements InstanceProvider { * @param instaceProviders - array of instance providers. * @return An default instance generator. */ - public static DefaultInstances fromArray(InstanceProvider... instaceProviders) { - return new DefaultInstances(ImmutableList.copyOf(instaceProviders)); + public static DefaultInstances fromArray(InstanceProvider... instanceProviders) { + return new DefaultInstances(ImmutableList.copyOf(instanceProviders)); + } + + /** + * Construct a default instance generator using the given instance providers. + * @param instaceProviders - collection of instance providers. + * @return An default instance generator. + */ + public static DefaultInstances fromCollection(Collection instanceProviders) { + return new DefaultInstances(ImmutableList.copyOf(instanceProviders)); } /** @@ -241,11 +250,15 @@ public class DefaultInstances implements InstanceProvider { private T getDefaultInternal(Class type, List providers, int recursionLevel) { // The instance providiers should protect themselves against recursion - for (InstanceProvider generator : providers) { - Object value = generator.create(type); - - if (value != null) - return (T) value; + try { + for (InstanceProvider generator : providers) { + Object value = generator.create(type); + + if (value != null) + return (T) value; + } + } catch (NotConstructableException e) { + return null; } // Guard against recursion @@ -276,7 +289,7 @@ public class DefaultInstances implements InstanceProvider { } } catch (Exception e) { - // Nope, we couldn't create this type + // Nope, we couldn't create this type. Might for instance be NotConstructableException. } // No suitable default value could be found diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/reflect/instances/InstanceProvider.java b/ProtocolLib/src/main/java/com/comphenix/protocol/reflect/instances/InstanceProvider.java index 223a2c77..0c71ed92 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/reflect/instances/InstanceProvider.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/reflect/instances/InstanceProvider.java @@ -25,11 +25,11 @@ import javax.annotation.Nullable; * @author Kristian */ public interface InstanceProvider { - /** * Create an instance given a type, if possible. * @param type - type to create. * @return The instance, or NULL if the type cannot be created. + * @throws NotConstructableException Thrown to indicate that this type cannot or should never be constructed. */ public abstract Object create(@Nullable Class type); } \ No newline at end of file diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/reflect/instances/NotConstructableException.java b/ProtocolLib/src/main/java/com/comphenix/protocol/reflect/instances/NotConstructableException.java new file mode 100644 index 00000000..4a7f38e3 --- /dev/null +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/reflect/instances/NotConstructableException.java @@ -0,0 +1,42 @@ +package com.comphenix.protocol.reflect.instances; + +/** + * Invoked when a instance provider indicates that a given type cannot or should not be + * constructed under any circumstances. + * + * @author Kristian + */ +public class NotConstructableException extends IllegalArgumentException { + /** + * Generated by Eclipse. + */ + private static final long serialVersionUID = -1144171604744845463L; + + /** + * Construct a new not constructable exception. + */ + public NotConstructableException() { + super("This object should never be constructed."); + } + + /** + * Construct a new not constructable exception with a custom message. + */ + public NotConstructableException(String message) { + super(message); + } + + /** + * Construct a new not constructable exception with a custom message and cause. + */ + public NotConstructableException(String message, Throwable cause) { + super(message, cause); + } + + /** + * Construct a new not constructable exception with a custom cause. + */ + public NotConstructableException(Throwable cause) { + super( cause); + } +} 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 b59e3522..03966736 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/utility/MinecraftReflection.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/utility/MinecraftReflection.java @@ -25,6 +25,7 @@ import java.lang.reflect.Field; import java.lang.reflect.Method; import java.lang.reflect.Modifier; import java.net.ServerSocket; +import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Set; @@ -586,6 +587,36 @@ public class MinecraftReflection { FuzzyReflection.fromClass(getCraftItemStackClass(), true).getFieldByName("handle").getType()); } } + + /** + * Retrieve the Block (NMS) class. + * @return Block (NMS) class. + */ + public static Class getBlockClass() { + try { + return getMinecraftClass("Block"); + } catch (RuntimeException e) { + FuzzyReflection reflect = FuzzyReflection.fromClass(getItemStackClass()); + Set> candidates = new HashSet>(); + + // Minecraft objects in the constructor + for (Constructor constructor : reflect.getConstructors()) { + for (Class clazz : constructor.getParameterTypes()) { + if (isMinecraftClass(clazz)) { + candidates.add(clazz); + } + } + } + + // Useful constructors + Method selected = + reflect.getMethod(FuzzyMethodContract.newBuilder(). + parameterMatches(FuzzyMatchers.matchAnyOf(candidates)). + returnTypeExact(float.class). + build()); + return setMinecraftClass("Block", selected.getParameterTypes()[0]); + } + } /** * Retrieve the WorldType class. diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/wrappers/WrappedDataWatcher.java b/ProtocolLib/src/main/java/com/comphenix/protocol/wrappers/WrappedDataWatcher.java index 09ec2b10..fda3ea21 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/wrappers/WrappedDataWatcher.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/wrappers/WrappedDataWatcher.java @@ -410,7 +410,7 @@ public class WrappedDataWatcher implements Iterable { try { Object watchable = getWatchedObject(index); - + if (watchable != null) { new WrappedWatchableObject(watchable).setValue(newValue, update); } else { @@ -551,8 +551,17 @@ public class WrappedDataWatcher implements Iterable { List candidates = fuzzy.getMethodListByParameters(Void.TYPE, new Class[] { int.class, Object.class}); - for (Method method : candidates) { + // Load the get-method + try { + getKeyValueMethod = fuzzy.getMethodByParameters( + "getWatchableObject", MinecraftReflection.getWatchableObjectClass(), new Class[] { int.class }); + getKeyValueMethod.setAccessible(true); + } catch (IllegalArgumentException e) { + // Use the fallback method + } + + for (Method method : candidates) { if (!method.getName().startsWith("watch")) { createKeyValueMethod = method; } else { @@ -569,15 +578,21 @@ public class WrappedDataWatcher implements Iterable { } else { throw new IllegalStateException("Unable to find create and update watchable object. Update ProtocolLib."); } - } - - // Load the get-method - try { - getKeyValueMethod = fuzzy.getMethodByParameters( - "getWatchableObject", MinecraftReflection.getWatchableObjectClass(), new Class[] { int.class }); - getKeyValueMethod.setAccessible(true); - } catch (IllegalArgumentException e) { - // Use fallback method + + // Be a little scientist - see if this in fact IS the right way around + try { + WrappedDataWatcher watcher = new WrappedDataWatcher(); + watcher.setObject(0, 0); + watcher.setObject(0, 1); + + if (watcher.getInteger(0) != 1) { + throw new IllegalStateException("This cannot be!"); + } + } catch (Exception e) { + // Nope + updateKeyValueMethod = candidates.get(0); + createKeyValueMethod = candidates.get(1); + } } }