From b947ed1193d66fbdabe035c781139e4fdc4905f9 Mon Sep 17 00:00:00 2001 From: "Kristian S. Stangeland" Date: Sun, 8 Dec 2013 07:14:16 +0100 Subject: [PATCH] Synchronize before attempting to modify thread-safe fields. We also re-wrap proxied lists in Collections.synchronizedList(). May fix ticket #157 on BukkitDev. --- .../protocol/injector/netty/ChannelProxy.java | 2 + .../injector/netty/NettyProtocolInjector.java | 14 ++- .../reflect/DefaultFieldAccessor.java | 62 +++++++++++++ .../reflect/DefaultMethodAccessor.java | 54 +++++++++++ .../protocol/reflect/FuzzyReflection.java | 93 ++++++++++++------- .../protocol/reflect/VolatileField.java | 57 +++++++----- 6 files changed, 218 insertions(+), 64 deletions(-) create mode 100644 ProtocolLib/src/main/java/com/comphenix/protocol/reflect/DefaultFieldAccessor.java create mode 100644 ProtocolLib/src/main/java/com/comphenix/protocol/reflect/DefaultMethodAccessor.java diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/injector/netty/ChannelProxy.java b/ProtocolLib/src/main/java/com/comphenix/protocol/injector/netty/ChannelProxy.java index 70c5a593..2f689bbd 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/injector/netty/ChannelProxy.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/injector/netty/ChannelProxy.java @@ -1,5 +1,6 @@ package com.comphenix.protocol.injector.netty; +import java.lang.reflect.Field; import java.net.SocketAddress; import java.util.Map; import java.util.concurrent.Callable; @@ -25,6 +26,7 @@ abstract class ChannelProxy implements Channel { private static final FieldAccessor MARK_NO_MESSAGE = new FieldAccessor() { public void set(Object instance, Object value) { } public Object get(Object instance) { return null; } + public Field getField() { return null; }; }; // Looking up packets in inner classes diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/injector/netty/NettyProtocolInjector.java b/ProtocolLib/src/main/java/com/comphenix/protocol/injector/netty/NettyProtocolInjector.java index 1866d532..4d28097a 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/injector/netty/NettyProtocolInjector.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/injector/netty/NettyProtocolInjector.java @@ -5,6 +5,7 @@ import java.lang.reflect.Field; import java.lang.reflect.InvocationTargetException; import java.lang.reflect.Method; import java.net.InetSocketAddress; +import java.util.Collections; import java.util.List; import java.util.Set; import org.bukkit.Bukkit; @@ -108,9 +109,13 @@ public class NettyProtocolInjector implements ChannelListener { bootstrapFields = getBootstrapFields(serverConnection); for (VolatileField field : bootstrapFields) { - field.setValue(new BootstrapList( - (List) field.getValue(), connectionHandler - )); + final List list = (List) field.getValue(); + + // Synchronize with each list before we attempt to replace them. + // We also reapply the synchronized list wrapper. + field.setValue(Collections.synchronizedList(new BootstrapList( + list, connectionHandler + ))); } injected = true; @@ -148,7 +153,8 @@ public class NettyProtocolInjector implements ChannelListener { // Find and (possibly) proxy every list for (Field field : FuzzyReflection.fromObject(serverConnection, true).getFieldListByType(List.class)) { - VolatileField volatileField = new VolatileField(field, serverConnection, true); + VolatileField volatileField = new VolatileField(field, serverConnection, true).toSynchronized(); + @SuppressWarnings("unchecked") List list = (List) volatileField.getValue(); diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/reflect/DefaultFieldAccessor.java b/ProtocolLib/src/main/java/com/comphenix/protocol/reflect/DefaultFieldAccessor.java new file mode 100644 index 00000000..d3400d18 --- /dev/null +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/reflect/DefaultFieldAccessor.java @@ -0,0 +1,62 @@ +package com.comphenix.protocol.reflect; + +import java.lang.reflect.Field; + +import com.comphenix.protocol.reflect.FuzzyReflection.FieldAccessor; + +final class DefaultFieldAccessor implements FieldAccessor { + private final Field field; + + public DefaultFieldAccessor(Field field) { + this.field = field; + } + + @Override + public Object get(Object instance) { + try { + return field.get(instance); + } catch (IllegalArgumentException e) { + throw new RuntimeException("Cannot read " + field, e); + } catch (IllegalAccessException e) { + throw new IllegalStateException("Cannot use reflection.", e); + } + } + + @Override + public void set(Object instance, Object value) { + try { + field.set(instance, value); + } catch (IllegalArgumentException e) { + throw new RuntimeException("Cannot set field " + field + " to value " + value, e); + } catch (IllegalAccessException e) { + throw new IllegalStateException("Cannot use reflection.", e); + } + } + + @Override + public Field getField() { + return field; + } + + @Override + public int hashCode() { + return field != null ? field.hashCode() : 0; + } + + @Override + public boolean equals(Object obj) { + if (this == obj) + return true; + + if (obj instanceof DefaultFieldAccessor) { + DefaultFieldAccessor other = (DefaultFieldAccessor) obj; + return other.field == field; + } + return true; + } + + @Override + public String toString() { + return "DefaultFieldAccessor [field=" + field + "]"; + } +} \ No newline at end of file diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/reflect/DefaultMethodAccessor.java b/ProtocolLib/src/main/java/com/comphenix/protocol/reflect/DefaultMethodAccessor.java new file mode 100644 index 00000000..0129d411 --- /dev/null +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/reflect/DefaultMethodAccessor.java @@ -0,0 +1,54 @@ +package com.comphenix.protocol.reflect; + +import java.lang.reflect.InvocationTargetException; +import java.lang.reflect.Method; + +import com.comphenix.protocol.reflect.FuzzyReflection.MethodAccessor; + +final class DefaultMethodAccessor implements MethodAccessor { + private final Method method; + + public DefaultMethodAccessor(Method method) { + this.method = method; + } + + @Override + public Object invoke(Object target, Object... args) { + try { + return method.invoke(target, args); + } catch (IllegalAccessException e) { + throw new IllegalStateException("Cannot use reflection.", e); + } catch (InvocationTargetException e) { + throw new RuntimeException("An internal error occured.", e.getCause()); + } catch (IllegalArgumentException e) { + throw e; + } + } + + @Override + public Method getMethod() { + return method; + } + + @Override + public int hashCode() { + return method != null ? method.hashCode() : 0; + } + + @Override + public boolean equals(Object obj) { + if (this == obj) + return true; + + if (obj instanceof DefaultMethodAccessor) { + DefaultMethodAccessor other = (DefaultMethodAccessor) obj; + return other.method == method; + } + return true; + } + + @Override + public String toString() { + return "DefaultMethodAccessor [method=" + method + "]"; + } +} \ No newline at end of file diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/reflect/FuzzyReflection.java b/ProtocolLib/src/main/java/com/comphenix/protocol/reflect/FuzzyReflection.java index 47509f8b..0c887a05 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/reflect/FuzzyReflection.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/reflect/FuzzyReflection.java @@ -19,7 +19,6 @@ package com.comphenix.protocol.reflect; import java.lang.reflect.Constructor; import java.lang.reflect.Field; -import java.lang.reflect.InvocationTargetException; import java.lang.reflect.Method; import java.lang.reflect.Modifier; import java.util.ArrayList; @@ -62,6 +61,12 @@ public class FuzzyReflection { * @param value - the new value of the field. */ public void set(Object instance, Object value); + + /** + * Retrieve the underlying field. + * @return The field. + */ + public Field getField(); } /** @@ -76,6 +81,44 @@ public class FuzzyReflection { * @return The return value, or NULL for void methods. */ public Object invoke(Object target, Object... args); + + /** + * Retrieve the underlying method. + * @return The method. + */ + public Method getMethod(); + } + + /** + * Represents a field accessor that synchronizes access to the underlying field. + * @author Kristian + */ + private static final class SynchronizedFieldAccessor implements FieldAccessor { + private final FieldAccessor accessor; + private SynchronizedFieldAccessor(FieldAccessor accessor) { + this.accessor = accessor; + } + + @Override + public void set(Object instance, Object value) { + Object lock = accessor.get(instance); + + if (lock != null) { + synchronized (lock) { + accessor.set(instance, value); + } + } else { + accessor.set(instance, value); + } + } + @Override + public Object get(Object instance) { + return accessor.get(instance); + } + @Override + public Field getField() { + return accessor.getField(); + } } // The class we're actually representing @@ -170,26 +213,19 @@ public class FuzzyReflection { */ public static FieldAccessor getFieldAccessor(final Field field, boolean forceAccess) { field.setAccessible(true); - - return new FieldAccessor() { - @Override - public Object get(Object instance) { - try { - return field.get(instance); - } catch (IllegalAccessException e) { - throw new IllegalStateException("Cannot use reflection.", e); - } - } - - @Override - public void set(Object instance, Object value) { - try { - field.set(instance, value); - } catch (IllegalAccessException e) { - throw new IllegalStateException("Cannot use reflection.", e); - } - } - }; + return new DefaultFieldAccessor(field); + } + + /** + * Retrieve a field accessor where the write operation is synchronized on the current field value. + * @param accessor - the accessor. + * @return The field accessor. + */ + public static FieldAccessor getSynchronized(final FieldAccessor accessor) { + // Only wrap once + if (accessor instanceof SynchronizedFieldAccessor) + return accessor; + return new SynchronizedFieldAccessor(accessor); } /** @@ -232,20 +268,7 @@ public class FuzzyReflection { * @return The method accessor. */ public static MethodAccessor getMethodAccessor(final Method method) { - return new MethodAccessor() { - @Override - public Object invoke(Object target, Object... args) { - try { - return method.invoke(target, args); - } catch (IllegalAccessException e) { - throw new IllegalStateException("Cannot use reflection.", e); - } catch (InvocationTargetException e) { - throw new RuntimeException("An internal error occured.", e.getCause()); - } catch (IllegalArgumentException e) { - throw e; - } - } - }; + return new DefaultMethodAccessor(method); } /** diff --git a/ProtocolLib/src/main/java/com/comphenix/protocol/reflect/VolatileField.java b/ProtocolLib/src/main/java/com/comphenix/protocol/reflect/VolatileField.java index 1752c87c..bcd7d564 100644 --- a/ProtocolLib/src/main/java/com/comphenix/protocol/reflect/VolatileField.java +++ b/ProtocolLib/src/main/java/com/comphenix/protocol/reflect/VolatileField.java @@ -19,6 +19,7 @@ package com.comphenix.protocol.reflect; import java.lang.reflect.Field; +import com.comphenix.protocol.reflect.FuzzyReflection.FieldAccessor; import com.google.common.base.Objects; /** @@ -27,8 +28,7 @@ import com.google.common.base.Objects; * @author Kristian */ public class VolatileField { - - private Field field; + private FieldAccessor accessor; private Object container; // The current and previous values @@ -48,7 +48,7 @@ public class VolatileField { * @param container - the object this field belongs to. */ public VolatileField(Field field, Object container) { - this.field = field; + this.accessor = FuzzyReflection.getFieldAccessor(field); this.container = container; } @@ -59,17 +59,27 @@ public class VolatileField { * @param forceAccess - whether or not to override any scope restrictions. */ public VolatileField(Field field, Object container, boolean forceAccess) { - this.field = field; + this.accessor = FuzzyReflection.getFieldAccessor(field, true); this.container = container; this.forceAccess = forceAccess; } + /** + * Initializes a volatile field with the given accessor and associated object. + * @param accessor - the field accessor. + * @param container - the object this field belongs to. + */ + public VolatileField(FieldAccessor accessor, Object container) { + this.accessor = accessor; + this.container = container; + } + /** * Retrieves the current field. * @return The stored field. */ public Field getField() { - return field; + return accessor.getField(); } /** @@ -127,14 +137,9 @@ public class VolatileField { // Remember to safe the previous value ensureLoaded(); - try { - FieldUtils.writeField(field, container, newValue, forceAccess); - current = newValue; - currentSet = true; - - } catch (IllegalAccessException e) { - throw new RuntimeException("Unable to write field " + field.getName(), e); - } + writeFieldValue(newValue); + current = newValue; + currentSet = true; } /** @@ -178,11 +183,19 @@ public class VolatileField { } else { // This can be a bad sign System.out.println(String.format("[ProtocolLib] Unable to switch %s to %s. Expected %s but got %s.", - field.toGenericString(), previous, current, getValue())); + getField().toGenericString(), previous, current, getValue())); } } } + /** + * Retrieve a synchronized version of the current field. + * @return A synchronized volatile field. + */ + public VolatileField toSynchronized() { + return new VolatileField(FuzzyReflection.getSynchronized(accessor), container); + } + /** * Determine whether or not we'll need to revert the value. */ @@ -203,11 +216,7 @@ public class VolatileField { * @return The field value. */ private Object readFieldValue() { - try { - return FieldUtils.readField(field, container, forceAccess); - } catch (IllegalAccessException e) { - throw new RuntimeException("Unable to read field " + field.getName(), e); - } + return accessor.get(container); } /** @@ -215,11 +224,7 @@ public class VolatileField { * @param newValue - the new value. */ private void writeFieldValue(Object newValue) { - try { - FieldUtils.writeField(field, container, newValue, forceAccess); - } catch (IllegalAccessException e) { - throw new RuntimeException("Unable to write field " + field.getName(), e); - } + accessor.set(container, newValue); } @Override @@ -229,6 +234,8 @@ public class VolatileField { @Override public String toString() { - return "VolatileField [field=" + field + ", container=" + container + ", previous=" + previous + ", current=" + current + "]"; + return "VolatileField [accessor=" + accessor + ", container=" + container + ", previous=" + + previous + ", current=" + current + ", previousLoaded=" + previousLoaded + + ", currentSet=" + currentSet + ", forceAccess=" + forceAccess + "]"; } }