From eb12808483e753430e8f82d256bdb1dcb1f9b342 Mon Sep 17 00:00:00 2001 From: "Kristian S. Stangeland" Date: Thu, 4 Oct 2012 17:43:46 +0200 Subject: [PATCH] Prevent method #3 from creating uneeded proxy objects. This would happen if the NetServerHandler is already a proxy, and the injection method is unable to create an instance of the proxy type. It's best to fail instead of polluting (due to constructors with side-effects) Minecraft with failed proxy objects. --- .../injector/NetworkServerInjector.java | 40 +++++----- .../reflect/instances/DefaultInstances.java | 73 ++++++++++++++++--- 2 files changed, 83 insertions(+), 30 deletions(-) diff --git a/ProtocolLib/src/com/comphenix/protocol/injector/NetworkServerInjector.java b/ProtocolLib/src/com/comphenix/protocol/injector/NetworkServerInjector.java index 3c053311..50f311ad 100644 --- a/ProtocolLib/src/com/comphenix/protocol/injector/NetworkServerInjector.java +++ b/ProtocolLib/src/com/comphenix/protocol/injector/NetworkServerInjector.java @@ -19,10 +19,8 @@ import com.comphenix.protocol.events.PacketListener; import com.comphenix.protocol.reflect.FieldUtils; import com.comphenix.protocol.reflect.FuzzyReflection; import com.comphenix.protocol.reflect.ObjectCloner; -import com.comphenix.protocol.reflect.instances.CollectionGenerator; import com.comphenix.protocol.reflect.instances.DefaultInstances; import com.comphenix.protocol.reflect.instances.ExistingGenerator; -import com.comphenix.protocol.reflect.instances.PrimitiveGenerator; /** * Represents a player hook into the NetServerHandler class. @@ -87,7 +85,7 @@ public class NetworkServerInjector extends PlayerInjector { Callback sendPacketCallback = new MethodInterceptor() { @Override public Object intercept(Object obj, Method method, Object[] args, MethodProxy proxy) throws Throwable { - + Packet packet = (Packet) args[0]; if (packet != null) { @@ -121,10 +119,10 @@ public class NetworkServerInjector extends PlayerInjector { // Use the existing field values when we create our copy DefaultInstances serverInstances = DefaultInstances.fromArray( - ExistingGenerator.fromObjectFields(serverHandler), - PrimitiveGenerator.INSTANCE, - CollectionGenerator.INSTANCE); - + ExistingGenerator.fromObjectFields(serverHandler)); + serverInstances.setNonNull(true); + serverInstances.setMaximumRecursion(1); + Object proxyObject = serverInstances.forEnhancer(ex).getDefault(serverClass); // Inject it now @@ -144,23 +142,23 @@ public class NetworkServerInjector extends PlayerInjector { if (serverHandlerRef != null && serverHandlerRef.isCurrentSet()) { ObjectCloner.copyTo(serverHandlerRef.getValue(), serverHandlerRef.getOldValue(), serverHandler.getClass()); serverHandlerRef.revertValue(); - } - - serverInjection.revertServerHandler(serverHandler); - - try { - if (getNetHandler() != null) { - // Restore packet listener - try { - FieldUtils.writeField(netHandlerField, networkManager, serverHandlerRef.getOldValue(), true); - } catch (IllegalAccessException e) { - // Oh well - e.printStackTrace(); + + try { + if (getNetHandler() != null) { + // Restore packet listener + try { + FieldUtils.writeField(netHandlerField, networkManager, serverHandlerRef.getOldValue(), true); + } catch (IllegalAccessException e) { + // Oh well + e.printStackTrace(); + } } + } catch (IllegalAccessException e) { + e.printStackTrace(); } - } catch (IllegalAccessException e) { - e.printStackTrace(); } + + serverInjection.revertServerHandler(serverHandler); } @Override diff --git a/ProtocolLib/src/com/comphenix/protocol/reflect/instances/DefaultInstances.java b/ProtocolLib/src/com/comphenix/protocol/reflect/instances/DefaultInstances.java index 7b846077..8c3b8ebc 100644 --- a/ProtocolLib/src/com/comphenix/protocol/reflect/instances/DefaultInstances.java +++ b/ProtocolLib/src/com/comphenix/protocol/reflect/instances/DefaultInstances.java @@ -41,13 +41,18 @@ public class DefaultInstances { /** * The maximum height of the hierachy of creates types. Used to prevent cycles. */ - private final static int MAXIMUM_RECURSION = 20; + private int maximumRecursion = 20; /** * Ordered list of instance provider, from highest priority to lowest. */ private ImmutableList registered; + /** + * Whether or not the constructor must be non-null. + */ + private boolean nonNull; + /** * Construct a default instance generator using the given instance providers. * @param registered - list of instance providers. @@ -56,6 +61,16 @@ public class DefaultInstances { this.registered = registered; } + /** + * Copy a given instance provider. + * @param other - instance provider to copy. + */ + public DefaultInstances(DefaultInstances other) { + this.nonNull = other.nonNull; + this.maximumRecursion = other.maximumRecursion; + this.registered = other.registered; + } + /** * Construct a default instance generator using the given instance providers. * @param instaceProviders - array of instance providers. @@ -81,6 +96,40 @@ public class DefaultInstances { return registered; } + /** + * Retrieve whether or not the constructor's parameters must be non-null. + * @return TRUE if they must be non-null, FALSE otherwise. + */ + public boolean isNonNull() { + return nonNull; + } + + /** + * Set whether or not the constructor's parameters must be non-null. + * @param nonNull - TRUE if they must be non-null, FALSE otherwise. + */ + public void setNonNull(boolean nonNull) { + this.nonNull = nonNull; + } + + /** + * Retrieve the the maximum height of the hierachy of creates types. + * @return Maximum height. + */ + public int getMaximumRecursion() { + return maximumRecursion; + } + + /** + * Set the maximum height of the hierachy of creates types. Used to prevent cycles. + * @param maximumRecursion - maximum recursion height. + */ + public void setMaximumRecursion(int maximumRecursion) { + if (maximumRecursion < 1) + throw new IllegalArgumentException("Maxmimum recursion height must be one or higher."); + this.maximumRecursion = maximumRecursion; + } + /** * Retrieves a default instance or value that is assignable to this type. *

