CglibAopProxy detects package-visible methods when defined in a different ClassLoader

Issue: SPR-11618

spring-aop/src/main/java/org/springframework/aop/framework/CglibAopProxy.java

@@ -172,7 +172,7 @@ class CglibAopProxy implements AopProxy, Serializable {
 			}
 
 			// Validate the class, writing log messages as necessary.
-			validateClassIfNecessary(proxySuperClass);
+			validateClassIfNecessary(proxySuperClass, classLoader);
 
 			// Configure CGLIB Enhancer...
 			Enhancer enhancer = createEnhancer();
@@ -239,11 +239,11 @@ class CglibAopProxy implements AopProxy, Serializable {
 	 * Checks to see whether the supplied {@code Class} has already been validated and
 	 * validates it if not.
 	 */
-	private void validateClassIfNecessary(Class<?> proxySuperClass) {
-		if (logger.isWarnEnabled()) {
+	private void validateClassIfNecessary(Class<?> proxySuperClass, ClassLoader proxyClassLoader) {
+		if (logger.isInfoEnabled()) {
 			synchronized (validatedClasses) {
 				if (!validatedClasses.containsKey(proxySuperClass)) {
-					doValidateClass(proxySuperClass);
+					doValidateClass(proxySuperClass, proxyClassLoader);
 					validatedClasses.put(proxySuperClass, Boolean.TRUE);
 				}
 			}
@@ -251,19 +251,28 @@ class CglibAopProxy implements AopProxy, Serializable {
 	}
 
 	/**
-	 * Checks for final methods on the {@code Class} and writes warnings to the log
-	 * for each one found.
+	 * Checks for final methods on the given {@code Class}, as well as package-visible
+	 * methods across ClassLoaders, and writes warnings to the log for each one found.
 	 */
-	private void doValidateClass(Class<?> proxySuperClass) {
-		if (logger.isWarnEnabled()) {
-			Method[] methods = proxySuperClass.getMethods();
+	private void doValidateClass(Class<?> proxySuperClass, ClassLoader proxyClassLoader) {
+		if (!Object.class.equals(proxySuperClass)) {
+			Method[] methods = proxySuperClass.getDeclaredMethods();
 			for (Method method : methods) {
-				if (!Object.class.equals(method.getDeclaringClass()) && !Modifier.isStatic(method.getModifiers()) &&
-						Modifier.isFinal(method.getModifiers())) {
-					logger.warn("Unable to proxy method [" + method + "] because it is final: " +
-							"All calls to this method via a proxy will be routed directly to the proxy.");
+				int mod = method.getModifiers();
+				if (!Modifier.isStatic(mod)) {
+					if (Modifier.isFinal(mod)) {
+						logger.info("Unable to proxy method [" + method + "] because it is final: " +
+								"All calls to this method via a proxy will NOT be routed to the target instance.");
+					}
+					else if (!Modifier.isPublic(mod) && !Modifier.isProtected(mod) && !Modifier.isPrivate(mod) &&
+							proxyClassLoader != null && proxySuperClass.getClassLoader() != proxyClassLoader) {
+						logger.info("Unable to proxy method [" + method + "] because it is package-visible " +
+								"across different ClassLoaders: All calls to this method via a proxy will " +
+								"NOT be routed to the target instance.");
+					}
 				}
 			}
+			doValidateClass(proxySuperClass.getSuperclass(), proxyClassLoader);
 		}
 	}
 
@@ -604,7 +613,7 @@ class CglibAopProxy implements AopProxy, Serializable {
 	 */
 	private static class DynamicAdvisedInterceptor implements MethodInterceptor, Serializable {
 
-		private AdvisedSupport advised;
+		private final AdvisedSupport advised;
 
 		public DynamicAdvisedInterceptor(AdvisedSupport advised) {
 			this.advised = advised;
@@ -622,8 +631,8 @@ class CglibAopProxy implements AopProxy, Serializable {
 					oldProxy = AopContext.setCurrentProxy(proxy);
 					setProxyContext = true;
 				}
-				// May be null Get as late as possible to minimize the time we
-				// "own" the target, in case it comes from a pool.
+				// May be null. Get as late as possible to minimize the time we
+				// "own" the target, in case it comes from a pool...
 				target = getTarget();
 				if (target != null) {
 					targetClass = target.getClass();
@@ -689,13 +698,13 @@ class CglibAopProxy implements AopProxy, Serializable {
 
 		private final MethodProxy methodProxy;
 
-		private boolean protectedMethod;
+		private final boolean publicMethod;
 
 		public CglibMethodInvocation(Object proxy, Object target, Method method, Object[] arguments,
 				Class<?> targetClass, List<Object> interceptorsAndDynamicMethodMatchers, MethodProxy methodProxy) {
 			super(proxy, target, method, arguments, targetClass, interceptorsAndDynamicMethodMatchers);
 			this.methodProxy = methodProxy;
-			this.protectedMethod = Modifier.isProtected(method.getModifiers());
+			this.publicMethod = Modifier.isPublic(method.getModifiers());
 		}
 
 		/**
@@ -704,11 +713,11 @@ class CglibAopProxy implements AopProxy, Serializable {
 		 */
 		@Override
 		protected Object invokeJoinpoint() throws Throwable {
-			if (this.protectedMethod) {
-				return super.invokeJoinpoint();
+			if (this.publicMethod) {
+				return this.methodProxy.invoke(this.target, this.arguments);
 			}
 			else {
-				return this.methodProxy.invoke(this.target, this.arguments);
+				return super.invokeJoinpoint();
 			}
 		}
 	}
@@ -829,8 +838,8 @@ class CglibAopProxy implements AopProxy, Serializable {
 				// of the target type. If so we know it never needs to have return type
 				// massage and can use a dispatcher.
 				// If the proxy is being exposed, then must use the interceptor the
-				// correct one is already configured. If the target is not static cannot
-				// use a Dispatcher because the target can not then be released.
+				// correct one is already configured. If the target is not static, then
+				// cannot use a dispatcher because the target cannot be released.
 				if (exposeProxy || !isStatic) {
 					return INVOKE_TARGET;
 				}

spring-context/src/test/java/org/springframework/aop/framework/CglibProxyTests.java

@@ -102,6 +102,7 @@ public final class CglibProxyTests extends AbstractAopProxyTests implements Seri
 	@Test
 	public void testProtectedMethodInvocation() {
 		ProtectedMethodTestBean bean = new ProtectedMethodTestBean();
+		bean.value = "foo";
 		mockTargetSource.setTarget(bean);
 
 		AdvisedSupport as = new AdvisedSupport(new Class<?>[]{});
@@ -109,8 +110,47 @@ public final class CglibProxyTests extends AbstractAopProxyTests implements Seri
 		as.addAdvice(new NopInterceptor());
 		AopProxy aop = new CglibAopProxy(as);
 
-		Object proxy = aop.getProxy();
+		ProtectedMethodTestBean proxy = (ProtectedMethodTestBean) aop.getProxy();
 		assertTrue(AopUtils.isCglibProxy(proxy));
+		assertEquals(proxy.getClass().getClassLoader(), bean.getClass().getClassLoader());
+		assertEquals("foo", proxy.getString());
+	}
+
+	@Test
+	public void testPackageMethodInvocation() {
+		PackageMethodTestBean bean = new PackageMethodTestBean();
+		bean.value = "foo";
+		mockTargetSource.setTarget(bean);
+
+		AdvisedSupport as = new AdvisedSupport(new Class<?>[]{});
+		as.setTargetSource(mockTargetSource);
+		as.addAdvice(new NopInterceptor());
+		AopProxy aop = new CglibAopProxy(as);
+
+		PackageMethodTestBean proxy = (PackageMethodTestBean) aop.getProxy();
+		assertTrue(AopUtils.isCglibProxy(proxy));
+		assertEquals(proxy.getClass().getClassLoader(), bean.getClass().getClassLoader());
+		assertEquals("foo", proxy.getString());
+	}
+
+	@Test
+	public void testPackageMethodInvocationWithDifferentClassLoader() {
+		ClassLoader child = new ClassLoader(getClass().getClassLoader()) {
+		};
+
+		PackageMethodTestBean bean = new PackageMethodTestBean();
+		bean.value = "foo";
+		mockTargetSource.setTarget(bean);
+
+		AdvisedSupport as = new AdvisedSupport(new Class<?>[]{});
+		as.setTargetSource(mockTargetSource);
+		as.addAdvice(new NopInterceptor());
+		AopProxy aop = new CglibAopProxy(as);
+
+		PackageMethodTestBean proxy = (PackageMethodTestBean) aop.getProxy(child);
+		assertTrue(AopUtils.isCglibProxy(proxy));
+		assertNotEquals(proxy.getClass().getClassLoader(), bean.getClass().getClassLoader());
+		assertNull(proxy.getString());  // we're stuck in the proxy instance
 	}
 
 	@Test
@@ -410,54 +450,59 @@ public final class CglibProxyTests extends AbstractAopProxyTests implements Seri
 	}
 
 
-	public static class HasFinalMethod {
+	public static class NoArgCtorTestBean {
+
+		private boolean called = false;
 
-		public final void foo() {
+		public NoArgCtorTestBean(String x, int y) {
+			called = true;
+		}
+
+		public boolean wasCalled() {
+			return called;
+		}
+
+		public void reset() {
+			called = false;
 		}
 	}
-}
 
 
-class CglibTestBean {
+	public static class ProtectedMethodTestBean {
 
-	private String name;
+		public String value;
 
-	public CglibTestBean() {
-		setName("Some Default");
+		protected String getString() {
+			return this.value;
+		}
 	}
 
-	public void setName(String name) {
-		this.name = name;
-	}
 
-	public String getName() {
-		return this.name;
+	public static class PackageMethodTestBean {
+
+		public String value;
+
+		String getString() {
+			return this.value;
+		}
 	}
 }
 
 
-class NoArgCtorTestBean {
-
-	private boolean called = false;
+class CglibTestBean {
 
-	public NoArgCtorTestBean(String x, int y) {
-		called = true;
-	}
+	private String name;
 
-	public boolean wasCalled() {
-		return called;
+	public CglibTestBean() {
+		setName("Some Default");
 	}
 
-	public void reset() {
-		called = false;
+	public void setName(String name) {
+		this.name = name;
 	}
-}
-
 
-class ProtectedMethodTestBean {
-
-	protected String getString() {
-		return "foo";
+	public String getName() {
+		return this.name;
 	}
 }