Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SpEL variable evaluation fails with NPE against ConcurrentHashMap [SPR-17565] #22097

Closed
spring-issuemaster opened this issue Dec 4, 2018 · 6 comments

Comments

@spring-issuemaster
Copy link
Collaborator

@spring-issuemaster spring-issuemaster commented Dec 4, 2018

Michael Igler opened SPR-17565 and commented

After upgrading from spring-boot 2.1.0.RELEASE to 2.1.1.RELEASE I get an NPE when the following code is evaluated:

@Override
 @PreAuthorize("hasPermission(#entity, 'write')")
 <S extends TimeFrame> S save(@P("entity") S entity, int depth);
Caused by: java.lang.NullPointerException at java.util.concurrent.ConcurrentHashMap.putVal(ConcurrentHashMap.java:1011) at java.util.concurrent.ConcurrentHashMap.put(ConcurrentHashMap.java:1006) at org.springframework.expression.spel.support.StandardEvaluationContext.setVariable(StandardEvaluationContext.java:234) at org.springframework.security.access.expression.method.MethodSecurityEvaluationContext.addArgumentsAsVariables(MethodSecurityEvaluationContext.java:115) at org.springframework.security.access.expression.method.MethodSecurityEvaluationContext.lookupVariable(MethodSecurityEvaluationContext.java:70) at org.springframework.expression.spel.ExpressionState.lookupVariable(ExpressionState.java:146) at org.springframework.expression.spel.ast.VariableReference.getValueInternal(VariableReference.java:76) at org.springframework.expression.spel.ast.MethodReference.getArguments(MethodReference.java:163) at org.springframework.expression.spel.ast.MethodReference.getValueInternal(MethodReference.java:93) at org.springframework.expression.spel.ast.SpelNodeImpl.getTypedValue(SpelNodeImpl.java:116) at org.springframework.expression.spel.standard.SpelExpression.getValue(SpelExpression.java:300) at org.springframework.security.access.expression.ExpressionUtils.evaluateAsBoolean(ExpressionUtils.java:26) at org.springframework.security.access.expression.method.ExpressionBasedPreInvocationAdvice.before(ExpressionBasedPreInvocationAdvice.java:59) at org.springframework.security.access.prepost.PreInvocationAuthorizationAdviceVoter.vote(PreInvocationAuthorizationAdviceVoter.java:72) at org.springframework.security.access.prepost.PreInvocationAuthorizationAdviceVoter.vote(PreInvocationAuthorizationAdviceVoter.java:40) at org.springframework.security.access.vote.AffirmativeBased.decide(AffirmativeBased.java:63) at org.springframework.security.access.intercept.AbstractSecurityInterceptor.beforeInvocation(AbstractSecurityInterceptor.java:233) at org.springframework.security.access.intercept.aopalliance.MethodSecurityInterceptor.invoke(MethodSecurityInterceptor.java:65) at org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:186) at org.springframework.aop.interceptor.ExposeInvocationInterceptor.invoke(ExposeInvocationInterceptor.java:93) at org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:186) at org.springframework.aop.framework.JdkDynamicAopProxy.invoke(JdkDynamicAopProxy.java:212) at com.sun.proxy.$Proxy194.save(Unknown Source) at de.company.ebm.backlog.web.TimeFrameController.createEntity(TimeFrameController.java:75) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) at java.lang.reflect.Method.invoke(Method.java:497) at org.springframework.aop.support.AopUtils.invokeJoinpointUsingReflection(AopUtils.java:343) at org.springframework.aop.framework.ReflectiveMethodInvocation.invokeJoinpoint(ReflectiveMethodInvocation.java:198) at org.springframework.aop.framework.CglibAopProxy$CglibMethodInvocation.invokeJoinpoint(CglibAopProxy.java:752) at org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:163) at org.springframework.security.access.intercept.aopalliance.MethodSecurityInterceptor.invoke(MethodSecurityInterceptor.java:69) at org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:186) at org.springframework.aop.framework.CglibAopProxy$DynamicAdvisedInterceptor.intercept(CglibAopProxy.java:688) at de.emons.ebm.backlog.web.TimeFrameController$$EnhancerBySpringCGLIB$$a8a5f58f.createEntity(<generated>) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) at java.lang.reflect.Method.invoke(Method.java:497) at org.springframework.web.method.support.InvocableHandlerMethod.doInvoke(InvocableHandlerMethod.java:189) at org.springframework.web.method.support.InvocableHandlerMethod.invokeForRequest(InvocableHandlerMethod.java:138) at org.springframework.web.servlet.mvc.method.annotation.ServletInvocableHandlerMethod.invokeAndHandle(ServletInvocableHandlerMethod.java:102) at org.springframework.web.servlet.mvc.method.annotation.RequestMappingHandlerAdapter.invokeHandlerMethod(RequestMappingHandlerAdapter.java:895) at org.springframework.web.servlet.mvc.method.annotation.RequestMappingHandlerAdapter.handleInternal(RequestMappingHandlerAdapter.java:800) at org.springframework.web.servlet.mvc.method.AbstractHandlerMethodAdapter.handle(AbstractHandlerMethodAdapter.java:87) at org.springframework.web.servlet.DispatcherServlet.doDispatch(DispatcherServlet.java:1038) at org.springframework.web.servlet.DispatcherServlet.doService(DispatcherServlet.java:942) at org.springframework.web.servlet.FrameworkServlet.processRequest(FrameworkServlet.java:1005) ... 73 more 

 

