[concurrency-interest] concurrency errors in java.lang.Throwable

Joe Bowbeer joe.bowbeer at gmail.com
Wed Aug 2 12:59:42 EDT 2006


On 8/2/06, Rémi Forax <forax at univ-mlv.fr> wrote:
> I think i've found a concurrency bug in the Sun implementation
> of java.lang.Throwable.
>
> Since 1.4, Throwable accepts a cause at construction time
> using a ad hoc constructors or after if initilialisation using
> the method initCause().
>
> First, the field can't be final and is not volatile, the method
> getCause() is not
> synchronized and  doesn't use a lock, so a thread can create an
> exception with a cause and another thread can see the same
> exception without a cause.
>
> Second, initCause is synchronized but getCause() is not, so a thread
> can call initCause() and another thread never see the change.
>
> Am i right ?
>
> Should the field that store the cause be volatile and use
> a CAS in initCause() to initialize the cause (the cause can't be
> initialized twice) ?
>
> Rémi Forax
>

I noticed the same thing a short while ago and discussed it with the
experts.  A summary of that discussion follows.

David Holmes writes:

  "Being safely publishable even when published without
synchronization goes a step beyond basic thread-safety. That is not
the general expectation from a thread-safe class. Yes the publishing
aspect is subtle and most people don't get it until they read
something like JCiP, but it is a classic concurrency problem: the
variable to which the object is published is a shared, mutable
variable - hence access to it requires synchronization.

I don't think we should be expending effort trying to make thread-safe
classes publishable without synchronization.(*) Rather we need to
educate because no matter what we do with JDK classes the programmer
will still screw up their own if they don't understand the issues."

(*) An exception is made for classes like String that are involved in
security.  These must be publishable without synchronization.  Or,
rather, unsafe publication of these classes must not represent a
security vulnerability.

David Holmes again:

  "As far as I can see with unsafe publication (and synchronized
getCause) the worst that can happen is you will see no-cause when
there should be one. Even if you see the default initialized cause
value that will be null and getCause will return null. We don't try to
make this work for other mutable classes so I don't see why Throwable
needs to be special. In the few scenarios where I can imagine passing
Throwables across threads, the Throwable isn't mutated after being
thrown, and the exchange between threads will involve
synchronization."


We decided that it would be an improvement (for clarity as much as
anything) if detailMessage were declared final.

One could also argue for synchronizing getCause, since all the other
setter/getter methods are, though, as far as we know, the
synchronization of initCause is only supposed to be preventing
multiple threads from calling that method simultaneously.  It's
supposed to be "call-once".

Are we overlooking a security vulnerability?


As an exercise, we sketched a Throwable implementation that would be
"safe" for un-safe publication:

public class Throwable {

  private final AtomicReference<Throwable> causeRef =
     new AtomicReference<Throwable>(this);

  public Throwable getCause() {
     Throwable t = causeRef.get();
     return (t == this ? null : t);
 }

  public Throwable initCause(Throwable cause) {
     if (cause == this)
         throw new IllegalArgumentException("Self-causation not permitted");
    if (!causeRef.compareAndSet(this, cause))
      throw new IllegalArgumentException("Can't overwrite cause");
  }
}

We thought it was a good, concise example of a thread-safe one-shot --
but that it was overkill in this situation...

--Joe



More information about the Concurrency-interest mailing list