tomcat-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jim Manico <...@manico.net>
Subject On Tomcat Concurrency Problems
Date Fri, 16 May 2008 04:37:26 GMT
A senior developer from the company I work for had the following 
comments (in support of Tomcat) regarding the very interesting threading 
post from Lloyd Chambers last week.

****

This is an interesting post, but not for the reason that the author 
intended. I'm sure he's an awesome developer who totally understands 
this stuff.  And the findings might be serious problems.  But if they 
are real, *he blew the writeups (to the Tomcat dev list).*

Reread these findings -- they're all theoretical.  I guess they all sort 
of represent "violation of best practice". If you ever write that it 
just means that you can't find an actual problem to report.  Every one 
of these findings raises obvious questions that the finding should have 
answered.  *Always answer "who cares"*

· How is SingleSignOn actually instantiated?  (my guess is that there 
aren't lots of threads at startup).
· Are the "public" get/setRequireReauthentiation() methods actually 
called from any different threads?  (my guess is that this doesn't make 
a bit of difference).
· Are there any classes that extend SingleSignOn?  (if not then who 
cares about subclasses modifying non-finals).
· What actually calls findSessions() and does it modify the array?  (my 
guess is not)

The conclusion that SSO is a "disaster waiting to happen" *is the kind 
of FUD (fear uncertainty doubt) talk that undermines the whole point.*  
The reference to "Java Concurrency in Practice" is probably 
well-intentioned, but just comes off snotty.

We face these dangers in our writings (as an Application Security 
company) as well. Always try to put yourself in the customer's *(in this 
case, the Tomcat dev team) *shoes and really explain more than just the 
narrow-scope technical problem.  If you're using the words this guy is 
using -- something is wrong with your findings:

·        "Perhaps"
·        "Could"
·        "Presumably"
·        "What if"
·        "Bug-prone if"

-- 
Jim Manico, Senior Application Security Engineer
jim.manico@aspectsecurity.com | jim@manico.net
(301) 604-4882 (work)
(808) 652-3805 (cell)

Aspect Security^(TM)
Securing your applications at the source
http://www.aspectsecurity.com



(post from Lloyd Chambers)

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 </exchweb/bin/redir.asp?URL=http://diglloyd.com/>

[Mac OS X 10.5.2 Intel, Tomcat 6.0.16]


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message