From f1ddd051a32fd39012f896929b766f69f88f0df0 Mon Sep 17 00:00:00 2001 From: Juergen Hoeller Date: Thu, 5 Jun 2025 20:36:10 +0200 Subject: [PATCH] Restore synchronization against AspectJ race condition behind PointcutExpression Closes gh-34735 --- .../aspectj/AspectJExpressionPointcut.java | 115 ++++++++++-------- .../aop/aspectj/ShadowMatchUtils.java | 62 +++++----- 2 files changed, 93 insertions(+), 84 deletions(-) diff --git a/spring-aop/src/main/java/org/springframework/aop/aspectj/AspectJExpressionPointcut.java b/spring-aop/src/main/java/org/springframework/aop/aspectj/AspectJExpressionPointcut.java index aeb258e0a3b..56f983ab9eb 100644 --- a/spring-aop/src/main/java/org/springframework/aop/aspectj/AspectJExpressionPointcut.java +++ b/spring-aop/src/main/java/org/springframework/aop/aspectj/AspectJExpressionPointcut.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2024 the original author or authors. + * Copyright 2002-2025 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -112,12 +112,12 @@ public class AspectJExpressionPointcut extends AbstractExpressionPointcut private BeanFactory beanFactory; @Nullable - private transient ClassLoader pointcutClassLoader; + private transient volatile ClassLoader pointcutClassLoader; @Nullable - private transient PointcutExpression pointcutExpression; + private transient volatile PointcutExpression pointcutExpression; - private transient boolean pointcutParsingFailed = false; + private transient volatile boolean pointcutParsingFailed; /** @@ -197,11 +197,14 @@ public class AspectJExpressionPointcut extends AbstractExpressionPointcut * Lazily build the underlying AspectJ pointcut expression. */ private PointcutExpression obtainPointcutExpression() { - if (this.pointcutExpression == null) { - this.pointcutClassLoader = determinePointcutClassLoader(); - this.pointcutExpression = buildPointcutExpression(this.pointcutClassLoader); + PointcutExpression pointcutExpression = this.pointcutExpression; + if (pointcutExpression == null) { + ClassLoader pointcutClassLoader = determinePointcutClassLoader(); + pointcutExpression = buildPointcutExpression(pointcutClassLoader); + this.pointcutClassLoader = pointcutClassLoader; + this.pointcutExpression = pointcutExpression; } - return this.pointcutExpression; + return pointcutExpression; } /** @@ -467,40 +470,24 @@ public class AspectJExpressionPointcut extends AbstractExpressionPointcut } private ShadowMatch getShadowMatch(Method targetMethod, Method originalMethod) { - ShadowMatch shadowMatch = ShadowMatchUtils.getShadowMatch(this, targetMethod); + ShadowMatchKey key = new ShadowMatchKey(this, targetMethod); + ShadowMatch shadowMatch = ShadowMatchUtils.getShadowMatch(key); if (shadowMatch == null) { - PointcutExpression fallbackExpression = null; - Method methodToMatch = targetMethod; - try { + PointcutExpression pointcutExpression = obtainPointcutExpression(); + synchronized (pointcutExpression) { + shadowMatch = ShadowMatchUtils.getShadowMatch(key); + if (shadowMatch != null) { + return shadowMatch; + } + PointcutExpression fallbackExpression = null; + Method methodToMatch = targetMethod; try { - shadowMatch = obtainPointcutExpression().matchesMethodExecution(methodToMatch); - } - catch (ReflectionWorldException ex) { - // Failed to introspect target method, probably because it has been loaded - // in a special ClassLoader. Let's try the declaring ClassLoader instead... try { - fallbackExpression = getFallbackPointcutExpression(methodToMatch.getDeclaringClass()); - if (fallbackExpression != null) { - shadowMatch = fallbackExpression.matchesMethodExecution(methodToMatch); - } - } - catch (ReflectionWorldException ex2) { - fallbackExpression = null; - } - } - if (targetMethod != originalMethod && (shadowMatch == null || - (Proxy.isProxyClass(targetMethod.getDeclaringClass()) && - (shadowMatch.neverMatches() || containsAnnotationPointcut())))) { - // Fall back to the plain original method in case of no resolvable match or a - // negative match on a proxy class (which doesn't carry any annotations on its - // redeclared methods), as well as for annotation pointcuts. - methodToMatch = originalMethod; - try { - shadowMatch = obtainPointcutExpression().matchesMethodExecution(methodToMatch); + shadowMatch = pointcutExpression.matchesMethodExecution(methodToMatch); } catch (ReflectionWorldException ex) { - // Could neither introspect the target class nor the proxy class -> - // let's try the original method's declaring class before we give up... + // Failed to introspect target method, probably because it has been loaded + // in a special ClassLoader. Let's try the declaring ClassLoader instead... try { fallbackExpression = getFallbackPointcutExpression(methodToMatch.getDeclaringClass()); if (fallbackExpression != null) { @@ -511,21 +498,45 @@ public class AspectJExpressionPointcut extends AbstractExpressionPointcut fallbackExpression = null; } } + if (targetMethod != originalMethod && (shadowMatch == null || + (Proxy.isProxyClass(targetMethod.getDeclaringClass()) && + (shadowMatch.neverMatches() || containsAnnotationPointcut())))) { + // Fall back to the plain original method in case of no resolvable match or a + // negative match on a proxy class (which doesn't carry any annotations on its + // redeclared methods), as well as for annotation pointcuts. + methodToMatch = originalMethod; + try { + shadowMatch = pointcutExpression.matchesMethodExecution(methodToMatch); + } + catch (ReflectionWorldException ex) { + // Could neither introspect the target class nor the proxy class -> + // let's try the original method's declaring class before we give up... + try { + fallbackExpression = getFallbackPointcutExpression(methodToMatch.getDeclaringClass()); + if (fallbackExpression != null) { + shadowMatch = fallbackExpression.matchesMethodExecution(methodToMatch); + } + } + catch (ReflectionWorldException ex2) { + fallbackExpression = null; + } + } + } } + catch (Throwable ex) { + // Possibly AspectJ 1.8.10 encountering an invalid signature + logger.debug("PointcutExpression matching rejected target method", ex); + fallbackExpression = null; + } + if (shadowMatch == null) { + shadowMatch = new ShadowMatchImpl(org.aspectj.util.FuzzyBoolean.NO, null, null, null); + } + else if (shadowMatch.maybeMatches() && fallbackExpression != null) { + shadowMatch = new DefensiveShadowMatch(shadowMatch, + fallbackExpression.matchesMethodExecution(methodToMatch)); + } + shadowMatch = ShadowMatchUtils.setShadowMatch(key, shadowMatch); } - catch (Throwable ex) { - // Possibly AspectJ 1.8.10 encountering an invalid signature - logger.debug("PointcutExpression matching rejected target method", ex); - fallbackExpression = null; - } - if (shadowMatch == null) { - shadowMatch = new ShadowMatchImpl(org.aspectj.util.FuzzyBoolean.NO, null, null, null); - } - else if (shadowMatch.maybeMatches() && fallbackExpression != null) { - shadowMatch = new DefensiveShadowMatch(shadowMatch, - fallbackExpression.matchesMethodExecution(methodToMatch)); - } - shadowMatch = ShadowMatchUtils.setShadowMatch(this, targetMethod, shadowMatch); } return shadowMatch; } @@ -720,4 +731,8 @@ public class AspectJExpressionPointcut extends AbstractExpressionPointcut } } + + private record ShadowMatchKey(AspectJExpressionPointcut expression, Method method) { + } + } diff --git a/spring-aop/src/main/java/org/springframework/aop/aspectj/ShadowMatchUtils.java b/spring-aop/src/main/java/org/springframework/aop/aspectj/ShadowMatchUtils.java index beb3ac63bb9..f17b6ab9c6e 100644 --- a/spring-aop/src/main/java/org/springframework/aop/aspectj/ShadowMatchUtils.java +++ b/spring-aop/src/main/java/org/springframework/aop/aspectj/ShadowMatchUtils.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2024 the original author or authors. + * Copyright 2002-2025 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -16,24 +16,48 @@ package org.springframework.aop.aspectj; -import java.lang.reflect.Method; import java.util.Map; import java.util.concurrent.ConcurrentHashMap; import org.aspectj.weaver.tools.ShadowMatch; -import org.springframework.aop.support.ExpressionPointcut; import org.springframework.lang.Nullable; /** * Internal {@link ShadowMatch} utilities. * * @author Stephane Nicoll + * @author Juergen Hoeller * @since 6.2 */ public abstract class ShadowMatchUtils { - private static final Map shadowMatchCache = new ConcurrentHashMap<>(256); + private static final Map shadowMatchCache = new ConcurrentHashMap<>(256); + + + /** + * Find a {@link ShadowMatch} for the specified key. + * @param key the key to use + * @return the {@code ShadowMatch} to use for the specified key, + * or {@code null} if none found + */ + @Nullable + static ShadowMatch getShadowMatch(Object key) { + return shadowMatchCache.get(key); + } + + /** + * Associate the {@link ShadowMatch} with the specified key. + * If an entry already exists, the given {@code shadowMatch} is ignored. + * @param key the key to use + * @param shadowMatch the shadow match to use for this key + * if none already exists + * @return the shadow match to use for the specified key + */ + static ShadowMatch setShadowMatch(Object key, ShadowMatch shadowMatch) { + ShadowMatch existing = shadowMatchCache.putIfAbsent(key, shadowMatch); + return (existing != null ? existing : shadowMatch); + } /** * Clear the cache of computed {@link ShadowMatch} instances. @@ -42,34 +66,4 @@ public abstract class ShadowMatchUtils { shadowMatchCache.clear(); } - /** - * Return the {@link ShadowMatch} for the specified {@link ExpressionPointcut} - * and {@link Method} or {@code null} if none is found. - * @param expression the expression - * @param method the method - * @return the {@code ShadowMatch} to use for the specified expression and method - */ - @Nullable - static ShadowMatch getShadowMatch(ExpressionPointcut expression, Method method) { - return shadowMatchCache.get(new Key(expression, method)); - } - - /** - * Associate the {@link ShadowMatch} to the specified {@link ExpressionPointcut} - * and method. If an entry already exists, the given {@code shadowMatch} is - * ignored. - * @param expression the expression - * @param method the method - * @param shadowMatch the shadow match to use for this expression and method - * if none already exists - * @return the shadow match to use for the specified expression and method - */ - static ShadowMatch setShadowMatch(ExpressionPointcut expression, Method method, ShadowMatch shadowMatch) { - ShadowMatch existing = shadowMatchCache.putIfAbsent(new Key(expression, method), shadowMatch); - return (existing != null ? existing : shadowMatch); - } - - - private record Key(ExpressionPointcut expression, Method method) {} - }