"DIGLLOYD INC" <[EMAIL PROTECTED]> wrote in message news:[EMAIL PROTECTED] > 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: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
