From a295a28e4be9dbace211f278ffec309e91a96f86 Mon Sep 17 00:00:00 2001 From: Juergen Hoeller Date: Thu, 30 Sep 2021 17:33:58 +0200 Subject: [PATCH 1/2] Defensively handle fast class generation failure for individual methods Includes rethrowing of last actual defineClass exception encountered. Closes gh-27490 --- .../aop/framework/CglibAopProxy.java | 36 ++++++++++---- .../cglib/core/ReflectUtils.java | 48 ++++++++++--------- 2 files changed, 52 insertions(+), 32 deletions(-) diff --git a/spring-aop/src/main/java/org/springframework/aop/framework/CglibAopProxy.java b/spring-aop/src/main/java/org/springframework/aop/framework/CglibAopProxy.java index 43d747f90c..022cc0fddf 100644 --- a/spring-aop/src/main/java/org/springframework/aop/framework/CglibAopProxy.java +++ b/spring-aop/src/main/java/org/springframework/aop/framework/CglibAopProxy.java @@ -679,13 +679,19 @@ class CglibAopProxy implements AopProxy, Serializable { Object retVal; // Check whether we only have one InvokerInterceptor: that is, // no real advice, but just reflective invocation of the target. - if (chain.isEmpty() && Modifier.isPublic(method.getModifiers())) { + if (chain.isEmpty() && CglibMethodInvocation.isMethodProxyCompatible(method)) { // We can skip creating a MethodInvocation: just invoke the target directly. // Note that the final invoker must be an InvokerInterceptor, so we know // it does nothing but a reflective operation on the target, and no hot // swapping or fancy proxying. Object[] argsToUse = AopProxyUtils.adaptArgumentsIfNecessary(method, args); - retVal = methodProxy.invoke(target, argsToUse); + try { + retVal = methodProxy.invoke(target, argsToUse); + } + catch (CodeGenerationException ex) { + CglibMethodInvocation.logFastClassGenerationFailure(method); + retVal = AopUtils.invokeJoinpointUsingReflection(target, method, argsToUse); + } } else { // We need to create a method invocation... @@ -737,10 +743,7 @@ class CglibAopProxy implements AopProxy, Serializable { super(proxy, target, method, arguments, targetClass, interceptorsAndDynamicMethodMatchers); // Only use method proxy for public methods not derived from java.lang.Object - this.methodProxy = (Modifier.isPublic(method.getModifiers()) && - method.getDeclaringClass() != Object.class && !AopUtils.isEqualsMethod(method) && - !AopUtils.isHashCodeMethod(method) && !AopUtils.isToStringMethod(method) ? - methodProxy : null); + this.methodProxy = (isMethodProxyCompatible(method) ? methodProxy : null); } @Override @@ -776,10 +779,25 @@ class CglibAopProxy implements AopProxy, Serializable { @Override protected Object invokeJoinpoint() throws Throwable { if (this.methodProxy != null) { - return this.methodProxy.invoke(this.target, this.arguments); + try { + return this.methodProxy.invoke(this.target, this.arguments); + } + catch (CodeGenerationException ex) { + logFastClassGenerationFailure(this.method); + } } - else { - return super.invokeJoinpoint(); + return super.invokeJoinpoint(); + } + + static boolean isMethodProxyCompatible(Method method) { + return (Modifier.isPublic(method.getModifiers()) && + method.getDeclaringClass() != Object.class && !AopUtils.isEqualsMethod(method) && + !AopUtils.isHashCodeMethod(method) && !AopUtils.isToStringMethod(method)); + } + + static void logFastClassGenerationFailure(Method method) { + if (logger.isDebugEnabled()) { + logger.debug("Failed to generate CGLIB fast class for method: " + method); } } } diff --git a/spring-core/src/main/java/org/springframework/cglib/core/ReflectUtils.java b/spring-core/src/main/java/org/springframework/cglib/core/ReflectUtils.java index 4420c50c9e..2f3ac37e65 100644 --- a/spring-core/src/main/java/org/springframework/cglib/core/ReflectUtils.java +++ b/spring-core/src/main/java/org/springframework/cglib/core/ReflectUtils.java @@ -63,17 +63,16 @@ public class ReflectUtils { private static final Method classLoaderDefineClassMethod; - private static final ProtectionDomain PROTECTION_DOMAIN; - private static final Throwable THROWABLE; + private static final ProtectionDomain PROTECTION_DOMAIN; + private static final List OBJECT_METHODS = new ArrayList(); static { Method privateLookupIn; Method lookupDefineClass; Method classLoaderDefineClass; - ProtectionDomain protectionDomain; Throwable throwable = null; try { privateLookupIn = (Method) AccessController.doPrivileged(new PrivilegedExceptionAction() { @@ -102,39 +101,37 @@ public class ReflectUtils { String.class, byte[].class, Integer.TYPE, Integer.TYPE, ProtectionDomain.class); } }); - protectionDomain = getProtectionDomain(ReflectUtils.class); - AccessController.doPrivileged(new PrivilegedExceptionAction() { - public Object run() throws Exception { - Method[] methods = Object.class.getDeclaredMethods(); - for (Method method : methods) { - if ("finalize".equals(method.getName()) - || (method.getModifiers() & (Modifier.FINAL | Modifier.STATIC)) > 0) { - continue; - } - OBJECT_METHODS.add(method); - } - return null; - } - }); } catch (Throwable t) { privateLookupIn = null; lookupDefineClass = null; classLoaderDefineClass = null; - protectionDomain = null; throwable = t; } + privateLookupInMethod = privateLookupIn; lookupDefineClassMethod = lookupDefineClass; classLoaderDefineClassMethod = classLoaderDefineClass; - PROTECTION_DOMAIN = protectionDomain; THROWABLE = throwable; + PROTECTION_DOMAIN = getProtectionDomain(ReflectUtils.class); + + AccessController.doPrivileged(new PrivilegedAction() { + public Object run() { + Method[] methods = Object.class.getDeclaredMethods(); + for (Method method : methods) { + if ("finalize".equals(method.getName()) + || (method.getModifiers() & (Modifier.FINAL | Modifier.STATIC)) > 0) { + continue; + } + OBJECT_METHODS.add(method); + } + return null; + } + }); } // SPRING PATCH END - private static final String[] CGLIB_PACKAGES = { - "java.lang", - }; + private static final String[] CGLIB_PACKAGES = {"java.lang"}; static { primitives.put("byte", Byte.TYPE); @@ -499,6 +496,7 @@ public class ReflectUtils { ProtectionDomain protectionDomain, Class contextClass) throws Exception { Class c = null; + Throwable t = THROWABLE; // Preferred option: JDK 9+ Lookup.defineClass API if ClassLoader matches if (contextClass != null && contextClass.getClassLoader() == loader && @@ -516,6 +514,7 @@ public class ReflectUtils { // in case of plain LinkageError (class already defined) // or IllegalArgumentException (class in different package): // fall through to traditional ClassLoader.defineClass below + t = ex; } catch (Throwable ex) { throw new CodeGenerationException(ex); @@ -539,9 +538,11 @@ public class ReflectUtils { throw new CodeGenerationException(ex.getTargetException()); } // in case of UnsupportedOperationException, fall through + t = ex.getTargetException(); } catch (Throwable ex) { // publicDefineClass method not available -> fall through + t = ex; } // Classic option: protected ClassLoader.defineClass method @@ -562,6 +563,7 @@ public class ReflectUtils { if (!ex.getClass().getName().endsWith("InaccessibleObjectException")) { throw new CodeGenerationException(ex); } + t = ex; } } } @@ -584,7 +586,7 @@ public class ReflectUtils { // No defineClass variant available at all? if (c == null) { - throw new CodeGenerationException(THROWABLE); + throw new CodeGenerationException(t); } // Force static initializers to run. From 4f44ae3f282b72d7051a2ac67ec5b001ebd9e884 Mon Sep 17 00:00:00 2001 From: Juergen Hoeller Date: Thu, 30 Sep 2021 17:34:22 +0200 Subject: [PATCH 2/2] Polishing --- .../junit/jupiter/SpringExtension.java | 22 +++++++++---------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/spring-test/src/main/java/org/springframework/test/context/junit/jupiter/SpringExtension.java b/spring-test/src/main/java/org/springframework/test/context/junit/jupiter/SpringExtension.java index dfd7aaaa28..8861c5e362 100644 --- a/spring-test/src/main/java/org/springframework/test/context/junit/jupiter/SpringExtension.java +++ b/spring-test/src/main/java/org/springframework/test/context/junit/jupiter/SpringExtension.java @@ -89,8 +89,8 @@ public class SpringExtension implements BeforeAllCallback, AfterAllCallback, Tes * {@link Namespace} in which {@code @Autowired} validation error messages * are stored, keyed by test class. */ - private static final Namespace AUTOWIRED_VALIDATION_NAMESPACE = Namespace.create(SpringExtension.class.getName() + - "#autowired.validation"); + private static final Namespace AUTOWIRED_VALIDATION_NAMESPACE = + Namespace.create(SpringExtension.class.getName() + "#autowired.validation"); private static final String NO_AUTOWIRED_VIOLATIONS_DETECTED = "NO AUTOWIRED VIOLATIONS DETECTED"; @@ -101,8 +101,8 @@ public class SpringExtension implements BeforeAllCallback, AfterAllCallback, Tes private static final MethodFilter autowiredTestOrLifecycleMethodFilter = ReflectionUtils.USER_DECLARED_METHODS - .and(method -> !Modifier.isPrivate(method.getModifiers())) - .and(SpringExtension::isAutowiredTestOrLifecycleMethod); + .and(method -> !Modifier.isPrivate(method.getModifiers())) + .and(SpringExtension::isAutowiredTestOrLifecycleMethod); /** @@ -147,16 +147,16 @@ public class SpringExtension implements BeforeAllCallback, AfterAllCallback, Tes // We save the result in the ExtensionContext.Store so that we don't // re-validate all methods for the same test class multiple times. Store store = context.getStore(AUTOWIRED_VALIDATION_NAMESPACE); - String errorMessage = store.getOrComputeIfAbsent(context.getRequiredTestClass(), - testClass -> { + + String errorMessage = store.getOrComputeIfAbsent(context.getRequiredTestClass(), testClass -> { Method[] methodsWithErrors = ReflectionUtils.getUniqueDeclaredMethods(testClass, autowiredTestOrLifecycleMethodFilter); return (methodsWithErrors.length == 0 ? NO_AUTOWIRED_VIOLATIONS_DETECTED : String.format( - "Test methods and test lifecycle methods must not be annotated with @Autowired. " + - "You should instead annotate individual method parameters with @Autowired, " + - "@Qualifier, or @Value. Offending methods in test class %s: %s", - testClass.getName(), Arrays.toString(methodsWithErrors))); + "Test methods and test lifecycle methods must not be annotated with @Autowired. " + + "You should instead annotate individual method parameters with @Autowired, " + + "@Qualifier, or @Value. Offending methods in test class %s: %s", + testClass.getName(), Arrays.toString(methodsWithErrors))); }, String.class); if (errorMessage != NO_AUTOWIRED_VIOLATIONS_DETECTED) { @@ -247,7 +247,7 @@ public class SpringExtension implements BeforeAllCallback, AfterAllCallback, Tes private boolean supportsApplicationEvents(ParameterContext parameterContext) { if (ApplicationEvents.class.isAssignableFrom(parameterContext.getParameter().getType())) { Assert.isTrue(parameterContext.getDeclaringExecutable() instanceof Method, - "ApplicationEvents can only be injected into test and lifecycle methods"); + "ApplicationEvents can only be injected into test and lifecycle methods"); return true; } return false;