@@ -158,12 +207,8 @@ public class DefaultInstances { @SuppressWarnings("unchecked") private T getDefaultInternal(Class type, List providers, int recursionLevel) { - - // Guard against recursion - if (recursionLevel > MAXIMUM_RECURSION) { - return null; - } - + + // The instance providiers should protect themselves against recursion for (InstanceProvider generator : providers) { Object value = generator.create(type); @@ -171,6 +216,11 @@ public class DefaultInstances { return (T) value; } + // Guard against recursion + if (recursionLevel >= maximumRecursion) { + return null; + } + Constructor minimum = getMinimumConstructor(type); // Create the type with this constructor using default values. This might fail, though. @@ -183,13 +233,18 @@ public class DefaultInstances { // Fill out for (int i = 0; i < parameterCount; i++) { params[i] = getDefaultInternal(types[i], providers, recursionLevel + 1); + + // Did we break the non-null contract? + if (params[i] == null && nonNull) + return null; } - + return createInstance(type, minimum, types, params); } } catch (Exception e) { // Nope, we couldn't create this type + e.printStackTrace(); } // No suitable default value could be found @@ -204,7 +259,7 @@ public class DefaultInstances { public DefaultInstances forEnhancer(Enhancer enhancer) { final Enhancer ex = enhancer; - return new DefaultInstances(registered) { + return new DefaultInstances(this) { @SuppressWarnings("unchecked") @Override protected T createInstance(Class type, Constructor constructor, Class[] types, Object[] params) {