First, I'm an experienced developer, and well-versed in Java
threading. My main work is on the Glassfish project at Sun.
I've been looking into the code of
org.apache.catalina.authenticator.SingleSignOn to see how it works,
and I've noticed a number of thread-safety bugs. My understanding is
that Valve SingleSignOn could run on any thread (eg any incoming
request).
1. Most of the instance variables are not 'final'. So when
SingleSignOn is instantiated, there is no guarantee that the instance
will be seen correctly by any other thread; it's state is undefined
from the point of view of another thread.
Its immutable variable values (eg the Maps) should be 'final' so as to
benefit from the JVM guarantee of thread safety for 'final' instance
variables. And/or there needs to be an external synchronizing side-
effect that would make the object instance "visible" to other
threads. Perhaps there exists such an external side effect that masks
this bug.
2. The get/set methods are thread-unsafe. For example,
getRequireReauthentication() and setRequireReauthentication() are not
'synchronized', and the variable 'requireReauthentication' is not
'volatile'. As a result, different threads could see different values
and/or never see an updated value. Since these are public methods,
and presumably can be called from any number of different threads,
this is a problem.
3. Variables 'lifecycle' and 'reverse' and 'cache' are not 'final'.
See #1 above, but this also exposes/encourages another thread-safety
issue: what if the variables themselves are changed by a subclass
(since SingleSignOn is not a 'final' class).
4. The Map classes use external synchronization, which is bug-prone
if any modifications are made (and there might be some race conditions
here as well). The java.util.concurrent.ConcurrentHashMap class is
much more scalable and doesn't have these risks.
5. Method deregister() (both variants) contain a race condition
whereby the call sso.findSessions(). Method findSessions() is
'synchronized', but *returns its internal array 'sessions'*!!! Upon
return there is no further thread-safety; 'sessions' is now exposed
and anything looking at it is now subject to a race condition and/or
"visibility" problem.
These are almost certainly not all the issues. From what I can see,
SingleSignOn is a disaster waiting to happen on the wrong hardware,
where caches are not write-through as they are on most of today's
processors. And even on common hardware (eg Intel), there are still
some "windows of opportunity" for failures to occur, especially item
#5 above.
The class SingleSignOnEntry is also not thread-safe; for example
updateCredentials() is thread-unsafe.
I recommend "Java Concurrency in Practice" for understanding these
issues.
Lloyd Chambers
http://diglloyd.com
[Mac OS X 10.5.2 Intel, Tomcat 6.0.16]
---------------------------------------------------------------------
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
