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 |
Automated Detection
Sound automated detection is infeasible; heuristic checks could be useful.
| Tool | Version | Checker | Description |
|---|---|---|---|
| Parasoft Jtest | 2023.1 | CERT.TSM00.OSNS | Avoid overriding synchronized methods with non-synchronized methods |
Bibliography
21 Comments
David Svoboda
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.
Dhruv Mohindra
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).
David Svoboda
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.
Dhruv Mohindra
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.
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.
Robert Seacord
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() { } // 1and
public void foo() { synchronized(this) { } } // 2The 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
Dhruv Mohindra
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.
Robert Seacord
David commented earlier that:
I'm not sure I don't also hold this opinion.
You said earlier that:
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.
Dhruv Mohindra
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:
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 thatgetFlag()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:
So your second example is compliant with this guideline.
Dhruv Mohindra
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.
Robert Seacord (Manager)
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;Dhruv Mohindra
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.
Pennie Walters
The Automated Detection section is not complete. It's marked TODO.
Masaki Kubo
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
loggeris coming from.class Derived extends Base { @Override public void doSomething() { try { super.doSomething(); } finally { logger.log(Level.FINE, "Did something"); } } }David Svoboda
Fixed, thanks.
James Ahlborn
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.
David Svoboda
Both compliant solutions are safe as written. The problem comes when you start to fill out the other details of the
BaseandDerivedclasses. 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.
James Ahlborn
"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).
David Svoboda
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?
James Ahlborn
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.
David Svoboda
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'.
That is a worthwhile guideline. I'm not (yet!) convinced it warrants a new rule in the CERT/Oracle Java standard.
James Ahlborn
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.