Skip to end of metadata
Go to start of metadata

Overriding thread-safe methods with methods that are unsafe for concurrent use can result in improper synchronization when a client that depends on the thread-safety promised by the parent inadvertently operates on an instance of the subclass. For example, an overridden synchronized method's contract can be violated when a subclass provides an implementation that is unsafe for concurrent use. Such overriding can easily result in errors that are difficult to diagnose. Consequently, programs must not override thread-safe methods with methods that are unsafe for concurrent use.

The locking strategy of classes designed for inheritance should always be documented. This information can subsequently be used to determine an appropriate locking strategy for subclasses (see LCK00-J. Use private final lock objects to synchronize classes that may interact with untrusted code and LCK11-J. Avoid client-side locking when using classes that do not commit to their locking strategy).

Noncompliant Code Example (Synchronized Method)

This noncompliant code example overrides the synchronized doSomething() method in the Base class with an unsynchronized method in the Derived class:

class Base {
  public synchronized void doSomething() {
    // ...
  }
}

class Derived extends Base {
  @Override public void doSomething() {
    // ...
  }
}

The doSomething() method of the Base class can be safely used by multiple threads, but instances of the Derived subclass cannot.

This programming error can be difficult to diagnose because threads that accept instances of Base can also accept instances of its subclasses. Consequently, clients could be unaware that they are operating on a thread-unsafe instance of a subclass of a thread-safe class.

Compliant Solution (Synchronized Method)

This compliant solution synchronizes the doSomething() method of the subclass:

class Base {
  public synchronized void doSomething() {
    // ...
  }
}

class Derived extends Base {
  @Override public synchronized void doSomething() {
    // ...
  }
}

This solution also complies with LCK00-J. Use private final lock objects to synchronize classes that may interact with untrusted code because the accessibility of the class is package-private. Package-private accessibility is permitted when untrusted code cannot infiltrate the package.

Compliant Solution (Private Final Lock Object)

This compliant solution ensures that the Derived class is thread-safe by overriding the synchronized doSomething() method of the Base class with a method that synchronizes on a private final lock object.

class Base {

  public synchronized void doSomething() {
    // ...
  }
}

class Derived extends Base {
  private final Object lock = new Object();

  @Override public void doSomething() {
    synchronized (lock) {
      // ...
    }
  }
}

This is an acceptable solution, provided the locking policy of the Derived class is consistent with that of the Base class.

Noncompliant Code Example (Private Lock)

This noncompliant code example defines a doSomething() method in the Base class that uses a private final lock in accordance with LCK00-J. Use private final lock objects to synchronize classes that may interact with untrusted code.

class Base {
  private final Object lock = new Object();

  public void doSomething() {
    synchronized (lock) {
      // ...
    }
  }
}

class Derived extends Base {
  Logger logger = // Initialize

  @Override public void doSomething() {
    try {
      super.doSomething();
    } finally {
      logger.log(Level.FINE, "Did something");
    }
  }
}

It is possible for multiple threads to cause the entries to be logged in an order that differs from the order in which the tasks are performed. Consequently, the doSomething() method of the Derived class cannot be used safely by multiple threads because it is not thread-safe.

Compliant Solution (Private Lock)

This compliant solution synchronizes the doSomething() method of the subclass using its own private final lock object:

class Base {
  private final Object lock = new Object();

  public void doSomething() {
    synchronized (lock) {
      // ...
    }
  }
}

class Derived extends Base {
  Logger logger = // Initialize

  private final Object lock = new Object();

  @Override public void doSomething() {
    synchronized (lock) {
      try {
        super.doSomething();
      } finally {
        logger.log(Level.FINE, "Did something");
      }
    }
  }
}

Note that the Base and Derived objects maintain distinct locks that are inaccessible from each other's classes. Consequently, Derived can provide thread-safety guarantees independent of Base.

Risk Assessment

Overriding thread-safe methods with methods that are unsafe for concurrent access can result in unexpected behavior.

Rule

Severity

Likelihood

Remediation Cost

Priority

Level

TSM00-J

Low

Probable

Medium

P4

L3

Bibliography

 


