Class methods that can be invoked from untrusted code to modify a {{static}} field must synchronize access to that field. This is necessary because there is no guarantee that untrusted clients will externally synchronize when accessing the field. Because a {{static}} field is shared by all clients, untrusted clients may violate the contract by failing to provide suitable locking.
According to Joshua Block \[[Bloch 08|AA. Java References#Bloch 08]\]:
{quote}
If a method modifies a static field, you must synchronize access to this field, even if the method is typically used only by a single thread. It is not possible for clients to perform external synchronization on such a method because there can be no guarantee that unrelated clients will do likewise.
{quote}
Protecting the fields of a class is unnecessary when it is designed for single-threaded use. Such classes are required to document their lack of thread-safety. For example, the Java Platform, Standard Edition 6 API Specification for the {{java.lang.StringBuilder}} class states \[[API 06|AA. Java References#API 06]\]:
{quote}
This class is designed for use as a drop-in replacement for {{StringBuffer}} in places where the string buffer was being used by a single thread (as is generally the case). Where possible, it is recommended that this class be used in preference to {{StringBuffer}} as it will be faster under most implementations.
{quote}
Multithreaded clients must synchronize accesses to classes whose documentation explicitly specifies the class is not thread-safe or fails to provide any assurances that the class is safe-thread.
h2. Noncompliant Code Example
This noncompliant code example does not synchronize access to the {{static}} field {{counter}}.
{code:bgColor=#FFCCCC}
/** This class is not thread-safe! */
public final class CountHits {
private static int counter;
public void incrementCounter() {
counter++;
}
}
{code}
{mc} This class is not thread-safe because it violates [CON01-J. Ensure that compound operations on shared variables are atomic]. {mc}
The class relies on clients to externally synchronize the object and this class documents its lack of thread-safety. This code is noncompliant, not because it violates [CON01-J. Ensure that compound operations on shared variables are atomic], and not because it documents its lack of thread-safety, but rather because it has an accessible mutable static field {{counter}}. Untrusted code is free to modify {{CountHits.counter}} with no respect for this class's documented lack of thread-safety.
h2. Compliant Solution
This compliant solution uses a static private final lock to protect the {{counter}} field and consequently, does not depend on any external synchronization. This is recommended by [CON04-J. Use private final lock objects to synchronize classes that may interact with untrusted code].
{code:bgColor=#ccccff}
/** This class is thread-safe */
public final class CountHits {
private static int counter;
private static final Object lock = new Object();
public void incrementCounter() {
synchronized (lock) {
counter++;
}
}
}
{code}
h2. Risk Assessment
Failing to protect classes containing accessible static members can result in unexpected results when a client fails to obey the classes' synchronization policy.
|| Rule || Severity || Likelihood || Remediation Cost || Priority || Level ||
| CON32- 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+CON32-J].
h2. References
\[[API 06|AA. Java References#API 06]\]
\[[Bloch 08|AA. Java References#Bloch 08]\] Item 67: "Avoid excessive synchronization"
h2. Issue Tracking
{tasklist:Review List}
||Completed||Priority||Locked||CreatedDate||CompletedDate||Assignee||Name||
|T|M|F|1270153006582|1270216402886|svoboda|and specifies its lack of thread-safety in the documentation. => rephrase to : and documents its lack of thread-safety. <---- FIXED ~DS|
|T|M|F|1270152984383|1270219726077|rcs|protecting fields is a weak word. Should be changed to synchronizing access to fields.|
|F|M|F|1270152995003| | |and not because it documents its lack of thread-safety; suggest removing this|
|F|M|F|1270153016807| | |The title can also be Ensure classes containing publicly accessible mutable static fields are thread-safe|
|F|M|F|1270215734760| |svoboda|"Classes that use {{static}} fields that are both publicly accessible and mutable must always protect accesses to their fields, *regardless of their documentation.*" => A class that is internally providing synchronization would not document that it is not thread-safe. So I don't understand why you added the words in bold.|
|F|M|F|1270215935982| |svoboda|"The class relies on clients to externally synchronize the object and *this class documents* its lack of thread-safety" => "this class" occurs twice in the sentence. Should be "and documents its lack of thread safety".|
|F|M|F|1270325608947| |dmohindr|"any assurances that the class is safe-thread." => safe-thread should be thread-safe!|
{tasklist}
----
[!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]
| 