tomcat-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From DIGLLOYD INC <digllo...@diglloyd.com>
Subject thread safety of org.apache.catalina.authenticator.SingleSignOn
Date Wed, 14 May 2008 19:05:05 GMT
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: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org


Mime
View raw message