21 Comments

  1. Rule LCK00-J. Use private final lock objects to synchronize classes that may interact with untrusted code,
    mandates that any class that uses a synchronized method must clearly
    document proper usage of its intrinsic lock. This requirement
    would also apply to any subclass that overrides a synchronized method,
    making it clear why a non-sync method might validly override a sync
    method.

    There are no security issues with a non-sync method overriding a sync
    method, just usability issues (the dev might get confused). The
    documentation mandated by CON04-J serves to minimize such
    issues. Consequently, I think this rule should be scrapped.

    1. Proper documentation helps but doesn't ensure that source code is secure. It might be better to build a detector for this condition instead of relying on documentation. We all know the problem with documentation. That said, tools already detect this violation such as Fortify (under code quality: Code Correctness: Non-Synchronized Method).

      1. Wouldn't this constitute a valid exception?

        class SynchronizedSubclass extends SynchronizedBase {
          public void doSomething() {
              // do some preprocessing
              super.doSomething(); // synchronized
              // do some postprocessing
            }	
          }
        }
        

        For instance, what if super.doSomething() was simply wrapped in a
        try/catch block in SynchronizedSubclass? Or what if the wrapping code
        merely logged that doSomething() was called?

        I can understand that a subclass that violates a sync contract can
        pose a problem; I just am not convinced that its as simple as flagging
        a missing 'synchronized' keyword.

        Perhaps we can retain this rule and add an exception that a subclass
        may override a non-sync method as long as it provides sufficient
        documentation as to why this is OK. (To catch violations, we need some
        standard for recognizing sufficient documentation.)

        One other item: the block-synchronization CS can go away, other rules
        (CON04?) show that block sync is as useful as method sync, and more
        versitile, too. FTM EX1 mentions the same thing as the block-sync
        CS.

        1. If you're doing some post processing then it is not an exception (atomicity reasons), otherwise yes. The logging example is unsafe if the logger is not thread-safe. If it is, and atomicity of the block is not a concern, then it can be treated as an exception.

          Currently this is a fairly strong guideline and we should be able to catch violations only on the basis of method synchronization. However, it might also be possible to "find" synchronized blocks in overridden methods of superclasses. If they exist then it is likely that the overriding method must also use some form of synchronization for non-trivial tasks.

          Perhaps we can retain this rule and add an exception that a subclass may override a non-sync method as long as it provides sufficient documentation as to why this is OK.

          I suspect you meant overriding a synchronized method. I think the exception would say that unless the subclass performs some thread-unsafe actions or those that require atomicity, it may violate this guideline.

          The block synchronization CS for this guideline seems essential. We could get rid of Compliant Solution (synchronized method in subclass) if you prefer.

      2. The Fortify guideline at http://www.fortify.com/vulncat/en/vulncat/java/code_correctness_non_synchronized_method_overrides.html

        references the Sun Bug ID: 4294756

        http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=4294756

        The resolution below is that this request for enhancement was closed as "will not fix". The description of the bug and response below is informative.

        Bug ID: 4294756
        Votes 0
        Synopsis Javac should warn if synchronized method is overridden with a non synchronized
        Category java:compiler
        Reported Against 1.2.2
        Release Fixed
        State 11-Closed, Will Not Fix, request for enhancement
        Priority: 4-Low
        Related Bugs
        Submit Date 25-NOV-1999
        Description

        java version "1.2"
        Classic VM (build JDK-1.2-V, green threads, sunwjit)

        According to the Java Language Specification (or more precisely it does not say
        anything about this (or I have not found it)) it is OK to override a
        synchronized method with a non synchronized one. If this is customer or bad might be
        discussed, but to exemplify why I think this is bad I'll give you an example.

        First we have 3 classes, Thread1, Thread2 and Bar. Both Thread1 and Thread2 have
        references to the same Bar instance and they execute the same method repeatedly
        in Bar, synchronizedMethod, I.e. Bar looks like this

        public class Bar {
          public synchronized void synchronizedMethod() {
             for (int i=0; i<10; i++) System.out.print(i);
             System.out.println();
          }
        }
        

        This works OK, since synchronizedMethod() is synchronized, and both Thread1 and
        Thread2 assume that whenever they use an instance of a Bar method the
        synchronizedMethod() is synchronized.

        Then we add a 4th class, a subclass to Bar called Foo

        public class Foo extends Bar {
           public void synchronizedMethod() {
             for (int i=0; i<10; i++) System.out.print(i);
             System.out.println();
          }
        }
        

        Now we will have problems if a Foo instance is casted to a Bar instance and this
        instance is given to both Thread1 and Thread2. Since Thread1 and Thread2 can not
        tell the difference between a Bar and a Foo (since they wrere written before Foo
        was ever thought of), they will gladly assume that the Bar instance they are
        given is properly synchronized...

        Declaring a method as synchronized is like signing a contract, promising that
        the method will always be sysnchronized. And if this contract is ever broken,
        unexpected/gruelsome consequencies might occur.

        One more thing. There is a subtle difference between

        public void synchronized foo() { } // 1
        

        and

        public void foo() { synchronized(this) { } } // 2
        

        The difference is that in the first case the compiler will mark the synchronized
        method with a flag and in the other case it will insert monitorenter/monitorexit
        bytecodes. I am only worried about the first case.

        This subtle difference makes it very easy for the compiler to issue a warning
        whenever the first case is found, and I think that would be very customer to have
        that waring. I don't think you can change the spec without messing up a lot of
        code (for instance by enforcing all synchronized methods to stay synchronized
        when they are overridden), right?
        (Review ID: 98287)
        ======================================================================

        Work Around

        This can only be adressed with coding standards, but a coding standard can be
        broken very easily, either consciuosly or not. And if the coding standard is
        broken, it might trigger very hard-to-find bugs which in turn becomes very
        costly...
        ======================================================================

        Evaluation

        Whether a method is synchronized or not is not considered a part of its
        signature in Java. That is, it is not part of that portion of the contract
        of the method that language requires the compiler to check automatically.

        The use of the synchronized modifier, as well as other synchronization via
        explicit 'synchronized' statements, is a part of the implementation of an
        abstraction represented by a class, and an alternate implementation captured
        in a subclass may use a different synchronization strategy in order to
        implement equivalent semantics. As an example, consider the case in which a
        small critical section (protected by a 'synchronized' statement) within a larger
        unsynchronized method replaces a smaller method that was protected in its
        entirety by a synchronized method modifier.

        From the standpoint of the user of a class, the relevant contract is whether
        the methods of the class can be safely invoked by multiple concurrent threads,
        or what constraints on concurrent access must be observed. This is in general
        more subtle than what can be captured by the presence or absence of a
        synchronized method modifier, and should be documented by other means.

        A lint-like tool could be written to warn of possible problems in cases
        such as the submitter describes, but such an unreliable heuristic does not
        belong in the compiler. We would generally not consider putting warning
        messages into the compiler except for usages that have been formally
        deprecated in the language specification.

        xxxxx@xxxxx 1999-12-20

        1. Yes, in fact we reference that bug too. Do you see something that can be added to the current guideline? The bug submitter didn't go into details about what/how subclasses should synchronize.

          We link to CON04 which provides specific documentation requirements (reproduced below) and helps in deciding the subclasses' locking strategy.

          If a class uses a private final lock to synchronize shared data, subclasses must also use a private final lock. However, if a class uses intrinsic synchronization over the class object without documenting its locking policy, subclasses may not use intrinsic synchronization over their own class object, unless they explicitly document their locking policy. If the superclass documents its policy by stating that client-side locking is supported, the subclasses have the option of choosing between intrinsic locking over the class object and a private lock. Regardless of which is chosen, subclasses must document their locking policy.

          1. David commented earlier that:

            There are no security issues with a non-sync method overriding a sync method, just usability issues (the dev might get confused).

            I'm not sure I don't also hold this opinion.

            You said earlier that:

            Proper documentation helps but doesn't ensure that source code is secure.

            Which also sounds right (if it did, they would be considered annotations and not documentation).

            My take is that if we are going to keep this, and I'm not positive we should, we have to be more forthcoming with the issues here.

            First, I view this guideline as not prohibiting a specific type of error prohibiting a pattern that is associated with errors.

            I think this rule needs a noncompliant solution that demonstrates the entire problem, more like the problem reported in the Sun Bug report. Of course, once you do that, it is likely that the noncompliant code example will violate some other guideline, which is why this guideline is questionable. What you might end up with is another NCE/CS pair to illustrate another security anti-pattern.

            We also need to explore the other cases.

            For example, is the following example (of something like it ;^) compliant or noncompliant:

            final class SyncFlag {
              private volatile boolean flag = true;
             
              public synchronized void toggle() { 
                flag ^= true; // Same as flag = !flag; 
              }
            
              public synchronized boolean getFlag() { 
                return flag;
              }
            }
            
            class NSyncFlag extends Base SyncFlag {
             
              public boolean getFlag() { 
                return flag;
              }
            }
            

            How about this example that mixes synchronized methods with synchronized blocks?

            class Base {
            
              public synchronized void doSomething() {
                // ...	
              }
            }
            
            class Derived extends Base {
            
              private final Object lock = new Object();
            
              public void doSomething() {
                synchronized (lock) {
                  // ...	
                }	
              }
            }
            

            Again, I'm not sure the patterns are as important as the the fact that the visibility or atomicity requirements are met.

            1. If you derive from a class Foo that is thread-safe (but your object is not thread-safe) and pass along the object to a class that expects a thread-safe Foo object, the class will happily accept the sub-type but will not warn that there is a problem. No explicit casts are required.

              Essentially the problem we describe is the same as the bug post, just that we have omitted the Thread1 and Thread2 classes for brevity and instead written:

              This problem is hard to notice because threads that accept instances of Base also accept instances of its subclasses. Consequently, the threads may incorrectly assume that the subclasses are thread-safe.

              What other guidelines does the NCE violate?

              Regarding other cases:

              First example: At one point this guideline had an exception that said: if your subclass method is atomic, you may violate this guideline by not synchronizing the method. Then I changed the title which made this exception unnecessary because such a method is thread-safe and thus exempt from this guideline. Coming to your example, synchronization is being used to ensure visibility so if the flag variable is non-private in the base class, the subclass would also need to ensure visibility by using synchronization. Note that this is required even though getFlag() is atomic. For this reason we can infer that getFlag() in the subclass is not thread-safe and should therefore use synchronization (verify with the title). (On a sidenote, your example also declares flag as volatile; if it does not synchronize getFlag() then the subclass is also not required to synchronize the method because it is thread-safe in the absence of visibility issues)

              Second example: I have covered this case as quoted earlier:

              However, if a class uses intrinsic synchronization over the class object (i.e. your example) without documenting its locking policy, subclasses may not use intrinsic synchronization over their own class object, unless they explicitly document their locking policy (they may use synchronized keyword only if they document that they are doing so). If the superclass documents its policy by stating that client-side locking is supported (again, your example), the subclasses have the option of choosing between intrinsic locking over the class object and a private lock (subclasses may either use the synchronized keyword OR private final locks). Regardless of which is chosen, subclasses must document their locking policy.

              So your second example is compliant with this guideline.

              1. I'd also like to point out that figuring out whether the subclass is thread-safe or not is not the responsibility of this guideline. Such information can be obtained using other guidelines and used to detect a violation of this one. In your second example, it can be easily determined that Derived is thread-safe.

                We urge people to document the synchronization policies when inheriting and not the absence thereof because it is not allowed by this guideline.

                EDIT: I have also been thinking that any class is allowed to violate the concurrency guidelines if they explicitly document so (for example, StringBuilder). Don't know where such an exception can go (perhaps the intro?). With this in mind, a subclass of a thread-safe class is not allowed to follow this exception.

              2. I think both examples are compliant. For the first example the getFlag() method in the base class and the derived class are both thread-safe.

                I guess you they way you would implement a checker for this rule is:

                for each derived class
                  find the base class
                  for each method in the base class 
                    if (thread-safe) then
                       if (overridden method is not thread-safe) then
                         issue diagnostic
                       end if;
                    end if;
                  end for;
                end for;
                
                1. Yes... Since that class won't compile here's an example of noncompliance:

                  class SyncFlag {
                    boolean flag = true;
                   
                   // ...
                  
                    public synchronized boolean getFlag() { 
                      return flag;
                    }
                  }
                  
                  class NSyncFlag extends SyncFlag {
                    public boolean getFlag() { 
                      return flag;
                    }
                  }
                  

                  Above is noncompliant because getFlag() in the subclass does not ensure visibility. If the variable were to be declared volatile then it would be compliant because the method is then thread-safe.

                  Agree with the checker.

  2. The Automated Detection section is not complete. It's marked TODO.

  3. For both "Noncompliant Code Example (Private Lock)" and "Compliant Solution (Private Lock)",
    class 'Derived' needs something like "Logger logger = Logger.getLogger(this.getClass().getName());" or "// ..." to clarify where logger is coming from.

    class Derived extends Base {
      @Override public void doSomething() {
        try {
          super.doSomething();
        } finally {
          logger.log(Level.FINE, "Did something");
        }
      }
    }
    
  4. I feel that the 2 compliant solutions using a "private lock" are dubious.  while one could argue that they are, in theory, thread safe, it would be very difficult to maintain any meaningful guarantees on the overall state of the object.  for instance, it would be almost impossible for the subclass to maintain some additional state which is "in sync" with the parent class's state because the subclass cannot control access to the parent class's other methods while updating itself.

    not to mention this the "private lock" solutions are also more deadlock prone.

    1. Both compliant solutions are safe as written. The problem comes when you start to fill out the other details of the Base and Derived classes. In particular what guarantees can you make for a publicly-accessible base class and derived class where both claim thread-safety? That is the source of your doubts.

      The private lock solutions are more deadlock-prone only in the trivial sense that code using more locks is more deadlock-prone than code using less locks. We have other rules addressing deadlock. The base and derived private locks cannot deadlock with each other.

      1. "correct as written", yes.  however, i was addressing the actual utility of the idea, not whether or not the trivial example is technically correct.  i would hope that would strive to provide compliant solutions which are actually viable in "real life" code bases.

        they could deadlock with each other depending on the rest of the implementation.  the examples acquire the private lock and then acquire the parent lock.  if another parent method call acquired the parent lock first and then invoked the overridden method you could easily get a deadlock (i don't think this violates any other rule).

        1. I agree, the code examples are theoretical models, and not realistic, given how much they are lack. I do wonder if its OK to have a derived class whose base class is both public and non-abstract, and both are thread-safe.

          You can have deadlock if you have a method that grabs the parent lock first and then tries to grab the child lock. It would violate our rule against deadlock: LCK07-J. Avoid deadlock by requesting and releasing locks in the same order. This code could do it:

          Derived d = new Derived();
          Thread t1 = new Thread(new Runnable() {
              public void run() {
                d.doSomething();
              }}).start();
          Thread t2 = new Thread(new Runnable() {
              public void run() {
                d.super.doSomething();    
                d.doSomething();
              }}).start();
          

          Perhaps such a scenario requires some better protection against deadlock. I'm not sure there's a realistic scenario that requires that both base & derived classes be publicly accessible this way....maybe we should require that the base class be private. Any suggestions?

          1. Actually, that example won't deadlock (and d.super.doSomething() is not legal java syntax).

            This implementation of Base would be required:

            class Base {
              private final Object lock = new Object();
             
              public void doSomething() {
                synchronized (lock) {
                  // ...
                }
              }
            
              public void doSomethingElse() {
                synchronized (lock) {
                  doSomething();
                }
              }
            }
            
            Derived d = new Derived();
            Thread t1 = new Thread(new Runnable() {
                public void run() {
                  d.doSomething();
                }}).start();
            Thread t2 = new Thread(new Runnable() {
                public void run() { 
                  d.doSomethingElse();
                }}).start();

            Obviously this violates the deadlock rule, but the Base class in isolation is not in violation and the Derived class cannot know if it is in violation without knowing the internals of the Base class.  That is why i think this compliant solution is "dubious".

            I think the only safe rule is that a base class with private synchronization should not allow itself to be overridden in a non-thread-safe way, i.e. no mutable internal state is exposed in a non-thread-safe way and any overridable methods cannot affect the overall thread-safety of the class.

            1. Sorry for being so slow to respond; it's been a busy few months.

              I'll agree that your code is a better example of deadlock than mine. I don't think it renders the compliant solution as 'dubious'. Or rather, it's only 'dubious' in that you can write bad code that complies with this rule. The CS is safe, but only if you don't implement other methods (unrealistic) or your methods don't violate other rules (eg your deadlock rule). This is admittedly a problem, but IMHO the problem is endemic to concurrency in general, not to this rule in particular. The fact that programming a thread-safe subclass requires intimate knowledge of the workings of the superclass is one of the many reasons why concurrency is 'hard'.

              I think the only safe rule is that a base class with private synchronization should not allow itself to be overridden in a non-thread-safe way, i.e. no mutable internal state is exposed in a non-thread-safe way and any overridable methods cannot affect the overall thread-safety of the class.

              That is a worthwhile guideline. I'm not (yet!) convinced it warrants a new rule in the CERT/Oracle Java standard.

              1. Ultimately, my main point wasn't about the possibility of deadlock, more about the difficulty in maintaining atomic guarantees when your class is effectively using separate internal locks (that was what my "dubious" statement was originally referring to).  i certainly agree with your assertion that concurrency is "hard".  especially when you don't have access to your parent class' lock.