tomcat-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Bill Barker" <wbar...@wilshire.com>
Subject Re: thread safety of org.apache.catalina.authenticator.SingleSignOn
Date Thu, 15 May 2008 02:10:39 GMT


"DIGLLOYD INC" <diglloyd1@diglloyd.com> wrote in message 
news:E526E60F-935F-4784-8C8A-CF18455DD902@diglloyd.com...
> 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.
>

Not really much of a problem, since they are set during startup (while 
parsing server.xml), and before Tomcat is multi-threaded.  Since there will 
be many thousands of lines of code executed before the first request comes 
in, this is mostly a theoretical problem.

> 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.
>

Again, not a real problem (unless you are playing fancy JMX games ;).  The 
setters are called at startup while parsing the server.xml file, and after 
that the values are assumed to never change.

> 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).
>

The general policy is that if you replace a Tomcat component with your own 
subclass, then this sort of thing is your responsibility.  However, patches 
are always welcome ;).

> 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.
>

SSO doesn't get a lot of love ;), so other than cosmetic changes, it is 
still much the same as it was for Tomcat 4.0 (which needed to be able to run 
on Java2).  Looking at the code, I don't see any serious race conditions 
here, but again patches are always welcome ;)

> 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.
>

Ok, yes, there is a race condition here (but not easy to hit in real-life 
apps).  In particular, if the last Session expires at the same time that the 
user attempts to use a new Context, then you could hit this.  Again, patches 
are always welcome ;).

> 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.
>

I hope that you agree that it isn't actually a "disaster", but yes it has 
it's issues (mostly forcing users to re-login when they thought they were 
logged in already).

> The class SingleSignOnEntry is also not thread-safe; for example 
> updateCredentials() is thread-unsafe.
>

Point taken, but in the real world, it will almost always be setting the 
values to the same values that it had before.  But I can see that if you 
want to mix BASIC and FORM contexts under SSO this could cause problems.

> 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