From e52973cc53d9e2f4986e722216a1ef4b7ea9a8bd Mon Sep 17 00:00:00 2001 From: Liam Miller-Cushon Date: Sun, 18 Sep 2016 02:59:37 -0700 Subject: [PATCH] Add checkcasts for arguments of invoked bridges This fixes a TODO in Enhancer: the assumption that a bridge's parameters are assignable to the target's parameters doesn't hold in certain cross-compilation scenarios. The included test case fails with a VerifyError if the checkcasts are omitted. Also, make BridgeMethodResolver use the same class loader as Enhancer to make it easier to test. --- .../sf/cglib/proxy/BridgeMethodResolver.java | 14 +- .../net/sf/cglib/proxy/CallbackGenerator.java | 2 +- .../java/net/sf/cglib/proxy/Enhancer.java | 20 ++- .../proxy/MethodInterceptorGenerator.java | 3 +- .../net/sf/cglib/proxy/NoOpGenerator.java | 3 +- .../java/net/sf/cglib/proxy/TestEnhancer.java | 167 +++++++++++++++++- 6 files changed, 186 insertions(+), 23 deletions(-) diff --git a/cglib/src/main/java/net/sf/cglib/proxy/BridgeMethodResolver.java b/cglib/src/main/java/net/sf/cglib/proxy/BridgeMethodResolver.java index 574857c1..6eb23a02 100644 --- a/cglib/src/main/java/net/sf/cglib/proxy/BridgeMethodResolver.java +++ b/cglib/src/main/java/net/sf/cglib/proxy/BridgeMethodResolver.java @@ -42,9 +42,11 @@ class BridgeMethodResolver { private final Map/* */declToBridge; + private final ClassLoader classLoader; - public BridgeMethodResolver(Map declToBridge) { + public BridgeMethodResolver(Map declToBridge, ClassLoader classLoader) { this.declToBridge = declToBridge; + this.classLoader = classLoader; } /** @@ -58,7 +60,7 @@ public BridgeMethodResolver(Map declToBridge) { Class owner = (Class)entry.getKey(); Set bridges = (Set)entry.getValue(); try { - new ClassReader(owner.getName()) + new ClassReader(classLoader.getResourceAsStream(owner.getName().replace('.', '/') + ".class")) .accept(new BridgedFinder(bridges, resolved), ClassReader.SKIP_FRAMES | ClassReader.SKIP_DEBUG); } catch(IOException ignored) {} @@ -68,14 +70,14 @@ public BridgeMethodResolver(Map declToBridge) { private static class BridgedFinder extends ClassVisitor { private Map/**/ resolved; - private Set/**/ eligableMethods; + private Set/**/ eligibleMethods; private Signature currentMethod = null; - BridgedFinder(Set eligableMethods, Map resolved) { + BridgedFinder(Set eligibleMethods, Map resolved) { super(Opcodes.ASM5); this.resolved = resolved; - this.eligableMethods = eligableMethods; + this.eligibleMethods = eligibleMethods; } public void visit(int version, int access, String name, @@ -85,7 +87,7 @@ public void visit(int version, int access, String name, public MethodVisitor visitMethod(int access, String name, String desc, String signature, String[] exceptions) { Signature sig = new Signature(name, desc); - if (eligableMethods.remove(sig)) { + if (eligibleMethods.remove(sig)) { currentMethod = sig; return new MethodVisitor(Opcodes.ASM5) { public void visitMethodInsn(int opcode, String owner, String name, diff --git a/cglib/src/main/java/net/sf/cglib/proxy/CallbackGenerator.java b/cglib/src/main/java/net/sf/cglib/proxy/CallbackGenerator.java index 283f22ac..eb4b5e3b 100644 --- a/cglib/src/main/java/net/sf/cglib/proxy/CallbackGenerator.java +++ b/cglib/src/main/java/net/sf/cglib/proxy/CallbackGenerator.java @@ -31,6 +31,6 @@ interface Context int getIndex(MethodInfo method); void emitCallback(CodeEmitter ce, int index); Signature getImplSignature(MethodInfo method); - void emitInvoke(CodeEmitter e, MethodInfo method); + void emitLoadArgsAndInvoke(CodeEmitter e, MethodInfo method); } } diff --git a/cglib/src/main/java/net/sf/cglib/proxy/Enhancer.java b/cglib/src/main/java/net/sf/cglib/proxy/Enhancer.java index 7e7d831b..600b73d9 100644 --- a/cglib/src/main/java/net/sf/cglib/proxy/Enhancer.java +++ b/cglib/src/main/java/net/sf/cglib/proxy/Enhancer.java @@ -1129,7 +1129,7 @@ private void emitMethods(final ClassEmitter ce, List methods, List actualMethods } } - final Map bridgeToTarget = new BridgeMethodResolver(declToBridge).resolveAll(); + final Map bridgeToTarget = new BridgeMethodResolver(declToBridge, getClassLoader()).resolveAll(); Set seenGen = new HashSet(); CodeEmitter se = ce.getStaticHook(); @@ -1155,19 +1155,22 @@ public void emitCallback(CodeEmitter e, int index) { public Signature getImplSignature(MethodInfo method) { return rename(method.getSignature(), ((Integer)positions.get(method)).intValue()); } - public void emitInvoke(CodeEmitter e, MethodInfo method) { + public void emitLoadArgsAndInvoke(CodeEmitter e, MethodInfo method) { // If this is a bridge and we know the target was called from invokespecial, // then we need to invoke_virtual w/ the bridge target instead of doing // a super, because super may itself be using super, which would bypass // any proxies on the target. Signature bridgeTarget = (Signature)bridgeToTarget.get(method.getSignature()); if (bridgeTarget != null) { - // TODO: this assumes that the target has wider or the same type - // parameters than the current. - // In reality this should always be true because otherwise we wouldn't - // have had a bridge doing an invokespecial. - // If it isn't true, we would need to checkcast each argument - // against the target's argument types + // checkcast each argument against the target's argument types + for (int i = 0; i < bridgeTarget.getArgumentTypes().length; i++) { + e.load_arg(i); + Type target = bridgeTarget.getArgumentTypes()[i]; + if (!target.equals(method.getSignature().getArgumentTypes()[i])) { + e.checkcast(target); + } + } + e.invoke_virtual_this(bridgeTarget); Type retType = method.getSignature().getReturnType(); @@ -1185,6 +1188,7 @@ public void emitInvoke(CodeEmitter e, MethodInfo method) { e.checkcast(retType); } } else { + e.load_args(); e.super_invoke(method.getSignature()); } } diff --git a/cglib/src/main/java/net/sf/cglib/proxy/MethodInterceptorGenerator.java b/cglib/src/main/java/net/sf/cglib/proxy/MethodInterceptorGenerator.java index b0e00a9d..9c86f21d 100644 --- a/cglib/src/main/java/net/sf/cglib/proxy/MethodInterceptorGenerator.java +++ b/cglib/src/main/java/net/sf/cglib/proxy/MethodInterceptorGenerator.java @@ -139,8 +139,7 @@ private static void superHelper(CodeEmitter e, MethodInfo method, Context contex e.throw_exception(ABSTRACT_METHOD_ERROR, method.toString() + " is abstract" ); } else { e.load_this(); - e.load_args(); - context.emitInvoke(e, method); + context.emitLoadArgsAndInvoke(e, method); } } diff --git a/cglib/src/main/java/net/sf/cglib/proxy/NoOpGenerator.java b/cglib/src/main/java/net/sf/cglib/proxy/NoOpGenerator.java index 84920be6..655a5e68 100644 --- a/cglib/src/main/java/net/sf/cglib/proxy/NoOpGenerator.java +++ b/cglib/src/main/java/net/sf/cglib/proxy/NoOpGenerator.java @@ -32,8 +32,7 @@ public void generate(ClassEmitter ce, Context context, List methods) { TypeUtils.isPublic(method.getModifiers()))) { CodeEmitter e = EmitUtils.begin_method(ce, method); e.load_this(); - e.load_args(); - context.emitInvoke(e, method); + context.emitLoadArgsAndInvoke(e, method); e.return_value(); e.end_method(); } diff --git a/cglib/src/test/java/net/sf/cglib/proxy/TestEnhancer.java b/cglib/src/test/java/net/sf/cglib/proxy/TestEnhancer.java index 03a7db1f..19b19cd9 100644 --- a/cglib/src/test/java/net/sf/cglib/proxy/TestEnhancer.java +++ b/cglib/src/test/java/net/sf/cglib/proxy/TestEnhancer.java @@ -15,6 +15,7 @@ */ package net.sf.cglib.proxy; +import java.io.ByteArrayInputStream; import java.io.ByteArrayOutputStream; import java.io.IOException; import java.io.InputStream; @@ -26,8 +27,9 @@ import java.lang.reflect.Modifier; import java.util.ArrayList; import java.util.Arrays; +import java.util.HashMap; import java.util.List; - +import java.util.Map; import junit.framework.Test; import junit.framework.TestCase; import junit.framework.TestSuite; @@ -38,6 +40,9 @@ import net.sf.cglib.core.Predicate; import net.sf.cglib.core.ReflectUtils; import net.sf.cglib.reflect.FastClass; +import org.objectweb.asm.ClassWriter; +import org.objectweb.asm.MethodVisitor; +import org.objectweb.asm.Opcodes; /** *@author Juozas Baliuka @@ -1256,9 +1261,163 @@ public int accept(Method method) { intf.aMethod(null); assertEquals(Arrays.asList(Concrete.class), paramTypes); } - - - + + public void testBridgeParameterCheckcast() throws Exception { + + // If the compiler used for Z omits the bridge method, and X is compiled with javac, + // javac will generate an invokespecial bridge in X. + + // public interface I { + // public A f(B b); + // } + // public abstract class Z implements I { + // public U f(Long id) { + // return null; + // } + // } + // public class X extends Z {} + + final Map classes = new HashMap(); + + { + ClassWriter cw = new ClassWriter(0); + cw.visit( + 49, + Opcodes.ACC_PUBLIC | Opcodes.ACC_ABSTRACT | Opcodes.ACC_INTERFACE, + "I", + "Ljava/lang/Object;", + "java/lang/Object", + null); + { + MethodVisitor mv = + cw.visitMethod( + Opcodes.ACC_PUBLIC | Opcodes.ACC_ABSTRACT, + "f", + "(Ljava/lang/Object;)Ljava/lang/Object;", + "(TB;)TA;", + null); + mv.visitEnd(); + } + cw.visitEnd(); + classes.put("I.class", cw.toByteArray()); + } + { + ClassWriter cw = new ClassWriter(0); + cw.visit( + 49, + Opcodes.ACC_PUBLIC | Opcodes.ACC_SUPER | Opcodes.ACC_ABSTRACT, + "Z", + "Ljava/lang/Object;LI;", + "java/lang/Object", + new String[] {"I"}); + { + MethodVisitor mv = cw.visitMethod(Opcodes.ACC_PUBLIC, "", "()V", null, null); + mv.visitCode(); + mv.visitVarInsn(Opcodes.ALOAD, 0); + mv.visitMethodInsn(Opcodes.INVOKESPECIAL, "java/lang/Object", "", "()V", false); + mv.visitInsn(Opcodes.RETURN); + mv.visitMaxs(1, 1); + mv.visitEnd(); + } + { + MethodVisitor mv = + cw.visitMethod( + Opcodes.ACC_PUBLIC, + "f", + "(Ljava/lang/String;)Ljava/lang/Number;", + "(Ljava/lang/String;)TU;", + null); + mv.visitCode(); + mv.visitInsn(Opcodes.ACONST_NULL); + mv.visitInsn(Opcodes.ARETURN); + mv.visitMaxs(1, 2); + mv.visitEnd(); + } + cw.visitEnd(); + classes.put("Z.class", cw.toByteArray()); + } + { + ClassWriter cw = new ClassWriter(0); + cw.visit( + 49, Opcodes.ACC_PUBLIC | Opcodes.ACC_SUPER, "X", "LZ;", "Z", null); + { + MethodVisitor mv = cw.visitMethod(Opcodes.ACC_PUBLIC, "", "()V", null, null); + mv.visitCode(); + mv.visitVarInsn(Opcodes.ALOAD, 0); + mv.visitMethodInsn(Opcodes.INVOKESPECIAL, "Z", "", "()V", false); + mv.visitInsn(Opcodes.RETURN); + mv.visitMaxs(1, 1); + mv.visitEnd(); + } + { + MethodVisitor mv = + cw.visitMethod( + Opcodes.ACC_PUBLIC | Opcodes.ACC_BRIDGE | Opcodes.ACC_SYNTHETIC, + "f", + "(Ljava/lang/Object;)Ljava/lang/Object;", + null, + null); + mv.visitCode(); + mv.visitVarInsn(Opcodes.ALOAD, 0); + mv.visitVarInsn(Opcodes.ALOAD, 1); + mv.visitTypeInsn(Opcodes.CHECKCAST, "java/lang/String"); + mv.visitMethodInsn( + Opcodes.INVOKESPECIAL, "Z", "f", "(Ljava/lang/String;)Ljava/lang/Number;", false); + mv.visitInsn(Opcodes.ARETURN); + mv.visitMaxs(2, 2); + mv.visitEnd(); + } + + cw.visitEnd(); + + classes.put("X.class", cw.toByteArray()); + } + + ClassLoader classLoader = + new ClassLoader(getClass().getClassLoader()) { + @Override + public InputStream getResourceAsStream(String name) { + InputStream is = super.getResourceAsStream(name); + if (is != null) { + return is; + } + if (classes.containsKey(name)) { + return new ByteArrayInputStream(classes.get(name)); + } + return null; + } + + public Class findClass(String name) throws ClassNotFoundException { + byte[] ba = classes.get(name.replace('.', '/') + ".class"); + if (ba != null) { + return defineClass(name, ba, 0, ba.length); + } + throw new ClassNotFoundException(name); + } + }; + + List retTypes = new ArrayList(); + List paramTypes = new ArrayList(); + Interceptor interceptor = new Interceptor(retTypes, paramTypes); + + Enhancer e = new Enhancer(); + e.setClassLoader(classLoader); + e.setSuperclass(classLoader.loadClass("X")); + e.setCallback(interceptor); + + Object c = e.create(); + + for (Method m : c.getClass().getDeclaredMethods()) { + if (m.getName().equals("f") && m.getReturnType().equals(Object.class)) { + m.invoke(c, new Object[] {null}); + } + } + + // f(Object)Object should bridge to f(Number)String + assertEquals(Arrays.asList(Object.class, Number.class), retTypes); + assertEquals(Arrays.asList(Object.class, String.class), paramTypes); + } + static class ErasedType {} static class RetType extends ErasedType {} static class Refined extends RetType {}