"Client-side locking entails guarding client code that uses some object X with the lock X uses to guard its own state. In order to use client-side locking, you must know what lock X uses." \[[Goetz 06|AA. Java References#Goetz 06]\].
While client-side locking is acceptable if the thread-safe class commits to its locking strategy and clearly documents it, Goetz et al. \[[Goetz 06|AA. Java References#Goetz 06]\] caution against its misuse:
{quote}
IfExtension extendingis amore classfragile tothan addadding anothercode atomicdirectly operationto isa fragileclass, because itthe implementation distributesof the lockingsynchronization codepolicy foris anow classdistributed over multiple, classesseparately inmaintained ansource objectfiles. hierarchy, client-side locking is even more fragile because it entails putting locking code for class C into classes that are totally unrelated to C. Exercise care when using client-side locking on classes that do not commit to their locking strategyIf the underlying class were to change its synchronization policy by choosing a different lock to guard its state variables, the subclass would subtly and silently break, because it no longer used the right lock to control concurrent access to the base class's state.
{quote}
The documentation ofAlso:
{quote}
If extending a class thatto doesadd supportanother client-sideatomic lockingoperation shouldis explicitlyfragile state its applicability. An example of when not to usebecause it distributes the locking code for a class over multiple classes in an object hierarchy, client-side locking is theeven class {{java.util.concurrent.ConcurrentHashMap<K,V>}}, whose documentation states \[[API 06|AA. Java References#API 06]\]: {mc} s/not// right? ~DS {mc}
{quote}
... even though all operations are thread-safe, retrieval operations do not entail locking, and there is not any support for locking the entire table in a way that prevents all access. This class is fully interoperable with Hashtable in programs that rely on its thread safety but not on its synchronization details.
{quote}
In general, more fragile because it entails putting locking code for class C into classes that are totally unrelated to C. Exercise care when using client-side locking on classes that do not commit to their locking strategy.
{quote}
The documentation of a class that does support client-side locking should explicitly state its applicability. An example of when not to use client-side locking onlyis when the documentation of the class recommends it. For example, the documentation of the wrapper method {{synchronizedList()}} of class {{class {{java.util.concurrent.CollectionsConcurrentHashMap<K,V>}}, whose documentation states \[[API 06|AA. Java References#API 06]\]: states:
{{mc} s/not// right? ~DS {mc}
{quote}
In order to guarantee serial access, it is critical that all access to the backing list is accomplished through the returned list. It is imperative that the user manually synchronize on the returned list when iterating over it. Failure to follow this advice may result in non-deterministic behavior.
{quote}
Note that the advice suggested above is compliant with [CON40-J. Do not synchronize on a collection view if the backing collection is accessible] when the backing list is inaccessible from a caller that can potentially inflict some harm.
h2. Noncompliant Code Example
This noncompliant code example uses a thread-safe class {{Book}} that cannot be refactored. This could happen, for example, when the source code is not available for review or the class is part of a general library that cannot be extended.
{code}
final class Book {
// May change its locking policy in the future to use private internal locks
private final String title;
private Calendar dateIssued;
private Calendar dateDue;
Book(String title) {
this.title = title;
}
public synchronized void issue(int days) {
dateIssued = Calendar.getInstance();
dateDue = Calendar.getInstance();
dateDue.add(dateIssued.DATE, days);
}
public synchronized Calendar getDueDate() {
return dateDue;
}
}
{code}
This class does not commit to its locking strategy. That is, it reserves the right to change its locking strategy without notice. Furthermore, it does not document that callers can safely use client-side locking. The client class {{BookWrapper}} uses client-side locking in the {{renew()}} method by synchronizing on a {{Book}} instance.
{code:bgColor=#FFCCCC}
// Client
public class BookWrapper... even though all operations are thread-safe, retrieval operations do not entail locking, and there is not any support for locking the entire table in a way that prevents all access. This class is fully interoperable with Hashtable in programs that rely on its thread safety but not on its synchronization details.
{quote}
In general, use client-side locking only when the documentation of the class recommends it. For example, the documentation of the wrapper method {{synchronizedList()}} of class {{java.util.Collections}} \[[API 06|AA. Java References#API 06]\] states:
{quote}
In order to guarantee serial access, it is critical that all access to the backing list is accomplished through the returned list. It is imperative that the user manually synchronize on the returned list when iterating over it. Failure to follow this advice may result in non-deterministic behavior.
{quote}
Note that the advice suggested above is compliant with [CON40-J. Do not synchronize on a collection view if the backing collection is accessible] when the backing list is inaccessible from a caller that can potentially inflict some harm.
h2. Noncompliant Code Example (intrinsic lock)
This noncompliant code example uses a thread-safe class {{Book}} that cannot be refactored. This could happen, for example, when the source code is not available for review or the class is part of a general library that cannot be extended.
{code}
final class Book {
// May change its locking policy in the future to use private internal locks
private final String title;
private Calendar dateIssued;
private Calendar dateDue;
Book(String title) {
this.title = title;
}
public synchronized void issue(int days) {
dateIssued = Calendar.getInstance();
dateDue = Calendar.getInstance();
dateDue.add(dateIssued.DATE, days);
}
public synchronized Calendar getDueDate() {
return dateDue;
}
}
{code}
This class does not commit to its locking strategy. That is, it reserves the right to change its locking strategy without notice. Furthermore, it does not document that callers can safely use client-side locking. The client class {{BookWrapper}} uses client-side locking in the {{renew()}} method by synchronizing on a {{Book}} instance.
{code:bgColor=#FFCCCC}
// Client
public class BookWrapper {
private final Book book;
BookWrapper(Book book) {
this.book = book;
}
public void issue(int days) {
book.issue();
}
public Calendar getDueDate() {
return book.getDueDate();
}
public void renew() {
synchronized(book) {
if (book.getDueDate().after(Calendar.getInstance())) {
throw new IllegalStateException("Book overdue");
} else {
book.issue(14); // Issue book for 14 days
}
}
}
}
{code}
If class {{Book}} changes its synchronization policy in the future, the {{BookWrapper}} class's locking strategy might silently break. For instance, the {{Bookwrapper}} class's locking strategy will definitely break if {{Book}} is modified to use an internal private lock, as recommended by [CON04-J. Use private final lock objects to synchronize classes that may interact with untrusted code]. This is because threads that call {{getDueDate()}} of class {{BookWrapper}} may perform operations on the thread-safe {{Book}} using its new locking policy, however, threads that call method {{renew()}} will always synchronize on the intrinsic lock of the {{Book}} instance. Consequently, the implementation will use two different locks.
h2. Compliant Solution (intrinsic lock)
This compliant solution uses an internal private lock object and synchronizes all its methods using this lock.
{code:bgColor=#ccccff}
public final class BookWrapper {
private final Book book;
private final Object lock = new Object();
BookWrapper(Book book) {
this.book = book;
}
public void issue(int days) {
synchronized(lock) {
book.issue();
}
}
public Calendar getDueDate() {
synchronized(lock) {
return book.getDueDate();
}
}
public void renew() {
synchronized(lock) {
if (book.getDueDate().after(Calendar.getInstance())) {
throw new IllegalStateException("Book overdue");
} else {
book.issue(14); // Issue book for 14 days
}
}
}
}
{code}
Consequently, the {{BookWrapper}} class's locking strategy is independent of the locking policy of the {{Book}} instance. This solution incurs a very small performance penalty but the resulting code is much more robust \[[Goetz 06|AA. Java References#Goetz 06]\].
h2. Noncompliant Code Example (accessible internal lock)
In this noncompliant code example, the class {{PrintableIPAddressList}} extends the thread-safe class {{IPAddressList}}. It locks on {{IpAddressList}}'s member {{ips}} in the method {{addAndPrintIPAddresses()}}. This is another example of client-side locking, because a subclass locks on an object owned and locked by its superclass.
{code:bgColor=#FFCCCC}
// This class may change its locking policy in the future, for example,
// when new non-atomic methods are added
class IPAddressList {
private final Book book;
BookWrapper(Book book) {
this.book = bookList<InetAddress> ips = Collections.synchronizedList(new ArrayList<InetAddress>());
}
public voidList<InetAddress> issuegetList(int days) {
book.issue(); return ips; // No defensive copies required as package-private visibility
}
public Calendarvoid getDueDateaddIPAddress(InetAddress address) {
return bookips.getDueDateadd(address);
}
}
class publicPrintableIPAddressList voidextends renew()IPAddressList {
public void synchronizedaddAndPrintIPAddresses(bookInetAddress address) {
if (book.getDueDate().after(Calendar.getInstance())synchronized(getList()) {
throw new IllegalStateException("Book overdue"addIPAddress(address);
} else {
book.issue(14); // Issue book for 14 daysInetAddress[] ia = (InetAddress[]) getList().toArray(new InetAddress[0]);
}// ...
}
}
}
{code}
If the class {{BookIPAddressList}} changes its synchronization policy in the future, the {{BookWrapper}} class's locking strategy might silently break. For instance, the {{Bookwrapper}} class's locking strategy will definitely break if {{Book}} is modified to use an internal private lock, as recommended by [CON04-J. Use private final lock objects to synchronize classes that may interact with untrusted code]. This is because threads that call {{getDueDate()}} of class {{BookWrapper}} may perform operations on the thread-safe {{Book}} using its new locking policy, however, threads that call method {{renew()}} will always synchronize on the intrinsic lock of the {{Book}} instance. Consequently, the implementation will use two different locks.
h2. Compliant Solution (composition) is modified to use block synchronization on a private final lock object, as recommended by [CON04-J. Use private final lock objects to synchronize classes that may interact with untrusted code], the subclass {{PrintableIPAddressList}} will silently break. Moreover, when a wrapper such as {{Collections.synchronizedList()}} is used, it is unwieldy for a client to determine the type of the class ({{List}}) that is being wrapped to extend it \[[Goetz 06|AA. Java References#Goetz 06]\].
h2. Compliant Solution (accessible internal lock)
Composition offers encapsulation benefits at the cost of performance. The performance impact is usually minimal. Refer to [OBJ07-J. Understand how a superclass can affect a subclass] for more information on implementing composition.
This compliant solution useswraps an internal private lock object andof synchronizes all its methods using this lock.
{code:bgColor=#ccccff}
public final class BookWrapper {
private final Book book;
private final Object lock = new Object();
BookWrapper(Book book) {
this.book = book;
}
public void issue(int days) {
synchronized(lockclass {{IPAddressList}} and provides synchronized accessors that can be used to manipulate the state of the object.
{code:bgColor=#ccccff}
// Class IPAddressList remains unchanged
class PrintableIPAddressList {
private final IPAddressList ips;
public PrintableIPAddressList(IPAddressList list) {
this.ips = book.issue()list;
}
}
public synchronized Calendarvoid getDueDateaddIPAddress() {
synchronized(lock) {
InetAddress address) {
return bookips.getDueDateaddIPAddress(address);
}
}
public synchronized void renewaddAndPrintIPAddresses(InetAddress address) {
synchronizedaddIPAddress(lockaddress); {
InetAddress[] ia if= (book.getDueDateInetAddress[]) ips.getList().after(Calendar.getInstance())) {
toArray(new InetAddress[0]);
throw new IllegalStateException("Book overdue");
} else {
book.issue(14); // Issue book for 14 days
}
}
}
}
{code}
Consequently, the {{BookWrapper}} class's locking strategy is independent of the locking policy of the {{Book}} instance. This solution incurs a very small performance penalty but the resulting code is much more robust// ...
}
}
{code}
This approach allows the {{CompositeCollection}} class to use its own intrinsic lock in a way that is completely independent of the lock of the underlying list class. This allows the underlying collection to be not thread-safe because the {{CompositeCollection}} wrapper prevents direct access to its methods by publishing its own synchronized equivalents. This approach provides consistent locking even if the underlying class changes its locking policy in the future. \[[Goetz 06|AA. Java References#Goetz 06]\].
h2. Risk Assessment
Using client-side locking when the thread-safe class does not commit to its locking strategy can cause data inconsistencies and deadlock.
|| Rule || Severity || Likelihood || Remediation Cost || Priority || Level ||
| CON31- J | low | probable | medium | {color:green}{*}P4{*}{color} | {color:green}{*}L3{*}{color} |
h3. Automated Detection
TODO
h3. Related Vulnerabilities
Search for vulnerabilities resulting from the violation of this rule on the [CERT website|https://www.kb.cert.org/vulnotes/bymetric?searchview&query=FIELD+KEYWORDS+contains+CON38-J].
h2. References
\[[API 06|AA. Java References#API 06]\] Class Vector, Class WeakReference, Class ConcurrentHashMap<K,V>
\[[JavaThreads 04|AA. Java References#JavaThreads 04]\] 8.2 "Synchronization and Collection Classes"
\[[Goetz 06|AA. Java References#Goetz 06]\] 4.4.1. Client-side Locking, 4.4.2. Composition and 5.2.1. ConcurrentHashMap
\[[Lee 09|AA. Java References#Lee 09]\] "Map & Compound Operation"
----
[!The CERT Sun Microsystems Secure Coding Standard for Java^button_arrow_left.png!|VOID CON06-J. Do not defer a thread that is holding a lock] [!The CERT Sun Microsystems Secure Coding Standard for Java^button_arrow_up.png!|11. Concurrency (CON)] [!The CERT Sun Microsystems Secure Coding Standard for Java^button_arrow_right.png!|CON08-J. Do not call alien methods that synchronize on the same objects as any callers in the execution chain]
|
