MethodReference isn't thread-safe [SPR-12269]

[spring-projects-issues]

Bryan Turner opened SPR-12269 and commented

In MethodReference, cachedExecutor is declared volatile. That implies that this class is expected to be used by multiple threads, but almost none of the methods that access that field do so safely.

We're ending up in this method due to Spring Security using SpEL expressions for permission checks. Under light load, the system works fine. Under heavier load, we're seeing NullPointerException stacks that are topped like this:

2014-09-27 21:01:04,386 ERROR [threadpool:thread-28592]  c.a.s.i.c.StateTransferringExecutor Error while processing asynchronous task
java.lang.NullPointerException: null
        at org.springframework.expression.spel.ast.MethodReference.getValueInternal(MethodReference.java:86) ~[MethodReference.class:4.1.0.RELEASE]
        at org.springframework.expression.spel.ast.SpelNodeImpl.getTypedValue(SpelNodeImpl.java:126) ~[SpelNodeImpl.class:4.1.0.RELEASE]
        at org.springframework.expression.spel.standard.SpelExpression.getValue(SpelExpression.java:296) ~[SpelExpression.class:4.1.0.RELEASE]

A look at MethodReference on line 86 shows:

TypedValue result = getValueInternal(evaluationContext, value, targetType, arguments);
if (cachedExecutor.get() instanceof ReflectiveMethodExecutor) {
     ReflectiveMethodExecutor executor = (ReflectiveMethodExecutor) cachedExecutor.get();

This has 2 problems:

1. There's no null check, but there are multiple other codepaths that can set cachedExecutor to null
2. Even if there was, a la CachedMethodExecutor.isCompilable, they won't actually protect anything; they're just a race condition.

Since cachedExecutor is volatile, all of the code that uses it, if they need to touch it multiple times, needs to first assign it to a local variable to freeze its state and then access the local variable instead. Otherwise it will always be susceptible to race conditions. This appears to be missing in:

• MethodReference.getValueInternal(ExpressionState)
• MethodValueRef.getValue()
• CachedMethodExecutor.isCompilable()
• CachedMethodExecutor.generateCode() (This method only touches cachedExecutor once, but that's only because it's not null checking)

MethodReference.getCachedExecutor has exactly the type of code it seems like every method should have:

CachedMethodExecutor executorToCheck = this.cachedExecutor;
if (executorToCheck != null && executorToCheck.isSuitable(value, target, argumentTypes)) {

---

Affects: 4.1 GA

Referenced from: commits 0cc877a, c508a70

[spring-projects-issues]

Juergen Hoeller commented

Good catch - revised for 4.1.1 now. Please give the upcoming 4.1.1 snapshot a try if you have the chance...

Juergen

[spring-projects-issues]

Bryan Turner commented

Juergen,

Thanks for the remarkably quick turnaround! I really appreciate it.

It looks like the new code in updateExitTypeDescriptor still isn't quite right, though:

private void updateExitTypeDescriptor() {
    CachedMethodExecutor executorToCheck = this.cachedExecutor;
    if (executorToCheck.get() instanceof ReflectiveMethodExecutor) {
        Method method = ((ReflectiveMethodExecutor) executorToCheck.get()).getMethod();
        this.exitTypeDescriptor = CodeFlow.toDescriptor(method.getReturnType());
    }
}

It seems like this needs a executorToCheck != null before the executorToCheck.get().

Thanks again!
Bryan Turner

[spring-projects-issues]

Juergen Hoeller commented

Good catch - fixed now!

Juergen