Bill,
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.
Lloyd
On May 14, 2008, at 7:10 PM, Bill Barker wrote:
"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]
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]
