tomcat-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
Subject Re: thread safety of org.apache.catalina.authenticator.SingleSignOn
Date Thu, 15 May 2008 02:38:55 GMT

Thank your for the thoughtful response.

I agree that there is unlikely to be a problem in practice on commonly- 
used systems (which typically have write-through caches).  And most  
JVM implementations (if not all) can't track small changes; everything  
gets flushed pretty quickly because there is no practical way to be  
more granular.

In the future though, systems with lots of caching and/or dozens or  
hundreds of CPU cores might encounter problems; future performance  
gains require either far higher memory bandwidth or "lazy" flushing of  
memory to disk to minimize memory contention.

I think at the least using 'final' and 'volatile' would address most  
of the issues.   But not the race condition involving 'sessions'.  But  
there I just don't understand the scenario enough to know if it ever  
will actually occur.


On May 14, 2008, at 7:10 PM, Bill Barker wrote:

> "DIGLLOYD INC" <> wrote in message
>> 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
>> [Mac OS X 10.5.2 Intel, Tomcat 6.0.16]
> ---------------------------------------------------------------------
> To unsubscribe, e-mail:
> For additional commands, e-mail:

Lloyd Chambers

[Mac OS X 10.5.2 Intel, Tomcat 6.0.16]

To unsubscribe, e-mail:
For additional commands, e-mail:

View raw message