[SHIRO-762] Mark SecurityUtils.securityManager as volatile

[boris-petrov]

As it can be modified and read by different threads. This has been biting me for a very long time now.

[fpapon]

@boris-petrov Thanks for the PR.
Can you explain a little more because I'm not sure to understand in which case the reference of the securityManager in the SecurityUtils will be updated, volatile only concern the reference.

[boris-petrov]

@fpapon - thanks for the time.

Exactly the reference is the problem. Marking this field as volatile means that whenever some thread sets the value (using setSecurityManager) all other threads from that time on will "see" the correct security manager. Otherwise, they can "see" a previous one.

And that's exactly what happens in our tests. We set initially a security manager, then we destroy it and set another one for the rest of the time. A few minutes later, a thread does new Subject.Builder().sessionCreationEnabled(false).sessionId(shiroSessionId).buildSubject() which blows up with a NPE:

java.lang.NullPointerException: null
        at org.apache.shiro.mgt.SessionsSecurityManager.getSession(SessionsSecurityManager.java:156)
        at org.apache.shiro.mgt.DefaultSecurityManager.resolveContextSession(DefaultSecurityManager.java:461)
        at org.apache.shiro.mgt.DefaultSecurityManager.resolveSession(DefaultSecurityManager.java:447)
        at org.apache.shiro.mgt.DefaultSecurityManager.createSubject(DefaultSecurityManager.java:343)
        at org.apache.shiro.subject.Subject$Builder.buildSubject(Subject.java:845)
        at com.company.SomeClass.buildSubject(SomeClass.java:321)
        ...

Because it still "sees" the previously set (and destroyed) security manager even though it's been a long time since a new one has been set.

Marking this field as volatile is practically free and is making the code work correctly.

[fpapon]

@boris-petrov ok I see it now, it make sense ;)

[fpapon]

LGTM
Thanks!

[fpapon]

@bdemers can you have a second eyes on it?

[bmarwell]

There could still be a race-condition on updating the variables. The volatile keyword will fix the read. If this is not enough for every use case (e.g. hold Threads while it is updated), we could go for AtomicReference - but that is probably not needed here.

[boris-petrov]

@bmhm - yes, there could be a race-condition on updating the variable, but this is not Shiro's concern, it is the application's. I.e. it is a bug in the application. But volatile missing is a bug in Shiro (and the application can do nothing to work around it).

[bmarwell]

@bmhm - yes, there could be a race-condition on updating the variable, but this is not Shiro's concern, it is the application's. I.e. it is a bug in the application. But volatile missing is a bug in Shiro (and the application can do nothing to work around it).

I can agree on that. LGTM then.

[bdemers]

👍
@boris-petrov If possible, I would like to know more about your use case and how/why you are setting/using the static instance. (i.e. is there something in Shiro that we could improve, or a framework we could support to help avoid this situation).

Thanks for your contribution!

[fpapon]

@boris-petrov As we have an open release vote (1.5.3), I will merge your PR just after, don't worry if it take 2/3 days ;)

[boris-petrov]

@fpapon - great, no problem, thanks!

@bdemers - well, I'm doing programmatic configuration (without any ini files) so I have to set the "global" SecurityManager, right? After that I'm not using it in any way, I'm just sometimes creating Subjects manually and that internally uses the static SecurityManager. I don't think there is anything here that can/needs to be improved.

There is however a pain point that I see also others have hit before - using an InheritableThreadLocal instead of a "normal" ThreadLocal. In addition to what the people in that thread explain as problems, another thing to note is that once such a threadpool thread stores the SecurityManager in its thread local storage, it will be such forever - even if the global static SecurityManager is changed, the threadpool threads are bound forever to the old one which could lead to nasty situations. I believe this has to be revisited and perhaps an option to be added for Shiro to use "normal" ThreadLocal's instead of inheritable-ones.

[bdemers]

Thanks for following up @boris-petrov!

Re: needing to set the global SecurityManager, often it is used when creating subjects manually outside of another context like a servlet filter (or equivalent). No problem there, I was just wondering if there was a framework or something you were using.

As for the thread local issue. I've created this JIRA: https://issues.apache.org/jira/browse/SHIRO-763

Thanks again!