If downgrading to spring 5.1.2.RELEASE it works again.


Affects: 5.1.3

Issue Links:

  • #21980 StandardEvaluationContext does not support concurrent variable access

Backported to: 5.0.12

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Dec 4, 2018

Juergen Hoeller commented

Rob Winch, have you seen this before? Is MethodSecurityEvaluationContext possibly trying to set a variable with a null name there (since we're explicitly handling null values but without checking the key since we always expect it to be non-null)?

In any case, this seems to be a regression caused by #21980 where we switched to a ConcurrentHashMap for variable storage in StandardEvaluationContext.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Dec 4, 2018

Rob Winch commented

Juergen Hoeller I haven't seen this before, but can confirm it is happening. The reason is that Spring Security's AnnotationParameterNameDiscoverer is resolving the second parameter name as null which is then set on the StandardEvaluationContext.

I created spring-projects/spring-security#6223 to address this from the Spring Security perspective. There may be other places, but it appears that MethodBasedEvaluationContext will need to be more defensive as well.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Dec 4, 2018

Juergen Hoeller commented

Rob Winch, as far as I can tell, MethodBasedEvaluationContext is safe in that it only sets a parameter name if actually resolved. A ParameterNameDiscoverer either returns null or a fully resolved name array, so checking for not-null is sufficient there.

So it looks like a fix on Spring Security's side is all that's needed here. In addition, we could defensively ignore a setVariable call with a null name since it doesn't indicate storage of a value for a null name but rather an unresolved parameter that cannot be looked up in any case.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Dec 5, 2018

Juergen Hoeller commented

I've addressed this on StandardEvaluationContext's end through leniently ignoring null names in setVariable, also backported to 5.0.12. Nevertheless, this should also be fixed on the caller's side, not passing in a null name to begin with.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Dec 7, 2018

Rob Winch commented

Juergen Hoeller Thanks.

Given that StandardEvaluationContext's ignores null values this is not necessary. However, I wonder if it makes sense to proactively update MethodBasedEvaluationContext to ignore null entries in paramNames.

Based upon what you said above it appears that ParameterNameDiscoverer is intended to resolve all parameter names or nothing. However, my initial understanding from the Javadoc was that if at least one name was discovered it could return a non-null array with null values for any parameter name that couldn't be determined.

This also makes sense for an annotation based ParameterNameDiscoverer when a user only annotates a single argument that they care about (as illustrated in the issue in this issue). I hope that this behavior can be supported because it has been working in Spring Security for many years at this point.

In either case, it might be worth putting some polish on the ParameterNameDiscoverer javadoc. 

Thoughts?

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Dec 12, 2018

Juergen Hoeller commented

Rob Winch, indeed, I've revised MethodBasedEvaluationContext to defensively check for a non-null entry there, and added some notes on it to ParameterNameDiscoverer (even if we generally recommend stub names wherever feasible, like the JDK's parameter reflection API does).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.